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.