fix: add extra grpc header which is all lower case#756
fix: add extra grpc header which is all lower case#756timn wants to merge 1 commit intoa2aproject:mainfrom
Conversation
According to https://httpwg.org/specs/rfc7540.html#HttpHeaders headers in HTTP/2 (grpc transport) must all be lower case. Thus, using A2A via grpc fails when using the HTTP header. Add specific all lowercase header for gRPC.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where A2A communication via gRPC failed due to incorrect HTTP header casing. It introduces a dedicated lowercase header for gRPC extensions and updates the gRPC request handler and associated tests to utilize this new header, ensuring adherence to HTTP/2 specifications for gRPC transport. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with gRPC headers by introducing a new lowercase constant GRPC_EXTENSION_HEADER and using it in the gRPC handler, aligning with HTTP/2 specifications. The changes are logical and the accompanying test modifications ensure correctness. My feedback includes a few minor suggestions to remove redundant .lower() calls on the new constant, which is already defined in lowercase, to improve code clarity.
| context.set_trailing_metadata( | ||
| [ | ||
| (HTTP_EXTENSION_HEADER.lower(), e) | ||
| (GRPC_EXTENSION_HEADER.lower(), e) |
There was a problem hiding this comment.
The GRPC_EXTENSION_HEADER constant is already in lowercase. The call to .lower() is redundant and can be removed for clarity.
| (GRPC_EXTENSION_HEADER.lower(), e) | |
| (GRPC_EXTENSION_HEADER, e) |
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | ||
| (GRPC_EXTENSION_HEADER.lower(), 'bar'), |
There was a problem hiding this comment.
The GRPC_EXTENSION_HEADER constant is already in lowercase. The calls to .lower() are redundant and can be removed for clarity.
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | |
| (GRPC_EXTENSION_HEADER.lower(), 'bar'), | |
| (GRPC_EXTENSION_HEADER, 'foo'), | |
| (GRPC_EXTENSION_HEADER, 'bar'), |
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | ||
| (GRPC_EXTENSION_HEADER.lower(), 'baz'), |
There was a problem hiding this comment.
The GRPC_EXTENSION_HEADER constant is already in lowercase. The calls to .lower() are redundant and can be removed for clarity.
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | |
| (GRPC_EXTENSION_HEADER.lower(), 'baz'), | |
| (GRPC_EXTENSION_HEADER, 'foo'), | |
| (GRPC_EXTENSION_HEADER, 'baz'), |
| (GRPC_EXTENSION_HEADER.lower(), 'foo ,, bar,'), | ||
| (GRPC_EXTENSION_HEADER.lower(), 'baz , bar'), |
There was a problem hiding this comment.
The GRPC_EXTENSION_HEADER constant is already in lowercase. The calls to .lower() are redundant and can be removed for clarity.
| (GRPC_EXTENSION_HEADER.lower(), 'foo ,, bar,'), | |
| (GRPC_EXTENSION_HEADER.lower(), 'baz , bar'), | |
| (GRPC_EXTENSION_HEADER, 'foo ,, bar,'), | |
| (GRPC_EXTENSION_HEADER, 'baz , bar'), |
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | ||
| (GRPC_EXTENSION_HEADER.lower(), 'bar'), |
There was a problem hiding this comment.
The GRPC_EXTENSION_HEADER constant is already in lowercase. The calls to .lower() are redundant and can be removed for clarity.
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | |
| (GRPC_EXTENSION_HEADER.lower(), 'bar'), | |
| (GRPC_EXTENSION_HEADER, 'foo'), | |
| (GRPC_EXTENSION_HEADER, 'bar'), |
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | ||
| (GRPC_EXTENSION_HEADER.lower(), 'baz'), |
There was a problem hiding this comment.
The GRPC_EXTENSION_HEADER constant is already in lowercase. The calls to .lower() are redundant and can be removed for clarity.
| (GRPC_EXTENSION_HEADER.lower(), 'foo'), | |
| (GRPC_EXTENSION_HEADER.lower(), 'baz'), | |
| (GRPC_EXTENSION_HEADER, 'foo'), | |
| (GRPC_EXTENSION_HEADER, 'baz'), |
1 participant
According to https://httpwg.org/specs/rfc7540.html#HttpHeaders headers in HTTP/2 (grpc transport) must all be lower case. Thus, using A2A via grpc fails when using the HTTP header. Add specific all lowercase header for gRPC.
Description
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.bash scripts/format.shfrom the repository root to format)