Rewritten or Restructure

[climba03003]

Prerequisites

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

🚀 Feature Proposal

We should reduce the methods to something that is necessary.
For example, parts, files, file does nearly the same but confusing.

We should either provide parsing method directly or async iterator for advanced user.

The current request.body object is not user friendly, which leads to multiple config (e.g. keyValues)

Motivation

With the growth of feature and never removed deprecated functions.
The current codebase is messy and not user-friendly to beginner.

I found must people is not family with how stream works and ask for simple solution multiple times.
That's why I created fastify-formidable and fastify-busboy.

I believe we can do something to provide a more generic interface and not fixed to a single parsing engine.
So, I created @kakang/fastify-multipart and seek for advice.

Example

No response

[Uzlopak]

I think the biggest issue is handling big files as new devs dont think about memory leaks etc..

[Eomm]

I call it "Apple style" - The user is requesting a limited number of options otherwise it can't choose

I agree from the support perspective

We should list the use case we want to support first

[mcollina]

I kind of agree that a refactoring of this is needed, however I'm not sure exactly in which direction.

Most of the recently added features are not safe to use in generic sense, as keeping files in memory is a bad practice.

[climba03003]

We should list the use case we want to support first

The important use-case and most requesting feature is the below two.

1. Solution for beginner

No extra configuration and provides json body for the users.
AFAICT most of the web server (e.g. caddy, nginx, apache, php-fpm) download the files to the temporary folder directly and allows the proxy server to consume later.

2. Solution for advanced user

async iterator is one of the option which is the .parts API currently.
Providing .files, .file, .saveRequestFiles is strange for me, especially .file. If the client is sending multiple file but you only called .file the request should be staling.

[kibertoad]

@climba03003 Why not use https://www.npmjs.com/package/@fastify/busboy?

[climba03003]

Why not use https://www.npmjs.com/package/@fastify/busboy?

Since here we are using @fastify/busboy, I just choose other competitor for the parsing engines.
Not a big deal to add one by default though.

[climba03003]

next branch is created for gathering breaking changes.
I have rebased both #465 and #468

[Eomm]

Could we breakdown some issues into a project for this?

[gurgunday]

Before more things are removed/changed, I just want to add that I really do like the DX of attachFieldsToBody: true and attachFieldsToBody: 'keyValues'

The docs warn you about accumulating files in memory but provide multiple ways to deal with that:

1. Setting a fileSize limit when registering and listening for error.statusCode === 413 in an error handler -> very easy and I frequently find myself using this

2. Defining an onFile handler -> a little harder to integrate but still works

As I said, option 1 (especially with 'keyValues') is really useful and can be foolproof if it's explained properly

For example, parts, files, file does nearly the same but confusing.

So, all in all, I'm more interested in modifying/refactoring these lower-level methods and maybe changing the defaults of the plugin

[Uzlopak]

Maybe we should design multipart to be configured "incremental". You need multipart for processing forms. E.g. a login form does not need file uploads. So we first support out of the box that you can process forms and have the fields processed as attributes of the body object.
Files should be then either be flushed away or throw an error.

Then we can determine, what we do with the files. Should we out of the box handle them as Buffers assigned to the body? Should we handle them as streams?
Obviously they will always be problematic.

Also we should consider, that we maybe could implement a better integration into the fastify schema system.

In a perfect world, i would register multipart plugin with maybe some general limits.
Then in the route i can specifiy formdata as possible request body. If formdata is specified I would like that multipart is setup accordingly. I didnt define files in the schema? Throw an error. I get a field which is not defined in the schema? Throw an error.
I want to specify in the schema the maxLength of a file part and that is respected over the general setting of multipart.
I expect a file but i dont have defined what i want to do with the file? onRoute hook should throw early.

Maybe every file part is a stream. We supply the guys with two stores, like we supply fastify-rate-limit with two stores. One memory and one redis.

So the body gets a getter for the file fields, which instantiate a stream. So your Memory store is the leaky bad one, like in fastify-rate-limit.

You upload the file, the memory store keeps the Buffer in memory. Memory store is a wrapper around a Map. We generate an uuid as key and store the buffer in there. Then the body gets a corresponding property named to the file field and the getter is basically Readable(Map.get(uuid)). We could enrich that Readable with attributes like size and contentEncoding, and maybe even the key in the Map (makes more sense with the redis implementation below)
So when we are in the handler, we can consume the stream.

The redis store basically the same. We configure the redis store and pass it as a configuration to the multipart plugin. We pump the files with uuids to the redis instance. Also add a property to the body, with a getter to a redisstream. Then in the handler you can use the stream to pipe it somewhere else.

Imho this would be the best. For the memory store and for the redis store you could configure ttl.

When i used file uploads in express, i always streamed the files to redis and passed the key of redis to the other microservices. So the other microservices could handle them, and maybe even unlink them after use.

The benefit is also, that people could e.g. implement their own stores,like a mongodb gridfs store, a sqlite store, or a fs store or, or, or...

Also by using streams and stores, it is easier to test without databases and we kind of encourage people to consider using streams and it would be easier to implement microservices, as the state would be forced to be outside the fastify microservice.

I wrote once a redis stream implementation. Maybe it could be adapted accordingly.

redis/ioredis#1376

[gurgunday]

Maybe we should design multipart to be configured "incremental". You need multipart for processing forms. E.g. a login form does not need file uploads. So we first support out of the box that you can process forms and have the fields processed as attributes of the body object. Files should be then either be flushed away or throw an error.

Fully agree, things can be a lot cleaner if file processing is opt-in, set either by a global setting or by per-route configuration, like how rate-limit works. This would mean registering the plugin with fileSize: 0 by default according to the current implementation

Then in the route i can specifiy formdata as possible request body. If formdata is specified I would like that multipart is setup accordingly. I didnt define files in the schema? Throw an error. I get a field which is not defined in the schema? Throw an error.

I'm digging it, as long as it's consistent with other default content parsers. For instance, if I'm not mistaken, Fastify doesn't throw when a field that wasn't defined in the schema exists in JSON input

For the rest, I know everyone hates on storing files in-memory and all, but I love it and use it very often – it's just too convenient 🤣

And it doesn't have to be leaky and bad. As I described above, by setting an appropriate size limit and error handler, it should always be possible to get rid of a file if necessary while making it easy to keep the validated buffer around during the lifecycle of a request

So yeah I'm behind this plan as long as the in-memory store is not left out :D

[gurgunday]

Finally, since getting #468 out would really help me now and I'm nearly sure nobody is consuming files as strings, maybe bumping major twice wouldn't be so bad – it doesn't have to include #465

Or maybe a new release channel for next?

@mcollina