Fix overlay, jsonpath, schema unmarshaling, and converter bugs#359
Merged
Fix overlay, jsonpath, schema unmarshaling, and converter bugs#359
Conversation
- fix(jsonpath): Modify() on root path, Remove() splice for arrays and recursive descent, filter segments properly update parent references - fix(parser): Schema.UnmarshalJSON promotes map[string]any back to *Schema for Items/AdditionalProperties and related fields; Operation marshals empty Security as [] instead of omitting it - fix(converter): transfer all OAS 2.0 validation keywords and Items when converting parameters to OAS 3.x - fix(makefile): replace local SDK path pinning with GOTOOLCHAIN=auto - fix(tests): replace t.Fatal/t.Skip nil guards with require.NotNil and explicit return to resolve SA5011 staticcheck issues Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- fix(jsonpath): add RecursiveSegment case to modifyInParent so $..field overlay updates actually apply instead of silently no-oping - fix(jsonpath): add depth limits to removeFromParentAt/modifyInParentAt (matching maxRecursionDepth=500 used by recursiveDescend) - fix(jsonpath): Modify() root path returns error for non-map documents instead of silently discarding fn's return value - fix(parser): promoteSchemaOrBool returns (any, error) so marshal/unmarshal failures surface as parse errors rather than silent type-assertion panics - fix(converter): warn when param.Items.CollectionFormat is non-csv, matching the existing top-level parameter warning - test(jsonpath): assert array splice correctness for index removal; add RecursiveSegment-through-arrays and modifyInParent recursive tests - test(parser): cover AdditionalItems, UnevaluatedItems, UnevaluatedProperties promotion in Schema.UnmarshalJSON - test(converter): cover ExclusiveMaximum/ExclusiveMinimum transfer and items.collectionFormat warning Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- fix(jsonpath): move depth-limit check before child-segment application in removeFromParentAt/modifyInParentAt for consistent truncation behaviour matching recursiveDescend - fix(parser): remove unused type-switch variable in promoteSchemaOrBool - fix(converter): newConverter() now calls New() to match production initialization (IncludeInfo defaults to true) - test(parser): add bool=true cases for additionalProperties, and bool preservation tests for additionalItems/unevaluatedItems/unevaluatedProperties - test(parser): add populated security marshal and empty-security round-trip subtests to TestOperationEmptySecurityMarshal - test(overlay): add root-target overwrites-existing-key subtest Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds richer OAS2→OAS3 parameter/schema conversion, fixes JSONPath Modify/Remove to support root and recursive splices, promotes JSON-decoded schema fields to concrete types, changes Operation security marshaling to distinguish nil vs non-nil-empty, modernizes some tests to use testify, and updates Makefile toolchain handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
==========================================
+ Coverage 84.77% 84.81% +0.04%
==========================================
Files 194 194
Lines 27246 27345 +99
==========================================
+ Hits 23097 23192 +95
+ Misses 2829 2820 -9
- Partials 1320 1333 +13
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/jsonpath/eval.go (1)
352-388:⚠️ Potential issue | 🟠 MajorClear removed slice entries before reslicing.
The new splice paths shorten the slice, but they still leave removed
anyvalues in the backing array (v[:0], filter compaction, and the no-allocation index-removal case). On large docs that keeps removed subtrees live until the whole document is dropped.Proposed fix
case IndexSegment: if arr, ok := parent.([]any); ok { idx := s.Index if idx < 0 { idx = len(arr) + idx } if idx >= 0 && idx < len(arr) { - return append(arr[:idx:idx], arr[idx+1:]...) + copy(arr[idx:], arr[idx+1:]) + arr[len(arr)-1] = nil + return arr[:len(arr)-1] } } @@ case WildcardSegment: switch v := parent.(type) { case map[string]any: for key := range v { delete(v, key) } case []any: - return v[:0] + clear(v) + return v[:0] } @@ case FilterSegment: switch v := parent.(type) { case map[string]any: for key, val := range v { if evalFilter(val, s.Expr) { delete(v, key) } } case []any: - result := v[:0] - for _, elem := range v { - if !evalFilter(elem, s.Expr) { - result = append(result, elem) - } - } - return result + n := 0 + for _, elem := range v { + if !evalFilter(elem, s.Expr) { + v[n] = elem + n++ + } + } + clear(v[n:]) + return v[:n] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jsonpath/eval.go` around lines 352 - 388, The slice splice paths leave removed elements live in the backing array; clear those references to allow GC. In the IndexSegment case (symbol: IndexSegment, variable: arr, field: s.Index) set arr[idx] = nil before returning the spliced slice; in the WildcardSegment case for []any (symbol: WildcardSegment, variable: v) nil out each element (for i := range v { v[i] = nil }) before returning v[:0]; in the FilterSegment case for []any (symbol: FilterSegment, variable: v, helper: evalFilter) compact kept elements into result as you already do but then nil any trailing slots in the original backing array (for i := len(result); i < len(v); i++ { v[i] = nil }) before returning result so removed subtrees are not retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@converter/helpers_test.go`:
- Around line 722-747: Add a unit test that verifies convertOAS2ParameterToOAS3
handles an array parameter with nil Items: create a
TestConvertOAS2ParameterToOAS3_ArrayWithoutItems that constructs a
parser.Parameter with Type="array" and Items=nil, calls
c.convertOAS2ParameterToOAS3(param, result, "test"), and asserts the returned
parameter and its Schema are non-nil, Schema.Type == "array", and Schema.Items
is nil; reference newConverter() to obtain the Converter and use
ConversionResult to collect issues.
In `@converter/helpers.go`:
- Around line 70-75: The conversion currently copies boolean flags
param.ExclusiveMaximum/ExclusiveMinimum into
converted.Schema.ExclusiveMaximum/ExclusiveMinimum, which is invalid for OAS
3.1+ where these fields are numeric boundaries; update the conversion logic to
detect the target OAS version (e.g., an existing target/version variable or
function context) and: for OAS <3.1 keep the boolean copy, but for OAS 3.1+ set
converted.Schema.ExclusiveMaximum = param.Maximum (or
converted.Schema.ExclusiveMaximum = param.Maximum if param.ExclusiveMaximum is
true) and converted.Schema.ExclusiveMinimum = param.Minimum when the respective
flags are true, or alternatively emit a conversion issue when you cannot derive
the numeric boundary; reference the param.ExclusiveMaximum,
param.ExclusiveMinimum, param.Maximum, param.Minimum and
converted.Schema.ExclusiveMaximum/ExclusiveMinimum symbols when making the
change.
In `@internal/jsonpath/eval.go`:
- Around line 121-127: The root-path Modify branch currently accepts typed-nil
maps because the type assertion only checks map type, so before calling fn(doc)
ensure doc is a non-nil map: cast doc to m, ok := doc.(map[string]any); if !ok
|| m == nil return fmt.Errorf(...); otherwise call fn with m (or doc) as before;
update the error message to indicate a nil-map was provided; also add a
regression test that passes var doc map[string]any (a typed nil) to Modify to
verify it fails fast.
In `@internal/jsonpath/jsonpath_test.go`:
- Around line 1025-1043: TestModifyRoot currently only asserts the happy path;
add negative cases that call Parse("$") and then p.Modify on documents that are
(a) a non-map value (e.g. a string or int) and (b) a typed-nil map like var m
map[string]any = nil, to verify Modify("$") returns an error (using
require.Error) rather than panicking or silently succeeding; locate Parse and
the p.Modify call inside TestModifyRoot and add assertions that the error is
returned and that the original value is unchanged for these cases.
In `@overlay/overlay_test.go`:
- Around line 1220-1225: mustParseSpec currently constructs a raw map and
therefore bypasses the parser-backed code path that Apply() uses; add a
parser-backed test case so the new $ / $.. behavior is exercised end-to-end.
Update tests to include a fixture that parses spec JSON via the real parser (use
parser.ParseResult produced by the parser, or add a helper named like
mustParseSpecFromParser that calls the parser to return *parser.ParseResult) and
use that result in an Apply() test to cover integration behavior rather than
only the raw map path. Ensure the new test uses the same spec JSON as the
existing cases and asserts the same expectations so regressions on parser-backed
paths are detected.
In `@parser/schema_json_test.go`:
- Around line 287-296: Add two regression tests to cover the edge cases: (1)
ensure items boolean values are preserved as bool when unmarshalling (create a
test similar to the existing one that unmarshals `{"type":"array","items":true}`
into a Schema and asserts s.Items is a bool and true), and (2) ensure
items-as-array (tuple form) unmarshals to a slice of *Schema (unmarshal
`{"type":"array","items":[{...},{...}]}` into Schema and assert s.Items is of
type []*Schema with expected length and contents). Reference the existing test
pattern that uses Schema and s.Items to place and implement these new cases.
In `@parser/schema_json.go`:
- Around line 119-140: promoteSchemaOrBool currently only handles
nil/bool/*Schema and map[string]any but must also convert []any (tuple-style
OAS2 "items") into []*Schema like decodeSchemaOrBool does; change
promoteSchemaOrBool to detect case []any, iterate over elements and for each
element call decodeFromMap (or promote using the same logic used for
map[string]any) to produce a []*Schema slice, returning it or an error if any
element fails, and replace the json.Marshal/json.Unmarshal round-trip for
map[string]any with decodeFromMap to decode into a Schema directly (referencing
promoteSchemaOrBool, decodeSchemaOrBool, decodeFromMap, and Schema to locate the
code to update).
---
Outside diff comments:
In `@internal/jsonpath/eval.go`:
- Around line 352-388: The slice splice paths leave removed elements live in the
backing array; clear those references to allow GC. In the IndexSegment case
(symbol: IndexSegment, variable: arr, field: s.Index) set arr[idx] = nil before
returning the spliced slice; in the WildcardSegment case for []any (symbol:
WildcardSegment, variable: v) nil out each element (for i := range v { v[i] =
nil }) before returning v[:0]; in the FilterSegment case for []any (symbol:
FilterSegment, variable: v, helper: evalFilter) compact kept elements into
result as you already do but then nil any trailing slots in the original backing
array (for i := len(result); i < len(v); i++ { v[i] = nil }) before returning
result so removed subtrees are not retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c00a7b36-2c0a-4ee6-89df-60455f210049
📒 Files selected for processing (14)
Makefileconverter/converter_test.goconverter/helpers.goconverter/helpers_test.gofixer/fixer_corpus_test.gointernal/jsonpath/eval.gointernal/jsonpath/jsonpath_test.gooverlay/overlay_test.goparser/internal/jsonhelpers/helpers.goparser/parser_test.goparser/paths_json.goparser/paths_json_test.goparser/schema_json.goparser/schema_json_test.go
- jsonpath: guard against typed-nil map[string]any in Modify root path (a typed nil passes the type assertion but panics on write) - parser: handle []any in promoteSchemaOrBool for OAS 2.0 tuple-form items arrays (matches existing decodeSchemaOrBool YAML path) - tests: add TestModifyRoot_InvalidDoc for non-map and typed-nil cases - tests: add Items bool and tuple-array round-trip cases to schema JSON tests - tests: add TestConvertOAS2ParameterToOAS3_ArrayWithoutItems for nil Items Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@parser/schema_json.go`:
- Around line 138-150: The tuple-array branch currently builds a []*Schema and
silently drops promoted non-*Schema values; change the logic in the `case
[]any:` branch to collect all promoted results (not just `*Schema`) into a
`[]any` (e.g., name it `promotedElems`), appending `promoted` for every element
after `promoteSchemaOrBool` succeeds, and return that slice so boolean or
passthrough elements (like `true`) are preserved; update the function return
type and any callers of this function accordingly so the branch can return
`[]any` instead of `[]*Schema`, while keeping the existing error handling from
`promoteSchemaOrBool`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce16e9c7-8b03-46ff-a7ed-b5aee06fd8f1
📒 Files selected for processing (5)
converter/helpers_test.gointernal/jsonpath/eval.gointernal/jsonpath/jsonpath_test.goparser/schema_json.goparser/schema_json_test.go
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
1 participant
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.
Summary
Fixes seven open issues spanning jsonpath evaluation, overlay application, schema JSON unmarshaling, parameter conversion, and operation serialization.
Closes #349
Closes #350
Closes #351
Closes #352
Closes #353
Closes #355
Closes #357
Changes
JSONPath (
internal/jsonpath) — fixes #350, #351, #352Modify(): was returning an error; now correctly callsfnin-place for map documents (overlay: target "$" (root) fails with "cannot modify root in place" #350)Remove()($..field): matched nodes but never deleted them; fixed by addingRecursiveSegmentsupport throughoutremoveFromParentAt(overlay: recursive descent removal ($..field) matches but does not delete #351)Remove(): was leaving nil gaps; now usesappend(arr[:i:i], arr[i+1:]...)to produce a correctly-sized slice (overlay: array element filter removal leaves null entries instead of splicing #352)modifyInParentrecursive descent: was silently no-oping on$..fieldupdates; addedRecursiveSegmentcase tomodifyInParentAtremoveFromParentAt/modifyInParentAtnow cap recursion atmaxRecursionDepth=500, consistent withrecursiveDescendModify()root non-map guard: returns an error for non-map root documents instead of silently discarding the return value offnSchema JSON (
parser/schema_json.go) — fixes #353, #355Schema.UnmarshalJSON: thetype Aliastrick bypasses custom unmarshalers, leavingany-typed fields (Items,AdditionalProperties,AdditionalItems,UnevaluatedItems,UnevaluatedProperties) asmap[string]any. AddedpromoteSchemaOrBool()to promote them back to*Schemaafter decoding, preventing type-assertion panics downstream (converter: JSON fast-path leaves $ref unrewritten in any-typed schema fields (Items, AdditionalProperties) #353, Semantic dedup rewriter silently skipsany-typed schema fields (Items, AdditionalProperties, etc.) #355)promoteSchemaOrBoolnow returns(any, error)so parse failures surface immediately rather than as silent type mismatches far from the decode siteOperation Security Marshaling (
parser/paths_json.go) — fixes #349Operation.MarshalJSON()fast-path was skipping the slow path whenSecurity != nil, so an empty-but-non-nil slice was omitted instead of serialized as[]SetIfSliceNotNilgeneric helper tojsonhelpersto distinguish nil (omit) from[]T{}(serialize as[])Parameter Conversion (
converter/helpers.go) — fixes #357convertOAS2ParameterToOAS3was only copyingType/Formatwhen building the OAS 3.x schema from an inline-typed parameter; all OAS 2.0 validation keywords (minimum,maximum,exclusiveMinimum,exclusiveMaximum,minLength,maxLength,pattern,enum,default,multipleOf,minItems,maxItems,uniqueItems) and nesteditemsare now transferred (converter: array parameter items lost when converting OAS 2.0 → 3.0 #357)collectionFormatonitemsnow emits a conversion warning, matching the existing top-level parameter warningBuild (
Makefile)GOROOT/PATHSDK pinning withexport GOTOOLCHAIN = auto— portable across CI and all developer machines without hardcoded~/sdk/go1.25.8pathsTest Plan
make checkpasses (lint + format + all tests)TestRemoveArrayIndex_Splices,TestRemoveArrayFilter_Splices,TestRemoveRecursiveDescent,TestRemoveRecursive_ThroughArrays,TestModifyRoot,TestModifyRecursiveDescentTestApplyRootTarget,TestApplyRecursiveRemove,TestApplyArrayFilterRemoveTestSchemaJSONRoundTripsubtests for Items, AdditionalProperties, AdditionalItems, UnevaluatedItems, UnevaluatedProperties (both*Schemaandboolforms)TestOperationEmptySecurityMarshalincluding populated and round-trip subtestsTestConvertOAS2ParameterToOAS3_ArrayItems,TestConvertOAS2ParameterToOAS3_ValidationKeywords,TestConvertOAS2ParameterToOAS3_ExclusiveKeywords,TestConvertOAS2ParameterToOAS3_ItemsCollectionFormat🤖 Generated with Claude Code