Implement RouteHandlerServices.Map and use in RequestDelegateGenerator #46180

captainsafia · 2023-01-20T03:28:10Z

Addresses #46163

Adds a RouteHandlerServices.Map implementation per API review above
Remove SourceGeneratorEndpointDataSource implementation in RequestDelegateGenerator's output
Make IEndpointRouteBuilder.GetOrAddRouteEndpointDataSource internal to support invocation from RouteHandlerServices.Map
Update generated thunks to match signature expected by RouteHandlerServices.Map

src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs

JamesNK · 2023-01-20T03:49:35Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs

src/Http/Routing/src/Builder/RouteHandlerServices.cs

BrennanConroy · 2023-01-20T16:45:08Z

src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs

-                builder.Metadata.Add(new SourceKey{{StaticRouteHandlerModelEmitter.EmitSourceKey(endpoint)}});
+                if (options == null)
+                {
+                    return new RequestDelegateMetadataResult { EndpointMetadata = new List<object>().AsReadOnly() };


nit:
Replace new List<object>().AsReadOnly() with ReadOnlyCollection<object>.Empty, at least 3 places can be changed

TIL! Looks like this was added in 8.0? Very nice...

Oh, I just assumed it existed and saw that it did 😄
Didn't realize it's new!

I made the same comment, but you can use Array.Empty<object>() instead.

I ended up using ReadOnlyCollection<object>. In the interest of readability it does a better job of communicating the type passed to the initializer here and I don't think there's any memory implications with that singleton.

src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs

src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateGeneratorTests.cs

eerhardt

This is awesome work, @captainsafia.

I feel like I'm better able to give a decent code review now that I can see the generated code.

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

eerhardt · 2023-01-20T23:06:37Z

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

+
+        };
+
+        %GENERATEDCODEATTRIBUTE%


Similar comment as above. Do we need this attribute on these methods when the whole class is marked as generated?

I noodled on our use of the GeneratedCode attribute here a little bit more. Since the primary goal of the attribute is to make it easier to inspect if a type is codegenned from metadata, I think it makes sense to only include it in types that could be referenced from user code. Specifically, this would be:

The SourceKey definition

The strongly-typed Map overloads

I refactored the code to apply it to these places. Note, I opted to remove it from the static class containing the strongly-typed Map overloads since it's unlikely that user code ends up resolving that directly.

Let me know if you think this philosophy makes sense

I believe we want the GeneratedCodeAttribute on all the classes we generate in this file.

The reason is so things like analyzers, code-style, and code coverage tools don't warn/report things from inside this code. The devs can't change this code, so they shouldn't see tools talking about.

Even for the internal static class GenerateRouteBuilderEndpoints class. The Regex generator puts it on the partial methods, and not the class, because the class is partial to the user's code. So it would be incorrect to put it on the whole class, since the user writes code in this class too.

The Regex generator puts it on the partial methods, and not the class, because the class is partial to the user's code. So it would be incorrect to put it on the whole class, since the user writes code in this class too.

This insights makes sense to me.

Here's the modification I made:

Moved the GenerateRouteBuilderEndpoints to the code generated in the Microsoft.AspNetCore.Builder namespace to fix the global:: using issue

Added the GeneratedCode attribute to the GenerateRouteBuilderEndpoints class

So, all the classes we codegen inside that namespacce are marked.

Classes marked as file private are not marked, which I assume is fine.

Classes marked as file private are not marked, which I assume is fine.

I think they should be marked as well. That way they don't show up in code coverage reports, that devs can't do anything about.

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

eerhardt · 2023-01-23T16:52:28Z

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

+// </auto-generated>
+//------------------------------------------------------------------------------
+#nullable enable
+using global::Microsoft.AspNetCore.Http;


What's this using left for? I assume it shouldn't be needed, since everything outside of the namespace Microsoft.AspNetCore.Http.Generated should be fully qualified.

eerhardt · 2023-01-23T17:01:55Z

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

+
+        };
+
+        %GENERATEDCODEATTRIBUTE%


I believe we want the GeneratedCodeAttribute on all the classes we generate in this file.

The reason is so things like analyzers, code-style, and code coverage tools don't warn/report things from inside this code. The devs can't change this code, so they shouldn't see tools talking about.

