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

Avoid piping the same file multiple times in case saveRequestFile is called twice. #444

Merged
merged 2 commits into from
Jun 10, 2023

Conversation

arthurfiorette
Copy link
Contributor

@arthurfiorette arthurfiorette commented May 26, 2023

In cases where multiple hooks and the handler needs to use the saved request files, and they cannot share the first call's result of saveRequestFile, they would call it twice or more which would result into piping the same file to disk more than once. As we can check by the following code:

fastify.post('/', async (req, rep) => {
  await req.saveRequestFiles() // [{ filepath: '/tmp/0388009396720a01.txt' }]
  await req.saveRequestFiles() // [{ filepath: '/tmp/0388009396720a02.txt' }]
  await req.saveRequestFiles() // [{ filepath: '/tmp/0388009396720a03.txt' }]
})

Now, saveRequestFiles will use the same system file and do not write the disk more than needed.

fastify.post('/', async (req, rep) => {
  await req.saveRequestFiles() // [{ filepath: '/tmp/0388009396720a04.txt' }]
  await req.saveRequestFiles() // [{ filepath: '/tmp/0388009396720a04.txt' }]
  await req.saveRequestFiles() // [{ filepath: '/tmp/0388009396720a04.txt' }]
  //                                       (Same filepath) ⬆️

  // where they are saved
  req.savedRequestFiles // [{ filepath: '/tmp/0388009396720a04.txt' }]
})

Checklist

@arthurfiorette
Copy link
Contributor Author

@climba03003, sorry for pinging, but are you willing to merge this pr?

@joao-cabral
Copy link

I need this merge 😓

@felipe-bergamaschi
Copy link

@mcollina We need this merge 😓

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.
Sorry for the delay, I really do not have time currently.
The implementation is good.

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 c94b2cb into fastify:master Jun 10, 2023
16 checks passed
@renovate renovate bot mentioned this pull request Jul 8, 2023
1 task
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.

None yet

5 participants