-
-
Notifications
You must be signed in to change notification settings - Fork 958
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
The GET method cannot be used with a body #966
Comments
Got follows the RFC |
Also this is a duplicate issue |
@szmarczak Can you link to the RFC and the duplicate issue? We should probably document this more clearly in the readme. |
I searched for the exact error message but I guess it changed in v10. The only reason why I encountered the issue is because I don't think a client library needs to be this strict necessarily, especially if servers support it. There could also be a warning or an opt-in-flag. But for the sake of completeness, the HTTP/2 RFC does state that GETs don't have a body: https://tools.ietf.org/html/rfc7540#section-8.1.3 |
@sindresorhus I can't find the issue, but I remember there was one 100%. |
I guess we should look at the closed issues and pick the most common ones. Then add these to the FAQ. |
It's a balance. Node.js core is generally fast and loose and doesn't protect you against mistakes. We want to make something human-friendly. GET with a body is almost always a mistake. If enough people need this, we could consider an option to allow it, but it definitely doesn't make sense to allow it as default behavior. |
I'm not sure I agree that it's almost certainly a mistake, but I definitely support your reasoning. Thanks for taking the time to discuss this. |
My team has to integrate with some third party APIs which unfortunately require a body with a GET. Is it possible to work around this or are we stuck on version 9? |
@jakesjews I don't think this will be an option. As @timdp has already mentioned, HTTP2 doesn't support GET with a payload anyway. |
Nuance: the RFC doesn't support it. The RFC also wants you to keep the header size under 2,000 bytes but I've seen URLs way longer than that. Should got also disallow those? |
I just call this function then get exports.ipStack = ip => {
const query = {access_key: config.ipStackToken};
return got.get(
`http://api.ipstack.com/${ip}?${queryString.stringify(query)}`,
{
json: true,
timeout: config.vendorApiTimeout
}
).then(response => response.body);
};
|
Breaking backward compatibility without any option to return known behavior is not the best way, in my mind. Simple option, but I would not need to migrate the whole codebase. |
Please read the docs before complaining. |
I did, unfortunately, I cannot find any ability to get around that behavior. Based on https://github.com/sindresorhus/got/blame/master/readme.md#L188 and https://github.com/sindresorhus/got/blob/master/source/normalize-arguments.ts#L348 it looks impossible. What do I miss? |
@nordluf Unfortunately there's nothing you can do but to switch to POST instead. We understand your frustration, but this is done to make the behavior more reliable according to the RFC. @kelp404's example has got some inconsistencies: the |
Elastic Search requires the use of body with GET. Just an example where people get creative with their usages of HTTP. I think that a blanket restriction like that is quite weird, when it has been supported for so long by HTTP/1.1. |
Elasticsearch will actually happily accept POST as well because Elastic are aware of this discrepancy. |
@timdp , Elasticsearch statement about it is the following (emphasis mine):
That does not really matter, though, as Elasticserach is just an example. But looking into the docs, RFC 7237 says that "[it] is a product of the Internet Engineering Task Force (IETF). It represents the consensus of the IETF community. It has received public review and has been approved for publication by the Internet Engineering Steering Group (IESG)." It points to RFC7231, "Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content", which contains this excerpt about GET method (emphasis mine):
There is nothing — in the current documentation — that determines that GET bodies should be prohibited, specially when coming from a client library making the request. Therefore, the current behaviour has been determined by I would support a change in this behaviour, if possible. |
Not true, I’m afraid. As mentioned above, the more recent HTTP 2 RFC states:
|
So we should assume On the documentation, that is not specified and, on the comparison table, it actually mentions HTTP/2 support, which implies that it is not the main target, but an extra feature. |
That's correct. HTTP/2 support is still in the works. We don't allow sending body for GETs because that's undefined behavior. Some servers may reject the request, some may not. |
Doesn't that make sense to allow a user to decide if his server capable to serve the body in GET requests if HTTP/1.1 is in use? |
That's an anti pattern |
Which one is anti-pattern: use body in GET or allow to setup library behavior? What is you suggestion to do instead? I'm against making POST request to retrieve information. As of RFC (https://tools.ietf.org/html/rfc7231#section-4.3.1) POST semantics is - create or publish something. |
that one |
I'm going to lock this issue, the decision has been already made long ago. |
There is now an option to allow GET with a body. However, we still strongly recommend against using it unless you really have no other choice. https://github.com/sindresorhus/got/releases/tag/v10.6.0 |
Describe the bug
I ran into this when upgrading to [email protected].
I realize this is up for debate, but to my best knowledge, a lot of HTTP servers do support GET requests with a body. Elasticsearch, for example, heavily uses this. (They also provide an escape hatch with POST though.)
I don't think
got
should be the limiting factor, given that the underlying modules allow it.Checklist
The text was updated successfully, but these errors were encountered: