Okhttp: MockWebServer should allow GET with a body

[lazystone]

okhttp3.internal.http.HttpMethod forbids having body on GET request:

fun permitsRequestBody(method: String): Boolean = !(method == "GET" || method == "HEAD")

While I agree that this is a bad practice to use GET with a body, problem is that we have to work with 3rd party APIs which no one will ever change and they do demand GET with a body.

In this case it's a huge show-stopper for us :(

I propose to either remove GET from permitsRequestBody or at least make it somehow configurable.

Or, at least remove this validation from MockWebServer.

[yschimke]

This is by design and to spec. See also https://github.com/square/okhttp/issues/5803

Sorry we can't help here. OkHttp is quite opinionated in following the Spec (doing things correctly) and following the browsers (Firefox, Chrome) and very common servers (Nginx, Apache etc) when it's ambiguous.

[lazystone]

@yschimke could you point out to the specs? I think that they do not forbid GET+body explicitly.

Elasticsearch(and they actually prefer GET+body) say that RFC 7231 does not forbid get+body.

So, saying that it's forbidden by spec is not true I believe.

And don't get me wrong - I don't like GET+body, but forbidding something which is allowed by specs making okhttp unusable in some cases.

There are other ways to encourage best practices as Spring Framework does for example - they do not allow body in their builder when you use .get() but in order to allow user provide body on GET request they have another API call .method(HttpMethod.GET).body(...).

[yschimke]

Sorry. But no current plans to support this.

[swankjesse]

I'm under the impression that the weird Elasticsearch endpoint that uses a GET body is also available as a POST.

[lazystone]

Elasticsearch was just an example - there are other APIs which use the same approach. As I mentioned before - it's not forbidden by specs to use GET+body.

Also fixing this is one line change in okhttp3.internal.http.HttpMethod

[yschimke]

@lazystone It's a deliberate opinionated decision. Not something that was missed.

[lazystone]

That's Ok, but then please stop saying that it's forbidden by specs, because it's not true.

If you deliberately restrict some functionality - be fair. Say that you forbid it because you don't like it, not because it violates HTTP specs.

[swankjesse]

Also fixing this is one line change in okhttp3.internal.http.HttpMethod

Changing Boolean to support a new probably value is also a one line change!

We don't want GET to have bodies because it adds more work for people using it:

• the cache needs to change to not cache GET requests with body
• interceptors can no longer assume GET requests lack a body for caching and logging
• URLs for GET requests no longer uniquely identify the request
• Existing code that includes a length header like Transfer-Encoding: chunked in GET requests will require the corresponding bytes or crash

One of the major benefits of the GET request is that it doesn't include a body! And that makes it easier to work with. That the Elasticsearch maintainers don't appreciate this is their foolishness. I won't subject OkHttp’s users to that unnecessary complexity so they can be cute.

[lazystone]

As I said above:

That's Ok, but then please stop saying that it's forbidden by specs

I'm OK with maintainers decision to not support something. I do understand that if you considered GET+body as impossible then there are might be some dark corners in the code that need to be fixed in order to support this. That is OK.

If you are honest about that, then you can add little "known limitations" somewhere on this page.

But just closing tickets with "forbidden by specs" or "it's by design" is wrong I believe, because first of all it's not forbidden and then you don't give a chance for somebody else to contribute.