diff --git a/compilers/openapi/cycles_test.go b/compilers/openapi/cycles_test.go index 10e12ce..f28ebff 100644 --- a/compilers/openapi/cycles_test.go +++ b/compilers/openapi/cycles_test.go @@ -8,11 +8,13 @@ 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/openapitest" "github.com/dexpace/morphic/compilers/openapi/internal/scan" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" "github.com/dexpace/morphic/ir" ) @@ -96,7 +98,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(), @@ -209,6 +211,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) +} + // TestCompile_SchemaEmptyPointerSegmentIsUnresolved pins where reading the // empty token changes a verdict rather than a hang. Reading '/A/' as stopping // at A made this shape a cycle; reading it as descending through A makes it diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index a9dd091..3e10899 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 c65156b..d4bdb9e 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 858e844..4616514 100644 --- a/compilers/openapi/internal/load/entry_internal_test.go +++ b/compilers/openapi/internal/load/entry_internal_test.go @@ -8,11 +8,14 @@ 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/openapitest" "github.com/dexpace/morphic/compilers/openapi/internal/overlay" + "github.com/dexpace/morphic/compilers/openapi/internal/sourceindex" + "github.com/dexpace/morphic/ir" ) // TestLoad_DegenerateCycleIsRefusedBeforeParsing pins the first gate in the @@ -298,3 +301,84 @@ 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. +func TestLoad_ADocumentTooLargeToIndexIsRefused(t *testing.T) { + 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), 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") + 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, openapitest.SourceOf(minimal31), Options{}) + + require.NoError(t, err) + require.NotNil(t, got) + assert.False(t, diag.HasError(diags), "unexpected refusal: %+v", diags) +} + +// 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 + base.buildIndex = func(root *yaml.Node) sourceindex.Index { + built++ + return defaultIndex(root) + } + return base, &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) { + t.Parallel() + opts, built := countingIndexBuilder(Options{}) + + 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) + 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) { + 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) + 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 c64f9de..f8cd6b0 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" ) @@ -49,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 @@ -90,17 +99,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, opts) + 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) { @@ -149,12 +158,44 @@ func Load(ctx context.Context, srcIndex int, src compilers.Source, opts Options) }, diags, nil } +// 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) +} + +// 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, 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}, + "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 { @@ -164,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, scan.CyclesInNode(srcIndex, root)...) + return origin, append(diags, refusals(srcIndex, root, opts)...) } // metaSchemaReconciledMinor is the OpenAPI minor whose schema findings are @@ -364,11 +405,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 da7d690..5f1398e 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 44661bd..ee2b1b1 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 b6df2ac..cc033a4 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" ) @@ -58,7 +59,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) @@ -71,7 +72,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 { @@ -208,16 +209,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 { @@ -269,20 +297,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") } @@ -307,8 +329,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) { @@ -540,9 +562,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") } @@ -583,7 +605,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") @@ -666,9 +688,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: @@ -723,7 +748,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) @@ -746,7 +771,7 @@ components: pathItems: A: {$ref: '#/components/pathItems/A/'} ` - 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) @@ -771,41 +796,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 0000000..a021054 --- /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 0000000..78ad43a --- /dev/null +++ b/compilers/openapi/internal/sourceindex/sourceindex_internal_test.go @@ -0,0 +1,279 @@ +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_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. +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 0400569..3affcba 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 20f359a..0179eea 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",