Enable writeOnly and test readOnly and deprecated#146
Conversation
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: This PR aims to fix the handling of
writeOnlyanddeprecatedproperties in OpenAPI schema generation. It aligns the behavior with updates in thejsonschema-golibrary and resolves issue #145 by ensuring these properties are correctly reflected from struct tags. - Key components modified:
openapi3/jsonschema.go: Updated logic for converting betweenopenapi3.Schemaandjsonschema.Schema, now using direct field access forwriteOnlyanddeprecatedinstead ofExtraProperties.openapi31/jsonschema.go: Simplified theisDeprecatedhelper function to use the directDeprecatedfield.openapi3/reflect_test.go: Added a new test caseTestReadOnlyWriteOnlyDeprecatedto verify the correct reflection ofreadOnly,writeOnly, anddeprecatedtags..gitignore: Addedgo.workandgo.work.sum.
- Cross-component impacts: This PR has a critical dependency on an unmerged pull request in the
swaggest/jsonschema-gorepository (specifically,swaggest/jsonschema-go#125). The changes rely onwriteOnlyanddeprecatedbeing first-class fields injsonschema.Schema. - Business value alignment: Improves compliance with JSON Schema and OpenAPI specifications. Fixes a bug where
writeOnlywas not correctly reflected, enhancing the accuracy of generated API schemas. The move to strongly-typed fields improves code maintainability and reduces the risk of errors compared to usingExtraProperties.
1.2 Technical Architecture
- System design modifications: The primary architectural change is in how
writeOnlyanddeprecatedschema properties are accessed and propagated. Previously, these were often handled via theExtraPropertiesmap. This PR transitions to using dedicated, strongly-typed fields (e.g.,js.WriteOnly,js.Deprecated) provided by the updatedjsonschema-golibrary. - Component interaction changes: The interaction between
openapi-goandjsonschema-goregarding these properties is now more direct and type-safe. - Integration points impact: Relies on the updated interface of
jsonschema.Schemafrom thejsonschema-golibrary. - Dependency changes and implications: Requires
swaggest/jsonschema-go#125to be merged and this PR'sgo.modto be updated to point to a commit/version ofswaggest/jsonschema-gothat includes these changes. The addition ofgo.workandgo.work.sumto.gitignoresuggests local Go workspace usage, which is generally fine but should align with repository conventions.
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Unmerged External Dependency
- Analysis Confidence: High
- Impact: The PR is currently dependent on
swaggest/jsonschema-go#125, which is not yet merged. Attempting to merge this PR as-is will likely lead to build failures or runtime errors because the necessary fields (WriteOnly,Deprecated) onjsonschema.Schemawill not exist in the version ofjsonschema-gofetched by default. - Resolution:
- Ensure
swaggest/jsonschema-go#125is merged into its base branch. - Update the
go.modfile in this PR to reference the new version or specific commit ofswaggest/jsonschema-gothat includes the merged changes. - Remove any temporary
replacedirectives ingo.modthat might have been used for local development against the unmerged dependency.
- Ensure
Issue: Potential Breaking Change for Consumers Accessing ExtraProperties
- Analysis Confidence: High
- Impact: If any consumers of this library were indirectly relying on
writeOnlyordeprecatedflags being present in theExtraPropertiesmap ofjsonschema.Schemaobjects (whichopenapi-gouses internally), this change will break their code as these properties will now be exclusively in dedicated fields. - Resolution: Clearly document this change in the release notes for the version that includes this PR. Advise users that
writeOnlyanddeprecatedare now handled via direct fields onopenapi3.Schema(which are correctly populated fromjsonschema.Schema's direct fields) and are no longer inExtraPropertiesat thejsonschema-golevel for these specific keywords.
2.2 Should Fix (P1🟡)
Issue: Missing Validation for Conflicting Schema Annotations
- Analysis Confidence: High
- Impact: The OpenAPI specification implies a property should not be both
readOnlyandwriteOnly. While this PR correctly reflects these individual properties, it does not add validation or warnings if a struct field is tagged as both (e.g.,readOnly:"true" writeOnly:"true"). This could lead to semantically confusing or incorrect OpenAPI schemas being generated. - Suggested Solution: Implement logic, likely during schema reflection, to detect if a field is marked as both
readOnlyandwriteOnly. If such a conflict is found, the library could:- Log a warning.
- Prioritize one over the other (e.g.,
readOnlytakes precedence). - Make this behavior configurable, potentially erroring out.
Also, add a test case to verify the handling of such conflicting tags.
Issue: Incomplete Documentation for Struct Tag Usage and Changes
- Analysis Confidence: Medium
- Impact: Developers using this library might not be fully aware of the correct struct tags for
deprecated,readOnly, andwriteOnly, or how their internal handling has changed (especially the move fromExtraPropertiesfordeprecatedandwriteOnly). This can lead to incorrect usage or confusion. - Suggested Solution: Update the library's documentation (e.g., README, GoDoc comments for relevant types/functions) to:
- Clearly list all supported struct tags for schema generation, including
deprecated,readOnly, andwriteOnly, with concise examples. - Briefly mention the internal change that
deprecatedandwriteOnlyare now sourced from direct fields in the underlyingjsonschema-godependency, improving accuracy.
- Clearly list all supported struct tags for schema generation, including
2.3 Consider (P2🟢)
Area: Test Coverage Expansion for Schema Properties
- Analysis Confidence: Medium
- Improvement Opportunity: The new test
TestReadOnlyWriteOnlyDeprecatedis good for positive cases. To further improve robustness, consider adding negative test cases:- A test to ensure that fields without
readOnly,writeOnly, ordeprecatedtags do not have these properties set in the generated schema (i.e., no false positives).
- A test to ensure that fields without
Area: .gitignore additions of Go Workspace files
- Analysis Confidence: Low
- Improvement Opportunity: The addition of
go.workandgo.work.sumto.gitignoreis generally acceptable as these are often user-specific or for local development in multi-module setups. Confirm this aligns with the repository's contribution guidelines or general policy on Go workspace files. If the repository is not typically managed as part of a Go workspace, these might be unnecessary.
2.4 Summary of Action Items
- P0 (Blocker): Resolve the external dependency on
swaggest/jsonschema-go#125by ensuring it's merged and then updating this PR'sgo.modaccordingly. (Timeline: Before Merge) - P0: Document the breaking change related to
ExtraPropertiesin release notes. (Timeline: Before Merge) - P1: Implement validation/handling for conflicting
readOnlyandwriteOnlytags. (Timeline: Before Merge or High Priority Post-Merge) - P1: Update documentation regarding struct tag usage for `deprecated
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
96c2a26 to
6edc33f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #146 +/- ##
==========================================
- Coverage 40.72% 40.11% -0.62%
==========================================
Files 16 16
Lines 6607 10383 +3776
==========================================
+ Hits 2691 4165 +1474
- Misses 3440 5708 +2268
- Partials 476 510 +34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes #145
Depends on swaggest/jsonschema-go#125
Description
See #145.
writeOnlywasn't being properly reflected in openAPI schemas generated by this library because of issues injsonschema-go.TODO
Changes
deprecatedto use new field onSchemaobject rather than readingExtraPropertieswriteOnly,readOnly, anddeprecatedare properly reflectedTesting
Review. See that tests pass.