-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow large Parse File uploads #9286
base: alpha
Are you sure you want to change the base?
feat: Allow large Parse File uploads #9286
Conversation
Thanks for opening this pull request! |
spec/FilesRouter.spec.js
Outdated
|
||
afterAll(async () => { | ||
// clean up the server for resuse | ||
if (server && server.close) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be enough to just call await reconfigureServer()
to init the server with the default config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your right. Missed that, new to testing here.
Signed-off-by: Manuel <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #9286 +/- ##
==========================================
- Coverage 93.50% 84.74% -8.76%
==========================================
Files 186 186
Lines 14807 14831 +24
==========================================
- Hits 13845 12569 -1276
- Misses 962 2262 +1300 ☔ View full report in Codecov by Sentry. |
There seems to be some test coverage missing, could you add that? |
return reject(err); | ||
}); | ||
|
||
const iv = crypto.randomBytes(16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will be executed in every case, which can be unnecessary. Could you refactor like in the original code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its the original order I think I would have to have duplicate initializations for cipher
and iv
when its a blob case and when else. I could also do the null ternary like I do with cipher, if you would prefer that?
|
||
try { | ||
// when working with a Blob, it could be over the max size of a buffer, so we need to stream it | ||
if (typeof Blob !== 'undefined' && data instanceof Blob) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what circumstances would Blob be undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled it from this line in the JS SDK
https://github.com/parse-community/Parse-SDK-JS/blob/3e955cd3944de39a559e38cbac58d86faa892606/src/ParseFile.ts#L119
I think maybe some node versions might not have it?
Pull Request
Issue
Closes: #9283
Approach
During routing if a file size is larger than can be handled by the V8 engine as a string, it is used as a Blob. This changes touches both the
FilesRouter
andGridFSBucketAdapter
where the creation/upload happens, and the file is handled as a Buffer. I also added a new FilesRouter test spec for these changes.Tasks