-
Notifications
You must be signed in to change notification settings - Fork 103
feat: Add tenant parameter to REST push notification config endpoints #542
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
Conversation
Summary of ChangesHello @ehsavoie, 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 significantly enhances the client and server components by integrating multi-tenancy support through the addition of a Highlights
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
|
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.
Code Review
This pull request introduces significant changes to support multi-tenancy, primarily by adding a tenant parameter to various API calls and updating URL routing. It also includes substantial refactoring, such as renaming supportsAuthenticatedExtendedCard to supportsExtendedAgentCard and cleaning up constructors. While the changes are extensive and mostly correct, I've identified several issues. There are critical errors in test data JSON that will cause parsing failures, buggy tenant path extraction logic in both client and server code, and several missing mappings in the gRPC mappers for the new tenant field. These issues could break multi-tenancy functionality and tests.
client/transport/jsonrpc/src/test/java/io/a2a/client/transport/jsonrpc/JsonMessages.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrpc/src/test/java/io/a2a/client/transport/jsonrpc/JsonMessages.java
Show resolved
Hide resolved
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransport.java
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
spec-grpc/src/main/java/io/a2a/grpc/mapper/DeleteTaskPushNotificationConfigParamsMapper.java
Show resolved
Hide resolved
spec-grpc/src/main/java/io/a2a/grpc/mapper/SetTaskPushNotificationConfigMapper.java
Show resolved
Hide resolved
04f8704 to
45e3caf
Compare
|
/gemini review |
|
/gemini review |
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.
Code Review
This pull request introduces significant changes to add multi-tenancy support, which involved refactoring API paths, request/parameter objects, and updating generated code. The /v1 prefix has been removed from REST API paths to accommodate tenant-specific routes. Additionally, there are several API cleanups, such as replacing generic metadata maps with a specific tenant field in parameter objects, and bug fixes, like handling nullable historyLength. My review identified a critical issue with an inconsistent regular expression for taskId in one of the REST routes, which could lead to routing failures. I also found several instances of inconsistent naming for the task identifier in path parameters, which I've flagged as medium-severity maintainability issues.
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
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.
Code Review
This pull request introduces multi-tenancy support across the A2A client and server, primarily by adding an optional 'tenant' path parameter to various API endpoints and their corresponding client and server-side request objects. Key changes include updating protobuf definitions to include a 'tenant' field in request messages and agent interfaces, modifying REST and JSON-RPC transports to construct URLs and handle requests with tenant information, and adjusting related test cases. Additionally, the pull request renames supportsAuthenticatedExtendedCard to supportsExtendedAgentCard for clarity, refactors TextPart and DataPart constructors to remove an unused metadata parameter, and updates gRPC client error handling to include StatusException. Review comments highlighted several instances where the 'tenant' field was either incorrectly hardcoded to null or missing from mapper configurations in the server-side logic, and also pointed out an incorrect regex for the getAuthenticatedExtendedCard endpoint in the REST server routes.
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Outdated
Show resolved
Hide resolved
spec-grpc/src/main/java/io/a2a/grpc/mapper/DeleteTaskPushNotificationConfigParamsMapper.java
Show resolved
Hide resolved
spec-grpc/src/main/java/io/a2a/grpc/mapper/GetTaskPushNotificationConfigParamsMapper.java
Show resolved
Hide resolved
spec-grpc/src/main/java/io/a2a/grpc/mapper/ListTaskPushNotificationConfigParamsMapper.java
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request introduces a tenant parameter to various REST endpoints to standardize tenant handling and resolve routing ambiguities, particularly for push notification configurations. The changes are comprehensive, affecting the specification, transport layers, server handlers, and tests. The implementation is mostly solid, but I've identified a key issue with the getAuthenticatedExtendedCard endpoint where the new tenant functionality seems to be either incomplete or incorrectly implemented.
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
49a9160 to
0aefff0
Compare
|
/gemini review |
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.
Code Review
This pull request introduces tenant support for REST push notification endpoints, a crucial feature for multi-tenancy. The changes are comprehensive, spanning the client, server, and specification modules to ensure consistent tenant handling across the board. The approach of using regular expressions for routing and a trailing slash to resolve ambiguity between GET and LIST operations is well-executed.
My review focuses on enhancing code maintainability by reducing duplication and addressing a potential issue with an unused parameter. I've identified a few areas in RestTransport.java that could be refactored to be more DRY (Don't Repeat Yourself). Additionally, I've flagged an unused tenant parameter in RestHandler.java that requires attention and suggested a style improvement in A2AServerRoutes.java.
Overall, these are solid changes that improve the API's consistency and capabilities.
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
Show resolved
Hide resolved
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransport.java
Show resolved
Hide resolved
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransport.java
Outdated
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
- Add tenant parameter to RestHandler.getTaskPushNotificationConfiguration to match other push notification methods - Add route with trailing slash to distinguish GET (single config) from LIST in A2AServerRoutes - Update RestTransport to use trailing slash when configId is null - Update tests to use new method signatures - Update REST routes to match the new teannt path element This ensures consistent tenant handling across all push notification endpoints and fixes routing ambiguity between GET and LIST operations. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
…a2aproject#542) - Add tenant parameter to RestHandler.getTaskPushNotificationConfiguration to match other push notification methods - Add route with trailing slash to distinguish GET (single config) from LIST in A2AServerRoutes - Update RestTransport to use trailing slash when configId is null - Update tests to use new method signatures - Update REST routes to match the new teannt path element This ensures consistent tenant handling across all push notification endpoints and fixes routing ambiguity between GET and LIST operations. - [X] Follow the [`CONTRIBUTING` Guide](../CONTRIBUTING.md). - [C] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [X Ensure the tests pass - [X] Appropriate READMEs were updated (if necessary) Fixes a2aproject#531 🦕 Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
Add tenant parameter to RestHandler.getTaskPushNotificationConfiguration to match other push notification methods
Add route with trailing slash to distinguish GET (single config) from LIST in A2AServerRoutes
Update RestTransport to use trailing slash when configId is null
Update tests to use new method signatures
Update REST routes to match the new teannt path element
This ensures consistent tenant handling across all push notification endpoints and fixes routing ambiguity between GET and LIST operations.
Follow the
CONTRIBUTINGGuide.[C] Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
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.[X Ensure the tests pass
Appropriate READMEs were updated (if necessary)
Fixes #531 🦕