Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions index/find_component_external.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ func (index *SpecIndex) lookupRolodex(ctx context.Context, uri []string) *Refere
"file", absoluteFileLocation)
return nil
}
if rFile.GetIndex() == nil && !IsFileBeingIndexed(ctx, absoluteFileLocation) {
rFile.WaitForIndexing()
}
// Looking up a component needs the parsed YAML, not the completed index.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Component lookups never used idx (FindComponent gets index), so dropping the wait is safe there.

Whole-file refs are the catch. When the target's index is still pending, Index: idx on the whole-file Reference now falls back to the caller's index. With ResolveNestedRefsWithDocumentContext on, the resolver searches nested refs in that index, which is the wrong document.

Can you only take Index from the target (rFile.GetIndex()), never the caller, and add a whole-file $ref: target.yaml case to the regression? Then this is good to go.

// Waiting here can deadlock concurrently indexed documents that reference
// each other, even when the referenced schema graph itself is finite.
if rFile.GetIndex() != nil {
idx = rFile.GetIndex()
}
Expand Down
84 changes: 84 additions & 0 deletions index/find_component_pending_index_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package index

import (
"context"
"path/filepath"
"testing"
"time"

"github.com/pb33f/go-yaml"
"github.com/pb33f/testify/assert"
)

func TestLookupRolodexDoesNotWaitForTargetIndex(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "target.yaml")
cfg := CreateOpenAPIIndexConfig()
cfg.BasePath = dir
rolo := NewRolodex(cfg)
cfg.Rolodex = rolo
files, err := NewLocalFSWithConfig(&LocalFSConfig{BaseDirectory: dir, IndexConfig: cfg})
assert.NoError(t, err)
pending := &LocalFile{
fullPath: path,
data: []byte("components:\n schemas:\n Item:\n type: object\n"),
indexingComplete: make(chan struct{}),
}
files.Files.Store(path, pending)
rolo.AddLocalFS(dir, files)
defer close(pending.indexingComplete)
var root yaml.Node
assert.NoError(t, yaml.Unmarshal([]byte("components: {}"), &root))
idx := NewSpecIndexWithConfig(&root, cfg)
result := make(chan *Reference, 1)
go func() {
result <- idx.lookupRolodex(context.Background(), []string{path, "components/schemas/Item"})
}()
select {
case ref := <-result:
assert.NotNil(t, ref)
case <-time.After(time.Second):
t.Fatal("reference lookup waited for target indexing; mutually referencing files can deadlock")
}
}

func TestExplicitWaitForIndexingStillWaitsForCompletion(t *testing.T) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: after this change WaitForIndexing and IsFileBeingIndexed have no production callers, so this test (and the extra assert in search_index_test.go) only keeps coverage on them. Fine to keep since both are exported, just flagging that they're dead in-tree now.

for name, file := range map[string]func(chan struct{}) *rolodexFile{
"local": func(done chan struct{}) *rolodexFile {
return &rolodexFile{localFile: &LocalFile{indexingComplete: done}}
},
"remote": func(done chan struct{}) *rolodexFile {
return &rolodexFile{remoteFile: &RemoteFile{indexingComplete: done}}
},
} {
t.Run(name, func(t *testing.T) {
indexed := make(chan struct{})
t.Cleanup(func() {
select {
case <-indexed:
default:
close(indexed)
}
})
returned := make(chan struct{})
go func() {
file(indexed).WaitForIndexing()
close(returned)
}()
select {
case <-returned:
t.Fatal("explicit wait returned before indexing completed")
case <-time.After(10 * time.Millisecond):
}
close(indexed)
select {
case <-returned:
case <-time.After(time.Second):
t.Fatal("explicit wait did not return after indexing completed")
}
// Files without an indexing operation have nothing to wait for.
file(nil).WaitForIndexing()
})
}
(&rolodexFile{}).WaitForIndexing()
}
1 change: 1 addition & 0 deletions index/search_index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,7 @@ paths: {}`
func TestIsFileBeingIndexed_HTTPPathMatch(t *testing.T) {
// Test that HTTP paths match when the path portion is the same
ctx := context.Background()
assert.False(t, IsFileBeingIndexed(ctx, "https://example.com/schemas/pet.yaml"))

// Add an HTTP file to the indexing context
files := make(map[string]bool)
Expand Down
Loading