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

Mimetype Parsing for Non-File Parts in MultipartForm #805

Closed
sabihismail opened this issue Jan 1, 2022 · 5 comments
Closed

Mimetype Parsing for Non-File Parts in MultipartForm #805

sabihismail opened this issue Jan 1, 2022 · 5 comments

Comments

@sabihismail
Copy link

sabihismail commented Jan 1, 2022

Support plan

  • Which support plan is this issue covered by? (Community, Sponsor, Enterprise): Community
  • Currently blocking your project/work? (yes/no): No, but v1 didn't have this issue at the time, only when I updated did this fail
  • Affecting a production system? (yes/no): Yes

Context

  • Node.js version: v16.13.1
  • Release Line of Formidable (Legacy, Current, Next): Current & Next
  • Formidable exact version: v2.0.1, also tried latest v3
  • Environment (node, browser, native, OS): Node on Windows & Linux
  • Used with (popular names of modules): NextJS

What are you trying to achieve or the steps to reproduce?

When I use the C# methods to generate a MultipartFormDataContent object, passing in a StringContent object includes a mimetype (Content-Type) for that header.

Based on formidible, it assumes any part with a mimetype is a file, which doesn't seem to be the case for some core libraries (according to Microsoft's implementation).

I'm not entirely sure which is the expected and correct implementation based on the RFC (sorry, not enough time to research this myself). I was wondering, back in v1 before I upgraded to v2, this seemed to have been accounted for and so I wonder if this upgrade introduced a bug?

What was the result you got?

Using form.parse, err was empty, fields was not populated, and files had 2 entries, one which was the file and one which was a string with a mimetype of text/plain; charset=utf-8. When I removed the header from the string part manually in the client, fields was populated as expected (1 entry) and so was files (also 1 entry).

What result did you expect?

It should maybe be smarter in recognizing the mimetype, or maybe outlining in the documentation that the mimetype should be null for non-file parts. Or maybe, Microsoft is the one in the wrong and there is no action to be performed :)

@sabihismail sabihismail added the bug label Jan 1, 2022
@GrosSacASac
Copy link
Contributor

Files is more a concept than a thing on the web platform. What you do with the incoming data is up to you. You could, after form.parse handle files with text/plain like fields.


this is how we think it is a field
if (!part.mimetype) {

and this is how it was done in 1.2.6
if (part.filename === undefined)

In the usual case a file has both filename and mimetype when upload through a regular web html form.


Should we change something ?

@sabihismail
Copy link
Author

Hey, thanks for the quick response. I think for this it's worth having it outlined on the documentation and that would be sufficient enough of a solution, specifically the criteria for when a field is populated. It took myself a while to figure out how fields was populated since I was unfamiliar with the codebase.

Thanks!

@GrosSacASac
Copy link
Contributor

20b4dd2

Good enough ?

@sabihismail
Copy link
Author

Yes thank you!

@tunnckoCore
Copy link
Member

tunnckoCore commented Jan 17, 2022

Yea, in v1 it was part.filename == '', then i tried to shorten it and make some more sense or refactor, and changed it to !part.filename then some weird bugs appear and failing tests. So i thought.. okay, in the general case, fields "don't have" mimetype and if there is it most probably is file and that's the expected.

So.. yea, that was the case. Good adding it to the changelog, we probably missed a lot of things in the long process. Thank god we published it as v2, so it's acceptable there can be breaking changes, haha.

@GrosSacASac great, we may add it to the readme docs somewhere too?

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

No branches or pull requests

3 participants