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

Allow parsing empty request bodies #895

Merged
merged 2 commits into from Apr 21, 2018
Merged

Conversation

nwoltman
Copy link
Contributor

Disallowing empty bodies was an optimization for parsing application/json requests (since JSON bodies are not valid if they are empty). However, now that the ContentTypeParser can be used to parse other content types, this has become a bug.

Fixes #894

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

Disallowing empty bodies was an optimization for parsing `application/json` requests (since JSON bodies are not valid if they are empty). However, now that the ContentTypeParser can be used to parse other content types, this has become a bug.
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.

Given that the original issue was about a DELETE, can you add a test for that as well?

LGTM on the rest of the code.

@jsumners
Copy link
Member

And what happens if an empty application/json payload is received?

Copy link
Member

@allevo allevo left a comment

Choose a reason for hiding this comment

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

What happens if the request has an empty body with a content-length header?

Copy link
Member

@allevo allevo left a comment

Choose a reason for hiding this comment

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

Sorry my bad.

LGTM

Copy link
Member

@allevo allevo left a comment

Choose a reason for hiding this comment

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

LGTM with @mcollina nit

@nwoltman
Copy link
Contributor Author

Given that the original issue was about a DELETE, can you add a test for that as well?

@mcollina Added.

And what happens if an empty application/json payload is received?

@jsumners The same thing as any other request with invalid JSON: JSON.parse() will throw.

What happens if the request has an empty body with a content-length header?

@allevo If the Content-Length is 0, it is a valid request. If it is a different number, this error is sent.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@delvedor delvedor added the bugfix Issue or PR that should land as semver patch label Apr 21, 2018
@nwoltman nwoltman merged commit 8c5e732 into fastify:master Apr 21, 2018
@nwoltman nwoltman deleted the fix-empty-body branch April 21, 2018 22:09
@lependu lependu mentioned this pull request Jul 7, 2018
4 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix Issue or PR that should land as semver patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants