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 2 commits into
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)
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:
|
|
@mtrezza any chance to run the workflow ? |
|
Done |
|
@AdrianCurtin how does the S3 PR play with this one parse-community/parse-server-s3-adapter#242 Both are needed ? |
|
This PR was designed to respect legacy behavior (or else personally I would have moved config to the first argument to match getFileLocation's argument order). For a fileadapter that does not return result.name or result.url, there should be no breaking changes. So old s3 adapters and other fileadapters should continue to function as is. The pr for the s3 adapter parse-community/parse-server-s3-adapter#242 will take this argument but won't actually return the proper location (url) without the config being passed. So if this PR is merged then 242 can function and preserve filenames. But if this PR is not merged, 242 will still run, but won't preserve the filenames (legacy behavior). Both need to be merged in order to fix the issue, but if either is merged (or for instance an older parse-server with a newer s3 adapter), it should behave with legacy behaviors. |
|
I think i've a last question @AdrianCurtin : how the fallback approach of createdFile.url || getFileLocation, play in case of using "publicServerUrl", does the adapter is aware of the publicServerUrl to return appropriate path (like https://example.com/parse/files/xxxxxxx.jpeg) and not the url of the Storage provider, since parse-server support proxying files |
|
@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). |
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 {} |
There was a problem hiding this comment.
Agreed, the signature contradicted the doc. Fixed in f8d1d51: config is now config?: Config, so the type matches the "(Optional)" wording and existing adapters that take four parameters stay valid.
The controller already passes config for the adapters that want it, so nothing is forced to change to keep working.
| 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'); | ||
| }); |
There was a problem hiding this comment.
This has since been addressed on the branch. FilesController#createFile now passes config as the fifth argument and consumes the adapter's return value, resolving the location after creation rather than before, so a filename the adapter changed is reflected in both the returned name and the URL. An adapter that returns nothing falls back to the previous behavior.
Verified against the current branch: spec/FilesController.spec.js, spec/ParseFile.spec.js and the Parse.File specs in spec/CloudCode.spec.js all pass, including the assertion that createFile is called with config.
| expect(createFileSpy).toHaveBeenCalledWith( | ||
| jasmine.any(String), | ||
| newData, | ||
| 'text/plain', | ||
| newOptions | ||
| newOptions, | ||
| jasmine.objectContaining({ applicationId: 'test' }) | ||
| ); |
There was a problem hiding this comment.
This has since been addressed on the branch. FilesController#createFile now passes config as the fifth argument and consumes the adapter's return value, resolving the location after creation rather than before, so a filename the adapter changed is reflected in both the returned name and the URL. An adapter that returns nothing falls back to the previous behavior.
Verified against the current branch: spec/FilesController.spec.js, spec/ParseFile.spec.js and the Parse.File specs in spec/CloudCode.spec.js all pass, including the assertion that createFile is called with config.
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
|
Rebased onto current alpha and squashed into a single commit. With 3 changes, not just a rebase src/Controllers/FilesController.js was not in the diff. It got dropped during the "stripped down config" / "revert to full config" back and forth, so the adapter never actually received the config and its return value was never used, while the tests asserting both behaviors stayed in the PR. Those three FilesController specs fail on the pre-rebase branch. This never showed up because the test workflow needs maintainer approval on fork PRs, so only CodeRabbit, Snyk and the commit message check ever ran. Full config is passed per @Moumouls Test failure that forced that workaround was my mistake. Assertions now check applicationId and mount, using Parse.serverURL from the test suite rather than a hard-coded URL, per @mtrezza 's earlier note |
…and mark config optional
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