Adding AcceptedResult and <controller>.Accepted() responses

[jterry75]

Just wondering if there was a specific reason that there is no class AcceptedResult like (OkResult) and Accepted() response on ControllerBase for async operations. It seems like this would be a common scenario so before I PR'd one up wanted to make sure its not a design decision.

Thanks!

[Eilon]

@jterry75 there's probably no special reason we didn't do it. It's most likely that we either just didn't think to do it, and/or that it's not a very common status code (most HTTP requests return results immediately).

Do you find yourself using HTTP 202 Accepted often?

[jterry75]

When writing goal driven / document based REST we often use this. Basically the response is either HTTP 200 OK if the goal has been reached or HTTP 202 Accepted if everything is valid but the task is still running in the background. Subsequent calls with the same goal will continue to return HTTP 202 Accepted until the goal has been reached or failed. In which case the last call is HTTP 200 OK or Some error result.

[rynowak]

I've seen HTTP 201 Created used for this in the past, and (at least to me) 202 seems more appropriate for an asynchronously created resource. Would be nice to have IMO. Example from our own code: https://github.com/aspnet/benchmarks/blob/dev/src/BenchmarksServer/Controllers/JobsController.cs#L54

I know @javiercn has had opinions about this and other things 202 related in the past.

[Eilon]

@jbagga please work with @pranavkm and others to come up with some proposals on exactly which methods we should add.

[pranavkm]

Will do. 409 - Conflict is another response extension we're missing from WebAPI. https://msdn.microsoft.com/en-us/library/system.web.http.apicontroller.conflict(v=vs.118).aspx#M:System.Web.Http.ApiController.Conflict. Should we consider adding that one too?

[jterry75]

I didn't do 409 Conflict but I submitted a PR: #5215 for some of these a while back but was told that there isn't a lot of value in having the classes over just setting the HTTPResult/Response yourself so it was closed.

[javiercn]

My only concern here is that we have an explosion of helper methods on Controller and ControllerBase. I think we should keep only the common methods that we have and the ones we added for backwards compatibility with MVC.

I consider that is easy enough to just either extend Controller or ControllerBase to include these result types if its a common pattern on your app or just create some extension methods over controllerbase in you application.

If you care about reusing these methods across projects, it's easy enough to put them either on a separate project or create a nuget package for them yourself.

The point that I'm trying to make is that the more "uncommon" methods that we add to controller and controllerbase, the harder it makes to find the "common" ones while writing code (makes the intellisense list larger, adds more dependencies in some cases, etc.). In my opinion this adds more complexity to these types without providing a whole lot of additional value.

That said, I think that its ok to add new methods, but I think we should have a clear scenario that shows the usage of the method in context and determine how common that scenario as a way to evaluate if a helper method should be in the box.

For example, Ok(payload) the scenario is a user returning data from an API, which is a common enough pattern to be included in the box.

@jbagga @Eilon @pranavkm thoughts?

[Eilon]

@javiercn indeed, the goal is not to add as many as possible, e.g. at least one for every RFC-standard HTTP method. But rather to add ones that seem reasonably useful because they add clear value and make code more readable and/or testable. So let's come up with that list 😄

[rynowak]

Will do. 409 - Conflict is another response extension we're missing from WebAPI. https://msdn.microsoft.com/en-us/library/system.web.http.apicontroller.conflict(v=vs.118).aspx#M:System.Web.Http.ApiController.Conflict. Should we consider adding that one too?

We've looked into this in the past. The primary reason why we had explicit support for 409 was that we did special handling for concurrency errors in EF+scaffolding. It wasn't based on common usage of code that users write, so we didn't include it.

[jbagga]

@javiercn, @Eilon, @pranavkm

304 Not Modified could be another useful one.

The PR @jterry75 sent in include 202 Accepted, 412 Preconditon Failed and 303 See Other. But it seems like 202 and 304 are worth investigating further. Others may be for specific use case scenarios and creating your own NuGet packages as @javiercn suggested sounds reasonable.

[ivaylokenov]

I saw this issue and created NuGet packages containing HTTP action results and ControllerBase extension methods. They are available for download and the source code is public: https://github.com/ivaylokenov/AspNetCore.Mvc.HttpActionResults. Added most of the mentioned ones here and plan to add all the others from the HTTP specification in the next couple of days. Pull requests are more than welcome of course.

[davidfowl]

@ivaylokenov thats NPM style package granularity. Just have a single package... 😄

[ivaylokenov]

@davidfowl Love the good old 'NodeJS is not cool' joke. 😄
My only consideration with a single package is when all status codes are implemented the intellisense on the controller class will be full of not very widely used extension methods (or not used at all in modern applications).

[rynowak]

@ivaylokenov - FYI the reason we don't do things like this with extension methods is that you end up having to do this.Accepted(...) for it to be valid inside the controller class. If you're like me, and this is the style you prefer (pun intended) it's not a big deal.