Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Discussion for announcement "MVC's action result naming changes" #4118

Closed
kichalla opened this issue Feb 19, 2016 · 16 comments
Closed

Discussion for announcement "MVC's action result naming changes" #4118

kichalla opened this issue Feb 19, 2016 · 16 comments

Comments

@kichalla
Copy link
Member

Discussion issue for aspnet/Announcements#153

@zygimantas
Copy link

I am using RC1 and I personally miss few result types:

  1. 403 ForbiddenResult (I see it already exists in repository, named ForbidResult (why not Forbidden as in https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html ?))
  2. 405 MethodNotAllowedResult
  3. 409 ConflictResult

Is there any discussion why few of these response types are implemented, while others are not?

@pranavkm
Copy link
Contributor

@zygimantas some context to the ForbidResult name choice - #3461 (comment). It boils down to keeping it similar to the names AuthenticationManager uses - https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http.Abstractions/Authentication/AuthenticationManager.cs#L87

@304NotModified
Copy link

Just Google "http 403" . It should be forbidden IMO.

Also +1 for 405 and 409. Those make sense

@zygimantas
Copy link

@pranavkm thanks, it makes sense in general, but it conflicts with other response types, which are named accordingly to HTTP status codes. I would choose "Forbidden" name, no matter what name AuthenticationManageruses. In addition to that, "Forbid" is a verb.

Right now, I must use 3 different notations in a single project:

  1. new BadRequestResult() - because it is clearly readable.
  2. new HttpStatusCodeResult(409) - because GoneRequestResult does not exist.
  3. this.CreatedAtAction() - because of headers and payload.

Something like this might give more consistency:

StatusCodeResult(
    int statusCode,
    object content = null,
    collection_type? headers = null,
    int? subStatusCode = null)

And there might be an enumeration or class with constants which would translate readable status codes to int, i.e: StatusCodes.Forbidden to 403

Related issue: aspnet/HttpAbstractions#174

@aggieben
Copy link

Is there a particular reason that the Http prefix was dropped? Even though I'm familiar with status codes, I find it helpful to let intellisense help me pick from a list. I think this particular part of the change would also make things more opaque to a newcomer.

@pebezo
Copy link

pebezo commented Feb 19, 2016

Thanks for making it more consistent. I also agree with @zygimantas that it should be "Forbidden". In the AuthenticationManager example "forbid" is used as verb (ForbidAsync = "forbid this right now in an async fashion"), not an adjective. It even "sounds" bad... "forbid (the) result" rather than "result of type http forbidden".

@Eilon Eilon added this to the Discussion milestone Feb 19, 2016
@rynowak
Copy link
Member

rynowak commented Feb 19, 2016

Is there a particular reason that the Http prefix was dropped?

This was never applied consistently in MVC or WebAPI. We started out trying to be consistent with the past releases (to avoid breaking your code), and embraced the fact that weren't ever consistent about it in previous versions. However, we've continued to get feedback about it to the point where it seemed worth the break.

Given a choice, we'd pick less boilerplate over more, so we dropped the prefix.

@304NotModified
Copy link

ForbidAsync = "forbid this right now in an async fashion"),

Good example.

Is there a particular reason that the Http prefix was dropped?

  • Given a choice, we'd pick less boilerplate over more, so we dropped the prefix.

If there is not real benefit, I would choose for one which is most used - that is with the prefix in previous ASP.NET versions.

@tuespetre
Copy link
Contributor

Building on @zygimantas and @304NotModified, I would like to see a StatusCode convenience method.

@aggieben
Copy link

@rynowak I understand the desire for less boilerplate, but having prefixes that cause all those classnames wasn't just boilerplate, it helped people find them.

I agree with @tuespetre - if the status code names are going to be harder to discover in intellisense/autocomplete (and they are now), then we should at least have an easier way to indicate a status code directly.

@304NotModified
Copy link

One thing isn't fully clear to me. Are those exceptions always bind to a http status code? If so, then prefixing the "http" isn't bad IMO as the namespace (Microsoft.AspNetCore.Mvc) isn't containing "HttpStatuses" or something like that.

(edit, maybe I have to rename my username ;))

@pebezo
Copy link

pebezo commented Feb 24, 2016

@aggieben

I agree with @tuespetre - if the status code names are going to be harder to discover in intellisense/autocomplete (and they are now), then we should at least have an easier way to indicate a status code directly.

In my opinion Visual Studio should sort all the possible options based on what's the most appropriate / likely item. For example, if you're in a function that returns IActionResult then it should first list all the available methods or classes that actually implement IActionResult. Kind of silly to show everything in alphabetical order...

@pebezo
Copy link

pebezo commented Feb 24, 2016

@MicahZoltu
Copy link

The lack of consistency here is really frustrating. The status codes all have well defined names so naming seems like a non-problem. If other parts of the framework name things counter to the RFC and can't be changed for compatibility reasons that is fine but I think it is far more important for users to have some way to rely on consistent naming within a given context. When in the context of writing a Controller (one of the most common contexts in MVC I would imagine) and you need to return a status code the naming is all over the map.

For example, if I want to return an 200 OK with a payload I can do Ok(payload). If I want to return a 400 Bad Request with a payload I can do HttpBadRequest(payload). If I want to return a 409 Conflict with a payload it appears I have to manually construct a response and throw it as an exception.

There are relatively few status codes out there and they are all pretty well defined (at least as far as naming goes). It seems like it would be a pretty easy task to simply provide a set of result wrappers (like Ok or HttpBadRequest); maybe an hour or two of typing. Also, I would rather have both Ok and OkResult and HttpOk as options for backwards compatibility if it meant at least one of them had all of the status codes in that format. Boilerplate typing (like the Http prefix) doesn't bother me in the slightest, but having every one of my status code responses constructed in a different way is maddening. I have one controller action right now that has the following:

  • HttpBadRequest(payload)
  • new NoContentResult()
  • Manually constructed 409 Conflict with payload.

At this point I have a reasonably strong desire to just not use the framework supplied IActionResults and build my own set of helpers that are at least consistent.

@304NotModified
Copy link

Yes, please make it consistent.

@Eilon
Copy link
Member

Eilon commented Jun 9, 2017

We are closing this issue because no further action is planned for this issue. If you still have any issues or questions, please log a new issue with any additional details that you have.

@Eilon Eilon closed this as completed Jun 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants