Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

skip content-length on DELETE and OPTIONS methods with no body #32259

Merged
merged 9 commits into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/System.Net.Http/src/System/Net/Http/HttpMethod.cs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ internal bool MustHaveRequestBody
// Normalize before calling this
Debug.Assert(ReferenceEquals(this, Normalize(this)));

return !ReferenceEquals(this, HttpMethod.Get) && !ReferenceEquals(this, HttpMethod.Head) && !ReferenceEquals(this, HttpMethod.Connect);
return !ReferenceEquals(this, HttpMethod.Get) && !ReferenceEquals(this, HttpMethod.Head) && !ReferenceEquals(this, HttpMethod.Connect) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a change request just an observation, you could use is here to make this cleaner.
return !(this is HttpMethod.Get || this is HttpMethod.Head || this is HttpMethod.Connect || this is HttpMethod.Options || this is HttpMethod.Delete);

Copy link

@samsosa samsosa Sep 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ReferenceEquals? Why not String.Equals()? Does this work with
var method = new System.Net.Http.HttpMethod("GET"); ?

... I see, .Normalize() does the magic!

!ReferenceEquals(this, HttpMethod.Options) && !ReferenceEquals(this, HttpMethod.Delete);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Net.Http.Headers;
using System.Net.Test.Common;
using System.Threading;
using System.Threading.Tasks;

using Xunit;

namespace System.Net.Http.Functional.Tests
{
public class HttpRequestMessageTest
public class HttpRequestMessageTest : HttpClientTestBase
{
Version _expectedRequestMessageVersion = !PlatformDetection.IsFullFramework ? new Version(2,0) : new Version(1, 1);

Expand Down Expand Up @@ -215,6 +217,43 @@ public void ToString_DefaultAndNonDefaultInstance_DumpAllFields()
"}", rm.ToString());
}

[Theory]
[InlineData("DELETE")]
[InlineData("OPTIONS")]
[InlineData("HEAD")]
public async Task HttpRequest_BodylessMethod_NoContentLength(string method)
{
if (IsWinHttpHandler || IsNetfxHandler || IsUapHandler)
{
// Some platform handlers differ but we don't take it as failure.
return;
}

using (HttpClient client = new HttpClient())
{
await LoopbackServer.CreateServerAsync(async (server, uri) =>
{
var request = new HttpRequestMessage();
request.RequestUri = uri;
request.Method = new HttpMethod(method);

Task<HttpResponseMessage> requestTask = client.SendAsync(request);
await server.AcceptConnectionAsync(async connection =>
{
List<string> headers = await connection.ReadRequestHeaderAsync();
foreach (string line in headers)
{
// There should be no Content-Length header.
Assert.False(line.StartsWith("Content-length:", StringComparison.InvariantCultureIgnoreCase));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use Assert.DoesNotContain pattern instead of Assert.False.

See example:

Assert.DoesNotContain(receivedRequest, line => line.StartsWith("Transfer-Encoding"));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It produces better output when things fail. And it is pattern we already use elsewhere in the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Existing pattern also exit but I can change it as you suggested.

}

await connection.SendResponseAsync();
await requestTask;
});
});
}
}

#region Helper methods

private class MockContent : HttpContent
Expand Down