Ignore content type parameters when matching it#235
Ignore content type parameters when matching it#235anuraaga merged 6 commits intoconnectrpc:mainfrom
Conversation
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
changed the title
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
| codec_name = protocol.codec_name_from_content_type( | ||
| headers.get("content-type", ""), stream=not is_unary | ||
| ) | ||
| codec = self._codecs.get(codec_name.lower()) |
There was a problem hiding this comment.
I also noticed a couple of spots of random lowercasing and consolidated them, with some cleanup to WSGI
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
lgtm, assuming we don't need to worry about the grpc case (e.g. application/grpc+json; charset=utf-8)? I don't see that mentioned in https://connectrpc.com/docs/multi-protocol/ / https://chromium.googlesource.com/external/github.com/grpc/grpc/+/HEAD/doc/PROTOCOL-HTTP2.md but perhaps in practice it ... could happen? 😄
| def validate_response( | ||
| self, request_codec_name: str, status_code: int, response_content_type: str | ||
| ) -> None: | ||
| response_content_type = _normalize_content_type(response_content_type) |
There was a problem hiding this comment.
nit that we're double-normalizing the content type between here and codec_name_from_content_type below, but no real suggestions on a refactor — probably fine to leave.
|
|
||
|
|
||
| @pytest.mark.parametrize("header", _charset_content_type_cases) | ||
| def test_json_charset_content_type(header: str) -> None: |
There was a problem hiding this comment.
would it also be good to flex streaming in these tests? I see we have validate_stream_response; presumably a well-behaved connect server wouldn't have an issue, though...
There was a problem hiding this comment.
Almost thought it's too difficult to be worth it but realized I could get by with a normal client and transport middleware to force content-type
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
force-pushed
the
json-charset-tests
branch
from
bdb57fc to
a59b8d4
Compare
|
While not too worried about it, for gRPC, I think we are covered anyways by the grammar definition not including paramaters, except for the
|
2 participants
Fixes #234
I had intended to have similar handling of content type as Go but realized they treat it quite differently on server vs client side and missed that. This takes the simplest approach of just splitting on
;and ignoring parameters - I don't think anyone will miss the use case of being able to match on them with custom codecs. Note that custom codecs and header matching isn't defined in the protocol spec and i.e. connect-es don't support custom codecs FWICT. We may not have either but it is important for supporting different protobuf implementations.For reference, Go, which does canonicalization in a way that technically custom codecs can match on parameters. Digging in, it looks like their somewhat complicated loop is to avoid two passes for both finding a
;and uppercase (2 passes goes from 9ns to 11ns on my machine). As we can't write such fast loops in Python, I take the typical approach of always calling.strip().lower().https://github.com/connectrpc/connect-go/blob/a5a6c30f3776b06ae05a66ab3bdd2d60c46db6db/protocol.go#L361