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

Support for saveRequestFiles with attachFieldsToBody set true #409

Merged
merged 8 commits into from
Jan 13, 2023

Conversation

Ceres6
Copy link
Contributor

@Ceres6 Ceres6 commented Jan 9, 2023

Added files async files generator function to saveRequestFiles which fetches files from body instead of from getMultipartFiles when `attachFieldsToBody is set to true

Partially addresses #313

Checklist

@mcollina mcollina requested a review from StarpTech January 10, 2023 08:53
@mcollina
Copy link
Member

Why does it only partially address #313?

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

@Ceres6
Copy link
Contributor Author

Ceres6 commented Jan 10, 2023

Why does it only partially address #313?

Because the typescript problem described couldn't be reproduced, and in case of custom onFile it will only work as long as it still creates the _buf property on files attached to body. I guess that if that's not the case the user should as well store the files in that method as well.

@mcollina
Copy link
Member

and in case of custom onFile it will only work as long as it still creates the _buf property on files attached to body.

Could you throw a proper error if this is the case?

index.js Outdated
filename,
encoding,
mimetype,
limit: false
Copy link
Member

Choose a reason for hiding this comment

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

is this change relevant?

index.js Outdated
next(err)
},
options
)
Copy link
Member

Choose a reason for hiding this comment

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

is this change relevant? Seems just linting

index.js Outdated
})
file.on('limit', () => {
lastFile.limit = true
})
Copy link
Member

Choose a reason for hiding this comment

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

is this change relevant?

index.js Outdated
if (
options.attachFieldsToBody === true ||
options.attachFieldsToBody === 'keyValues'
) {
Copy link
Member

Choose a reason for hiding this comment

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

is this change relevant?

index.js Outdated
'FST_FILE_BUFFER_NOT_FOUND',
'the file buffer was not found',
500
)
Copy link
Member

Choose a reason for hiding this comment

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

is this change relevant? Seems just linting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, linter went crazy, I think they're all solved now

index.js Outdated
@@ -528,6 +535,29 @@ function fastifyMultipart (fastify, options, done) {
return requestFiles
}

async function * filesFromFields (container) {
Copy link
Member

Choose a reason for hiding this comment

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

Why async generator but not a generator?
It doesn't really need to be async.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already changed

@Ceres6 Ceres6 requested review from mcollina and climba03003 and removed request for StarpTech, mcollina and climba03003 January 10, 2023 18:37
@Ceres6
Copy link
Contributor Author

Ceres6 commented Jan 10, 2023

It seems I can only re-request one review @mcollina @climba03003 @StarpTech, sorry for the inconvenience

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 mcollina merged commit 4c0079c into fastify:master Jan 13, 2023
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.

4 participants