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

Errors thrown on GET requests with bodies, despite them being allowed in the HTTP spec #670

Closed
jordantaylor2 opened this issue Oct 10, 2019 · 4 comments · Fixed by #703
Closed

Comments

@jordantaylor2
Copy link

jordantaylor2 commented Oct 10, 2019

In our use of fasthttp, we've encountered this issue as one of our downstream services supports gets with a body. I've been doing some reading and it seems that the spirit of the HTTP spec is that non-posts have no restrictions on having a body, but no logic should be done on the body of a non-POST request so checking its size to produce an error shouldn't be how its done. Rather, the body should just be passed along and not checked by the HTTP implementation.

As seen in the HTTP spec for GET requests, there is no restriction on e.g. a GET request having a body.

On other projects on GitHub, I've seen similar issues come up and I feel this comment states it well: postmanlabs/postman-app-support#131 (comment)

I think that it makes sense to avoid throwing an error on non-POST requests with a body based on the aforementioned. Would it be possible to change this behavior? It seems to be from here

@erikdubbelboer
Copy link
Collaborator

After thinking about it I'm in favor of changing this behavior and just always allowing a body.

@kirillDanshin @dgrr what do you guys think?

@tsingson
Copy link

5 years ago, my project in java that support GET with payload. sometime it's very useful.

@micross
Copy link

micross commented Nov 24, 2019

upset that fasthttp reject the message body from the GET request

@erikdubbelboer
Copy link
Collaborator

This should be fixed now in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants