diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index 3f5a463e..e659f472 100644 --- a/compilers/openapi/detect.go +++ b/compilers/openapi/detect.go @@ -2,7 +2,6 @@ package openapi import ( "bytes" - "encoding/json" "fmt" yaml "gopkg.in/yaml.v3" @@ -12,22 +11,16 @@ import ( "github.com/dexpace/morphic/ir" ) -// maxSniffBytes bounds the prefix Detect parses on its fast path. Detection -// reads two top-level keys, and 64 KiB reaches them in any document a person -// wrote, so the cost of asking stays flat while spec size does not: a full parse -// of a 10 MB document costs hundreds of milliseconds before the compiler's own -// parse begins. It is a bound on the fast path, not on detection — a document -// whose prefix declares neither key while its bytes name one is read whole, per -// sniffWhole. +// maxSniffBytes is the size at which detection stops parsing and scans instead. +// Detection reads two top-level keys, and 64 KiB reaches them in any document a +// person wrote, so the cost of asking stays flat while spec size does not: a +// full parse of a 10 MB document costs hundreds of milliseconds before the +// compiler's own size and node budgets have agreed to pay for one. +// +// Nothing is declined for being large. Past the cap the same two keys are read +// by scanProbe, in one linear pass that builds no tree. const maxSniffBytes = 64 << 10 -// maxSniffEntries bounds the top-level entries read from a flow-style mapping. -// A document declares few top-level keys however large it grows, so a mapping -// that runs past this without naming either key is not one this compiler will -// take. The bound is on entries, not bytes: one of them may be megabytes long, -// 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. @@ -90,47 +83,122 @@ func (*Compiler) Detect(src compilers.Source) (compilers.SourceFormat, []ir.Diag // declaresProbeKey reports whether data names one of the discriminating keys as // a top-level key. It is what separates a source of this compiler's own from one -// of another format that was never its business, and it is asked twice: before -// sniff parses a large document whole, and after a parse failed, where "not -// YAML" alone says only what a Protobuf or Smithy source would also say. +// of another format that was never its business, and it is asked once: after a +// parse failed, where "not YAML" alone says only what a Protobuf or Smithy +// source would also say. A parse only runs at or below the cap — past it the +// scan answers and cannot fail — so the bytes reaching here are never large. // -// The whole of data is read. A byte scan costs a fraction of the parse it stands -// in front of, and the key it looks for is exactly the one that can sit -// megabytes into a document — bounding this to the prefix would blind it in -// precisely the case it exists to catch. +// Top-level is the whole of the claim, and the two styles answer it by different +// structure: column 0 in block style, the root mapping's own entries in flow +// style. Neither reading may be widened to "the name occurs somewhere followed +// by a colon", because other formats nest a key of that name, and reporting +// their bytes under this compiler's parse error is the one thing detection must +// never do. func declaresProbeKey(data []byte) bool { - return declaresKey(data, "openapi") || declaresKey(data, "swagger") + data = trimBOM(data) + return declaresBlockKey(data, "openapi") || declaresBlockKey(data, "swagger") || + declaresFlowKey(data) } -// declaresKey reports whether data names key at the top level, in either style: -// unquoted at the start of a line for block style, or quoted for flow style, -// which is how JSON writes every key. +// declaresBlockKey reports whether data writes key bare at the start of a line, +// which in block style is where a top-level key goes and nowhere else: a key +// nested under another is indented past column 0, and a block scalar's content +// is indented past its own key. // -// Both spellings require the colon that makes it a key. Without it, a document -// of another format that merely mentions the word — in a comment, or as a value -// — would be claimed as this compiler's and reported under its parse error. -func declaresKey(data []byte, key string) bool { - block := []byte(key + ":") - if bytes.HasPrefix(data, block) || bytes.Contains(data, []byte("\n"+key+":")) { - return true - } - return followedByColon(data, []byte(`"`+key+`"`)) -} - -// followedByColon reports whether name occurs in data followed by a colon, -// ignoring the whitespace a flow mapping may put between them. -func followedByColon(data, name []byte) bool { - for i := 0; ; { - j := bytes.Index(data[i:], name) - if j < 0 { - return false +// Only the bare spelling is read here, because the quoted one is how flow style +// writes every key and flow structure is what scopes it — declaresFlowKey has +// it. A block document that quotes its top-level key is therefore not seen, and +// is declined in silence rather than claimed; that is the direction to be wrong +// in, and the spelling is rare enough that widening column 0 to admit the shape +// JSON writes at every depth would cost far more than it buys. +// +// The colon that makes it a key is required. Without it, a document of another +// format that merely mentions the word — in a comment, or as a value — would be +// claimed as this compiler's and reported under its parse error. What follows +// the colon is not: this guard is asked only after a reading has failed, so +// there is no version left to read, and nothing else will claim a file this +// compiler has already called broken. scanProbe names a format and routes the +// source, so its block reading requires the separated colon YAML does; the +// looseness here is deliberate, not inherited. +func declaresBlockKey(data []byte, key string) bool { + name := []byte(key + ":") + return bytes.HasPrefix(data, name) || bytes.Contains(data, append([]byte("\n"), name...)) +} + +// declaresFlowKey reports whether data opens a flow mapping — the shape JSON +// writes — that names one of the discriminating keys among its own entries. +// +// Nesting depth is what makes the answer top-level, and it is the half a plain +// search for `"openapi":` gets wrong: a quoted name followed by a colon reads as +// a key wherever it sits, and other formats nest one. A source that opens no +// mapping at all — a JSON array, say — declares nothing here for the same +// reason: whatever it names, it does not name it as its own root key. +// +// The scan is a lexer, not a parser: it tracks quoted strings and nesting and +// reads nothing else. It has to answer on bytes that will not parse, which is +// the case it exists for — a document broken before the key that names it — so +// there is no tree to ask instead. +func declaresFlowKey(data []byte) bool { + i := skipSpace(data, 0) + if i == len(data) || data[i] != '{' { + return false + } + + for depth := 0; i < len(data); { + switch data[i] { + case '"': + name, next := flowString(data, i) + if depth == 1 && isProbeName(name) && startsWithColon(data, next) { + return true + } + i = next + case '{', '[': + depth++ + i++ + case '}', ']': + depth-- + i++ + default: + i++ } - rest := bytes.TrimLeft(data[i+j+len(name):], " \t\r\n") - if len(rest) > 0 && rest[0] == ':' { - return true + } + return false +} + +// flowString returns the bytes between the quotes of the string data[i] opens, +// and the index just past its closing quote. An unterminated string runs to the +// end of data: there is nothing past it left to read. +func flowString(data []byte, i int) ([]byte, int) { + for j := i + 1; j < len(data); j++ { + switch data[j] { + case '\\': + j++ + case '"': + return data[i+1 : j], j + 1 } - i += j + len(name) } + return nil, len(data) +} + +// isProbeName reports whether name is one of the discriminating keys. +func isProbeName(name []byte) bool { + return string(name) == "openapi" || string(name) == "swagger" +} + +// skipSpace returns the index of the first byte at or after i that is not +// whitespace, or len(data) if there is none. +func skipSpace(data []byte, i int) int { + for i < len(data) && (data[i] == ' ' || data[i] == '\t' || data[i] == '\r' || data[i] == '\n') { + i++ + } + return i +} + +// startsWithColon reports whether the first non-whitespace byte at or after i is +// the colon that makes the name before it a key. +func startsWithColon(data []byte, i int) bool { + i = skipSpace(data, i) + return i < len(data) && data[i] == ':' } // sniff reads the discriminating keys out of data, and returns the zero probe @@ -138,51 +206,269 @@ func followedByColon(data, name []byte) bool { // worth reporting is Detect's question, not this one's: here it is only the // record of what happened. // -// A document within the cap is decoded whole and exactly. A larger one is read -// from its prefix first, and only from all of itself when that prefix answered -// nothing and the bytes past it name a key this compiler serves. +// A document within the cap is decoded whole and exactly, which is the only way +// to tell one that declares nothing from one that will not parse. A larger one +// is scanned instead: the answer detection owes is which of two keys a document +// declares, and a scan reads that in one linear pass, where a parse builds a +// tree of everything between them before the compiler's size and node budgets +// have agreed to pay for one. func sniff(data []byte) (sniffProbe, error) { + probe, err := readProbe(data) + return declaredVersions(probe), err +} + +// readProbe reads the probe keys by whichever means the document's size affords. +func readProbe(data []byte) (sniffProbe, error) { if len(data) <= maxSniffBytes { return decodeYAML(data) } + return scanProbe(data), nil +} - probe, err := sniffPrefix(data[:maxSniffBytes]) - if probe.OpenAPI != "" || probe.Swagger != "" { - return probe, nil +// declaredVersions drops any value that does not read as a version. A key alone +// does not declare a format: another format's document may write the word — at +// column 0 in Markdown prose, or as a field of its own — and what separates that +// from a declaration is the version beside it. Claiming it instead reports this +// compiler's complaint over a file that was never its own. +func declaredVersions(probe sniffProbe) sniffProbe { + if !isVersion(probe.OpenAPI) { + probe.OpenAPI = "" } - if declaresProbeKey(data) { - return sniffWhole(data) + if !isVersion(probe.Swagger) { + probe.Swagger = "" } - return probe, err + return probe } -// sniffPrefix reads the probe keys from the first maxSniffBytes of a document -// too large to decode whole. The prefix cannot simply be cut: flow style — JSON -// is the common case — is one token stream with no line structure, so its -// entries are streamed instead, and block style is cut at its last complete -// line. -func sniffPrefix(prefix []byte) (sniffProbe, error) { - if probe, ok := decodeFlowEntries(prefix); ok { - return probe, nil +// isVersion reports whether value reads as a dotted version: digits and dots, +// beginning with a digit. It admits the three shapes majorMinor is written for — +// "3.1.0", "3.1", and a bare "4" — and nothing that a sentence of prose is. +func isVersion(value string) bool { + if value == "" || value[0] < '0' || value[0] > '9' { + return false } - return decodeYAML(wholeLines(prefix)) + for i := range len(value) { + if (value[i] < '0' || value[i] > '9') && value[i] != '.' { + return false + } + } + return true } -// sniffWhole reads the probe keys from a whole document past the cap, for the -// one case that earns the parse: the prefix declared neither key, yet the bytes -// name one further in. Mapping key order carries no meaning, so a document that -// writes a multi-megabyte `components` before its `openapi` is as valid as one -// that writes them the other way round, and declining it would reject a valid -// document over nothing. +// scanProbe reads the probe keys and their versions out of data without building +// a tree of it. Both styles are scanned, because which one a document is written +// in is not known until it has been read: block style writes a top-level key at +// column 0, flow style writes it among the root mapping's own entries. // -// Nothing another format wrote reaches here — declaresProbeKey guards the call — -// so the cost is paid only for bytes this compiler is about to parse in full -// anyway, and the answer for everyone else is still the fast path's silence. -func sniffWhole(data []byte) (sniffProbe, error) { - if probe, ok := decodeFlowEntries(data); ok { - return probe, nil +// Both keys are read wherever they sit, so where a document declares one carries +// no meaning here — mapping keys being unordered, that is the whole property. +// Which of the two wins when a document declares both is Detect's question. +// +// The scan reads less than the parse it stands in for, and the cap decides which +// of them answers, so every shape they disagree on is a document that names one +// format below the cap and another above it. The shapes the scan declines by +// design — a root merge key, a quoted key in block style, an unquoted one in +// flow style, an anchor or a tag before the version — are declared in +// TestReadings_AgreeExceptWhereDeclared, each against its reason, rather than +// here: that table fails when a reason goes stale or a new divergence appears, +// and prose can do neither. +func scanProbe(data []byte) sniffProbe { + var probe sniffProbe + first := firstDocument(trimBOM(data)) + scanBlockProbe(first, &probe) + scanFlowProbe(first, &probe) + return probe +} + +// bomUTF8 is the UTF-8 byte-order mark. YAML admits one at the start of a +// stream and JSON is its subset, so a spec written by an editor that emits one +// is a spec like any other: yaml.v3 reads straight through it and so does the +// loader. The byte scans have to skip it themselves, or the same document would +// name a format at the cap and none one byte past it. +const bomUTF8 = "\xef\xbb\xbf" + +// trimBOM returns data without a leading byte-order mark. Comparing through a +// string conversion compiles to a comparison rather than a copy, so the scan +// stays allocation-free. +func trimBOM(data []byte) []byte { + if len(data) >= len(bomUTF8) && string(data[:len(bomUTF8)]) == bomUTF8 { + return data[len(bomUTF8):] + } + return data +} + +// firstDocument returns the content of data's first YAML document. A stream may +// carry several, opened by `---` and ended by `...` at column 0, and load reads +// only the first, so a key in a later one names a format for bytes the compile +// never parses. A marker ends the document even in the middle of a scalar, +// which is what makes a line scan the right reading for one. +// +// The opening marker is left behind rather than returned, so what comes back +// begins where the document's own bytes do: a flow document written after a +// `---`, or on its line, is then the same bytes to the flow scan as one written +// without a marker at all. +func firstDocument(data []byte) []byte { + start, opened := 0, false + for i := 0; i < len(data); { + line, next := nextLine(data, i) + marker, isMarker := docMarker(line) + if isMarker && (opened || marker == '.') { + return data[start:i] + } + if isMarker { + start, opened = i+len("---"), true + } + if contentLine(line) { + opened = true + } + i = next + } + return data[start:] +} + +// nextLine returns the line beginning at i, without its terminator, and the +// index of the line after it. +func nextLine(data []byte, i int) (line []byte, next int) { + line = data[i:] + if j := bytes.IndexByte(line, '\n'); j >= 0 { + return line[:j], i + j + 1 + } + return line, len(data) +} + +// docMarker reports whether line is a document marker — `---` opening one or +// `...` ending one — and which. A marker is the three bytes at column 0 +// followed by whitespace or the end of the line; `----` and `...x` are content. +func docMarker(line []byte) (marker byte, ok bool) { + if len(line) < 3 || (line[0] != '-' && line[0] != '.') { + return 0, false + } + if line[1] != line[0] || line[2] != line[0] { + return 0, false + } + if len(line) > 3 && line[3] != ' ' && line[3] != '\t' && line[3] != '\r' { + return 0, false + } + return line[0], true +} + +// contentLine reports whether line carries document content: anything but +// blank space, a comment, or a `%` directive. Content before any `---` opens +// the document implicitly, so a marker after it ends the document rather than +// opening one. +func contentLine(line []byte) bool { + rest := bytes.TrimLeft(line, " \t\r") + return len(rest) > 0 && rest[0] != '#' && rest[0] != '%' +} + +// scanBlockProbe reads a block document's top-level entries, which are its lines +// beginning at column 0. It allocates nothing per line: a document past the cap +// is megabytes of lines this walks and keeps none of. +func scanBlockProbe(data []byte, probe *sniffProbe) { + for i := 0; i < len(data); { + var line []byte + line, i = nextLine(data, i) + if name, value, ok := blockEntry(line); ok { + setVersion(probe, name, value) + } + } +} + +// blockEntry returns the probe key line writes and the value beside it, and +// reports whether line writes one at all. +// +// A space, a tab or the end of the line has to follow the colon. YAML reads +// `openapi:3.1.0` as a plain scalar and not as a key — the parse below the cap +// refuses that document for having a string at its root — so a scan that took it +// for a key would name a format on bytes the parser says declare none. +func blockEntry(line []byte) (name, value []byte, ok bool) { + name, rest, cut := bytes.Cut(line, []byte(":")) + if !cut || !isProbeName(name) || !separated(rest) { + return nil, nil, false + } + return name, blockValue(rest), true +} + +// separated reports whether rest, the bytes after a colon, begins the way a +// block mapping value must: with whitespace, or with nothing at all. +func separated(rest []byte) bool { + return len(rest) == 0 || rest[0] == ' ' || rest[0] == '\t' || rest[0] == '\r' +} + +// blockValue returns the scalar a block entry writes after its colon, without the +// space around it, a trailing comment, or the quotes either style of quoting may +// have put around it. +func blockValue(raw []byte) []byte { + value := bytes.TrimSpace(raw) + if i := bytes.Index(value, []byte(" #")); i >= 0 { + value = bytes.TrimSpace(value[:i]) + } + if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') && value[len(value)-1] == value[0] { + value = value[1 : len(value)-1] + } + return value +} + +// scanFlowProbe reads the entries of the flow mapping data opens, which is the +// shape JSON writes. Nesting depth is what makes an entry the document's own: a +// quoted name followed by a colon reads as a key wherever it sits, and other +// formats nest one. +// +// The scan is a lexer, not a parser: it tracks quoted strings and nesting and +// reads nothing else. It has to answer on bytes that will not parse, which is the +// case it exists for — a document broken before the key that names it — so there +// is no tree to ask instead. +func scanFlowProbe(data []byte, probe *sniffProbe) { + i := skipSpace(data, 0) + if i == len(data) || data[i] != '{' { + return + } + + for depth := 0; i < len(data); { + switch data[i] { + case '"': + name, next := flowString(data, i) + if value, after, ok := flowValue(data, next); depth == 1 && isProbeName(name) && ok { + setVersion(probe, name, value) + i = after + continue + } + i = next + case '{', '[': + depth++ + i++ + case '}', ']': + depth-- + i++ + default: + i++ + } + } +} + +// flowValue returns the quoted scalar written after the colon at i, and the index +// just past it. A name with no colon after it is no key, and a version written as +// anything but a string does not declare a dialect this compiler serves. +func flowValue(data []byte, i int) ([]byte, int, bool) { + i = skipSpace(data, i) + if i == len(data) || data[i] != ':' { + return nil, i, false + } + if i = skipSpace(data, i+1); i == len(data) || data[i] != '"' { + return nil, i, false + } + value, next := flowString(data, i) + return value, next, true +} + +// setVersion stores value under probe's field for name. +func setVersion(probe *sniffProbe, name, value []byte) { + switch string(name) { + case "openapi": + probe.OpenAPI = string(value) + case "swagger": + probe.Swagger = string(value) } - return decodeYAML(data) } // decodeYAML reads the probe keys from a complete YAML (or JSON, its subset) @@ -360,65 +646,6 @@ func (p *sniffProbe) fillFrom(other sniffProbe) { } } -// 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 -// yields every entry it completed, where decoding those same bytes whole reports -// only that they end early. -func decodeFlowEntries(data []byte) (sniffProbe, bool) { - dec := json.NewDecoder(bytes.NewReader(data)) - tok, err := dec.Token() - if err != nil || tok != json.Delim('{') { - return sniffProbe{}, false - } - - var probe sniffProbe - for range maxSniffEntries { - key, err := dec.Token() - if err != nil { - break - } - var value json.RawMessage - if err := dec.Decode(&value); err != nil { - break - } - recordEntry(&probe, key, value) - } - return probe, true -} - -// recordEntry stores value under probe's field for key. key is compared as read -// rather than asserted to a string: the closing delimiter of the mapping -// arrives here too, and it matches neither name. -func recordEntry(probe *sniffProbe, key json.Token, value json.RawMessage) { - switch key { - case "openapi": - probe.OpenAPI = jsonString(value) - case "swagger": - probe.Swagger = jsonString(value) - } -} - -// jsonString returns value as a string, or "" for any other shape. A version -// that is not a string does not declare a dialect. -func jsonString(value json.RawMessage) string { - var out string - if err := json.Unmarshal(value, &out); err != nil { - return "" - } - return out -} - -// wholeLines returns prefix up to and including its last newline, so a block -// document is cut between entries rather than inside one. A prefix with no -// newline in it is returned as it is; there is no better cut to make. -func wholeLines(prefix []byte) []byte { - if i := bytes.LastIndexByte(prefix, '\n'); i >= 0 { - return prefix[:i+1] - } - return prefix -} - // majorMinor returns the "major.minor" prefix of a dotted version string, // e.g. "3.1.0" → "3.1". Strings with fewer than two dots — a bare major // version, or a version already in major.minor form — are returned unchanged. diff --git a/compilers/openapi/detect_scan_test.go b/compilers/openapi/detect_scan_test.go new file mode 100644 index 00000000..99e70ff2 --- /dev/null +++ b/compilers/openapi/detect_scan_test.go @@ -0,0 +1,332 @@ +package openapi + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/compilers" + "github.com/dexpace/morphic/compilers/openapi/internal/diag" +) + +// TestDetect_KeyOrderSurvivesTheCap pins the property key order was still +// deciding: a document that declares both keys names one format, and which of +// them detection reaches first is an artefact of where the cap fell, not of the +// document. The prefix answered on `swagger` and stopped, so the same bytes read +// as swagger@2.0 above the cap and openapi@3.0 below it. It pins that one +// shape; the two readings are held to one answer across the shapes they can +// differ on by TestReadings_AgreeExceptWhereDeclared. +func TestDetect_KeyOrderSurvivesTheCap(t *testing.T) { + t.Parallel() + small := `{"swagger":"2.0","openapi":"3.0.3"}` + big := `{"swagger":"2.0","pad":"` + flowPad() + `","openapi":"3.0.3"}` + require.LessOrEqual(t, len(small), maxSniffBytes) + require.Greater(t, len(big), maxSniffBytes) + + want := compilers.SourceFormat{Name: "openapi", Version: "3.0"} + for name, src := range map[string]string{"below the cap": small, "above the cap": big} { + t.Run(name, func(t *testing.T) { + t.Parallel() + got, _, ok := New().Detect(compilers.Source{Path: "spec.json", Data: []byte(src)}) + assert.True(t, ok) + assert.Equal(t, want, got, "which key the cap fell after does not decide the format") + }) + } +} + +// TestDetect_AValueThatIsNoVersionIsNoDeclaration pins the other half of whose +// bytes these are. A key alone does not declare a format: prose sitting beside +// the word is what a document of another format writes, and claiming it reports +// this compiler's complaint over a file that was never its own. +func TestDetect_AValueThatIsNoVersionIsNoDeclaration(t *testing.T) { + t.Parallel() + cases := []struct{ name, src string }{ + {"prose beside the key", "openapi: is a format\n"}, + {"a pointer beside the key", "openapi: see the docs\n"}, + {"prose beside swagger", "swagger: yes\n"}, + // Version-shaped at its start and not to its end. A prerelease suffix is + // the live spelling of this: it names no dialect this compiler serves, and + // reading only the leading digits would claim one it does not. + {"a prerelease suffix", "openapi: 3.1.0-rc1\n"}, + {"digits and then a word", "openapi: 3x\n"}, + {"markdown past the cap", "# Notes\n\n" + strings.Repeat("filler text\n", 8000) + "openapi: is a format\n"}, + {"flow style, prose for a version", `{"openapi":"is a format"}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, diags, ok := New().Detect(compilers.Source{Path: "README.md", Data: []byte(tc.src)}) + assert.False(t, ok, "prose beside the word is not a declaration of this format") + assert.Equal(t, compilers.SourceFormat{}, got) + assert.Nil(t, codesOf(diags), "another format's file earns no complaint from this one") + }) + } +} + +// TestDetect_DoesNotParseTheWholeDocument pins the cost. Detection answers one +// question about two keys, and it runs before the compiler's size and node +// budgets with no context to cancel it, so a parse here is one the loader has +// not yet agreed to pay for. A scan allocates a handful of times whatever the +// document's size; a parse allocates per node. +func TestDetect_DoesNotParseTheWholeDocument(t *testing.T) { + var b strings.Builder + b.WriteString("info:\n title: T\nfiller:\n") + for b.Len() < 4<<20 { + b.WriteString(" - key: " + strings.Repeat("v", 80) + "\n") + } + b.WriteString("openapi: 3.1.0\n") + src := compilers.Source{Path: "big.yaml", Data: []byte(b.String())} + + allocs := testing.AllocsPerRun(3, func() { + if _, _, ok := New().Detect(src); !ok { + t.Fatal("the document declares a version this compiler serves") + } + }) + assert.Less(t, allocs, 100.0, + "detection scans for a key; it must not build a tree of the whole document") +} + +// TestCompile_AnUnreadableSourceIsADiagnostic pins where the complaint about a +// broken document comes from once detection no longer parses one. It has to stay +// a diagnostic: engine.Run turns a compiler's Go error into its own, and the CLI +// maps that to exit 2 — the code it uses for being invoked wrong — so a spec it +// read would be reported as a misuse of itself. +func TestCompile_AnUnreadableSourceIsADiagnostic(t *testing.T) { + t.Parallel() + src := "bad: [unterminated\n" + strings.Repeat("filler: x\n", 8000) + "openapi: 3.1.0\n" + doc, diags, err := New().Compile(context.Background(), + []compilers.Source{{Path: "api.yaml", Data: []byte(src)}}, compilers.Options{}) + + require.NoError(t, err, "a document that will not parse is a finding, not a failure of the compiler") + assert.Nil(t, doc) + assert.Equal(t, []string{diag.UndecodableSource}, codesOf(diags)) +} + +// TestScanProbe_ReadsTheVersionBesideTheKey pins what the scan reads, on bytes +// that would defeat a parser. Each case is a document past the cap in one of the +// two styles, or one broken in a way that leaves the declaration legible: the +// scan's whole reason to exist is that it answers where a parse cannot. +func TestScanProbe_ReadsTheVersionBesideTheKey(t *testing.T) { + t.Parallel() + cases := []struct { + name, src string + want sniffProbe + }{ + {"flow style", `{"openapi":"3.1.0"}`, sniffProbe{OpenAPI: "3.1.0"}}, + {"flow style, space around the colon", `{"openapi" : "3.1.0"}`, sniffProbe{OpenAPI: "3.1.0"}}, + {"flow style, broken before the key", `{"a":1,"b" 2,"openapi":"3.1.0"}`, sniffProbe{OpenAPI: "3.1.0"}}, + {"block style", "openapi: 3.1.0\n", sniffProbe{OpenAPI: "3.1.0"}}, + {"block style, quoted", "openapi: \"3.1.0\"\n", sniffProbe{OpenAPI: "3.1.0"}}, + {"block style, single quoted", "openapi: '3.1.0'\n", sniffProbe{OpenAPI: "3.1.0"}}, + {"block style, trailing comment", "openapi: 3.1.0 # the version\n", sniffProbe{OpenAPI: "3.1.0"}}, + {"block style, no trailing newline", "openapi: 3.1.0", sniffProbe{OpenAPI: "3.1.0"}}, + {"both keys, whichever order", `{"swagger":"2.0","openapi":"3.1.0"}`, + sniffProbe{OpenAPI: "3.1.0", Swagger: "2.0"}}, + + // A version that is not a quoted scalar declares no dialect, and must not + // be read as one by accident. + {"flow style, non-string version", `{"openapi":3}`, sniffProbe{}}, + {"the name has no colon after it", `{"openapi","3.1.0"}`, sniffProbe{}}, + // Depth is what makes an entry the document's own. + {"nested one level down", `{"a":{"openapi":"3.1.0"}}`, sniffProbe{}}, + {"a document that opens a sequence", `[{"openapi":"3.1.0"}]`, sniffProbe{}}, + {"block style, indented under another key", "a:\n openapi: 3.1.0\n", sniffProbe{}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, scanProbe([]byte(tc.src))) + }) + } +} + +// TestDetect_TheCapBoundaryReadsTheSameBothWays pins the seam. The cap decides +// which of two readings answers — an exact parse at or below it, a scan above — +// and a document does not change format by growing one byte. The boundary is +// where an off-by-one in the comparison hides, and either reading alone still +// looks right from the other side of it. +func TestDetect_TheCapBoundaryReadsTheSameBothWays(t *testing.T) { + t.Parallel() + want := compilers.SourceFormat{Name: "openapi", Version: "3.1"} + for _, delta := range []int{-1, 0, 1} { + t.Run(fmt.Sprintf("cap%+d", delta), func(t *testing.T) { + t.Parallel() + head := "openapi: 3.1.0\n" + src := head + "#" + strings.Repeat("p", maxSniffBytes+delta-len(head)-2) + "\n" + require.Len(t, src, maxSniffBytes+delta, "the case must sit exactly on the boundary") + + got, diags, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(src)}) + assert.True(t, ok) + assert.Equal(t, want, got, "one byte of padding does not change what a document declares") + assert.Nil(t, codesOf(diags)) + }) + } +} + +// TestDetect_TheCapDecidesWhichReadingAnswers pins the comparison itself. The +// two readings agree on most documents either can read — every shape they do +// not is declared in TestReadings_AgreeExceptWhereDeclared — so a document that +// both read alike cannot tell them apart and an off-by-one at the cap hides +// behind that agreement. A broken document is where they differ and must: the parse at or +// below the cap has read the whole thing and can say it is this compiler's and +// unreadable, while the scan above it has read one key and cannot tell a broken +// spec from another format's file, so it declines rather than guess. +func TestDetect_TheCapDecidesWhichReadingAnswers(t *testing.T) { + t.Parallel() + broken := func(size int) []byte { + head := "openapi: [unterminated\n" + src := head + "#" + strings.Repeat("p", size-len(head)-2) + "\n" + require.Len(t, src, size) + return []byte(src) + } + cases := []struct { + name string + size int + wantCode []string + }{ + {"at the cap, parsed exactly", maxSniffBytes, []string{diag.UndecodableSource}}, + {"one byte past the cap, scanned", maxSniffBytes + 1, nil}, + } + 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: broken(tc.size)}) + assert.False(t, ok, "neither reading finds a version in a document broken before one") + assert.Equal(t, compilers.SourceFormat{}, got) + assert.Equal(t, tc.wantCode, codesOf(diags)) + }) + } +} + +// TestDetect_AByteOrderMarkIsNotAFormat pins the seam at the size it bites. A +// mark before the first byte is invisible to the parse below the cap and was +// fatal to the scan above it, so the same document read as OpenAPI at 64 KiB and +// as nothing at all one byte past — on a file no editor thinks is unusual. +func TestDetect_AByteOrderMarkIsNotAFormat(t *testing.T) { + t.Parallel() + want := compilers.SourceFormat{Name: "openapi", Version: "3.1"} + for name, body := range map[string]string{ + "block style": "openapi: 3.1.0\n#" + flowPad() + "\n", + "flow style": `{"openapi":"3.1.0","pad":"` + flowPad() + `"}`, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + src := "\xef\xbb\xbf" + body + require.Greater(t, len(src), maxSniffBytes, "the case must exceed the cap to test the scan") + got, diags, ok := New().Detect(compilers.Source{Path: "spec.yaml", Data: []byte(src)}) + assert.True(t, ok, "a byte-order mark is not part of what a document declares") + assert.Equal(t, want, got) + assert.Nil(t, codesOf(diags)) + }) + } +} + +// readingsRow is one document read both ways. want is what both readings return +// absent a declared loss; declared is why the scan reads less than the parse, or +// "" for a row that must read the same both ways; scan is what the scan returns +// instead, when declared. +type readingsRow struct { + name, src string + want sniffProbe + declared string + scan sniffProbe +} + +// agreeingReadings are the shapes both readings must answer alike. The document +// markers are here in every position a line scan can get wrong, and the shapes +// the scan used to read that the parse never did. +func agreeingReadings() []readingsRow { + const v = "3.1.0" + return []readingsRow{ + {name: "block style", src: "openapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "flow style", src: `{"openapi":"3.1.0"}`, want: sniffProbe{OpenAPI: v}}, + {name: "CRLF line endings", src: "openapi: 3.1.0\r\ninfo: {}\r\n", want: sniffProbe{OpenAPI: v}}, + {name: "a tab after the colon", src: "openapi:\t3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "a byte-order mark, block style", src: "\xef\xbb\xbfopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "a byte-order mark, flow style", src: "\xef\xbb\xbf{\"openapi\":\"3.1.0\"}", want: sniffProbe{OpenAPI: v}}, + {name: "an explicit document start", src: "---\nopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "a comment before the document start", src: "# spec\n---\nopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + // 1.1, because yaml.v3 refuses a 1.2 directive outright and the loader + // shares that parser: such a document is undecodable at any size, so the + // two readings cannot disagree on its format. + {name: "a directive before the document start", src: "%YAML 1.1\n---\nopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "a flow document after the marker", src: "--- {\"openapi\":\"3.1.0\"}\n", want: sniffProbe{OpenAPI: v}}, + {name: "a flow document on the line after the marker", src: "---\n{\"openapi\":\"3.1.0\"}\n", want: sniffProbe{OpenAPI: v}}, + {name: "an end marker after the key", src: "openapi: 3.1.0\n...\n", want: sniffProbe{OpenAPI: v}}, + {name: "a plain scalar opening with dashes", src: "---- : 1\nopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "a key opening with one dash", src: "-x: 1\nopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + {name: "a key opening with one dot", src: ".x: 1\nopenapi: 3.1.0\n", want: sniffProbe{OpenAPI: v}}, + // A root sequence is no mapping, so neither reading finds a key in it: + // the parse refuses the root, and `- openapi` is not the name. + {name: "a root sequence", src: "- openapi: 3.1.0\n"}, + // Bytes the compile never parses cannot name its format: load reads the + // first document only. + {name: "the key in a second document", src: "kind: Foo\n---\nopenapi: 3.1.0\n"}, + {name: "the key after an end marker", src: "kind: Foo\n...\nopenapi: 3.1.0\n"}, + {name: "the key after a block scalar the marker ends", src: "text: |\n line\n---\nopenapi: 3.1.0\n"}, + // `openapi:3.1.0` is a plain scalar, not a key: the parse refuses the + // document for having a string at its root, and the scan reads no entry. + {name: "no space after the colon", src: "openapi:3.1.0\n"}, + {name: "the name as a value", src: `{"note":"openapi"}`}, + } +} + +// declaredReadings are the shapes the scan reads less than the parse, each +// against its reason. A tolerance added to the scan deletes a row from here, and +// a shape it stops reading adds one — neither can happen silently. +func declaredReadings() []readingsRow { + const v = "3.1.0" + return []readingsRow{ + {name: "a root merge key", src: "base: &b\n openapi: 3.1.0\n<<: *b\n", want: sniffProbe{OpenAPI: v}, + declared: "following `<<` means resolving an anchor, which means the parse the cap " + + "exists to avoid — and one an untrusted source would choose"}, + {name: "a quoted key in block style", src: "\"openapi\": 3.1.0\n", want: sniffProbe{OpenAPI: v}, + declared: "the quoted spelling is how flow style writes every key; admitting it at " + + "column 0 reopens the guard for zero-indent JSON at every depth"}, + {name: "an unquoted key in flow style", src: "{openapi: 3.1.0}", want: sniffProbe{OpenAPI: v}, + declared: "the flow scan is a JSON lexer, and JSON quotes every key"}, + {name: "an anchor before the version", src: "openapi: &v 3.1.0\n", want: sniffProbe{OpenAPI: v}, + declared: "the scan reads the scalar as written, and `&v 3.1.0` is no version", scan: sniffProbe{OpenAPI: "&v 3.1.0"}}, + {name: "a tag before the version", src: "openapi: !!str 3.1.0\n", want: sniffProbe{OpenAPI: v}, + declared: "the scan reads the scalar as written, and `!!str 3.1.0` is no version", scan: sniffProbe{OpenAPI: "!!str 3.1.0"}}, + } +} + +// TestReadings_AgreeExceptWhereDeclared holds the two readings to one answer. +// The cap decides which of them runs, so every shape they disagree on is a +// document that names one format below 64 KiB and another above it — and the +// committed corpus cannot see any of it, its largest spec being a few KiB. +// +// declaredReadings is the whole of the list of shapes the scan does not read; +// every other row must read the same both ways. The two halves are asserted in +// opposite directions, so a declared row whose scan catches up fails until its +// reason is deleted, and an undeclared row the scan stops reading fails until +// one is written. +func TestReadings_AgreeExceptWhereDeclared(t *testing.T) { + t.Parallel() + for _, tc := range agreeingReadings() { + t.Run("agree/"+tc.name, func(t *testing.T) { + t.Parallel() + require.Empty(t, tc.declared, "an agreeing row carries no reason") + parsed, _ := decodeYAML([]byte(tc.src)) + assert.Equal(t, tc.want, parsed, "the parse reads what the row says") + assert.Equal(t, tc.want, scanProbe([]byte(tc.src)), "the scan reads what the parse reads") + }) + } + for _, tc := range declaredReadings() { + t.Run("declared/"+tc.name, func(t *testing.T) { + t.Parallel() + require.NotEmpty(t, tc.declared, "a declared loss carries its reason") + parsed, err := decodeYAML([]byte(tc.src)) + require.NoError(t, err, "the loss is the scan's alone: the parse reads the document") + assert.Equal(t, tc.want, parsed, "the parse reads what the row says") + assert.Equal(t, tc.scan, scanProbe([]byte(tc.src)), "the scan reads what the row declares it reads") + assert.NotEqual(t, tc.want, tc.scan, "the scan now reads this shape, so the reason (%s) is stale: "+ + "move the row to agreeingReadings", tc.declared) + }) + } +} diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index 433d7d78..9f8d1336 100644 --- a/compilers/openapi/detect_test.go +++ b/compilers/openapi/detect_test.go @@ -67,19 +67,40 @@ func TestDetect_Formats(t *testing.T) { // key, so the parse error describes a parser that was wrong to be asked. {"unparseable, key only mentioned", "svc.proto", "syntax = \"openapi\";\n{[", compilers.SourceFormat{}, false, nil}, - // Past the sniff cap and still this compiler's: the key it declares is in - // the prefix, so the fast path alone is enough to call it broken rather - // than somebody else's. + // Past the cap, where detection scans rather than parses, and the key it + // writes has no version beside it. A scan cannot tell that from another + // format's file naming the word, and claiming the wrong one of those two + // is the costlier mistake, so it declines and the caller is told the + // format was not recognized. {"unparseable past the cap", "api.yaml", padTo("openapi: [unterminated\n", "filler: x\n"), - compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, + compilers.SourceFormat{}, false, nil}, // Declares the key only past the cap, on a prefix that does not parse. The - // key search reads every byte, so the declaration is found and the source - // is this compiler's own — broken, and said so, rather than declined as - // somebody else's for want of looking. + // scan reads every byte, so the version is found and the format named; that + // the bytes around it will not parse is the compile's finding to report, + // where the parse that discovers it is one the loader had agreed to pay + // for. See TestCompile_AnUnreadableSourceIsADiagnostic. {"key past the cap on an unparseable prefix", "api.yaml", padTo("bad: [unterminated\n", "filler: x\n") + "openapi: 3.1.0\n", - compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, + compilers.SourceFormat{Name: "openapi", Version: "3.1"}, true, nil}, + // The same case in flow style, which is what the motivating spec is written + // in. A JSON document has no line structure to cut at, and the scan needs + // none: it tracks nesting through bytes a parser stops at. + {"key past the cap on an unparseable flow prefix", "spec3.json", + `{"pad":"` + flowPad() + `","bad" 1,"openapi":"3.1.0"}`, + compilers.SourceFormat{Name: "openapi", Version: "3.1"}, true, nil}, + // Another format's document, past the cap, naming the word as a key and + // broken besides. It opens no mapping of its own, so the key is not its + // declaration of itself and this compiler has nothing to say: reporting a + // parse error here would claim bytes that were never its own. + {"a broken document of another format names the key", "asyncapi.json", + `[{"openapi":"3.1.0"},"` + flowPad() + `"`, + compilers.SourceFormat{}, false, nil}, + // The same, one level down inside a mapping that does open the document. + // A nested key names a field, not the format of the file holding it. + {"a broken document of another format nests the key", "other.json", + `{"pad":"` + flowPad() + `","deep":{"openapi":"3.1.0"},"bad" 1}`, + compilers.SourceFormat{}, false, nil}, {"empty", "empty.yaml", "", compilers.SourceFormat{}, false, nil}, } for _, tc := range cases { @@ -140,6 +161,10 @@ func bigComponents() (flow, block string) { return f.String(), b.String() } +// flowPad returns a run of bytes long enough that a flow entry holding it puts +// everything after it past the sniff cap. +func flowPad() string { return strings.Repeat("p", maxSniffBytes) } + // padTo returns src grown past the sniff cap by appending filler, so sniff reads // a prefix first rather than decoding the source whole on sight. func padTo(src, filler string) string { @@ -151,12 +176,11 @@ func padTo(src, filler string) string { return b.String() } -// TestSniff_BeyondTheCap pins both paths a document larger than the cap can -// take. The prefix answers on its own whenever it names a key, in whichever -// style the document is written; when it names neither, a document whose bytes -// name one further in is read whole rather than declined, because where a writer -// put a key in a mapping says nothing about what the document is. Bytes that -// name neither key anywhere never leave the prefix. +// TestSniff_BeyondTheCap pins the reading a document larger than the cap gets: +// a scan that finds a key wherever it sits, in whichever style the document is +// written, because where a writer put a key in a mapping says nothing about what +// the document is. Bytes that name neither key are declined in silence — the +// scan has read one key and cannot tell a broken spec from another format's file. func TestSniff_BeyondTheCap(t *testing.T) { t.Parallel() const filler = "# a line of padding that says nothing about the format\n" @@ -181,9 +205,8 @@ func TestSniff_BeyondTheCap(t *testing.T) { // fails, and the answer is silence rather than a parser's complaint. {"protobuf past the cap", padTo("syntax = \"proto3\";\n", "message M { string a = 1; }\n"), sniffProbe{}}, - // The word is there past the cap and is not a key, so the whole read is - // never reached — asserted on the guard itself below, since the probe a - // whole read would return here is the zero one either way. + // The word is there past the cap and is not a key: a value is not a + // declaration, and the flow scan reads names only where a colon follows. {"the word past the cap is not a key", `{"x":"` + pad + `","note":"openapi"}`, sniffProbe{}}, } @@ -198,92 +221,51 @@ func TestSniff_BeyondTheCap(t *testing.T) { } } -// TestDeclaresProbeKey_GuardsTheWholeRead pins the one decision that keeps a -// document of another format off the slow path: the whole of a source is scanned -// for a key, and only a declaration — the name with the colon that makes it one -// — counts as having found it. -func TestDeclaresProbeKey_GuardsTheWholeRead(t *testing.T) { +// TestDeclaresProbeKey_ScopesTheNameToItsOwnDocument pins the guard that decides +// whose bytes these are. A name followed by a colon is a key wherever it sits, +// so the scan has to say *whose* key: block style answers with column 0, flow +// style with the root mapping's own depth. Everything below is a document naming +// the word somewhere it does not declare this format, and the answer for each is +// no — a compiler that says otherwise reports its own parse error over a file +// that was never its own. +// +// None of the cases exceeds the cap, and none can: a reading only fails at or +// below it, scanProbe returning no error above. What the scan does past the cap +// with the same shapes is TestScanProbe_ReadsTheVersionBesideTheKey's and +// TestSniff_BeyondTheCap's. +func TestDeclaresProbeKey_ScopesTheNameToItsOwnDocument(t *testing.T) { t.Parallel() - pad := strings.Repeat("p", maxSniffBytes) cases := []struct { name, src string want bool }{ - {"declared past the cap in flow style", `{"x":"` + pad + `","openapi":"3.1.0"}`, true}, - {"declared past the cap in block style", "x: " + pad + "\nswagger: \"2.0\"\n", true}, - {"named past the cap as a value", `{"x":"` + pad + `","note":"openapi"}`, false}, - {"named past the cap in prose", "x: " + pad + "\n# openapi is a format\n", false}, + {"flow mapping declares it", `{"openapi":"3.1.0"}`, true}, + {"flow mapping declares swagger", `{"swagger":"2.0"}`, true}, + {"space around the mapping and the colon", " \n\t{\"openapi\" : \"3.1.0\"}", true}, + {"an escape hides no key from the scan", `{"a\"b":1,"openapi":"3.1.0"}`, true}, + {"block style at column 0", "openapi: 3.1.0\n", true}, + {"block style past a byte-order mark", "\xef\xbb\xbfopenapi: [unterminated\n", true}, + {"flow style past a byte-order mark", "\xef\xbb\xbf{\"openapi\":\"3.1.0\",", true}, + + {"nested one level down", `{"a":{"openapi":"3.1.0"}}`, false}, + {"nested inside a sequence", `{"a":[{"openapi":"3.1.0"}]}`, false}, + {"a document that opens a sequence", `[{"openapi":"3.1.0"}]`, false}, + {"block style indented under another key", "a:\n openapi: 3.1.0\n", false}, + {"the name is a value", `{"note":"openapi"}`, false}, + {"the name in a comment", "x: 1\n# openapi is a format\n", false}, + {"the name has no colon after it", `{"openapi",1}`, false}, + {"the name ends the bytes", `{"openapi"`, false}, + {"a string runs off the end", `{"a":"unterminated`, false}, + {"nothing but whitespace", " \n\t ", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - require.Greater(t, len(tc.src), maxSniffBytes, "the case must exceed the cap to test it") assert.Equal(t, tc.want, declaresProbeKey([]byte(tc.src))) }) } } -func TestDecodeFlowEntries_ReadsWhatTheCutLeft(t *testing.T) { - t.Parallel() - cases := []struct { - name, prefix string - want sniffProbe - wantFlow bool - }{ - {"complete document", `{"openapi":"3.1.0","info":{"title":"T"}}`, - sniffProbe{OpenAPI: "3.1.0"}, true}, - {"cut inside a later value", `{"openapi":"3.1.0","info":{"title":"T`, - sniffProbe{OpenAPI: "3.1.0"}, true}, - {"cut inside a key", `{"openapi":"3.1.0","inf`, - sniffProbe{OpenAPI: "3.1.0"}, true}, - {"swagger", `{"swagger":"2.0","info":{}}`, sniffProbe{Swagger: "2.0"}, true}, - // A version that is not a string declares no dialect, and must not be - // read as one by accident. - {"non-string version", `{"openapi":3}`, sniffProbe{}, true}, - {"no flow mapping", "openapi: 3.1.0\n", sniffProbe{}, false}, - {"not even a token", "\x00", sniffProbe{}, false}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - got, flow := decodeFlowEntries([]byte(tc.prefix)) - assert.Equal(t, tc.wantFlow, flow) - assert.Equal(t, tc.want, got) - }) - } -} - -// TestDecodeFlowEntries_StopsAtTheEntryCap proves the walk is bounded by its own -// count and not only by the byte cap: a declaration after maxSniffEntries other -// entries is not read. -func TestDecodeFlowEntries_StopsAtTheEntryCap(t *testing.T) { - t.Parallel() - var b strings.Builder - b.WriteByte('{') - for i := range maxSniffEntries + 1 { - if i > 0 { - b.WriteByte(',') - } - b.WriteString(`"k`) - b.WriteString(strings.Repeat("x", 3)) - b.WriteString(string(rune('a' + i%26))) - b.WriteString(strings.Repeat("y", i%7)) - b.WriteString(`":0`) - } - b.WriteString(`,"openapi":"3.1.0"}`) - - got, flow := decodeFlowEntries([]byte(b.String())) - require.True(t, flow) - assert.Equal(t, sniffProbe{}, got, "the entry past the cap is not read") -} - -func TestWholeLines(t *testing.T) { - t.Parallel() - assert.Equal(t, "a\nb\n", string(wholeLines([]byte("a\nb\nc")))) - assert.Equal(t, "nolines", string(wholeLines([]byte("nolines"))), - "a prefix with no newline has no better cut to make") -} - func TestMajorMinor(t *testing.T) { t.Parallel() assert.Equal(t, "3.1", majorMinor("3.1.0")) diff --git a/compilers/openapi/internal/load/entry_internal_test.go b/compilers/openapi/internal/load/entry_internal_test.go index 4616514b..70b40d04 100644 --- a/compilers/openapi/internal/load/entry_internal_test.go +++ b/compilers/openapi/internal/load/entry_internal_test.go @@ -37,7 +37,7 @@ func TestLoad_DegenerateCycleIsRefusedBeforeParsing(t *testing.T) { // TestLoad_UnparseableSourceIsAGoError pins the other side of that split: bytes // that are not a document at all are an I/O-level failure, so they leave as a Go // error naming the source rather than as a diagnostic about the spec. (The -// errParse sentinel is narrower — it marks only a recovered parser panic, which +// ErrParse sentinel is narrower — it marks only a recovered parser panic, which // TestUnmarshal_RecoversParserPanic covers.) func TestLoad_UnparseableSourceIsAGoError(t *testing.T) { t.Parallel() @@ -270,7 +270,7 @@ func TestLoad_ADocumentThatFailsToBuildIsAGoError(t *testing.T) { doc, diags, err := Load(t.Context(), 5, openapitest.SourceOf(" "), Options{}) require.Error(t, err) - assert.ErrorIs(t, err, errParse) + assert.ErrorIs(t, err, ErrParse) assert.Contains(t, err.Error(), "source 5", "the failing source is named") assert.Nil(t, doc) assert.Nil(t, diags) diff --git a/compilers/openapi/internal/load/load.go b/compilers/openapi/internal/load/load.go index ce80aa23..9904727e 100644 --- a/compilers/openapi/internal/load/load.go +++ b/compilers/openapi/internal/load/load.go @@ -84,9 +84,12 @@ func budgetRefusal(srcIndex int, format string, observed, limit int) ir.Diagnost ir.Provenance{Source: srcIndex}, format, observed, limit) } -// errParse marks a hard failure to parse a source document — an I/O- or -// programmer-level error, distinct from a spec problem reported as a diagnostic. -var errParse = errors.New("parse source") +// ErrParse marks a hard failure to read a source document: bytes that are not +// YAML, or that fault the parser. It is exported because the compiler above +// converts it into a diagnostic — a document that will not parse is a problem +// with the document, and engine.Run turns a Go error from a compiler into one of +// its own, which the CLI reports on the channel it uses for being invoked wrong. +var ErrParse = errors.New("parse source") // maxSchemaScanDepth bounds the scalar scan of a schema node (styleguide // bounded-recursion rule); a schema nested deeper is pathological, not a spec the @@ -486,7 +489,7 @@ func nodeCount(root *yaml.Node) int { func decode(data []byte) (*yaml.Node, error) { var root yaml.Node if err := yaml.Unmarshal(data, &root); err != nil { - return nil, fmt.Errorf("%w: %w", err, errParse) + return nil, fmt.Errorf("%w: %w", err, ErrParse) } return &root, nil } @@ -499,7 +502,7 @@ func decode(data []byte) (*yaml.Node, error) { // an overlay. // // It converts a panic from the third-party parser — which faults on degenerate -// input such as a whitespace-only document — into an errParse error, so the +// input such as a whitespace-only document — into an ErrParse error, so the // compiler upholds the no-panics-escape invariant instead of crashing the // caller's process. The named returns are reset in the recover so a // partially-assigned document never leaks. @@ -507,7 +510,7 @@ func unmarshal(ctx context.Context, data []byte, root *yaml.Node) (doc *soa.Open defer func() { if r := recover(); r != nil { doc, valErrs = nil, nil - err = fmt.Errorf("parser panicked (%v): %w", r, errParse) + err = fmt.Errorf("parser panicked (%v): %w", r, ErrParse) } }() if len(data) == 0 { @@ -541,7 +544,7 @@ func resolveAll(ctx context.Context, doc *soa.OpenAPI, opts soa.ResolveAllOption defer func() { if r := recover(); r != nil { resErrs = nil - err = fmt.Errorf("reference resolver panicked (%v): %w", r, errParse) + err = fmt.Errorf("reference resolver panicked (%v): %w", r, ErrParse) } }() return doc.ResolveAllReferences(ctx, opts) diff --git a/compilers/openapi/internal/load/load_internal_test.go b/compilers/openapi/internal/load/load_internal_test.go index 028278cb..c97a02eb 100644 --- a/compilers/openapi/internal/load/load_internal_test.go +++ b/compilers/openapi/internal/load/load_internal_test.go @@ -106,7 +106,7 @@ func parseSpec(t *testing.T, spec string) (*soa.OpenAPI, []error) { // TestUnmarshal_RecoversParserPanic pins the no-panics-escape invariant: the // third-party parser faults on a whitespace-only document, and unmarshal must -// convert that panic into an errParse error instead of letting it escape. +// convert that panic into an ErrParse error instead of letting it escape. // // The decode ahead of it succeeds — whitespace is well-formed YAML — so this // still lands in unmarshal rather than being caught a step earlier. @@ -117,7 +117,7 @@ func TestUnmarshal_RecoversParserPanic(t *testing.T) { doc, valErrs, err := unmarshal(t.Context(), []byte(" "), root) require.Error(t, err) - assert.ErrorIs(t, err, errParse) + assert.ErrorIs(t, err, ErrParse) assert.Nil(t, doc) assert.Nil(t, valErrs) } @@ -152,7 +152,7 @@ func TestResolveAll_RecoversResolverPanic(t *testing.T) { resErrs, err := resolveAll(t.Context(), doc, soa.ResolveAllOptions{}) require.Error(t, err) - assert.ErrorIs(t, err, errParse) + assert.ErrorIs(t, err, ErrParse) assert.Contains(t, err.Error(), "reference resolver panicked") assert.Nil(t, resErrs, "a partially-populated result never leaks") } diff --git a/compilers/openapi/openapi.go b/compilers/openapi/openapi.go index 441990b1..a9b25378 100644 --- a/compilers/openapi/openapi.go +++ b/compilers/openapi/openapi.go @@ -2,6 +2,7 @@ package openapi import ( "context" + "errors" "fmt" "github.com/dexpace/morphic/compilers" @@ -62,6 +63,15 @@ func (c *Compiler) Compile(ctx context.Context, sources []compilers.Source, opts return nil, nil, err } loadedDoc, diags, err := load.Load(ctx, rootSrcIndex, sources[0], loadOptions(formatOpts)) + if errors.Is(err, load.ErrParse) { + // Detection named this source's format by scanning for the key it + // declares, which is an answer a broken document gives as readily as a + // whole one. The parse that finds it broken is this one, so the complaint + // is this one's to carry — as a diagnostic, because a Go error here + // leaves engine.Run as a Go error and the CLI reads that as a misuse of + // itself rather than as a spec it could not read. + return nil, append(diags, undecodable(err)), nil + } if err != nil || loadedDoc == nil { return nil, diags, err } @@ -190,3 +200,16 @@ func loweringCtx(doc *load.Document, o Options) lowering.Ctx { return lowering.New(rootSrcIndex, doc.Doc, doc.Source, o.Grouping, limits, o.StreamingMedia, o.Promotions, doc.Overlay) } + +// undecodable reports a source this compiler recognized and could not read. +// +// NoSource, not source 0: the parse that failed is the one that would have built +// the document, so no document is returned and there is no source table for a +// provenance to index into. A Source of 0 against the nil document engine.Run +// hands on resolves to no path at all, so it would name nothing while claiming +// to. The loader's own message carries the position instead, which is the half +// of a location a reader can act on here. +func undecodable(err error) ir.Diagnostic { + return diag.Newf(ir.SeverityError, diag.UndecodableSource, ir.Provenance{Source: ir.NoSource}, + "source cannot be read: %s", diag.OneLine(err)) +} diff --git a/compilers/openapi/openapi_internal_test.go b/compilers/openapi/openapi_internal_test.go index 94bc8e5a..91bc56fe 100644 --- a/compilers/openapi/openapi_internal_test.go +++ b/compilers/openapi/openapi_internal_test.go @@ -13,6 +13,7 @@ import ( "github.com/dexpace/morphic/compilers/openapi/internal/diag" "github.com/dexpace/morphic/compilers/openapi/internal/lowering" "github.com/dexpace/morphic/compilers/openapi/internal/openapitest" + "github.com/dexpace/morphic/ir" ) func TestParse_UnsupportedVersion(t *testing.T) { @@ -24,11 +25,20 @@ func TestParse_UnsupportedVersion(t *testing.T) { assert.True(t, openapitest.HasDiag(diags, diag.UnsupportedVersion)) } +// TestParse_UnmarshalError pins where a document that will not parse is +// reported. It is a finding about the source, not a failure of the compiler: +// engine.Run turns a compiler's Go error into one of its own, and the CLI reads +// that as having been invoked wrong rather than as a spec it could not read. func TestParse_UnmarshalError(t *testing.T) { t.Parallel() - _, _, err := New().Compile(context.Background(), + doc, diags, err := New().Compile(context.Background(), []compilers.Source{openapitest.SourceOf("\t\t: : : not valid : yaml\n\x00")}, compilers.Options{}) - require.Error(t, err) + require.NoError(t, err) + assert.Nil(t, doc) + require.Len(t, diags, 1) + assert.Equal(t, diag.UndecodableSource, diags[0].Code) + assert.Equal(t, ir.NoSource, diags[0].Provenance.Source, + "no document comes back, so there is no source table for a Source of 0 to index into") } // TestRun_RegistryRefusalsAreSurfaced covers the reporting of an entry diff --git a/engine/engine_test.go b/engine/engine_test.go index ff7f1521..80a6b25a 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -662,3 +662,31 @@ func TestEngine_RunDiagnosticsAreOneLineEach(t *testing.T) { }) } } + +// TestEngine_RunReadsAVersionKeyPastTheSniffCap drives the whole pipeline over +// the shape that motivated the detection change: a JSON document whose version +// key sits behind an object too large to read on the fast path. Nothing else +// reaches that path from the outside — the largest spec in the corpus is a few +// kilobytes — so without this the scan ships covered only by tests that call +// detection directly, and a break between Detect and a compiled document would +// have nothing to fail. +func TestEngine_RunReadsAVersionKeyPastTheSniffCap(t *testing.T) { + t.Parallel() + var b strings.Builder + b.WriteString(`{"info":{"title":"T","version":"1"},"paths":{},"components":{"schemas":{`) + for i := 0; b.Len() <= 64<<10; i++ { + if i > 0 { + b.WriteByte(',') + } + fmt.Fprintf(&b, `"S%d":{"type":"object","description":"a schema"}`, i) + } + b.WriteString(`}},"openapi":"3.1.0"}`) + require.Greater(t, b.Len(), 64<<10, "the version key must sit past the cap to test it") + + eng, err := engine.New() + require.NoError(t, err) + res, err := eng.Run(t.Context(), writeNamed(t, "spec3.json", b.String()), engine.RunOptions{}) + require.NoError(t, err) + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: "3.1"}, res.Format) + require.NotNil(t, res.Document, "a document that declares its version last still compiles") +} diff --git a/internal/harness/internal_test.go b/internal/harness/internal_test.go index a215ae7e..6ed700c3 100644 --- a/internal/harness/internal_test.go +++ b/internal/harness/internal_test.go @@ -144,6 +144,24 @@ func TestDeterministic_MismatchIsReported(t *testing.T) { assert.Contains(t, detail, "IR JSON differs") } +// TestCheck_CompilerErrorIsAnErrorOutcome drives the arm that separates a +// compiler that could not run from a spec that was found wanting. It goes +// through the seam because the OpenAPI compiler no longer reaches it on a +// document that will not parse — that is a diagnostic now, and an ErrorDiag +// outcome — leaving cancellation and a caller's bad options as the live +// producers of a Go error here. +func TestCheck_CompilerErrorIsAnErrorOutcome(t *testing.T) { + orig := compile + t.Cleanup(func() { compile = orig }) + compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, errors.New("compile boom") + } + + r := Check(context.Background(), "spec", []byte("x")) + assert.Equal(t, OutcomeError, r.Outcome) + assert.Contains(t, r.Detail, "compile boom") +} + func TestCheck_CompilerPanicIsCaptured(t *testing.T) { orig := compile t.Cleanup(func() { compile = orig })