Fix for: Minimal APIs and controllers treat header arrays differently by smnsht · Pull Request #58251 · dotnet/aspnetcore

smnsht · 2024-10-05T09:25:42Z

Fix for: Minimal APIs and controllers treat header arrays differently #54978

Alter BindParameterFromProperty(...) method

Description

The root cause of the issue is in GetValueFromProperty(...) behavior - it returns an Expression that evaluates httpContext.Request.Headers[key]. What we want, is to call httpContext.Request.Headers.GetCommaSeparatedValues(key) when the source is "header" and [FromHeader] is attached to an array.

GetCommaSeparatedValues(key) is an extension method, implemented as ParsingHelpers.GetHeaderSplit(IHeaderDictionary, string)

Fixes #54978

BrennanConroy · 2024-10-07T17:12:33Z

There is an active PR for this that has gone through some review and iterations.

If you want to work on this bug you should check with the original PR author and see if they aren't going to work on their fix anymore so that you can take over.

smnsht · 2024-10-07T19:02:49Z

My bad.

I've asked him just now.

BrennanConroy · 2024-10-08T22:02:12Z

Thanks for checking with them, looks like you're good to go.

I'd recommend taking a look at their PR and the reviews that took place to get an idea of what the current change might be missing.

smnsht · 2024-10-09T14:05:50Z

I'd recommend taking a look at their PR and the reviews that took place to get an idea of what the current change might be missing.

I think a unit test with IEnumerable (other PR) should not be added. This code could not be compiled

dotnet 8.0 - screenshot

Confirmation would be appreciated

I see there are changes in RequestDelegateGenerator.cs in other PR. I don't know yet why this change is needed, but I noticed it was explicitly requested by the maintainer. My current understanding is that I should figure this out and properly implement it.

Confirmation would be appreciated

captainsafia · 2024-10-09T17:05:39Z

I see there are changes in RequestDelegateGenerator.cs in other PR. I don't know yet why this change is needed, but I noticed it was explicitly requested by the maintainer. My current understanding is that I should figure this out and properly implement it.

The Request Delegate Generator is the source generator implementation that compiles arbitrary delegates to RequestDelegates when you are using native AoT. The behavior between the source generator and non-source generator based implementations should be the same which is why I provided that feedback.

I think a unit test with IEnumerable (other PR) should not be added. This code could not be compiled

This error comes from our analyzers. We'll probably want to update them so they account for the expanded support.

Also, FWIW, I do think it is probably sufficient for us to just support arrays in this binding since that's what we do elsewhere for parameters.

smnsht · 2024-10-09T18:33:34Z

@captainsafia
Thanks. Going to fix that.

captainsafia

Looking good! Left a few comments inline.

captainsafia · 2024-10-22T23:42:10Z

You'll want to revert the changes in this submodule before we're able to merge this.

@captainsafia
done

captainsafia · 2024-10-22T23:43:56Z

+    [InlineData("""app.MapGet("/", ([FromHeader(Name = "q")] string[]? arr) => arr);""", "",      "[]")]
+    [InlineData("""app.MapGet("/", ([FromHeader(Name = "q")] string[]? arr) => arr);""", "a,b,c", "[\"a\",\"b\",\"c\"]")]
+    [InlineData("""app.MapGet("/", ([FromHeader(Name = "q")] int[]? arr) => arr);""",    "1,2,3", "[1,2,3]")]
+    public async Task RequestDelegateGenerator_FromHeader_CommaSeparatedValues(string source, string headerContent, string expectedBody)


If you move this test case to RequestDelegateCreationTests.Arrays.cs we'll get coverage for both compile-time and run-time code gen with a single test case.

@captainsafia
moved

captainsafia · 2024-10-22T23:45:10Z

                    var wasParamCheckFailure = false;
                    // Endpoint Parameter: p (Type = Microsoft.AspNetCore.Http.Generators.Tests.ParsableTodo[], IsOptional = False, IsParsable = True, IsArray = True, Source = Header)
-                    var p_raw = httpContext.Request.Headers["p"];
+                    var p_raw = httpContext.Request.Headers.GetCommaSeparatedValues("p");


Nice use of the extension method here!

captainsafia · 2024-10-22T23:53:00Z

