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

Adding AcceptedResult and <controller>.Accepted() responses #4937

Closed
jterry75 opened this Issue Jun 30, 2016 · 14 comments

Comments

Projects
None yet
8 participants
@jterry75

jterry75 commented Jun 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Jun 30, 2016

Member

@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?

Member

Eilon commented Jun 30, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@jterry75

jterry75 Jun 30, 2016

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.

jterry75 commented Jun 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Jun 30, 2016

Member

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.

Member

rynowak commented Jun 30, 2016

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 Eilon added this to the 1.1.0 milestone Sep 29, 2016

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Sep 29, 2016

Member

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

Member

Eilon commented Sep 29, 2016

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

@pranavkm

This comment has been minimized.

Show comment
Hide comment
@pranavkm

pranavkm Sep 29, 2016

Member

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?

Member

pranavkm commented Sep 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jterry75

jterry75 Sep 29, 2016

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.

jterry75 commented Sep 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@javiercn

javiercn Sep 29, 2016

Member

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?

Member

javiercn commented Sep 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Sep 30, 2016

Member

@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 😄

Member

Eilon commented Sep 30, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Sep 30, 2016

Member

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.

Member

rynowak commented Sep 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jbagga

jbagga Sep 30, 2016

Contributor

@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.

Contributor

jbagga commented Sep 30, 2016

@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.

@jbagga jbagga added 2 - Working and removed 1 - Ready labels Sep 30, 2016

@ivaylokenov

This comment has been minimized.

Show comment
Hide comment
@ivaylokenov

ivaylokenov Oct 2, 2016

Contributor

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.

Contributor

ivaylokenov commented Oct 2, 2016

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

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Oct 2, 2016

Member

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

Member

davidfowl commented Oct 2, 2016

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

@ivaylokenov

This comment has been minimized.

Show comment
Hide comment
@ivaylokenov

ivaylokenov Oct 2, 2016

Contributor

@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).

Contributor

ivaylokenov commented Oct 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Oct 3, 2016

Member

@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.

Member

rynowak commented Oct 3, 2016

@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.

@jbagga jbagga changed the title from Can we add AcceptedResult and <controller>.Accepted() responses? to Adding AcceptedResult and <controller>.Accepted() responses Oct 10, 2016

@Eilon Eilon modified the milestones: 1.1.0, 1.1.0-preview1 Oct 11, 2016

@jbagga jbagga closed this in #5355 Oct 18, 2016

jbagga added a commit that referenced this issue Oct 18, 2016

@jbagga jbagga added 3 - Done and removed 2 - Working labels Oct 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment