Repository navigation
fix(compilers/openapi): read an unquoted date or << as a string - #1371
Open
OmarAlJarrah wants to merge 8 commits into
Open
OmarAlJarrah wants to merge 8 commits into
OmarAlJarrah wants to merge 8 commits into
Conversation
OpenAPI holds YAML to its JSON schema ruleset, where an unquoted date such as 2021-01-01 is a string. The YAML parser tags it a timestamp, and the parser library's model decodes no string from a timestamp, so every string field written that way lowered as left out: the document's title and version, a tag's name, an operation's ID and summary, a required property name, a parameter's name, an http scheme's token. A $ref so written read as no reference, so the object it stood for vanished with nothing reported. Nothing reported any of it, and two parameters named by different dates drew a false duplicate-parameter error. The load phase now tags such a scalar a string in the walk that already releases anchors, before the model is built, in the source and in each external document it prepares. A document the resolver parsed itself under a respelled URL is rebuilt when it holds a plain date, as when it holds an anchor. A scalar the source tags outright keeps its tag, so what an explicit !!timestamp or a local tag reads as stays undecided here.
The retag of plain dates reads the parser's tag, not the text, so every spelling yaml.v3 resolves as a timestamp is a string: a date with one-digit fields, a date and time with a T or a space, and a date under YAML's non-specific tag. Pin each, at the predicate and end to end, so a reading narrowed to one spelling fails.
A plain date, a !!binary and a local tag are where the parser library's model and the rule table part: the table reads any scalar as a string, and the model decodes none of these. The fixture set the raw-YAML reader rule names and the spellings a generator is told to write left all three out, so two defects of a security requirement sat at a plain date through grids of about 200 specs. Name the three in both rules.
A twin sweep over the corpus, each spec compiled with its string values swapped for a plain date and for the same date quoted, found one site the twin test did not name: on main a plain-date discriminator propertyName reads as empty, so the discriminator names no property and the IR fails ir/empty-prop-ref under --verify. The load phase's retag already reads it as written; hold it there.
17 tasks done
The YAML parser tags a plain `<<` as a merge wherever it stands, and the parser library's model decodes no string from a merge, as it decodes none from a timestamp. So a `<<` written as a value read as left out exactly as a plain date did: `title: <<` lowered with no name, and `$ref: <<` dropped the parameter it stood for with exit 0 and nothing reported, where `$ref: "<<"` is followed and reported unresolved. Only a mapping key merges. YAML 1.2, which OpenAPI recommends, has no merge or timestamp type, and its core schema reads both scalars as strings. The load phase's walk now judges each scalar from its parent, so it knows a key from a value, and tags a string any date written with no tag and any plain `<<` but a key (mistagged). A `<<` key still merges, and a scalar tagged outright keeps its tag. The documents stated the reading as the JSON schema ruleset's. That ruleset calls a plain scalar matching none of its patterns an error (YAML 1.2.2 §10.2.2); it is the core schema (§10.3.2) that reads one as a string, and OpenAPI's limit on tags leaves !!str the one tag that holds it. The yaml-decoding and undecoded-scalar entries now say so. The twin test's rows run again with each date written `<<`, but those writing a date as a key, where `<<` merges. The undecoded-scalar canary pins that the model reads a plain `<<` as it reads a plain date.
The validation oracle's plain-date reason said OpenAPI holds YAML to its JSON schema ruleset and so reads a plain date as a string. That ruleset reads such a scalar as an error; YAML 1.2's core schema reads it as a string, and OpenAPI's limit on tags leaves !!str the one tag that holds it. The reason now says that. The references Status named only a local tag among the $ref spellings the model decodes no string from. A $ref written `!!binary aGk=` or `!!timestamp 2021-01-01` drops its object with nothing reported too, so the line names every tag outside the JSON schema ruleset, as the yaml-decoding Status does. A security Status line left unwrapped is rewrapped.
The parser tags a plain `<<` a merge wherever it stands, and the model decodes no string from it, so a `<<` written as a value reads as left out just as a plain date does. The two skill rules naming the scalars the parser tags as no string listed a plain date, a !!binary and a local tag, and a fix for dates built from them left the `<<` value standing. Name it beside the plain date in both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
YAML 1.2, which OpenAPI recommends (3.1.2 §Format), has no timestamp type: its core schema reads an unquoted date such as
2021-01-01as a string, and OpenAPI's limit on tags ("Tags MUST be limited to those allowed by YAML's JSON schema ruleset") leaves!!strthe one tag that can hold it. The YAML parser tags it!!timestamp, and the parser library's model decodes no string from a timestamp (docs/parser-library.md#undecoded-scalar). So every string field written that way lowered as if it were left out, with exit 0 and nothing reported:info.title,info.versionnameandversionabsentname, an operation'stagsg/tagswith the empty nameg/tags/2021-01-01operationId,summaryget_b, no summaryrequired: [2021-01-01]title, a response'sdescription$refon a path item, parameter, request body, response, header, link, callback or 3.2 content entry$ref, and reported asopenapi/unresolved-refwhen it reaches nothingsummary$ref, with no collision reportedopenapi/duplicate-parametererror, and IDs that spell no namehttpscheme'sschemeopenapi/incomplete-security-schemeerror, and the scheme lowers asunresolvedcustomscheme with that tokenpropertyName--verifyrefuses asir/empty-prop-refA
<<written as a value failed the same way. The parser tags a plain<<!!mergewherever it stands, and the model decodes no string from a merge, sotitle: <<lowered with no name and$ref: <<dropped the object it stood for with exit 0, where$ref: "<<"is followed and reported asopenapi/unresolved-ref. Only a mapping key merges, and YAML 1.2 reads<<as a string.The load phase now tags each such scalar
!!strbefore the model is built: any date written with no tag, and any plain<<but a mapping key (load.mistagged). This happens in the walk that already clears anchors, so it adds no walk;releaseAnchorsis renamedreadyForModelsince it now does both. The walk judges each scalar from its parent, which knows a key from a value, so a<<key still merges. The walk runs on the source after an overlay applies, which covers a date the overlay grafts, and on every external document the reader prepares. One route reached the model without that walk: an external document named by a URL thatnet/urlrespells is parsed by the resolver itself. That document was rebuilt only when its parse held an anchor. Now it is also rebuilt when the parse holds such a scalar (anyUnready).A default, enum member, example or extension already kept a date or a
<<as its text (value.Classify), so the retag changes nothing there.A scalar the source tags outright keeps its tag. Retagging an explicit
!!timestamp soonwould keep as text a default thatvaluesrefuses today, which is #245's question to answer. And!!binary aGk=means the byteshi, not the textaGk=. A field under an explicit tag outside the ruleset (!local,!!binary,!!timestamp) still reads as left out with nothing reported, the same as onmain. That is filed as #1368, and the rule Status lines and comments that cited this issue for local tags now cite #1368.nodeview.Strings,auth.unresolvableSchemeDiagsandauth.docsBesideread a requirement's scopes and a scheme's$reffrom the node. They stay, because they still serve those spellings.Exit status moves in both directions. A document whose
$refis written as a plain date or a plain<<now exits 1 where it exited 0, because the reference is followed and reported when it reaches nothing. A document that drew a falseduplicate-parameterorincomplete-security-schemeerror from a plain date now exits 0. The IR's shape andIRVersionare unchanged.Skills. Two skill rules list the spellings a reader of raw YAML is probed with, and neither named a scalar the parser tags as no string: a plain date, a plain
<<value, a!!binary, a local tag. That is where the model and the rule table part, and two defects in #1226 got through grids built from those lists.morphic-openapi-compiler'syaml-alias-anchor-merge-spellingsnow names them in its fixture set and its Probe, andmorphic-verifying-changes'generator-widthnow names them among the spellings a generator writes. The Probe is the comparison this PR's tests make: at each string position, diff the IR and diagnostics against the ordinary spelling.Landing beside other PRs. #1227 landed on
mainand conflicted with this change in one Status paragraph ofdocs/compilers/openapi.md#references, which both rewrite. The merge ofmainkeeps #1227's text with its sentence citing #1280 replaced by this change's, citing #1368. No open PR namesreleaseAnchors,anyAnchored,hasAnchororplainDate, which this change renames, none adds a spec writing a plain date or<<value, and none conflicts with this head that does not already conflict withmain.Closes #1280
Closes #1282
Test plan
e2e.TestCompile_APlainDateCompilesAsItsQuotedTwincompiles each site in the issue and its comments, anhttpscheme token, the other spellings yaml.v3 resolves as a timestamp (one-digit fields, a date and time with aTor a space), a value, an extension and a key. It requires the IR, diagnostics included, to equal that of a twin that quotes every date. Twins also cover a date in an external document, read from a file and over HTTP, and a date an overlay grafts.load.TestResolveExternal_RecoversAPlainDateBehindARespelledURLcovers the respelled URL.e2e.TestCompile_ADateTaggedOutrightKeepsItsTagchecks that an explicit!!timestamp soondefault is still refused with the warning it drew before.e2e.TestCompile_APlainMergeValueCompilesAsItsQuotedTwinruns the same rows with each date written<<, all but the three that write a date as a key, where<<merges, against a twin quoting each<<.load.TestUnreadyholds the rebuild gate to a<<value and item, and not to a<<key or a<<quoted or tagged outright. Theundecoded-scalarcanary pins that the model reads a plain<<as it reads a plain date.yaml-timestamp-scalarsconformance case now writes a plain-dateinfo.versionand operationsummary, so the harness sweep and the order oracle reach plain dates at string fields. The library's validator calls both type mismatches. They are recorded invalidation_disagreements.txtunder a new reason,plain-date, which cites §Format and YAML 1.2's core schema.plainDatealways false, rerun on the pushed tree, reddens 13 of the 15 twin rows, both external-document subtests, the overlay twin, theyaml-timestamp-scalarscase,TestUnreadyand the respelled-URL test. The value and key rows lowered alike onmainalready; they stay as guards that the retag changes nothing there.TaggedStylecheck reddensTestCompile_ADateTaggedOutrightKeepsItsTagandTestUnready.TestUnreadyand the respelled-URL test.<<arm always false reddens 11 of the 12<<twin rows andTestUnready's value and item rows; the value row lowered alike already.<<key too reddens theyaml-merge-keysconformance case,TestUnready's merge-key row, and tests reading merged schemas, servers and media types.<<twin rows,TestUnreadyand theyaml-merge-keyscase.<<only inside a mapping reddens the<<twin row for an operation'stagsandTestUnready's item row.mainwas merged in, dropping the date arm, the<<arm or the whole predicate still reddens the tests above.main@3188471d against this branch): every YAML spec intestdataand the reference corpus, 1,758, compiled on both. Exit codes match on every spec (1,418 exit 0, 340 exit 1 on each side). Two specs differ, each only where a date becomes text: theyaml-timestamp-scalarscase gains itsversionandsummary, and one reference spec gainsversion: "2021-03-25", which it writes unquoted atinfo.version. No spec in either writes a plain<<value, and JSON writes no plain scalar of either kind. At 55716bf,.claude/skills/morphic-verifying-changes/scripts/sweep.pyover 2,181 specs, JSON included, againstmain@6996ce45 matched exit codes on every spec (1,661 exit 0, 520 exit 1 on each side) with the same two differences, and the case's dates as defaults, enum members, examples and extensions lowered the same on both sides.cloudflare/api-schemas@c312f753, compiled with--verifyat 55716bf: exit 1 on both builds, with the same IR byte for byte pastsources, and the same 1,862 diagnostic lines. It is JSON, so it writes no plain date or<<.openapi,type,in,style,format,$refand the schema identifiers) replaced by an unquoted2021-01-01, the other with the same date quoted: 2,177 pairs and 367,785 values. On this branch the two halves of every pair agree in exit code, IR and diagnostics. Onmain, 2,075 pairs differ, and 353 plain halves break an IR invariant under--verify, every one of themir/empty-prop-reffrom a discriminator'spropertyName, the site that sweep added to the table and the twin test. The 4 specs left over write no replaceable value or do not parse (a stream whose first document is empty, a deliberately malformed spec).morphic-harnessat 55716bf, overtestdata(227 files) and the reference corpus (2,079): line for line the same outcomes onmainand this branch. Over the twins on this branch, each plain half gets the verdict its quoted half does, the paths it names aside, on all 2,173 pairs it reads; 678 plain halves pass every oracle, determinism, idempotence and order-invariance among them, and the rest stop at an error diagnostic the replacement causes in both halves.main@3188471d and this head run alternately on one thread with the collector off, 7 runs a side, on a 722 KB YAML spec from the reference corpus: +0.05% at the median against a 0.3% range on each side. At 55716bf, the golden spec as JSON (27 MB) and as block YAML (21.7 MB), too large to hold with the collector off, read −1.2% and −0.8% with each side's range at 1–9%, which is noise either way. A document read under a respelled URL that writes a plain date or<<value anywhere, a value included, now costs a second resolution pass, as one writing an anchor already did.go test ./...andmake fmt vet lintpassed locally. On the final tree,go build ./...,go vet ./compilers/openapi/...,internal/archtest, the plain-date,<<, lowering-spec, conformance and external-resolution tests, andskills_lint.pypass; the two skill rules sit within the 150-word cap. The full gate, coverage included, is left to CI.