Repository navigation
Reject unknown Connect stream compression with unimplemented #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
217e97e
cc5f673
45d0fc2
a77bba7
a4df3eb
15c8062
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -338,7 +338,7 @@ async def _read_get_request( | |
| message = message.encode("utf-8") | ||
|
|
||
| # Handle compression | ||
| compression_name = params.get("compression", ["identity"])[0] | ||
| compression_name = params.get("compression", [""])[0] or "identity" | ||
| compression = self._compressions.get(compression_name) | ||
| if not compression: | ||
| raise ConnectError( | ||
|
|
@@ -375,7 +375,7 @@ async def _read_post_request( | |
| req_body = b"".join(chunks) | ||
|
|
||
| # Handle compression if specified | ||
| compression_name = headers.get("content-encoding", "identity").lower() | ||
| compression_name = (headers.get("content-encoding") or "identity").lower() | ||
| compression = self._compressions.get(compression_name) | ||
| if not compression: | ||
| raise ConnectError( | ||
|
|
@@ -410,8 +410,10 @@ async def _handle_stream( | |
| try: | ||
| await metadata_run.start() | ||
| if not req_compression: | ||
| compression_name = headers.get(protocol.compression_header_name()) | ||
| raise ConnectError( | ||
| Code.UNIMPLEMENTED, "Unrecognized request compression" | ||
| Code.UNIMPLEMENTED, | ||
| f"unknown compression: '{compression_name}': supported encodings are {', '.join(self._compressions.keys())}", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have this same f-string in quite a few places; could extract to a helper. Also, we mention
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in The message now comes from Agreed on identity as well. It only showed up because 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. |
||
| ) | ||
| request_stream = _request_stream( | ||
| receive, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withunknown compression: 'xor': supported encodings are Xor, identity. I added a test for that on both servers.