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

java.lang.IllegalArgumentException: method GET must not have a request body. #3154

Closed
1 of 3 tasks
hex6ng opened this issue Feb 5, 2017 · 21 comments
Closed
1 of 3 tasks

Comments

@hex6ng
Copy link

hex6ng commented Feb 5, 2017

What kind of issue is this?

  • Question. This issue tracker is not the place for questions. If you want to ask how to do
    something, or to understand why something isn't working the way you expect it to, use Stack
    Overflow. https://stackoverflow.com/questions/tagged/okhttp

  • Bug report. If you’ve found a bug, spend the time to write a failing test. Bugs with tests
    get fixed. Here’s an example: https://gist.github.com/swankjesse/981fcae102f513eb13ed

  • Feature Request. Start by telling us what problem you’re trying to solve. Often a solution
    already exists! Don’t send pull requests to implement new features without first getting our
    support. Sometimes we leave features out on purpose to keep the project small.

I'm trying to do a get request to Elasticsearch with a query body but OkHttp doesn't allow that. The query works with curl:

curl -XGET 'localhost:9200/fncd/event/_search?scroll=1m' -d '
{
    "query": {
	    "range" : {
	        "lastUpdated" : {
	            "gte": 1486184400000
	        }
	    }
    }
}
'
@swankjesse
Copy link
Member

OkHttp doesn’t support bodies for GET requests. For one reason, it makes caching too difficult to reason about.

@dmscheck
Copy link

dmscheck commented Mar 2, 2017

Will the change in PR #3038 fix this? Is there a timeframe on when that will be released?

@swankjesse
Copy link
Member

It’s unlikely we’ll ever support request bodies with GET.

No further action to take on this.

@alvico
Copy link

alvico commented Jul 25, 2017

Is there any reason why you did not plan to support this? It will be quite useful to use it, for instance when testing elasticsearch queries like in the example of the question. (Had the same problem xD)

@swankjesse
Copy link
Member

OkHttp is an opinionated HTTP client. By constraining what we support to what the specs allow, we’re able to keep things relatively simple and offer great features like smart caching.

Elasticsearch is wrong for exposing APIs that use GET requests with bodies. They should fix their API.

@fengshuzi
Copy link

please

@DCKcode
Copy link

DCKcode commented Jul 3, 2020

@swankjesse Could you please reopen this? While it was true that in the distant past the HTTP forbade the server from processing the body of GET requests, any such passages indicating this were specifically removed from RFC7230-7237 [it did always permit clients to send them!]. There is no argument based in the HTTP RFCs not to allow this.

See also: https://stackoverflow.com/a/983458/884848

@swankjesse
Copy link
Member

A payload within a GET request message has no defined semantics;
sending a payload body on a GET request might cause some existing
implementations to reject the request.

OkHttp is one of those existing implementations!

Permitting GETS to carry a semantic-free request body comes at a significant cost.

  • Caching will behave unpredictably
  • Interceptors may crash or leak resources
  • Request bodies may interfere with pooled HTTP/1 connections, permitting request smuggling attacks.

Could you explain your use case? I’ve never heard a compelling use case other than “elastic thought it would be cool LOL”.

@DCKcode
Copy link

DCKcode commented Jul 4, 2020

Ah thanks - I understand the caching reason.

The thing Elastic does is not that unique I would say - sometimes being able to perform rich queries or even rich authorization structures in the body of the GET request can make an API more "developer friendly" (subjective, I know) without encoding the same information a request parameter or a header. In those cases "I'm performing a complicated query, it's in the request body" use cases, caching might be of lesser concern. I suppose that it would be reasonable behavior not to perform caching for GET requests that have a body.

One thing I don't understand here yet is why you bring up interceptors or request smuggling here, as those risks exist regardless of the HTTP method used?

@swankjesse
Copy link
Member

Code exists that assumes GET requests don’t have a body. If that invariant is changed, this code may no longer be correct.

