fix: remove schemes nesting#220
Conversation
Summary of ChangesHello @yarolegovich, 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 streamlines the handling of security requirements within the Highlights
Changelog
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 aims to remove a layer of nesting from the SecurityRequirement structure. The changes in the Go type definitions and protobuf conversions correctly reflect this. However, an issue was identified in a2a/auth.go where custom JSON marshalers for SecurityRequirementsOptions re-introduce a nesting layer for JSON serialization, contradicting the PR's intent and creating inconsistency with the now-flattened YAML serialization. This violates the rule that API definitions must follow the governing specification. It is recommended to remove these custom marshalers to achieve a truly flat structure in the JSON API. The other changes in the PR are mostly related to code formatting.
| func (rs SecurityRequirementsOptions) MarshalJSON() ([]byte, error) { | ||
| type wrapper struct { | ||
| Schemes map[SecuritySchemeName]SecuritySchemeScopes `json:"schemes"` | ||
| } | ||
| var out []wrapper | ||
| for _, req := range rs { | ||
| out = append(out, wrapper{Schemes: req}) | ||
| } | ||
| return json.Marshal(out) | ||
| } | ||
|
|
||
| func (rs *SecurityRequirementsOptions) UnmarshalJSON(b []byte) error { | ||
| type wrapper struct { | ||
| Schemes map[SecuritySchemeName]SecuritySchemeScopes `json:"schemes"` | ||
| } | ||
| var wrapped []wrapper | ||
| if err := json.Unmarshal(b, &wrapped); err != nil { | ||
| return err | ||
| } | ||
| result := make(SecurityRequirementsOptions, 0, len(wrapped)) | ||
| for _, w := range wrapped { | ||
| result = append(result, w.Schemes) | ||
| } | ||
| *rs = result | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The custom MarshalJSON and UnmarshalJSON methods for SecurityRequirementsOptions seem to contradict the goal of this pull request, which is to "remove schemes nesting". These methods re-introduce a nesting layer for JSON serialization, which seems to contradict the PR's intent and creates an inconsistency with the now-flattened YAML serialization. This goes against the principle that API definitions must follow the governing specification, prioritizing it over internal code consistency.
These methods wrap the map[SecuritySchemeName]SecuritySchemeScopes in a struct with a schemes field, resulting in a JSON structure like [{"schemes": {...}}]. This is very similar to the old structure which used a scheme key, and doesn't fully remove the nesting from the JSON representation.
If the goal is to truly flatten the structure, I recommend removing these custom marshalers. Without them, SecurityRequirementsOptions would serialize to [{"oauth2": ...}, {"mTLS": ...}], which is a cleaner and flatter API. Since a breaking change is already being introduced (by renaming scheme to schemes), it would be better to go with the simpler, flatter structure which aligns with the PR's title. This would also make the JSON serialization consistent with the YAML serialization, which is now flattened.
References
- API definitions must follow the governing specification, prioritizing it over internal code consistency.
1 participant
No description provided.