Skip to content

openapi: decide whether a scalar failing its own tag is refused or kept as text #245

Open

Description

@OmarAlJarrah

Summary

annotation.RawFromNode keeps a !!timestamp or !!binary scalar as the text the
source wrote, but still refuses one whose text does not satisfy the tag it declares:

x-when: !!timestamp notadate   # dropped, warning "extension could not be serialized"
x-blob: !!binary "###"         # dropped, same warning

The whole extension is discarded and the caller gets a warning diagnostic
(ExtensionsFrom, annotation.go).

That refusal is inherited rather than chosen. It exists because the conversion used
to decode these scalars to produce its output, so a decode failure had nowhere to
go. Nothing consumes the decoded value any more — the decode survives only as a tag
check — so the refusal is now the one thing standing between a construct the source
wrote and the IR.

Why it may be wrong

RawFromNode's documented rule is that it refuses what JSON cannot name: a
non-string mapping key, a key written twice, .nan/.inf. Both scalars above are
perfectly nameable in JSON — they are strings. They are refused for failing YAML
type resolution, which is a different rule, and one the sibling arm already declines
to apply: a tag yaml.v3 resolves no type for (!acme/thing) keeps its text rather
than being dropped, on the stated grounds that refusing would lose an extension value
the source did write.

Against that: dropping with a warning is louder than preserving silently, and a
!!binary "###" is a genuinely malformed construct that an author probably wants to
hear about. The IR has no channel for "kept, but suspect" — RawFromNode returns
(value, error) and the error path is what produces the diagnostic — so keeping the
text and keeping the warning is not currently expressible without threading a
diagnostic through the walk.

Decision needed

Which of "refuse and warn" or "preserve as text" is right for a scalar whose tag
promises a type its text does not hold, and whether the IR wants a way to say both.

Whatever is chosen, RawFromNode's doc comment states the current rule and must move
with it.

Context

Split out of #242, which settled what these scalars preserve and deliberately left
what they accept unchanged so the two questions could be answered separately. The
!!timestamp / !!binary arm of scalar in
compilers/openapi/internal/annotation/rawjson.go points here.

Activity

  1. added
    priority:P4Refactors, tests, CI, hostile-input hardening, cosmetic diagnostics, other formats
    on Oct 19, 2026
  2. OmarAlJarrah commented on Oct 22, 2026

    @OmarAlJarrah
    MemberAuthor

    The !!null arm of rawConv.scalar belongs to this decision too. It returns null without reading the text, so a !!null scalar whose text is not null converts without error. That is neither of the two outcomes weighed above: the text is neither refused nor kept, but replaced.

    openapi: 3.1.0
    info: {title: T, version: "1"}
    paths: {}
    x-flag: !!null abc

    morphic compile exits 0 with no diagnostics and keeps "openapi:x-flag": {"reason":"vendor_extension","value":null,...} (morphic @dfdadd5d). With !!bool abc in its place, the extension is dropped with warning openapi/degraded-construct ...: extension "x-flag" could not be serialized. yaml.v3 itself refuses the first: decoding a: !!null abc into any fails with cannot decode !!str `abc` as a !!null.

    So whichever rule this settles for a scalar whose text does not satisfy its tag, !!null should follow it. RawFromNode's doc comment lists such a scalar among its refusals; #1073 makes it name !!null as the exception and point here.

  3. OmarAlJarrah commented on Oct 23, 2026

    @OmarAlJarrah
    MemberAuthor

    After #1106, value lowering and verbatim preservation read a scalar's kind through one classifier, value.Classify, so whichever way this is decided, it lands in one place.

    Until then the classifier applies the rule both documents state today, refusal, to every tag. That now includes !!null and !!timestamp scalars whose text the tag does not admit (!!null abc, !!timestamp soon), which converted without error before (#1072 noted the !!null case as part of this decision). If this issue decides on keeping the text instead, the change is confined to Classify, and annotation.TestScalarRenderers_AgreeOnKind holds the two renderers to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority:P4Refactors, tests, CI, hostile-input hardening, cosmetic diagnostics, other formatstype:bugDefect in shipped behavior

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions