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

Update HttpStatusCode enum with updates #15650

Closed
mteper opened this issue Nov 6, 2015 · 54 comments
Closed

Update HttpStatusCode enum with updates #15650

mteper opened this issue Nov 6, 2015 · 54 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Milestone

Comments

@mteper
Copy link

mteper commented Nov 6, 2015

Latest Proposal

We should update current HttpStatusCode to include more official status codes defined in recent RFCs.

Proposed API

    public enum HttpStatusCode
    {
        // Informational 1xx
        Continue = 100,
        SwitchingProtocols = 101,
+       Processing = 102,
+       EarlyHints = 103,

        // Successful 2xx
        OK = 200,
        Created = 201,
        Accepted = 202,
        NonAuthoritativeInformation = 203,
        NoContent = 204,
        ResetContent = 205,
        PartialContent = 206,
+       MultiStatus = 207,
+       AlreadyReported = 208,
        
+       IMUsed = 226,

        // Redirection 3xx
        MultipleChoices = 300,
        Ambiguous = 300,
        MovedPermanently = 301,
        Moved = 301,
        Found = 302,
        Redirect = 302,
        SeeOther = 303,
        RedirectMethod = 303,
        NotModified = 304,
        UseProxy = 305,
        Unused = 306,
        TemporaryRedirect = 307,
        RedirectKeepVerb = 307,
+       PermanentRedirect = 308,

        // Client Error 4xx
        BadRequest = 400,
        Unauthorized = 401,
        PaymentRequired = 402,
        Forbidden = 403,
        NotFound = 404,
        MethodNotAllowed = 405,
        NotAcceptable = 406,
        ProxyAuthenticationRequired = 407,
        RequestTimeout = 408,
        Conflict = 409,
        Gone = 410,
        LengthRequired = 411,
        PreconditionFailed = 412,
        RequestEntityTooLarge = 413,
        RequestUriTooLong = 414,
        UnsupportedMediaType = 415,
        RequestedRangeNotSatisfiable = 416,
        ExpectationFailed = 417,
+       // Removed status code: ImATeapot = 418,

+       MisdirectedRequest = 421,
+       UnprocessableEntity = 422,
+       Locked = 423,
+       FailedDependency = 424,

        UpgradeRequired = 426,
        
+       PreconditionRequired = 428,
+       TooManyRequests = 429,
        
+       RequestHeaderFieldsTooLarge = 431,
        
+       UnavailableForLegalReasons = 451,

        // Server Error 5xx
        InternalServerError = 500,
        NotImplemented = 501,
        BadGateway = 502,
        ServiceUnavailable = 503,
        GatewayTimeout = 504,
        HttpVersionNotSupported = 505,
+       VariantAlsoNegotiates = 506,
+       InsufficientStorage = 507,
+       LoopDetected = 508,
        
+       NotExtended = 510,
+       NetworkAuthenticationRequired = 511,
    }

Details

Values added from WinRT: Windows.Web.Http.HttpStatusCode:

AlreadyReported
FailedDependency
IMUsed
InsufficientStorage
Locked
LoopDetected
MultiStatus
NetworkAuthenticationRequired
None (not sure what's this, not added)
NotExtended
PermanentRedirect
PreconditionRequired
Processing
RequestHeaderFieldsTooLarge
TooManyRequests
UnprocessableEntity
VariantAlsoNegotiates

Original Proposal

Please update with codes from RFCs 2817, 5785, 6266, 6585.

@mteper
Copy link
Author

mteper commented Nov 6, 2015

Main RFC, with links to update RFCs: https://tools.ietf.org/html/rfc2616

@davidsh
Copy link
Contributor

davidsh commented Nov 6, 2015

RFC2616 is obsolete now. The latest set of HTTP RFCs are 7230 thru 7240:

http://tools.ietf.org/html/rfc7230

Also, are you asking for adding new enum values or changing existing ones? We can't change existing ones. And adding new ones would be an API change which we would have to figure out how to get working across all the platforms as well as Desktop.

@davidsh
Copy link
Contributor

davidsh commented Nov 6, 2015

cc: @SidharthNabar

@davidfowl
Copy link
Member

If we make a new enum or type we should also look at the fact that we have our own

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http.Abstractions/StatusCodes.cs#L6

/cc @Tratcher @Eilon

@davidsh
Copy link
Contributor

davidsh commented Nov 9, 2015

Don't understand why you would have another enum type that is substantially similar to System.Net HttpStatusCode.

cc: @ericstj @weshaggard

@mteper
Copy link
Author

mteper commented Nov 9, 2015

What @davidsh said. But if we must have a new one, perhaps it should be more complete?

@davidfowl
Copy link
Member

I'm fuzzy on the reasons but it might have been a dependency thing? Where are the status codes defined right now? It might have also been a flexibility thing so it was ok to change outside of .NET Framework (since we didn't want to bother unifying the type).

@davidsh
Copy link
Contributor

davidsh commented Nov 9, 2015

@Eilon
Copy link
Member

Eilon commented Nov 9, 2015

