[cozystack-api] Fix non-existing OpenAPI refs - #1208
Conversation
|
""" WalkthroughThis update refactors and extends the OpenAPI schema post-processing logic for both v2 and v3 formats. It introduces new helper functions for cloning and patching schemas, adds support for status schemas, unifies cloning logic, and implements a comprehensive mechanism to rewrite Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant OpenAPIProcessor
User->>Server: Request OpenAPI schema
Server->>OpenAPIProcessor: Build and post-process schema
OpenAPIProcessor->>OpenAPIProcessor: Clone base, status, and list schemas per kind
OpenAPIProcessor->>OpenAPIProcessor: Patch .spec with user schema
OpenAPIProcessor->>OpenAPIProcessor: Rewrite all $ref references
OpenAPIProcessor->>Server: Return processed OpenAPI schema
Server->>User: Respond with OpenAPI schema
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello Andrei Kvapil (@kvaps), 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 generated OpenAPI specifications (both v2 and v3) contained non-existing or incorrect internal references ($ref) for custom resource schemas. My changes introduce a robust system to dynamically clone base schemas, apply correct Group-Version-Kind (GVK) extensions, and globally rewrite all internal references to accurately reflect the specific custom resource kind, ensuring the OpenAPI spec is valid and correctly consumable by API clients.
Highlights
- OpenAPI Reference Correction: I've implemented a new mechanism to dynamically rewrite OpenAPI
$ref(references) within the generated specification. This ensures that schema references, particularly forstatusandlistobjects, correctly point to the specific custom resourcekind(e.g.,MyKindStatus,MyKindList) instead of a genericApplicationtype. - Unified Schema Generation Logic: I've introduced a new helper function,
cloneKindSchemas, to centralize the logic for cloning base schemas (Application,ApplicationList,ApplicationStatus) and applyingx-kubernetes-group-version-kindextensions for each custom resourcekind. This reduces duplication and improves maintainability for both OpenAPI v2 and v3 generation. - Global Document Reference Rewriting: A recursive document traversal (
walkAndRewriteRefs) has been added to find and correct all$refinstances across the entire OpenAPI document. This ensures comprehensive consistency in references after the initial schema cloning and GVK application. - Improved Schema Management: I've introduced an
apiPrefixconstant andbaseStatusRefto improve the organization and completeness of base schema definitions. Minor refinements were also made todeepCopySchemaandfindSpecContainerfor robustness and clarity.
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 (@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 is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments to provide feedback.
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 refactors the OpenAPI post-processing logic to correctly handle and rewrite references for custom resource kinds, fixing an issue with non-existing refs. The changes introduce several helper functions to reduce code duplication between the v2 and v3 processors, which improves maintainability. The review focuses on error handling, identifying places where errors are silently ignored, which could lead to hard-to-debug issues. Suggestions are provided to handle these errors properly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/cmd/server/openapi.go (2)
249-255: Improve error context for document rewriting failures.The error from
rewriteDocRefsor the finaljson.Unmarshalcould be more descriptive.// Rewrite all $ref in the document out, err := rewriteDocRefs(doc) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to rewrite document refs: %w", err) } -return doc, json.Unmarshal(out, doc) +if err := json.Unmarshal(out, doc); err != nil { + return nil, fmt.Errorf("failed to unmarshal rewritten document: %w", err) +} +return doc, nil
308-314: Apply consistent error handling improvements as in v3 processor.Similar to the v3 processor, improve error context for better debugging.
// Rewrite all $ref in the document out, err := rewriteDocRefs(sw) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to rewrite swagger refs: %w", err) } -return sw, json.Unmarshal(out, sw) +if err := json.Unmarshal(out, sw); err != nil { + return nil, fmt.Errorf("failed to unmarshal rewritten swagger: %w", err) +} +return sw, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cmd/server/openapi.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/ferretdb/README.md:35-37
Timestamp: 2025-07-02T09:58:11.406Z
Learning: In the cozystack repository, the maintainer NickVolynkin prefers to keep realistic-looking example credentials in README documentation rather than using generic placeholders like <ACCESS_KEY>, even though they are just examples and not real secrets.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (5)
pkg/cmd/server/openapi.go (5)
16-22: Good refactoring to extract common prefix and add status reference.The extraction of
apiPrefixconstant improves maintainability, and addingbaseStatusRefcompletes the schema reference set needed for proper OpenAPI generation.
35-53: Safe simplification of the Properties check.The removal of
Properties != nilcheck is safe since Go's map access on a nil map returns the zero value andokwill be false.
55-79: Clean refactoring of patchSpec function.The simplified assignment of
AdditionalPropertiesmaintains the same behavior while improving readability.
171-192: Well-implemented reference rewriting logic.The function correctly handles both OpenAPI v3 and v2 reference formats and appropriately transforms Application references to kind-specific ones.
121-130: Add error handling for final JSON marshaling.While the initial unmarshal error is handled, the final
json.Marshalon line 129 could fail and should be checked.func rewriteDocRefs(doc interface{}) ([]byte, error) { raw, _ := json.Marshal(doc) var any interface{} if err := json.Unmarshal(raw, &any); err != nil { return nil, err } walkAndRewriteRefs(any, "") - return json.Marshal(any) + result, err := json.Marshal(any) + if err != nil { + return nil, fmt.Errorf("failed to marshal rewritten document: %w", err) + } + return result, nil }Likely an incorrect or invalid review comment.
fa0b9b6 to
1dd5d83
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
1dd5d83 to
181e8dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cmd/server/openapi.go (1)
92-100: Inconsistent error handling approach.The nil checks after
deepCopySchemacalls are unreachable sincedeepCopySchemawill panic on marshaling errors rather than return nil. Consider either:
- Removing the nil checks since panics are expected behavior
- Modifying
deepCopySchemato return errors instead of panicking- // Ensure we have valid clones - if obj == nil || status == nil || list == nil { - return nil, nil, nil - }Since
deepCopySchemapanics on errors, these nil checks are redundant and unreachable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cmd/server/openapi.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (7)
pkg/cmd/server/openapi.go (7)
16-22: LGTM: Good consolidation of string constants.The addition of
baseStatusRefand consolidation of API prefix constants improves maintainability and aligns with the PR objective of fixing OpenAPI references.
24-39: LGTM: Error handling properly implemented.The function now correctly handles marshaling/unmarshaling errors with panic and proper error wrapping, addressing the critical issues raised in previous reviews. Panicking is appropriate here since schema marshaling failures indicate programming errors rather than recoverable runtime conditions.
68-68: LGTM: Improved struct initialization clarity.The explicit struct initialization for
SchemaOrBoolimproves code readability and makes the intent clearer.Also applies to: 78-78
134-146: LGTM: Proper error handling implemented.The function correctly handles marshaling errors, addressing the issues raised in previous reviews. The approach of using generic interface{} for document manipulation is appropriate for this ref rewriting task.
148-208: LGTM: Comprehensive ref rewriting implementation.The recursive walking logic correctly handles both GVK extraction and ref rewriting. The scoped context approach (passing
currentKindby value) ensures that kind context is properly isolated between different schema sections.The
rewriteRefForKindfunction correctly handles all Application* ref patterns and supports both OpenAPI v2 and v3 formats.
1-332: Overall: Strong refactoring that addresses PR objectives.This comprehensive refactoring successfully addresses the PR objective of fixing non-existing OpenAPI refs by:
- Adding ApplicationStatus support - introduces
baseStatusRefand handles status schemas- Implementing comprehensive ref rewriting - the new
rewriteDocRefsmechanism ensures all$refreferences are properly updated- Improving error handling - addresses previous review comments about silent failures
- Consolidating duplicate code - the
cloneKindSchemashelper eliminates repetitionThe changes maintain backward compatibility while significantly improving the robustness of the OpenAPI schema processing.
46-50: No panic: map key lookup on nilPropertiesis safe
In Go, readings.Properties["spec"]on a nil map returns the zero value andok==false— it does not panic. The removedProperties != nilcheck isn’t needed.
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
Release note
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Bug Fixes