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

Option to append request.id / other unique ID to onFile? #427

Closed
2 tasks done
digitalmio opened this issue Mar 23, 2023 · 7 comments · Fixed by #431
Closed
2 tasks done

Option to append request.id / other unique ID to onFile? #427

digitalmio opened this issue Mar 23, 2023 · 7 comments · Fixed by #431
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@digitalmio
Copy link
Contributor

digitalmio commented Mar 23, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

On file function gets only part and based on filename, you can then save the file to disc etc. This solution is good if the file names are unique, but when 2 users will send the file with the same name, it would become problematic.
It would be good if second param with req.id or other unique ID that would allow to trace the file back to request.
If there is other/easier way - pls let me know. Thanks!

@liudichen
Copy link

liudichen commented Mar 24, 2023

As far as I know, based on the source code, if you use req.saveRequsteFiles() ,names of those files may be unique ids.
Alternatively, you can customize onFile by specifying a file name when saving the file.Then you can get 1 object with the original name and a new unique name (with which to cache the file). Remember to manually release the local file cache(Maybe you can custom this repo to auto release it, maybe like this).

const generateID = ()=> `a unique ID, such as nanoId, uuid`

const onFile =  (field, file, filename, encoding, mimetype, body) =>  {
  const fileData = []
  const lastFile = body[field][body[field].length - 1]
  file.on('data', data => { if (!lastFile.limit) { fileData.push(data) } })
  file.on('limit', () => { lastFile.limit = true })
  file.on('end', () => {
    if (!lastFile.limit) {
      const buffer = Buffer.concat(fileData)
      lastFile.size = buffer.length
      const dir = path.join(os.tmpdir(), 'fastify-multipart')
     if (!fs.existsSync(dir)) { fs.mkdirSync(dir) }
    const filepath = path.join(dir, toID() + path.extname(filename))
     try {
        fs.writeFileSync(filepath, buffer)
        lastFile.filepath = filepath
     } catch (error) {
        lastFile.error = 'error when write to disk'
     }
     delete lastFile.data      
    } else {
        delete lastFile.data
    }
  })
}

@digitalmio
Copy link
Contributor Author

Hey,

Thanks, yes, I know all this, and even now I can use any unique filename when I'm storing it in on the disk. The problem is that I don't see a way to send this ID "back" to route, or get it from route.
Currently, file will have unique ID, but I won't be able to link it to request...
Hope it makes sense

@liudichen
Copy link

I haven't used the original fastify, but in Nestjs, the request instance can be obtained in route.

@digitalmio
Copy link
Contributor Author

digitalmio commented Mar 24, 2023

Thanks, but I need solution for Fastify.
Looking into code, I cannot do it, but it should be easy option to add it here. Just want to check with the community if it makes sense.

@digitalmio
Copy link
Contributor Author

Can I ask for comments please, does it makes sense?
Like - imagine that on file you would like to upload file directly to S3, you can save it using requestId as a filename/folder name.
@mcollina - I would be super grateful for your comment.

@Eomm
Copy link
Member

Eomm commented Apr 1, 2023

This solution is good if the file names are unique, but when 2 users will send the file with the same name, it becomes problematic.

Storing an uploaded file within its original file name is problematic.
As a general rule - never do that because:

  • it is a security risk, a bad user may upload a malicious file name
  • as you already know: it could overwrite existing files
  • file system limitation: try to create a file named "foo" and "FOO" on a windows server

I think we could implement the context binding for the onFile function tho:

async function onFile(part) {
  console.log(this) // it is the fastify request object
  await pump(part.file, fs.createWriteStream(this.reqId))
}

@Eomm Eomm added semver-minor Issue or PR that should land as semver minor good first issue Good for newcomers labels Apr 1, 2023
@digitalmio
Copy link
Contributor Author

Created pull request, not sure if I should bump package.json version too?

@Eomm Eomm closed this as completed in #431 Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants