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

HttpRequest.ContentLength is null when using TestHost #18463

Closed
benmccallum opened this issue Jan 20, 2020 · 13 comments · Fixed by #24107
Closed

HttpRequest.ContentLength is null when using TestHost #18463

benmccallum opened this issue Jan 20, 2020 · 13 comments · Fixed by #24107
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update.
Milestone

Comments

@benmccallum
Copy link
Contributor

benmccallum commented Jan 20, 2020

Describe the bug

⚠️Update: I'm no longer taking this approach and inspecting the Content-Length header's value, we found a much better way with help from David Fowler, but the issue is still valid. For reproducability, I've branched the commit at the time here.

Apologies if I'm completely wrong here. Over here I'm upgrading a middleware to 3.x and System.Text.Json.

I'm trying to optimize a deserialization from the HttpRequest.Body by pre-allocating a buffer using the shared array pool given HttpRequest.ContentLength. See here. Problem is, when our integration tests run, which use HttpClient.PostAsync(..., new StringContent(...)) to post to a TestHost, the Content-Length header is always null.

Is this is a limitation of the test host, or a bug? I guess I could write a unit test the specific internals of that function.

To Reproduce

  1. Clone PRbranch linked above.
  2. Put a breakpoint in GraphQLHttpMiddleware in the JsonContentType case of the switch statement.
  3. Test Explorer > Samples.Server.Tests (netcoreapp3.1) > right-click "Single_Query..." and Debug.
  4. Breakpoint will be hit and httpRequest.ContentLength will be null.

Further technical details

  • ASP.NET Core version: 3.1.0
  • Include the output of dotnet --info
    image
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and it's version: VS Community 2019 v16.4.3
@analogrelay
Copy link
Contributor

I think this would depend on what StringContent does. Looking at it, it doesn't set Content-Length, which means this is somewhat expected. Our general rule-of-thumb for using TestHost here is that if you are coupled directly to specific headers like Content-Length you need to set them in the test client code. On the server, in general, the only way to know for sure the length of the body is to read it (since the request could be a chunked request or a Connection: close request with no Content-Length header).

@analogrelay analogrelay added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Jan 22, 2020
@benmccallum
Copy link
Contributor Author

Sorry I've been absent, been dealing with SameSite cookie fun and haven't had a chance to think about this and then am on hols until Thursday.

How could I explicitly set the content length? I notice that if I do var content = new StringContent(...); that content.Headers.ContentLength (from the top of my head) had a value, so it's a little strange this doesn't get passed on through.

Will have more of a think about this next week.

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Jan 24, 2020
@analogrelay
Copy link
Contributor

How could I explicitly set the content length? I notice that if I do var content = new StringContent(...); that content.Headers.ContentLength (from the top of my head) had a value, so it's a little strange this doesn't get passed on through.

Hrm, that is interesting. There may be a bug for us to look at here. We'll see what we find. I'd definitely agree that if the content has a ContentLength we should try to propagate that.

Sorry I've been absent, been dealing with SameSite cookie fun

Oh we're certainly familiar with that ;). No worries!

@analogrelay
Copy link
Contributor

@Tratcher was saying that the ContentLength property itself is a bit of a trap here. If you dereference it, the HttpContent computes the length on-demand, but the actual Header isn't present in the header collection (until you set it).

Still, I think we can do something to fix this. It seems like there's some weird edge case stuff going on here that we can try to tidy up.

@analogrelay analogrelay added enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue labels Jan 24, 2020
@analogrelay analogrelay added this to the Next sprint planning milestone Jan 24, 2020
@ADustyOldMuffin
Copy link

Could I maybe take a whack at this if it's still available? Is the idea to set the header content-length from the ContentLength property if it's valid/set?

@Tratcher
Copy link
Member

Yeah, when copying request headers from HttpContent the Content-Length header needs to be checked separately. Give it a try.

@TheDruidsKeeper
Copy link

TheDruidsKeeper commented Apr 14, 2020

I'm running into this same issue, which unfortunately causes ServiceStack framework to not hydrate request dto's. In case it's useful to anyone else, I managed to work around the issue by doing the following on the WebHostBuilder:

new WebHostBuilder()
.ConfigureServices(services =>
{
	services.AddSingleton<IStartupFilter, UnitTestStartupFilter>();
});
[...]
	public class UnitTestStartupFilter : IStartupFilter
	{
		public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
		{
			return app =>
			{
				app.Use(async (context, next) => {
					context.Request.EnableBuffering();
					using (var ms = new MemoryStream())
					{
						await context.Request.Body.CopyToAsync(ms);
						context.Request.ContentLength = ms.Length;
					}
					context.Request.Body.Seek(0, SeekOrigin.Begin);
					await next.Invoke();
				});
				next(app);
			};
		}
	}

It's not ideal but works to get the content length set, which gets everything else working properly downstream.

@Tratcher
Copy link
Member

@TheDruidsKeeper that assumes the entire request body is read in one read. A better workaround would be to set the content length on the client.

@TheDruidsKeeper
Copy link

TheDruidsKeeper commented Apr 15, 2020

@Tratcher Apparently I had gone way off the deep end... I had found that on the client side the StringContent.Headers.ContentLength did actually have a value, so spent all my time looking server side. I did a bit more testing now and found that the easiest solution is to simply read the ContentLength property:

var reqMessage = new HttpRequestMessage(httpMethod, uri)
{
	Content = new StringContent(request.ToJson(), Encoding.UTF8, "application/json")
};
Console.WriteLine($"Length: {reqMessage.Content.Headers.ContentLength}");

This solution works perfectly (and is much better than my previous suggestion).

@Tratcher
Copy link
Member

@TheDruidsKeeper yeah, that's a known annoyance with HttpContent dotnet/runtime#16162. I contributed to that design flaw about 10 years ago so it's only fair that it comes back to haunt me now.

@benmccallum
Copy link
Contributor Author

benmccallum commented Apr 15, 2020

How could I explicitly set the content length? I notice that if I do var content = new StringContent(...); that content.Headers.ContentLength (from the top of my head) had a value, so it's a little strange this doesn't get passed on through.

@TheDruidsKeeper Confused me too ;) Sorry my comment wasn't more obvious. Perhaps should add to the desc.

@krispenner
Copy link

Same issue here, I'm going to add a couple keywords to this issue hoping that anyone Googling the problem will find this issue (and workaround) quicker than I did. Don't mean to spam this but just spent 5 hours searching the wrong terms until I realized it was related to the content length which then brought me directly to here.

HttpClient post to TestServer is missing body
HttpClient send to TestServer string content not working

Workaround:

private HttpContent CreateJsonContent(object obj)
{
    var json = SerializeJson(obj);
    var content = new StringContent(json, Encoding.UTF8, "application/json");

#pragma warning disable IDE0059 // Unnecessary assignment of a value
    // required due to https://github.com/dotnet/aspnetcore/issues/18463
    var contentLenth = content.Headers.ContentLength;
#pragma warning restore IDE0059 // Unnecessary assignment of a value

    return content;
}

@krispenner
Copy link

Possibly a better workaround, since I needed this elsewhere, I've moved it to a new class under my testing namespace.

    public class StringContentWithLength : StringContent
    {
        public StringContentWithLength(string content)
            : base(content)
        {
            EnsureContentLength();
        }

        public StringContentWithLength(string content, Encoding encoding)
            : base(content, encoding)
        {
            EnsureContentLength();
        }

        public StringContentWithLength(string content, Encoding encoding, string mediaType)
            : base(content, encoding, mediaType)
        {
            EnsureContentLength();
        }

        public StringContentWithLength(string content, string unvalidatedContentType)
            : base(content)
        {
            Headers.TryAddWithoutValidation("Content-Type", unvalidatedContentType);
            EnsureContentLength();
        }

        private void EnsureContentLength()
        {
#pragma warning disable IDE0059 // Unnecessary assignment of a value
            // required due to https://github.com/dotnet/aspnetcore/issues/18463
            var contentLenth = Headers.ContentLength;
#pragma warning restore IDE0059 // Unnecessary assignment of a value
        }
    }

@Tratcher Tratcher added this to the 5.0.0-rc1 milestone Jul 30, 2020
@Tratcher Tratcher added the Done This issue has been fixed label Jul 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Aug 30, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants