MapAction MVP #29878

halter73 · 2021-02-03T18:56:52Z

This is an early prototype for MapAction. This is a good time to provide feedback on names and project structure.

Before this can be merged we need to support more parameter binding attributes and add tests. Input and output formatters, model binding validation, Swagger/OpenApi support, MapAction filters and more will all come later.

Addresses #29684

SteveSandersonMS · 2021-02-03T19:07:37Z

src/Http/samples/HttpApiSampleApp/Startup.cs

+                [HttpPost("/EchoTodo")]
+                JsonResult EchoTodo([FromBody] Todo todo) => new(todo);
+
+                endpoints.MapAction((Func<Todo, JsonResult>)EchoTodo);


The cast is a bit weird here - do any of the language enhancements we've discussed help out with this kind of methodgroup-to-delegate conversion?

cc @jaredpar @MadsTorgersen

Brain is a bit fried today but not sure which enhancement we discussed that would fix this.

If this were written as a lambda I agree the discussions we've had would help. It would have a natural type, we'd synthesize a delegate, use that and it would become the type passed to MapAction(Delegate). This is a method group though, not a lambda. Don't recall us talking about enhancing those.

It's not outside the realm of possibilities, I just haven't thought about it a bunch yet.

This came up as well. The idea is that overloads aren't supported we didn't understand why this would be problematic.

Brain is feeling a bit better today: medium rare, no longer well done.

Yeah we did talk about this. It's the same idea: giving method groups a natural type. That lets them have a tie breaker in cases where a Delegate overload exists.

@jaredpar We should figure out what preview release of .NET 6 we could expect a version of this. We'll likely have something in preview2 and I don't expect we'd have anything that early in the compiler timeframe but we should target a release because this is pretty unusable in C# without it 😄

eng/SharedFramework.Local.props

src/Http/Http.Api.Abstractions/src/IResult.cs

src/Http/Http.Api/src/AwaitableInfo.cs

src/Http/Http.Api/src/MapActionEndpointRouteBuilderExtensions.cs

src/Http/Http.Api/src/MapActionExpressionTreeBuilder.cs

davidfowl · 2021-02-03T21:42:25Z

src/Http/Http.Api/src/MapActionEndpointRouteBuilderExtensions.cs

+        /// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
+        public static IEndpointConventionBuilder MapAction(
+            this IEndpointRouteBuilder endpoints,
+            Delegate action)


Linker unfriendly 😄 .

cc @eerhardt

What's linker unfriendly about the API? You can have APIs that take delegates, and the linker won't trim them away.

We do a ton of reflection on the delegate that's passed in.

The Expression stuff is probably going to get you into more trouble. I'm in the middle of annotating Linq.Expressions now, and it isn't pretty how much stuff is not trim compatible.

We're also considering Roslyn source generators as an alternative which I assume is more linker-friendly. @davidfowl has already started experimenting with this at https://github.com/davidfowl/uController/tree/aa8bcb4b30764e42bd72d326cb2718a4d4eaf4a9/src/uController.SourceGenerator.

Yeah, source generators are usually better, if possible

src/Mvc/Mvc.Core/src/Properties/AssemblyInfo.cs

src/Mvc/Mvc.Core/src/FromBodyAttribute.cs

eng/SharedFramework.Local.props

src/Http/Http.Api.Abstractions/src/IFromRouteAttribute.cs

src/Http/Http.Api/src/MapActionExpressionTreeBuilder.cs

pranavkm · 2021-02-03T21:35:56Z

src/Http/Http.Api/src/MapActionExpressionTreeBuilder.cs

+                //    paramterExpression = BindParamenter(formProperty, parameter, parameter.FromForm);
+                //}
+                //else if (parameter.FromBody)
+                if (parameter.CustomAttributes.Any(a => typeof(IFromBodyAttribute).IsAssignableFrom(a.AttributeType)))


We spent a bit of time trying to declutter ApiControllers by inferring default sources for parameters. The rules are fairly simple:

Complex type -> FromBody

Name appears as a route value -> FromRoute

Everything else -> FromQuery

It would be crummy to lose that as part of this programming model.

pranavkm · 2021-02-03T21:45:14Z

src/Http/Http.Api/src/MapActionExpressionTreeBuilder.cs

+                {
+                    var bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType!);
+
+                    await invoker(target, httpContext, bodyValue);


We have to think about validation looks like here. The way this works in MVC is that deserialization errors and validation errors end up in model state. For ApiControllers a filter turns the model state in a 400 response (which includes the a serialized form of model state).

We could re-use ModelState here. It's not necessarily the best data structure to represent a graph of values, but MVC's validation system already understands it so that's one fewer thing to deal with.

How do we surface the error to the user? We could bind a ModelState and let you deal with it. If we wanted to support MVC's auto validation as a feature (it's pretty neat and removes boiler plate), we could add it as a filter that's configured by default.

We shouldn't add any implicit behavior until we figure out a way to remove said behavior. We currently don't have an application model so we need something reasonable that describes this behavior.

If we support something like model state, it would need to be an explicit parameter to the method (which is a much cleaner way of opting-into functionality).

We're going to have some way to configure filters for these globally (services.AddRouting(options => options.MapActionFilters. [Insert / Remove ])). That should be sufficient to enable some implicit runtime behavior that's also removable.

But binding the type once we enable validation would be a good start.

We should do that after, not in this PR

Oh yeah, absolutely. Just pointing out that we have a gap here.

We're going to have some way to configure filters for these globally

So you're going to end up with an application model, just without the "see and modify" part of it 😆

src/Mvc/Mvc.Core/src/FromRouteAttribute.cs

src/Mvc/Mvc.Core/src/StatusCodeResult.cs

Pilchie · 2021-02-03T23:03:14Z

What area- label should we use for this sort of work?

halter73 · 2021-02-03T23:25:09Z

I think area-servers is fine since it will live Microsoft.AspNetCore.Http(.Abstractions). It's definitely more MVC-adjacent than most stuff with the area-servers label though. I could go either way.

davidfowl · 2021-02-03T23:28:29Z

Lets give it both labels to confuse everyone 😄

src/Mvc/Mvc.Core/src/JsonResult.cs

halter73 · 2021-02-04T22:07:28Z

@Pilchie after meeting with @mkArtakMSFT today, I think it makes sense to use the area-mvc label and use the MVC syncs to discuss this.

src/Http/Http.Api.Abstractions/src/IFromBodyAttribute.cs

src/Mvc/Mvc.Core/src/Properties/AssemblyInfo.cs

src/Http/Http.Abstractions/src/Api/IFromRouteMetadata.cs

src/Http/HttpAbstractions.slnf

src/Http/Routing/src/MapAction/MapActionEndpointRouteBuilderExtensions.cs

pranavkm · 2021-02-18T17:08:25Z

src/Http/Routing/src/Builder/MapActionEndpointRouteBuilderExtensions.cs

+
+            const int defaultOrder = 0;
+
+            foreach (var routeAttribute in routeAttributes)


Would it be problematic to say

a) one action -> one route attribute (exactly one method constraint \ template)
b) each route attribute must have a template

The current implementation simply skips over route attributes without a template.

Tratcher · 2021-02-18T18:22:31Z

src/Http/Routing/src/Internal/MapActionExpressionTreeBuilder.cs

+                        catch (IOException ex)
+                        {
+                            LogRequestBodyIOException(httpContext, ex);
+                            httpContext.Abort();


@pranavkm is this how MVC handles it? I thought it set a 400 response status instead?

Yea, why is it aborting the connection?

JSON parsing errors result in a 400 response that includes details from the parser. Other IO errors result in 400s.

IMO, I assumed the entirety of this type was WIP so I hadn't looked at it in detail. We should factor it because this isn't a sustainable format to maintain.

This is very much a work in progress. I imagine we'll want to make logic like this reusable from different kinds of code-gen and move it out of this class at some point.

As for the status code, it now sets a 400 for InvalidDataExceptions. For aborted request, the status code doesn't matter much since the response doesn't get written to a socket and Kestrel will set the status code to 0 before hosting logs it anyway.

Hmm, we missed the invalid Json scenario here. That should also return a 400. edit Nevermind, is InvalidDataException the exception thrown for invalid Json?

src/Http/Routing/src/Internal/MapActionExpressionTreeBuilder.cs

ghost · 2021-03-01T19:16:53Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

halter73 requested review from davidfowl, pranavkm and javiercn February 3, 2021 18:56

SteveSandersonMS reviewed Feb 3, 2021

View reviewed changes