Set default accept-encoding to compression algorithms supported by the client #71
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e client Signed-off-by: Achille Roussel <achille.roussel@gmail.com>
anuraaga
reviewed
Dec 24, 2025
Collaborator
anuraaga
left a comment
anuraaga
left a comment
There was a problem hiding this comment.
Thanks! This indeed looks like a bug
src/connectrpc/_compression.py
Outdated
| This excludes 'identity' since it's an implicit fallback, and returns | ||
| only compressions that are actually available (i.e., their dependencies are installed). | ||
| """ | ||
| preferred_order = ["gzip", "br", "zstd"] |
Collaborator
There was a problem hiding this comment.
Let's move this out of the function, maybe DEFAULT_ACCEPT_ENCODING_COMPRESSIONS
src/connectrpc/_compression.py
Outdated
| only compressions that are actually available (i.e., their dependencies are installed). | ||
| """ | ||
| preferred_order = ["gzip", "br", "zstd"] | ||
| return [name for name in preferred_order if name in _compressions] |
Collaborator
There was a problem hiding this comment.
What do you think about returning the joined string without the intermediate list? A microoptimization, but the list is only benefiting the unit test currently IIUC
Signed-off-by: Achille Roussel <achille.roussel@gmail.com>
Contributor
Author
|
Thanks for the review @anuraaga, I addressed your feedback! |
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
changed the title
anuraaga
approved these changes
Dec 25, 2025
Collaborator
|
Thanks as always @achille-roussel - will see how it goes with the holidays but will try to get this released soon |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I observed content encoding negotiation issues that I believe was caused by the connect-python client having a hardcoded list of supported encodings despite not necessarily having the corresponding modules loaded. This PR should address it by generating the default Accept-Encoding header based on what the client supports, as detected dynamically at run time.