Fixing minimal api header array binding #55803
Conversation
…reated correctly with tests
…om/MattyLeslie/aspnetcore into fixingMinimalApiHeaderArrayBinding
dotnet-issue-labeler
bot
added
the
area-networking
dotnet-policy-service
bot
added
the
community-contribution
…n `SplitAndTrim(string[] values)`
MattyLeslie
requested review from
BrennanConroy,
halter73 and
captainsafia
as code owners
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
/azp run |
dotnet-policy-service
bot
removed
the
pending-ci-rerun
|
Azure Pipelines successfully started running 3 pipeline(s). |
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
@BrennanConroy, may I please request a review on this. |
Co-authored-by: Brennan <brecon@microsoft.com>
Co-authored-by: Brennan <brecon@microsoft.com>
| } | ||
| private static void EmitHeaderValueParsing(CodeWriter codeWriter) | ||
| { | ||
| codeWriter.WriteLine("var headerParameters = handler.Method.GetParameters().Where(p => p.GetCustomAttribute<FromHeaderAttribute>() != null).ToList();"); |
There was a problem hiding this comment.
When emitting code at compile-time, we want to do the discovery of parameters like this via static analysis.
You'll observe that in other parts of the Request Delegate Generator (RDG) code, we generate an EndpointParameter object that we construct after statically analyzing the arguments then we use the information in that data model to emit the generated code. We don't need to re-compute things like the headerName dynamically like this.
You can find the pre-existing code where we emit the binding logic for header parameters here.
There was a problem hiding this comment.
Thank you for the review, I will be working on this.
|
@MattyLeslie Thanks for taking a stab at this! I posted some comments about the approach to take for this change in RDG based on the existing implementation. Let me know if you have questions about it. |
| } | ||
|
|
||
| var result = new List<string>(); | ||
| Span<Range> parts = stackalloc Range[16]; |
There was a problem hiding this comment.
This doesn't seem correct. If there are more than 16 values in a single header it will miss them. Maybe just revert to the previous code for now. We can look at improving the perf later.
There was a problem hiding this comment.
Thanks @BrennanConroy, I will revert that, I am going to mark this as draft as I'm still working on this.
|
@MattyLeslie Would you please let me know if you are going to work on this fix? |
Yeah if your PR works they're welcome to take it. I just haven't had the time I had when I started it. |
Ensuring Minimal APIs and controllers treat header arrays the same.
Summary:
Passing comma-separated list of values to the header was treated differently by controller action and Minimal API endpoint during binding to array. For controller action they were parsed as separate values while for Minimal APIs - as single one. This PR aimed to ensure they both were parsed as separate values by adapting the binding logic within
RequestDelegateFactoryChanges:
BindParameterFromExpressionnow handles comma-separated values appropriately and returns them as separate values.RequestDelegateFactoryTeststo ensureRequestDelegate_ShouldHandleSingleHeaderValue()RequestDelegateFactoryTeststo ensureRequestDelegate_ShouldHandleCommaSeparatedHeaderValuesFixes #54978 by ensuring minimal API's and Controllers treat header arrays the same.