diff --git a/Makefile b/Makefile index a4d929d7..5f563853 100644 --- a/Makefile +++ b/Makefile @@ -5,14 +5,11 @@ MAIN_PATH=./cmd/oastools BENCH_DIR=benchmarks BENCH_TIME=5s -# Pin GOROOT to the SDK matching go.mod when system Go is older. -# Update the version here when bumping go.mod's go directive. -GO_SDK := $(wildcard $(HOME)/sdk/go1.25.8) -QUALITY_TARGETS := tidy fmt test test-quick test-race test-full test-coverage test-corpus test-corpus-short integration-test integration-test-debug count-tests count-benchmarks lint vet -ifneq ($(GO_SDK),) -$(QUALITY_TARGETS): export GOROOT = $(GO_SDK) -$(QUALITY_TARGETS): export PATH := $(GO_SDK)/bin:$(PATH) -endif +# Allow go commands to automatically download the toolchain version declared in +# go.mod when the local Go binary is older. This is the portable alternative to +# pinning GOROOT/PATH to a specific SDK path. +QUALITY_TARGETS := tidy fmt test test-quick test-race test-full test-coverage test-corpus test-corpus-short integration-test integration-test-debug count-tests count-benchmarks lint vet check +$(QUALITY_TARGETS): export GOTOOLCHAIN = auto # Default target all: build diff --git a/converter/converter_test.go b/converter/converter_test.go index ddb0b1ac..b20fe47a 100644 --- a/converter/converter_test.go +++ b/converter/converter_test.go @@ -118,10 +118,7 @@ func writeTempYAML(t *testing.T, doc any) string { // TestConverterNew tests the New() constructor func TestConverterNew(t *testing.T) { c := New() - - if c == nil { - t.Fatal("Expected non-nil Converter") - } + require.NotNil(t, c, "Expected non-nil Converter") if c.StrictMode { t.Error("Expected StrictMode to be false by default") @@ -580,14 +577,10 @@ func TestRefRewritingOAS2ToOAS3(t *testing.T) { // Verify refs were rewritten in components/schemas ownerSchema := oas3Doc.Components.Schemas["Owner"] - if ownerSchema == nil { - t.Fatal("Owner schema not found") - } + require.NotNil(t, ownerSchema, "Owner schema not found") petProp := ownerSchema.Properties["pet"] - if petProp == nil { - t.Fatal("Pet property not found") - } + require.NotNil(t, petProp, "Pet property not found") expectedRef := "#/components/schemas/Pet" if petProp.Ref != expectedRef { @@ -596,14 +589,10 @@ func TestRefRewritingOAS2ToOAS3(t *testing.T) { // Verify refs were rewritten in paths pathItem := oas3Doc.Paths["/pets"] - if pathItem == nil { - t.Fatal("Path /pets not found") - } + require.NotNil(t, pathItem, "Path /pets not found") responseSchema := pathItem.Get.Responses.Codes["200"].Content["application/json"].Schema - if responseSchema == nil { - t.Fatal("Response schema not found") - } + require.NotNil(t, responseSchema, "Response schema not found") if responseSchema.Ref != expectedRef { t.Errorf("Expected response schema ref '%s', got '%s'", expectedRef, responseSchema.Ref) @@ -689,14 +678,10 @@ func TestRefRewritingOAS3ToOAS2(t *testing.T) { // Verify refs were rewritten in definitions ownerSchema := oas2Doc.Definitions["Owner"] - if ownerSchema == nil { - t.Fatal("Owner schema not found") - } + require.NotNil(t, ownerSchema, "Owner schema not found") petProp := ownerSchema.Properties["pet"] - if petProp == nil { - t.Fatal("Pet property not found") - } + require.NotNil(t, petProp, "Pet property not found") expectedRef := "#/definitions/Pet" if petProp.Ref != expectedRef { @@ -705,14 +690,10 @@ func TestRefRewritingOAS3ToOAS2(t *testing.T) { // Verify refs were rewritten in paths pathItem := oas2Doc.Paths["/pets"] - if pathItem == nil { - t.Fatal("Path /pets not found") - } + require.NotNil(t, pathItem, "Path /pets not found") responseSchema := pathItem.Get.Responses.Codes["200"].Schema - if responseSchema == nil { - t.Fatal("Response schema not found") - } + require.NotNil(t, responseSchema, "Response schema not found") if responseSchema.Ref != expectedRef { t.Errorf("Expected response schema ref '%s', got '%s'", expectedRef, responseSchema.Ref) diff --git a/converter/helpers.go b/converter/helpers.go index 68d77d64..5991b247 100644 --- a/converter/helpers.go +++ b/converter/helpers.go @@ -51,10 +51,35 @@ func (c *Converter) convertOAS2ParameterToOAS3(param *parser.Parameter, result * if param.Schema != nil { converted.Schema = c.convertOAS2SchemaToOAS3(param.Schema) } else if param.Type != "" { - // Convert type/format to schema + // Convert type/format to schema, transferring all OAS 2.0 validation keywords converted.Schema = &parser.Schema{ - Type: param.Type, - Format: param.Format, + Type: param.Type, + Format: param.Format, + Default: param.Default, + Enum: param.Enum, + Maximum: param.Maximum, + Minimum: param.Minimum, + MaxLength: param.MaxLength, + MinLength: param.MinLength, + Pattern: param.Pattern, + MaxItems: param.MaxItems, + MinItems: param.MinItems, + UniqueItems: param.UniqueItems, + MultipleOf: param.MultipleOf, + } + if param.ExclusiveMaximum { + converted.Schema.ExclusiveMaximum = true + } + if param.ExclusiveMinimum { + converted.Schema.ExclusiveMinimum = true + } + if param.Items != nil { + converted.Schema.Items = convertOAS2ItemsToSchema(param.Items) + if param.Items.CollectionFormat != "" && param.Items.CollectionFormat != "csv" { + c.addIssueWithContext(result, path, + fmt.Sprintf("Parameter items uses collectionFormat '%s'", param.Items.CollectionFormat), + "OAS 3.x uses 'style' and 'explode' instead; 'csv' format maps to style=form") + } } // Handle collection format diff --git a/converter/helpers_test.go b/converter/helpers_test.go index 1aca1607..72a8cde4 100644 --- a/converter/helpers_test.go +++ b/converter/helpers_test.go @@ -639,6 +639,133 @@ func assertHasIssueContaining(t *testing.T, issues []ConversionIssue, substring t.Errorf("Expected at least one issue containing %q, but none found in %d issues", substring, len(issues)) } +// TestConvertOAS2ParameterToOAS3_ArrayItems verifies that Items and validation +// keywords on OAS 2.0 array parameters are transferred to the OAS 3.0 schema +// (issue #357). +func TestConvertOAS2ParameterToOAS3_ArrayItems(t *testing.T) { + c := newConverter() + result := &ConversionResult{} + + strType := "string" + param := &parser.Parameter{ + Name: "chain_id", + In: "query", + Type: "array", + Items: &parser.Items{Type: strType}, + } + + converted := c.convertOAS2ParameterToOAS3(param, result, "test") + require.NotNil(t, converted) + require.NotNil(t, converted.Schema) + assert.Equal(t, "array", converted.Schema.Type) + require.NotNil(t, converted.Schema.Items, "Items should be transferred from OAS 2.0 parameter") + + itemsSchema, ok := converted.Schema.Items.(*parser.Schema) + require.True(t, ok, "Items should be *Schema") + assert.Equal(t, strType, itemsSchema.Type) +} + +func TestConvertOAS2ParameterToOAS3_ValidationKeywords(t *testing.T) { + c := newConverter() + result := &ConversionResult{} + + min := float64(1) + max := float64(100) + minLen := 2 + maxLen := 50 + param := &parser.Parameter{ + Name: "q", + In: "query", + Type: "string", + Minimum: &min, + Maximum: &max, + MinLength: &minLen, + MaxLength: &maxLen, + Pattern: "^[a-z]+$", + Enum: []any{"a", "b"}, + } + + converted := c.convertOAS2ParameterToOAS3(param, result, "test") + require.NotNil(t, converted.Schema) + assert.Equal(t, &min, converted.Schema.Minimum) + assert.Equal(t, &max, converted.Schema.Maximum) + assert.Equal(t, &minLen, converted.Schema.MinLength) + assert.Equal(t, &maxLen, converted.Schema.MaxLength) + assert.Equal(t, "^[a-z]+$", converted.Schema.Pattern) + assert.Equal(t, []any{"a", "b"}, converted.Schema.Enum) +} + +// TestConvertOAS2ParameterToOAS3_ExclusiveKeywords verifies that ExclusiveMaximum +// and ExclusiveMinimum bool flags are transferred to the OAS 3.x schema. +func TestConvertOAS2ParameterToOAS3_ExclusiveKeywords(t *testing.T) { + c := newConverter() + result := &ConversionResult{} + + min := float64(0) + max := float64(10) + param := &parser.Parameter{ + Name: "count", + In: "query", + Type: "integer", + Minimum: &min, + Maximum: &max, + ExclusiveMinimum: true, + ExclusiveMaximum: true, + } + + converted := c.convertOAS2ParameterToOAS3(param, result, "test") + require.NotNil(t, converted.Schema) + assert.Equal(t, true, converted.Schema.ExclusiveMinimum, "ExclusiveMinimum should be transferred") + assert.Equal(t, true, converted.Schema.ExclusiveMaximum, "ExclusiveMaximum should be transferred") +} + +// TestConvertOAS2ParameterToOAS3_ItemsCollectionFormat verifies that a +// non-csv collectionFormat on items generates a conversion warning. +func TestConvertOAS2ParameterToOAS3_ItemsCollectionFormat(t *testing.T) { + c := newConverter() + result := &ConversionResult{} + + param := &parser.Parameter{ + Name: "tags", + In: "query", + Type: "array", + Items: &parser.Items{ + Type: "string", + CollectionFormat: "pipes", + }, + } + + c.convertOAS2ParameterToOAS3(param, result, "test") + assert.Greater(t, countIssuesContaining(result.Issues, "collectionFormat"), 0, + "should warn about non-csv collectionFormat on items") +} + +// TestConvertOAS2ParameterToOAS3_ArrayWithoutItems verifies that an array +// parameter with nil Items is handled gracefully. +func TestConvertOAS2ParameterToOAS3_ArrayWithoutItems(t *testing.T) { + c := newConverter() + result := &ConversionResult{} + + param := &parser.Parameter{ + Name: "tags", + In: "query", + Type: "array", + // Items intentionally nil + } + + converted := c.convertOAS2ParameterToOAS3(param, result, "test") + require.NotNil(t, converted) + require.NotNil(t, converted.Schema) + assert.Equal(t, "array", converted.Schema.Type, "Schema.Type should be array") + assert.Nil(t, converted.Schema.Items, "Schema.Items should be nil when source has no Items") +} + +// newConverter creates a Converter for unit testing helpers using the same +// initialization path as production code. +func newConverter() *Converter { + return New() +} + // countIssuesContaining counts issues whose message contains the given substring. func countIssuesContaining(issues []ConversionIssue, substring string) int { count := 0 diff --git a/fixer/fixer_corpus_test.go b/fixer/fixer_corpus_test.go index 6241e5db..c75e60ae 100644 --- a/fixer/fixer_corpus_test.go +++ b/fixer/fixer_corpus_test.go @@ -22,6 +22,7 @@ func TestCorpus_FixerReducesErrors(t *testing.T) { spec := corpusutil.GetByName("DigitalOcean") if spec == nil { t.Skip("DigitalOcean spec not found in corpus") + return } corpusutil.SkipIfNotCached(t, *spec) corpusutil.SkipLargeInShortMode(t, *spec) @@ -161,6 +162,7 @@ func TestCorpus_FixerWithInferTypes(t *testing.T) { spec := corpusutil.GetByName("Asana") if spec == nil { t.Skip("Asana spec not found in corpus") + return } corpusutil.SkipIfNotCached(t, *spec) diff --git a/internal/jsonpath/eval.go b/internal/jsonpath/eval.go index 599afb00..e0f3dd06 100644 --- a/internal/jsonpath/eval.go +++ b/internal/jsonpath/eval.go @@ -67,28 +67,44 @@ func (p *Path) Set(doc any, value any) error { // Remove removes all matching nodes from the document. // // Returns the modified document. For maps, matching keys are deleted. -// For arrays, matching indices are removed (with index shift). +// For arrays, matching elements are spliced out (not set to nil). +// Returns the original document unmodified (not an error) if the path matches +// no nodes — callers that need to distinguish no-match from success should +// pre-check with Get. func (p *Path) Remove(doc any) (any, error) { if len(p.segments) < 2 { return nil, fmt.Errorf("jsonpath: cannot remove root") } - // Get the parent nodes and the final key + lastSeg := p.segments[len(p.segments)-1] parentPath := &Path{ raw: p.raw, segments: p.segments[:len(p.segments)-1], } - parents := parentPath.Get(doc) - if len(parents) == 0 { - // No matches - nothing to remove - return doc, nil + // When the parent is the root document, apply removal directly. + if len(parentPath.segments) <= 1 { + return removeFromParent(doc, lastSeg), nil } - lastSeg := p.segments[len(p.segments)-1] + // For deeper paths, go one level further to the grandparent. This lets us + // update the grandparent's reference to the parent slice when splicing, + // since a new slice header cannot be reflected through a plain any parameter. + grandParentPath := &Path{ + raw: p.raw, + segments: p.segments[:len(p.segments)-2], + } + parentSeg := p.segments[len(p.segments)-2] - for _, parent := range parents { - removeFromParent(parent, lastSeg) + grandParents := grandParentPath.Get(doc) + if len(grandParents) == 0 { + return doc, nil + } + + for _, gp := range grandParents { + modifyInParent(gp, parentSeg, func(v any) any { + return removeFromParent(v, lastSeg) + }) } return doc, nil @@ -97,11 +113,19 @@ func (p *Path) Remove(doc any) (any, error) { // Modify applies a transformation function to all matching nodes. // // The function receives each matched value and should return the new value. -// The document is modified in place. +// The document is modified in place. For the root path ("$"), the document +// must be a map[string]any; fn is expected to mutate it in place (e.g. +// mergeDeep). Root replacement via fn's return value is not supported since +// the caller's variable cannot be reassigned. func (p *Path) Modify(doc any, fn func(any) any) error { if len(p.segments) < 2 { - // Modifying root means replacing entire doc - not supported in-place - return fmt.Errorf("jsonpath: cannot modify root in place") + m, ok := doc.(map[string]any) + if !ok || m == nil { + return fmt.Errorf("jsonpath: root path Modify requires a non-nil map document; got %T", doc) + } + // fn is expected to mutate the map in place; return value is ignored. + fn(m) + return nil } // Get the parent nodes and the final segment @@ -312,8 +336,14 @@ func setInParent(parent any, seg Segment, value any) error { } } -// removeFromParent removes nodes from the parent at the location specified by the segment. -func removeFromParent(parent any, seg Segment) { +// removeFromParent removes nodes from the parent at the location specified by the +// segment and returns the (possibly new) parent value. Callers must use the +// return value when the parent is a slice, since splicing produces a new header. +func removeFromParent(parent any, seg Segment) any { + return removeFromParentAt(parent, seg, 0) +} + +func removeFromParentAt(parent any, seg Segment, depth int) any { switch s := seg.(type) { case ChildSegment: if m, ok := parent.(map[string]any); ok { @@ -321,15 +351,13 @@ func removeFromParent(parent any, seg Segment) { } case IndexSegment: - // Note: Removing from arrays by index is tricky as it shifts other elements. - // For simplicity, we set to nil rather than removing. if arr, ok := parent.([]any); ok { idx := s.Index if idx < 0 { idx = len(arr) + idx } if idx >= 0 && idx < len(arr) { - arr[idx] = nil + return append(arr[:idx:idx], arr[idx+1:]...) } } @@ -340,10 +368,7 @@ func removeFromParent(parent any, seg Segment) { delete(v, key) } case []any: - // Clear array elements - for i := range v { - v[i] = nil - } + return v[:0] } case FilterSegment: @@ -355,25 +380,46 @@ func removeFromParent(parent any, seg Segment) { } } case []any: - // For arrays, we need to collect indices to remove, then remove in reverse order - var toRemove []int - for i, elem := range v { - if evalFilter(elem, s.Expr) { - toRemove = append(toRemove, i) + result := v[:0] + for _, elem := range v { + if !evalFilter(elem, s.Expr) { + result = append(result, elem) } } - // Remove in reverse order to maintain correct indices - for i := len(toRemove) - 1; i >= 0; i-- { - idx := toRemove[i] - // Set to nil marker (actual removal would require modifying the slice) - v[idx] = nil + return result + } + + case RecursiveSegment: + if s.Child != nil { + if depth > maxRecursionDepth { + jsonpathLogger.Warn("jsonpath recursive remove truncated at depth limit", + "depth", depth, "maxDepth", maxRecursionDepth) + return parent + } + // Remove child at this level, then recurse into all descendants. + parent = removeFromParentAt(parent, s.Child, depth) + switch v := parent.(type) { + case map[string]any: + for key, val := range v { + v[key] = removeFromParentAt(val, seg, depth+1) + } + case []any: + for i, elem := range v { + v[i] = removeFromParentAt(elem, seg, depth+1) + } } } } + + return parent } // modifyInParent applies a transformation function to matching nodes in the parent. func modifyInParent(parent any, seg Segment, fn func(any) any) { + modifyInParentAt(parent, seg, fn, 0) +} + +func modifyInParentAt(parent any, seg Segment, fn func(any) any, depth int) { switch s := seg.(type) { case ChildSegment: if m, ok := parent.(map[string]any); ok { @@ -420,5 +466,26 @@ func modifyInParent(parent any, seg Segment, fn func(any) any) { } } } + + case RecursiveSegment: + if s.Child != nil { + if depth > maxRecursionDepth { + jsonpathLogger.Warn("jsonpath recursive modify truncated at depth limit", + "depth", depth, "maxDepth", maxRecursionDepth) + return + } + // Apply transform at this level, then recurse into all descendants. + modifyInParentAt(parent, s.Child, fn, depth) + switch v := parent.(type) { + case map[string]any: + for _, val := range v { + modifyInParentAt(val, seg, fn, depth+1) + } + case []any: + for _, elem := range v { + modifyInParentAt(elem, seg, fn, depth+1) + } + } + } } } diff --git a/internal/jsonpath/jsonpath_test.go b/internal/jsonpath/jsonpath_test.go index 060e463f..f667b753 100644 --- a/internal/jsonpath/jsonpath_test.go +++ b/internal/jsonpath/jsonpath_test.go @@ -980,6 +980,150 @@ func TestRecursiveDescentDepthLimit_WarningEmitted(t *testing.T) { assert.Contains(t, buf.String(), "truncated at depth limit") } +// TestRemoveArrayFilter_Splices verifies that filter removal on arrays splices +// elements out rather than setting them to nil (issue #352). +func TestRemoveArrayFilter_Splices(t *testing.T) { + doc := map[string]any{ + "parameters": []any{ + map[string]any{"name": "apikey", "in": "query"}, + map[string]any{"name": "limit", "in": "query"}, + }, + } + + p, err := Parse("$.parameters[?@.name=='apikey']") + require.NoError(t, err) + _, err = p.Remove(doc) + require.NoError(t, err) + + params := doc["parameters"].([]any) + require.Len(t, params, 1, "splice should remove element, not nil-pad") + assert.Equal(t, "limit", params[0].(map[string]any)["name"]) +} + +// TestRemoveRecursiveDescent verifies that $..field removal deletes the field +// from all descendants (issue #351). +func TestRemoveRecursiveDescent(t *testing.T) { + doc := map[string]any{ + "x-struct": "root", + "info": map[string]any{"title": "t", "x-struct": "info"}, + "nested": map[string]any{ + "deep": map[string]any{"x-struct": "deep"}, + }, + } + + p, err := Parse("$..x-struct") + require.NoError(t, err) + _, err = p.Remove(doc) + require.NoError(t, err) + + assert.NotContains(t, doc, "x-struct", "root-level x-struct should be removed") + assert.NotContains(t, doc["info"], "x-struct", "info-level x-struct should be removed") + deep := doc["nested"].(map[string]any)["deep"].(map[string]any) + assert.NotContains(t, deep, "x-struct", "nested x-struct should be removed") +} + +// TestModifyRoot verifies that Modify on root path "$" merges into the document +// in place (issue #350). +func TestModifyRoot(t *testing.T) { + doc := map[string]any{ + "openapi": "3.0.0", + "info": map[string]any{"title": "t"}, + } + + p, err := Parse("$") + require.NoError(t, err) + err = p.Modify(doc, func(v any) any { + if m, ok := v.(map[string]any); ok { + m["host"] = "api.example.com" + } + return v + }) + require.NoError(t, err) + assert.Equal(t, "api.example.com", doc["host"], "root Modify should merge into document") +} + +// TestModifyRoot_InvalidDoc verifies that Modify on root path "$" returns an +// error for non-map and typed-nil map documents. +func TestModifyRoot_InvalidDoc(t *testing.T) { + p, err := Parse("$") + require.NoError(t, err) + + t.Run("non-map value returns error", func(t *testing.T) { + err := p.Modify("not a map", func(v any) any { return v }) + require.Error(t, err) + }) + + t.Run("typed-nil map returns error", func(t *testing.T) { + var doc map[string]any + err := p.Modify(doc, func(v any) any { return v }) + require.Error(t, err) + }) +} + +// TestRemoveArrayIndex_Splices verifies that Remove on an index segment produces +// a correctly spliced slice (no nil gaps) — regression for issue #351. +func TestRemoveArrayIndex_Splices(t *testing.T) { + doc := map[string]any{ + "servers": []any{"a", "b", "c"}, + } + + p, err := Parse("$.servers[1]") + require.NoError(t, err) + result, err := p.Remove(doc) + require.NoError(t, err) + + servers := result.(map[string]any)["servers"].([]any) + require.Len(t, servers, 2, "splice should remove element, not nil-pad") + assert.Equal(t, "a", servers[0]) + assert.Equal(t, "c", servers[1]) +} + +// TestRemoveRecursive_ThroughArrays verifies that $..field removal descends +// through arrays to remove the field from every object inside them. +func TestRemoveRecursive_ThroughArrays(t *testing.T) { + doc := map[string]any{ + "items": []any{ + map[string]any{"name": "a", "x-internal": true}, + map[string]any{"name": "b", "x-internal": false}, + }, + } + + p, err := Parse("$..x-internal") + require.NoError(t, err) + _, err = p.Remove(doc) + require.NoError(t, err) + + items := doc["items"].([]any) + require.Len(t, items, 2) + assert.NotContains(t, items[0], "x-internal", "x-internal should be removed from first item") + assert.NotContains(t, items[1], "x-internal", "x-internal should be removed from second item") + assert.Equal(t, "a", items[0].(map[string]any)["name"], "name should be preserved") + assert.Equal(t, "b", items[1].(map[string]any)["name"], "name should be preserved") +} + +// TestModifyRecursiveDescent verifies that Modify with $..field applies the +// transform to every matching field at all depths. +func TestModifyRecursiveDescent(t *testing.T) { + doc := map[string]any{ + "description": "root", + "info": map[string]any{"description": "info level"}, + "paths": map[string]any{ + "/users": map[string]any{"description": "path level"}, + }, + } + + p, err := Parse("$..description") + require.NoError(t, err) + err = p.Modify(doc, func(v any) any { + return "updated" + }) + require.NoError(t, err) + + assert.Equal(t, "updated", doc["description"], "root description should be updated") + assert.Equal(t, "updated", doc["info"].(map[string]any)["description"], "info description should be updated") + assert.Equal(t, "updated", doc["paths"].(map[string]any)["/users"].(map[string]any)["description"], "path description should be updated") +} + // TestRecursiveDescentDepthLimit_nilChild verifies the depth cap when // recursiveDescend delegates to collectAllDescendants (nil child segment). func TestRecursiveDescentDepthLimit_nilChild(t *testing.T) { diff --git a/overlay/overlay_test.go b/overlay/overlay_test.go index dbaa5e22..dcc15593 100644 --- a/overlay/overlay_test.go +++ b/overlay/overlay_test.go @@ -1,6 +1,7 @@ package overlay import ( + "encoding/json" "errors" "testing" @@ -1127,6 +1128,103 @@ func TestDryRun(t *testing.T) { }) } +// TestApplyRootTarget verifies that target "$" correctly merges into the root +// document (issue #350). +func TestApplyRootTarget(t *testing.T) { + spec := mustParseSpec(t, `{"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{}}`) + o := &Overlay{ + Version: "1.0.0", + Info: Info{Title: "test", Version: "1.0.0"}, + Actions: []Action{ + {Target: "$", Update: map[string]any{"x-custom": "added"}}, + }, + } + + a := NewApplier() + result, err := a.ApplyParsed(spec, o) + require.NoError(t, err) + require.Equal(t, 1, result.ActionsApplied) + + doc := result.Document.(map[string]any) + assert.Equal(t, "added", doc["x-custom"], "root update should merge x-custom into document") + + t.Run("root update overwrites existing key", func(t *testing.T) { + spec2 := mustParseSpec(t, `{"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{}}`) + o2 := &Overlay{ + Version: "1.0.0", + Info: Info{Title: "test", Version: "1.0.0"}, + Actions: []Action{ + {Target: "$", Update: map[string]any{"openapi": "3.1.0"}}, + }, + } + result2, err2 := a.ApplyParsed(spec2, o2) + require.NoError(t, err2) + doc2 := result2.Document.(map[string]any) + assert.Equal(t, "3.1.0", doc2["openapi"], "root update should overwrite existing key") + }) +} + +// TestApplyRecursiveRemove verifies that $..field remove deletes the field from +// all descendants (issue #351). +func TestApplyRecursiveRemove(t *testing.T) { + spec := mustParseSpec(t, `{"openapi":"3.0.0","info":{"title":"t","version":"1","x-struct":"root"},"paths":{"/a":{"get":{"parameters":[{"name":"p","in":"query","schema":{"type":"string","x-struct":"deep"}}],"responses":{"200":{"description":"ok"}}}}}}`) + o := &Overlay{ + Version: "1.0.0", + Info: Info{Title: "test", Version: "1.0.0"}, + Actions: []Action{ + {Target: "$..x-struct", Remove: true}, + }, + } + + a := NewApplier() + result, err := a.ApplyParsed(spec, o) + require.NoError(t, err) + require.Equal(t, 1, result.ActionsApplied) + + doc := result.Document.(map[string]any) + info := doc["info"].(map[string]any) + assert.NotContains(t, info, "x-struct", "x-struct should be removed from info") + + paths := doc["paths"].(map[string]any) + getOp := paths["/a"].(map[string]any)["get"].(map[string]any) + params := getOp["parameters"].([]any) + schema := params[0].(map[string]any)["schema"].(map[string]any) + assert.NotContains(t, schema, "x-struct", "x-struct should be removed from nested schema") +} + +// TestApplyArrayFilterRemove verifies that filter removal on arrays splices +// elements out rather than nil-padding (issue #352). +func TestApplyArrayFilterRemove(t *testing.T) { + spec := mustParseSpec(t, `{"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/a":{"get":{"parameters":[{"name":"apikey","in":"query","schema":{"type":"string"}},{"name":"limit","in":"query","schema":{"type":"integer"}}],"responses":{"200":{"description":"ok"}}}}}}`) + o := &Overlay{ + Version: "1.0.0", + Info: Info{Title: "test", Version: "1.0.0"}, + Actions: []Action{ + {Target: "$.paths.*.get.parameters[?@.name=='apikey']", Remove: true}, + }, + } + + a := NewApplier() + result, err := a.ApplyParsed(spec, o) + require.NoError(t, err) + require.Equal(t, 1, result.ActionsApplied) + + doc := result.Document.(map[string]any) + paths := doc["paths"].(map[string]any) + getOp := paths["/a"].(map[string]any)["get"].(map[string]any) + params := getOp["parameters"].([]any) + require.Len(t, params, 1, "filter remove should splice element, not nil-pad") + assert.Equal(t, "limit", params[0].(map[string]any)["name"]) +} + +// mustParseSpec is a test helper that parses a JSON spec string into a ParseResult. +func mustParseSpec(t *testing.T, specJSON string) *parser.ParseResult { + t.Helper() + var doc map[string]any + require.NoError(t, json.Unmarshal([]byte(specJSON), &doc)) + return &parser.ParseResult{Document: doc} +} + // TestDryRunWithOptions tests the functional options API for dry-run. func TestDryRunWithOptions(t *testing.T) { result, err := DryRunWithOptions( diff --git a/parser/internal/jsonhelpers/helpers.go b/parser/internal/jsonhelpers/helpers.go index f67073b2..15d360b2 100644 --- a/parser/internal/jsonhelpers/helpers.go +++ b/parser/internal/jsonhelpers/helpers.go @@ -216,6 +216,16 @@ func SetIfSliceNotEmpty[T any](m map[string]any, key string, value []T) { } } +// SetIfSliceNotNil sets a slice field in the map only if the slice is non-nil. +// Unlike SetIfSliceNotEmpty, this preserves empty (non-nil) slices, serializing +// them as []. This is important for fields like operation-level security where +// an empty slice has semantic meaning (disable inherited security requirements). +func SetIfSliceNotNil[T any](m map[string]any, key string, value []T) { + if value != nil { + m[key] = value + } +} + // SetIfMapNotEmpty sets a map field in the map only if the map has length > 0. // This is useful for MarshalJSON to avoid adding empty map fields. // Note: In Go, both nil maps and empty maps should be omitted from JSON output. diff --git a/parser/parser_test.go b/parser/parser_test.go index 42976728..89030067 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -158,13 +158,8 @@ paths: // Verify QUERY operation is present usersPath := doc.Paths["/users"] - if usersPath == nil { - t.Fatal("Expected /users path to be present") - } - - if usersPath.Query == nil { - t.Fatal("Expected QUERY operation to be present") - } + require.NotNil(t, usersPath, "Expected /users path to be present") + require.NotNil(t, usersPath.Query, "Expected QUERY operation to be present") if usersPath.Query.OperationID != "queryUsers" { t.Errorf("Expected operationId 'queryUsers', got %s", usersPath.Query.OperationID) diff --git a/parser/paths_json.go b/parser/paths_json.go index 81b2c229..ab92c28d 100644 --- a/parser/paths_json.go +++ b/parser/paths_json.go @@ -55,8 +55,10 @@ func (p *PathItem) UnmarshalJSON(data []byte) error { // into the top-level JSON object, as Go's encoding/json doesn't support // inline maps like yaml:",inline". func (o *Operation) MarshalJSON() ([]byte, error) { - // Fast path: no Extra fields, use standard marshaling - if len(o.Extra) == 0 { + // Fast path: no Extra fields and nil security, use standard marshaling. + // Security is excluded from the fast path because a non-nil empty slice + // must serialize as [] (disable inherited security), not be omitted. + if len(o.Extra) == 0 && o.Security == nil { type Alias Operation return marshalToJSON((*Alias)(o)) } @@ -74,7 +76,7 @@ func (o *Operation) MarshalJSON() ([]byte, error) { jsonhelpers.SetIfNotNil(m, "requestBody", o.RequestBody) jsonhelpers.SetIfMapNotEmpty(m, "callbacks", o.Callbacks) jsonhelpers.SetIfTrue(m, "deprecated", o.Deprecated) - jsonhelpers.SetIfSliceNotEmpty(m, "security", o.Security) + jsonhelpers.SetIfSliceNotNil(m, "security", o.Security) jsonhelpers.SetIfSliceNotEmpty(m, "servers", o.Servers) jsonhelpers.SetIfSliceNotEmpty(m, "consumes", o.Consumes) jsonhelpers.SetIfSliceNotEmpty(m, "produces", o.Produces) diff --git a/parser/paths_json_test.go b/parser/paths_json_test.go index 30885816..63f9993a 100644 --- a/parser/paths_json_test.go +++ b/parser/paths_json_test.go @@ -587,3 +587,66 @@ func TestEncodingUnmarshalJSON(t *testing.T) { }) } } + +// TestOperationEmptySecurityMarshal verifies that a non-nil empty Security slice +// marshals as [] rather than being omitted (issue #349). An empty security array +// on an operation explicitly disables any document-level security requirements. +func TestOperationEmptySecurityMarshal(t *testing.T) { + okResponses := &Responses{Codes: map[string]*Response{"200": {Description: "ok"}}} + + t.Run("nil security is omitted", func(t *testing.T) { + op := &Operation{ + Responses: okResponses, + Security: nil, + Extra: map[string]any{"x-test": "val"}, + } + data, err := json.Marshal(op) + require.NoError(t, err) + assert.NotContains(t, string(data), `"security"`, "nil security should be omitted") + }) + + t.Run("empty security slice marshals as []", func(t *testing.T) { + op := &Operation{ + Responses: okResponses, + Security: []SecurityRequirement{}, + Extra: map[string]any{"x-test": "val"}, + } + data, err := json.Marshal(op) + require.NoError(t, err) + assert.Contains(t, string(data), `"security":[]`, "empty non-nil security should marshal as []") + }) + + t.Run("empty security slice marshals as [] without extra fields", func(t *testing.T) { + op := &Operation{ + Responses: okResponses, + Security: []SecurityRequirement{}, + } + data, err := json.Marshal(op) + require.NoError(t, err) + assert.Contains(t, string(data), `"security":[]`, "empty non-nil security should marshal as [] even without x- fields") + }) + + t.Run("populated security slice marshals correctly", func(t *testing.T) { + op := &Operation{ + Responses: okResponses, + Security: []SecurityRequirement{ + {"api_key": []string{}}, + {"oauth2": []string{"read", "write"}}, + }, + } + data, err := json.Marshal(op) + require.NoError(t, err) + assert.Contains(t, string(data), `"security"`, "populated security should be present") + assert.Contains(t, string(data), `"api_key"`, "security requirement should contain api_key") + assert.Contains(t, string(data), `"oauth2"`, "security requirement should contain oauth2") + }) + + t.Run("empty security array round-trips through unmarshal", func(t *testing.T) { + input := `{"responses":{"200":{"description":"ok"}},"security":[]}` + var op Operation + err := json.Unmarshal([]byte(input), &op) + require.NoError(t, err) + require.NotNil(t, op.Security, "empty security array should unmarshal to non-nil slice") + assert.Empty(t, op.Security, "security slice should be empty after round-trip") + }) +} diff --git a/parser/schema_json.go b/parser/schema_json.go index 87d25a77..83238b5d 100644 --- a/parser/schema_json.go +++ b/parser/schema_json.go @@ -2,6 +2,7 @@ package parser import ( "encoding/json" + "fmt" "github.com/erraggy/oastools/parser/internal/jsonhelpers" ) @@ -93,9 +94,64 @@ func (s *Schema) UnmarshalJSON(data []byte) error { return err } s.Extra = jsonhelpers.ExtractExtensions(data) + // The Alias trick bypasses custom unmarshalers, causing encoding/json to decode + // any-typed fields (Items, AdditionalProperties, etc.) as map[string]any instead + // of *Schema. Promote them back so downstream type assertions work correctly. + var err error + if s.Items, err = promoteSchemaOrBool(s.Items); err != nil { + return err + } + if s.AdditionalProperties, err = promoteSchemaOrBool(s.AdditionalProperties); err != nil { + return err + } + if s.AdditionalItems, err = promoteSchemaOrBool(s.AdditionalItems); err != nil { + return err + } + if s.UnevaluatedItems, err = promoteSchemaOrBool(s.UnevaluatedItems); err != nil { + return err + } + if s.UnevaluatedProperties, err = promoteSchemaOrBool(s.UnevaluatedProperties); err != nil { + return err + } return nil } +// promoteSchemaOrBool converts a map[string]any value (produced by the standard +// JSON decoder for any-typed schema fields) into a *Schema. Bool values and +// already-typed *Schema values pass through unchanged. Returns an error if the +// map cannot be round-tripped through JSON into a *Schema, so callers get a +// clear parse error rather than a silent type-assertion panic downstream. +func promoteSchemaOrBool(v any) (any, error) { + switch val := v.(type) { + case nil, bool, *Schema: + return v, nil + case map[string]any: + data, err := json.Marshal(val) + if err != nil { + return nil, fmt.Errorf("parser: schema field promotion: %w", err) + } + s := &Schema{} + if err := json.Unmarshal(data, s); err != nil { + return nil, fmt.Errorf("parser: schema field promotion: %w", err) + } + return s, nil + case []any: + // OAS 2.0 tuple validation: items can be an array of schemas. + schemas := make([]*Schema, 0, len(val)) + for _, elem := range val { + promoted, err := promoteSchemaOrBool(elem) + if err != nil { + return nil, err + } + if s, ok := promoted.(*Schema); ok { + schemas = append(schemas, s) + } + } + return schemas, nil + } + return v, nil +} + // MarshalJSON implements custom JSON marshaling for Discriminator. // This is required to flatten Extra fields (specification extensions like x-*) // into the top-level JSON object, as Go's encoding/json doesn't support diff --git a/parser/schema_json_test.go b/parser/schema_json_test.go index 23a8a058..c9337150 100644 --- a/parser/schema_json_test.go +++ b/parser/schema_json_test.go @@ -284,6 +284,112 @@ func TestSchemaJSONRoundTrip(t *testing.T) { assert.Equal(t, original.Extra, decoded.Extra) }) + // Regression test for issue #353/#355: the JSON fast-path (type Alias trick) + // must promote any-typed fields (Items, AdditionalProperties, etc.) from + // map[string]any back to *Schema so downstream type assertions work. + t.Run("Items decoded as *Schema not map[string]any", func(t *testing.T) { + data := []byte(`{"type":"array","items":{"$ref":"#/definitions/Foo"}}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + _, ok := s.Items.(*Schema) + assert.True(t, ok, "Items should be *Schema after JSON unmarshal, not map[string]any") + }) + + t.Run("Items bool true preserved (OAS 3.1+)", func(t *testing.T) { + data := []byte(`{"type":"array","items":true}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + b, ok := s.Items.(bool) + assert.True(t, ok, "items true should remain bool") + assert.True(t, b) + }) + + t.Run("Items array decoded as []*Schema (OAS 2.0 tuple)", func(t *testing.T) { + data := []byte(`{"type":"array","items":[{"type":"string"},{"type":"integer"}]}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + arr, ok := s.Items.([]*Schema) + assert.True(t, ok, "items array should be []*Schema after JSON unmarshal") + assert.Len(t, arr, 2) + }) + + t.Run("additionalProperties object decoded as *Schema", func(t *testing.T) { + data := []byte(`{"type":"object","additionalProperties":{"type":"string"}}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + _, ok := s.AdditionalProperties.(*Schema) + assert.True(t, ok, "additionalProperties should be *Schema after JSON unmarshal") + }) + + t.Run("additionalProperties bool false preserved", func(t *testing.T) { + data := []byte(`{"type":"object","additionalProperties":false}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + b, ok := s.AdditionalProperties.(bool) + assert.True(t, ok, "additionalProperties false should remain bool") + assert.False(t, b) + }) + + t.Run("additionalProperties bool true preserved", func(t *testing.T) { + data := []byte(`{"type":"object","additionalProperties":true}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + b, ok := s.AdditionalProperties.(bool) + assert.True(t, ok, "additionalProperties true should remain bool") + assert.True(t, b) + }) + + t.Run("additionalItems decoded as *Schema", func(t *testing.T) { + data := []byte(`{"type":"array","additionalItems":{"type":"string"}}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + _, ok := s.AdditionalItems.(*Schema) + assert.True(t, ok, "additionalItems should be *Schema after JSON unmarshal") + }) + + t.Run("additionalItems bool preserved", func(t *testing.T) { + data := []byte(`{"type":"array","additionalItems":false}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + b, ok := s.AdditionalItems.(bool) + assert.True(t, ok, "additionalItems false should remain bool") + assert.False(t, b) + }) + + t.Run("unevaluatedItems decoded as *Schema", func(t *testing.T) { + data := []byte(`{"type":"array","unevaluatedItems":{"type":"integer"}}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + _, ok := s.UnevaluatedItems.(*Schema) + assert.True(t, ok, "unevaluatedItems should be *Schema after JSON unmarshal") + }) + + t.Run("unevaluatedItems bool preserved", func(t *testing.T) { + data := []byte(`{"type":"array","unevaluatedItems":false}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + b, ok := s.UnevaluatedItems.(bool) + assert.True(t, ok, "unevaluatedItems false should remain bool") + assert.False(t, b) + }) + + t.Run("unevaluatedProperties decoded as *Schema", func(t *testing.T) { + data := []byte(`{"type":"object","unevaluatedProperties":{"type":"boolean"}}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + _, ok := s.UnevaluatedProperties.(*Schema) + assert.True(t, ok, "unevaluatedProperties should be *Schema after JSON unmarshal") + }) + + t.Run("unevaluatedProperties bool preserved", func(t *testing.T) { + data := []byte(`{"type":"object","unevaluatedProperties":false}`) + var s Schema + require.NoError(t, json.Unmarshal(data, &s)) + b, ok := s.UnevaluatedProperties.(bool) + assert.True(t, ok, "unevaluatedProperties false should remain bool") + assert.False(t, b) + }) + t.Run("XML round-trip", func(t *testing.T) { original := &XML{ Name: "pet",