only add requestFile to tmpUploads if it is not rejected by busboy #171
Conversation
index.js
Outdated
| requestFiles.push({ ...file, filepath }) | ||
| // busboy set truncated to true when the configured file size limit was reached | ||
| if (file.file.truncated) { | ||
| const err = new RequestFileTooLargeError() | ||
| err.part = file | ||
| throw err | ||
| } | ||
| // Delay adding the filepath to tmpUploads until the file has been checked for size | ||
| this.tmpUploads.push(filepath) |
There was a problem hiding this comment.
We need to add it there to clean up the files after the response.
There was a problem hiding this comment.
I also opened an issue to explain further why this change is necessary, see #172
There was a problem hiding this comment.
In short: this adds the file to the list to be cleaned up when the response is closed, but by then it is already deleted.
There was a problem hiding this comment.
You're right. We remove the file afterward in the catch handler. I propose to leave it as it is but remove the additional unlink operation in the catch case.
There was a problem hiding this comment.
Ok, I'm thinking this should be better
index.js
Outdated
| requestFiles.push({ ...file, filepath }) | ||
| // busboy set truncated to true when the configured file size limit was reached | ||
| if (file.file.truncated) { | ||
| const err = new RequestFileTooLargeError() | ||
| err.part = file | ||
| throw err | ||
| } | ||
| // Delay adding the filepath to tmpUploads until the file has been checked for size | ||
| this.tmpUploads.push(filepath) |
There was a problem hiding this comment.
You're right. We remove the file afterward in the catch handler. I propose to leave it as it is but remove the additional unlink operation in the catch case.
|
Yeah I was just writing that up, guess you were faster :) |
index.js
Outdated
| this.log.debug({ err }, 'could not delete file') | ||
| } | ||
|
|
||
| this.log.error(err) |
There was a problem hiding this comment.
Adding a bit more context makes it easier to know where it failed.
this.log.error({ err }, 'save request file')There was a problem hiding this comment.
Done. Are the object brackets necessary? I added them just in case
Checklist
npm run testandnpm run benchmarkand the Code of conduct
PASS test/multipart-big-stream.test.js 3 OK 254.126ms
PASS test/multipart-concat.test.js 9 OK 256.44ms
PASS test/legacy/multipart.test.js 43 OK 446.868ms
PASS test/multipart-body-schema.test.js 8 OK 374.937ms
PASS test/multipart-attach-body.test.js 16 OK 312.564ms
PASS test/legacy/append-body.test.js 74 OK 1s
PASS test/multipart-disk.test.js 42 OK 527.466ms
PASS test/multipart-security.test.js 8 OK 283.763ms
PASS test/multipart-small-stream.test.js 2 OK 141.19ms
PASS test/multipart.test.js 68 OK 459.869ms
PASS test/big.test.js 9 OK 9s
PASS test/legacy/big.test.js 12 OK 12s
🌈 SUMMARY RESULTS 🌈
Suites: 12 passed, 12 of 12 completed
Asserts: 294 passed, of 294
Time: 17s
----------|----------|----------|----------|----------|-------------------|
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files | 97.28 | 90.82 | 100 | 97.23 | |
index.js | 97.28 | 90.82 | 100 | 97.23 |... 80,281,470,488 |
----------|----------|----------|----------|----------|-------------------|