We chose a different strategy for two reasons:

  1. We found that using an enum to describe this was cumbersome when the API you're calling uses an int due to the required cast. If your code is staying within the System.Net universe there's no problem, but the moment you try to interoperate with any other code you end up with casts.
  2. We found that having the status codes in the names of the values helped usability (subjective, of course). If anyone out there is like me, I remember half the HTTP status codes by name (e.g. NoContent), and half by number (302 is temporary redirect).

@stephentoub
Copy link
Member

We chose a different strategy for two reasons

Fine concerns, but to me neither of those seems like enough reason to introduce another type with the same values for the same purpose. (And for the second case, if the first is an enum, then you can do (HttpStatusCode)302 if you want the enum value associated with the number.) My $.02.

@davidfowl
Copy link
Member

Can't change this type because it unifies with .NET Framework right? That's another good reason to make a new one.

@stephentoub
Copy link
Member

I'm not following why that's a good reason. Why not just add additional values to HttpStatusCode here and then to the desktop implementation the next time it revs? We need to be able to add additional members/etc. to existing framework types, and shouldn't just add a new type every time we want to augment something. In some cases, sure, it makes sense to branch out; this doesn't seem like one of them to me, at least not for the cited reasons.

@davidfowl
Copy link
Member

@stephentoub Because it forces multi-targeting if anyone wants to use a new enum value.

@stephentoub
Copy link
Member

Or just using it by number until they're able to upgrade to the newer version of the contract, e.g. (HttpStatusCode)302. I'd much prefer that than having two types for the same purpose.

@davidfowl
Copy link
Member

Or just have one type in a new place that doesn't unify because it's just a class and there's little value (IMO) in unifying this enum.

@Tratcher
Copy link
Member

Tratcher commented Nov 9, 2015

This is all getting side-tracked from the real reason we did this; we didn't want the type to be an enum because it's a poor fit for this data set, so we choose const int's instead.

@mteper
Copy link
Author

mteper commented Nov 9, 2015

Actually, it's getting side-tracked because the issue reported here is about missing status codes. 😄

That said, I'll throw in a +1 for the enum over const int because of enum's inherent discoverability. There is already a precedent in the framework for static consts intended to help consistency (around MIME types, I think), but each time I need them, I have to hunt around for the static class that declares them. We can't have string based enums in C# but we can for ints, and it's definitely convenient.

@khellang
Copy link
Member

khellang commented Nov 9, 2015

@mteper We've been through most of this discussion (also, re: discoverability) at aspnet/HttpAbstractions#345 (comment)

@mteper
Copy link
Author

mteper commented Nov 9, 2015

Thanks @khellang!

Funny, I saw that thread back when it was originally discussed and completely forgot about it! FWIW, I'd back your struct-based proposal.

Back to the issue at hand, whatever mechanism the .NET folks decide to go with, can we add in the extra codes now, which the bits are still baking?

@kylelaverty
Copy link

Hey, I was wondering what was going on with this? The Windows guys have already created their own copy of the Enum and extended it: https://msdn.microsoft.com/en-us/library/windows/apps/windows.web.http.httpstatuscode?cs-save-lang=1&cs-lang=csharp#code-snippet-1

@davidsh
Copy link
Contributor

davidsh commented Jul 8, 2016

The WinRT Windows.Web.Http.HttpStatusCode was added as part of the larger effort when the WinRT APIs were added. Those types are distinct from the .NET types. They are used for WinRT apps across UWP apps for Windows for C++, C# and JS languages.

We still have plans to add enum values for .NET System.Net.HttpStatusCode type. However, it is part of the System.Net.Primitives contract and we need to work out how to rev that contract version and also add those values to .NET Framework (Desktop) since that type lives there as well.

@davidsh davidsh removed their assignment Nov 18, 2016
@CIPop
Copy link
Member

CIPop commented Dec 12, 2016

It shouldn't be very hard to add the new enum values to System.Net.Primitives in netcoreapp1.2 and above.
Marking that work as up-for-grabs.

I believe a different issue should be opened to track the discussion regarding adding a new type or unifying with ASP.Net.

@shmuelie
Copy link
Contributor

I'd like to request 418 be added too 😈

@jnm2
Copy link
Contributor

jnm2 commented Dec 14, 2016

My fave is https://en.wikipedia.org/wiki/HTTP_451

@karelz
Copy link
Member

karelz commented Mar 16, 2017

@kwaazaar are you interested in tackling the larger enum update? We can help you with the API review if you need guidance ...

@kwaazaar
Copy link

@karelz: do you want me to create the pullrequest for this issue? That's fine, just let me know.

@karelz
Copy link
Member

karelz commented Mar 17, 2017

Actually, my apologies, I spoke up too soon (previous reply deleted to avoid confusion).
First step is to create API proposal (it has to be thought through carefully). Then we need to approve the API. Then it will be the right time for PR implementation.
It's still up for grabs. Just don't jump directly to API proposal.

@karelz
Copy link
Member

karelz commented Jan 18, 2018

@Caesar1995 can you please prepare the formal API proposal (see the links above - please check latest RFCs as well)

@davidsh
Copy link
Contributor

davidsh commented Jan 19, 2018

