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

void controller method returns status 200 instead of 204 #16925

Closed
syndicatedshannon opened this issue Nov 8, 2019 · 8 comments
Closed

void controller method returns status 200 instead of 204 #16925

syndicatedshannon opened this issue Nov 8, 2019 · 8 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Comments

@syndicatedshannon
Copy link

syndicatedshannon commented Nov 8, 2019

In ASP.NET Core, Controller methods with a void or Task return type produce an HTTP status code 200. That is imprecise judging by RFC, and should instead return 204. No out-of-box ASP.NET Core configuration is available to respond according to W3C guidance. This is a change from ASP.NET, though unchanged in ASP.NET Core.

This could be resolved or improved in a non-breaking change via configuration.

RFC 2616 on Status Code Definitions does not specify that 204 must be used for empty bodies. However, it does make clear that 204 most specifically designates the status of a response where the body does not represent an entity: That's almost certainly the best interpretation of a void or Task response. It also states by example that a 200 response includes an entity or resource.

Other guidance, such as MDN documents on 200 and 204, are more explicit, and also state the inverse in some places; that without a body a 204 is then appropriate:

A 204 (No Content) status code if the action has been enacted and no further information is to be supplied.
A 200 (OK) status code if the action has been enacted and the response message includes a representation describing the status.

RFC 7231 on HTTP/1.1 is perhaps most relevant. It states (note "ought", rather than "must", but also "always"):

6.3.1. 200 OK
Aside from responses to CONNECT, a 200 response always has a payload,
though an origin server MAY generate a payload body of zero length.
If no payload is desired, an origin server ought to send 204 (No
Content) instead.

EDITS:

  • Multiple edits to clarify RFC and documented conventions.
  • Edit to clarify the current 200 response has been present throughout ASP.NET Core versions
  • Edit to clarify that I'm not requesting the breaking change of adjusting default response in existing implementations
@javiercn javiercn added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 8, 2019
@javiercn
Copy link
Member

javiercn commented Nov 8, 2019

@syndicatedshannon thanks for contacting us.

I don't think this is different from previous versions of ASP.NET Core, but I'm happy to be corrected. The default behavior in ASP.NET Core (and many other servers) is to return 200 by default, which is what a void returning action does.

Unless we get a report that indicates that this was a change between a previous version and the current one, we are likely not going to change this behavior, as doing so would break many applications.

There are easy alternatives to this, like simply returning an action result from your action to obtain the exact behavior you want.

I'm closing this issue as we don't plan to change the current behavior. If you can provide an example that shows that this was indeed a behavior change between 3.0 and a previous version, we will re-evaluate this issue.

I hope you understand that we value not breaking other developers existing apps over the correctness of a specific choice, specially when there are alternatives and the impact is reduced.

@javiercn javiercn closed this as completed Nov 8, 2019
@syndicatedshannon
Copy link
Author

@javiercn Our API surface returns T, not IHttpActionResult<T>, as in our opinion this supported usage clarifies the surface and dependents (including tests). What alternative would you recommend in this case?

@syndicatedshannon
Copy link
Author

@cremor Can you comment? I see the Microsoft documentation that you referenced:

Return type How Web API creates the response
void Return empty 204 (No Content)

However, I do not know offhand about your mention prior versions. Do you have easy access to this?

@syndicatedshannon
Copy link
Author

syndicatedshannon commented Nov 8, 2019

@javiercn I disagree with the close based solely upon a breaking change, when addressing the issue does not require a breaking change. I believe following RFC standards is sufficiently important to fix the issue in addition to preventing breaking changes, even if an options flag is required. Does that make sense?

Separately, I do understand your recommend to close based on alternatives/workarounds. Would you please take a moment to refer me to a workaround when not returning IHttpActionResult?

@javiercn
Copy link
Member

javiercn commented Nov 8, 2019

Separately, I do understand your recommend to close based on alternatives/workarounds. Would you please take a moment to refer me to a workaround when not returning IHttpActionResult?

@syndicatedshannon I believe you could tweak things here with a global filter. That gives you control over the final result.

@syndicatedshannon
Copy link
Author

I believe you could tweak things here with a global filter.

@javiercn I did find that option; I commented on a similar stackoverflow answer yesterday. Note the answer mentions cons of that approach.

I'm still willing to do it, but do you think perhaps supporting this RFC guidance I mentioned in the issue report deserves built-in functionality, without custom filter implementation/insertion?

@cremor
Copy link
Contributor

cremor commented Nov 8, 2019

@cremor Can you comment? I see the Microsoft documentation that you referenced:

Note that the linked documentation is for ASP.NET Web API 2 (that is the version of ASP.NET on the full .NET Framework, which was released before .NET Core was a thing). @javiercn said that no older version of ASP.NET Core ever returned 204 for void. I haven't checked if this is true, but I'll believe him.

So the difference is between ASP.NET (from the full framework) and ASP.NET Core. That's no problem because those are completely different frameworks with many differences.

While I agree that void/Task returning action methods should optimally return a 204, I also see the reasoning that introducing a breaking change in ASP.NET Core for a change that is not really required is not something Microsoft wants to do.

@syndicatedshannon
Copy link
Author

syndicatedshannon commented Nov 8, 2019

@cremor Thanks for the feedback. I'll wait to hear if @javiercn replies to the value of addressing this (which IMO is required for providing core support for RFC).

If no reply, I'll open a separate feature request, clarifying that I'm not requesting the default functionality changed (breaking changes). Personally, I do believe this is a defect, but I appreciate the plausibility of addressing may be clearer when viewed as a feature. Perhaps adding "option/feature flag" in the title will also clarify.

Again, I do find it hard to believe the current functionality isn't an oversight that the team would want to correct, if it was given consideration. But, of course if I hear that, I'll just be disappointed, add a shim to my software, and move on. Happens all the time ;)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

No branches or pull requests

3 participants