Skip to content
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

ActionResult<T> should convert to OkObjectResult or set StatusCode to 200 #27165

Closed
stevendarby opened this issue Oct 23, 2020 · 5 comments
Closed
Labels
affected-very-few This issue impacts very few customers area-mvc Includes: MVC, Razor, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. severity-minor This label is used by an internal tool
Milestone

Comments

@stevendarby
Copy link

stevendarby commented Oct 23, 2020

Currently if you return T in an action that returns ActionResult<T> it will create an ObjectResult. The problem is that the StatusCode remains null rather than 200, even though the eventual response generated from it will be 200.

This makes it confusing in, for example, an action filter, where a developer may only want to handle Ok object results on the executed context, including where T is returned directly. They may:

  • Safe cast to OkObjectResult and find null
  • Better, they safe cast to ObjectResult which works but then find StatusCode is null

They may not handle the result as they should as it's not obvious that a null StatusCode would eventually wind up as a 200 in the response. It seems to be an implementation detail that could easily change.

The assumption is always that returning T will result in a 200 OK (e.g. documentation mentions how you can leave out the T type in a ProducesResponseType attribute for 200 when using ActionResult<T>).

Can this be further enforced by creating an OkObjectResult (which will set StatusCode to 200 in its constructor)? If not, perhaps just an ObjectResult as it currently does, but setting StatusCode to 200 with member initialisation?

Is there perhaps room for improvement in documentation about any of this? Happy to be pointed to anything that makes it explicit how response status codes are created from result status codes including when the result status code is null etc.

@javiercn javiercn added the area-mvc Includes: MVC, Razor, Localization, CORS, most templates label Oct 23, 2020
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Oct 23, 2020
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Oct 23, 2020
@msftbot
Copy link
Contributor

msftbot bot commented Oct 23, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@pranavkm
Copy link
Contributor

@Snappyfoo setting the StatusCode would be a less dramatic change. There's a special case we would have to configure for ActionResult<ProblemDetails> since ObjectResult specializes on that type: https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.Core/src/ObjectResult.cs#L78-L88, but it should be find outside of that.

@pranavkm pranavkm modified the milestones: Next sprint planning, Backlog Nov 6, 2020
@msftbot
Copy link
Contributor

msftbot bot commented Nov 6, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@pranavkm pranavkm added good first issue Good for newcomers. affected-very-few This issue impacts very few customers severity-minor This label is used by an internal tool labels Nov 6, 2020
pranavkm pushed a commit that referenced this issue Dec 1, 2020
…28267)

Summary of the changes
 - For `ActionResult<T>` that wraps an object value, set status code to 200 when converting to `IActionResult`
 - Special handling for `ProblemDetails` (honors `StatusCode` in `ProblemDetails` if it's set)

Addresses #27165
@davidfowl
Copy link
Member

@pranavkm isn't this done?

@pranavkm
Copy link
Contributor

Resolved as part of #28267

@pranavkm pranavkm modified the milestones: Backlog, 6.0-preview1 Mar 29, 2021
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers area-mvc Includes: MVC, Razor, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

5 participants