@Caesar1995 Please also look thru this list (WinRT Windows.Web.Http.HttpStatusCode) and make sure that all values there are included in System.Net.HttpStatusCode revisions.

See: https://docs.microsoft.com/en-us/uwp/api/windows.web.http.httpstatuscode

@caesar-chen
Copy link
Contributor

We can move discussion to: dotnet/corefx#26441

@karelz
Copy link
Member

karelz commented Jan 19, 2018

I have moved the API proposal to the top post - let's keep this issue as the primary tracking issue - it has upvotes + people interested are already subscribed.

@karelz karelz reopened this Jan 19, 2018
@KeithHenry
Copy link

This needs updating, it's definitely a bug in .NET that it's effectively blind to any HTTP status codes less than about a decade old.

It shouldn't require 3 years of wrangling to update a list of HTTP status codes.

If it does then maybe we need to consider not using this enum as the only representation of the status code, and update classes like HttpClient to return the raw code as well as the enum.

@khellang
Copy link
Member

If it does then maybe we need to consider not using this enum as the only representation of the status code

TBH, an enum is a very poor choice for an open set in the first place. See aspnet/HttpAbstractions#345 (comment) (mentioned earlier in the thread) 😉

@ghost
Copy link

ghost commented Jan 21, 2018

Kristian, I like your struct approach better and it would be EXTREMELY cool if code owners read it as well and approve/dismiss it with proper reasoning in this iteration. However, current fear is that it would delay the arrival of missing codes in any shape by another 2-3 years.. so greedily, whatever gets approved lets snatch that first then reiterate 😉

@Joe4evr
Copy link
Contributor

Joe4evr commented Jan 23, 2018

+       ImATeapot = 418,

Is this actually unironically used that much?

@abatishchev
Copy link
Contributor

The existence of a Fool's Day RFC is not a reason to add it.

@karelz
Copy link
Member

karelz commented Jan 23, 2018

Looks like other stacks (Node.js, Go, Python and ASP.NET) already decided to keep it: https://en.wikipedia.org/wiki/Hyper_Text_Coffee_Pot_Control_Protocol

@karelz
Copy link
Member

karelz commented Jan 23, 2018

@kasper3 @khellang if you are interested in different HttpStatusCode representation (not enum), please file a new API proposal. We should ensure we are adding value on top of existing ASP.NET variant.

@karelz
Copy link
Member

karelz commented Jan 23, 2018

@KeithHenry are you interested in grabbing the issue and updating your PR dotnet/corefx#26415 with full list approved above?

@khellang
Copy link
Member

khellang commented Jan 23, 2018

I think we should avoid adding 418.

The HTTP Working Group has tried hard to get this status code removed from various libraries and languages. A lot of these have decided to keep it around (for now) as it would be a breaking change to remove it.

It would be a mistake to add it to .NET now. See golang/go#21326, nodejs/node#14644, psf/requests#4238 and aspnet/HttpAbstractions#915

// @mnot

@karelz
Copy link
Member

karelz commented Jan 23, 2018

Fair, if all the reasons for keeping it in other platforms were only breaking changes with previous versions, we should not add it (I think we can live with difference between ASP.NET codes and .NET codes).

I would suggest to keep the line in, commented out with link to why it's left out (keeping context for future).

@shmuelie
Copy link
Contributor

shmuelie commented Feb 6, 2018

Now that it IS in IANA, should it be included? (I honestly don't care either way, but it does have a semi-official status now)

@karelz
Copy link
Member

karelz commented Feb 6, 2018

@SamuelEnglard what should be included in your opinion?
It makes 418 reserved, IMO more for the purpose of other standards to not reuse the number, rather than something this enumeration should capture. Or am I missing something?

@khellang
Copy link
Member

khellang commented Feb 6, 2018

That's a draft. https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml (last updated 2017-12-20) still says 418 is "Unassigned" 🤔

Anyway, I don't think there's any point in including status codes that are "Reserved":

This document changes 418 to the status of "Reserved" in the IANA HTTP Status Code registry to reflect that.

@shmuelie
Copy link
Contributor

shmuelie commented Feb 6, 2018

@khellang missed that it's still draft status, thanks for that!

As to what to do...

The HTCPCP protocol is built on top of HTTP, with the addition of a
few new methods, header fields and return codes. (src)

So HTCPCP is related to HTTP/1.1 the same way SPDY, HTTP/2, or even WebSockets are. It's another branch in the family. So if we put HTTP/2 status codes in here HTCPCP status codes can too.

@khellang
Copy link
Member

khellang commented Feb 6, 2018

Why so eager to add a non-standard status code against the HTTP working group's wishes? If you need it in your apps, you can still use it, but I don't think there's any point in adding it to the framework now -- especially since it's sailing up to become a reserved status code (probably before .NET Core 2.1 ships) soon.

@shmuelie
Copy link
Contributor

shmuelie commented Feb 6, 2018

I think 418 should be kept around, just maybe not an HTTP status code (HTCPCP status code I suppose).

@khellang
Copy link
Member

khellang commented Feb 7, 2018

Right. That's a different API proposal - maybe in combination with HtcpcpClient? 😉😂

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Projects
None yet
Development

No branches or pull requests