Infer that interface parameters are services by halter73 · Pull Request #31658 · dotnet/aspnetcore

halter73 · 2021-04-09T23:59:44Z

Also some MapAction -> MinimalAction renaming.

Addresses two checkboxes in #30248 (comment).

As of #31603, all minimal action parameters without an attribute or a simple type (one with a static bool TryParse(string, out T) method) are inferred to be services. This was never intended to be the final convention.

We decided to infer only interface parameters are services. Any non-interface parameter without an attribute is now inferred to be a [FromBody] parameter (not allowed to be empty).

davidfowl

This LGTM, I left a few minor comments

davidfowl · 2021-04-10T01:19:55Z

+        {
+            if (factoryContext.JsonRequestBodyType is not null)
+            {
+                throw new InvalidOperationException("Action cannot have more than one FromBody attribute.");


This error is kinda jank for the case where there's no explicit attribute.

Does this code path execute in the context of executing the delegate (as opposed to whenever we build the route table)? If it's the latter, we should include more details about what method produced this error. It might be good to include that regardless.

This is build time, but I agree this error message is terrible especially in light of the new convention-based inference of parameter sources. I want to track more information about the parameters when building up the RequestDelegate arguments which will lead to better errors, but I've already started on the culture stuff so I think I'll do this as part of this PR.

davidfowl · 2021-04-10T01:21:16Z

+            }
+
+            factoryContext.JsonRequestBodyType = parameterType;
+            factoryContext.AllowEmptyRequestBody = allowEmpty;


Interesting that inference means you lose this feature. Can we use nullability to determine this?

Thoughts @pranavkm?

davidfowl · 2021-04-10T06:28:27Z

@halter73 I played with this a little. I think binding errors need to either throw exceptions (so that they show up in the developer exception page) OR, we change the log to more than debug (like error). That does mean it could get noisy since the errors are drive by the client side.

pranavkm · 2021-04-10T12:54:26Z

Need to look at this in detail, but should we treat collection interfaces as being FromBody? It’s not incredibly common to try and get optional services in your action, much more common to want to read an IList<T>

davidfowl · 2021-04-10T16:26:03Z

Need to look at this in detail, but should we treat collection interfaces as being FromBody? It’s not incredibly common to try and get optional services in your action, much more common to want to read an IList

Sounds reasonable to me. In fact, the DI contract only demands that IEnumerable<T> can be injected (though other containers might support other syntax).

I also realize that DbContext types won't work with the interface only constraint.

halter73 · 2021-04-13T00:01:33Z

@JamesNK I missed you on the initial review. Do you have any opinions on how we should infer service vs [FromBody] parameters?

JamesNK · 2021-04-13T00:04:48Z

Inferring interfaces are always services seems fine to me.

JamesNK · 2021-04-13T00:07:29Z

Actually, a question. What happens if someone has IList<int> or IEnumerable<int> as a parameter. I'm guessing it fails, but how does it fail and what is the error message?

IEnumerable<T> can be resolved from services, if T instances have been registered with DI. Do you have a test for that?

pranavkm · 2021-04-13T00:07:53Z

            }
+            else
+            {
+                return BindParameterFromBody(parameter.ParameterType, allowEmpty: false, factoryContext);


Yeah, I think 👍🏽 @davidfowl 's comment - could we rely on the presence of a default value to determine if this is optional? e.g.

MyModel Echo(MyModel model) => model; // AllowEmpty = false MyModel Echo(MyModel model = null) => model; // AllowEmpty = true

If we were absolutely bonkers like MVC, we could also rely on nullability of the parameter to determine optionality:

MyModel Echo(MyModel? model) => model; // AllowEmpty = true

Yep. I'm a huge fain of using nullability to determine AllowEmpty behavior. I think we should also use it when the [FromBody] attribute is applied without an explicit parameter.

Behold ye what needs to be done for nullability: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs#L465-L568. It would be nice to put this in a shared space.

Btw, it would be a lot easier to infer nullability once we have a source generator.

pranavkm · 2021-04-13T00:09:45Z

+        {
+            if (factoryContext.JsonRequestBodyType is not null)
+            {
+                throw new InvalidOperationException("Action cannot have more than one FromBody attribute.");


Does this code path execute in the context of executing the delegate (as opposed to whenever we build the route table)? If it's the latter, we should include more details about what method produced this error. It might be good to include that regardless.

pranavkm · 2021-04-13T00:12:03Z

IMO, it might be simple to say any collection type is assumed to be bound from the body - it really helps to keep the mental model simple and not have esoteric rules especially when we don't have tooling that helps our analysis. It's not really hard to annotate the parameter if you really needed an optional DI service.

halter73 · 2021-04-13T00:22:45Z

Actually, a question. What happens if someone has IList or IEnumerable as a parameter. I'm guessing it fails, but how does it fail and what is the error message?

Currently it would fail at runtime with an InvalidOperationException thrown from GetRequireService<IList<int>>() and a 500 response.

IEnumerable can be resolved from services, if T instances have been registered with DI. Do you have a test for that?

This should work. I can a test for that, but this is really just testing DI behavior unless we want to have a special behavior for collection interfaces.

JamesNK · 2021-04-13T00:30:44Z

This should work. I can a test for that, but this is really just testing DI behavior unless we want to have a special behavior for collection interfaces.

I think you should add a test. Think of tests as drawing a line in the sand as "this is how it works". That way if we accidentally/purposefully change the behavior we'll see a broken test and know that we're making a breaking change.

halter73 · 2021-04-13T00:55:15Z

IMO, it might be simple to say any collection type is assumed to be bound from the body - it really helps to keep the mental model simple and not have esoteric rules especially when we don't have tooling that helps our analysis. It's not really hard to annotate the parameter if you really needed an optional DI service.

I like it. This means the [FromService] attribute will be required to inject IEnumerable<TService>.

halter73 · 2021-04-13T00:57:07Z

What does everyone think of inferring any parameter with an interface type that extends IEnumerable<T> is from the body?

davidfowl · 2021-04-13T01:29:04Z

What does everyone think of inferring any parameter with an interface type that extends IEnumerable is from the body?

Walking the interface hierarchy? Bleh. What about IAsyncEnumerable<T> 😄 . I think it's fine. I don't want to stall this PR on these smaller decisions. Lets pick some behavior and document it for preview4

halter73 added 3 commits April 9, 2021 16:51

Add tests!

24a748c

Infer that interface parameters are services

44a588c

MapAction -> MinimalAction

ea59c39

halter73 requested review from LadyNaggaga, captainsafia, davidfowl and pranavkm April 9, 2021 23:59

halter73 requested review from Tratcher, javiercn and jkotalik as code owners April 9, 2021 23:59

ghost added the area-runtime label Apr 9, 2021

halter73 added the feature-minimal-actions Controller-like actions for endpoint routing label Apr 9, 2021

More test coverage

6fbdbc6

halter73 force-pushed the halter73/interfaces-are-probably-services branch from 9978e7e to 6fbdbc6 Compare April 10, 2021 00:10

davidfowl reviewed Apr 10, 2021

View reviewed changes

Comment thread src/Http/Http.Extensions/src/RequestDelegateFactory.cs

fix build

38dbab5

davidfowl approved these changes Apr 10, 2021

View reviewed changes

halter73 requested a review from JamesNK April 12, 2021 23:59

JamesNK approved these changes Apr 13, 2021

View reviewed changes

pranavkm reviewed Apr 13, 2021

View reviewed changes

halter73 merged commit d70439c into main Apr 13, 2021

halter73 deleted the halter73/interfaces-are-probably-services branch April 13, 2021 01:39

This was referenced May 4, 2021

RequestDelegateFactory should produce clear errors for invalid Delegates #30322

Closed

Use nullibility to infer optionality of service and body parameters #32375

Closed

Infer collection interfaces like IList come from the body in minimal actions #32376

Closed

amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023

Conversation

halter73 commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pranavkm commented Apr 10, 2021

Uh oh!

davidfowl commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Apr 13, 2021

Uh oh!

JamesNK commented Apr 13, 2021

Uh oh!

JamesNK commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pranavkm commented Apr 13, 2021

Uh oh!

halter73 commented Apr 13, 2021

Uh oh!

JamesNK commented Apr 13, 2021

Uh oh!

halter73 commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Apr 13, 2021

Uh oh!

davidfowl commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

halter73 commented Apr 9, 2021 •

edited

Loading

davidfowl commented Apr 10, 2021 •

edited

Loading

davidfowl commented Apr 10, 2021 •

edited

Loading

JamesNK commented Apr 13, 2021 •

edited

Loading

halter73 commented Apr 13, 2021 •

edited

Loading