Add initial parameter support to RequestDelegateGenerator #46407

halter73 · 2023-02-02T01:40:11Z

The only supported EndpointParameterSources are initially "SpecialTypes" like HttpRequest and CancellationToken.

Partially address #46275

src/Http/Http.Extensions/gen/DiagnosticDescriptors.cs

captainsafia · 2023-02-02T01:50:01Z

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Endpoint.cs

+
+        if (!operation.TryGetRouteHandlerPattern(out var routeToken))
+        {
+            Diagnostics.Add(DiagnosticDescriptors.UnableToResolveRoutePattern);


I'm inclined to have each component of the model maintain its own diagnostics. As we build up the diagnostics we emit, it'll make it easier to warn for specific scenarios that might not be immediately accessible at the top-level.

For example, let's say that we find a TryParse method on a parameter that doesn't have the correct signature. It would be easier to read and track if we were to capture that diagnostic when we're constructing the parameter instead of bubbling it up and capturing them all at the top.

The downside is it does make the code to aggregate them all when we emit a little honky but that seems worth it for the benefits of having diagnostics localized to where they would be thrown (similar to an exception).

I think it would be better to give EndpointParameter a backreference to Endpoints.Diagnostics if we want to capture the diagnostic while constructing the parameter. I just didn't like greedily creating a list for each small component even though it's not that expensive. The aggregation and repeated declarations were kind of verbose too as you say.

src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

captainsafia · 2023-02-06T20:00:35Z

@halter73 Thoughts on rebasing this and taking it out of draft?

halter73 · 2023-02-06T21:38:43Z

@halter73 Thoughts on rebasing this and taking it out of draft?

I'm doing that now.

mitchdenny · 2023-02-06T22:04:52Z

@halter73 Thoughts on rebasing this and taking it out of draft?

Was going to ask the same thing as I'm keen to riff off this but didn't want to get 3 layers deep in merge conflicts ;)

- The only supported EndpointParameterSources are initially "SpecialTypes" like HttpRequest and CancellationToken. # Conflicts: # src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs # src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Endpoint.cs # src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointResponse.cs # src/Http/Http.Extensions/gen/StaticRouteHandlerModel/StaticRouteHandlerModel.Emitter.cs # src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateGeneratorTestBase.cs

mitchdenny · 2023-02-07T03:45:35Z

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/StaticRouteHandlerModel.Emitter.cs

@@ -68,7 +65,7 @@ public static string EmitRequestHandler(this Endpoint endpoint)
                    {{handlerSignature}}
                    {
                        {{setContentType}}
-                        {{resultAssignment}}{{awaitHandler}}handler();
+                        {{resultAssignment}}{{awaitHandler}}handler({{endpoint.EmitArgumentList()}});


Probably something that will need to change when I start working on the parsing, because this list of arguments will need to be mapped to a set of local variables which are injected beforehand to store the results of the appropriate TryParse(...) operation.

mitchdenny

This looks good to me. I think it is something that would be good to get in and start iterating on.

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointResponse.cs

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/InvocationOperationExtensions.cs

mitchdenny · 2023-02-07T04:36:46Z

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/WellKnownTypeData.cs

@@ -7,19 +7,39 @@ internal static class WellKnownTypeData
 {
    public enum WellKnownType


I made the other classes internal even though I don't think it matters much in a source generator/analzyer. This one I didn't bother with since it's inside of internal static class WellKnownTypeData, and I want to move this to the top level in another PR anyway.

brunolins16 · 2023-02-07T05:56:59Z

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

+        }
+    }
+
+    // TODO: Handle special form types like IFormFileCollection that need special body-reading logic.


Just for curiosity, what is your idea about form types? Probably you'll need to track them to call ReadFormAsync somewhere, right?

Yeah. It will be similar to JSON bodies, BindAsync, and even TryParse handling.

halter73 · 2023-02-07T18:54:48Z

There's definitely follow up work, but I think this is in a good state to merge. Can someone please approve this PR?

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Endpoint.cs

halter73 · 2023-02-07T21:11:12Z

I noticed a bug when testing with the MinimalSample where we were using the StartLinePosition instead of EndLinePostion. This could lead to KeyNotFoundException's at startup like the following.

