fix(compilers/openapi): detect a version key past the sniff cap

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

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,34 +150,41 @@ 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
 	}{
 		{"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")
 }