fix(compilers/openapi): detect a version key past the sniff cap - #435
fix(compilers/openapi): detect a version key past the sniff cap#435fuad-daoud wants to merge 1 commit into
Conversation
Detect capped its search for `openapi`/`swagger` at the first 64 KiB, so a valid document that writes a large object before its version key was reported as an unrecognized format. Stripe's published spec3.json is one: `components` runs to megabytes and `openapi` lands at byte 2,593,401. Mapping key order carries no meaning, so the same document with its keys the other way round compiled fine — the format answer depended on where a writer put a key. The 64 KiB prefix keeps its place as the fast path, and every document that declares a key there is still answered without a full parse. When the prefix declares neither key, a byte scan of the whole source decides whether to read it whole: only bytes that name `openapi:` or `swagger:` as a top-level key reach the parse, so a source of another format still gets the fast path's silence and never a complaint from this compiler. That scan is what Detect already used to tell its own broken source from another format's, and it is no longer bounded to the prefix either. A document whose prefix does not parse and whose declaration sits past the cap is now reported as an undecodable OpenAPI source rather than declined, which is the answer the surrounding rule always intended; detection now reads the bytes it would have had to read to say otherwise. Fixes #420 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
Wahbeh-Mohammad
left a comment
Wahbeh-Mohammad
left a comment
There was a problem hiding this comment.
Checked by running the code, not just reading it. The Stripe case works: real spec3.json goes from exit 1 on main to exit 0 here. One correction to the body: the note that the fix "missed JSON" is wrong, JSON is the case that works.
Four comments inline: the unbounded whole-file parse, other formats' files getting claimed, tests that stay green with the logic broken, and key order still deciding the result when both keys are present.
| // 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) { |
There was a problem hiding this comment.
Detection now reads and parses the whole file, with no limit.
Before this change Detect looked at the first 64 KiB and stopped. Now, when the prefix does not answer, sniffWhole parses the entire document with yaml.Unmarshal. That runs before the compiler's size and node budgets, and Detect has no context or options, so nothing can cancel or cap it.
Measured inside Detect, YAML with openapi at the end:
| Size | Time | Allocation |
|---|---|---|
| 8 MB | 256 ms | 181 MB |
| 32 MB | 944 ms | 719 MB |
The same 32 MB file through the CLI takes 4.2 s and is then refused by the node budget anyway, so detection paid for a parse the loader was about to reject. On main that file is declined in 11 ms.
Suggestion: don't parse the whole file to find one key. The byte scan in declaresKey already finds it; extend it to read the value beside it. For JSON, walk json.Decoder.Token() and track nesting depth so only depth-1 keys count. For block YAML, take the rest of the line after a column-0 openapi:. Linear, no allocation, and it also resolves the two comments below on other formats and on key order.
| // 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 { |
There was a problem hiding this comment.
Files from other formats now get claimed as broken OpenAPI.
declaresKey matches "openapi": at any depth and openapi: at the start of any line. On main this only saw 64 KiB; now it sees the whole file. A >64 KiB protobuf file with a nested "openapi": property, or a Markdown file with openapi: at column 0, now fails with:
error openapi/undecodable-source: source declares an OpenAPI or Swagger key and cannot be read: yaml: line 3: ...
On main the same files print unrecognized spec format; this build compiles openapi@3.0, ..., which is the message that helps. The comment on sniffWhole ("Nothing another format wrote reaches here") is not true, and the doc comments on declaresProbeKey and declaresKey say "top level" while the code has no depth check.
Suggestion: scope the scan to top level (see the comment on sniffWhole), and update the three comments.
| // 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) { |
There was a problem hiding this comment.
The tests pass with the logic broken.
I planted these one at a time and ran the detection tests; all stayed green:
- delete the
if declaresProbeKey(data)guard insniff - set
maxSniffEntries = 2 - change the loop to
for range maxSniffEntries + 1 - change
<=to<at the cap check insniff - remove the prefix fast path entirely (always parse whole)
- make the block-YAML prefix fallback return nothing
TestSniff_BeyondTheCap discards the error (probe, _ := sniff(...)), and this test only calls declaresProbeKey, so neither can see whether the whole read happened. There is also no test past Detect: nothing in testdata/ is over 64 KiB, and the harness calls Compile directly.
Suggestion: assert the error in the cases where it is the difference; add a case at exactly maxSniffBytes; put openapi at entry maxSniffEntries (must be read) and maxSniffEntries+1 (must not); add one engine.Run test with a generated >64 KiB JSON, version last.
| if probe, ok := decodeFlowPrefix(prefix); ok { | ||
|
|
||
| probe, err := sniffPrefix(data[:maxSniffBytes]) | ||
| if probe.OpenAPI != "" || probe.Swagger != "" { |
There was a problem hiding this comment.
Key order still decides the result when both keys are present.
{"swagger":"2.0", <64 KiB filler>, "openapi":"3.0.3"} short-circuits here on swagger and returns swagger@2.0, which this build then refuses. The same document under 64 KiB goes through decodeYAML, reads both keys, and returns openapi@3.0. Rare, but it is the exact property TestDetect_KeyOrderDoesNotDecideTheFormat claims. Goes away with the top-level scan suggested on sniffWhole.
|
Review done by me + fable |
3 participants
Closes #420. Not breaking.
Stripe's published
spec3.json— valid OpenAPI 3.0 — was rejected asengine/unrecognized-formatbecause detection sniffed only the first 64 KiB looking for a top-levelopenapikey, and Stripe puts a multi-megabytecomponentsobject first.openapilands at byte 2,593,401.The 64 KiB prefix scan stays as the fast path, extracted unchanged into
sniffPrefix. When the prefix declares neitheropenapinorswagger, a cheap whole-source byte scan decides whether the document is worth reading whole. A source of another format still gets the fast path's silence —detect.go's rule that this compiler must never claim or complain about another format's bytes is preserved.Verified end-to-end, not just at
Detect: a synthetic 279 KB spec withopenapiat byte 279,062 goesexit 1→exit 0through the real CLI.Stack 1 of 8. Base
main— review and merge bottom-up. Every commit here passedmake gatewhen it landed, and the full gate was re-run on the top of the stack. Run it asGOTOOLCHAIN=go1.26.3 make gate; this machine's Go 1.27 fails it for reasons unrelated to any change (#431).🤖 Generated with Claude Code
https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1