diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index a5befa0a..c54fc80d 100644 --- a/compilers/openapi/detect.go +++ b/compilers/openapi/detect.go @@ -11,15 +11,20 @@ import ( "github.com/dexpace/morphic/ir" ) -// maxSniffBytes bounds the bytes Detect parses. 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. +// 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. const maxSniffBytes = 64 << 10 -// maxSniffEntries bounds the top-level entries read from a flow-style prefix. -// The keys being looked for are declared among a document's first few, and a -// prefix full of nothing else is not one this compiler will take. +// 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 // sniffProbe holds the two discriminating top-level keys. Which one is present @@ -66,16 +71,16 @@ 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 that -// will not parse from one of another format that was never its business: a -// parse failure alone says only "not YAML", which a Protobuf or Smithy source -// is not either. +// 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. // -// Only the bounded prefix is read, for the reason sniff bounds its own reads. +// 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. func declaresProbeKey(data []byte) bool { - if len(data) > maxSniffBytes { - data = data[:maxSniffBytes] - } return declaresKey(data, "openapi") || declaresKey(data, "swagger") } @@ -110,26 +115,58 @@ func followedByColon(data, name []byte) bool { } } -// sniff reads the discriminating keys out of at most maxSniffBytes bytes, and -// returns the zero probe and the parser's error for anything it cannot read. -// Whether that error is worth reporting is Detect's question, not this one's: -// here it is only the record of what happened. +// sniff reads the discriminating keys out of data, and returns the zero probe +// and the parser's error for anything it cannot read. Whether that error is +// 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 -// decoded from a prefix, which 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. +// 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. func sniff(data []byte) (sniffProbe, error) { if len(data) <= maxSniffBytes { return decodeYAML(data) } - prefix := data[:maxSniffBytes] - if probe, ok := decodeFlowPrefix(prefix); ok { + + probe, err := sniffPrefix(data[:maxSniffBytes]) + if probe.OpenAPI != "" || probe.Swagger != "" { + return probe, nil + } + if declaresProbeKey(data) { + return sniffWhole(data) + } + return probe, err +} + +// 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 } return decodeYAML(wholeLines(prefix)) } +// 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. +// +// 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 + } + return decodeYAML(data) +} + // decodeYAML reads the probe keys from a complete YAML (or JSON, its subset) // document. func decodeYAML(data []byte) (sniffProbe, error) { @@ -140,12 +177,13 @@ func decodeYAML(data []byte) (sniffProbe, error) { return probe, nil } -// decodeFlowPrefix reads the top-level entries of a prefix that opens a flow -// mapping, and reports whether it was one. 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 decodeFlowPrefix(prefix []byte) (sniffProbe, bool) { - dec := json.NewDecoder(bytes.NewReader(prefix)) +// 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 diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index c2bc3063..7523650e 100644 --- a/compilers/openapi/detect_test.go +++ b/compilers/openapi/detect_test.go @@ -1,6 +1,7 @@ package openapi import ( + "fmt" "strings" "testing" @@ -65,19 +66,19 @@ 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 search reads the - // same bounded prefix the decode did, so a document too large to parse in - // full is still recognized as broken rather than as somebody else's. + // 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. {"unparseable past the cap", "api.yaml", padTo("openapi: [unterminated\n", "filler: x\n"), compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, // Declares the key only past the cap, on a prefix that does not parse. The - // key search reads the same bounded prefix the decode did and so does not - // see it either; claiming the source would assert something about bytes - // detection never read. + // 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. {"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, nil}, + compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, {"empty", "empty.yaml", "", compilers.SourceFormat{}, false, nil}, } for _, tc := range cases { @@ -92,8 +93,54 @@ func TestDetect_Formats(t *testing.T) { } } -// padTo returns src grown past the sniff cap by appending filler, so sniff takes -// its bounded-prefix path rather than decoding the source whole. +// TestDetect_KeyOrderDoesNotDecideTheFormat is the shape this bound was getting +// wrong: a published spec whose `components` object runs to megabytes and whose +// `openapi` key sits behind it. A JSON object's keys are unordered, so the same +// document written with its version key first and with it last is one document, +// and detection has to answer the same for both. Only the version-last spellings +// go past the prefix — they are the cases a prefix-only sniff declines. +func TestDetect_KeyOrderDoesNotDecideTheFormat(t *testing.T) { + t.Parallel() + flow, block := bigComponents() + cases := []struct{ name, path, src string }{ + {"flow json, version first", "spec3.json", `{"openapi":"3.0.3",` + flow + `}`}, + {"flow json, version last", "spec3.json", `{` + flow + `,"openapi":"3.0.3"}`}, + {"block yaml, version first", "api.yaml", "openapi: 3.0.3\n" + block}, + {"block yaml, version last", "api.yaml", block + "openapi: 3.0.3\n"}, + } + 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") + got, diags, ok := New().Detect(compilers.Source{Path: tc.path, Data: []byte(tc.src)}) + assert.True(t, ok, "a valid document must not be declined over where it declares its version") + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: "3.0"}, got) + assert.Nil(t, codesOf(diags), "a document this compiler recognizes carries no complaint") + }) + } +} + +// bigComponents returns a `components` entry whose value alone runs past the +// sniff cap, in flow and in block style. It stands in for the schema catalogue a +// published spec leads with; what matters is only that it is one entry too large +// to read past. +func bigComponents() (flow, block string) { + var f, b strings.Builder + f.WriteString(`"components":{"schemas":{`) + b.WriteString("components:\n schemas:\n") + for i := 0; f.Len() <= maxSniffBytes || b.Len() <= maxSniffBytes; i++ { + if i > 0 { + f.WriteByte(',') + } + fmt.Fprintf(&f, `"S%d":{"type":"object","description":"a schema"}`, i) + fmt.Fprintf(&b, " S%d:\n type: object\n description: a schema\n", i) + } + f.WriteString(`}}`) + return f.String(), b.String() +} + +// 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 { var b strings.Builder b.WriteString(src) @@ -103,13 +150,16 @@ func padTo(src, filler string) string { return b.String() } -// TestSniff_BeyondTheCap pins what the bound buys and what it costs. A document -// larger than the cap is read from its first maxSniffBytes in whichever style it -// is written, and a declaration past that point is not seen — detection stays -// flat in document size rather than paying a full parse to read two keys. +// 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. func TestSniff_BeyondTheCap(t *testing.T) { t.Parallel() const filler = "# a line of padding that says nothing about the format\n" + pad := strings.Repeat("p", maxSniffBytes) cases := []struct { name, src string want sniffProbe @@ -117,20 +167,24 @@ func TestSniff_BeyondTheCap(t *testing.T) { {"block yaml declaring first", padTo("openapi: 3.1.0\n", filler), sniffProbe{OpenAPI: "3.1.0"}}, {"block yaml declaring past the cap", - padTo("", filler) + "openapi: 3.1.0\n", sniffProbe{}}, + padTo("", filler) + "openapi: 3.1.0\n", sniffProbe{OpenAPI: "3.1.0"}}, {"flow json declaring first", - `{"openapi":"3.1.0","x":"` + strings.Repeat("p", maxSniffBytes) + `"}`, - sniffProbe{OpenAPI: "3.1.0"}}, + `{"openapi":"3.1.0","x":"` + pad + `"}`, sniffProbe{OpenAPI: "3.1.0"}}, {"flow json declaring past the cap", - `{"x":"` + strings.Repeat("p", maxSniffBytes) + `","openapi":"3.1.0"}`, - sniffProbe{}}, + `{"x":"` + pad + `","openapi":"3.1.0"}`, sniffProbe{OpenAPI: "3.1.0"}}, {"flow json swagger first", - `{"swagger":"2.0","x":"` + strings.Repeat("p", maxSniffBytes) + `"}`, - sniffProbe{Swagger: "2.0"}}, + `{"swagger":"2.0","x":"` + pad + `"}`, sniffProbe{Swagger: "2.0"}}, + {"flow json swagger past the cap", + `{"x":"` + pad + `","swagger":"2.0"}`, sniffProbe{Swagger: "2.0"}}, // Neither YAML nor JSON, and larger than the cap: the prefix is parsed, // 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 past the cap is not a key", + `{"x":"` + pad + `","note":"openapi"}`, sniffProbe{}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -143,7 +197,32 @@ func TestSniff_BeyondTheCap(t *testing.T) { } } -func TestDecodeFlowPrefix_ReadsWhatTheCutLeft(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) { + 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}, + } + 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 @@ -166,17 +245,17 @@ func TestDecodeFlowPrefix_ReadsWhatTheCutLeft(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got, flow := decodeFlowPrefix([]byte(tc.prefix)) + got, flow := decodeFlowEntries([]byte(tc.prefix)) assert.Equal(t, tc.wantFlow, flow) assert.Equal(t, tc.want, got) }) } } -// TestDecodeFlowPrefix_StopsAtTheEntryCap proves the walk is bounded by its own +// 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 TestDecodeFlowPrefix_StopsAtTheEntryCap(t *testing.T) { +func TestDecodeFlowEntries_StopsAtTheEntryCap(t *testing.T) { t.Parallel() var b strings.Builder b.WriteByte('{') @@ -192,7 +271,7 @@ func TestDecodeFlowPrefix_StopsAtTheEntryCap(t *testing.T) { } b.WriteString(`,"openapi":"3.1.0"}`) - got, flow := decodeFlowPrefix([]byte(b.String())) + got, flow := decodeFlowEntries([]byte(b.String())) require.True(t, flow) assert.Equal(t, sniffProbe{}, got, "the entry past the cap is not read") }