Even for the internal static class GenerateRouteBuilderEndpoints class. The Regex generator puts it on the partial methods, and not the class, because the class is partial to the user's code. So it would be incorrect to put it on the whole class, since the user writes code in this class too.

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

eerhardt

LGTM. Thanks for fixing all my nits. :) I think the code gen looks really clean now.

eerhardt · 2023-01-23T19:09:04Z

...test/RequestDelegateGenerator/Baselines/MapGet_NoParam_StringReturn_WithFilter.generated.txt

+        private static void PopulateMetadata<T>(MethodInfo method, EndpointBuilder builder) where T : IEndpointMetadataProvider
+        {
+            T.PopulateMetadata(method, builder);
+        }
+
+        private static void PopulateMetadata<T>(ParameterInfo parameter, EndpointBuilder builder) where T : IEndpointParameterMetadataProvider
+        {
+            T.PopulateMetadata(parameter, builder);
+        }


I don't see these methods used. I assume they will be used in the future. Not a huge deal, but in the future we might want to only generate them if they are actually being used.

Yes 🙊 ... we'll eventually use this when we add support for populating metadata for types that implement the IEndpointMetadataProvider interfaces. I can remove it in the next PR...

dotnet#46180) * Implement RouteHandlerServices.Map and use in RequestDelegateGenerator * Tweak generated code * Remove warnings from generated code * Fix docstring for RouteHandlerServices.Map * Add baseline tests and fix global:: issue * Clean up generated code * Add HTTP verb caching and fix usings * Use FQN and remove GeneratedCode attribute

captainsafia requested review from BrennanConroy, Tratcher, halter73 and javiercn as code owners January 20, 2023 03:28

ghost added the area-runtime label Jan 20, 2023

captainsafia requested review from JamesNK, davidfowl and eerhardt January 20, 2023 03:31

JamesNK reviewed Jan 20, 2023

View reviewed changes

captainsafia added 3 commits January 19, 2023 23:37

Implement RouteHandlerServices.Map and use in RequestDelegateGenerator

43fba6d

Tweak generated code

72651e8

Remove warnings from generated code

6327fcf

captainsafia force-pushed the safia/map-rdsg branch from e2c1815 to 6327fcf Compare January 20, 2023 07:38

Fix docstring for RouteHandlerServices.Map

a20c8c7

Youssef1313 reviewed Jan 20, 2023

View reviewed changes

src/Http/Routing/src/Builder/RouteHandlerServices.cs Show resolved Hide resolved

build-analysis bot mentioned this pull request Jan 20, 2023

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

BrennanConroy reviewed Jan 20, 2023

View reviewed changes

eerhardt reviewed Jan 20, 2023

View reviewed changes

Add baseline tests and fix global:: issue

20730af

eerhardt reviewed Jan 20, 2023

View reviewed changes

Clean up generated code

b78c9ea

eerhardt reviewed Jan 23, 2023

View reviewed changes

Add HTTP verb caching and fix usings

26e81ae

eerhardt reviewed Jan 23, 2023

View reviewed changes

Use FQN and remove GeneratedCode attribute

8146bb4

eerhardt approved these changes Jan 23, 2023

View reviewed changes

captainsafia enabled auto-merge (squash) January 23, 2023 19:44

captainsafia merged commit c5a59c4 into main Jan 23, 2023

captainsafia deleted the safia/map-rdsg branch January 23, 2023 20:27

ghost added this to the 8.0-preview1 milestone Jan 23, 2023

eerhardt mentioned this pull request Mar 13, 2023

Create source generator for configuration binding dotnet/runtime#82179

Merged

layomia mentioned this pull request Mar 16, 2023

Improve formatting of config binder generator output dotnet/runtime#83538

Closed

layomia mentioned this pull request May 10, 2023

Capture remaining binding Bind & Get methods for source generator support dotnet/runtime#85843

Merged

2 tasks

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

Implement RouteHandlerServices.Map and use in RequestDelegateGenerator #46180

Implement RouteHandlerServices.Map and use in RequestDelegateGenerator #46180

Uh oh!

Conversation

captainsafia commented Jan 20, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants