Skip to content

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76ed8e0
Avoid allocating FormFeature in HttpRequest.HasFormContentType
captainsafia Sep 20, 2023
5e33d72
Address feedback and fix back-compat
captainsafia Sep 20, 2023
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
37 changes: 14 additions & 23 deletions src/Http/Http/src/Features/FormFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Text;
using Microsoft.AspNetCore.Antiforgery;
using Microsoft.AspNetCore.Http.Metadata;
Expand All @@ -21,6 +20,7 @@ public class FormFeature : IFormFeature
private FormOptions _options;
private Task<IFormCollection>? _parsedFormTask;
private IFormCollection? _form;
private MediaTypeHeaderValue? _mediaTypeHeaderValue;

/// <summary>
/// Initializes a new instance of <see cref="FormFeature"/>.
Expand Down Expand Up @@ -71,8 +71,12 @@ private MediaTypeHeaderValue? ContentType
{
get
{
_ = MediaTypeHeaderValue.TryParse(_request.ContentType, out var mt);
return mt;
if (_mediaTypeHeaderValue != null)
{
return _mediaTypeHeaderValue;
}
_ = MediaTypeHeaderValue.TryParse(_request.ContentType, out _mediaTypeHeaderValue);
return _mediaTypeHeaderValue;
}
}

Expand All @@ -82,13 +86,12 @@ public bool HasFormContentType
get
{
// Set directly
if (Form != null)
if (_form != null)
{
return true;
}

var contentType = ContentType;
return HasApplicationFormContentType(contentType) || HasMultipartFormContentType(contentType);
return HttpExtensions.HasAnyFormContentType(_request.ContentType);
}
}

