fix: update RestErrorMapper to handle RFC 7807 Problem Details error format#737
fix: update RestErrorMapper to handle RFC 7807 Problem Details error format#737jmesnil merged 2 commits intoa2aproject:mainfrom
Conversation
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 significantly enhances the client's error handling by integrating support for the RFC 7807 Problem Details standard. This change allows the client to correctly interpret and process more structured and detailed error responses from the server, while also ensuring that older, legacy error formats are still handled gracefully. The updates standardize how errors are presented and consumed, improving interoperability and diagnostic capabilities. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for RFC 7807 Problem Details in error responses, which is a great step towards standardizing the API. The changes correctly implement this on both the server-side for generating errors and the client-side for parsing them, including backward compatibility for the old format. My review identifies a couple of high-severity issues: one regarding inconsistent error message handling in the client's error mapper, and another related to silent failures in error serialization on the server. I've also noted a minor documentation issue. The test updates are comprehensive and effectively validate the new error format.
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestErrorMapper.java
Outdated
Show resolved
Hide resolved
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
Show resolved
Hide resolved
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
Show resolved
Hide resolved
be629a0 to
1c35f04
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request implements support for RFC 7807 Problem Details error format, ensuring consistent and standardized error responses across the REST transport layer. The client-side RestErrorMapper has been updated to parse this new format while maintaining backward compatibility with the legacy error structure. Comprehensive unit and integration tests have been added to validate the new error handling for content type and protocol version issues, which is a significant improvement. The new README.md provides documentation for the updated error handling, enhancing clarity for consumers of the API. The review comment suggests adding an explicit content type check for the CreateTaskPushNotificationConfiguration method to ensure consistent error reporting, aligning with the project's error handling strategy.
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
8d679f2 to
a0a4736
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the REST transport layer to handle and produce error responses compliant with RFC 7807 Problem Details. The client-side RestErrorMapper is updated to parse the new format while maintaining backward compatibility. On the server-side, RestHandler now generates RFC 7807 responses, and A2AServerRoutes adds explicit content-type validation for POST endpoints. The changes are well-implemented and include thorough test updates. I have a couple of suggestions to improve maintainability and correct the documentation.
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
…format This covers the core fix: the client's RestErrorMapper now reads the type URI from Problem Details responses (introduced by the server-side changes on this branch) instead of the old error/message fields, with backward compatibility for the legacy format. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement, successfully updating the REST transport to use RFC 7807 Problem Details for error responses. This enhances standards compliance and makes errors clearer for clients. The changes are well-executed across the client-side error mapper and server-side error generation, with backward compatibility thoughtfully maintained. The new documentation in transport/rest/README.md is a valuable addition. I have one suggestion to make the new content-type validation more robust.
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
|
@ehsavoie I think this PR needs to be revisited after the v1.0.0 spec because they changed that section in https://github.com/a2aproject/A2A/blob/v1.0.0/docs/specification.md#116-error-handling |
- Removing consumes from route annotations and adding explicit ContentTypeNotSupportedError validation in sendMessage/sendMessageStreaming - Moving VersionNotSupportedError HTTP status from 501 -> 400 - Adding unit and integration tests for both scenarios fix: HTTP+JSON error mapping is incorrect for ContentTypeNotSupportedError & VersionNotSupportedError Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
|
Let's merge it and start from it before integrating the changes with the v1.0.0 tag of the spec |
This covers the core fix: the client's RestErrorMapper now reads the type URI from Problem Details responses (introduced by the server-side changes on this branch) instead of the old error/message fields, with backward compatibility for the legacy format.
Fixes #727
Fixes #730 🦕