fix(compilers/openapi): scope the key scan, keep the read's error

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))
+		})
+	}
+}

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"))

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)