Unhandled exception. System.Collections.Generic.KeyNotFoundException: The given key '(C:\dev\dotnet\aspnetcore\src\Http\samples\MinimalSample\Program.cs, 15)' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Microsoft.AspNetCore.Http.Generated.<GeneratedRouteBuilderExtensions_g>F7174CD3CED625D68146A5AD64E1117AD1DCB273ED9FB961765E672E7C9CE3350__GeneratedRouteBuilderExtensionsCore.MapCore(IEndpointRouteBuilder routes, String pattern, Delegate handler, IEnumerable`1 httpMethods, String filePath, Int32 lineNumber) in C:\dev\dotnet\aspnetcore\src\Http\samples\MinimalSample\Microsoft.AspNetCore.Http.Generators\Microsoft.AspNetCore.Http.Generators.RequestDelegateGenerator\GeneratedRouteBuilderExtensions.g.cs:line 380
   at Microsoft.AspNetCore.Builder.GenerateRouteBuilderEndpoints.MapGet(IEndpointRouteBuilder endpoints, String pattern, Func`1 handler, String filePath, Int32 lineNumber) in C:\dev\dotnet\aspnetcore\src\Http\samples\MinimalSample\Microsoft.AspNetCore.Http.Generators\Microsoft.AspNetCore.Http.Generators.RequestDelegateGenerator\GeneratedRouteBuilderExtensions.g.cs:line 47
   at Program.<Main>$(String[] args) in C:\dev\dotnet\aspnetcore\src\Http\samples\MinimalSample\Program.cs:line 15

I added the fix along with a regression test to this already-approved PR with 258d799 (#46407).

halter73 requested review from mitchdenny, brunolins16 and captainsafia February 2, 2023 01:40

halter73 changed the title ~~Add support for response writing to RequestDelegateGenerator~~ Add initial parameter support to RequestDelegateGenerator Feb 2, 2023

captainsafia reviewed Feb 2, 2023

View reviewed changes

mitchdenny reviewed Feb 2, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs Show resolved Hide resolved

mkArtakMSFT 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 Feb 2, 2023

Base automatically changed from safia/rdg-responses to main February 3, 2023 01:40

halter73 force-pushed the halter73/46275-p1 branch from 772352f to 63bc01f Compare February 7, 2023 01:56

halter73 marked this pull request as ready for review February 7, 2023 01:57

halter73 requested review from Tratcher and BrennanConroy as code owners February 7, 2023 01:57

Use HashCode struct in EndpointParameter

ae9e6fc

mitchdenny reviewed Feb 7, 2023

View reviewed changes

Simplify tests

4164537

mitchdenny reviewed Feb 7, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointResponse.cs Outdated Show resolved Hide resolved

mitchdenny reviewed Feb 7, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/InvocationOperationExtensions.cs Outdated Show resolved Hide resolved

mitchdenny reviewed Feb 7, 2023

View reviewed changes

brunolins16 reviewed Feb 7, 2023

View reviewed changes

s/public/internal

bba095c

captainsafia approved these changes Feb 7, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Endpoint.cs Outdated Show resolved Hide resolved

halter73 added 2 commits February 7, 2023 13:05

fixup

141fad8

Use StartLinePosition instead of EndLinePostion

258d799

halter73 enabled auto-merge (squash) February 7, 2023 21:12

halter73 merged commit c21b67c into main Feb 7, 2023

halter73 deleted the halter73/46275-p1 branch February 7, 2023 22:53

ghost added this to the 8.0-preview2 milestone Feb 7, 2023

		@@ -7,19 +7,39 @@ internal static class WellKnownTypeData
		{
		public enum WellKnownType

Add initial parameter support to RequestDelegateGenerator #46407

Add initial parameter support to RequestDelegateGenerator #46407

Uh oh!

Conversation

halter73 commented Feb 2, 2023

Uh oh!

Uh oh!

captainsafia Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

halter73 Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

captainsafia commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Feb 6, 2023

Uh oh!

mitchdenny commented Feb 6, 2023

Uh oh!

mitchdenny Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

mitchdenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mitchdenny Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

halter73 Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

brunolins16 Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

halter73 Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

halter73 commented Feb 7, 2023

Uh oh!

Uh oh!

halter73 commented Feb 7, 2023

Uh oh!

Uh oh!

captainsafia commented Feb 6, 2023 •

edited

Loading