Support for JSONSchema SkipUnsupportedProperties#241
Open
mintyleaf wants to merge 1 commit intoswaggest:masterfrom
Open
Support for JSONSchema SkipUnsupportedProperties#241mintyleaf wants to merge 1 commit intoswaggest:masterfrom
mintyleaf wants to merge 1 commit intoswaggest:masterfrom
Conversation
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR adds error handling for unsupported properties in JSON Schema reflection to prevent panics during decoder initialization, but it introduces potential maintainability and testing gaps that could impact future reliability.
🌟 Strengths
- Prevents runtime panics when structs contain unsupported types, enhancing system stability for affected endpoints.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | request/factory.go | Bug | Prevents decoder initialization panics from unsupported types | symbol:DecoderFactory method:makeDefaultDecoder |
| P2 | request/factory.go | Maintainability | Error equality may break with wrapped errors | - |
| P2 | request/factory.go | Testing | Missing tests for skip property handling | - |
🔍 Notable Themes
- Error handling could be strengthened to be more resilient against future changes in dependency implementations.
- Additional test coverage is necessary to validate the new code path and ensure consistent behavior.
📈 Risk Diagram
This diagram illustrates the new error handling logic for unsupported properties during JSON Schema reflection and its associated risks.
sequenceDiagram
participant DF as DecoderFactory
participant JSR as JSONSchema Reflector
DF->>JSR: Reflect(vi)
JSR-->>DF: err or schema
alt err == ErrSkipProperty
DF->>DF: return
else
DF->>DF: panic(err)
end
note over DF: R1(P1): New error handling prevents panics but relies on direct error equality
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
Comment on lines
291
to
297
| s, err := df.JSONSchemaReflector.Reflect(vi) | ||
| if err != nil { | ||
| if err == jsonschema.ErrSkipProperty { | ||
| return | ||
| } | ||
| panic(err.Error()) | ||
| } |
There was a problem hiding this comment.
P1 | Confidence: High
- This change enables graceful handling of
jsonschema.ErrSkipPropertyduring JSON Schema reflection in default value processing. Without this fix, any struct field with unsupported types would cause a panic during decoder initialization, breaking the entire request processing pipeline for affected endpoints. The change preserves existing panic behavior for other errors while allowing proper skipping of unsupported properties. - The error comparison uses direct equality check with
jsonschema.ErrSkipProperty, which could be fragile if the error value implementation changes in the future. Consider usingerrors.Is()for more robust error handling. - The PR doesn't include test coverage for the new error handling path. Tests should verify that structs with unsupported properties don't panic when
ErrSkipPropertyoccurs and that default value processing continues normally for supported fields.
Suggested change
| s, err := df.JSONSchemaReflector.Reflect(vi) | |
| if err != nil { | |
| if err == jsonschema.ErrSkipProperty { | |
| return | |
| } | |
| panic(err.Error()) | |
| } | |
| s, err := df.JSONSchemaReflector.Reflect(vi) | |
| if err != nil { | |
| if errors.Is(err, jsonschema.ErrSkipProperty) { | |
| return | |
| } | |
| panic(err.Error()) | |
| } |
Evidence: path:web/service.go, symbol:DecoderFactory, method:makeDefaultDecoder
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
i have an edgecase, where i need to inline include a really large and upstream dependent go structure from another package into the Input/Output usecase structure definition
That structure has all the needed json tags, yet, it has the
context.CancelFunc 'json:"-"'field, which causes panic at the reflection step due to obviously unsupported typejsonschema has the SkipUnsupportedProperties option for that case, which can be enabled by modifying default decoder options, but it not supported by the rest itself
https://github.com/swaggest/jsonschema-go/blob/a12059cd34136a1006ac484eacff11e4c167d071/context.go#L243