Use enum for method rather than string compares#2027
Use enum for method rather than string compares#2027benaadams wants to merge 2 commits intoaspnet:devfrom
Conversation
|
I think that enum types should inherit from lowest possible type. HttpMethod can inherit from sbyte. |
Rather than |
|
It'd mean adding values is an even bigger breaking change than normal if people have decided to store it in a database somewhere. |
|
|
||
| Custom, | ||
|
|
||
| Unknown = byte.MaxValue |
There was a problem hiding this comment.
It's best practice to make Unknown/None the zero value, the default for uninitialized fields.
There was a problem hiding this comment.
Its used as the index into _methodNames array and does some pretty weird things based on with the number/order
There was a problem hiding this comment.
Could just reset it to Get rather than Unknown; isn't a path where it can't get set to something
| } | ||
| set | ||
| { | ||
| CustomMethod = HttpUtilities.GetKnownMethod(value, out var method) ? null : value.ToUpperInvariant(); |
There was a problem hiding this comment.
This mirrors the Http.Sys APIs
Yep, what's there now is just what's needed to make it work. Still needs to be optimized (not just the method but all the pseudo-header field reads). |
| var firstUppercaseChar = (char)(value[0] & ~0x20); | ||
| if (length == 3) | ||
| { | ||
| if (firstUppercaseChar == 'G' && string.Equals(value, "GET", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
OrdinalIgnoreCase
https://tools.ietf.org/html/rfc7230#section-3.1.1:
The request method is case-sensitive.
There was a problem hiding this comment.
Sometimes wonder if they flip coins to determine what is case sensitive in the specs. Will change
There was a problem hiding this comment.
When it doubt use case in-sensitive for parsing, regardless of the spec.
There was a problem hiding this comment.
@Tratcher I would go with stricter parsing when in doubt. The risk is that Kestrel might interpret a request differently than a proxy. For instance, imagine a proxy is configured with rules that should be applied to all POST requests to a given endpoint. With case-insensitive parsing in Kestrel, these rules could possibly be bypassed by sending a pOsT request.
ghost
removed
the
|
This PR needs to do over. We refactored everything for http2 |
8 participants
For GET method
IsHeadtest is performed on the stringMethodwhich sinceGETisn't reference equal toHEADdrops through toString.Equals->OrdinalIgnoreCaseComparer.EqualsHowever the Parser starts with it as an Enum, which then gets converted to a string; so it can just be tested with the enum.
Http/2 change isn't so good; but ti gets method from header collection, so that probably needs a more general revisit
Resolves #2020