Repository navigation
Reject unknown Connect stream compression with unimplemented - #365
Conversation
A Connect streaming request with an unsupported connect-content-encoding
was treated as identity: a compressed message then failed with internal
("sent compressed message without compression support"), and an
uncompressed one went through. The spec asks servers to handle this
header like content-encoding and answer unimplemented with the supported
encodings, as connect-go does and as connect-py already does for Connect
unary and gRPC.
An empty header still means identity, as in connect-go. The stream path
now reuses the unary error message, so gRPC and gRPC-Web errors list the
supported encodings too.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
An empty content-encoding, grpc-encoding, or GET compression parameter was rejected with unimplemented on every path except Connect streams. Treat it as identity everywhere, as connect-go does, so all paths handle request compression the same way. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
looks right to me, just had some small suggestions / nits. definitely would be nice to send an issue/PR for stream compression upstream to connectrpc/conformance; I would think that would have caught these issues
|
|
||
| # Handle compression if specified | ||
| compression_name = headers.get("content-encoding", "identity").lower() | ||
| compression_name = (headers.get("content-encoding") or "identity").lower() |
There was a problem hiding this comment.
pre-existing, but should we drop the .lower() here and in the sync server code? Looks like neither the stream path nor connect-go do this.
There was a problem hiding this comment.
Good catch, dropped it in a77bba7. It turned out to be more than an inconsistency: a compression registered under a name with uppercase letters (say Xor) worked for streams, but unary requests failed with unknown compression: 'xor': supported encodings are Xor, identity. I added a test for that on both servers.
| raise ConnectError( | ||
| Code.UNIMPLEMENTED, "Unrecognized request compression" | ||
| Code.UNIMPLEMENTED, | ||
| f"unknown compression: '{compression_name}': supported encodings are {', '.join(self._compressions.keys())}", |
There was a problem hiding this comment.
we have this same f-string in quite a few places; could extract to a helper. Also, we mention identity (assuming _default_compressions), which feels a bit weird?
There was a problem hiding this comment.
Done in a4df3eb and 15c8062.
The message now comes from unknown_compression_error in _compression.py, used in all six places.
Agreed on identity as well. It only showed up because resolve_compressions adds it for lookups, and connect-go and connect-es list only real compressions. It's no longer listed at all. With compression disabled, the message now says compression is not supported, where connect-go ends it with an empty list.
This change makes #361 conflict in the sync server's unary POST and GET paths (the lines right after the raise), but keeping both sides resolves it.
The Connect unary POST path lowercased content-encoding before looking it up, unlike every other path, connect-go, and connect-es. A compression registered under a name with uppercase letters therefore worked for streams but failed unary requests with unimplemented. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
The same unknown compression message was built in six places. Build it in one helper, and leave identity out of the supported encodings unless nothing else is supported: identity is always accepted, it only showed up because resolve_compressions adds it for lookups, and connect-go and connect-es list only real compressions. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
With no compression configured, the unknown compression error still listed identity as the only supported encoding. identity is always accepted and is not a compression, so say that compression is not supported instead of listing it or ending with an empty list. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
|
Thanks for the review!
Agreed on conformance. I'll open an issue on |
Each server path looked up the request compression on its own, and the copies drifted apart: #365 fixed an unknown Connect stream compression being treated as identity, Connect unary POST lowercasing the name, and empty encodings being accepted on some paths only. Resolve the name in resolve_request_compression instead, so every path treats an empty name as identity and matches names exactly. No behavior change. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: i2y <6240399+i2y@users.noreply.github.com>
2 participants
Connect streaming requests with an unsupported
connect-content-encodingare currently treated as identity. A compressed message then fails withinternal("sent compressed message without compression support"), and an uncompressed one goes through.The spec asks servers to handle this header like
content-encodingand answerunimplementedwith the supported encodings. connect-go does this for every protocol, and connect-py already does it for Connect unary and gRPC, so this brings Connect streams in line.While at it, request compression now works the same way on every path:
content-encodingorgrpc-encodingheader, or an emptycompressionquery parameter) means identity, as in connect-go. Connect unary and gRPC used to reject it withunimplemented, while Connect streams accepted it.content-encoding, so a compression registered asXorworked for streams but failed unary requests.GZIPis now rejected there too, as it already was on the other paths.unknown_compression_error. Like connect-go, it lists only real compressions, so identity no longer shows up. When none are configured, it sayscompression is not supportedinstead of ending with an empty list.Conformance only has
unexpected-compressioncases for Connect unary and gRPC/gRPC-Web, which is likely why this slipped through, so a Connect streaming case belongs there too.A heads-up for #348:
_server_anyio.pyhas its own copy of this check with the old message, so whichever PR lands second may want to switch it tounknown_compression_error.The new behavior tests in
test_compression.pyfail onmain(20 of 30) and pass here.uv run poe checkand the server conformance suites pass too.