diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index 3f5a463e..07317796 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. @@ -98,39 +91,112 @@ func (*Compiler) Detect(src compilers.Source) (compilers.SourceFormat, []ir.Diag // 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") + 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+`"`)) +// 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. +func declaresBlockKey(data []byte, key string) bool { + name := []byte(key + ":") + return bytes.HasPrefix(data, name) || bytes.Contains(data, append([]byte("\n"), name...)) } -// 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 +// 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 +204,161 @@ 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. +func scanProbe(data []byte) sniffProbe { + var probe sniffProbe + scanBlockProbe(data, &probe) + scanFlowProbe(data, &probe) + return probe +} + +// 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); { + line := data[i:] + if j := bytes.IndexByte(line, '\n'); j >= 0 { + line, i = line[:j], i+j+1 + } else { + i = len(data) + } + if name, value, ok := bytes.Cut(line, []byte(":")); ok && isProbeName(name) { + setVersion(probe, name, blockValue(value)) + } + } +} + +// 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 +536,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..e7624dd6 --- /dev/null +++ b/compilers/openapi/detect_scan_test.go @@ -0,0 +1,200 @@ +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_SizeDoesNotDecideTheFormat 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. +func TestDetect_SizeDoesNotDecideTheFormat(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, "the same document names the same format at any size") + }) + } +} + +// 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 every document either can read, so a document that both +// can read 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)) + }) + } +} diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index 433d7d78..5e9696aa 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 { @@ -213,6 +238,13 @@ func TestDeclaresProbeKey_GuardsTheWholeRead(t *testing.T) { {"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}, + // A key, and still not this document's: it names a field of something + // nested, which says nothing about the format of the file around it. + {"named past the cap as a nested key", `{"x":"` + pad + `","in":{"openapi":"3.1.0"}}`, false}, + // A document that opens no mapping declares no top-level key at all, so + // whatever its members name, none of it is a declaration of this format. + {"named past the cap in a document that opens no mapping", + `[{"x":"` + pad + `"},{"openapi":"3.1.0"}]`, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -223,67 +255,43 @@ func TestDeclaresProbeKey_GuardsTheWholeRead(t *testing.T) { } } -func TestDecodeFlowEntries_ReadsWhatTheCutLeft(t *testing.T) { +// TestDeclaresProbeKey_ScopesTheNameToItsOwnDocument pins the half of 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. +func TestDeclaresProbeKey_ScopesTheNameToItsOwnDocument(t *testing.T) { t.Parallel() cases := []struct { - name, prefix string - want sniffProbe - wantFlow bool + name, src string + want 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}, + {"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}, + + {"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 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() - got, flow := decodeFlowEntries([]byte(tc.prefix)) - assert.Equal(t, tc.wantFlow, flow) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, declaresProbeKey([]byte(tc.src))) }) } } -// 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..25917196 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,11 @@ 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. It +// names the source rather than NoSource: by the time a parse has failed the +// source table exists, so the finding can point at the file it is about. +func undecodable(err error) ir.Diagnostic { + return diag.Newf(ir.SeverityError, diag.UndecodableSource, ir.Provenance{Source: rootSrcIndex}, + "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..ebbec3e2 100644 --- a/compilers/openapi/openapi_internal_test.go +++ b/compilers/openapi/openapi_internal_test.go @@ -24,11 +24,17 @@ 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) + assert.True(t, openapitest.HasDiag(diags, diag.UndecodableSource)) } // TestRun_RegistryRefusalsAreSurfaced covers the reporting of an entry diff --git a/docs/ir-design.md b/docs/ir-design.md index e559000c..bb53af0b 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -1850,8 +1850,10 @@ this from `Unmodeled` and no two derive it differently: 4. **A node with no `Provenance` is not promoted into.** A node carrying a `Deprecation` and no provenance could not satisfy rule 3, and a heuristic that cannot be audited is worse than an empty field. Giving such a node a provenance is a change to this document, and the promotion - follows it rather than preceding it — which is the order `Parameter` went through: it was the - instance this rule named until it gained the `Provenance` §7.2 now gives it. + follows it rather than preceding it — which is the order `Parameter` went through, and it held + this rule's only named instance until it gained the `Provenance` §7.2 now gives it. `Variant` + (§4.4) and `EnumMember` (§4.5) are the instances today: each carries a `Deprecation` and no + provenance of its own, so no key maps into either until one of them gains one. A value the mapped field cannot hold — anything but text, for the four `Deprecation` members — is reported and not coerced, since the document means something else by the key. Text of the right 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 })