Skip to content

Commit c1bed75

Browse files
erraggyclaude
andcommitted
fix: address CodeRabbit review findings
- 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>
1 parent 90e9d9b commit c1bed75

6 files changed

Lines changed: 85 additions & 9 deletions

File tree

converter/helpers_test.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -740,9 +740,10 @@ func TestConvertOAS2ParameterToOAS3_ItemsCollectionFormat(t *testing.T) {
740740
"should warn about non-csv collectionFormat on items")
741741
}
742742

743-
// newConverter creates a minimal Converter for unit testing helpers.
743+
// newConverter creates a Converter for unit testing helpers using the same
744+
// initialization path as production code.
744745
func newConverter() *Converter {
745-
return &Converter{}
746+
return New()
746747
}
747748

748749
// countIssuesContaining counts issues whose message contains the given substring.

internal/jsonpath/eval.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -390,13 +390,13 @@ func removeFromParentAt(parent any, seg Segment, depth int) any {
390390

391391
case RecursiveSegment:
392392
if s.Child != nil {
393-
// Remove child at this level, then recurse into all descendants.
394-
parent = removeFromParentAt(parent, s.Child, depth)
395393
if depth > maxRecursionDepth {
396394
jsonpathLogger.Warn("jsonpath recursive remove truncated at depth limit",
397395
"depth", depth, "maxDepth", maxRecursionDepth)
398396
return parent
399397
}
398+
// Remove child at this level, then recurse into all descendants.
399+
parent = removeFromParentAt(parent, s.Child, depth)
400400
switch v := parent.(type) {
401401
case map[string]any:
402402
for key, val := range v {
@@ -468,13 +468,13 @@ func modifyInParentAt(parent any, seg Segment, fn func(any) any, depth int) {
468468

469469
case RecursiveSegment:
470470
if s.Child != nil {
471-
// Apply transform at this level, then recurse into all descendants.
472-
modifyInParentAt(parent, s.Child, fn, depth)
473471
if depth > maxRecursionDepth {
474472
jsonpathLogger.Warn("jsonpath recursive modify truncated at depth limit",
475473
"depth", depth, "maxDepth", maxRecursionDepth)
476474
return
477475
}
476+
// Apply transform at this level, then recurse into all descendants.
477+
modifyInParentAt(parent, s.Child, fn, depth)
478478
switch v := parent.(type) {
479479
case map[string]any:
480480
for _, val := range v {

overlay/overlay_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1147,6 +1147,21 @@ func TestApplyRootTarget(t *testing.T) {
11471147

11481148
doc := result.Document.(map[string]any)
11491149
assert.Equal(t, "added", doc["x-custom"], "root update should merge x-custom into document")
1150+
1151+
t.Run("root update overwrites existing key", func(t *testing.T) {
1152+
spec2 := mustParseSpec(t, `{"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{}}`)
1153+
o2 := &Overlay{
1154+
Version: "1.0.0",
1155+
Info: Info{Title: "test", Version: "1.0.0"},
1156+
Actions: []Action{
1157+
{Target: "$", Update: map[string]any{"openapi": "3.1.0"}},
1158+
},
1159+
}
1160+
result2, err2 := a.ApplyParsed(spec2, o2)
1161+
require.NoError(t, err2)
1162+
doc2 := result2.Document.(map[string]any)
1163+
assert.Equal(t, "3.1.0", doc2["openapi"], "root update should overwrite existing key")
1164+
})
11501165
}
11511166

11521167
// TestApplyRecursiveRemove verifies that $..field remove deletes the field from

parser/paths_json_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,4 +625,28 @@ func TestOperationEmptySecurityMarshal(t *testing.T) {
625625
require.NoError(t, err)
626626
assert.Contains(t, string(data), `"security":[]`, "empty non-nil security should marshal as [] even without x- fields")
627627
})
628+
629+
t.Run("populated security slice marshals correctly", func(t *testing.T) {
630+
op := &Operation{
631+
Responses: okResponses,
632+
Security: []SecurityRequirement{
633+
{"api_key": []string{}},
634+
{"oauth2": []string{"read", "write"}},
635+
},
636+
}
637+
data, err := json.Marshal(op)
638+
require.NoError(t, err)
639+
assert.Contains(t, string(data), `"security"`, "populated security should be present")
640+
assert.Contains(t, string(data), `"api_key"`, "security requirement should contain api_key")
641+
assert.Contains(t, string(data), `"oauth2"`, "security requirement should contain oauth2")
642+
})
643+
644+
t.Run("empty security array round-trips through unmarshal", func(t *testing.T) {
645+
input := `{"responses":{"200":{"description":"ok"}},"security":[]}`
646+
var op Operation
647+
err := json.Unmarshal([]byte(input), &op)
648+
require.NoError(t, err)
649+
require.NotNil(t, op.Security, "empty security array should unmarshal to non-nil slice")
650+
assert.Empty(t, op.Security, "security slice should be empty after round-trip")
651+
})
628652
}

parser/schema_json.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,11 @@ func (s *Schema) UnmarshalJSON(data []byte) error {
122122
// map cannot be round-tripped through JSON into a *Schema, so callers get a
123123
// clear parse error rather than a silent type-assertion panic downstream.
124124
func promoteSchemaOrBool(v any) (any, error) {
125-
switch val := v.(type) {
125+
switch v.(type) {
126126
case nil, bool, *Schema:
127127
return v, nil
128128
case map[string]any:
129-
data, err := json.Marshal(val)
129+
data, err := json.Marshal(v)
130130
if err != nil {
131131
return nil, fmt.Errorf("parser: schema field promotion: %w", err)
132132
}

parser/schema_json_test.go

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ func TestSchemaJSONRoundTrip(t *testing.T) {
303303
assert.True(t, ok, "additionalProperties should be *Schema after JSON unmarshal")
304304
})
305305

306-
t.Run("additionalProperties bool preserved", func(t *testing.T) {
306+
t.Run("additionalProperties bool false preserved", func(t *testing.T) {
307307
data := []byte(`{"type":"object","additionalProperties":false}`)
308308
var s Schema
309309
require.NoError(t, json.Unmarshal(data, &s))
@@ -312,6 +312,15 @@ func TestSchemaJSONRoundTrip(t *testing.T) {
312312
assert.False(t, b)
313313
})
314314

315+
t.Run("additionalProperties bool true preserved", func(t *testing.T) {
316+
data := []byte(`{"type":"object","additionalProperties":true}`)
317+
var s Schema
318+
require.NoError(t, json.Unmarshal(data, &s))
319+
b, ok := s.AdditionalProperties.(bool)
320+
assert.True(t, ok, "additionalProperties true should remain bool")
321+
assert.True(t, b)
322+
})
323+
315324
t.Run("additionalItems decoded as *Schema", func(t *testing.T) {
316325
data := []byte(`{"type":"array","additionalItems":{"type":"string"}}`)
317326
var s Schema
@@ -320,6 +329,15 @@ func TestSchemaJSONRoundTrip(t *testing.T) {
320329
assert.True(t, ok, "additionalItems should be *Schema after JSON unmarshal")
321330
})
322331

332+
t.Run("additionalItems bool preserved", func(t *testing.T) {
333+
data := []byte(`{"type":"array","additionalItems":false}`)
334+
var s Schema
335+
require.NoError(t, json.Unmarshal(data, &s))
336+
b, ok := s.AdditionalItems.(bool)
337+
assert.True(t, ok, "additionalItems false should remain bool")
338+
assert.False(t, b)
339+
})
340+
323341
t.Run("unevaluatedItems decoded as *Schema", func(t *testing.T) {
324342
data := []byte(`{"type":"array","unevaluatedItems":{"type":"integer"}}`)
325343
var s Schema
@@ -328,6 +346,15 @@ func TestSchemaJSONRoundTrip(t *testing.T) {
328346
assert.True(t, ok, "unevaluatedItems should be *Schema after JSON unmarshal")
329347
})
330348

349+
t.Run("unevaluatedItems bool preserved", func(t *testing.T) {
350+
data := []byte(`{"type":"array","unevaluatedItems":false}`)
351+
var s Schema
352+
require.NoError(t, json.Unmarshal(data, &s))
353+
b, ok := s.UnevaluatedItems.(bool)
354+
assert.True(t, ok, "unevaluatedItems false should remain bool")
355+
assert.False(t, b)
356+
})
357+
331358
t.Run("unevaluatedProperties decoded as *Schema", func(t *testing.T) {
332359
data := []byte(`{"type":"object","unevaluatedProperties":{"type":"boolean"}}`)
333360
var s Schema
@@ -336,6 +363,15 @@ func TestSchemaJSONRoundTrip(t *testing.T) {
336363
assert.True(t, ok, "unevaluatedProperties should be *Schema after JSON unmarshal")
337364
})
338365

366+
t.Run("unevaluatedProperties bool preserved", func(t *testing.T) {
367+
data := []byte(`{"type":"object","unevaluatedProperties":false}`)
368+
var s Schema
369+
require.NoError(t, json.Unmarshal(data, &s))
370+
b, ok := s.UnevaluatedProperties.(bool)
371+
assert.True(t, ok, "unevaluatedProperties false should remain bool")
372+
assert.False(t, b)
373+
})
374+
339375
t.Run("XML round-trip", func(t *testing.T) {
340376
original := &XML{
341377
Name: "pet",

0 commit comments

Comments
 (0)