Expand FromX support and add route vs. query param inference #46715

captainsafia · 2023-02-16T22:33:48Z

Part of #46275.

Add support for parameters with FromRoute and FromHeader attribute
- Includes support for Name argument on attributes
- Includes support for requiredness checks via nullability and default value checks
Add support for Name argument in FromQuery attribute
Add support for default value-based requiredness check on FromQuery attributes
Update tests to use SharedTypes instead of types in test source
Add support for inference for parameters between route and query values

mitchdenny · 2023-02-16T22:41:06Z

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

I'm starting to get a bit concerned about the tight coupling between EndpointParameterEmitter and EndpointParameter (and friends). It is starting to seem to me like we should just collapse the types together and just have a method on EndpointParameter to emit the block of code.

I was actually thinking the other way around. EndpointParameter should be strictly about processing syntax to set state on the parameter objects and EndpointParameterEmitter should handle constructing the blocks of code.

I'll add a commit to showcase what this might look like.

captainsafia · 2023-02-22T22:45:39Z

@dotnet/minimal-apis Can I get a review? 🙏🏽

...DelegateGenerator/Baselines/MapAction_ExplicitBodyParam_ComplexReturn_Snapshot.generated.txt

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

eerhardt · 2023-02-23T16:03:08Z

...Http.Extensions/test/RequestDelegateGenerator/RequestDelegateGeneratorIncrementalityTests.cs

-        var updatedSource = @"app.MapGet(""/"", ([FromBody] Todo? todo) => TypedResults.Ok(todo));";
+        var source = $"""app.MapGet("/", ([{typeof(FromBodyAttribute)}] {typeof(Todo)} todo) => TypedResults.Ok(todo));""";
+        var updatedSource = $"""
+#pragma warning disable CS8622


For my learning, why do we get this warning here?

This is a result of the bug outlined in #46622

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

eerhardt · 2023-02-23T16:22:30Z

...uestDelegateGenerator/Baselines/MapAction_ExplicitSource_SimpleReturn_Snapshot.generated.txt

+                        var value_raw = GeneratedRouteBuilderExtensionsCore.ResolveFromRouteOrQuery(httpContext, "value", options?.RouteParameterNames);
+                        var value_local = value_raw;


Why do we need value_local here?

This is meant to indicate where the TryParse code in #46696 would go. It mostly exists to make rebasing easier since we'll need to support TryParse on parameters from multiple sources.

...uestDelegateGenerator/Baselines/MapAction_ExplicitSource_SimpleReturn_Snapshot.generated.txt

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

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

eerhardt · 2023-02-23T16:36:53Z

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs


        // Preamble for diagnostics purposes.
        builder.AppendLine($"""
                        {endpointParameter.EmitParameterDiagnosticComment()}


(nit on existing code)

Does this need to be wrapped in a """ string? Why can't it just be:

builder.AppendLine(endpointParameter.EmitParameterDiagnosticComment());

Indentation! 😢

But see #46845 for some goodies.

- Add support for EmpyBodyBehavior check - Update source snapshots to implicit tests - Avoid codegenning `TryResolveRouteOrQuery` if not needed - Clean up test types

eerhardt

LGTM. I just had a few more minor comments.

eerhardt · 2023-02-24T16:27:07Z

src/Shared/RoslynUtils/SymbolExtensions.cs

+            if (namedArgument.Key == argumentName)
+            {
+                var routeParameterNameConstant = namedArgument.Value;
+                argumentValue = (T)routeParameterNameConstant.Value!;


What if the argument is set to null? Are we sure this will always be non null?

Ah, good call! Yes, it's possible to do something like [FromQuery(Name = null)] for example. I updated the implementation here and added a guard against this scenario.

Sidenote: I think this is a bug in the RDF, but we'll find out in #46838

...DelegateGenerator/Baselines/MapAction_ExplicitBodyParam_ComplexReturn_Snapshot.generated.txt

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

eerhardt · 2023-02-24T16:35:42Z

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

+            return false;
        }
-        return false;
+        isOptional |= fromBodyAttribute.TryGetNamedArgumentValue<int>("EmptyBodyBehavior", out var emptyBodyBehaviorValue) && emptyBodyBehaviorValue == 1;


Does this code still handle a custom IFromBodyMetadata attribute when the AllowEmpty property is set in the attribute?

I was a little iffy on whether or not we should support both. We use the custom FromBodyAttribute implementation in our RDF tests (ref) but I don't think this is a common pattern in user code.

I ended up adding it back since we're not in a position to support FromBody vs. FromService inference and might need to implement a FromBody attribute in our own source to avoid importing the Mvc namespace.

- Guard parameter name from attribute against null values - Support FromBody with AllowEmpty named argument

captainsafia · 2023-02-24T20:02:58Z

/azp run

azure-pipelines · 2023-02-24T20:03:14Z

Azure Pipelines successfully started running 3 pipeline(s).

captainsafia requested review from eerhardt and mitchdenny February 16, 2023 22:33

captainsafia requested review from BrennanConroy, Tratcher and halter73 as code owners February 16, 2023 22:33

Expand FromX support and add route vs. query param inference

9e15ee2

captainsafia force-pushed the safia/fromx-support branch from c5d5681 to 9e15ee2 Compare February 16, 2023 22:52

Fix incrementality test

27a71f6

mitchdenny reviewed Feb 19, 2023

View reviewed changes

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 20, 2023

Move code emissions out of parsing logic

4a2d762

captainsafia mentioned this pull request Feb 22, 2023

TryParse support for request delegate source generator #46696

Merged

8 tasks

eerhardt reviewed Feb 23, 2023

View reviewed changes

captainsafia added 2 commits February 23, 2023 13:32

Address feedback from peer review

d0f04e0

- Add support for EmpyBodyBehavior check - Update source snapshots to implicit tests - Avoid codegenning `TryResolveRouteOrQuery` if not needed - Clean up test types

Remove string interpolation in source under test

e47e69d

captainsafia force-pushed the safia/fromx-support branch from 802218a to e47e69d Compare February 24, 2023 04:33

eerhardt approved these changes Feb 24, 2023

View reviewed changes

captainsafia added 2 commits February 24, 2023 11:38

Address review feedback

99f9af9

- Guard parameter name from attribute against null values - Support FromBody with AllowEmpty named argument

Undo format change

c7b6fb1

build-analysis bot mentioned this pull request Feb 24, 2023

The type initializer for 'Microsoft.TeamFoundation.DistributedTask.Pipelines.Yaml.PipelineParser' threw an exception dotnet/arcade#12700

Closed

5 tasks

captainsafia merged commit 965a8df into main Feb 24, 2023

captainsafia deleted the safia/fromx-support branch February 24, 2023 22:53

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

		var value_raw = GeneratedRouteBuilderExtensionsCore.ResolveFromRouteOrQuery(httpContext, "value", options?.RouteParameterNames);
		var value_local = value_raw;

Expand FromX support and add route vs. query param inference #46715

Expand FromX support and add route vs. query param inference #46715

Uh oh!

Conversation

captainsafia commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia commented Feb 22, 2023

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!

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!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia Feb 24, 2023 • 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia commented Feb 24, 2023

Uh oh!

azure-pipelines bot commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

captainsafia Feb 24, 2023 •

edited

Loading