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

Why does HttpClient in Core allow GET requests with bodies, while Framework version does not? #28135

Closed
joshfree opened this Issue Mar 16, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@joshfree
Copy link
Member

joshfree commented Mar 16, 2018

Opened on behalf of @IanKemp from dotnet/core#1333


@IanKemp writes -
As per the title, the HttpClient implementation between Core and Framework differs in this regard. Consider the following example:

using (var client = new HttpClient())
{
    var request = new HttpRequestMessage
    {
        RequestUri = new Uri("some url"),
        Method = HttpMethod.Get,
    };

    request.Content = new ByteArrayContent(Encoding.UTF8.GetBytes("some json"));

    request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/json");

    var result = client.SendAsync(request).Result;
    result.EnsureSuccessStatusCode();

    var responseBody = await result.Content.ReadAsStringAsync().ConfigureAwait(false);
}
  • In .NET Core (tested with 1.0 and 2.0), the above executes successfully.
  • In .NET framework (tested with 4.7.1, 4.6.1, 4.5), the above throws a ProtocolViolationException with the message Cannot send a content-body with this verb-type on the SendAsync call.

While I am very happy that Core allows this (technically correct, but unusual) request type, I am less happy that the Framework does not support it. Why does Core allow this why Framework does not? Is this intentional or an oversight? Is there somewhere where these differences/idiosyncrasies are documented?

(For another example of differing HTTP behaviour in Core vs Framework, see this issue.)


@AppBeat writes -
Interesting. According to this post: https://stackoverflow.com/questions/978061/http-get-with-request-body
standard does not explicitly forbid this. GET body should be ignored by server.


@IanKemp writes -
@AppBeat The server is free to ignore or accept the body; that should not prevent the client from sending a request that the server may ignore. It is ultimately up to the client to determine whether the server accepts requests like this.

A popular example of a product (server) that supports GET requests with bodies is Elasticsearch, specifically their REST query API. In particular, the section on that page "A GET Request with a Body?" explains their rationale (but note that they do also allow POST as a fallback option for clients that do not support this).


@AppBeat writes -
I didn't say that this should be removed from .NET Core :) Although this is not common (bad?) practice I think .NET Core version in this case is more correct than .NET Framework implementation.

I will try to test this on new managed implementation of SocketsHttpHandler which will probably be prefered HttpHandler in future for more consistent behaviour across all different platforms.
https://github.com/dotnet/corefx/tree/master/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler

I created new functional test for SocketsHttpHandler and it works as it should:

[Fact]
public async Task SendAsync_HttpGetWithPayload_Success()
{
    await LoopbackServer.CreateServerAsync(async (server, url) =>
    {
        string responseBody =
            "HTTP/1.1 200 OK\r\n" +
            $"Date: {DateTimeOffset.UtcNow:R}\r\n" +
            "Content-Length: 0\r\n" +
            "Connection: close\r\n" +
            "\r\n";

        using (HttpClient client = CreateHttpClient())
        {
            var request = new HttpRequestMessage
            {
                RequestUri = url,
                Method = HttpMethod.Get,
            };

            request.Content = new StringContent("{}", Encoding.UTF8, "application/json");
            Task<HttpResponseMessage> getResponseTask = client.SendAsync(request);
            await server.AcceptConnectionAsync(async connection =>
            {
                Task<List<string>> serverTask = connection.ReadRequestHeaderAndSendCustomResponseAsync(responseBody);
                await TestHelper.WhenAllCompletedOrAnyFailed(getResponseTask, serverTask);
            });

            Assert.True(getResponseTask.IsCompletedSuccessfully);
            var result = getResponseTask.Result;
            Assert.True(result.IsSuccessStatusCode);
        }
    });
}
@ghost

This comment has been minimized.

Copy link

ghost commented Mar 16, 2018

This maybe a breaking change with older versions of HttpClient, but it is useful. curl supports GET with body curl -d "stuff" -XGET http://site/path.
GET is idempotent, that means the contents of request shouldn't change the response: https://tools.ietf.org/html/rfc7231#section-4.2.2 suggests PUT, DELETE and safe methods are idempotent, and 4.2.1 says GET and HEAD are defined as safe methods. Which means it is upto the server to make sure RFC is respected and RFC does not impose any restriction on client to allow sending GET's body.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Mar 16, 2018

It would be good to know if SocketsHttpHandler allows it or not (it should for compat with curl/winhttp). If it does not, please file a new bug.

.NET Framework is separate (older) implementation based on Sockets as well. If it doesn't allow these request, it is likely a more strict RFC interpretation. Given that GET bodies are useless (they are ignored by servers) and that it is not a regression on .NET Framework, I don't see any reason to change .NET Framework -- there is easy workaround and the scenario is very corner-case.

Closing as question answered. If you disagree feel free to reopen / ping me. Thanks!

@karelz karelz closed this Mar 16, 2018

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Mar 16, 2018

It would be good to know if SocketsHttpHandler allows it or not (it should for compat with curl/winhttp).

It does.

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Mar 16, 2018

From RFC7231:
https://tools.ietf.org/html/rfc7231#page-24

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.

So, either way is ok. Servers might reject a GET request with a body. The design picked in .NET Core was to be flexible and allow this since the RFC doesn't explicitly forbid it.

@karelz karelz added this to the 2.1.0 milestone Mar 18, 2018

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