-        BindParameterFromValue(parameter, GetValueFromProperty(property, itemProperty, key, GetExpressionType(parameter.ParameterType)), factoryContext, source);
+    private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, PropertyInfo itemProperty, string key, RequestDelegateFactoryContext factoryContext, string source)
+    {
+        var expressionType = GetExpressionType(parameter.ParameterType);


Are string arrays the only type that we need to handle here? Does MVC support serializing int[] from the header for example?

@captainsafia

Fixed

…e test case.

This reverts commit ba2d730.

This reverts commit b618f3c.

captainsafia

Looks good overall! Left one small comment inline that we can use as a way to rekick the stale build for this change.

captainsafia · 2025-01-06T17:29:46Z

+    {
+        var valueExpression = (source == "header" && parameter.ParameterType.IsArray)
+            ? Expression.Call(
+                typeof(ParsingHelpers).GetMethod(nameof(ParsingHelpers.GetHeaderSplit), BindingFlags.Public | BindingFlags.Static, [typeof(IHeaderDictionary), typeof(string)])!,


Nit: consider caching this at the top of the file as the other MethodInfo definitions.

I will do this in February.

MethodInfo caching done

Copilot

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

ghost added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 5, 2024

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Oct 5, 2024

martincostello added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Oct 5, 2024

smnsht changed the title ~~Fix for: Minimal APIs and controllers treat header arrays differently #55803~~ Fix for: Minimal APIs and controllers treat header arrays differently #54978 Oct 5, 2024

smnsht changed the title ~~Fix for: Minimal APIs and controllers treat header arrays differently #54978~~ Fix for: Minimal APIs and controllers treat header arrays differently Oct 5, 2024

smnsht marked this pull request as ready for review October 5, 2024 11:30

smnsht requested review from BrennanConroy, captainsafia and halter73 as code owners October 5, 2024 11:30

smnsht marked this pull request as draft October 9, 2024 18:33

Jayuu22 approved these changes Oct 18, 2024

View reviewed changes

smnsht marked this pull request as ready for review October 20, 2024 19:37

smnsht requested review from a team and wtgodbe as code owners October 20, 2024 19:37

mkArtakMSFT assigned adityamandaleeka Oct 22, 2024

captainsafia reviewed Oct 22, 2024

View reviewed changes

dotnet-policy-service Bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 30, 2024

smnsht added 6 commits November 2, 2024 11:09

dotnet#54978

0c2be2f

update EndpointParameterEmitter

9f1a7fa

update snapshots

99619bf

go back 4 commits in googletest

ba2d730

imporove BindParameterFromProperty(...) readability

d5222e1

get coverage for both compile-time and run-time code gen with a singl…

f890c72

…e test case.

smnsht added 3 commits November 2, 2024 11:18

merging main

b618f3c

Revert "go back 4 commits in googletest"

a2b3732

This reverts commit ba2d730.

Revert "merging main"

a021c08

This reverts commit b618f3c.

smnsht force-pushed the 54978 branch from 744b17b to a021c08 Compare November 2, 2024 09:38

smnsht and others added 3 commits November 3, 2024 00:47

Merge branch 'dotnet:main' into 54978

3c184ec

empty

99b5052

Merge branch '54978' of https://github.com/smnsht/aspnetcore into 54978

8b0829f

mkArtakMSFT assigned captainsafia and unassigned adityamandaleeka Dec 19, 2024

captainsafia approved these changes Jan 6, 2025

View reviewed changes

captainsafia requested a review from Copilot January 13, 2025 18:35

Copilot AI reviewed Jan 13, 2025

View reviewed changes

smnsht added 3 commits February 26, 2025 16:18

Merge branch 'main' into 54978

6871f56

Merge branch 'main' into 54978

9219c3e

add caching for GetHeaderSplit MethodInfo

4677be0

captainsafia merged commit 99c501c into dotnet:main Mar 4, 2025

dotnet-policy-service Bot added this to the 10.0-preview3 milestone Mar 4, 2025

smnsht deleted the 54978 branch March 11, 2025 12:24

Conversation

smnsht commented Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix for: Minimal APIs and controllers treat header arrays differently #54978

Description

Uh oh!

BrennanConroy commented Oct 7, 2024

Uh oh!

smnsht commented Oct 7, 2024

Uh oh!

BrennanConroy commented Oct 8, 2024

Uh oh!

smnsht commented Oct 9, 2024

Uh oh!

captainsafia commented Oct 9, 2024

Uh oh!

smnsht commented Oct 9, 2024

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smnsht Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smnsht Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

smnsht commented Oct 5, 2024 •

edited

Loading

smnsht Oct 23, 2024 •

edited

Loading

smnsht Oct 23, 2024 •

edited

Loading