feat: Allow File adapter to create file with specific locations or dynamic filenames#9557
feat: Allow File adapter to create file with specific locations or dynamic filenames#9557AdrianCurtin wants to merge 17 commits intoparse-community:alphafrom
Conversation
Thanks for opening this pull request! |
AdrianCurtin
changed the title
AdrianCurtin
changed the title
mtrezza
left a comment
mtrezza
left a comment
There was a problem hiding this comment.
Could you add a test for this?
|
@mtrezza |
|
@mtrezza |
|
@AdrianCurtin Thank you for picking this up again, I've restarted the CI, let's see... |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extends the ChangesFile Adapter Configuration Extension
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
In order to get this to pass the CI checks, I limited the server config access to just app id, mount and the legacy filekey, since that's what it looks like all of the current file adapters are using. If something else is required we could add it or weaken the tests so that a less precise config could be checked. lmk what you think |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9557 +/- ##
==========================================
- Coverage 92.99% 92.57% -0.43%
==========================================
Files 187 187
Lines 15096 15097 +1
Branches 174 174
==========================================
- Hits 14039 13976 -63
- Misses 1045 1105 +60
- Partials 12 16 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Moumouls So really calling getFileLocation here is just a thing that would happen if the url wasn't provided by the createFile itself (current behavior). As long as getFileLocation respects the public url it should still work as expected. As implemented, a user who does not use some argument (like generate key) to modify the filename would ideally not see any difference in behavior and the createdFile.url || getFileLocation fallback would simply prevent getFileLocation from being called twice when it already gave you an answer once. (getFileLocation function always being accessible within the file adapter itself, but requiring the config to be called). On the other hand, if an adapter createFile opted to return a url that was different than the call from getFileLocation (for instance hypothetically a resolved ip address or something) then this would break the functionality you mentioned. But if they instead opted to not return anything or call getFileLocation internally, then there would be no change in behavior. I delegated this to the file adapter in the case the getFileLocation might be pre-computed or some other variation and prevent some redundant processing, or some file adapter that might need to modify the filename based on the result of getFileLocation (for instance an invalid character) or something. (but I didn't do any of this in the s3 adapter pr, it just calls getFileLocation so that create file directly returns whatever it was supposed to. |
|
So to summarize, if developer use generateKey and public server url, the system give priority to created URL not the public one. question: does the current generateKey usage with public server url and parse file proxy activated behave this way ? it could be interesting to cover this, and check if we have a related bug, or to unsure we don't break existing parse server using generateKey with parse proxy, but seems weird since your current PR here and on S3 actually fix the implementation of generateKey which is currently broken if i understand correctly |
|
generate_key i think is a specific s3 adapter functionality and the preserve filenames also must be enabled too, so its a double-opt-in there to even begin to play with the filenames. Frankly i don't know if the parse file proxy works at all with S3 adapter (i have never tried). But in theory if it worked, generate key should have nothing to do with the parse file proxy since it's exclusively modifying the filename (and potentially s3 path by extension). The generate key does not play with the s3 root itself. But lets say that the internal call in the s3 adapter was not to url = await getFileLocation, but instead to url = await getResolvedFileLocationNoProxy() Then in this hypothetical example the url's would not match because getFileLocation and getResolvedFileLocationNoProxy would produce different results. So the only way to break parse proxy behavior is to either a) use a getFileLocation that does not respect parse proxy behavior in the first place, or b) use a novel getFileLocation internally or method that does not respect the parse proxy behavior. But option a) would obviously break that function anyways and b) might be more particular. Maybe a note for future developers or a test should basically say that createFileresult.url should match getFileLocation unless there is a specific requirement for them to differ. We could use an additional test for this, but that actually could be a valid behavior? Consider a situation where you want the file to be accessible once and never again, createFileResult.url could be an accessor returned by file creation and getFileLocation for generic file access. In either case this is an opt-in behavior by the developer of the adapter, opt-out should simply return the file location. |
|
Maybe a simpler answer would be that yes it gives preference to the created url, but in most cases this is expected to be the same as the public server url. Generate key should have nothing to do with it. The developer of the adapter would need to intentionally return a different created url than would be expected. |
|
okay @mtrezza seems good to me may be we need another final approval from another contributor @dplewis ? to move forward ? Thanks @AdrianCurtin for your detailed answers and the time spent into this PR ! |
|
Could you please look at the open conversations and close the ones that are done? Would also be good to have another AI agent look over the changes and potential implications. |
|
@Moumouls I think most of the conversations here are resolved. Anything I got back for this part from AI agents was mostly concerning passing the full config for the server var into the file adapter. Which could contain information if the file adapter wasn't privileged to know it. However the fact that the file adapter gets the information from getFileLocation anyways kind of makes that a moot point. It wouldn't be a bad idea to in the future restrict which extensions/adapters get the full file adapter info, but for the sake of this particular PR that would be a much more significant revision and could break behaviors where other people use such terms. (not during create file specifically but elsewhere). |
…ation Strip down the config variables that get sent along with the file
There was a problem hiding this comment.
Pull request overview
This PR aims to extend Parse Server’s file upload pipeline so a files adapter can (optionally) return an updated filename and/or URL during createFile, and so adapters receive config during createFile to enable internal getFileLocation calls and other config-dependent behavior.
Changes:
- Updates the
FilesAdapter#createFileinterface documentation/signature to include aconfigargument and allow returning{ url, name, location }. - Adds unit tests asserting
FilesController#createFileprefers adapter-returnedname/urlwhen provided, otherwise falls back togetFileLocation. - Updates Cloud Code save-file hook tests to expect
configis forwarded toadapter.createFile.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Adapters/Files/FilesAdapter.js | Extends adapter interface/docs for createFile to accept config and optionally return updated file info. |
| spec/FilesController.spec.js | Adds tests for new adapter-return behavior (name/url override vs fallback). |
| spec/CloudCode.spec.js | Updates expectations that createFile is called with config as an additional argument. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @return {Promise<{url?: string, name?: string, location?: string}>|Promise<undefined>} Either a plain promise that should fail if storage didn't succeed, or a promise resolving to an object containing url and/or an updated filename and/or location (if relevant) | ||
| */ | ||
| createFile(filename: string, data, contentType: string, options: Object): Promise {} | ||
| createFile(filename: string, data, contentType: string, options: Object, config: Config): Promise {} |
| it('should return filename and url when adapter returns both', async () => { | ||
| const config = Config.get(Parse.applicationId); | ||
| const adapterWithReturn = { ...mockAdapter }; | ||
| adapterWithReturn.createFile = () => { | ||
| return Promise.resolve({ | ||
| name: 'newFilename.txt', | ||
| url: 'http://example.com/newFilename.txt' | ||
| }); | ||
| }; | ||
| adapterWithReturn.getFileLocation = () => { | ||
| return Promise.resolve('http://example.com/file.txt'); | ||
| }; | ||
| const controllerWithReturn = new FilesController(adapterWithReturn, null, { preserveFileName: true }); | ||
|
|
||
| const result = await controllerWithReturn.createFile( | ||
| config, | ||
| 'originalFile.txt', | ||
| 'data', | ||
| 'text/plain' | ||
| ); | ||
|
|
||
| expect(result.name).toBe('newFilename.txt'); | ||
| expect(result.url).toBe('http://example.com/newFilename.txt'); | ||
| }); |
| expect(createFileSpy).toHaveBeenCalledWith( | ||
| jasmine.any(String), | ||
| newData, | ||
| 'text/plain', | ||
| newOptions | ||
| newOptions, | ||
| jasmine.objectContaining({ applicationId: 'test' }) | ||
| ); |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Adapters/Files/FilesAdapter.js`:
- Around line 36-41: FilesController.createFile currently calls
this.adapter.createFile without the config and ignores its return value; update
the call to await this.adapter.createFile(filename, data, contentType, options,
config) (passing config as the 5th parameter) and capture its result (e.g.,
const adapterResult = await ...). Then merge adapterResult into the response: if
adapterResult.name override the filename, if adapterResult.url or
adapterResult.location override the returned url/location, and include any other
adapterResult fields as appropriate; if adapterResult is undefined, fall back to
the original url/name logic. Ensure the function references
FilesController.createFile and the adapter.createFile signature from
FilesAdapter.js.
- Line 39: The return type docs for FilesAdapter list both url and location but
FilesRouter only reads createFileResult.url and createFileResult.name, so
adapters returning {location: '…'} produce undefined URLs; either remove the
ambiguous location field from FilesAdapter.js JSDoc, or (preferred) update the
controller/FilesRouter to resolve an alias by reading createFileResult.url ??
createFileResult.location (use url if present, otherwise location) and document
that precedence; search for FilesAdapter, FilesRouter, and createFileResult in
the diff to implement the consistent resolution and update the JSDoc
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f040517c-d94d-4bf8-996d-482675f6534f
📒 Files selected for processing (3)
spec/CloudCode.spec.jsspec/FilesController.spec.jssrc/Adapters/Files/FilesAdapter.js
✅ Files skipped from review due to trivial changes (1)
- spec/CloudCode.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/FilesController.spec.js
| * @param {Config} config - (Optional) server configuration | ||
| * @discussion config may be passed to adapter to allow for more complex configuration and internal call of getFileLocation (if needed). This argument is not supported by all file adapters. Check the your adapter's documentation for compatibility | ||
| * | ||
| * @return {Promise} a promise that should fail if the storage didn't succeed | ||
| * @return {Promise<{url?: string, name?: string, location?: string}>|Promise<undefined>} Either a plain promise that should fail if storage didn't succeed, or a promise resolving to an object containing url and/or an updated filename and/or location (if relevant) | ||
| */ | ||
| createFile(filename: string, data, contentType: string, options: Object): Promise {} | ||
| createFile(filename: string, data, contentType: string, options: Object, config: Config): Promise {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether FilesController.createFile passes config to adapter and consumes return value
rg -n "adapter\.createFile" src/Controllers/FilesController.js -A 3Repository: parse-community/parse-server
Length of output: 213
FilesController.createFile must pass config to the adapter and use its return value
The adapter interface documents that config is forwarded as a 5th parameter and that an adapter may return {url, name} to override defaults. However, src/Controllers/FilesController.js:49 currently calls the adapter without passing config and discards its return value:
await this.adapter.createFile(filename, data, contentType, options);
return {
url: location,
name: filename,
};This means:
- Adapters receive
undefinedforconfig, causing any internalgetFileLocationcall to fail. - Adapter-returned values are silently ignored; callers always receive the pre-adapter filename and URL.
Update the controller to pass config and consume the adapter's response:
Proposed fix
- const location = await this.adapter.getFileLocation(config, filename);
- await this.adapter.createFile(filename, data, contentType, options);
- return {
- url: location,
- name: filename,
- }
+ const adapterResult = await this.adapter.createFile(filename, data, contentType, options, config);
+ const resolvedName = (adapterResult && adapterResult.name) ? adapterResult.name : filename;
+ const resolvedUrl = (adapterResult && adapterResult.url)
+ ? adapterResult.url
+ : await this.adapter.getFileLocation(config, resolvedName);
+ return {
+ url: resolvedUrl,
+ name: resolvedName,
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Adapters/Files/FilesAdapter.js` around lines 36 - 41,
FilesController.createFile currently calls this.adapter.createFile without the
config and ignores its return value; update the call to await
this.adapter.createFile(filename, data, contentType, options, config) (passing
config as the 5th parameter) and capture its result (e.g., const adapterResult =
await ...). Then merge adapterResult into the response: if adapterResult.name
override the filename, if adapterResult.url or adapterResult.location override
the returned url/location, and include any other adapterResult fields as
appropriate; if adapterResult is undefined, fall back to the original url/name
logic. Ensure the function references FilesController.createFile and the
adapter.createFile signature from FilesAdapter.js.
| * @discussion config may be passed to adapter to allow for more complex configuration and internal call of getFileLocation (if needed). This argument is not supported by all file adapters. Check the your adapter's documentation for compatibility | ||
| * | ||
| * @return {Promise} a promise that should fail if the storage didn't succeed | ||
| * @return {Promise<{url?: string, name?: string, location?: string}>|Promise<undefined>} Either a plain promise that should fail if storage didn't succeed, or a promise resolving to an object containing url and/or an updated filename and/or location (if relevant) |
There was a problem hiding this comment.
location in the return type is never consumed and ambiguous vs. url
The documented return shape has both url and location, but FilesRouter (Context Snippet 4) only reads createFileResult.url and createFileResult.name. An adapter that returns { location: 'https://…' } instead of { url: 'https://…' } will silently produce an undefined URL for the stored file.
Either drop the location field from the documented return type, or (if it's intentional as an alias) document exactly how the controller will resolve precedence between url and location, and ensure the controller code actually implements that resolution.
📝 Proposed fix
- * `@return` {Promise<{url?: string, name?: string, location?: string}>|Promise<undefined>} Either a plain promise that should fail if storage didn't succeed, or a promise resolving to an object containing url and/or an updated filename and/or location (if relevant)
+ * `@return` {Promise<{url?: string, name?: string}>|Promise<undefined>} Either a plain promise that should fail if storage didn't succeed, or a promise resolving to an object containing the file URL (url) and/or the stored filename (name) if they differ from the input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Adapters/Files/FilesAdapter.js` at line 39, The return type docs for
FilesAdapter list both url and location but FilesRouter only reads
createFileResult.url and createFileResult.name, so adapters returning {location:
'…'} produce undefined URLs; either remove the ambiguous location field from
FilesAdapter.js JSDoc, or (preferred) update the controller/FilesRouter to
resolve an alias by reading createFileResult.url ?? createFileResult.location
(use url if present, otherwise location) and document that precedence; search
for FilesAdapter, FilesRouter, and createFileResult in the diff to implement the
consistent resolution and update the JSDoc accordingly.
Allow file adapter to override file name and location + give createFile access to config to allow getFileLocation calls internally
File adapter may specify a different filename or location during creation which should be returned after content storage.
Example, adapter timestamps upload filenames. getFileLocation may result in a slightly different url than the one used to create the file.
In the case where the url returned is identical this is not a problem. Additionally file adapter could call getFile location internally if it wanted to (and should probably have access to config for that reason).
In principle, if user wanted to name every file uploaded in an ordinal fashion (eg 1.jpg,2.jpg,3.jpg), they should allowed to do this.
As a separate improvement, this allows the url to be assigned during createFile as well (to prevent extra calls if the url is already known). Passing config to createFile allows getFileLocation to be called internally). Otherwise if url is not assigned, then getFileLocation can be called to retrieve it with the updated filename.
If url is not provided and filename is unchanged by createFile, then this code will not alter current functionality
Pull Request
Issue
Closes: #9556
Approach
Tasks
Summary by CodeRabbit
Tests
Refactor