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

fix: consume undisturbed request stream as-is #39

Merged
merged 8 commits into from
Oct 25, 2024

Conversation

c0per
Copy link
Contributor

@c0per c0per commented Dec 30, 2023

In current version, multipart form request will throw a parsing error.

This is because createMiddleware() treat req.body as the raw body and pass it to new Request().
But req.body is generated by express.json(), which only parse requests with content-type application/json and leave req.body undefined for other requests (like multipart form).

In the PR, req.body is prepared by express.raw({ type: '*/*' }). It will get a Buffer as the raw body for all the requests and pass it directly to new Request(). json, form, even other binary content types will be untouched by express and handled by user with Request API.

Use Express.js instead of HttpServer from open-draft since open-draft
will automatically convert json request body to Object.

With Express.js and express.raw(), everything works fine.
@c0per
Copy link
Contributor Author

c0per commented Dec 30, 2023

This will unfortunately be a breaking change. The must-need express.json() will be replaced by a express.raw({ type: '*/*' }).

I didn't think of a way to avoid a middleware. Express.js doesn't provide any way to access the raw request body but using express.raw()

@LeBenLeBen
Copy link
Contributor

Just spent 2 hours trying to debug why calling req.formData() like in the docs was returning Unexpected end of multipart data 😅 Thank you for contributing.

Let me know if I can be of any help to get this merged and released.

src/server.ts Outdated Show resolved Hide resolved
c0per and others added 2 commits March 27, 2024 09:38
Co-authored-by: Benoît Burgener <benoit.burgener@gmail.com>
@c0per
Copy link
Contributor Author

c0per commented Mar 27, 2024

I think a larger default size limit is a good idea.
I also changed the createServer() function to accept bodyParser options (optionally).

So for createServer() users, it's not a breaking change.
for createMiddleware(), it is breaking. They need to add app.use(express.raw()) manually.

@LeBenLeBen Please review these changes, wish we can merge this one. Thanks :)


const app = express()

app.use(express.raw({ type: '*/*' }))
Copy link
Member

@kettanaito kettanaito Mar 27, 2024

Choose a reason for hiding this comment

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

What do you think if we bake in express.raw() into createMiddleware() itself?

Applying it to the entire server is too much anyway since people can attach handlers-based routes to an existing Express server.

In other words, having the raw body parser is a prerequisite of the middleware. So it must be defined there, next to the middleware.

@kettanaito kettanaito changed the title Fix multipart/form-data request fix: parse multipart/form-data requests correctly Mar 27, 2024
@ernestostifano
Copy link

Hello guys, I got this error today: TypeError: Failed to parse body as FormData..

Is there anything I can do to help?

@ernestostifano
Copy link

I've just created an issue to cross-reference this PR: #47

@rwdevops999
Copy link

Still not solved. Going the Nock way.

@iivo-k
Copy link

iivo-k commented Oct 10, 2024

Any news about this issue, is a fix still in the pipeline or should this be considered abandoned? Mock server spawned with http-middleware only seems to handle json.

@ernestostifano
Copy link

@rwdevops999, @iivo-k, we did this in the meanwhile: #47 (comment)

@kettanaito kettanaito changed the title fix: parse multipart/form-data requests correctly fix: parse multipart/form-data requests Oct 25, 2024
@kettanaito kettanaito changed the title fix: parse multipart/form-data requests fix: consume undisturbed request stream as-is Oct 25, 2024
@kettanaito
Copy link
Member

Update

Instead of accounting for individual request content types, I'm creating a ReadableStream from whichever req stream is present. This way, we delegate the body parsing to the Request instance itself. I'm also checking if the request stream hasn't been parsed already, like when you have a custom body parser in your own middleware (req.readable). If it has been, the parsed req.body will be used as the request body.

I'm making exception for application/json streams because their parsed value will be an object while Request expects its body to be a string.

@kettanaito kettanaito merged commit 13cee76 into mswjs:main Oct 25, 2024
1 check passed
@kettanaito
Copy link
Member

Released: v0.10.2 🎉

This has been released in v0.10.2!

Make sure to always update to the latest version (npm i @mswjs/http-middleware@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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.

6 participants