Open Kynson opened 1 month ago
@harrishancock I don't suppose you remember why we're only case-insensitive for those six method names? Was this in the spec or something?
If by spec you mean the http spec, there's nothing about special handling casing of only these basic 6 methods. Per the spec, all http methods are case sensitive.
https://www.rfc-editor.org/rfc/rfc7231#section-4
The method token is case-sensitive because it might be used as a gateway to object-based systems with case-sensitive method names.
So we really ought to be treating get
, post
, put
, delete
, head
, and options
as invalid also.
For the original ask here, PATCH
is correct. patch
is incorrect.
Testing chromium, it does appear that chromium's implementation will automatically convert get
to GET
but won't convert patch
to PATCH
... and yeah, this appears to be part of the fetch standard https://fetch.spec.whatwg.org/#methods
It even calls out patch
vs PATCH
explicitly.
So it would appear that our implemented behavior is correct per the fetch spec and accordingly bends a rule of the HTTP RFC to special case these six methods. Good fun.
Quick test across run times for console.log((new Request('http://example.org', { method: 'patch' }).method)
patch
(per the spec, not uppercased)patch
(per the spec, not uppercased)PATCH
(does not match spec, uppercased)PATCH
(does not match spec, uppercased)I guess we're still violating spec either way in that if you specify patch
lower-case, we throw an exception, whereas we should accept it. It seems better to uppercase it than to throw an exception I think.
Arguably we should actually expand the underlying KJ HTTP implementation to support arbitrary method names, rather than restricting them to an enum. I'm not excited about doing that, though, as:
CONNECT
, doesn't use standard HTTP message framing.So I'd be in favor of just upper-casing all methods.
We've literally never heard anyone complain about wanting to use a method that's not on our list.
To be fair, they probably wouldn't. If they needed a method that didn't work in workers they're more likely to just not use workers than to complain about it not working
That said, upper-casing all methods is fine as a first step, I think. Likely requires a compat flag tho.
Marking as a feature request rather than a bug.
The Issue
new Request('', {method: 'patch'})
unexpectedly throws error 'Invalid HTTP method string: patch' butnew Request('', {method: 'PATCH'})
works fine.Minimal Reproducible Example: Workers Playground
Expected Behaviour
Both lowercase and uppercase method string should be supported (other methods support both lowercase and uppercase).
Suspected Cause
L1000 of workerd/api/http.c++:
Presumably, the switch statement will run when a lowercase method string is passed to the constructor as
tryParseHttpMethod
failed to parse the lowercase method string. Yet,PATCH
is missing from the switch statement, so the error is thrown. I am not particularly familiar with C++ so I might be wrong with this.