Use status codes defined in HttpAbstractions

[JunTaoLuo]

Reaction to aspnet/HttpAbstractions#748

cc @CesarBS

[cesarblum]

It's harmless but we use hardcoded status codes all over. Either let's leave it as it is, or change other places to use those consts.

Given numeric status codes are nothing cryptic (i.e. people are familiar with them), and not bound to change, I don't really see the need to do this other than to avoid typos (but tests should catch those).

We do have dupe code for reason phrases - https://github.com/aspnet/KestrelHttpServer/blob/49ff98f8cbcf511c343a7076ecdaece2b5306462/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/ReasonPhrases.cs. Consolidating that would be a more interesting change.

[Tratcher]

@CesarBS not everybody has all of the status codes memorized 😄. Especially not @Eilon.

Consolidating the reason phrases will be harder as Kestrel pre-encodes the bytes.

[cesarblum]

@Tratcher We could pass in the strings from HttpAbstractions to the code that pre-encodes the reason phrases, so we don't have duplicate strings in the two repos. Would have to make the dict in HttpAbstractions public though.

[Eilon]

I do like this change. I also don't think it needs to be all-or-none. We can gradually make our codebase better every day! 😄

[JunTaoLuo]

@CesarBS @Tratcher Before I make the suggested changes, let's work out what to do regarding some of the status codes.

1. WebDAV status codes. @Tratcher suggested that we ignore these status codes but we are inconsistent here. For example, we don't have 102 in HttpAbstractions but we have 423. We should keep the status codes consistent between Kestrel and HttpAbstractions so maybe we need to add it?
2. Status code 226. This isn't present in HttpAbstractions but is present in Kestrel. It's a status code unrelated to WebDAV and is used to send responses that involve instance-manipulation (delta encoding of responses).
3. RFC2616 vs RFC7231 reason phrases. For example 413 used to be "Request Entity Too Large" but it is now "Payload Too Large" I'm leaning towards using the new ones since that's what we plan to have in HttpAbstractions.
4. Unofficial status code 419. This isn't defined in the RFC and is missing in Kestrel reason phrases. It's present in HttpAbstractions but I suppose if we need to use it in Kestrel, just specify the status code and reason phrases explicitly. This isn't a problem but we should probably be aware that we have unofficial status codes.
5. Other status codes. 421, 428, 429, 511. We should probably add these to Kestrel as well.

[Tratcher]

A) Avoid removing any existing definitions where possible. Limit compiler breaking changes.
B) Adding everything in the IANA list should be fine. It'll be faster than debating the merits of individual codes.
C) Favor the latest RFC
D) 419 has an official RFC, it just got dropped from the IANA list. we can keep it.

[JunTaoLuo]

🆙📅

[Tratcher]

I like this part.

[Tratcher]

You'd want at least an assert here that GetReasonPhrase returned a non-empty value in case they get out of sync. Or tests for each of these.

[JunTaoLuo]

Yea, I'm also considering other ways to keep them in sync since we might forget to add a status code here. Potentially we'll use reflection to scan for all the status codes defined and generate code.

[halter73]

This might be a good candidate for Microsoft.AspNetCore.Server.Kestrel.GeneratedCode.

[JunTaoLuo]

That still requires us to maintain a list of all the status codes somewhere since the codes are defined as consts. It would be easier if StatusCodes were an enum instead and we can use Enum.GetValues(). That'd be a rather dramatic change though.