fix: duplication of parameters in json for defining subtypes. #241
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @xumk, 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 the 'kind' property was being duplicated during JSON serialization. The fix involves a precise adjustment to the Jackson's @JsonTypeInfo annotation, ensuring that the existing 'kind' property is correctly utilized for polymorphic type handling rather than creating a redundant one. This change improves the correctness and cleanliness of the generated JSON output.
Highlights
- JSON Serialization Fix: Modified the @JsonTypeInfo annotation in EventKind.java, Part.java, and StreamingEventKind.java.
- Property Handling Correction: Changed the 'include' attribute from JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY to correctly handle the 'kind' property for polymorphic type serialization, resolving a duplication issue.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in Jackson serialization that was causing a duplicate kind property to be generated. The change from JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY is the correct solution, as it instructs Jackson to use the existing getKind() method for type information instead of adding a new one. The fix is consistently applied across all relevant files and effectively resolves the issue.
|
@xumk Thanks for the PR! It looks like there are some test failures related to the change. |
xumk
force-pushed
the
issues/234
branch
2 times, most recently
from
2bc78ea to
605a388
Compare
|
@fjuma yes. i fixed it |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a JSON serialization issue with duplicate properties by switching to JsonTypeInfo.As.EXISTING_PROPERTY and using @JsonTypeName on subtypes. The changes are applied consistently across the affected class hierarchies. However, there is a significant concern: the lack of unit tests to verify this new serialization behavior. Changes to core data models, especially serialization logic, are critical and should be thoroughly tested to ensure correctness and prevent future regressions. I strongly recommend adding serialization tests for the modified class hierarchies (SecurityScheme, EventKind, Part, etc.) before this is merged. I've also left a couple of comments to improve consistency in static imports.
spec/src/main/java/io/a2a/spec/OpenIdConnectSecurityScheme.java
Outdated
Show resolved
Hide resolved
xumk
changed the title
|
@xumk It would be good to add tests for this change. |
|
@fjuma I'll try to do it soon. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue with duplicate properties during JSON serialization of subtypes. The change from JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY in combination with @JsonTypeName on the concrete classes is the right approach to solve this problem. The addition of a new test suite, SubTypeSerializationTest, is excellent. It uses a clever SingleKeyHashMap to ensure that no duplicate keys are produced during serialization, which effectively verifies the fix and protects against future regressions. I've only found a couple of minor naming issues in the new test file that could improve readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of duplicate kind or type properties in JSON serialization for subtypes. The approach of switching to JsonTypeInfo.As.EXISTING_PROPERTY and adding @JsonTypeName to subclasses is correct and follows Jackson best practices. The addition of the SubTypeSerializationTest class is excellent; it provides thorough test coverage for the fix and uses a clever SingleKeyHashMap to automatically detect duplicate keys. The code quality is high. I have one minor suggestion to improve the new test code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of duplicate type identifier properties in the JSON output by correctly configuring Jackson's subtype serialization. The switch to JsonTypeInfo.As.EXISTING_PROPERTY and the use of @JsonTypeName is the correct approach. I appreciate the addition of the SubTypeSerializationTest class, which not only verifies the fix but also prevents future regressions using a clever SingleKeyHashMap to detect duplicate properties. The changes are well-implemented and improve the correctness of the serialization logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of duplicate type properties (kind or type) during JSON serialization of subtypes. The core change of switching from JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY in the base interfaces (EventKind, Part, SecurityScheme, StreamingEventKind) is the correct approach for this problem. The addition of @JsonTypeName to all concrete subclasses is a good practice that improves clarity. I especially appreciate the new SubTypeSerializationTest, which is comprehensive and cleverly uses a custom SingleKeyHashMap to ensure the fix works as expected and prevents regressions. I have one minor suggestion for improving the new test code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with duplicate parameters in JSON serialization for subtypes. The approach of changing JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY and adding @JsonTypeName to subclasses is correct and effectively resolves the problem. The addition of SubTypeSerializationTest with parameterized tests is excellent, as it thoroughly validates the fix and protects against future regressions.
My review includes several suggestions to prevent potential NullPointerExceptions in the SecurityScheme implementations during deserialization, which could occur if the type property is missing from the JSON. I've also included a minor suggestion to improve code readability in the new test file.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of duplicate parameters in JSON serialization for subtypes by correctly configuring Jackson's type handling. The change from JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY is the right approach to prevent duplicate type identifiers. The addition of @JsonTypeName annotations on subclasses and the introduction of comprehensive parameterized tests, which cleverly use a custom SingleKeyHashMap to detect duplicate keys, are excellent. I've found one minor issue regarding a missing null check in one of the constructors, for which I've provided a suggestion.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of duplicate type properties (kind or type) during JSON serialization of polymorphic types. The core fix, changing JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY in base classes and interfaces, is correct and idiomatic for Jackson. The addition of @JsonTypeName to all subtypes is also correctly implemented. Furthermore, the introduction of parameterized tests using a custom SingleKeyHashMap to detect duplicate keys is a clever and robust way to verify the fix and prevent future regressions. The added null-checks in constructors also improve the overall robustness of the code. The changes are well-executed and I have no further recommendations.
|
@fjuma Hi, I added the tests. |
|
@fjuma i added it |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves an issue with duplicate parameters in JSON serialization for subtypes by correctly configuring Jackson's @JsonTypeInfo annotation. The change from JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY is the right fix. I appreciate the extensive refactoring to use @JsonTypeName and constants for type names, which greatly improves code maintainability and readability. The addition of parameter validation in constructors and a comprehensive new test suite (SubTypeSerializationTest) to verify the fix and prevent regressions is excellent. The changes are thorough and well-executed. I have a couple of minor suggestions to further improve encapsulation in the new test class.
… method and add correctly JsonTypeName for type
…ject#241) fixed a2aproject#234 fix kind dublicate because classes have gatKind method and add correctly JsonTypeName for type
2 participants
fixed #234 fix kind dublicate because classes have gatKind method and add correctly JsonTypeName for type