feat: Revise createFile logic to return modified filenames and location #242
Conversation
Thanks for opening this pull request! |
AdrianCurtin
changed the title
AdrianCurtin
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #242 +/- ##
==========================================
+ Coverage 97.19% 97.40% +0.20%
==========================================
Files 2 2
Lines 214 231 +17
==========================================
+ Hits 208 225 +17
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mtrezza Which tests are failing? |
|
See the CI job panel |
|
@mtrezza Added tests for the two conditions that were missing |
|
@mtrezza |
|
@vahidalizad What do you think? |
|
@mtrezza I think this is a good one! I had some minor change suggestions, but other than that, it looks great. I also reviewed the code in the Parse Server PR, which is necessary for this to work, and I think that looks good as well. |
|
Great! Are you able to post your change suggestion here using the Review feature on GitHub? Or do you still require permissions for that? |
Yes, I created a review successfully! |
|
Are you able to post the review in the "Files changed" tab? It should generate a GitHub comment here with your review feedback. |
index.js
Outdated
|
|
||
| let key_without_prefix = filename; | ||
| if (this._generateKey instanceof Function) { | ||
| try { |
There was a problem hiding this comment.
I believe removing the try block would achieve the same result while preserving the original error trace and improving readability.
index.js
Outdated
| return { | ||
| location: location, // actual upload location, used for tests | ||
| name: key_without_prefix, // filename in storage, consistent with other adapters | ||
| s3_response: response, // raw s3 response |
There was a problem hiding this comment.
I think we don't need the s3_response key and we can remove it.
index.js
Outdated
|
|
||
| return Object.assign(response || {}, { Location: location }); | ||
| let url; | ||
| if (Object.keys(config).length != 0) { // if config is passed, we can generate a presigned url here |
There was a problem hiding this comment.
Instead of checking only for the presence of keys, let's ensure we are checking for the correct keys.
Maybe change to:
if (config?.mount && config?.applicationId)
|
@AdrianCurtin just a friendly ping, did you get a chance to review the comments? |
- Add error handling for key generation - Standardize return object with location, url, and filename - Add support for optional config parameter - Return s3 response explicitly as a separate variable
📝 WalkthroughPre-merge checks (5 passed)✅ Passed checks (5 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
|
Revised! and thanks for your assistance! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
index.js (2)
192-197: Reconsider s3_response field (API surface and naming consistency)
- Exposing the raw AWS response increases the public API surface with little consumer value and couples clients to SDK internals. Prior feedback suggested removing it.
- If you keep it, prefer camelCase (s3Response) to match the file’s conventions.
Option A — remove it:
return { location: location, // actual upload location, used for tests name: key_without_prefix, // filename in storage, consistent with other adapters - s3_response: response, // raw s3 response ...url ? { url: url } : {} // url (optionally presigned) or non-direct access url };Option B — rename to camelCase:
return { location: location, // actual upload location, used for tests name: key_without_prefix, // filename in storage, consistent with other adapters - s3_response: response, // raw s3 response + s3Response: response, // raw s3 response ...url ? { url: url } : {} // url (optionally presigned) or non-direct access url };Note: If you choose Option B, please update the tests accordingly.
187-191: Only call getFileLocation when inputs are sufficient to avoid malformed URLsIf directAccess is false, calling getFileLocation with an arbitrary non-empty config can produce URLs with “undefined” segments. Gate this on the presence of required keys, or allow unconditionally only when directAccess is true. This aligns with the prior suggestion to check for config?.mount and config?.applicationId.
Apply this diff:
- let url; - if (config && typeof config === 'object' && Object.keys(config).length > 0) { // if config is passed, we can generate a presigned url here - url = await this.getFileLocation(config, key_without_prefix); - } + let url; + const shouldResolveUrl = + config && typeof config === 'object' && + ( + this._directAccess // baseUrl flow doesn't require mount/applicationId + || (config?.mount && config?.applicationId) // needed when serving via parse-server + ); + if (shouldResolveUrl) { + url = await this.getFileLocation(config, key_without_prefix); + }
🧹 Nitpick comments (1)
spec/test.spec.js (1)
871-903: Strengthen test by asserting getFileLocation was called with the expected argsThis ensures createFile passes the final key (without prefix) and the provided config to getFileLocation.
Apply this diff inside the test after obtaining result:
const result = await s3.createFile( 'file.txt', 'hello world', 'text/utf8', {}, { mount: 'http://example.com', applicationId: 'test123' } ); expect(result).toEqual({ location: jasmine.any(String), name: 'file.txt', s3_response: jasmine.any(Object), url: 'https://presigned-url.com/file.txt' }); + expect(s3.getFileLocation).toHaveBeenCalledWith( + { mount: 'http://example.com', applicationId: 'test123' }, + 'file.txt' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.js(2 hunks)spec/test.spec.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/test.spec.js (3)
lib/optionsFromArguments.js (1)
options(35-35)spec/mocks/s3adapter.js (2)
s3(11-15)S3Adapter(2-2)spec/support/server.js (1)
S3Adapter(8-8)
🔇 Additional comments (3)
spec/test.spec.js (3)
732-734: LGTM: asserts use new return shape (location)Switching to value.location is correct with the updated adapter response.
743-745: LGTM: asserts use new return shape (location)Consistent with the new API.
753-756: LGTM: asserts use new return shape (location)Consistent with the updated adapter response.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
index.js (1)
143-152: Async-safe generateKey handling looks good; consider rejecting whitespace-only keysThe async sync/await handling and type check are solid. One nit: whitespace-only strings currently pass; trim before validating to avoid uploading a key that is effectively empty.
Apply this diff:
- if (typeof key_without_prefix !== 'string' || key_without_prefix.length === 0) { + if (typeof key_without_prefix !== 'string' || key_without_prefix.trim().length === 0) { throw new Error('generateKey must return a non-empty string'); } + key_without_prefix = key_without_prefix.trim();spec/test.spec.js (4)
871-902: Strengthen assertions: verify getFileLocation call args and gatingTo ensure the URL path is exercised as intended, assert getFileLocation receives the expected config and key, and that the result includes url only when config is present.
Apply this diff:
// Mock getFileLocation to return a presigned URL - spyOn(s3, 'getFileLocation').and.returnValue(Promise.resolve('https://presigned-url.com/file.txt')); + spyOn(s3, 'getFileLocation').and.returnValue(Promise.resolve('https://presigned-url.com/file.txt')); @@ const result = await s3.createFile( 'file.txt', 'hello world', 'text/utf8', {}, { mount: 'http://example.com', applicationId: 'test123' } ); + expect(s3.getFileLocation).toHaveBeenCalledWith( + jasmine.objectContaining({ mount: 'http://example.com', applicationId: 'test123' }), + 'file.txt' + ); expect(result).toEqual({ location: jasmine.any(String), name: 'file.txt', url: 'https://presigned-url.com/file.txt' });
919-939: Add an assertion for the returnednameYou already assert the PutObjectCommand Key; also check the returned name matches the generated key.
Apply this diff:
- await s3.createFile('file.txt', 'hello world', 'text/utf8', {}); + const out = await s3.createFile('file.txt', 'hello world', 'text/utf8', {}); @@ - expect(commandArg.input.Key).toBe('async-file.txt'); + expect(commandArg.input.Key).toBe('async-file.txt'); + expect(out.name).toBe('async-file.txt');
941-959: Mirror thenameassertion for Promise-based generateKeySame as the async function case, assert the returned name.
Apply this diff:
- await s3.createFile('file.txt', 'hello world', 'text/utf8', {}); + const out = await s3.createFile('file.txt', 'hello world', 'text/utf8', {}); @@ - expect(commandArg.input.Key).toBe('promise-file.txt'); + expect(commandArg.input.Key).toBe('promise-file.txt'); + expect(out.name).toBe('promise-file.txt');
961-999: Add a test for whitespace-only return from generateKeyThe current validation would accept ' ' without trimming. Either implement trim in the adapter (preferred) or add a test to capture the intended rejection.
Proposed test (add near the related generateKey validation tests):
+ it('should reject when generateKey returns only whitespace', async () => { + const options = { + bucket: 'bucket-1', + generateKey: () => ' ' + }; + const s3 = new S3Adapter(options); + s3._s3Client = s3ClientMock; + + await expectAsync( + s3.createFile('file.txt', 'hello world', 'text/utf8', {}) + ).toBeRejectedWithError('generateKey must return a non-empty string'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.js(2 hunks)spec/test.spec.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/test.spec.js (3)
lib/optionsFromArguments.js (1)
options(35-35)spec/mocks/s3adapter.js (2)
s3(11-15)S3Adapter(2-2)spec/support/server.js (1)
S3Adapter(8-8)
🔇 Additional comments (3)
index.js (2)
191-194: Good: getFileLocation is gated by required config keysGating on config?.mount and config?.applicationId prevents unnecessary calls and aligns with parse-server expectations.
196-201: Return shape is clean and forward-compatibleReturning { location, name, url? } improves clarity and avoids leaking S3 internals. Matches other adapters’ semantics.
spec/test.spec.js (1)
732-735: Tests correctly updated to consumevalue.locationSwitching from value.Location to value.location matches the new return shape and keeps the assertions focused on the path component.
Also applies to: 742-746, 752-757
More tests and changes to make coderabbit happy
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
index.js (1)
211-214: Guarding getFileLocation with required config keys is correctMatches earlier review feedback to only call when both mount and applicationId are present.
🧹 Nitpick comments (5)
index.js (3)
143-154: Nit: rename key_without_prefix to camelCase for consistencyMost variables in this file use camelCase (e.g., fileName, locationBase). Renaming improves readability and consistency.
- async createFile(filename, data, contentType, options = {}, config = {}) { - let key_without_prefix = filename; + async createFile(filename, data, contentType, options = {}, config = {}) { + let keyWithoutPrefix = filename; if (typeof this._generateKey === 'function') { const candidate = this._generateKey(filename, contentType, options); - key_without_prefix = + keyWithoutPrefix = candidate && typeof candidate.then === 'function' ? await candidate : candidate; - if (typeof key_without_prefix !== 'string' || key_without_prefix.trim().length === 0) { + if (typeof keyWithoutPrefix !== 'string' || keyWithoutPrefix.trim().length === 0) { throw new Error('generateKey must return a non-empty string'); } - key_without_prefix = key_without_prefix.trim(); + keyWithoutPrefix = keyWithoutPrefix.trim(); } const params = { Bucket: this._bucket, - Key: this._bucketPrefix + key_without_prefix, + Key: this._bucketPrefix + keyWithoutPrefix, Body: data, }; @@ - let url; - if (config?.mount && config?.applicationId) { // if config has required properties for getFileLocation - url = await this.getFileLocation(config, key_without_prefix); - } + let url; + if (config?.mount && config?.applicationId) { // if config has required properties for getFileLocation + url = await this.getFileLocation(config, keyWithoutPrefix); + } @@ - return { - location: location, // actual upload location, used for tests - name: key_without_prefix, // filename in storage, consistent with other adapters - ...url ? { url: url } : {} // url (optionally presigned) or non-direct access url - }; + return { + location: location, // actual upload location, used for tests + name: keyWithoutPrefix, // filename in storage, consistent with other adapters + ...url ? { url: url } : {} // url (optionally presigned) or non-direct access url + };Also applies to: 157-157, 211-214, 216-220
188-209: Robust custom endpoint URL construction; consider a couple more edge cases in testsThis correctly handles path-style endpoints, virtual-hosted style, and endpoints with query strings. To further harden behavior, consider adding tests for:
- Endpoints with a trailing slash (e.g., https://minio.example.com/)
- IPv6-literal host endpoints (e.g., http://[::1]:9000) if you intend to support MinIO-style local setups
I can draft the tests if useful.
216-220: Optional: always include the url field for a stable return shapeIf you prefer a consistent shape, you could always include url (undefined when not available). This avoids consumers having to check field presence.
- return { - location: location, // actual upload location, used for tests - name: key_without_prefix, // filename in storage, consistent with other adapters - ...url ? { url: url } : {} // url (optionally presigned) or non-direct access url - }; + return { + location, + name: keyWithoutPrefix, + url: url ?? undefined + };spec/test.spec.js (2)
871-906: Good positive test for url with config; add a negative case to assert omissionConsider adding a companion test to assert that when config is missing required keys (e.g., only mount or only applicationId, or empty object), url is omitted and getFileLocation is not called.
Example test to add:
it('should not include url when config is missing required keys', async () => { const s3 = new S3Adapter({ bucket: 'bucket-1', presignedUrl: true }); s3._s3Client = s3ClientMock; const spy = spyOn(s3, 'getFileLocation').and.callThrough(); const result1 = await s3.createFile('file.txt', 'hello', 'text/plain', {}, {}); expect(spy).not.toHaveBeenCalled(); expect(result1.url).toBeUndefined(); const result2 = await s3.createFile('file.txt', 'hello', 'text/plain', {}, { mount: 'http://x' }); expect(spy).not.toHaveBeenCalled(); expect(result2.url).toBeUndefined(); const result3 = await s3.createFile('file.txt', 'hello', 'text/plain', {}, { applicationId: 'a' }); expect(spy).not.toHaveBeenCalled(); expect(result3.url).toBeUndefined(); });
1020-1100: LGTM: endpoint URL construction tests; add trailing-slash variantGreat coverage for query/path, path-style, virtual-hosted-style, malformed, and default AWS. Consider adding a trailing-slash endpoint to validate normalization:
Example test to add:
it('should handle endpoint with trailing slash', async () => { const s3 = new S3Adapter({ bucket: 'test-bucket', s3overrides: { endpoint: 'https://minio.example.com/' } }); s3._s3Client = s3ClientMock; const result = await s3.createFile('test.txt', 'hello world', 'text/utf8'); expect(result.location).toBe('https://minio.example.com/test-bucket/test.txt'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.js(2 hunks)spec/test.spec.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/test.spec.js (2)
spec/mocks/s3adapter.js (2)
s3(11-15)S3Adapter(2-2)spec/support/server.js (1)
S3Adapter(8-8)
🔇 Additional comments (8)
index.js (1)
143-154: Async generateKey support and validation look solidThis correctly awaits async/thenable generateKey results and enforces a non-empty string return, preventing subtle S3 key bugs.
spec/test.spec.js (7)
732-734: LGTM: assertions updated to new return shape (value.location)Using the WHATWG URL parser against value.location is appropriate.
743-745: LGTM: verification against bucketPrefix via value.locationThis correctly reflects the new createFile return contract.
753-756: LGTM: path assertions adapted to value.locationGood coverage for prefixed path and filename placement.
908-922: LGTM: generateKey error propagation testThis verifies that thrown errors from generateKey reject createFile with the same error.
923-946: LGTM: async generateKey is awaited; return shape and Key verifiedGood checks for both S3 command input.Key and returned name.
946-965: LGTM: Promise-based generateKey coveredCovers thenables distinct from async functions.
967-1017: LGTM: comprehensive validation of generateKey return typesCovers empty string, number, whitespace-only, and async null.
|
Thanks @AdrianCurtin, this looks good to me. |
|
Just fixed the one trailing space |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
index.js (1)
211-214: Conditional getFileLocation check matches earlier review suggestionThe explicit check for config.mount and config.applicationId is the right gate to avoid a second call from server-side code.
🧹 Nitpick comments (2)
index.js (2)
190-209: Robust endpoint handling — suggest URL-encoding the key pathNice improvement parsing custom endpoints and handling path- vs virtual-hosted-style, including malformed endpoint fallback. One polish: encode params.Key when embedding into a URL so keys with spaces, unicode, or reserved characters produce a valid link.
Apply this diff:
- const location = `${locationBase}/${params.Key}`; + const safeKeyPath = String(params.Key) + .split('/') + .map(encodeURIComponent) + .join('/'); + const location = `${locationBase}/${safeKeyPath}`;Optional follow-up tests:
- Keys with spaces (e.g., "my file.png")
- Keys with unicode (e.g., "fílé–名.png")
- Keys with subpaths ("folder/a b/c+d.png")
155-159: Normalize S3 key construction to prevent double-slashesBucket prefixes default to
'', so they’ll never beundefined, but user-provided prefixes may include leading or trailing slashes, and keys may start with/, leading tofoo//baror/bazobject keys. Normalize both segments and join with exactly one slash.• Apply this to all S3 operations (e.g., createFile, getFile, deleteFile):
- index.js:155–159
- index.js:224–227
- index.js:237–241
- index.js:290–294
• Optional: extract into a helper, e.g.
function joinS3Key(prefix, key) { … }, to dedupe.Revised snippet at 155–159:
- const params = { - Bucket: this._bucket, - Key: this._bucketPrefix + key_without_prefix, - Body: data, - }; + // Normalize prefix and key to avoid double-slashes + const prefix = String(this._bucketPrefix || '').replace(/^\/+|\/+$/g, ''); + const keyOnly = String(key_without_prefix).replace(/^\/+/, ''); + const params = { + Bucket: this._bucket, + Key: prefix ? `${prefix}/${keyOnly}` : keyOnly, + Body: data, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
index.js(2 hunks)
🔇 Additional comments (2)
index.js (2)
143-153: Async-safe generateKey with validation looks solidGood handling of sync/async generateKey, trimming, and non-empty string validation. This closes the class of bugs where a Promise or falsy/whitespace would slip into Key construction.
216-220: No internal.Locationreferences remain
The new{ location, name, url? }shape is in place and no code is still accessing the old S3 response’sLocationproperty. This is still a breaking change for consumers, so please ensure your docs reflect it:
locationnow replaces S3’sLocationfieldnameis the object key stripped of any prefixurlis only included when presigned or direct‐access logic applies
| const pathWithBucket = hasBucketInHostOrPath ? basePath : `${basePath}/${this._bucket}`; | ||
| locationBase = `${origin}${pathWithBucket}`; |
There was a problem hiding this comment.
question: is it valid to redirect a "domain" bucket to a "path" bucket ? not sure all S3 providers support this switch
We need to be sure that current system works with all S3 providers like DO Spaces, Google Storage with S3, Minio S3 ...
Did you test locally with this kind of providers to ensure S3 protocol compatibility ?
There was a problem hiding this comment.
I think we only need to test with S3, which is the spec others should follow when claiming S3 compatibility. However, it must be explicitly documented in the S3 specs, otherwise we may use a behavior that is not properly defined even for S3. If in doubt, ask AWS support.
| const u = new URL(this._endpoint); | ||
| const origin = `${u.protocol}//${u.host}`; | ||
| const basePath = (u.pathname || '').replace(/\/$/, ''); | ||
| const hasBucketInHostOrPath = | ||
| u.hostname.startsWith(`${this._bucket}.`) || |
There was a problem hiding this comment.
note: i can suggestion to add comments with an url example to show at each line what you replace and why, it will help for the maintenance in the long term
If parse-community/parse-server#9557 is merged,
this will address #237
This change will affect specific tests that check the response.Location argument and may affect any direct adapter usage of createFile that explicitly uses the s3 server response.
Closes: #237
Summary by CodeRabbit
New Features
Tests