Interceptors: an interceptor implementing a cache (not OkHttp's built-in cache) might not pay attention to the request body field (it’s not supposed to exist!) and treat all request bodies as equivalent with respect to caching. This would be bad.

Request smuggling: you implement a Java Servlet that takes the incoming servlet request and builds an OkHttp request from it, then forwards that on to an internal webservice. Previously those internal web services never had GETs with bodies; now they do. If those things don’t attempt to read and discard the request body, then you might be able to smuggle a 2nd request through the first request’s body.

Typically OkHttp’s invariants are there to prevent developers from doing incorrect things by accident. But they have another purpose; they allow developers to assume incorrect things aren’t going to get through when they aren’t directly creating the requests. If we change the rules, then code that was relying on those rules being enforced may break.

In some ways issue #4865 is similar. Allowing OkHttp to express which local network interface is used fixes a bug, but creates the possibility for many more.

@DCKcode
Copy link

DCKcode commented Jul 4, 2020

I may have misunderstood you before as you're not bringing up OkHttp specific concerns, but concerns about the surrounding ecosystem.

I respect the attitude towards the de facto state of things that GET requests may be somewhat surprising. If you want to take the position that it should be OkHttp's problem to solve the general (but incorrect) expectation that GETs don't have bodies, sure. Probably the best point of view from a stability perspective.

To me personally though, it's somewhat surprising that OkHttp goes to the length it currently does. Actively breaking GET requests with bodies in OkHttp feels out of place to me when all relevant specs allow it, real world use cases exist, and as far as I know this behavior is not shared by other common HTTP clients. It effectively makes OkHttp a "subset-of-HTTP client", effectively also making it opinionated towards how servers should behave. Again - I respect you guys are making different choices than other people - but the limitation seems unexpected and artificial to me.

@swankjesse
Copy link
Member

Yep, that’s an accurate description of the project. We don’t do the long tail of all features of every spec, but enough that makes the common stuff safe & easy. We also try hard to keep the project small and minimize dependencies.

Off the top of my head there’s a few other places where our opinions show up:

  • We lock down TLS beyond what the base platform permits.
  • We don’t support HTTP/2 over plaintext.
  • The only way to customize caching is via HTTP cache headers.

Looking at this list, it’s a lot of opinions that are shared by browsers. Which makes me wonder – can Chrome or Firefox do HTTP GETs with request bodies? That’d be something that might convince me we’re too far from the common path.

@DCKcode
Copy link

DCKcode commented Jul 4, 2020

Interesting question. It seems that the XMLHttpRequest spec specifies to actively ignore any body parameters attached to a GET request. So OkHttp is in line with the XHR spec at least in this regard.

If that's a standard to get parity for, then there goes the argument. It's worth noting that this standard stems from before the updates to the HTTP RFCs, but there you go.

@yschimke
Copy link
Collaborator

yschimke commented Jul 4, 2020

For me it's fundamentally that OkHttp is an opionated user agent, not a toolkit that aims to supports all use cases. For example, we follow the security changes that browsers make over time, such that each upgrade potentially breaks some clients because it forces people to upgrade security. It would be easier to try to keep everything working, but sometimes it's the right thing to do to put pressure for other systems to fix.

@chentianming11
Copy link

can use @HTTP(method = "get", path = "/user/get", hasBody = true), Use lowercase get to bypass restrictions!

@ZMobile
Copy link

ZMobile commented Jun 8, 2022

@swankjesse

A payload within a GET request message has no defined semantics;
sending a payload body on a GET request might cause some existing
implementations to reject the request.

OkHttp is one of those existing implementations!

Permitting GETS to carry a semantic-free request body comes at a significant cost.

  • Caching will behave unpredictably
  • Interceptors may crash or leak resources
  • Request bodies may interfere with pooled HTTP/1 connections, permitting request smuggling attacks.

Could you explain your use case? I’ve never heard a compelling use case other than “elastic thought it would be cool LOL”.

What if you're using GraphQL on the backend for queries specifically and don't like the Apollo client as it overcomplicates authenticating your requests? I just want a simple Graphql query format as so:

Screen Shot 2022-06-08 at 5 25 13 AM

Represented in retrofit as:
Screen Shot 2022-06-08 at 5 52 10 AM

Normally using GraphQL on the client side would necessitate the Apollo client rather than Retrofit, but I find if you're just using Graphql for queries and not mutations, and REST for the rest (specifically to utilize GraphQL's advantage over REST: avoiding over-fetching and under-fetching), retrofit should suffice, as I find using the Apollo client overcomplicates the implementation and authenticating requests. Additionally, I find Apollo's interceptors might clash with okhttp's interceptors if you have to use both (As they involve similar syntax).

To answer your question regarding what is an example of a proper use case for GET requests with bodies: GraphQL is a newer REST alternative which has queries that entirely consist of GET HTTP requests with bodies, and I nor any of GraphQL's users find that be bad architecture. Ideally I would like Retrofit to support GraphQL queries and mutations one day in addition to REST but in the meantime it helps to have it allow for GET requests with bodies.

I personally would recommend that you reopen this issue.

@yschimke
Copy link
Collaborator

yschimke commented Jun 8, 2022

Doesn't this contradict the GraphQL docs? https://graphql.org/learn/serving-over-http/#http-methods-headers-and-body

@ZMobile
Copy link

ZMobile commented Jun 8, 2022

@yschimke Those docs say one thing however on experimentation putting the query in the url parameters rather than the body of a GET request returns a "code 400 Bad Request" error:
Screen Shot 2022-06-08 at 6 03 14 AM

That being said, I've just found the same query can be performed with a POST request rather than a GET request:
Screen Shot 2022-06-08 at 5 57 13 AM

Which is what I suppose I'll be using for the time being. The only downsides are that this request takes longer to return a response than the equivalent GET request, and also I'd generally consider a GraphQL query to be more of a GET style request than a POST request, but I'll do whatever works.

@xibz
Copy link

xibz commented Aug 10, 2022

OkHttp is an opinionated HTTP client. By constraining what we support to what the specs allow, we’re able to keep things relatively simple and offer great features like smart caching.

Nice. Opinionated on a standard. lol

@brucehuang2
Copy link

a well-known open source library that is opinionated , closed, and ignores standard protocols

@b0r1ngx
Copy link

b0r1ngx commented Aug 29, 2023

If you wanna make get request with body, through OkHttp / Retrofit, use this working solution, from @chentianming11 upper:

Instead of @GET(...) / @POST(...)

Do @HTTP(method = "get", path = "/user/get", hasBody = true) - use lowercase get!

So it's start look like:

@HTTP(method = "get", path = "/user/get", hasBody = true)
suspend fun transcripts(@Body request: TranscriptsRequest): TranscriptsResponse

Perfectly works!

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