Skip to content

fix(compilers/openapi): keep undeclared keys on nested objects - #378

Open
OmarAlJarrah wants to merge 3 commits into
fix/openapi-extensions-everywherefrom
feat/openapi-census-remaining-carriers
Open

fix(compilers/openapi): keep undeclared keys on nested objects#378
OmarAlJarrah wants to merge 3 commits into
fix/openapi-extensions-everywherefrom
feat/openapi-census-remaining-carriers

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

This stacks on two open PRs and must merge after both. Its base is
fix/openapi-extensions-everywhere (#345), which adds the Unmodeled carriers
the objects below need, and it merges fix/openapi-unknown-key-census (#356),
which adds the census reader itself. #356 is a sibling of #345 rather than a
parent, so its commit appears in this branch's history; once both land the diff
here collapses to the last commit.

Summary

A key the OpenAPI model names no field for was kept at the objects that lower to
a node with an Unmodeled map of their own, and dropped in silence at the
objects nested inside one. Those had nowhere to land until the carriers went in.
This reads the census at each of them:

object carrier entry key
components document openapi:components/definitions
tag externalDocs document openapi:tags/0/externalDocs/title
operation externalDocs operation openapi:externalDocs/title
example ir.Example openapi:name
encoding owning content openapi:encoding/part/contentEncoding
oauth flows auth scheme openapi:flows/application
oauth flow ir.OAuthFlow openapi:scope
schema xml schema openapi:xml/attribute2
schema discriminator schema openapi:discriminator/mapping2
schema externalDocs schema openapi:externalDocs/title

Entries are keyed under the same scheme the extensions at those objects already
use — unscoped where the object has a map of its own, and scoped by the source
path from the carrier down where it rides on one. That is what lets a document
write title on its root, its tag's and its operation's externalDocs and get
three entries rather than one whose survivor depends on which lowering ran last;
the fixture writes exactly that, so the scoping is asserted rather than assumed.

The three schema sub-objects are graded as an OpenAPI object's keys (warning)
rather than as schema keywords (info), even though they hang off a schema: the
JSON Schema rule that an unrecognized keyword is legal governs the schema, and
xml, discriminator and externalDocs are OpenAPI objects the schema
vocabulary says nothing about.

One object was misclassified

components had been grouped with paths, responses and callbacks as a map
where every key is a valid entry and a census has nothing to say. It is not one:
only the map under each of its keys is the document's to name, while its own
key set is the fixed list of component kinds, which the library models as named
fields and takes a census over. Probing all four confirmed the split — the other
three fold an unrecognized key into themselves and report a type mismatch, and
their census is empty; components reports [definitions].

Two objects are deliberately left out

Both are stated at the code as well as here.

A path item (applyPathItem). The library folds a key it does not recognize
into the item's embedded operations map rather than recording it as undeclared,
so GetUnknownProperties returns nothing and there is no census to read —
confirmed at speakeasy-api/openapi v1.24.0, the version in go.mod, by reading
the census off the parsed model directly. Two things decide against the raw-node
read that would recover it:

  • The key is not lost in silence, which separates it from every object above.
    Folding it produces validation-type-mismatch at error severity naming the
    key at its own pointer, and that validation is unconditional. What is lost is
    the key's value, not the fact that it was written.
  • Recovering the value means reading the raw node against a path item's key
    vocabulary, and the only one this compiler owns is httpMethods — eight
    methods against the library's nine. It has no query, the method OpenAPI 3.2
    adds, and a 3.2 query operation sits in the same map as get, so the obvious
    reading would report a valid operation as an undeclared key. That vocabulary is
    what openapi: 3.2 additionalOperations are dropped entirely and silently #293 is about, so widening it here would settle that issue as a side
    effect of this one.

Filed as #377 with the reproduction and the options.

A Link Object, following the decision #345 records for its extensions. This
compiler lowers no Link Object anywhere: a response's links survive only as the
verbatim node kept on the response, and an unreferenced components/links entry
is dropped whole (verified: nothing of it appears in the compiled document). An
entry at one of the two positions would be the only trace of a construct the IR
does not model, while duplicating a value the node beside it already carries.

Test plan

  • testdata/openapi/unknown_keys.yaml gains one undeclared key per object above,
    each valued with the object it was written on and each a mistake real documents
    make — a Swagger 2.0 name (definitions for schemas, a flow named
    application), a neighbouring field (title on an externalDocs,
    contentEncoding on an encoding), or a field with the number wrong (scope).
    Living under testdata/ is what runs it through the harness oracles rather
    than through assertions alone; the sweep was confirmed to reach this file by
    breaking it and watching the sweep name it.
  • TestUnknownKeys_KeptAtEveryObject gains a row per object, each naming the
    Unmodeled map the entry must land on by the path the value-graph walk reaches
    it at, so an entry written to the wrong carrier fails rather than passing
    because the assertion looked only where it expected. The operation-externalDocs
    row names its carrier down to the operation, since the document writes that
    exact key too and the row is only evidence of the operation's if it cannot
    match the document's.
  • TestUnknownKeys_SchemaSubObjects covers the three schema sub-objects, which
    the fixture deliberately omits: the OpenAPI dialect meta-schema closes all three
    to anything but an x- key, so an undeclared key there draws a library
    validation error, and an error diagnostic stops harness.Check before the
    oracles that fixture exists to reach. Confirmed at both 3.0 and 3.1. The keys
    are kept and announced all the same, which is what the test pins.
  • Each site was proved to bite. The census was disabled at one object at a
    time and the suite re-run; all ten went red, each with the failing row naming
    that object: components, tag externalDocs, operation externalDocs, example,
    encoding, oauth flows, oauth flow, and — separately, one keyword at a time,
    since they share a reader — schema xml, discriminator and externalDocs.
  • The schema-sub-object rows use assert rather than require on the lookup, so
    a keyword missing from the map reports itself and leaves the other two still
    checked. With require the first failure aborted the rest, and a row that never
    runs is no evidence about the keyword it names.
  • Goldens regenerated with -update: nothing moved, including
    unwitnessed.golden.txt.
  • Full gate green: gofmt, go vet, golangci-lint (0 issues), go build, and
    coverage at 100%.

Closes #357

…s-remaining-carriers

The census this builds on lives on the #297 branch and the carriers it needs
live on the #345 branch, so this work stacks on both.
A key the OpenAPI model names no field for was kept at the objects that
lower to a node with an Unmodeled map of their own, and dropped in
silence at the objects nested inside one: an example, an encoding, the
oauth flows and each flow, a schema's xml, discriminator and
externalDocs, an operation's and a tag's externalDocs, and the
components object. Those had no Unmodeled map to land on until the
carriers went in; this reads the census at each of them and keys the
entries under the same scheme the extensions there already use, so an
entry says which object wrote it and two objects reaching one map cannot
collide.

The components object was classified as a map with nothing to census.
It is not: only the map under each of its keys is the document's to
name, while its own key set is the fixed list of component kinds, which
the library models as named fields and takes a census over. `paths`,
`responses` and a callback are the real maps of that kind, and each is
confirmed to fold an unrecognized key into itself rather than report it.

Two objects are deliberately left out, both recorded at the code:

- A path item. The library folds an unrecognized key into the item's
  embedded operations map, so there is no census to read; recovering the
  value needs a method vocabulary wider than the one this compiler owns,
  which is what #293 is about. Unlike every object above, the key is not
  lost in silence — folding it reports a type mismatch at error severity
  naming the key at its own pointer. Filed as #377.
- A Link Object, following the same decision made for its extensions.
  This compiler lowers no Link Object anywhere: a response's links
  survive only as a verbatim node, and an unreferenced components link
  is dropped whole.

A schema's xml, discriminator and externalDocs are censused but stay out
of the corpus fixture. The OpenAPI dialect meta-schema closes all three
to anything but an x- key, so an undeclared key there draws a library
validation error, and an error diagnostic stops harness.Check before the
oracles that fixture exists to reach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openapi: undeclared keys are still dropped at the objects with no Unmodeled carrier

1 participant