From d3558b2e27b9a8887c3e534fc26c162f6fe45279 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 03:59:59 +0300 Subject: [PATCH 1/3] perf(compilers/openapi): index the source tree once per compile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One compile parsed the same source bytes twice — once in the pre-parse cycle scan, once in the loader — and then walked the resulting node tree four times: the recursive-anchor descent, the pure-$ref collection, the raw node count, and the alias weigher, each starting from the root with no shared state. internal/sourceindex walks the decoded tree once and answers what the anchor descent and the node count each walked it to ask. The loader decodes, indexes, and hands the index to scan, so a source is parsed once and the two walks that only ever asked something about the tree itself become one. The walk is bounded by MaxIndexedNodes rather than by the input. A tree past it is refused under a new openapi/source-too-large code rather than half-counted: every answer in a truncated index is partial, including the node count the alias-expansion allowance is derived from, and an allowance computed from a count that stopped early would refuse documents on a bound they never crossed. Behaviour is unchanged. Every golden and conformance snapshot is byte-identical without regeneration, and the whole testdata corpus compiles to identical documents, diagnostics and exit codes before and after. --- compilers/openapi/cycles_test.go | 13 +- compilers/openapi/internal/diag/diag.go | 6 + compilers/openapi/internal/diag/diag_test.go | 3 +- .../internal/load/entry_internal_test.go | 85 ++++++ compilers/openapi/internal/load/load.go | 59 +++- .../scan/amplification_internal_test.go | 46 ++-- compilers/openapi/internal/scan/scan.go | 166 ++++-------- .../internal/scan/scan_internal_test.go | 106 ++++---- .../internal/sourceindex/sourceindex.go | 168 ++++++++++++ .../sourceindex/sourceindex_internal_test.go | 253 ++++++++++++++++++ docs/micro-compiler-design.md | 18 +- internal/archtest/arch_test.go | 19 +- 12 files changed, 720 insertions(+), 222 deletions(-) create mode 100644 compilers/openapi/internal/sourceindex/sourceindex.go create mode 100644 compilers/openapi/internal/sourceindex/sourceindex_internal_test.go diff --git a/compilers/openapi/cycles_test.go b/compilers/openapi/cycles_test.go index 4a18066e..e9213767 100644 --- a/compilers/openapi/cycles_test.go +++ b/compilers/openapi/cycles_test.go @@ -8,10 +8,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v3" "github.com/dexpace/morphic/compilers" "github.com/dexpace/morphic/compilers/openapi/internal/diag" "github.com/dexpace/morphic/compilers/openapi/internal/scan" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" "github.com/dexpace/morphic/ir" ) @@ -94,7 +96,7 @@ func TestDetectCycles_ComponentOnlyCyclesLeftToResolver(t *testing.T) { for _, tc := range componentOnlyCycles { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Empty(t, scan.Cycles(0, []byte(tc.data)), + assert.Empty(t, scan.Cycles(0, scanIndex(t, tc.data)), "a components-only cycle is the resolver's to report") _, diags, err := New().Compile(t.Context(), @@ -207,6 +209,15 @@ func TestCompile_RefShapedDataNotRefused(t *testing.T) { } } +// scanIndex decodes a spec and indexes it, which is what load hands the scan +// once the compile's one decode has run. +func scanIndex(t *testing.T, src string) sourceindex.Index { + t.Helper() + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(src), &root)) + return sourceindex.Build(&root, sourceindex.MaxIndexedNodes) +} + func readReproducer(t *testing.T, file string) []byte { t.Helper() data, err := os.ReadFile("../../testdata/openapi/" + file + ".yaml") diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index 59027084..1c731b53 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -38,6 +38,12 @@ const ( // for the source. It is a warning, never a refusal: the compile still // proceeds, and every cycle the scan did classify is still caught. CycleScanFailed = "openapi/cycle-scan-failed" + // SourceTooLarge reports a document with more YAML nodes than the pre-parse + // scan indexes (sourceindex.MaxIndexedNodes). Every answer the index gives + // about such a document is a partial one, including the node count the + // alias-expansion allowance is derived from, so the document is refused rather + // than scanned against a bound computed from a count that stopped early. + SourceTooLarge = "openapi/source-too-large" // OverlayInvalid reports an overlay document that could not be parsed, or that // parsed but is not a valid Overlay — a missing version, no actions, an action // naming no target. Nothing is applied, so the compile refuses rather than diff --git a/compilers/openapi/internal/diag/diag_test.go b/compilers/openapi/internal/diag/diag_test.go index 26e351ac..b062dcc3 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -128,7 +128,8 @@ func TestHasError_Cases(t *testing.T) { func codes() []string { return []string{ diag.Validation, diag.UnsupportedVersion, diag.UnresolvedRef, diag.CyclicRef, - diag.CycleScanFailed, diag.OverlayInvalid, diag.OverlayFailed, + diag.CycleScanFailed, diag.SourceTooLarge, + diag.OverlayInvalid, diag.OverlayFailed, diag.OverlayAction, diag.OverlayOriginIncomplete, diag.ValidationOnlyKeyword, diag.FalseSchema, diag.NumericPrecision, diag.ExclusiveBoundForm, diag.InvalidStatusKey, diff --git a/compilers/openapi/internal/load/entry_internal_test.go b/compilers/openapi/internal/load/entry_internal_test.go index 0e39055c..c679d4aa 100644 --- a/compilers/openapi/internal/load/entry_internal_test.go +++ b/compilers/openapi/internal/load/entry_internal_test.go @@ -8,10 +8,13 @@ import ( "github.com/speakeasy-api/openapi/validation" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v3" "github.com/dexpace/morphic/compilers" "github.com/dexpace/morphic/compilers/openapi/internal/diag" "github.com/dexpace/morphic/compilers/openapi/internal/overlay" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" + "github.com/dexpace/morphic/ir" ) // sourceOf wraps src as the single source a load call takes. @@ -302,3 +305,85 @@ func TestLoad_RejectsAnOverlaySharingTheSourceIndex(t *testing.T) { require.NoError(t, err, "an index of its own is fine: %+v", diags) assert.True(t, got.Overlay.Applied()) } + +// TestLoad_ADocumentTooLargeToIndexIsRefused drives the refusal a document draws +// before the cycle scan reads a single reference: one with more YAML nodes than +// the source index walks. +// +// The bound is reached by shrinking it rather than by building a document of +// sourceindex.MaxIndexedNodes nodes — that would be several gigabytes of +// fixture, and the part worth testing is what the loader does with a truncated +// index, not that the walk stops at a number. It is not parallel, because it +// rebinds a package-level value the parallel tests around it also read. +func TestLoad_ADocumentTooLargeToIndexIsRefused(t *testing.T) { + orig := buildIndex + t.Cleanup(func() { buildIndex = orig }) + buildIndex = func(root *yaml.Node) sourceindex.Index { return sourceindex.Build(root, 1) } + + doc, diags, err := Load(t.Context(), 4, sourceOf(minimal31), Options{}) + + require.NoError(t, err, "an oversized document is a spec problem, not a Go error") + assert.Nil(t, doc, "nothing is lowered from a document the pre-parse scan cannot cover") + require.Len(t, diags, 1) + assert.Equal(t, diag.SourceTooLarge, diags[0].Code) + assert.Equal(t, ir.SeverityError, diags[0].Severity) + assert.Equal(t, 4, diags[0].Provenance.Source, "the refusal names the source it read") +} + +// TestLoad_TheSameDocumentLoadsOnceItFitsTheIndex is the control for the test +// above: with the real bound in force the identical source loads, so the refusal +// is the bound's doing and not the document's. +func TestLoad_TheSameDocumentLoadsOnceItFitsTheIndex(t *testing.T) { + t.Parallel() + got, diags, err := Load(t.Context(), 4, sourceOf(minimal31), Options{}) + + require.NoError(t, err) + require.NotNil(t, got) + assert.False(t, diag.HasError(diags), "unexpected refusal: %+v", diags) +} + +// countingIndexBuilder makes the loader count its index builds for one test, +// and returns the counter. It is not parallel-safe, for the reason +// TestLoad_ADocumentTooLargeToIndexIsRefused is not. +func countingIndexBuilder(t *testing.T) *int { + t.Helper() + orig := buildIndex + t.Cleanup(func() { buildIndex = orig }) + built := 0 + buildIndex = func(root *yaml.Node) sourceindex.Index { + built++ + return orig(root) + } + return &built +} + +// TestLoad_IndexesTheSourceOnce guards the shape of this path rather than its +// output: one decode, one index, and every pre-parse refusal reading that index +// instead of walking the tree again. A change that re-added a walk of its own +// would break no assertion about what the compiler reports — the refusals would +// still be right — so the count is the only thing that can hold it. +func TestLoad_IndexesTheSourceOnce(t *testing.T) { + built := countingIndexBuilder(t) + + got, diags, err := Load(t.Context(), 0, sourceOf(minimal31), Options{}) + + require.NoError(t, err) + require.NotNil(t, got) + assert.False(t, diag.HasError(diags), "unexpected refusal: %+v", diags) + assert.Equal(t, 1, *built, "a compile with no overlay indexes its source exactly once") +} + +// TestLoad_IndexesAPatchedTreeAgain is the one second index that is correct: an +// overlay leaves behind a tree the first one no longer describes, and what the +// refusals answer for is the tree the parser is handed. +func TestLoad_IndexesAPatchedTreeAgain(t *testing.T) { + built := countingIndexBuilder(t) + + got, diags, err := Load(t.Context(), 0, sourceOf(minimal31), + overlayOptions(" - target: $.info\n update: {description: d}\n")) + + require.NoError(t, err) + require.NotNil(t, got) + assert.False(t, diag.HasError(diags), "unexpected refusal: %+v", diags) + assert.Equal(t, 2, *built, "the source, then the tree the overlay left behind") +} diff --git a/compilers/openapi/internal/load/load.go b/compilers/openapi/internal/load/load.go index aae8b0db..b54f335f 100644 --- a/compilers/openapi/internal/load/load.go +++ b/compilers/openapi/internal/load/load.go @@ -29,6 +29,7 @@ import ( "github.com/dexpace/morphic/compilers/openapi/internal/diag" "github.com/dexpace/morphic/compilers/openapi/internal/overlay" "github.com/dexpace/morphic/compilers/openapi/internal/scan" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" "github.com/dexpace/morphic/compilers/openapi/internal/value" "github.com/dexpace/morphic/ir" ) @@ -92,17 +93,17 @@ func Load(ctx context.Context, srcIndex int, src compilers.Source, opts Options) return nil, nil, fmt.Errorf("openapi: overlay source index %d is source %d's own", opts.OverlaySrcIndex, srcIndex) } - cyc := scan.Cycles(srcIndex, src.Data) - if diag.HasError(cyc) { - return nil, cyc, nil // degenerate cycle: refuse to lower, do not crash the parser - } - // cyc may still hold a non-fatal scan-incomplete warning; carry it forward. - root, err := decode(src.Data) if err != nil { return nil, nil, fmt.Errorf("openapi: decode source %d: %w", srcIndex, err) } + cyc := refusals(srcIndex, root) + if diag.HasError(cyc) { + return nil, cyc, nil // degenerate cycle: refuse to lower, do not crash the parser + } + // cyc may still hold a non-fatal scan-incomplete warning; carry it forward. + origin, patchDiags := patch(srcIndex, root, opts) cyc = append(cyc, patchDiags...) if diag.HasError(cyc) { @@ -151,12 +152,40 @@ func Load(ctx context.Context, srcIndex int, src compilers.Source, opts Options) }, diags, nil } +// buildIndex indexes a decoded tree under the compiler's node bound. It is a +// package-level function value only so a test can drive the truncated-index +// refusal without materializing a document of sourceindex.MaxIndexedNodes nodes; +// nothing in the pipeline rebinds it, so the stages stay pure and reentrant. +var buildIndex = func(root *yaml.Node) sourceindex.Index { + return sourceindex.Build(root, sourceindex.MaxIndexedNodes) +} + +// refusals indexes a decoded tree once and reports the pre-parse refusals over +// it: the degenerate reference and alias structures scan finds, or — when the +// document is too large to index in full — a refusal of its own. +// +// The size refusal is here rather than in scan because every answer in a +// truncated index is a partial one, and the alias-expansion allowance derived +// from a partial node count would refuse documents on a bound they never +// crossed. A document that large is beyond what the pre-parse guarantees cover, +// so it is refused rather than lowered on incomplete information. +func refusals(srcIndex int, root *yaml.Node) []ir.Diagnostic { + idx := buildIndex(root) + if idx.Truncated() { + return []ir.Diagnostic{diag.Newf(ir.SeverityError, diag.SourceTooLarge, + ir.Provenance{Source: srcIndex}, + "source document exceeds the %d-node bound the pre-parse scan indexes", + sourceindex.MaxIndexedNodes)} + } + return scan.Cycles(srcIndex, idx) +} + // patch applies the caller's overlay to the decoded tree, or does nothing when // there is none. // // It re-runs the pre-parse refusals over the result, because the tree that -// reaches the parser is no longer the bytes scan.Cycles saw: an overlay action -// can graft a $ref cycle onto a document that had none, and the guarantee those +// reaches the parser is no longer the one they first saw: an overlay action can +// graft a $ref cycle onto a document that had none, and the guarantee those // refusals exist for is about what the parser is handed. func patch(srcIndex int, root *yaml.Node, opts Options) (overlay.Origin, []ir.Diagnostic) { if opts.Overlay == nil { @@ -166,7 +195,7 @@ func patch(srcIndex int, root *yaml.Node, opts Options) (overlay.Origin, []ir.Di if diag.HasError(diags) { return overlay.Origin{}, diags } - return origin, append(diags, scan.CyclesInNode(srcIndex, root)...) + return origin, append(diags, refusals(srcIndex, root)...) } // metaSchemaReconciledMinor is the OpenAPI minor whose schema findings are @@ -366,11 +395,15 @@ func walkNumericScalars(node *yaml.Node, depth int, visit func(*yaml.Node)) { // renumbers every line in the document, and every diagnostic about the source // would then name a position in a file that exists nowhere. // +// It is the compile's only parse of the source: the pre-parse refusals used to +// decode the same bytes a second time to scan them, and now read the tree this +// produces. That also makes it the one place the yaml.v3 alias budget is spent, +// which is what bounds a billion-laughs expansion before anything walks it. +// // It carries no recover of its own, unlike the model build and the resolve below -// it. yaml.v3 converts its own faults into errors before they leave Unmarshal, -// and the pre-parse scan has already decoded these same bytes under a barrier by -// the time this runs — the third-party code that has been seen to fault here is -// the layer above the decode, which is where the barrier is. +// it. yaml.v3 converts its own faults into errors before they leave Unmarshal; +// the third-party code that has been seen to fault is the layer above the +// decode, which is where the barriers are. func decode(data []byte) (*yaml.Node, error) { var root yaml.Node if err := yaml.Unmarshal(data, &root); err != nil { diff --git a/compilers/openapi/internal/scan/amplification_internal_test.go b/compilers/openapi/internal/scan/amplification_internal_test.go index da7d6900..5f1398e2 100644 --- a/compilers/openapi/internal/scan/amplification_internal_test.go +++ b/compilers/openapi/internal/scan/amplification_internal_test.go @@ -11,7 +11,6 @@ import ( "gopkg.in/yaml.v3" "github.com/dexpace/morphic/compilers/openapi/internal/diag" - "github.com/dexpace/morphic/compilers/openapi/internal/nodeview" "github.com/dexpace/morphic/ir" ) @@ -22,7 +21,7 @@ func TestAliasAmplification_BombFixtureIsRefused(t *testing.T) { data, err := os.ReadFile(amplificationBombFixture) require.NoError(t, err) - diags := Cycles(0, data) + diags := scanBytes(t, data) require.NotEmpty(t, diags, "an amplifying document must be diagnosed") assert.Equal(t, diag.AliasAmplification, diags[0].Code) assert.Equal(t, ir.SeverityError, diags[0].Severity) @@ -44,13 +43,11 @@ func TestDetectCycles_LargeAliasFreeDocumentIsClean(t *testing.T) { t.Parallel() src := bigAliasFreeSpec(bigDocSchemaCount) - var root yaml.Node - require.NoError(t, yaml.Unmarshal([]byte(src), &root)) - raw := rawNodeCount(nodeview.DocumentRoot(&root)) + raw := indexOf(t, []byte(src)).Nodes() require.Greater(t, raw, int64(minExpandedNodes), "the fixture must actually exceed the floor for this test to prove anything") - assert.Empty(t, Cycles(0, []byte(src)), + assert.Empty(t, scanBytes(t, []byte(src)), "a large alias-free document must never be refused: what it costs is what its own bytes already bought") } @@ -65,7 +62,7 @@ func TestDetectCycles_AnchorReuseWithinBudgetIsClean(t *testing.T) { fmt.Fprintf(&b, " S%d: {properties: {p: *base}}\n", i) } - assert.Empty(t, Cycles(0, []byte(b.String())), + assert.Empty(t, scanBytes(t, []byte(b.String())), "ordinary anchor reuse well under the budget is not amplification") } @@ -99,10 +96,8 @@ func TestDetectCycles_RealWorldAnchorReuseIsClean(t *testing.T) { const siblings = 900 src := wideBaseReuseSpec(props, siblings) - var root yaml.Node - require.NoError(t, yaml.Unmarshal([]byte(src), &root)) - docRoot := nodeview.DocumentRoot(&root) - raw := rawNodeCount(docRoot) + docRoot := indexOf(t, []byte(src)).Root() + raw := rawNodes(docRoot) probe := newAliasWeigher(raw * 1000) _, exceeded := probe.weigh(docRoot) require.False(t, exceeded, "sanity: the probe's own allowance must not itself be crossed") @@ -116,7 +111,7 @@ func TestDetectCycles_RealWorldAnchorReuseIsClean(t *testing.T) { require.GreaterOrEqual(t, surplus, int64(realWorldWorstSurplus), "sanity: this fixture's surplus must meet or exceed the worst real spec measured") - assert.Empty(t, Cycles(0, []byte(src)), + assert.Empty(t, scanBytes(t, []byte(src)), "ordinary DRY reuse of one shared base across many sibling schemas, at least as demanding as the worst real spec measured, is not amplification") } @@ -126,7 +121,7 @@ func TestDetectCycles_SyntheticWideBaseReuseIsNowRefused(t *testing.T) { const siblings = 500 src := wideBaseReuseSpec(props, siblings) - diags := Cycles(0, []byte(src)) + diags := scanBytes(t, []byte(src)) require.NotEmpty(t, diags, "44x beyond any real spec's surplus must be refused") assert.Equal(t, diag.AliasAmplification, diags[0].Code) assert.Equal(t, ir.SeverityError, diags[0].Severity) @@ -166,16 +161,14 @@ func TestDetectCycles_FlatFanOutOfModestAnchorIsEventuallyRefused(t *testing.T) const props = 4 const under = 9_000 - assert.Empty(t, Cycles(0, []byte(flatFanOutSpec(props, under))), + assert.Empty(t, scanBytes(t, []byte(flatFanOutSpec(props, under))), "a modest anchor reused this many times has not yet crossed the surplus budget") const over = 11_000 src := flatFanOutSpec(props, over) - var root yaml.Node - require.NoError(t, yaml.Unmarshal([]byte(src), &root)) - docRoot := nodeview.DocumentRoot(&root) - raw := rawNodeCount(docRoot) + docRoot := indexOf(t, []byte(src)).Root() + raw := rawNodes(docRoot) probe := newAliasWeigher(raw * 1000) _, exceeded := probe.weigh(docRoot) require.False(t, exceeded, "sanity: the probe's own allowance must not itself be crossed") @@ -183,7 +176,7 @@ func TestDetectCycles_FlatFanOutOfModestAnchorIsEventuallyRefused(t *testing.T) require.Less(t, expanded, int64(maxAliasAmplification)*raw, "sanity: this document's ratio must stay under maxAliasAmplification, so the refusal below is provably the surplus bound's doing, not the ratio's") - diags := Cycles(0, []byte(src)) + diags := scanBytes(t, []byte(src)) require.NotEmpty(t, diags, "unbounded reuse of even a modest anchor must eventually be refused") assert.Equal(t, diag.AliasAmplification, diags[0].Code) assert.Equal(t, ir.SeverityError, diags[0].Severity) @@ -219,13 +212,13 @@ func TestAliasAmplification_BoundaryPair(t *testing.T) { t.Parallel() under := aliasFanOutNode(12) - require.Equal(t, int64(5), rawNodeCount(under), "sanity: the raw count aliasFanOutNode promises") - _, refused := aliasAmplification(0, under) + require.Equal(t, int64(5), rawNodes(under), "sanity: the raw count aliasFanOutNode promises") + _, refused := aliasAmplification(0, under, rawNodes(under)) assert.False(t, refused, "expandedWeight 24,573 stays under the 32,768 floor") over := aliasFanOutNode(13) - require.Equal(t, int64(5), rawNodeCount(over), "sanity: the raw count aliasFanOutNode promises") - d, refused := aliasAmplification(0, over) + require.Equal(t, int64(5), rawNodes(over), "sanity: the raw count aliasFanOutNode promises") + d, refused := aliasAmplification(0, over, rawNodes(over)) require.True(t, refused, "expandedWeight 49,149 crosses the 32,768 floor") assert.Equal(t, diag.AliasAmplification, d.Code) assert.Equal(t, ir.SeverityError, d.Severity) @@ -245,7 +238,7 @@ func TestExpandedWeight_NoAliasesEqualsRawCount(t *testing.T) { yscalar("b"), yseq(yscalar("x"), yscalar("y"), ymap(yscalar("c"), yscalar("2"))), yscalar("d"), ymap(yscalar("e"), yscalar("3"), yscalar("f"), yscalar("4")), ) - raw := rawNodeCount(root) + raw := rawNodes(root) w := newAliasWeigher(raw + 1000) // an allowance nothing here can cross _, exceeded := w.weigh(root) @@ -304,11 +297,6 @@ func TestAliasWeigher_SaturatesWithoutOverflow(t *testing.T) { assert.Greater(t, w.weight[culprit], int64(0), "the saturated value must not have wrapped negative") } -func TestRawNodeCount_NilRoot(t *testing.T) { - t.Parallel() - assert.Equal(t, int64(0), rawNodeCount(nil)) -} - func TestChildrenOf_AliasWithoutTarget(t *testing.T) { t.Parallel() orphan := &yaml.Node{Kind: yaml.AliasNode} diff --git a/compilers/openapi/internal/scan/scan.go b/compilers/openapi/internal/scan/scan.go index 44661bd2..ee2b1b15 100644 --- a/compilers/openapi/internal/scan/scan.go +++ b/compilers/openapi/internal/scan/scan.go @@ -7,6 +7,11 @@ // alias fan-out that expands to far more nodes than the document declares // exhausts memory inside the parser. Each reads the raw text through nodeview, // and each runs before the document is handed to either. +// +// What the tree says about itself — its size, and whether an alias points back +// at one of its own ancestors — is not rederived here. The caller supplies a +// sourceindex.Index built over the same tree, so the questions that need only a +// walk are answered once for every refusal that asks them. package scan import ( @@ -17,13 +22,14 @@ import ( "github.com/dexpace/morphic/compilers/openapi/internal/diag" "github.com/dexpace/morphic/compilers/openapi/internal/nodeview" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" "github.com/dexpace/morphic/ir" ) -// maxCycleDepth bounds every recursive descent in the cycle detector. It guards +// maxCycleDepth bounds how many hops one pure-$ref chain is followed. It guards // the walk against a runaway structure per the bounded-recursion rule; real -// specs nest far shallower, so nothing short of a document built to reach it — -// or a detector bug — ever does. +// specs chain far shorter, so nothing short of a document built to reach it — or +// a detector bug — ever does. const maxCycleDepth = 10000 // schemaEntryMapKeys name a mapping of schemas encountered outside a schema @@ -59,35 +65,29 @@ var schemaDataKeys = map[string]bool{ "const": true, "enum": true, } -// Cycles scans raw source bytes for degenerate reference structures that would -// otherwise crash, hang or exhaust memory in the third-party parser and resolver -// (GitHub #12, GitHub #27, speakeasy-api/openapi#231), before soa.Unmarshal ever -// runs. It reports as error diagnostics: a recursive YAML anchor, a pure-$ref -// cycle (a chain of schema $refs that never reaches a node without one), a -// reference whose pointer resolves through a reference already being resolved, -// and alias amplification (a billion-laughs expansion). A source that doesn't -// decode as YAML yields no cycles — the main parser reports that as a parse -// problem — and the scan runs under recoverCycleScan so a detector bug degrades -// to "no cycle found" rather than aborting. -func Cycles(srcIndex int, data []byte) []ir.Diagnostic { - return recoverCycleScan(srcIndex, func() []ir.Diagnostic { - return scanCycles(srcIndex, data) - }) -} - -// CyclesInNode is Cycles over an already-decoded tree, for a caller holding one -// the source bytes no longer describe. +// Cycles scans an indexed source tree for degenerate reference structures that +// would otherwise crash, hang or exhaust memory in the third-party parser and +// resolver (GitHub #12, GitHub #27, speakeasy-api/openapi#231), before +// soa.Unmarshal ever runs. It reports as error diagnostics: a recursive YAML +// anchor, a pure-$ref cycle (a chain of schema $refs that never reaches a node +// without one), a reference whose pointer resolves through a reference already +// being resolved, and alias amplification (a billion-laughs expansion). The scan +// runs under recoverCycleScan so a detector bug degrades to "no cycle found" +// rather than aborting. // -// An overlay is the reason it exists: its actions can graft a $ref cycle onto a -// document that had none, and the refusals Cycles makes before the parser ever -// runs have to cover the tree that reaches the parser rather than only the bytes -// that reached the overlay. Decoding is the caller's here, so nothing re-reads -// the source — but a caller that has only bytes must still use Cycles, whose -// decode is what bounds alias expansion (the yaml.v3 alias budget is per-Decode, -// so a tree handed in has already spent one this cannot re-run). -func CyclesInNode(srcIndex int, root *yaml.Node) []ir.Diagnostic { +// The index is the caller's, built over the tree that will reach the parser: an +// overlay can graft a $ref cycle onto a document that had none, so a patched +// tree is re-indexed and re-scanned rather than trusted to the bytes that +// reached the overlay. The caller must not hand over a truncated index — every +// answer in one is partial, and the alias-expansion allowance derived from a +// partial node count would refuse documents on a bound they never crossed. +// +// Only the decode that produced the tree bounds alias expansion inside the +// parser: the yaml.v3 alias budget is spent per Decode, so a tree that reached +// here without one has already escaped it and nothing here can re-run it. +func Cycles(srcIndex int, idx sourceindex.Index) []ir.Diagnostic { return recoverCycleScan(srcIndex, func() []ir.Diagnostic { - return scanNode(srcIndex, nodeview.DocumentRoot(root)) + return scanIndex(srcIndex, idx) }) } @@ -107,32 +107,21 @@ func recoverCycleScan(srcIndex int, scan func() []ir.Diagnostic) (diags []ir.Dia return scan() } -// scanCycles decodes source bytes and reports the first degenerate cycle found, -// or nil. -func scanCycles(srcIndex int, data []byte) []ir.Diagnostic { - if len(data) == 0 { - return nil - } - var root yaml.Node - if err := yaml.Unmarshal(data, &root); err != nil { - return nil +// scanIndex reports the first degenerate cycle the index's tree carries, or nil. +// The index's root is nil for a source with no document in it; the ref walk and +// the weigher both treat that as "nothing to scan", so no explicit nil guard is +// needed here. +func scanIndex(srcIndex int, idx sourceindex.Index) []ir.Diagnostic { + if alias, ok := idx.AnchorCycle(); ok { + return []ir.Diagnostic{cyclicDiag(srcIndex, alias, + "recursive YAML anchor %q references an ancestor node", anchorName(alias))} } - return scanNode(srcIndex, nodeview.DocumentRoot(&root)) -} -// scanNode reports the first degenerate cycle reachable from an already-decoded -// document root, or nil. docRoot may be nil for an empty or malformed document; -// the anchor and ref walks both treat that as "nothing to scan", so no explicit -// nil guard is needed here. -func scanNode(srcIndex int, docRoot *yaml.Node) []ir.Diagnostic { - if d, ok := anchorCycle(srcIndex, docRoot); ok { - return []ir.Diagnostic{d} - } - - // anchorCycle must run first: a recursive anchor makes expandedWeight - // infinite, and having already refused those is what makes the alias - // graph a DAG and the weigh walk below provably terminating. - diags := refCycles(srcIndex, docRoot) + // The anchor-cycle answer is read first: a recursive anchor makes + // expandedWeight infinite, and having already refused those is what makes + // the alias graph a DAG and the weigh walk below provably terminating. + root := idx.Root() + diags := refCycles(srcIndex, root) if diag.HasError(diags) { return diags } @@ -143,43 +132,12 @@ func scanNode(srcIndex int, docRoot *yaml.Node) []ir.Diagnostic { // Appending (rather than replacing) preserves any diag.CycleScanFailed // warning refCycles already produced, so a document that both truncates a // merge chain and amplifies reports both findings. - if d, ok := aliasAmplification(srcIndex, docRoot); ok { + if d, ok := aliasAmplification(srcIndex, root, idx.Nodes()); ok { return append(diags, d) } return diags } -// anchorCycle reports the first alias whose resolved target is one of its own -// ancestors — a recursive YAML anchor that expands without bound. Legal anchor -// reuse (an alias to a node that is not an ancestor) is left untouched. -func anchorCycle(srcIndex int, root *yaml.Node) (ir.Diagnostic, bool) { - return walkAnchors(srcIndex, root, map[*yaml.Node]bool{}, 0) -} - -// walkAnchors descends the node tree tracking the ancestor path; an alias -// pointing back into that path is a recursive anchor. It deliberately never -// resolves alias edges — doing so would destroy the very signal it detects, -// and keeps the walk bounded by the tree alone. -func walkAnchors(srcIndex int, n *yaml.Node, path map[*yaml.Node]bool, depth int) (ir.Diagnostic, bool) { - if n == nil || depth > maxCycleDepth { - return ir.Diagnostic{}, false - } - if n.Kind == yaml.AliasNode { - if n.Alias != nil && path[n.Alias] { - return cyclicDiag(srcIndex, n, "recursive YAML anchor %q references an ancestor node", anchorName(n)), true - } - return ir.Diagnostic{}, false - } - path[n] = true - for _, child := range n.Content { - if d, ok := walkAnchors(srcIndex, child, path, depth+1); ok { - return d, true - } - } - delete(path, n) - return ir.Diagnostic{}, false -} - // anchorName is the anchor label an alias points at, for the diagnostic message. func anchorName(alias *yaml.Node) string { if alias.Alias != nil && alias.Alias.Anchor != "" { @@ -640,12 +598,15 @@ const maxAliasSurplus = 1 << 18 // the one the post-order walk finishes first, and a useful place to point the // author. // -// Callers must run this only after anchorCycle has refused a recursive YAML -// anchor: that is what makes the alias graph a DAG and this walk's termination -// provable without a cap of its own. See scanCycles for the ordering, and +// raw is the document's own node count, which the source index already +// established; this walk weighs the expansion against it rather than re-deriving +// it. +// +// Callers must run this only after a recursive YAML anchor has been refused: +// that is what makes the alias graph a DAG and this walk's termination provable +// without a cap of its own. See scanIndex for the ordering, and // aliasWeigher.pushChildren for the defensive guard kept anyway. -func aliasAmplification(srcIndex int, root *yaml.Node) (ir.Diagnostic, bool) { - raw := rawNodeCount(root) +func aliasAmplification(srcIndex int, root *yaml.Node, raw int64) (ir.Diagnostic, bool) { allowance := computeAllowance(raw) culprit, exceeded := newAliasWeigher(allowance).weigh(root) @@ -673,29 +634,6 @@ func computeAllowance(raw int64) int64 { return ratioAllowance } -// rawNodeCount returns the number of nodes in the parsed tree rooted at n, -// before any alias is substituted. An alias node counts as one and is never -// descended into: yaml.v3 gives alias nodes empty Content, so no special -// casing is needed to keep one from being read as a copy of its target. -// -// The raw parse tree is a tree, not a graph — aliasing only adds edges this -// walk never follows — so the iterative stack visits each node exactly once -// and is bounded by the tree's own size. -func rawNodeCount(root *yaml.Node) int64 { - if root == nil { - return 0 - } - var count int64 - stack := []*yaml.Node{root} - for len(stack) > 0 { - n := stack[len(stack)-1] - stack = stack[:len(stack)-1] - count++ - stack = append(stack, n.Content...) - } - return count -} - // aliasAmplificationDiag builds a diag.AliasAmplification error diagnostic // anchored at the node whose expansion first crossed allowance, following // cyclicDiag's line:col provenance convention. The reported node count is a diff --git a/compilers/openapi/internal/scan/scan_internal_test.go b/compilers/openapi/internal/scan/scan_internal_test.go index 04fa3f4a..2533079d 100644 --- a/compilers/openapi/internal/scan/scan_internal_test.go +++ b/compilers/openapi/internal/scan/scan_internal_test.go @@ -14,6 +14,7 @@ import ( "github.com/dexpace/morphic/compilers/openapi/internal/diag" "github.com/dexpace/morphic/compilers/openapi/internal/nodeview" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" "github.com/dexpace/morphic/ir" ) @@ -57,7 +58,7 @@ func TestDetectCycles_Reproducers(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() data := readReproducer(t, tc.file) - diags := Cycles(0, data) + diags := scanBytes(t, data) require.NotEmpty(t, diags, "degenerate cycle must be diagnosed") assert.Equal(t, diag.CyclicRef, diags[0].Code) assert.Equal(t, ir.SeverityError, diags[0].Severity) @@ -70,7 +71,7 @@ func TestDetectCycles_LegalRecursionClean(t *testing.T) { t.Parallel() data, err := os.ReadFile("../../../../testdata/conformance/openapi/recursive.yaml") require.NoError(t, err) - assert.Empty(t, Cycles(0, data), "legal recursion is not a degenerate cycle") + assert.Empty(t, scanBytes(t, data), "legal recursion is not a degenerate cycle") } var refShapedDataSpecs = []struct { @@ -207,16 +208,43 @@ func TestDetectCycles_RefShapedDataIsClean(t *testing.T) { for _, tc := range refShapedDataSpecs { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Empty(t, Cycles(0, []byte(tc.data)), + assert.Empty(t, scanBytes(t, []byte(tc.data)), "a ref-shaped structure outside a schema position is not a degenerate cycle") }) } } -func TestDetectCycles_NonYAMLIsNoCycle(t *testing.T) { +// TestDetectCycles_EmptyDocumentIsNoCycle covers the index a source with nothing +// in it produces. A source that does not decode at all no longer reaches here — +// load refuses it as a parse error before it indexes anything — so the empty +// document is the only sourceless index the scan can be handed. +func TestDetectCycles_EmptyDocumentIsNoCycle(t *testing.T) { t.Parallel() - assert.Empty(t, Cycles(0, nil)) - assert.Empty(t, Cycles(0, []byte("\t\x00: ["))) + assert.Empty(t, Cycles(0, sourceindex.Build(nil, sourceindex.MaxIndexedNodes))) + assert.Empty(t, scanBytes(t, nil)) +} + +// scanBytes decodes source bytes and runs the refusals over the index built from +// them — what load does around Cycles, with the compile's one decode. +func scanBytes(t *testing.T, data []byte) []ir.Diagnostic { + t.Helper() + return Cycles(0, indexOf(t, data)) +} + +// indexOf decodes source bytes and indexes the tree. A fixture that does not +// decode never reaches the scan in a real compile — load returns a parse error +// first — so a decode failure here is a broken fixture, not a case to scan. +func indexOf(t *testing.T, data []byte) sourceindex.Index { + t.Helper() + var root yaml.Node + require.NoError(t, yaml.Unmarshal(data, &root), "the fixture must decode") + return sourceindex.Build(&root, sourceindex.MaxIndexedNodes) +} + +// rawNodes is a tree's own node count, before any alias is substituted, as the +// source index derives it. +func rawNodes(n *yaml.Node) int64 { + return sourceindex.Build(n, sourceindex.MaxIndexedNodes).Nodes() } func readReproducer(t *testing.T, file string) []byte { @@ -268,20 +296,14 @@ func TestRecoverCycleScan_PassesThroughResult(t *testing.T) { func TestDetectCycles_WhitespaceOnlyIsNoCycle(t *testing.T) { t.Parallel() - assert.Empty(t, Cycles(0, []byte("\n\n\n"))) - assert.Empty(t, Cycles(0, []byte("# only a comment\n"))) -} - -func TestWalkAnchors_NilNode(t *testing.T) { - t.Parallel() - _, ok := walkAnchors(0, nil, map[*yaml.Node]bool{}, 0) - assert.False(t, ok) + assert.Empty(t, scanBytes(t, []byte("\n\n\n"))) + assert.Empty(t, scanBytes(t, []byte("# only a comment\n"))) } func TestDetectCycles_LegalAliasReuseClean(t *testing.T) { t.Parallel() src := "a: &x {p: 1}\nb: *x\n" - assert.Empty(t, Cycles(0, []byte(src)), + assert.Empty(t, scanBytes(t, []byte(src)), "an alias to a non-ancestor anchor is legal reuse") } @@ -306,8 +328,8 @@ func TestDetectCycles_MalformedSchemaShapes(t *testing.T) { "components:\n schemas: [1, 2]\n" allOfNotSeq := "openapi: 3.1.0\ninfo: {title: t, version: '1'}\npaths: {}\n" + "components:\n schemas:\n A:\n allOf: {x: 1}\n" - assert.Empty(t, Cycles(0, []byte(schemasNotMap)), "schemas as a sequence is not a schema map") - assert.Empty(t, Cycles(0, []byte(allOfNotSeq)), "allOf as a mapping is not a schema list") + assert.Empty(t, scanBytes(t, []byte(schemasNotMap)), "schemas as a sequence is not a schema map") + assert.Empty(t, scanBytes(t, []byte(allOfNotSeq)), "allOf as a mapping is not a schema list") } func TestFollowRefChain_DepthCapReturnsFalse(t *testing.T) { @@ -539,9 +561,9 @@ paths: {} x-anchors: {k: &k '<<', base: &base {$ref: '#/components/schemas/A'}} components: {schemas: {A: {*k : *base}}} ` - assert.Empty(t, Cycles(0, []byte(quoted)), + assert.Empty(t, scanBytes(t, []byte(quoted)), "a quoted '<<' is a plain key to speakeasy, not a merge") - assert.Empty(t, Cycles(0, []byte(aliasedKey)), + assert.Empty(t, scanBytes(t, []byte(aliasedKey)), "an alias standing in for the key is a plain key to speakeasy, not a merge") } @@ -582,7 +604,7 @@ func TestDetectCycles_TruncationDoesNotDisableTheRestOfTheScan(t *testing.T) { b.WriteString(" A: {$ref: '#/components/schemas/B'}\n") b.WriteString(" B: {$ref: '#/components/schemas/A'}\n") - diags := Cycles(0, []byte(b.String())) + diags := scanBytes(t, []byte(b.String())) require.NotEmpty(t, diags) assert.Equal(t, diag.CyclicRef, diags[0].Code, "a cycle outside the truncated chain is still found, and outranks the warning") @@ -665,9 +687,12 @@ func TestDetectCycles_MergeChainPastBoundStaysFastAndWarns(t *testing.T) { func scanWithin(t *testing.T, src, blowup string) []ir.Diagnostic { t.Helper() const bound = 10 * time.Second + // Indexed on this goroutine: the walk is linear in the tree, and require + // must not be called from the one below. + idx := indexOf(t, []byte(src)) done := make(chan []ir.Diagnostic, 1) go func() { - done <- Cycles(0, []byte(src)) + done <- Cycles(0, idx) }() select { case diags := <-done: @@ -722,7 +747,7 @@ info: {title: t, version: '1'} paths: /a: {$ref: '#/paths/~1a/t'} ` - diags := Cycles(0, []byte(src)) + diags := scanBytes(t, []byte(src)) require.NotEmpty(t, diags, "a pointer that resolves through its own reference must be refused") assert.Equal(t, diag.CyclicRef, diags[0].Code) assert.Equal(t, ir.SeverityError, diags[0].Severity) @@ -747,41 +772,18 @@ func TestDetectCycles_PointerIsNormalizedLikeTheResolver(t *testing.T) { for name, ref := range refs { t.Run(name, func(t *testing.T) { t.Parallel() - diags := Cycles(0, []byte(head+ref+tail)) + diags := scanBytes(t, []byte(head+ref+tail)) require.NotEmpty(t, diags, "the resolver reads this pointer as naming /a") assert.Equal(t, diag.CyclicRef, diags[0].Code) }) } } -// TestCyclesInNode_FindsWhatCyclesFindsInTheSameBytes pins the node-taking entry -// against the byte-taking one. They must agree, because the only reason the -// second exists is a caller holding a tree the source bytes no longer describe — -// an overlay's — and a scan that classified a decoded tree differently would let -// exactly the documents it was added for through. -func TestCyclesInNode_FindsWhatCyclesFindsInTheSameBytes(t *testing.T) { +// TestDetectCycles_AcceptsATreeWithNoCycle is the control the refusal cases +// need: without it, a suite made only of refusals would pass on a scan that +// refused everything handed to it. +func TestDetectCycles_AcceptsATreeWithNoCycle(t *testing.T) { t.Parallel() - for _, tc := range cycleReproducers { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - data := readReproducer(t, tc.file) - - var root yaml.Node - require.NoError(t, yaml.Unmarshal(data, &root)) - - assert.Equal(t, Cycles(0, data), CyclesInNode(0, &root)) - }) - } -} - -// TestCyclesInNode_AcceptsATreeWithNoCycle is the control: without it, an -// agreement test over refusals alone would pass on an entry point that refused -// everything handed to it. -func TestCyclesInNode_AcceptsATreeWithNoCycle(t *testing.T) { - t.Parallel() - var root yaml.Node - require.NoError(t, yaml.Unmarshal([]byte( - "openapi: 3.1.0\ncomponents: {schemas: {A: {$ref: '#/components/schemas/B'}, B: {type: string}}}\n"), &root)) - - assert.Empty(t, CyclesInNode(0, &root)) + assert.Empty(t, scanBytes(t, []byte( + "openapi: 3.1.0\ncomponents: {schemas: {A: {$ref: '#/components/schemas/B'}, B: {type: string}}}\n"))) } diff --git a/compilers/openapi/internal/sourceindex/sourceindex.go b/compilers/openapi/internal/sourceindex/sourceindex.go new file mode 100644 index 00000000..a0210548 --- /dev/null +++ b/compilers/openapi/internal/sourceindex/sourceindex.go @@ -0,0 +1,168 @@ +// Package sourceindex answers, in one walk, the questions asked of a decoded +// source tree before any of it is lowered. +// +// The questions are small and unrelated to each other — how many nodes did the +// document declare, and does any alias point back at one of its own ancestors — +// but each was answered by a whole traversal of its own, so a compile walked the +// same tree once per question. They share a walk here instead, and the answers +// become a value the caller carries rather than a walk the caller repeats. +// +// The index is a value with no exported fields and no maps, so a copy is +// independent of the original and nothing that receives one can write through it +// to a holder. It is derived from the tree alone: two indexes built over the same +// tree are equal, whatever order their answers are read in. +package sourceindex + +import ( + yaml "gopkg.in/yaml.v3" + + "github.com/dexpace/morphic/compilers/openapi/internal/nodeview" +) + +// MaxIndexedNodes bounds how many nodes one index walks. +// +// It is not a memory guard: the tree is already materialized by the decode that +// produced it, and the index itself holds one counter and one node pointer +// whatever the tree's size. It is the explicit limit the bounded-everything rule +// requires of every loop, placed far above any document that could have been +// decoded in the first place — the largest spec in this repository's corpora +// parses to fewer than 5,000 nodes, and the largest public OpenAPI documents to +// a few million. A document past it is refused by the caller rather than +// half-counted, because a truncated count would understate the alias-expansion +// allowance derived from it and could refuse a document on a bound it never +// crossed. +const MaxIndexedNodes = 1 << 24 // 16,777,216 + +// maxTrackedDepth bounds how deep the ancestor path is tracked, and so how deep +// a recursive anchor is still recognized. It is this package's own bound on its +// own walk, deliberately equal to the one the recursive anchor descent it +// replaces carried, so a document the old walk stopped tracking at is the same +// document this one stops tracking at. Nodes below it are still counted; they +// are simply not candidates for an anchor cycle, exactly as before. +const maxTrackedDepth = 10000 + +// Index is what one walk over a decoded source tree found. +// +// A zero Index is the answer for a document with no content: no root, no nodes, +// no anchor cycle, nothing truncated. That is the same answer Build returns for +// an empty document, so a caller never has to tell the two apart. +type Index struct { + root *yaml.Node + nodes int64 + anchorCycle *yaml.Node + truncated bool +} + +// Build walks the tree under root once and returns what it found. root may be a +// document node or the content node itself; either way the index is rooted at +// the content, which is the node every consumer scans from. +// +// maxNodes is the caller's bound on the walk — MaxIndexedNodes in the compiler, +// smaller in a test that drives the truncated path. A non-positive bound indexes +// nothing and reports Truncated, which is the safe direction: it never claims a +// count it did not reach. +func Build(root *yaml.Node, maxNodes int64) Index { + content := nodeview.DocumentRoot(root) + if content == nil { + return Index{} + } + if maxNodes <= 0 { + return Index{root: content, truncated: true} + } + + idx := Index{root: content} + idx.walk(maxNodes) + return idx +} + +// Root is the node the index was built over — the content of a document node, +// or the node itself — or nil for an empty document. +func (x Index) Root() *yaml.Node { return x.root } + +// Nodes is how many nodes the tree declares, before any alias is substituted. An +// alias counts as one and its target is not counted again through it, which is +// what makes this the document's own size rather than its expansion. It is +// meaningful only when Truncated is false. +func (x Index) Nodes() int64 { return x.nodes } + +// AnchorCycle is the first alias, in document order, whose target is one of its +// own ancestors — a recursive YAML anchor that expands without bound. Legal +// anchor reuse, where an alias names a node that is not an ancestor, is not one. +func (x Index) AnchorCycle() (*yaml.Node, bool) { + return x.anchorCycle, x.anchorCycle != nil +} + +// Truncated reports whether the walk stopped at its node bound. Every other +// answer is then a partial one and must not be read as a fact about the +// document. +func (x Index) Truncated() bool { return x.truncated } + +// frame is one entry of the walk's explicit stack: a node to visit at a known +// depth, or the ancestor-path exit of a node whose children have all been +// visited. +type frame struct { + n *yaml.Node + depth int + exit bool +} + +// walk visits every node reachable through Content exactly once, in the +// depth-first document order a recursive descent would take, counting as it goes +// and recording the first alias that points back into the path it arrived by. +// +// The raw parse tree is a tree, not a graph: aliasing only adds edges this walk +// never follows, so no node is reached twice and the ancestor set is exactly the +// path from the root. That is what lets the walk be iterative and bounded by +// maxNodes rather than by a recursion cap. +// +// It does not stop at the first anchor cycle. A consumer that refuses the +// document on one never reads the count, but an index that answered one question +// only until another was answered would depend on which was asked first. +func (x *Index) walk(maxNodes int64) { + ancestors := map[*yaml.Node]bool{} + stack := []frame{{n: x.root}} + + for len(stack) > 0 { + f := stack[len(stack)-1] + stack = stack[:len(stack)-1] + + if f.exit { + delete(ancestors, f.n) + continue + } + if f.n == nil { + continue // never produced by a parse; not counted rather than dereferenced + } + + x.nodes++ + if x.nodes > maxNodes { + x.truncated = true + return + } + + tracked := f.depth <= maxTrackedDepth + if tracked && f.n.Kind == yaml.AliasNode { + x.recordAlias(f.n, ancestors) + continue // yaml.v3 gives an alias empty Content; its target is not its child + } + if tracked && len(f.n.Content) > 0 { + ancestors[f.n] = true + stack = append(stack, frame{n: f.n, exit: true}) + } + for i := len(f.n.Content) - 1; i >= 0; i-- { + stack = append(stack, frame{n: f.n.Content[i], depth: f.depth + 1}) + } + } +} + +// recordAlias keeps the first alias whose target is on the current path. Later +// ones are dropped rather than overwriting it, so the reported node is the one a +// depth-first descent would have stopped at. +func (x *Index) recordAlias(alias *yaml.Node, ancestors map[*yaml.Node]bool) { + if x.anchorCycle != nil || alias.Alias == nil { + return + } + if ancestors[alias.Alias] { + x.anchorCycle = alias + } +} diff --git a/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go b/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go new file mode 100644 index 00000000..dc780769 --- /dev/null +++ b/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go @@ -0,0 +1,253 @@ +package sourceindex + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v3" +) + +func yscalar(v string) *yaml.Node { + return &yaml.Node{Kind: yaml.ScalarNode, Value: v} +} + +func ymap(pairs ...*yaml.Node) *yaml.Node { + return &yaml.Node{Kind: yaml.MappingNode, Content: pairs} +} + +func yseq(items ...*yaml.Node) *yaml.Node { + return &yaml.Node{Kind: yaml.SequenceNode, Content: items} +} + +func yalias(target *yaml.Node) *yaml.Node { + return &yaml.Node{Kind: yaml.AliasNode, Alias: target} +} + +// decode is the one place these tests parse source text, so a fixture written as +// YAML is indexed over exactly the tree a compile would index. +func decode(t *testing.T, src string) *yaml.Node { + t.Helper() + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(src), &root)) + return &root +} + +func TestBuild_EmptyDocumentIndexesNothing(t *testing.T) { + t.Parallel() + for name, root := range map[string]*yaml.Node{ + "no node at all": nil, + "a document with no content": {Kind: yaml.DocumentNode}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + idx := Build(root, MaxIndexedNodes) + assert.Nil(t, idx.Root(), "an empty document has no content node") + assert.Equal(t, int64(0), idx.Nodes()) + assert.False(t, idx.Truncated(), "nothing to index is fully indexed") + _, found := idx.AnchorCycle() + assert.False(t, found) + }) + } +} + +// TestBuild_WhitespaceOnlySourceIsOneEmptyNode records what yaml.v3 actually +// hands back for a source with no document in it: not a document node with no +// content, but a node of no kind at all. It indexes as the single node it is, +// which is why the scan reads such a source as carrying nothing rather than as +// having failed. +func TestBuild_WhitespaceOnlySourceIsOneEmptyNode(t *testing.T) { + t.Parallel() + idx := Build(decode(t, "\n\n\n"), MaxIndexedNodes) + + require.NotNil(t, idx.Root()) + assert.Equal(t, yaml.Kind(0), idx.Root().Kind, "yaml.v3 leaves the node untouched") + assert.Equal(t, int64(1), idx.Nodes()) + _, found := idx.AnchorCycle() + assert.False(t, found) +} + +func TestBuild_RootIsTheDocumentsContent(t *testing.T) { + t.Parallel() + content := ymap(yscalar("a"), yscalar("1")) + doc := &yaml.Node{Kind: yaml.DocumentNode, Content: []*yaml.Node{content}} + + assert.Same(t, content, Build(doc, MaxIndexedNodes).Root(), + "a document node is unwrapped to the node every consumer scans from") + assert.Same(t, content, Build(content, MaxIndexedNodes).Root(), + "a content node passed directly is already the root") +} + +func TestBuild_CountsEveryNodeOnce(t *testing.T) { + t.Parallel() + // root, "a", "1", "b", the sequence, "x", "y" = 7. + root := ymap( + yscalar("a"), yscalar("1"), + yscalar("b"), yseq(yscalar("x"), yscalar("y")), + ) + assert.Equal(t, int64(7), Build(root, MaxIndexedNodes).Nodes()) +} + +// TestBuild_AnAliasCountsOnceAndIsNotFollowed pins the count to the document's +// own size rather than its expansion. The expansion is what the amplification +// refusal measures against this number, so a count that followed alias edges +// would compare a document against itself. +func TestBuild_AnAliasCountsOnceAndIsNotFollowed(t *testing.T) { + t.Parallel() + base := ymap(yscalar("a"), yscalar("1")) // 3 nodes + root := ymap( + yscalar("base"), base, + yscalar("reuse"), yalias(base), + ) + // root, "base", base's 3, "reuse", the alias = 7. + assert.Equal(t, int64(7), Build(root, MaxIndexedNodes).Nodes(), + "the alias contributes one node, not a copy of its target") +} + +func TestBuild_NilChildIsSkipped(t *testing.T) { + t.Parallel() + root := ymap(yscalar("k"), yscalar("v")) + root.Content = append(root.Content, nil) + + idx := Build(root, MaxIndexedNodes) + assert.Equal(t, int64(3), idx.Nodes(), + "the nil child contributes nothing and costs no dereference") + assert.False(t, idx.Truncated()) +} + +func TestBuild_NonPositiveBoundIndexesNothing(t *testing.T) { + t.Parallel() + idx := Build(ymap(yscalar("a"), yscalar("1")), 0) + assert.True(t, idx.Truncated(), "a bound that admits no node admits no answer either") + assert.Equal(t, int64(0), idx.Nodes()) + assert.NotNil(t, idx.Root(), "the tree is still named, so a caller can say what it refused") +} + +// TestBuild_StopsAtItsNodeBound is what keeps the walk bounded by something +// other than the input. The count it stops at is deliberately not reported as a +// fact: Truncated is the answer to every other question. +func TestBuild_StopsAtItsNodeBound(t *testing.T) { + t.Parallel() + root := ymap(yscalar("a"), yscalar("1"), yscalar("b"), yscalar("2")) // 5 nodes + + assert.False(t, Build(root, 5).Truncated(), "a tree exactly at the bound is fully indexed") + + stopped := Build(root, 4) + assert.True(t, stopped.Truncated(), "one node past the bound stops the walk") + _, found := stopped.AnchorCycle() + assert.False(t, found, "a truncated walk reports no finding it did not reach") +} + +// TestBuild_TruncationDoesNotMisreportAnAnchorCycle guards the direction that +// matters: a walk that stopped early must not claim the document is clean in a +// way a caller could act on. It reports Truncated, and the caller refuses. +func TestBuild_TruncationDoesNotMisreportAnAnchorCycle(t *testing.T) { + t.Parallel() + root := ymap(yscalar("a"), yscalar("1")) + root.Content = append(root.Content, yscalar("b"), yalias(root)) + + full := Build(root, MaxIndexedNodes) + _, found := full.AnchorCycle() + require.True(t, found, "the fixture really does carry a recursive anchor") + + assert.True(t, Build(root, 2).Truncated(), + "the same tree under a bound it crosses reports truncation rather than cleanliness") +} + +func TestAnchorCycle_AliasToAnAncestorIsFound(t *testing.T) { + t.Parallel() + inner := ymap(yscalar("k"), yscalar("v")) + root := ymap(yscalar("outer"), inner) + alias := yalias(root) + inner.Content = append(inner.Content, yscalar("loop"), alias) + + got, found := Build(root, MaxIndexedNodes).AnchorCycle() + require.True(t, found, "an alias naming a node it is nested inside expands without bound") + assert.Same(t, alias, got, "the reported node is the alias, where the author wrote it") +} + +// TestAnchorCycle_LegalReuseIsNotACycle is the control. Anchor reuse is ordinary +// YAML, and a walk that called every alias a cycle would refuse most specs that +// use anchors at all. +func TestAnchorCycle_LegalReuseIsNotACycle(t *testing.T) { + t.Parallel() + root := decode(t, "a: &x {p: 1}\nb: *x\n") + + _, found := Build(root, MaxIndexedNodes).AnchorCycle() + assert.False(t, found, "an alias to a node that is not an ancestor is legal reuse") +} + +func TestAnchorCycle_AliasWithoutATargetIsNotACycle(t *testing.T) { + t.Parallel() + root := ymap(yscalar("k"), &yaml.Node{Kind: yaml.AliasNode}) + + idx := Build(root, MaxIndexedNodes) + _, found := idx.AnchorCycle() + assert.False(t, found, "an alias that names nothing cannot name an ancestor") + assert.Equal(t, int64(3), idx.Nodes(), "it is still one of the document's nodes") +} + +// TestAnchorCycle_FirstInDocumentOrderWins pins which of several recursive +// anchors is reported. The answer has to be the first a depth-first descent +// would reach, or the diagnostic a document draws would depend on the walk's +// shape rather than on what the document says. +func TestAnchorCycle_FirstInDocumentOrderWins(t *testing.T) { + t.Parallel() + first, second := ymap(), ymap() + root := ymap(yscalar("a"), first, yscalar("b"), second) + firstAlias, secondAlias := yalias(root), yalias(root) + first.Content = append(first.Content, yscalar("loop"), firstAlias) + second.Content = append(second.Content, yscalar("loop"), secondAlias) + + got, found := Build(root, MaxIndexedNodes).AnchorCycle() + require.True(t, found) + assert.Same(t, firstAlias, got, "the earlier alias is the one reported") +} + +// TestBuild_TracksAncestorsOnlyToItsDepthBound pins the one thing maxTrackedDepth +// decides. Below it a node is still the document's — it is counted, and its +// children are walked — but it is no longer a candidate for an anchor cycle, +// which is exactly where the bounded recursive descent this walk replaced +// stopped looking. +func TestBuild_TracksAncestorsOnlyToItsDepthBound(t *testing.T) { + t.Parallel() + // A chain of single-entry mappings: root at depth 0, and each mapping's + // value two levels below its parent's. + root := ymap() + deepest := root + const links = maxTrackedDepth + for range links { + next := ymap() + deepest.Content = append(deepest.Content, yscalar("k"), next) + deepest = next + } + deepest.Content = append(deepest.Content, yscalar("loop"), yalias(root)) + + idx := Build(root, MaxIndexedNodes) + assert.False(t, idx.Truncated()) + // root, then two nodes per link (its key and the mapping it names), then the + // deepest mapping's own key and alias. + assert.Equal(t, int64(2*links+3), idx.Nodes(), + "every node below the tracked depth is still counted") + _, found := idx.AnchorCycle() + assert.False(t, found, + "an alias deeper than the tracked depth is out of the walk's reach, as it was before") +} + +// TestBuild_IsAFunctionOfTheTreeAlone is the determinism the compiler's output +// rests on: the same tree indexed twice answers identically, so nothing +// downstream can vary with when or how often the index was built. +func TestBuild_IsAFunctionOfTheTreeAlone(t *testing.T) { + t.Parallel() + root := decode(t, "a: &x {p: 1}\nb: [*x, {c: 2}]\n") + + first, second := Build(root, MaxIndexedNodes), Build(root, MaxIndexedNodes) + assert.Same(t, first.Root(), second.Root()) + assert.Equal(t, first.Nodes(), second.Nodes()) + assert.Equal(t, first.Truncated(), second.Truncated()) + + firstCycle, firstFound := first.AnchorCycle() + secondCycle, secondFound := second.AnchorCycle() + assert.Equal(t, firstFound, secondFound) + assert.Same(t, firstCycle, secondCycle) +} diff --git a/docs/micro-compiler-design.md b/docs/micro-compiler-design.md index 0400569b..3affcba7 100644 --- a/docs/micro-compiler-design.md +++ b/docs/micro-compiler-design.md @@ -684,12 +684,16 @@ sorts, which `maps.Keys` + `slices.Sorted` and the existing generic `sortedKeys` creates lives under `internal/`, so none of it is reachable from outside the compiler. The IR is the ABI (invariant 1); a restructuring that altered the compiler's own surface would be a second, unrelated change. -- **The source index.** Indexing the raw tree once (pointer → node + shape) would make resolution a - lookup, share one walk across cycle and amplification detection, and give `--explain` a substrate. - It is held back because `$ref` handling still carries an open defect — #40, percent-encoded - fragments failing to resolve — and an index built over it would bake it in. #143 (siblings - adjacent to a `$ref` on an allOf branch dropped) and #141 (an `$anchor` fragment derived from as - though it were a pointer) are closed. Filed as a follow-up blocked on the rest closing. +- **A pointer-keyed source index.** The `$ref` defects this was held back on — #40 (percent-encoded + fragments failing to resolve), #141 (an `$anchor` fragment derived from as though it were a + pointer) and #143 (siblings adjacent to a `$ref` on an allOf branch dropped) — are all closed, and + the walk-sharing half has since landed: `internal/sourceindex` walks the decoded tree once and + answers what the cycle and amplification refusals each used to walk it to ask, over the single + decode the loader now performs. What is still out of scope is the other half — a pointer → node + map that would make reference resolution a lookup, a per-node key index that would make + `annotation.RawChildNode` one, and the `--explain` substrate both would give. Each of those is + read during lowering rather than before it, so handing them down means widening the lowering + context and the signatures beneath it: its own change. - **Rebasing the GraphQL and Protobuf drafts.** They are evidence here, not work items. - **A new-compiler skeleton demo.** @@ -746,6 +750,6 @@ landing them first would only encode the current one. | #83 enforce size and complexity caps in lint | **Closed by 4.2**, deliberately last | | #66 extract a shared JSON-Schema→IR lowering core before the next compilers land | **Superseded.** Its premise expired — the next compilers landed without it (#20, #21). §3 replaces it with evidence-based promotion. To be closed with that reasoning, not silently | | #142 the annotation matrix cannot reach a carrier position | **Closed**, independently of this work as §8.4 said it could be: the grid gained a kind per carrier, and the two are separate kinds because their carriers hold different sets | -| #40, #141 `$ref` handling defects | **#141 closed**: a fragment that is not a JSON pointer is refused rather than derived from, and `irverify` now rejects an ID the grammar could not have produced. #40 still **blocks the source index** (§10). #143, listed here before, is closed | +| #40, #141 `$ref` handling defects | **Both closed**: a fragment that is not a JSON pointer is refused rather than derived from, `irverify` now rejects an ID the grammar could not have produced, and percent-encoded fragments resolve. With #143 they were what held the source index back; the walk-sharing half of it has since landed, and §10 records what remains out of scope | | #20, #21 GraphQL and Protobuf drafts | **Evidence, not work items** (§2). Rebasing is later work | | Naming grammar divergence across compilers | **Closed by #161**, filed and fixed separately: a live invariant-4 violation, independent of whether this architecture work proceeds | diff --git a/internal/archtest/arch_test.go b/internal/archtest/arch_test.go index bec94f78..e8eaa9fb 100644 --- a/internal/archtest/arch_test.go +++ b/internal/archtest/arch_test.go @@ -63,12 +63,19 @@ var rules = map[string][]string{ // unescaping one lookup needs, and is below both the scans that first wanted // it and the schema lowering that wants the same view. "compilers/openapi/internal/nodeview": {module + "/compilers/openapi/internal/ids", "gopkg.in/yaml.v3"}, - // The pre-lowering refusals. They read the source through nodeview and report - // through diag, and reach no part of the lowering — nothing here has a - // document to lower yet. + // One walk over the decoded source tree, answering what the pre-lowering + // refusals would otherwise each walk it to ask. It reaches nodeview for the + // document root and nothing else: an index of what the source says is not + // allowed to depend on what any consumer of it wants to say about that. + "compilers/openapi/internal/sourceindex": { + module + "/compilers/openapi/internal/nodeview", "gopkg.in/yaml.v3"}, + // The pre-lowering refusals. They read the source through nodeview and the + // index built over it, report through diag, and reach no part of the + // lowering — nothing here has a document to lower yet. "compilers/openapi/internal/scan": {module + "/ir", module + "/compilers/openapi/internal/diag", - module + "/compilers/openapi/internal/nodeview", "gopkg.in/yaml.v3"}, + module + "/compilers/openapi/internal/nodeview", + module + "/compilers/openapi/internal/sourceindex", "gopkg.in/yaml.v3"}, // What a schema or a carrier says about itself rather than about its shape, // plus the validation-only keywords the IR keeps verbatim. It reads the // parsed model and the raw nodes behind it, and holds no opinion about @@ -90,7 +97,8 @@ var rules = map[string][]string{ module + "/compilers/openapi/internal/ids", module + "/compilers/openapi/internal/nodeview", "github.com/speakeasy-api/openapi/overlay", "gopkg.in/yaml.v3"}, - // The entry side: parse, validate, resolve. It runs the pre-lowering refusals + // The entry side: parse, validate, resolve. It indexes the tree its one decode + // produced through sourceindex, runs the pre-lowering refusals over that index // through scan, applies the caller's overlay through overlay, and reads value // only to tell a real numeric-literal problem from a library artifact. It // reaches nothing that lowers — at this point there is no document to lower. @@ -98,6 +106,7 @@ var rules = map[string][]string{ module + "/compilers/openapi/internal/diag", module + "/compilers/openapi/internal/overlay", module + "/compilers/openapi/internal/scan", + module + "/compilers/openapi/internal/sourceindex", module + "/compilers/openapi/internal/value", "github.com/speakeasy-api/openapi/jsonschema/oas3", "github.com/speakeasy-api/openapi/marshaller", From 0644e3f5306636dc331ddd8a3fe46ca653c9bb55 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 15:49:30 +0300 Subject: [PATCH 2/3] refactor(compilers/openapi): carry the index seam in load options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-parse index was built through a package-level function variable so a test could shrink its node bound and count its calls. That is mutable package-level state in a pipeline stage, which the compiler has nowhere else, and it forced the three tests that used it to run sequentially: rebinding a value the parallel tests around them also read would otherwise race. Carry the builder as an unexported field of load.Options instead. The bound becomes an input to the stage like every other option, a test's choice is visible only to the load it passes it to, and the three tests are now parallel. Loads that leave it nil — everything outside this package's tests — index under the compiler's own bound exactly as before. --- .../internal/load/entry_internal_test.go | 41 +++++++++---------- compilers/openapi/internal/load/load.go | 30 ++++++++++---- 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/compilers/openapi/internal/load/entry_internal_test.go b/compilers/openapi/internal/load/entry_internal_test.go index 6e25ed94..4616514b 100644 --- a/compilers/openapi/internal/load/entry_internal_test.go +++ b/compilers/openapi/internal/load/entry_internal_test.go @@ -309,14 +309,14 @@ func TestLoad_RejectsAnOverlaySharingTheSourceIndex(t *testing.T) { // The bound is reached by shrinking it rather than by building a document of // sourceindex.MaxIndexedNodes nodes — that would be several gigabytes of // fixture, and the part worth testing is what the loader does with a truncated -// index, not that the walk stops at a number. It is not parallel, because it -// rebinds a package-level value the parallel tests around it also read. +// index, not that the walk stops at a number. func TestLoad_ADocumentTooLargeToIndexIsRefused(t *testing.T) { - orig := buildIndex - t.Cleanup(func() { buildIndex = orig }) - buildIndex = func(root *yaml.Node) sourceindex.Index { return sourceindex.Build(root, 1) } + t.Parallel() + opts := Options{buildIndex: func(root *yaml.Node) sourceindex.Index { + return sourceindex.Build(root, 1) + }} - doc, diags, err := Load(t.Context(), 4, openapitest.SourceOf(minimal31), Options{}) + doc, diags, err := Load(t.Context(), 4, openapitest.SourceOf(minimal31), opts) require.NoError(t, err, "an oversized document is a spec problem, not a Go error") assert.Nil(t, doc, "nothing is lowered from a document the pre-parse scan cannot cover") @@ -338,19 +338,16 @@ func TestLoad_TheSameDocumentLoadsOnceItFitsTheIndex(t *testing.T) { assert.False(t, diag.HasError(diags), "unexpected refusal: %+v", diags) } -// countingIndexBuilder makes the loader count its index builds for one test, -// and returns the counter. It is not parallel-safe, for the reason -// TestLoad_ADocumentTooLargeToIndexIsRefused is not. -func countingIndexBuilder(t *testing.T) *int { - t.Helper() - orig := buildIndex - t.Cleanup(func() { buildIndex = orig }) +// countingIndexBuilder returns base with an index builder that counts its calls, +// and the counter it writes. The count is one load's own, so tests using it stay +// independent of each other and of anything running beside them. +func countingIndexBuilder(base Options) (Options, *int) { built := 0 - buildIndex = func(root *yaml.Node) sourceindex.Index { + base.buildIndex = func(root *yaml.Node) sourceindex.Index { built++ - return orig(root) + return defaultIndex(root) } - return &built + return base, &built } // TestLoad_IndexesTheSourceOnce guards the shape of this path rather than its @@ -359,9 +356,10 @@ func countingIndexBuilder(t *testing.T) *int { // would break no assertion about what the compiler reports — the refusals would // still be right — so the count is the only thing that can hold it. func TestLoad_IndexesTheSourceOnce(t *testing.T) { - built := countingIndexBuilder(t) + t.Parallel() + opts, built := countingIndexBuilder(Options{}) - got, diags, err := Load(t.Context(), 0, openapitest.SourceOf(minimal31), Options{}) + got, diags, err := Load(t.Context(), 0, openapitest.SourceOf(minimal31), opts) require.NoError(t, err) require.NotNil(t, got) @@ -373,11 +371,12 @@ func TestLoad_IndexesTheSourceOnce(t *testing.T) { // overlay leaves behind a tree the first one no longer describes, and what the // refusals answer for is the tree the parser is handed. func TestLoad_IndexesAPatchedTreeAgain(t *testing.T) { - built := countingIndexBuilder(t) - - got, diags, err := Load(t.Context(), 0, openapitest.SourceOf(minimal31), + t.Parallel() + opts, built := countingIndexBuilder( overlayOptions(" - target: $.info\n update: {description: d}\n")) + got, diags, err := Load(t.Context(), 0, openapitest.SourceOf(minimal31), opts) + require.NoError(t, err) require.NotNil(t, got) assert.False(t, diag.HasError(diags), "unexpected refusal: %+v", diags) diff --git a/compilers/openapi/internal/load/load.go b/compilers/openapi/internal/load/load.go index 45f261e3..f8cd6b09 100644 --- a/compilers/openapi/internal/load/load.go +++ b/compilers/openapi/internal/load/load.go @@ -50,6 +50,14 @@ type Options struct { // OverlaySrcIndex is the index the overlay document takes in Document.Sources. // It is read only when Overlay is set. OverlaySrcIndex int + // buildIndex builds the pre-parse index over a decoded tree, or nil for the + // compiler's own node bound. It is unexported because it is this package's + // test seam: it drives the truncated-index refusal without materializing a + // document of sourceindex.MaxIndexedNodes nodes, and counts the indexes one + // load builds. Carrying it here rather than in a package-level variable keeps + // the bound an input to the stage, so nothing a test does to it is visible to + // a concurrent load. + buildIndex func(root *yaml.Node) sourceindex.Index } // errParse marks a hard failure to parse a source document — an I/O- or @@ -96,7 +104,7 @@ func Load(ctx context.Context, srcIndex int, src compilers.Source, opts Options) return nil, nil, fmt.Errorf("openapi: decode source %d: %w", srcIndex, err) } - cyc := refusals(srcIndex, root) + cyc := refusals(srcIndex, root, opts) if diag.HasError(cyc) { return nil, cyc, nil // degenerate cycle: refuse to lower, do not crash the parser } @@ -150,11 +158,10 @@ func Load(ctx context.Context, srcIndex int, src compilers.Source, opts Options) }, diags, nil } -// buildIndex indexes a decoded tree under the compiler's node bound. It is a -// package-level function value only so a test can drive the truncated-index -// refusal without materializing a document of sourceindex.MaxIndexedNodes nodes; -// nothing in the pipeline rebinds it, so the stages stay pure and reentrant. -var buildIndex = func(root *yaml.Node) sourceindex.Index { +// defaultIndex indexes a decoded tree under the compiler's node bound. It is +// what Options.buildIndex stands in for when a caller leaves it nil, which +// everything outside this package's tests does. +func defaultIndex(root *yaml.Node) sourceindex.Index { return sourceindex.Build(root, sourceindex.MaxIndexedNodes) } @@ -167,8 +174,13 @@ var buildIndex = func(root *yaml.Node) sourceindex.Index { // from a partial node count would refuse documents on a bound they never // crossed. A document that large is beyond what the pre-parse guarantees cover, // so it is refused rather than lowered on incomplete information. -func refusals(srcIndex int, root *yaml.Node) []ir.Diagnostic { - idx := buildIndex(root) +func refusals(srcIndex int, root *yaml.Node, opts Options) []ir.Diagnostic { + build := opts.buildIndex + if build == nil { + build = defaultIndex + } + + idx := build(root) if idx.Truncated() { return []ir.Diagnostic{diag.Newf(ir.SeverityError, diag.SourceTooLarge, ir.Provenance{Source: srcIndex}, @@ -193,7 +205,7 @@ func patch(srcIndex int, root *yaml.Node, opts Options) (overlay.Origin, []ir.Di if diag.HasError(diags) { return overlay.Origin{}, diags } - return origin, append(diags, refusals(srcIndex, root)...) + return origin, append(diags, refusals(srcIndex, root, opts)...) } // metaSchemaReconciledMinor is the OpenAPI minor whose schema findings are From 5176b0a5ab03c1f7a523091653aac46fe6b6fbc4 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 15:49:30 +0300 Subject: [PATCH 3/3] test(compilers/openapi): pin the ancestor-tracking depth bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestBuild_TracksAncestorsOnlyToItsDepthBound places its alias one level past maxTrackedDepth, so it holds only that a node beyond the bound is out of the walk's reach. Narrowing the comparison to f.depth < maxTrackedDepth left the whole suite green: an alias past the bound is unreachable either way, and no fixture puts one at the bound itself. That off-by-one is a real regression against the recursive descent this walk replaced, which tracked to depth 10000 inclusive and would still have caught a recursive anchor there. Add the control that separates the two — an alias at exactly the bound is still found — so the bound is pinned from both sides. --- .../sourceindex/sourceindex_internal_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go b/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go index dc780769..78ad43a7 100644 --- a/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go +++ b/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go @@ -234,6 +234,32 @@ func TestBuild_TracksAncestorsOnlyToItsDepthBound(t *testing.T) { "an alias deeper than the tracked depth is out of the walk's reach, as it was before") } +// TestBuild_TracksAncestorsAtItsDepthBound is the control for the test above, +// and the half that pins where the bound falls. Without it the comparison could +// be off by one — tracking to maxTrackedDepth-1 — and every assertion above +// would still hold, because an alias past the bound is out of reach either way. +// An alias at exactly the bound is the case that separates them, and it is the +// case the recursive descent this walk replaced still caught. +func TestBuild_TracksAncestorsAtItsDepthBound(t *testing.T) { + t.Parallel() + root := ymap() + deepest := root + const links = maxTrackedDepth - 1 + for range links { + next := ymap() + deepest.Content = append(deepest.Content, yscalar("k"), next) + deepest = next + } + // One level shallower than the test above, so these land at the bound itself. + deepest.Content = append(deepest.Content, yscalar("loop"), yalias(root)) + + idx := Build(root, MaxIndexedNodes) + assert.False(t, idx.Truncated()) + cycle, found := idx.AnchorCycle() + require.True(t, found, "an alias at exactly the tracked depth is still a candidate") + assert.Same(t, root, cycle.Alias, "the cycle reported is the one back to the root") +} + // TestBuild_IsAFunctionOfTheTreeAlone is the determinism the compiler's output // rests on: the same tree indexed twice answers identically, so nothing // downstream can vary with when or how often the index was built.