Expand Down Expand Up @@ -177,11 +180,11 @@ private async Task<IFormCollection> InnerReadFormAsync(CancellationToken cancell
// Some of these code paths use StreamReader which does not support cancellation tokens.
using (cancellationToken.Register((state) => ((HttpContext)state!).Abort(), _request.HttpContext))
{
var contentType = ContentType;
var contentType = _request.ContentType;
// Check the content-type
if (HasApplicationFormContentType(contentType))
if (HttpExtensions.HasApplicationFormContentType(contentType))
{
var encoding = FilterEncoding(contentType.Encoding);
var encoding = FilterEncoding(ContentType!.Encoding);
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't like that the ContentType property allocates a MediaTypeHeaderValue. IMO it should be a method.

Copy link
Member

Choose a reason for hiding this comment

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

The type is more reliable, methods just re-invent the parsers.

var formReader = new FormPipeReader(_request.BodyReader, encoding)
{
ValueCountLimit = _options.ValueCountLimit,
Expand All @@ -190,12 +193,12 @@ private async Task<IFormCollection> InnerReadFormAsync(CancellationToken cancell
};
formFields = new FormCollection(await formReader.ReadFormAsync(cancellationToken));
}
else if (HasMultipartFormContentType(contentType))
else if (HttpExtensions.HasMultipartFormContentType(contentType))
{
var formAccumulator = new KeyValueAccumulator();
var sectionCount = 0;

var boundary = GetBoundary(contentType, _options.MultipartBoundaryLengthLimit);
var boundary = GetBoundary(ContentType!, _options.MultipartBoundaryLengthLimit);
var multipartReader = new MultipartReader(boundary, _request.Body)
{
HeadersCountLimit = _options.MultipartHeadersCountLimit,
Expand Down Expand Up @@ -312,18 +315,6 @@ private static Encoding FilterEncoding(Encoding? encoding)
return encoding;
}

private static bool HasApplicationFormContentType([NotNullWhen(true)] MediaTypeHeaderValue? contentType)
{
// Content-Type: application/x-www-form-urlencoded; charset=utf-8
return contentType != null && contentType.MediaType.Equals("application/x-www-form-urlencoded", StringComparison.OrdinalIgnoreCase);
}

private static bool HasMultipartFormContentType([NotNullWhen(true)] MediaTypeHeaderValue? contentType)
{
// Content-Type: multipart/form-data; boundary=----WebKitFormBoundarymx2fSWqWSd0OxQqq
return contentType != null && contentType.MediaType.Equals("multipart/form-data", StringComparison.OrdinalIgnoreCase);
}

private bool ResolveHasInvalidAntiforgeryValidationFeature()
{
var hasInvokedMiddleware = _request.HttpContext.Items.ContainsKey("__AntiforgeryMiddlewareWithEndpointInvoked");
Expand Down
6 changes: 2 additions & 4 deletions src/Http/Http/src/Internal/DefaultHttpRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,8 @@ public override string? ContentType
set { Headers.ContentType = value; }
}

public override bool HasFormContentType
{
get { return FormFeature.HasFormContentType; }
}
public override bool HasFormContentType => HttpExtensions.HasAnyFormContentType(ContentType)
|| _features.Fetch(ref _features.Cache.Form, _features.Collection, collection => collection.Get<IFormFeature>()) != null;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just checking for the presence of the form feature? If present, it should check the form feature's HasFormContentType.

The current form feature should also take precedence if it exists.


public override IFormCollection Form
{
Expand Down
1 change: 1 addition & 0 deletions src/Http/Http/src/Microsoft.AspNetCore.Http.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<Compile Include="$(SharedSourceRoot)Debugger\HttpContextDebugFormatter.cs" LinkBase="Shared" />
<Compile Include="..\..\WebUtilities\src\AspNetCoreTempDirectory.cs" LinkBase="Internal" />
<Compile Include="..\..\..\Shared\Dictionary\AdaptiveCapacityDictionary.cs" LinkBase="Internal" />
<Compile Include="$(SharedSourceRoot)HttpExtensions.cs" LinkBase="Shared"/>
</ItemGroup>

<ItemGroup>
Expand Down
30 changes: 14 additions & 16 deletions src/Shared/HttpExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,28 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Http;

internal static class HttpExtensions
{
internal const string UrlEncodedFormContentType = "application/x-www-form-urlencoded";
internal const string MultipartFormContentType = "multipart/form-data";
private const string UrlEncodedFormContentType = "application/x-www-form-urlencoded";
private const string MultipartFormContentType = "multipart/form-data";

internal static bool IsValidHttpMethodForForm(string method) =>
HttpMethods.IsPost(method) || HttpMethods.IsPut(method) || HttpMethods.IsPatch(method);

internal static bool IsValidContentTypeForForm(string? contentType)
{
if (contentType == null)
{
return false;
}

// Abort early if this doesn't look like it could be a form-related content-type
internal static bool HasAnyFormContentType([NotNullWhen(true)] string? contentType)
=> HasApplicationFormContentType(contentType) || HasMultipartFormContentType(contentType);

if (contentType.Length < MultipartFormContentType.Length)
{
return false;
}
internal static bool HasApplicationFormContentType([NotNullWhen(true)] string? contentType)
{
// Content-Type: application/x-www-form-urlencoded; charset=utf-8
return contentType != null && contentType.Contains(UrlEncodedFormContentType, StringComparison.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

This is less precise / robust than the prior implementation. It's ok in the normal scenario, but it can cause strange random bugs for other inputs. E.g. this now matches "web-application/x-www-form-urlencoded-foo".

We have tools that can get the same precision as before without allocating:
https://github.com/dotnet/aspnetcore/blob/main/src/Shared/MediaType/ReadOnlyMediaTypeHeaderValue.cs

Copy link
Member Author

Choose a reason for hiding this comment

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

We have tools that can get the same precision as before without allocating:

Ah, perfect! This is exactly what I was looking for.

}

return contentType.Equals(UrlEncodedFormContentType, StringComparison.OrdinalIgnoreCase) ||
contentType.StartsWith(MultipartFormContentType, StringComparison.OrdinalIgnoreCase);
internal static bool HasMultipartFormContentType([NotNullWhen(true)] string? contentType)
{
// Content-Type: multipart/form-data; boundary=----WebKitFormBoundarymx2fSWqWSd0OxQqq
return contentType != null && contentType.Contains(MultipartFormContentType, StringComparison.OrdinalIgnoreCase);
}
}