feat: Allow File adapter to create file with specific locations or dynamic filenames #9557
Conversation
Thanks for opening this pull request! |
AdrianCurtin
changed the title
AdrianCurtin
changed the title
|
@mtrezza |
|
@mtrezza |
|
@AdrianCurtin Thank you for picking this up again, I've restarted the CI, let's see... |
| * @discussion options are not supported by all file adapters. Check the your adapter's documentation for compatibility | ||
| * @param {Config} config - (Optional) server configuration |
There was a problem hiding this comment.
Why would the whole Parse Server config be passed into the adapter? Or am I misreading this?
There was a problem hiding this comment.
@mtrezza
Allowing the server config to be passed to the createFile would allow a couple of things.
- files can be named based on serer attributes (ex: generateKey could store log files with the app id)
- getFileLocation can be called within createFile, either to test or check that the file has been created (especially if getFileLocation depends on the server config which it always has access to)
There may be other situations in which the server config may be useful apart from customizing file paths and running getFileLocation, but I don't see why createFile shouldn't have access to it if getFileLocation does.
If this was a smaller project I would probably have refactored adapter.createFile to use config as the first argument, but that had the potential to be fairly breaking and maybe its nicer for some people to use X.createFile('filename')
In any case tacking it at the end was the compromise I made.
In the current S3-adapter PR, this only allows getFileLocation to run inside and I didn't pass the parameters to generateKey.
I would say though that removing that expectation and removing getFileLocation from the adapter would make the changes more explicit, but then [location] = createFile would not always match the call from getFileLocation (if it depended in any way on server parameters)
There was a problem hiding this comment.
Is passing the server config to the adapter l a good approach? It creates a dependency on a specific config structure. If that structure changes, it could break the adapter, right? Or is the config already used in other parts of the adapter?
There was a problem hiding this comment.
The file location is based on the server config as is, so when you call getFileLocation you need the server config.
You only need the mount and the app id though for most getFileLocation calls, but the adapter gets the full server config in those cases.
So we can't actually know where the file ends up without the server config unfortunately (if it ends up being different from the getFileLocation)
We can opt instead for the full server config to pass through but that was making the tests unhappy a little and I haven't seen any instances of getFileLocation use more than just those parameters.
There was a problem hiding this comment.
This is the basic idea:
FilesController.createFile returns url (location) and filename
File location is determined by the file name and the server configuration
but adapter.createFile does not have access to the server config and therefore cannot return the stored file config if the filename or any other part has changed.
With this change, the expectation is that createFile might return an updated filename (if for instance generatekey was used to alter the filepath), and optionally, createFile could return the entire location (presumably by calling getFileLocation internally.
If createFile returns nothing (current implementation), the original filename will be returned and getFileLocation will be called using the original filename (after creation though instead of before). <- no breaking changes here
If a file adapter chooses to alter the filename, but does not internally implement getFileLocation and return a url, this version will simply call getFileLocation but with the revised filename so that it can be correctly found. Adapters would be responsible for returning altered filenames when they are changed and this is the current issue with the s3 adapter issue.
If a file adapter chooses to internally call getFileLocation or a similar function to return the URL, then we can simply return the location that was part of createFile instead of calling a separate function. (File adapters would be responsible for ensuring that the returned url would be equivalent to the getFileLocation url, again this is opt-in).
If a file adapter chose to alter the filename or some other aspect of file creation based on the server configuration (ex: s3 folders based on application id), it would be the file adapter's responsibility to ensure that the create file behavior was not brittle. However, this complexity is especially opt-in and would not be advised for anything beyond altering the filename if desired.
For most adapters, the best course of action here is to simply update them to return the filename as stored (if the adapter is one that might change the filename).
For some adapters the getFileLocation might simply be returning that file directly with some modification, or others like presigning might be more involved. Implementing this internally may result in performance increases (small likely) but also is not strictly necessary.
There was a problem hiding this comment.
@mtrezza passing the whole config to an adapter don't feel strange to me, it allow a true inversion of control, yes it can break in case of config change, but a developer is responsible of testing its custom adapter basically
spec/FilesController.spec.js
Outdated
| it('should return valid filename or url from createFile response when provided', async () => { | ||
| const config = Config.get(Parse.applicationId); | ||
|
|
||
| // Test case 1: adapter returns new filename and url |
There was a problem hiding this comment.
Could you please split this into separate tests, each one with a distinct title to describe what it's testing.
There was a problem hiding this comment.
You also may want to take a look at the failing tests in the CI
There was a problem hiding this comment.
Done, sorry about the delay, @mtrezza lmk when you have a second to rerun the CI
…le 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)
but use updated filename in getFileLocation
|
Warning Rate limit exceeded@AdrianCurtin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFilesAdapter.createFile gains a final Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FilesController
participant FilesAdapter
Client->>FilesController: createFile(filename, data, contentType, options)
FilesController->>FilesAdapter: createFile(filename, data, contentType, options, config)
FilesAdapter-->>FilesController: { name?, url? } or undefined
alt adapter provides name
FilesController->>FilesController: filename = result.name
end
alt adapter provides url
FilesController->>FilesController: url = result.url
else
FilesController->>FilesAdapter: getFileLocation(config, filename)
FilesAdapter-->>FilesController: url
end
FilesController-->>Client: { url, name: filename }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 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}
…ation Strip down the config variables that get sent along with the file
|
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 93.01% 93.01%
=======================================
Files 187 187
Lines 15096 15098 +2
Branches 174 174
=======================================
+ Hits 14041 14043 +2
Misses 1043 1043
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -3727,11 +3734,18 @@ describe('saveFile hooks', () => { | |||
| foo: 'bar', | |||
| }, | |||
| }; | |||
| // Get the actual config values that will be used | |||
| const expectedConfig = { | |||
There was a problem hiding this comment.
Can you import this from the test suite where it's defined?
There was a problem hiding this comment.
I updated it, but previously the checks were unhappy with the full config being passed through, I don't recall why off the top of my head
There was a problem hiding this comment.
I added it but i still stripped it down because previously the config had been modified through the process or something. We'll see if it passes CI, but Maybe the check just needs to be loosened here.
There was a problem hiding this comment.
If you don't mind assisting,
when i revert to the config('test") i end up with these errors
`
- saveFile hooks beforeSave(Parse.File) should change values of uploaded file by editing fileObject directly
- Expected spy createFile to have been called with:
[ <jasmine.any(String)>, Buffer [ 1, 2, 3 ], 'text/plain', Object({ tags: Object({ tagA: 'some-tag' }), metadata: Object({ foo: 'bar' }) }), Object({ applicationId: 'test', mount: undefined, fileKey: 'test' }) ]
but actual calls were:
[ '7e441dcb3fc497b19592367415376ea1_popeye.txt', Buffer [ 1, 2, 3 ], 'text/plain', Object({ metadata: Object({ foo: 'bar' }), tags: Object({ tagA: 'some-tag' }) }), Object({ applicationId: 'test', mount: 'http://localhost:8378/1', fileKey: 'test' }) ].
Call 0:
Expected $[4].mount = 'http://localhost:8378/1' to equal undefined.
- saveFile hooks beforeSave(Parse.File) should contain metadata and tags saved from client
- Expected spy createFile to have been called with:
[ <jasmine.any(String)>, <jasmine.any(Buffer)>, 'text/plain', Object({ metadata: Object({ foo: 'bar' }), tags: Object({ bar: 'foo' }) }), Object({ applicationId: 'test', mount: undefined, fileKey: 'test' }) ]
but actual calls were:
[ '352ace761fac45c85ba556c85548888d_popeye.txt', Buffer [ 1, 2, 3 ], 'text/plain', Object({ metadata: Object({ foo: 'bar' }), tags: Object({ bar: 'foo' }) }), Object({ applicationId: 'test', mount: 'http://localhost:8378/1', fileKey: 'test' }) ].
Call 0:
Expected $[4].mount = 'http://localhost:8378/1' to equal undefined.
- saveFile hooks beforeSave(Parse.File) should change values by returning new fileObject
- Expected spy createFile to have been called with:
[ <jasmine.any(String)>, Buffer [ 4, 5, 6 ], 'application/pdf', Object({ tags: Object({ tagA: 'some-tag' }), metadata: Object({ foo: 'bar' }) }), Object({ applicationId: 'test', mount: undefined, fileKey: 'test' }) ]
but actual calls were:
[ 'b048f76b35f8628bf9336c5efdd0eb95_donald_duck.pdf', Buffer [ 4, 5, 6 ], 'application/pdf', Object({ metadata: Object({ foo: 'bar' }), tags: Object({ tagA: 'some-tag' }) }), Object({ applicationId: 'test', mount: 'http://localhost:8378/1', fileKey: 'test' }) ].
Call 0:
Expected $[4].mount = 'http://localhost:8378/1' to equal undefined.
`
| const basicServerConfig = { | ||
| applicationId: config.applicationId, | ||
| mount: config.mount, | ||
| fileKey: config.fileKey | ||
| }; |
There was a problem hiding this comment.
issue: why passing a partial config, we could land with inconsistencies, i think the whole config should be passed for a true inversion of control
| * @discussion options are not supported by all file adapters. Check the your adapter's documentation for compatibility | ||
| * @param {Config} config - (Optional) server configuration |
There was a problem hiding this comment.
@mtrezza passing the whole config to an adapter don't feel strange to me, it allow a true inversion of control, yes it can break in case of config change, but a developer is responsible of testing its custom adapter basically
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
New Features
Documentation
Tests