From 48e8a10c3adb685c4d4c97da09b3fe1114863ee1 Mon Sep 17 00:00:00 2001 From: Fuad Daoud Date: Thu, 10 Sep 2026 12:52:51 +0300 Subject: [PATCH] fix(compilers/openapi): detect without decoding the root mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Format detection decoded a document's whole root mapping into a two-field struct to read the `openapi` / `swagger` key. yaml.v3 compares every pair of a mapping's keys before it reads any of them, so a mapping repeating one key n times raises n(n-1)/2 errors and then abandons the mapping — the probe came back empty as well as expensive. A 32 KB source repeating one key 6,553 times produced 21,467,628 errors and a 1.2 GB diagnostic in 16.7 s; a 128 KB one did not finish in 150 s. Both were reported as unreadable, though the parser the compiler goes on to use reads them and reports the repeats itself, once each and sited. Detection now parses the document and reads the two keys off the tree, which is linear and answers the same for a mapping whose keys repeat as for one whose keys do not. The 32 KB case takes 0.048 s and prints 6,553 sited warnings; the 128 KB case takes 0.147 s. Separately, diag.OneLine now bounds what a foreign error contributes to a diagnostic message. That is the general form of the same defect — a message a library can make arbitrarily large — and it covers the two overlay callers as well, where the library's own decode is still slow but its complaint no longer reaches the terminal whole. The cut lands on a rune boundary, so a message never carries half a rune to a reader. Two rules the walk now has and the decoder could not, since it refused any mapping that repeated a key at all: a key written twice takes its last spelling, matching the parser that later records the dialect on ir.SourceInfo, so one document cannot get two answers; and a key written directly beats one merged in through `<<`. Deliberately out of scope: the merge chain is bounded at maxMergeDepth, where the decoder followed one as far as yaml's own alias limits, and detection still reports an unreadable version key only where declaresProbeKey sees it declared at column 0 — widening that guard would claim documents of formats that nest a key of the same name. Closes #443 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P --- compilers/openapi/detect.go | 191 ++++++++++++- compilers/openapi/detect_test.go | 285 +++++++++++++++++++ compilers/openapi/internal/diag/diag.go | 47 ++- compilers/openapi/internal/diag/diag_test.go | 76 ++++- internal/archtest/recursion_test.go | 4 + 5 files changed, 592 insertions(+), 11 deletions(-) diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index c54fc80d..3f5a463e 100644 --- a/compilers/openapi/detect.go +++ b/compilers/openapi/detect.go @@ -3,6 +3,7 @@ package openapi import ( "bytes" "encoding/json" + "fmt" yaml "gopkg.in/yaml.v3" @@ -27,12 +28,29 @@ const maxSniffBytes = 64 << 10 // which is the whole reason the byte cap alone does not answer the question. const maxSniffEntries = 512 +// maxMergeDepth bounds how far a root mapping's merge keys are followed. A `<<` +// value may be an alias to a mapping that merges another, and an anchor may name +// a mapping that reaches itself, so the chain is not bounded by the document. +// Detection reads two keys off the root, which a document that merges at all +// reaches in one step; eight leaves room for a written chain and none for a +// crafted one. +const maxMergeDepth = 8 + +// mergeTag is the tag YAML resolves `<<` to. The tag is read rather than the +// key's text, because a mapping may legitimately hold a key spelled "<<" that +// was quoted into a plain string and merges nothing. +const mergeTag = "!!merge" + // sniffProbe holds the two discriminating top-level keys. Which one is present // is the whole of the format question: an OpenAPI 3.x document declares // `openapi`, a Swagger 2.0 document declares `swagger`. +// +// It carries no struct tags: nothing decodes into it. Both readers — the flow +// one over a JSON token stream and the block one over a parsed tree — name the +// two keys themselves, in recordEntry and fieldFor. type sniffProbe struct { - OpenAPI string `yaml:"openapi"` - Swagger string `yaml:"swagger"` + OpenAPI string + Swagger string } // Detect implements compilers.Compiler. It reports the dialect src declares, @@ -169,14 +187,179 @@ func sniffWhole(data []byte) (sniffProbe, error) { // decodeYAML reads the probe keys from a complete YAML (or JSON, its subset) // document. +// +// The document is parsed and its root mapping read; it is never decoded into +// sniffProbe. That is the whole of the fix for a 32 KB source producing a 1.2 GB +// diagnostic: yaml.v3 compares every pair of a mapping's keys before it reads +// any of them, so a mapping repeating one key n times raises n(n-1)/2 errors — +// 21 million of them for the 6,553-line case — and then abandons the mapping, so +// the probe came back empty as well as expensive. Reading the two keys off the +// parsed tree is linear, and answers for a document whose keys repeat exactly as +// for one whose keys do not. The parser this compiler goes on to use reports +// those repeats itself, once each and sited, which is where a reader wants them. func decodeYAML(data []byte) (sniffProbe, error) { - var probe sniffProbe - if err := yaml.Unmarshal(data, &probe); err != nil { + var doc yaml.Node + if err := yaml.Unmarshal(data, &doc); err != nil { return sniffProbe{}, err } + + root := documentRoot(&doc) + switch { + case root == nil: + // A stream that carried no document declares no key, which is a decline + // and not a failure: empty bytes are no more this compiler's than + // anybody else's. + return sniffProbe{}, nil + case root.Kind != yaml.MappingNode: + return sniffProbe{}, fmt.Errorf("document root is %s, not a mapping", root.ShortTag()) + default: + return probeFromMapping(root, maxMergeDepth) + } +} + +// documentRoot returns the content node of a decoded stream's first document, or +// nil for a stream that carried none. Decoding into a yaml.Node yields the +// document node itself, and only the first: a multi-document stream is read to +// its first document here exactly as the compiler's own load reads it. +func documentRoot(doc *yaml.Node) *yaml.Node { + if doc.Kind != yaml.DocumentNode || len(doc.Content) != 1 { + return nil + } + return doc.Content[0] +} + +// probeFromMapping reads the probe keys off a root mapping, following its merge +// keys for a key the mapping does not write itself. +// +// A key written directly wins over one merged in, which is the precedence YAML +// gives a merge. A key written twice takes its last spelling, which is what the +// parser this compiler goes on to use takes: detection names the dialect that +// routes the source, load records the one it read, and a document must not get +// two answers. Neither rule could be had before, since the decoder this replaces +// refused any mapping that repeated a key at all. +// +// depth is the merge chain still allowed. It is the bound on this recursion, +// checked before every descent, and the recursion is otherwise over a parsed +// tree of finite size. +func probeFromMapping(root *yaml.Node, depth int) (sniffProbe, error) { + probe, merges, err := probeFromEntries(root) + if err != nil || depth <= 0 { + return probe, err + } + + for _, merge := range merges { + merged, err := probeFromMerge(merge, depth-1) + if err != nil { + return sniffProbe{}, err + } + probe.fillFrom(merged) + } return probe, nil } +// probeFromEntries reads a mapping's own entries, and returns the values of its +// merge keys separately for the caller to follow. A mapping may write more than +// one `<<`, and their order is the order they are answered in. +func probeFromEntries(root *yaml.Node) (sniffProbe, []*yaml.Node, error) { + var probe sniffProbe + var merges []*yaml.Node + + for i := 0; i+1 < len(root.Content); i += 2 { + key, value := root.Content[i], root.Content[i+1] + if key.Tag == mergeTag { + merges = append(merges, value) + continue + } + field := probe.fieldFor(key) + if field == nil { + continue + } + version, err := probeVersion(value) + if err != nil { + return sniffProbe{}, nil, err + } + *field = version + } + return probe, merges, nil +} + +// probeFromMerge reads the probe keys out of one `<<` value, which YAML admits +// as an alias to a mapping, a mapping written out, or a sequence of either. +// Anything else merges nothing, which is the source's problem to be reported by +// the parser that reads it and not a reason for detection to refuse. +func probeFromMerge(merge *yaml.Node, depth int) (sniffProbe, error) { + if depth <= 0 { + return sniffProbe{}, nil + } + + switch merge.Kind { + case yaml.AliasNode: + if merge.Alias == nil { + return sniffProbe{}, nil + } + return probeFromMerge(merge.Alias, depth-1) + case yaml.MappingNode: + return probeFromMapping(merge, depth-1) + case yaml.SequenceNode: + // A sequence merges each of its entries, earlier ones winning over later, + // which is the precedence YAML gives them. + var probe sniffProbe + for _, item := range merge.Content { + merged, err := probeFromMerge(item, depth-1) + if err != nil { + return sniffProbe{}, err + } + probe.fillFrom(merged) + } + return probe, nil + default: + return sniffProbe{}, nil + } +} + +// probeVersion returns the version string a probe key's value declares, and an +// error for a value that is not a scalar at all. +// +// The scalar's text is taken as written rather than decoded, because the two +// disagree only for tags no version carries — a version key is not !!binary — +// and because decoding is what must not happen here: a mapping handed back to +// the decoder is the quadratic path decodeYAML exists to avoid, and a probe +// key's own value is the last place one could still be handed to it. +func probeVersion(value *yaml.Node) (string, error) { + if value.Kind != yaml.ScalarNode { + return "", fmt.Errorf("version key is %s, not a scalar", value.ShortTag()) + } + return value.Value, nil +} + +// fieldFor returns the probe field that key names, or nil for a key that names +// neither. Only a scalar names one: a mapping or sequence used as a key is legal +// YAML and is not one of the two spellings this looks for. +func (p *sniffProbe) fieldFor(key *yaml.Node) *string { + if key.Kind != yaml.ScalarNode { + return nil + } + switch key.Value { + case "openapi": + return &p.OpenAPI + case "swagger": + return &p.Swagger + default: + return nil + } +} + +// fillFrom takes from other only what p does not already declare, which is what +// makes a merged key lose to a written one. +func (p *sniffProbe) fillFrom(other sniffProbe) { + if p.OpenAPI == "" { + p.OpenAPI = other.OpenAPI + } + if p.Swagger == "" { + p.Swagger = other.Swagger + } +} + // decodeFlowEntries reads the top-level entries of data, which may be a whole // document or a prefix of one, and reports whether it opened a flow mapping. The // JSON decoder is used because it streams: a prefix cut mid-document still diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index 7523650e..433d7d78 100644 --- a/compilers/openapi/detect_test.go +++ b/compilers/openapi/detect_test.go @@ -7,6 +7,7 @@ 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" @@ -303,3 +304,287 @@ func codesOf(diags []ir.Diagnostic) []string { } return codes } + +// dupRepeats is how many times the fixtures below repeat a root key. It is not a +// threshold: the wrong answer reproduces at two repeats, and this many only +// makes the fixture recognizably a document rather than a corner. It is +// deliberately far below the 6,553 of the report — yaml.v3 raises one error per +// pair of matching keys, so that count produced 21,467,628 of them and a 1.2 GB +// message, and a fixture that large turns a revert into an out-of-memory kill +// instead of a failing assertion. What guards the cost is +// TestSniff_CostIsNotQuadraticInRepeatedKeys, which measures growth rather than +// paying for it. +const dupRepeats = 512 + +// TestDetect_RepeatedKeysDoNotDecideTheFormat pins the fix for the blow-up. A +// document that repeats a top-level key is a document with a duplicate key — +// the parser this compiler goes on to use says so, once per repeat and sited — +// and it is not a document of another format, nor one that cannot be read. +// Detection used to answer both of those, because it decoded the root mapping to +// read two keys and yaml.v3 abandons a mapping that repeats any key at all. +// +// Both orders are pinned: where a writer put the version key says nothing about +// what the document is, and a fixture that declares it first cannot see a +// regression that loses it to the repeats that follow. +func TestDetect_RepeatedKeysDoNotDecideTheFormat(t *testing.T) { + t.Parallel() + repeats := strings.Repeat("x: y\n", dupRepeats) + cases := []struct{ name, src string }{ + {"version first", "openapi: 3.0.3\ninfo: {title: t, version: v}\n" + repeats}, + {"version last", "info: {title: t, version: v}\n" + repeats + "openapi: 3.0.3\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, diags, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(tc.src)}) + assert.True(t, ok, "a document this compiler can lower must not be declined over a repeated key") + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: "3.0"}, got) + assert.Nil(t, codesOf(diags), "the repeats are the parser's to report, sited, not detection's") + }) + } +} + +// TestDetect_ARepeatedVersionKeyAgreesWithTheParser holds detection to the +// answer the lowering will reach. load reads the version off the parsed document +// and records it on ir.SourceInfo, and that parser takes a repeated key's last +// spelling; detection naming the first would give one document two dialects, +// one routing it and one describing it. +// +// The two orders are the test: a single order passes whichever spelling is +// taken. +func TestDetect_ARepeatedVersionKeyAgreesWithTheParser(t *testing.T) { + t.Parallel() + cases := []struct{ first, second, want string }{ + {"3.1.0", "3.0.3", "3.0"}, + {"3.0.3", "3.1.0", "3.1"}, + } + for _, tc := range cases { + t.Run(tc.first+" then "+tc.second, func(t *testing.T) { + t.Parallel() + src := "openapi: " + tc.first + "\nopenapi: " + tc.second + "\ninfo: {}\n" + got, _, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(src)}) + require.True(t, ok) + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: tc.want}, got, + "the last spelling is the one the parser reads and records") + }) + } +} + +// TestDetect_ReadsAVersionKeyThroughAMergeKey holds the merge cases the decoder +// this replaced handled for free. A root that merges another mapping declares +// what that mapping declares, and dropping it would put a new instance of "a +// field supplied through a merge key reaches the IR in no form" into the one +// place that decides whether the document is read at all. +func TestDetect_ReadsAVersionKeyThroughAMergeKey(t *testing.T) { + t.Parallel() + cases := []struct { + name, src, want string + }{ + {"alias", "base: &b\n openapi: 3.1.0\n<<: *b\ninfo: {}\n", "3.1"}, + {"mapping written out", "<<: {openapi: 3.1.0}\ninfo: {}\n", "3.1"}, + {"sequence of aliases", "one: &o\n unrelated: x\ntwo: &t\n openapi: 3.1.0\n<<: [*o, *t]\n", "3.1"}, + {"earlier merge wins", "one: &o\n openapi: 3.0.3\ntwo: &t\n openapi: 3.1.0\n<<: [*o, *t]\n", "3.0"}, + {"a written key beats a merged one", "base: &b\n openapi: 3.0.3\n<<: *b\nopenapi: 3.1.0\n", "3.1"}, + {"a quoted << merges nothing", "base: &b\n openapi: 3.1.0\n\"<<\": *b\ninfo: {}\n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, _, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(tc.src)}) + if tc.want == "" { + assert.False(t, ok, "a key spelled << as a plain string merges nothing") + return + } + require.True(t, ok) + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: tc.want}, got) + }) + } +} + +// TestSniff_BoundsAMergeChain pins the bound on the one recursion this file has. +// A merge key's value may be an alias to a mapping that merges another, so the +// chain is a property of the document and not of its size, and an anchor may +// name a mapping that reaches itself. The bound is what makes the walk finite; +// what it costs is a version key buried deeper than any document writes one. +func TestSniff_BoundsAMergeChain(t *testing.T) { + t.Parallel() + var b strings.Builder + for i := range maxMergeDepth + 2 { + fmt.Fprintf(&b, "l%d: &a%d\n", i, i) + if i == 0 { + b.WriteString(" openapi: 3.1.0\n") + continue + } + fmt.Fprintf(&b, " <<: *a%d\n", i-1) + } + deep := b.String() + fmt.Sprintf("<<: *a%d\n", maxMergeDepth+1) + + probe, err := sniff([]byte(deep)) + require.NoError(t, err, "a chain past the bound is declined, not failed") + assert.Empty(t, probe.OpenAPI, "past the bound the key is not followed to") + + shallow := "l0: &a0\n openapi: 3.1.0\n<<: *a0\n" + probe, err = sniff([]byte(shallow)) + require.NoError(t, err) + assert.Equal(t, "3.1.0", probe.OpenAPI, "the bound must not refuse the depth a document writes") +} + +// TestDecodeYAML_RefusesARootThatIsNoMapping pins the shape complaint. A source +// with no root mapping has no top-level keys, and saying so is what lets Detect +// report bytes that declare a key it serves and will not read. +func TestDecodeYAML_RefusesARootThatIsNoMapping(t *testing.T) { + t.Parallel() + cases := []struct{ name, src, wantErr string }{ + {"sequence", "- openapi: 3.1.0\n", "!!seq"}, + {"scalar", "just a string\n", "!!str"}, + {"empty", "", ""}, + {"comment only", "# openapi: 3.1.0\n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe, err := decodeYAML([]byte(tc.src)) + assert.Empty(t, probe.OpenAPI) + if tc.wantErr == "" { + assert.NoError(t, err, "bytes that carry no document decline rather than fail") + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr, "the complaint names the shape that was read") + }) + } +} + +// TestSniff_CostIsNotQuadraticInRepeatedKeys guards the half of the defect an +// answer cannot see. Reading two keys off a parsed tree is linear in the +// document; decoding the root mapping to read them is quadratic in how often a +// key repeats, because yaml.v3 compares every pair of keys before it reads any +// of them. Both spellings answer alike on a small fixture, and only one of them +// still answers on a large one. +// +// Allocation count is the probe because it is deterministic where wall time is +// not. Doubling the repeats doubles the parse, so the bound is loose enough for +// that and nowhere near a quadratic term: measured at this size the linear +// reading grows by 1.97 and the quadratic one by 4.64. +func TestSniff_CostIsNotQuadraticInRepeatedKeys(t *testing.T) { + head := "openapi: 3.0.3\ninfo: {}\n" + small := []byte(head + strings.Repeat("x: y\n", dupRepeats)) + large := []byte(head + strings.Repeat("x: y\n", dupRepeats*2)) + + smallAllocs := testing.AllocsPerRun(2, func() { _, _ = sniff(small) }) + largeAllocs := testing.AllocsPerRun(2, func() { _, _ = sniff(large) }) + + require.Positive(t, smallAllocs, "a measurement of nothing bounds nothing") + assert.Less(t, largeAllocs, smallAllocs*3, + "twice the repeats must cost about twice, not about four times") +} + +// TestDecodeYAML_RefusesAVersionKeyThatIsNoScalar pins the complaint for a +// version key whose value is a mapping or a sequence, written directly and +// reached through a `<<`. Such bytes name a key this compiler serves and do not +// say what dialect, which is unreadable here and nobody else's. +// +// Whether Detect reports that or declines in silence is declaresProbeKey's +// answer and not this one's, and it is asserted separately below: the guard +// reads a key at column 0, and a merged key is indented under the mapping that +// carries it. +func TestDecodeYAML_RefusesAVersionKeyThatIsNoScalar(t *testing.T) { + t.Parallel() + cases := []struct{ name, src, wantErr string }{ + {"mapping", "openapi: {a: b}\ninfo: {}\n", "!!map"}, + {"sequence", "openapi: [3.1.0]\ninfo: {}\n", "!!seq"}, + {"merged mapping", "base: &b\n openapi: {a: b}\n<<: *b\n", "!!map"}, + {"merged through a sequence", "base: &b\n openapi: {a: b}\n<<: [*b]\n", "!!map"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe, err := decodeYAML([]byte(tc.src)) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr, "the complaint names the shape that was read") + assert.Empty(t, probe.OpenAPI) + }) + } +} + +// TestDetect_ReportsAnUnreadableVersionKeyOnlyWhereItIsDeclared pins the split +// the guard makes. A version key written at column 0 makes the source +// recognizably this compiler's, so a value that is no version is reported; the +// same value reached through a `<<` is indented under the mapping that carries +// it, which declaresProbeKey does not read, so the source is declined in silence +// instead. +// +// The silent half is deliberate and is the direction to be wrong in: the guard +// may not be widened to "the name occurs somewhere followed by a colon" without +// claiming documents of formats that nest a key of that name, and reporting +// those under this compiler's parse error is the one thing detection must not +// do. +func TestDetect_ReportsAnUnreadableVersionKeyOnlyWhereItIsDeclared(t *testing.T) { + t.Parallel() + cases := []struct { + name, src string + wantCode []string + }{ + {"declared at column 0", "openapi: {a: b}\ninfo: {}\n", []string{diag.UndecodableSource}}, + {"reached through a merge", "base: &b\n openapi: {a: b}\n<<: *b\n", nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, diags, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(tc.src)}) + assert.False(t, ok) + assert.Equal(t, tc.wantCode, codesOf(diags)) + }) + } +} + +// TestDecodeYAML_PassesOverWhatNamesNoVersion holds the walk to reading only +// what it came for. A mapping may key an entry with a sequence, and a `<<` may +// be written with a value that merges nothing; both are the source's business +// and neither stops the two keys beside them from being read. +func TestDecodeYAML_PassesOverWhatNamesNoVersion(t *testing.T) { + t.Parallel() + cases := []struct{ name, src string }{ + {"a key that is a sequence", "? [a, b]\n: v\nopenapi: 3.1.0\n"}, + {"a merge of a scalar", "<<: not-a-mapping\nopenapi: 3.1.0\n"}, + {"a merge of a sequence of scalars", "<<: [x, y]\nopenapi: 3.1.0\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe, err := decodeYAML([]byte(tc.src)) + require.NoError(t, err) + assert.Equal(t, "3.1.0", probe.OpenAPI) + }) + } +} + +// TestProbeFromMapping_StopsAtTheBound reaches the bound at the mapping rather +// than at the merge, which is the other of the two places the count is spent. +// It is called directly because the depth a chain lands on is a property of the +// chain, and pinning the bound through one is pinning the chain instead. +func TestProbeFromMapping_StopsAtTheBound(t *testing.T) { + t.Parallel() + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("base: &b\n openapi: 3.1.0\n<<: *b\n"), &root)) + + spent, err := probeFromMapping(documentRoot(&root), 0) + require.NoError(t, err, "a walk that stops at the bound declines; it does not fail") + assert.Empty(t, spent.OpenAPI, "at the bound the merge is not followed") + + within, err := probeFromMapping(documentRoot(&root), maxMergeDepth) + require.NoError(t, err) + assert.Equal(t, "3.1.0", within.OpenAPI, "the same mapping within the bound is read") +} + +// TestProbeFromMerge_DeclinesAnAliasThatResolvedToNothing covers the guard on an +// alias node carrying no target. A parser resolves every alias it accepts, so the +// node is built here rather than parsed: the guard exists because dereferencing +// the field is what the next line does, and a nil there is a panic in detection, +// which runs before the compiler has decided the bytes are even its own. +func TestProbeFromMerge_DeclinesAnAliasThatResolvedToNothing(t *testing.T) { + t.Parallel() + probe, err := probeFromMerge(&yaml.Node{Kind: yaml.AliasNode}, maxMergeDepth) + require.NoError(t, err) + assert.Empty(t, probe.OpenAPI) +} diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index 80d7a0ec..b8216407 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -13,6 +13,7 @@ package diag import ( "fmt" "strings" + "unicode/utf8" "github.com/dexpace/morphic/ir" ) @@ -315,8 +316,25 @@ func HasError(diags []ir.Diagnostic) bool { return ir.HasError(diags) } +// MaxQuotedErrorBytes bounds what a foreign error contributes to a diagnostic +// message. A diagnostic is read by a person and stored by a log, and an error +// raised by a library obeys neither: yaml.v3 reports a duplicated mapping key +// once per prior occurrence of it, so a 32 KB source repeating one key 6,553 +// times raises an error of 1.2 GB. Quoting that whole is not a report. +// +// The cap is generous because the errors worth quoting are lists — the overlay +// validator writes one sentence per finding — and a list cut to its first entry +// says less than the reader came for. What a cut costs is the tail; what it +// buys is that a message is always a message. +const MaxQuotedErrorBytes = 4 << 10 + +// elidedMarker ends a message the cap cut. It carries no count: a marker whose +// text depends on how much was dropped makes the message depend on the whole +// error again, which is the dependency the cap exists to remove. +const elidedMarker = "… (elided)" + // OneLine collapses err's text onto a single line, for a diagnostic that carries -// an error raised by something else. +// an error raised by something else, and cuts it at MaxQuotedErrorBytes. // // A diagnostic is rendered one per line, so an embedded newline splits one // report into several — and every line after the first carries no severity, code @@ -328,9 +346,15 @@ func HasError(diags []ir.Diagnostic) bool { // Parts are joined with "; " so a flat list reads as a list, except after a part // that already ends in a colon, where the next line is that header's content and // a semicolon would read as a break in it. +// +// The scan stops at the cap rather than trimming afterwards, so the work is +// bounded by what is kept and not by what the library wrote. func OneLine(err error) string { var out strings.Builder - for _, line := range strings.Split(err.Error(), "\n") { + for rest := err.Error(); rest != "" && out.Len() < MaxQuotedErrorBytes; { + var line string + line, rest, _ = strings.Cut(rest, "\n") + part := strings.Join(strings.Fields(line), " ") if part == "" { continue @@ -344,5 +368,22 @@ func OneLine(err error) string { } out.WriteString(part) } - return out.String() + return cutToCap(out.String()) +} + +// cutToCap returns msg bounded by MaxQuotedErrorBytes, marked when it cut. +// +// The cut lands on a rune boundary. The bytes are a foreign library's and may be +// multi-byte, and half a rune in a diagnostic is ill-formed text put in front of +// a reader — the one thing a report must not do, and the reason checkDiagnostics +// refuses to quote invalid UTF-8 back at all. +func cutToCap(msg string) string { + if len(msg) <= MaxQuotedErrorBytes { + return msg + } + cut := MaxQuotedErrorBytes + for cut > 0 && !utf8.RuneStart(msg[cut]) { + cut-- + } + return msg[:cut] + elidedMarker } diff --git a/compilers/openapi/internal/diag/diag_test.go b/compilers/openapi/internal/diag/diag_test.go index 76fec0c9..5697c2bd 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -174,7 +174,10 @@ func TestCodes_MatchTheDeclaredSet(t *testing.T) { } // declaredCodeCount returns how many exported string constants the package -// declares, read from its own source. +// declares, read from its own source. A code is a string, so an exported +// constant of any other kind — MaxQuotedErrorBytes is one — is not one and is +// not counted; the kind is read off the declaration rather than the name, so a +// code added here is counted whatever it is called. // // It is parsed rather than written down because a maintained count is exactly // the claim that rots silently: a code added without touching this file would @@ -204,7 +207,8 @@ func declaredCodeCount(t *testing.T) int { return n } -// constNamesIn returns how many exported names decl declares as constants. +// constNamesIn returns how many exported names decl declares as string +// constants. func constNamesIn(decl ast.Decl) int { gen, ok := decl.(*ast.GenDecl) if !ok || gen.Tok != token.CONST { @@ -216,8 +220,8 @@ func constNamesIn(decl ast.Decl) int { if !ok { continue } - for _, name := range vs.Names { - if name.IsExported() { + for i, name := range vs.Names { + if name.IsExported() && isStringLiteral(vs, i) { n++ } } @@ -225,6 +229,17 @@ func constNamesIn(decl ast.Decl) int { return n } +// isStringLiteral reports whether the i'th name of vs is bound to a string +// literal. A ValueSpec with no values at position i is an iota-style or repeated +// declaration, which no code in this package uses and which names no string. +func isStringLiteral(vs *ast.ValueSpec, i int) bool { + if i >= len(vs.Values) { + return false + } + lit, ok := vs.Values[i].(*ast.BasicLit) + return ok && lit.Kind == token.STRING +} + // TestOneLine_CollapsesWhatALibraryWrote pins both join rules and the reason for // each: a flat list of findings reads as a list, while a header that ends in a // colon owns the line after it and must not be cut from it by a semicolon. @@ -251,3 +266,56 @@ func TestOneLine_CollapsesWhatALibraryWrote(t *testing.T) { }) } } + +// TestOneLine_BoundsWhatALibraryWrote pins the cap. A diagnostic message is +// something a person reads and something a log stores, and neither survives an +// unbounded one: yaml.v3 reports a duplicated mapping key once per prior +// occurrence, so a 32 KB source with one key repeated 6,553 times produces a +// 1.2 GB error string, which this used to copy whole into a message the CLI +// then printed. +func TestOneLine_BoundsWhatALibraryWrote(t *testing.T) { + t.Parallel() + huge := errors.New(strings.Repeat("a line of complaint\n", 1<<16)) + got := diag.OneLine(huge) + + assert.Less(t, len(got), diag.MaxQuotedErrorBytes+64, + "a foreign error may be any size; what it contributes to a message may not") + assert.True(t, strings.HasPrefix(got, "a line of complaint; a line of complaint"), + "the cut keeps the head, which is the part that says what went wrong") + assert.Contains(t, got, "elided", "a cut message says it was cut") +} + +// TestOneLine_CutsOnARuneBoundary holds the cut to well-formed output. The bytes +// being quoted are a foreign library's and may be multi-byte; cutting one in +// half would put ill-formed UTF-8 into a diagnostic, which is the one thing a +// report must never do to a reader. +func TestOneLine_CutsOnARuneBoundary(t *testing.T) { + t.Parallel() + for pad := range 8 { + got := diag.OneLine(errors.New(strings.Repeat("x", pad) + strings.Repeat("é", diag.MaxQuotedErrorBytes))) + assert.True(t, utf8.ValidString(got), "pad %d: a cut message is still text", pad) + } +} + +// TestOneLine_IsBoundedInWorkNotOnlyOutput holds the cap to being a bound on +// work. A message capped by collapsing the whole error and trimming the result +// still walks the whole error, which is the half that costs the time: the 1.2 GB +// case spent 7.4 s building the parts it was about to throw away. +// +// Allocation count is the probe because the per-line work is what allocates — +// one strings.Fields join per line — so a scan that stops at the cap allocates +// the same for two errors that both exceed it, and one that does not scales with +// the error. It is not run in parallel: AllocsPerRun measures the process. +func TestOneLine_IsBoundedInWorkNotOnlyOutput(t *testing.T) { + small := errors.New(strings.Repeat("line\n", 1<<10)) + large := errors.New(strings.Repeat("line\n", 1<<20)) + require.Greater(t, len(small.Error()), diag.MaxQuotedErrorBytes, + "both inputs must exceed the cap, or the comparison is between two uncapped runs") + + assert.Equal(t, diag.OneLine(small), diag.OneLine(large), + "past the cap the answer no longer depends on how much more there was") + assert.Equal(t, + testing.AllocsPerRun(2, func() { _ = diag.OneLine(small) }), + testing.AllocsPerRun(2, func() { _ = diag.OneLine(large) }), + "past the cap the work no longer depends on how much more there was") +} diff --git a/internal/archtest/recursion_test.go b/internal/archtest/recursion_test.go index 28c78744..07d709cc 100644 --- a/internal/archtest/recursion_test.go +++ b/internal/archtest/recursion_test.go @@ -55,6 +55,10 @@ var loweringRecursions = [][]string{ // refuses past maxDynamicAnchorDepth or a spent node budget and records the // refusal, so a caller learns the index is partial. {"anchorWalk.walk", "anchorWalk.walkMapping"}, + // Format detection's read of a root mapping's merge keys. A `<<` may name a + // mapping that merges another, so following one is following a chain the + // document's size does not bound. Bounded by maxMergeDepth. + {"probeFromMapping", "probeFromMerge"}, // Property lookup through a composition. Finding a property by wire name // descends into a model's base and mixins, each of which is a model whose // properties are looked up the same way.