From e97fa0648c4ab8fbfe4d8a826740a636d9b4be14 Mon Sep 17 00:00:00 2001 From: quobix Date: Fri, 19 Dec 2025 19:11:43 -0500 Subject: [PATCH 1/4] Added singleflight and deterministic ordering. smashing out non-deterministic issues once and for all, ordering maps and using singleflight and RWMutex. --- go.mod | 1 + go.sum | 2 + index/determinism_benchmark_test.go | 131 ++++++++++++++ index/extract_refs.go | 254 +++++++++++++++++++--------- index/index_model.go | 2 +- index/resolver.go | 34 +++- index/spec_index.go | 13 +- 7 files changed, 348 insertions(+), 89 deletions(-) create mode 100644 index/determinism_benchmark_test.go diff --git a/go.mod b/go.mod index 8c5e54f9..6ba8cd91 100644 --- a/go.mod +++ b/go.mod @@ -17,5 +17,6 @@ require ( github.com/kr/text v0.2.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/rogpeppe/go-internal v1.9.0 // indirect + golang.org/x/sync v0.19.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index cd3851a6..b7e9e111 100644 --- a/go.sum +++ b/go.sum @@ -23,6 +23,8 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= go.yaml.in/yaml/v4 v4.0.0-rc.3 h1:3h1fjsh1CTAPjW7q/EMe+C8shx5d8ctzZTrLcs/j8Go= go.yaml.in/yaml/v4 v4.0.0-rc.3/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0= +golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= +golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogRM/Nc3DYOhEAlW+xobZo= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/index/determinism_benchmark_test.go b/index/determinism_benchmark_test.go new file mode 100644 index 00000000..07d9cae5 --- /dev/null +++ b/index/determinism_benchmark_test.go @@ -0,0 +1,131 @@ +// Copyright 2024 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package index + +import ( + "os" + "testing" + + "go.yaml.in/yaml/v4" +) + +// TestDeterminism_ConsistentResults verifies that reference extraction produces +// identical results across multiple runs. This is the regression test for issue #441. +// +// The test runs the full indexing process multiple times on a large spec and verifies: +// 1. The same number of refs are found each time +// 2. The refs are in the exact same order each time +// 3. The same number of errors are reported each time +func TestDeterminism_ConsistentResults(t *testing.T) { + specPath := "../test_specs/stripe.yaml" + specBytes, err := os.ReadFile(specPath) + if err != nil { + t.Skipf("Could not load spec: %v", err) + } + + const runs = 10 + var baselineOrder []string + var baselineErrorCount int + + for i := 0; i < runs; i++ { + var rootNode yaml.Node + if err := yaml.Unmarshal(specBytes, &rootNode); err != nil { + t.Fatal(err) + } + + config := &SpecIndexConfig{ + AllowRemoteLookup: false, + AllowFileLookup: false, + } + + idx := NewSpecIndexWithConfig(&rootNode, config) + idx.BuildIndex() + + // Get sequenced refs - this is what must be deterministic + sequenced := idx.GetMappedReferencesSequenced() + order := make([]string, len(sequenced)) + for j, ref := range sequenced { + order[j] = ref.FullDefinition + } + + errorCount := len(idx.GetReferenceIndexErrors()) + + if i == 0 { + baselineOrder = order + baselineErrorCount = errorCount + t.Logf("Baseline: %d refs, %d errors", len(order), errorCount) + } else { + // Verify ref count is identical + if len(order) != len(baselineOrder) { + t.Fatalf("Run %d: different ref count: got %d, want %d", i, len(order), len(baselineOrder)) + } + // Verify error count is identical + if errorCount != baselineErrorCount { + t.Errorf("Run %d: different error count: got %d, want %d", i, errorCount, baselineErrorCount) + } + // Verify order is identical + for j := range order { + if order[j] != baselineOrder[j] { + t.Fatalf("Run %d: different order at index %d: got %s, want %s", + i, j, order[j], baselineOrder[j]) + } + } + } + } + t.Logf("All %d runs produced identical results", runs) +} + +// BenchmarkIndexing_Determinism benchmarks the indexing process while also +// verifying determinism. This ensures performance optimizations don't break +// the deterministic ordering guarantee. +func BenchmarkIndexing_Determinism(b *testing.B) { + specPath := "../test_specs/stripe.yaml" + specBytes, err := os.ReadFile(specPath) + if err != nil { + b.Skipf("Could not load spec: %v", err) + } + + var baselineOrder []string + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + var rootNode yaml.Node + if err := yaml.Unmarshal(specBytes, &rootNode); err != nil { + b.Fatal(err) + } + + config := &SpecIndexConfig{ + AllowRemoteLookup: false, + AllowFileLookup: false, + } + + idx := NewSpecIndexWithConfig(&rootNode, config) + idx.BuildIndex() + + // Get sequenced refs and verify determinism + sequenced := idx.GetMappedReferencesSequenced() + order := make([]string, len(sequenced)) + for j, ref := range sequenced { + order[j] = ref.FullDefinition + } + + if i == 0 { + baselineOrder = order + if len(order) == 0 { + b.Fatal("No references found") + } + } else { + // Verify order is identical on every iteration + if len(order) != len(baselineOrder) { + b.Fatalf("Iteration %d: different ref count: got %d, want %d", i, len(order), len(baselineOrder)) + } + for j := range order { + if order[j] != baselineOrder[j] { + b.Fatalf("Iteration %d: different order at index %d", i, j) + } + } + } + } +} diff --git a/index/extract_refs.go b/index/extract_refs.go index 11ed2495..bb06d7fd 100644 --- a/index/extract_refs.go +++ b/index/extract_refs.go @@ -12,14 +12,26 @@ import ( "path/filepath" "runtime" "slices" + "sort" "strconv" "strings" "sync" "github.com/pb33f/libopenapi/utils" "go.yaml.in/yaml/v4" + "golang.org/x/sync/singleflight" ) +// preserveLegacyRefOrder allows opt-out of deterministic ordering if issues arise. +// Set LIBOPENAPI_LEGACY_REF_ORDER=true to use the old non-deterministic ordering. +var preserveLegacyRefOrder = os.Getenv("LIBOPENAPI_LEGACY_REF_ORDER") == "true" + +// indexedRef pairs a resolved reference with its original input position for deterministic ordering. +type indexedRef struct { + ref *Reference + pos int +} + // ExtractRefs will return a deduplicated slice of references for every unique ref found in the document. // The total number of refs, will generally be much higher, you can extract those from GetRawReferenceCount() func (index *SpecIndex) ExtractRefs(ctx context.Context, node, parent *yaml.Node, seenPath []string, level int, poly bool, pName string) []*Reference { @@ -704,120 +716,204 @@ func (index *SpecIndex) ExtractRefs(ctx context.Context, node, parent *yaml.Node // ExtractComponentsFromRefs returns located components from references. The returned nodes from here // can be used for resolving as they contain the actual object properties. +// +// This function uses singleflight to deduplicate concurrent lookups for the same reference, +// channel-based collection to avoid mutex contention during resolution, and sorts results +// by input position for deterministic ordering. func (index *SpecIndex) ExtractComponentsFromRefs(ctx context.Context, refs []*Reference) []*Reference { - found := make([]*Reference, 0, len(refs)) // pre-allocate capacity to avoid reallocations - var foundMu sync.Mutex // protects found slice in async mode - - var refsToCheck []*Reference - refsToCheck = append(refsToCheck, refs...) + if len(refs) == 0 { + return nil + } + refsToCheck := refs mappedRefsInSequence := make([]*ReferenceMapped, len(refsToCheck)) - locate := func(ref *Reference, refIndex int) { - index.refLock.Lock() - if index.allMappedRefs[ref.FullDefinition] != nil { - rm := &ReferenceMapped{ - OriginalReference: ref, - Reference: index.allMappedRefs[ref.FullDefinition], - Definition: index.allMappedRefs[ref.FullDefinition].Definition, - FullDefinition: index.allMappedRefs[ref.FullDefinition].FullDefinition, + // Sequential mode: process refs one at a time (used for bundling) + if index.config.ExtractRefsSequentially { + found := make([]*Reference, 0, len(refsToCheck)) + for i, ref := range refsToCheck { + located := index.locateRef(ctx, ref) + if located != nil { + index.refLock.Lock() + if index.allMappedRefs[ref.FullDefinition] == nil { + index.allMappedRefs[located.FullDefinition] = located + found = append(found, located) + } + mappedRefsInSequence[i] = &ReferenceMapped{ + OriginalReference: ref, + Reference: located, + Definition: located.Definition, + FullDefinition: located.FullDefinition, + } + index.refLock.Unlock() + } else { + // Record error for definitive failure + _, path := utils.ConvertComponentIdIntoFriendlyPathSearch(ref.Definition) + index.errorLock.Lock() + index.refErrors = append(index.refErrors, &IndexingError{ + Err: fmt.Errorf("component `%s` does not exist in the specification", ref.Definition), + Node: ref.Node, + Path: path, + KeyNode: ref.KeyNode, + }) + index.errorLock.Unlock() } - mappedRefsInSequence[refIndex] = rm - index.refLock.Unlock() - return - } - index.refLock.Unlock() - - // if it's an external reference, we need to lock during FindComponent - uri := strings.Split(ref.FullDefinition, "#/") - unsafeAsync := len(uri) == 2 && len(uri[0]) > 0 - if unsafeAsync { - index.refLock.Lock() } - located := index.FindComponent(ctx, ref.FullDefinition) - if unsafeAsync { - index.refLock.Unlock() + // Collect sequenced results + for _, rm := range mappedRefsInSequence { + if rm != nil { + index.allMappedRefsSequenced = append(index.allMappedRefsSequenced, rm) + } } + return found + } - if located != nil { - // key node is always going to be nil when mapping, yamlpath API returns - // subnodes only, so we need to rollback in the nodemap a line (if we can) to extract - // the keynode. - if located.Node != nil { - index.nodeMapLock.RLock() - if located.Node.Line > 1 && len(index.nodeMap[located.Node.Line-1]) > 0 { - for _, v := range index.nodeMap[located.Node.Line-1] { - located.KeyNode = v - break - } + // Async mode: use singleflight for deduplication and channel-based collection + var wg sync.WaitGroup + var sfGroup singleflight.Group // Local to this call - no cross-index coupling + + // Channel-based collection - no mutex needed during resolution + resultsChan := make(chan indexedRef, len(refsToCheck)) + + // Concurrency control + maxConcurrency := runtime.GOMAXPROCS(0) + if maxConcurrency < 4 { + maxConcurrency = 4 + } + sem := make(chan struct{}, maxConcurrency) + + for i, ref := range refsToCheck { + i, ref := i, ref // capture loop variables + wg.Add(1) + + go func() { + sem <- struct{}{} + defer func() { <-sem }() + defer wg.Done() + + // Singleflight deduplication - one lookup per FullDefinition + result, _, _ := sfGroup.Do(ref.FullDefinition, func() (interface{}, error) { + // Fast path: already mapped + index.refLock.RLock() + if existing := index.allMappedRefs[ref.FullDefinition]; existing != nil { + index.refLock.RUnlock() + return existing, nil } - index.nodeMapLock.RUnlock() + index.refLock.RUnlock() + + // Do the actual lookup (only one goroutine per FullDefinition) + return index.locateRef(ctx, ref), nil + }) + + if result != nil { + resultsChan <- indexedRef{ref: result.(*Reference), pos: i} + } else { + resultsChan <- indexedRef{ref: nil, pos: i} // Track failures for reconciliation } + }() + } - // have we already mapped this? + // Close channel after all goroutines complete + go func() { + wg.Wait() + close(resultsChan) + }() + + // Collect results - single consumer, no lock needed + collected := make([]indexedRef, 0, len(refsToCheck)) + for r := range resultsChan { + collected = append(collected, r) + } + + // Sort by input position for deterministic ordering + if !preserveLegacyRefOrder { + sort.Slice(collected, func(i, j int) bool { + return collected[i].pos < collected[j].pos + }) + } + + // RECONCILIATION PHASE: Build final results with minimal locking + found := make([]*Reference, 0, len(collected)) + + for _, c := range collected { + ref := refsToCheck[c.pos] + located := c.ref + + // Reconcile nil results - check if another goroutine succeeded + if located == nil { + index.refLock.RLock() + located = index.allMappedRefs[ref.FullDefinition] + index.refLock.RUnlock() + } + + if located != nil { + // Add to allMappedRefs if not present index.refLock.Lock() - if index.allMappedRefs[ref.FullDefinition] == nil { - foundMu.Lock() - found = append(found, located) - foundMu.Unlock() + if index.allMappedRefs[located.FullDefinition] == nil { index.allMappedRefs[located.FullDefinition] = located + found = append(found, located) } - rm := &ReferenceMapped{ + mappedRefsInSequence[c.pos] = &ReferenceMapped{ OriginalReference: ref, Reference: located, Definition: located.Definition, FullDefinition: located.FullDefinition, } - mappedRefsInSequence[refIndex] = rm index.refLock.Unlock() } else { + // Definitive failure - record error _, path := utils.ConvertComponentIdIntoFriendlyPathSearch(ref.Definition) - indexError := &IndexingError{ + index.errorLock.Lock() + index.refErrors = append(index.refErrors, &IndexingError{ Err: fmt.Errorf("component `%s` does not exist in the specification", ref.Definition), Node: ref.Node, Path: path, KeyNode: ref.KeyNode, - } - - index.errorLock.Lock() - index.refErrors = append(index.refErrors, indexError) + }) index.errorLock.Unlock() } } - if index.config.ExtractRefsSequentially { - // sequential mode: process refs one at a time - for r := range refsToCheck { - locate(refsToCheck[r], r) - } - } else { - // async mode: use WaitGroup for proper synchronization - maxConcurrency := runtime.GOMAXPROCS(0) - if maxConcurrency < 4 { - maxConcurrency = 4 - } - sem := make(chan struct{}, maxConcurrency) - var wg sync.WaitGroup - wg.Add(len(refsToCheck)) - - for r := range refsToCheck { - go func(ref *Reference, idx int) { - sem <- struct{}{} - defer func() { <-sem }() - defer wg.Done() - locate(ref, idx) - }(refsToCheck[r], r) + // Collect sequenced results in input order + for _, rm := range mappedRefsInSequence { + if rm != nil { + index.allMappedRefsSequenced = append(index.allMappedRefsSequenced, rm) } + } - wg.Wait() + return found +} + +// locateRef finds a component for a reference, including KeyNode extraction. +// This is a helper used by ExtractComponentsFromRefs to isolate the lookup logic. +func (index *SpecIndex) locateRef(ctx context.Context, ref *Reference) *Reference { + // External references need locking during FindComponent + uri := strings.Split(ref.FullDefinition, "#/") + unsafeAsync := len(uri) == 2 && len(uri[0]) > 0 + if unsafeAsync { + index.refLock.Lock() + } + located := index.FindComponent(ctx, ref.FullDefinition) + if unsafeAsync { + index.refLock.Unlock() } - // collect results - for m := range mappedRefsInSequence { - if mappedRefsInSequence[m] != nil { - index.allMappedRefsSequenced = append(index.allMappedRefsSequenced, mappedRefsInSequence[m]) + if located == nil { + return nil + } + + // Extract KeyNode - yamlpath API returns subnodes only, so we need to + // rollback in the nodemap a line (if we can) to extract the keynode. + if located.Node != nil { + index.nodeMapLock.RLock() + if located.Node.Line > 1 && len(index.nodeMap[located.Node.Line-1]) > 0 { + for _, v := range index.nodeMap[located.Node.Line-1] { + located.KeyNode = v + break + } } + index.nodeMapLock.RUnlock() } - return found + return located } diff --git a/index/index_model.go b/index/index_model.go index d609fea9..db935233 100644 --- a/index/index_model.go +++ b/index/index_model.go @@ -383,7 +383,7 @@ type SpecIndex struct { enumCount int descriptionCount int summaryCount int - refLock sync.Mutex + refLock sync.RWMutex nodeMapLock sync.RWMutex componentLock sync.RWMutex errorLock sync.RWMutex diff --git a/index/resolver.go b/index/resolver.go index 944b63ff..89d14c58 100644 --- a/index/resolver.go +++ b/index/resolver.go @@ -10,6 +10,7 @@ import ( "path" "path/filepath" "slices" + "sort" "strings" "github.com/pb33f/libopenapi/utils" @@ -250,8 +251,17 @@ func visitIndexWithoutDamagingIt(res *Resolver, idx *SpecIndex) { res.journeysTaken++ res.VisitReference(ref.Reference, seenReferences, journey, false) } + + // Sort schema keys for deterministic iteration order schemas := idx.GetAllComponentSchemas() - for s, schemaRef := range schemas { + schemaKeys := make([]string, 0, len(schemas)) + for k := range schemas { + schemaKeys = append(schemaKeys, k) + } + sort.Strings(schemaKeys) + + for _, s := range schemaKeys { + schemaRef := schemas[s] if mappedIndex[s] == nil { seenReferences := make(map[string]bool) var journey []*Reference @@ -291,8 +301,16 @@ func visitIndex(res *Resolver, idx *SpecIndex) { } idx.pendingResolve = refs + // Sort schema keys for deterministic iteration order schemas := idx.GetAllComponentSchemas() - for s, schemaRef := range schemas { + schemaKeys := make([]string, 0, len(schemas)) + for k := range schemas { + schemaKeys = append(schemaKeys, k) + } + sort.Strings(schemaKeys) + + for _, s := range schemaKeys { + schemaRef := schemas[s] if mappedIndex[s] == nil { seenReferences := make(map[string]bool) var journey []*Reference @@ -301,8 +319,16 @@ func visitIndex(res *Resolver, idx *SpecIndex) { } } - schemas = idx.GetAllSecuritySchemes() - for s, schemaRef := range schemas { + // Sort security scheme keys for deterministic iteration order + securitySchemes := idx.GetAllSecuritySchemes() + securityKeys := make([]string, 0, len(securitySchemes)) + for k := range securitySchemes { + securityKeys = append(securityKeys, k) + } + sort.Strings(securityKeys) + + for _, s := range securityKeys { + schemaRef := securitySchemes[s] if mappedIndex[s] == nil { seenReferences := make(map[string]bool) var journey []*Reference diff --git a/index/spec_index.go b/index/spec_index.go index ee942955..d7cf7c01 100644 --- a/index/spec_index.go +++ b/index/spec_index.go @@ -103,12 +103,15 @@ func createNewIndex(ctx context.Context, rootNode *yaml.Node, index *SpecIndex, } } - // map poly refs + // map poly refs - sort keys for deterministic ordering + polyKeys := make([]string, 0, len(index.polymorphicRefs)) + for k := range index.polymorphicRefs { + polyKeys = append(polyKeys, k) + } + sort.Strings(polyKeys) poly := make([]*Reference, len(index.polymorphicRefs)) - z := 0 - for i := range index.polymorphicRefs { - poly[z] = index.polymorphicRefs[i] - z++ + for i, k := range polyKeys { + poly[i] = index.polymorphicRefs[k] } // pull out references From e2642b8dc16b16d68dc0d0a7c52390b438f6d75e Mon Sep 17 00:00:00 2001 From: quobix Date: Fri, 19 Dec 2025 19:53:06 -0500 Subject: [PATCH 2/4] address review comments, added some better docs too. --- index/extract_refs.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/index/extract_refs.go b/index/extract_refs.go index bb06d7fd..050700b6 100644 --- a/index/extract_refs.go +++ b/index/extract_refs.go @@ -735,7 +735,7 @@ func (index *SpecIndex) ExtractComponentsFromRefs(ctx context.Context, refs []*R located := index.locateRef(ctx, ref) if located != nil { index.refLock.Lock() - if index.allMappedRefs[ref.FullDefinition] == nil { + if index.allMappedRefs[located.FullDefinition] == nil { index.allMappedRefs[located.FullDefinition] = located found = append(found, located) } @@ -839,7 +839,10 @@ func (index *SpecIndex) ExtractComponentsFromRefs(ctx context.Context, refs []*R ref := refsToCheck[c.pos] located := c.ref - // Reconcile nil results - check if another goroutine succeeded + // Reconcile nil results - check if another goroutine succeeded. + // We use ref.FullDefinition here because that's the singleflight key, + // and located.FullDefinition should match ref.FullDefinition for the + // same reference (FindComponent returns the component at that definition). if located == nil { index.refLock.RLock() located = index.allMappedRefs[ref.FullDefinition] @@ -887,14 +890,17 @@ func (index *SpecIndex) ExtractComponentsFromRefs(ctx context.Context, refs []*R // locateRef finds a component for a reference, including KeyNode extraction. // This is a helper used by ExtractComponentsFromRefs to isolate the lookup logic. func (index *SpecIndex) locateRef(ctx context.Context, ref *Reference) *Reference { - // External references need locking during FindComponent + // External references require a full Lock (not RLock) during FindComponent because + // FindComponent may trigger rolodex file loading which mutates index state. + // Internal references can proceed without locking since they only read from + // already-populated data structures. uri := strings.Split(ref.FullDefinition, "#/") - unsafeAsync := len(uri) == 2 && len(uri[0]) > 0 - if unsafeAsync { + isExternalRef := len(uri) == 2 && len(uri[0]) > 0 + if isExternalRef { index.refLock.Lock() } located := index.FindComponent(ctx, ref.FullDefinition) - if unsafeAsync { + if isExternalRef { index.refLock.Unlock() } From 4e1d3ddf026455e45c5774045ccbe41bdfbe7b39 Mon Sep 17 00:00:00 2001 From: quobix Date: Fri, 19 Dec 2025 20:14:15 -0500 Subject: [PATCH 3/4] bump coverage --- index/extract_refs.go | 6 ++++-- index/spec_index_test.go | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/index/extract_refs.go b/index/extract_refs.go index 050700b6..89a64ada 100644 --- a/index/extract_refs.go +++ b/index/extract_refs.go @@ -805,8 +805,10 @@ func (index *SpecIndex) ExtractComponentsFromRefs(ctx context.Context, refs []*R return index.locateRef(ctx, ref), nil }) - if result != nil { - resultsChan <- indexedRef{ref: result.(*Reference), pos: i} + // Type assert and check for nil - interface containing nil pointer is not nil + located := result.(*Reference) + if located != nil { + resultsChan <- indexedRef{ref: located, pos: i} } else { resultsChan <- indexedRef{ref: nil, pos: i} // Track failures for reconciliation } diff --git a/index/spec_index_test.go b/index/spec_index_test.go index 8aab6cff..219d34bd 100644 --- a/index/spec_index_test.go +++ b/index/spec_index_test.go @@ -1000,6 +1000,51 @@ func TestSpecIndex_ExtractComponentsFromRefs(t *testing.T) { assert.Len(t, index.GetReferenceIndexErrors(), 0) } +func TestSpecIndex_ExtractComponentsFromRefs_EmptyRefs(t *testing.T) { + yml := `openapi: 3.0.0 +info: + title: Test + version: 1.0.0` + + var rootNode yaml.Node + _ = yaml.Unmarshal([]byte(yml), &rootNode) + + index := NewSpecIndexWithConfig(&rootNode, CreateOpenAPIIndexConfig()) + + // Call with empty refs - should return nil (covers line 725) + result := index.ExtractComponentsFromRefs(context.Background(), []*Reference{}) + assert.Nil(t, result) +} + +func TestSpecIndex_ExtractComponentsFromRefs_NotFoundAsync(t *testing.T) { + yml := `openapi: 3.0.0 +info: + title: Test + version: 1.0.0 +components: + schemas: + exists: + type: string` + + var rootNode yaml.Node + _ = yaml.Unmarshal([]byte(yml), &rootNode) + + config := CreateOpenAPIIndexConfig() + config.ExtractRefsSequentially = false // Ensure async mode + index := NewSpecIndexWithConfig(&rootNode, config) + + // Create a reference to a non-existent component + ref := &Reference{ + FullDefinition: "#/components/schemas/doesNotExist", + Definition: "#/components/schemas/doesNotExist", + } + + // Call with a ref that won't be found - covers line 811 (nil result in async mode) + result := index.ExtractComponentsFromRefs(context.Background(), []*Reference{ref}) + assert.Empty(t, result) + assert.Len(t, index.GetReferenceIndexErrors(), 1) +} + func TestSpecIndex_FindComponent_WithACrazyAssPath(t *testing.T) { yml := `paths: /crazy/ass/references: From 94dfb0da8f8e75e3d044434ca073e7dba54f1dce Mon Sep 17 00:00:00 2001 From: quobix Date: Fri, 19 Dec 2025 20:30:49 -0500 Subject: [PATCH 4/4] Address issue with dropping params. Reported on discord. params were being dropped on render because the skip flag was bugging things out. --- datamodel/high/node_builder.go | 7 +- datamodel/high/node_builder_test.go | 192 ++++++++++++++++++++++++++++ document_test.go | 143 +++++++++++++++++++++ 3 files changed, 338 insertions(+), 4 deletions(-) diff --git a/datamodel/high/node_builder.go b/datamodel/high/node_builder.go index 0e9aff5d..dc1a991c 100644 --- a/datamodel/high/node_builder.go +++ b/datamodel/high/node_builder.go @@ -394,6 +394,9 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *nodes.NodeEntry) *ya sl := utils.CreateEmptySequenceNode() skip := false for i := 0; i < m.Len(); i++ { + // Reset skip at the start of each iteration to handle items without low-level models + // (e.g., newly created high-level objects appended to an existing slice) + skip = false sqi := m.Index(i).Interface() // check if this is a reference. if glu, ok := sqi.(GoesLowUntyped); ok { @@ -406,11 +409,7 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *nodes.NodeEntry) *ya if !n.Resolve { sl.Content = append(sl.Content, n.renderReference(glu.GoLowUntyped().(low.IsReferenced))) skip = true - } else { - skip = false } - } else { - skip = false } } } diff --git a/datamodel/high/node_builder_test.go b/datamodel/high/node_builder_test.go index dfdbb1e8..6b6b9889 100644 --- a/datamodel/high/node_builder_test.go +++ b/datamodel/high/node_builder_test.go @@ -1406,3 +1406,195 @@ func TestNodeBuilder_RenderContext_FallbackToMarshalYAML_Slice(t *testing.T) { data, _ := yaml.Marshal(node) assert.Contains(t, string(data), "basic-item1") } + +// nilLowRenderable implements GoesLowUntyped but returns nil for GoLowUntyped() +// This simulates a newly created high-level object without a low-level model +type nilLowRenderable struct { + Value string +} + +func (n *nilLowRenderable) MarshalYAML() (interface{}, error) { + return utils.CreateStringNode("nillow-" + n.Value), nil +} + +func (n *nilLowRenderable) GoLowUntyped() any { + return nil // Simulates newly created high-level object without low-level model +} + +type testWithMixedSlice struct { + Items []*valueReferenceStruct `yaml:"items,omitempty"` +} + +type testWithNilLowSlice struct { + Items []*nilLowRenderable `yaml:"items,omitempty"` +} + +// TestNodeBuilder_SliceRefFollowedByNilLow tests the bug fix for rendering slices +// where a $ref item is followed by a newly created item without a low-level model. +// Before the fix, the 'skip' variable was not reset between iterations, causing +// items following a $ref to be incorrectly skipped when they had no low-level model. +func TestNodeBuilder_SliceRefFollowedByNilLow(t *testing.T) { + // Create a slice with a $ref followed by a new item without low-level model + refItem := &valueReferenceStruct{ + ref: true, + refStr: "#/components/parameters/RefId", + Value: "refValue", + } + newItem := &valueReferenceStruct{ + ref: false, // Not a reference + Value: "newValue", + } + + // The new item's GoLowUntyped returns a pointer to itself (not nil), + // but it's not a reference, so it should be rendered normally. + // However, to truly test the nil case, we need a different approach. + + ty := []*valueReferenceStruct{refItem, newItem} + t1 := test1{ + Throg: ty, + } + + nb := NewNodeBuilder(&t1, &t1) + node := nb.Render() + + data, _ := yaml.Marshal(node) + + // Both items should be rendered - the ref as $ref and the new item normally + assert.Contains(t, string(data), "$ref: '#/components/parameters/RefId'") + assert.Contains(t, string(data), "pizza") // newItem renders as "pizza" via MarshalYAML +} + +// testMixedItem can have either a reference low-level model or nil low-level model +type testMixedItem struct { + ref bool + refStr string + Value string + hasLowRef bool // if true, GoLowUntyped returns a reference; if false, returns nil +} + +func (t *testMixedItem) MarshalYAML() (interface{}, error) { + return utils.CreateStringNode("mixed-" + t.Value), nil +} + +func (t *testMixedItem) GoLowUntyped() any { + if t.hasLowRef { + return t + } + return nil +} + +func (t *testMixedItem) IsReference() bool { + return t.ref +} + +func (t *testMixedItem) GetReference() string { + return t.refStr +} + +func (t *testMixedItem) SetReference(ref string, _ *yaml.Node) { + t.refStr = ref +} + +func (t *testMixedItem) GetReferenceNode() *yaml.Node { + return nil +} + +type testWithMixedItems struct { + Items []*testMixedItem `yaml:"items,omitempty"` +} + +// TestNodeBuilder_SliceRefFollowedByNilLowItem tests the specific bug case: +// a $ref item followed by an item with nil GoLowUntyped() (newly created high-level object) +func TestNodeBuilder_SliceRefFollowedByNilLowItem(t *testing.T) { + // Create a slice where: + // 1. First item IS a reference with a low-level model + // 2. Second item has NO low-level model (GoLowUntyped returns nil) + refItem := &testMixedItem{ + ref: true, + refStr: "#/components/parameters/RefId", + Value: "refValue", + hasLowRef: true, // Has a low-level reference + } + newItem := &testMixedItem{ + ref: false, + Value: "newValue", + hasLowRef: false, // NO low-level model (simulates newly created item) + } + + ty := []*testMixedItem{refItem, newItem} + t1 := testWithMixedItems{ + Items: ty, + } + + nb := NewNodeBuilder(&t1, nil) + node := nb.Render() + + data, _ := yaml.Marshal(node) + result := strings.TrimSpace(string(data)) + + // The bug was that 'newItem' would be skipped because 'skip' wasn't reset + // after processing the $ref item, and newItem's GoLowUntyped() returns nil. + // After the fix, both items should be rendered. + assert.Contains(t, result, "$ref: '#/components/parameters/RefId'", "Reference item should be rendered") + assert.Contains(t, result, "mixed-newValue", "New item without low-level model should also be rendered") +} + +// TestNodeBuilder_SliceNilLowFollowedByRef tests the reverse case: +// an item with nil GoLowUntyped() followed by a $ref (should work in both cases) +func TestNodeBuilder_SliceNilLowFollowedByRef(t *testing.T) { + // Create a slice where: + // 1. First item has NO low-level model + // 2. Second item IS a reference with a low-level model + newItem := &testMixedItem{ + ref: false, + Value: "newValue", + hasLowRef: false, // NO low-level model + } + refItem := &testMixedItem{ + ref: true, + refStr: "#/components/parameters/RefId", + Value: "refValue", + hasLowRef: true, // Has a low-level reference + } + + ty := []*testMixedItem{newItem, refItem} + t1 := testWithMixedItems{ + Items: ty, + } + + nb := NewNodeBuilder(&t1, nil) + node := nb.Render() + + data, _ := yaml.Marshal(node) + result := strings.TrimSpace(string(data)) + + // Both items should be rendered + assert.Contains(t, result, "mixed-newValue", "New item without low-level model should be rendered") + assert.Contains(t, result, "$ref: '#/components/parameters/RefId'", "Reference item should be rendered") +} + +// TestNodeBuilder_SliceMultipleRefsAndNilLow tests multiple refs interspersed with nil-low items +func TestNodeBuilder_SliceMultipleRefsAndNilLow(t *testing.T) { + items := []*testMixedItem{ + {ref: true, refStr: "#/ref1", Value: "ref1", hasLowRef: true}, + {ref: false, Value: "new1", hasLowRef: false}, // nil low + {ref: true, refStr: "#/ref2", Value: "ref2", hasLowRef: true}, + {ref: false, Value: "new2", hasLowRef: false}, // nil low + } + + t1 := testWithMixedItems{ + Items: items, + } + + nb := NewNodeBuilder(&t1, nil) + node := nb.Render() + + data, _ := yaml.Marshal(node) + result := strings.TrimSpace(string(data)) + + // All items should be rendered + assert.Contains(t, result, "$ref: '#/ref1'") + assert.Contains(t, result, "mixed-new1") + assert.Contains(t, result, "$ref: '#/ref2'") + assert.Contains(t, result, "mixed-new2") +} diff --git a/document_test.go b/document_test.go index ef89c40e..0abf3689 100644 --- a/document_test.go +++ b/document_test.go @@ -887,6 +887,149 @@ func TestDocument_InputAsJSON_LargeIndent(t *testing.T) { assert.Equal(t, d, strings.TrimSpace(string(rend))) } +// TestDocument_AppendParameterAfterRef tests the bug fix for appending new parameters +// to an operation that has $ref parameters. Before the fix, if a $ref was the last +// parameter in the original spec, any appended parameters would be silently dropped +// during rendering because the 'skip' flag wasn't reset between slice iterations. +func TestDocument_AppendParameterAfterRef(t *testing.T) { + spec := `openapi: "3.1.0" +info: + title: Test + version: 1.0.0 +paths: + /test/{id}: + get: + parameters: + - $ref: '#/components/parameters/IdParam' + responses: + "200": + description: ok +components: + parameters: + IdParam: + name: id + in: path + required: true + schema: + type: string` + + doc, err := NewDocument([]byte(spec)) + if err != nil { + t.Fatalf("failed to create document: %v", err) + } + + result, errs := doc.BuildV3Model() + assert.NoError(t, errs) + + // Get the operation and append a new parameter + pathItem := result.Model.Paths.PathItems.GetOrZero("/test/{id}") + assert.NotNil(t, pathItem) + assert.NotNil(t, pathItem.Get) + + // Verify we have the original $ref parameter + assert.Len(t, pathItem.Get.Parameters, 1) + + // Append a new parameter (simulating what the user does) + newParam := &v3high.Parameter{ + Name: "Host", + In: "header", + } + pathItem.Get.Parameters = append(pathItem.Get.Parameters, newParam) + + // Render and reload + _, newDoc, _, err := doc.RenderAndReload() + assert.NoError(t, err) + + // Build the new model + newResult, errs := newDoc.BuildV3Model() + assert.NoError(t, errs) + + // Check that the new parameter is present + newPathItem := newResult.Model.Paths.PathItems.GetOrZero("/test/{id}") + assert.NotNil(t, newPathItem) + assert.NotNil(t, newPathItem.Get) + + // This was the bug: the new "Host" parameter was being dropped + assert.Len(t, newPathItem.Get.Parameters, 2, "Expected 2 parameters (original $ref + appended Host)") + + // Verify the Host parameter is present + foundHost := false + for _, p := range newPathItem.Get.Parameters { + if p.Name == "Host" && p.In == "header" { + foundHost = true + break + } + } + assert.True(t, foundHost, "Expected to find the appended 'Host' header parameter") +} + +// TestDocument_AppendParameterBeforeRef tests that appending works when $ref is not last +func TestDocument_AppendParameterBeforeRef(t *testing.T) { + spec := `openapi: "3.1.0" +info: + title: Test + version: 1.0.0 +paths: + /test/{id}: + get: + parameters: + - name: existing + in: query + schema: + type: string + - $ref: '#/components/parameters/IdParam' + responses: + "200": + description: ok +components: + parameters: + IdParam: + name: id + in: path + required: true + schema: + type: string` + + doc, err := NewDocument([]byte(spec)) + if err != nil { + t.Fatalf("failed to create document: %v", err) + } + + result, errs := doc.BuildV3Model() + assert.NoError(t, errs) + + // Get the operation and append a new parameter + pathItem := result.Model.Paths.PathItems.GetOrZero("/test/{id}") + + // Append a new parameter + newParam := &v3high.Parameter{ + Name: "Host", + In: "header", + } + pathItem.Get.Parameters = append(pathItem.Get.Parameters, newParam) + + // Render and reload + _, newDoc, _, err := doc.RenderAndReload() + assert.NoError(t, err) + + newResult, errs := newDoc.BuildV3Model() + assert.NoError(t, errs) + + newPathItem := newResult.Model.Paths.PathItems.GetOrZero("/test/{id}") + + // Should have 3 parameters: existing + $ref + Host + assert.Len(t, newPathItem.Get.Parameters, 3, "Expected 3 parameters") + + foundHost := false + for _, p := range newPathItem.Get.Parameters { + if p.Name == "Host" && p.In == "header" { + foundHost = true + break + } + } + assert.True(t, foundHost, "Expected to find the appended 'Host' header parameter") +} + func TestDocument_RenderWithIndention(t *testing.T) { spec := `openapi: "3.1.0" info: