diff --git a/index/rolodex_file_loader.go b/index/rolodex_file_loader.go index 63e5cb1b..abfa63e4 100644 --- a/index/rolodex_file_loader.go +++ b/index/rolodex_file_loader.go @@ -157,11 +157,7 @@ func (l *LocalFS) OpenWithContext(ctx context.Context, name string) (fs.File, er copiedCfg.Rolodex = l.rolodex } - // Add this file to the context's indexing set to prevent deadlocks - // when circular references cause the same file to be looked up recursively. - indexingCtx := AddIndexingFile(ctx, name) - - idx, _ := extractedFile.IndexWithContext(indexingCtx, &copiedCfg) + idx, _ := extractedFile.IndexWithContext(ctx, &copiedCfg) if idx != nil { NewResolver(idx) diff --git a/index/rolodex_nested_files_test.go b/index/rolodex_nested_files_test.go new file mode 100644 index 00000000..5bb2324e --- /dev/null +++ b/index/rolodex_nested_files_test.go @@ -0,0 +1,103 @@ +package index + +import ( + "context" + "fmt" + "io" + "net/http" + "strconv" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" +) + +const nestedRemoteFilesBase = "https://nested.test/" + +func nestedRemoteFileRef(n int) string { + return fmt.Sprintf("components:\n schemas:\n A:\n $ref: '%sf%d.yaml#/components/schemas/A'\n", nestedRemoteFilesBase, n) +} + +// indexNestedRemoteFiles indexes a root document followed by a chain of remote files f1 to +// fDepth, where each file references the next, so each one is indexed while the ones before +// it still are. It returns the number of files fetched and the result of indexing. +func indexNestedRemoteFiles(depth int, lastFile string) (int64, error) { + var fetched atomic.Int64 + cfg := CreateOpenAPIIndexConfig() + cfg.RemoteURLHandler = func(url string) (*http.Response, error) { + name := strings.TrimSuffix(strings.TrimPrefix(url, nestedRemoteFilesBase+"f"), ".yaml") + n, err := strconv.Atoi(name) + if err != nil { + return nil, fmt.Errorf("unexpected url %q", url) + } + fetched.Add(1) + doc := lastFile + if n < depth { + doc = nestedRemoteFileRef(n + 1) + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(doc)), + }, nil + } + + var root yaml.Node + if err := yaml.Unmarshal([]byte(nestedRemoteFileRef(1)), &root); err != nil { + return 0, err + } + + rolodex := NewRolodex(cfg) + remoteFS, err := NewRemoteFSWithConfig(cfg) + if err != nil { + return 0, err + } + rolodex.AddRemoteFS(nestedRemoteFilesBase, remoteFS) + rolodex.SetRootNode(&root) + err = rolodex.IndexTheRolodex(context.Background()) + return fetched.Load(), err +} + +const nestedRemoteLeaf = "components:\n schemas:\n A:\n type: string\n" + +func TestRolodex_NestedRemoteFiles_IndexesEveryFile(t *testing.T) { + fetched, err := indexNestedRemoteFiles(50, nestedRemoteLeaf) + require.NoError(t, err) + assert.Equal(t, int64(50), fetched) +} + +func TestRolodex_CircularRemoteFiles_DoNotDeadlock(t *testing.T) { + done := make(chan int64, 1) + go func() { + // The result of indexing depends on circular reference handling; only finishing matters here. + fetched, _ := indexNestedRemoteFiles(3, nestedRemoteFileRef(1)) + done <- fetched + }() + + select { + case fetched := <-done: + assert.Equal(t, int64(3), fetched) + case <-time.After(30 * time.Second): + t.Fatal("indexing circular remote files did not finish") + } +} + +func BenchmarkRolodex_NestedRemoteFiles(b *testing.B) { + for _, depth := range []int{10, 100, 500} { + b.Run(fmt.Sprintf("depth=%d", depth), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + fetched, err := indexNestedRemoteFiles(depth, nestedRemoteLeaf) + if err != nil { + b.Fatal(err) + } + if fetched != int64(depth) { + b.Fatalf("fetched %d files, want %d", fetched, depth) + } + } + }) + } +} diff --git a/index/rolodex_owned_file.go b/index/rolodex_owned_file.go index 247e8d7c..cfcead4b 100644 --- a/index/rolodex_owned_file.go +++ b/index/rolodex_owned_file.go @@ -54,8 +54,7 @@ func (r *Rolodex) ownIndexedFile(source RolodexFile) (*LocalFile, error) { cfg.SpecAbsolutePath = location cfg.SpecInfo = info cfg.AvoidBuildIndex = true - ctx := AddIndexingFile(context.Background(), location) - idx := NewSpecIndexWithConfigAndContext(ctx, info.RootNode, &cfg) + idx := NewSpecIndexWithConfigAndContext(context.Background(), info.RootNode, &cfg) resolver := NewResolver(idx) resolver.IgnoreArray = cfg.IgnoreArrayCircularReferences resolver.IgnorePoly = cfg.IgnorePolymorphicCircularReferences diff --git a/index/rolodex_remote_loader.go b/index/rolodex_remote_loader.go index aa8e89e2..57699bd4 100644 --- a/index/rolodex_remote_loader.go +++ b/index/rolodex_remote_loader.go @@ -644,7 +644,7 @@ func (i *RemoteFS) OpenWithContext(ctx context.Context, remoteURL string) (fs.Fi i.Files.Store(cacheKey, remoteFile) i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, remoteFile, nil) - i.indexRemoteFile(ctx, remoteFile, copiedCfg, remoteParsedURL, remoteParsedURLOriginal) + i.indexRemoteFile(ctx, remoteFile, copiedCfg, remoteParsedURL) return remoteFile, errors.Join(i.remoteErrors...) } @@ -800,13 +800,9 @@ func (i *RemoteFS) indexRemoteFile( ctx context.Context, remoteFile *RemoteFile, copiedCfg *SpecIndexConfig, - remoteParsedURL, remoteParsedURLOriginal *url.URL, + remoteParsedURL *url.URL, ) { - indexingCtx := AddIndexingFile(ctx, remoteParsedURL.Path) - indexingCtx = AddIndexingFile(indexingCtx, remoteParsedURL.String()) - indexingCtx = AddIndexingFile(indexingCtx, remoteParsedURLOriginal.String()) - - idx, idxError := remoteFile.Index(indexingCtx, copiedCfg) + idx, idxError := remoteFile.Index(ctx, copiedCfg) if idxError != nil && idx == nil { i.appendRemoteError(idxError) remoteFile.signalIndexingComplete() diff --git a/index/search_index.go b/index/search_index.go index d61e6a85..19b7b9f8 100644 --- a/index/search_index.go +++ b/index/search_index.go @@ -14,14 +14,20 @@ import ( type ContextKey string const ( - CurrentPathKey ContextKey = "currentPath" - FoundIndexKey ContextKey = "foundIndex" - RootIndexKey ContextKey = "currentIndex" - IndexingFilesKey ContextKey = "indexingFiles" // Tracks files being indexed in current call chain + CurrentPathKey ContextKey = "currentPath" + FoundIndexKey ContextKey = "foundIndex" + RootIndexKey ContextKey = "currentIndex" + // IndexingFilesKey holds the set used by AddIndexingFile and GetIndexingFiles. + // + // Deprecated: the library no longer tracks the files it is indexing. + IndexingFilesKey ContextKey = "indexingFiles" ) // GetIndexingFiles returns the set of files currently being indexed in the call chain. // Returns nil if not set. +// +// Deprecated: the library no longer tracks the files it is indexing, so this only +// returns what the caller added with AddIndexingFile. func GetIndexingFiles(ctx context.Context) map[string]bool { if v := ctx.Value(IndexingFilesKey); v != nil { return v.(map[string]bool) @@ -31,6 +37,8 @@ func GetIndexingFiles(ctx context.Context) map[string]bool { // AddIndexingFile adds a file to the indexing set in the context. // Returns a new context with the updated set. +// +// Deprecated: the library no longer reads this set when indexing. func AddIndexingFile(ctx context.Context, filePath string) context.Context { existing := GetIndexingFiles(ctx) newSet := make(map[string]bool) @@ -45,6 +53,9 @@ func AddIndexingFile(ctx context.Context, filePath string) context.Context { // For HTTP URLs, it also checks if the PATH portion matches any indexed file, // since the same file might be referenced with different hostnames (which get normalized // to a common server). +// +// Deprecated: the library no longer tracks the files it is indexing, so this only +// sees what the caller added with AddIndexingFile. func IsFileBeingIndexed(ctx context.Context, filePath string) bool { files := GetIndexingFiles(ctx) if files == nil { diff --git a/index/search_index_test.go b/index/search_index_test.go index 865738e5..4cc1e608 100644 --- a/index/search_index_test.go +++ b/index/search_index_test.go @@ -832,6 +832,16 @@ func TestIsFileBeingIndexed_HTTPPathMatch(t *testing.T) { assert.False(t, IsFileBeingIndexed(ctx, "https://example.com/other/file.yaml")) } +func TestAddIndexingFile_ExtendsTheSetOfTheChildContextOnly(t *testing.T) { + parent := AddIndexingFile(context.Background(), "/tmp/specs/pet.yaml") + child := AddIndexingFile(parent, "/tmp/specs/cat.yaml") + + assert.Equal(t, map[string]bool{"/tmp/specs/pet.yaml": true}, GetIndexingFiles(parent)) + assert.Equal(t, map[string]bool{"/tmp/specs/pet.yaml": true, "/tmp/specs/cat.yaml": true}, GetIndexingFiles(child)) + assert.True(t, IsFileBeingIndexed(child, "/tmp/specs/cat.yaml")) + assert.False(t, IsFileBeingIndexed(parent, "/tmp/specs/cat.yaml")) +} + func TestIsFileBeingIndexed_HTTPMatchesLocalFilename(t *testing.T) { ctx := context.Background()