Skip to content

Add remarks to FromBodyAttrbiute #56997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Add remarks to FromBodyAttrbiute #56997

merged 2 commits into from
Jul 31, 2024

Conversation

voroninp
Copy link
Contributor

Add remarks to FromBodyAttrbiute

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

[FromBody] attrbiute section of the docs dedicated to Model Binding contains a very important statement:

The ASP.NET Core runtime delegates the responsibility of reading the body to an input formatter.

This is quite an important detail (gotcha, actually) to capture it in the xmldoc of the attribute, so it's imediately visible in IDE.

@voroninp voroninp requested a review from a team as a code owner July 25, 2024 18:52
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 25, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 25, 2024
@@ -9,6 +9,9 @@ namespace Microsoft.AspNetCore.Mvc;
/// <summary>
/// Specifies that a parameter or property should be bound using the request body.
/// </summary>
/// <remarks>
/// Be default ASP.NET Core runtime delegates the responsibility of reading the body to an input formatter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true for MVC but not for minimal API, which also uses the same attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it behave for minipal API then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it just reads the form itself directly before passing control to the user's endpoint method using https://learn.microsoft.com/dotnet/api/microsoft.aspnetcore.http.features.iformfeature.readformasync (I haven't dug through the code to check, but that's what I see to remember from doing work on the binding "glue" stuff).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it:
Here and then here.

Ok, I need to change the wording. The fun thing that MVC's model binding involves validation, and even System.Text.Json does not care about [Required] attrbiute I get 400 Bad Request while for minimal API I get 200.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martincostello , I changed the wording.

[[FromBody] attrbiute](https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-8.0#frombody-attribute) section contains a very important comment :
> The ASP.NET Core runtime delegates the responsibility of reading the body to an input formatter.

This is quite an important detail to capture it in the xmldoc of this attribute.
Co-authored-by: Martin Costello <martin@martincostello.com>
@KennethHoff
Copy link

What exactly is "an input formatter"?

@voroninp
Copy link
Contributor Author

@KennethHoff ,is it a hint that link to the description of formatters concept is missing or a real question?

If it's the latter one , then
https://learn.microsoft.com/en-us/aspnet/core/web-api/advanced/formatting?view=aspnetcore-8.0

@KennethHoff
Copy link

@KennethHoff ,is it a hint that link to the description of formatters concept is missing or a real question?

Both, really 😅

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Out of curiousity, what motiviated you to add this note? It seems like you might not have expected input formatters to be coming in to play here?

@captainsafia captainsafia merged commit 42e6172 into dotnet:main Jul 31, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-rc1 milestone Jul 31, 2024
@voroninp
Copy link
Contributor Author

voroninp commented Aug 1, 2024

@captainsafia , well, I knew this, but I've learned it the hard way by making wrong assumptions. And minimal API behavior was a surprise.
This time I've noticed my colleague had wrong expectations about model validation.

@voroninp
Copy link
Contributor Author

voroninp commented Aug 1, 2024

@captainsafia , actually, there are many places where xmldocs are lacking important details. Sometimes, clarifications can be found in docs at Microsoft.Learn. But quite often I need to look into sources.

For exception handler middleware it is not mentioned that by default 404 status code without started response will cause InvalidOperationException.

I do not get the description for this overload.
I do not specify any url, what is an alternative pipeline in this case, what for?

I'd also change the wording of the comment for adding a custom exception handler from "it is used by exception handler middleware" to "it is required to use exception handler middleware for the custom handler to work. (Analyzer would be even better;-) )

For authentication it's not obvious when Thread.CurrentPrincipal is set and when it is not.

Why is Thread.CurrentPrincipal nullable, but HttpContext.User is not?

Or another example:
While GetRouteData method uses httpContext.Request.RouteValues directly, if feature is not found, GetRouteValue method gets route values from IRouteValuesFeature.
But why?!
Why not httpContext.Request.RouteValues[key] or not:

return (httpContext.Features.Get<IRouteValuesFeature>() ?? httpContext.Request.RouteValues)[key];

Does it mean IRouteValuesFeature feature is always there? (Then it should be Features.GetRequired<IRouteValuesFeature>() to convey the intent.)
If not, then context.GetRouteData().Values[key] and `context.GetRoutValue(key)' can return differen values.
Is it a bug, or is it by design, then what was the intention?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants