Skip to content
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: req.file throws filesize error #207

Merged
merged 2 commits into from
Mar 11, 2021
Merged

Conversation

brainrepo
Copy link
Contributor

Feature proposal:

Allow req.file to throw an error when the file is bigger than the limit

closes #196

Checklist

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good to me, I've left a few notes.

@@ -30,7 +30,7 @@ test('should throw fileSize limitation error on small payload', async function (
const part = await req.file({ limits: { fileSize: 2 } })
await sendToWormhole(part.file)
if (part.file.truncated) {
reply.code(500).send()
reply.code(413).send()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ops, simply my carelessness

@@ -504,6 +504,9 @@ function fastifyMultipart (fastify, options, done) {
let part
while ((part = await parts()) != null) {
if (part.file) {
if (part.file.truncated) {
throw new RequestFileTooLargeError()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure that the only condition for the file being truncated is that it is too large? Maybe it should be a different error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, Busboy the lib used under the hood sets the property truncated only when the stream reaches fileSizeLimit.

https://github.com/mscdex/busboy/blob/5ef3f9b1d08e7ccb7c9b3690e4c071089e66285c/lib/types/multipart.js#L222

Here the busboy docs:

If a configured file size limit was reached, stream will both have a boolean property truncated (best checked at the end of the stream) and emit a 'limit' event to notify you when this happens

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a comment?

@climba03003
Copy link
Member

If you specify it in the function getMultipartFile, it can not respect the route level options of throwFileSizeLimit.

@mcollina
Copy link
Member

A test is needed.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

can you check why CI is failing?

@brainrepo
Copy link
Contributor Author

can you check why CI is failing?

Yes, I am doing it, the ci fails on windows for ERR_STREAM_PREMATURE_CLOSE, but in my local fork the CI is passing.

@brainrepo
Copy link
Contributor Author

brainrepo commented Mar 10, 2021

can you check why CI is failing?

I have installed a windows machine on Virtualbox with node 12.21.0 as the CI, to reproduce the error, but everything works fine.

@mcollina
Copy link
Member

@StarpTech could you take a look?

@mcollina mcollina merged commit 030b3fd into fastify:master Mar 11, 2021
@StarpTech
Copy link
Member

Sorry, I had no time yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fileSize limit on fastify-multipart did not throw error
5 participants