Skip to content

fix(compilers/openapi): stop dropping writeOnly beside readOnly - #322

Merged
OmarAlJarrah merged 1 commit into
mainfrom
fix/openapi-readonly-writeonly-conflict
Aug 9, 2026
Merged

fix(compilers/openapi): stop dropping writeOnly beside readOnly#322
OmarAlJarrah merged 1 commit into
mainfrom
fix/openapi-readonly-writeonly-conflict

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

A property whose schema declared both readOnly and writeOnly lowered exactly as one declaring
readOnly alone: annotation.EffectiveVisibility guarded the two flags in order and returned on
the first match, so writeOnly contributed nothing, appeared nowhere in the emitted document, and
raised no diagnostic in either channel. The same contradiction spread over two allOf branches
already intersected to Visibility{None: true} and warned, so one document could get two different
answers depending on how it spelled the same thing.

The compiler now reads both flags and answers the intersection: readOnly means the value is not
writable, writeOnly that it is not readable, so a position in both is admitted by no lifecycle at
all — Visibility{None: true}, which is what merge.mergeVisibility has answered for the branch
spelling since #34. The two spellings now agree, in the IR and under the same diagnostic code.

Three decisions worth stating:

  • Not refused. JSON Schema 2020-12 defines the two keywords independently and forbids neither
    beside the other, so the document is contradictory but legal. It gets a warning, not an error.
  • Not kept under Unmodeled. That carve-out is for constructs the IR cannot express, and the
    IR expresses this one exactly: Visibility{None: true} is the shape it already has for "visible
    nowhere". Preserving the raw keywords beside a field that models them would restate what the
    carrier holds.
  • Precedence is per flag, which changes one case. A site that writes readOnly settles
    readOnly for that position and says nothing about writeOnly, which still resolves from the
    $ref target — the uniform use-site-over-referent merge of ir-design §14 that every other
    annotation here follows. So {$ref: X, readOnly: true} against a writeOnly X now yields
    None where it used to yield plain readOnly; TestEffectiveVisibility_MapsTheFlagsToLifecycles
    pinned that as "the site wins over the referent", and the site still does win — for the flag it
    writes. Treating the pair as one annotation that a node wins outright would have kept the old
    answer at the cost of discarding the target's flag silently, which is the defect being fixed.
    The reversed spelling (writeOnly at the site over a readOnly target) answered readOnly
    before this change, contradicting that same precedence claim; it is symmetric now.

EffectiveVisibility returns the contradiction as a bool rather than a diagnostic: it has no
provenance of its own, and provenance is built in exactly one place (lowering.Ctx). The caller
that has one — FillPropertyDetail, which serves both model properties and response/part headers —
reports it, mirroring how merge.reconcileProperty reports its own disjoint intersection from
outside mergeVisibility.

The openapi/disjoint-visibility code is reused rather than joined by a second one. The finding is
the same for a consumer — this field ends up carried by no request or response — and giving the two
spellings different codes would reintroduce, in the diagnostic channel, the divergence this change
removes from the IR. Its doc comment now covers both.

Swept for the same mechanism (an ordered guard collapsing co-declared contradictory keywords
into the first, silently): EffectiveDeprecated reads a single flag and has no pair to collapse;
pickFlag is the intended site-over-referent primitive; annotation.reconcileBound keeps the
tighter of two co-declared bounds and already reports doing so; schema.dispatchOf elects among
const/enum/allOf in a fixed order but records every loser under Unmodeled with a
diagnostic. One genuine sibling turned up outside this change's subsystem — a parameter or header
declaring both schema and content drops one in silence, and the two positions disagree about
which — filed as #320 rather than folded in here.

Test plan

  • TestModel_ReadOnlyAndWriteOnlyTogetherAreVisibleNowhere (new, internal/schema) compiles the
    issue's own reproduction and asserts the property is visible in none, that it no longer lowers as
    a readOnly-only sibling does, and that exactly one openapi/disjoint-visibility warning lands at
    the property's pointer naming writeOnly. Reverting the reader to its ordered guard turns all
    three red.
  • TestEffectiveVisibility_MapsTheFlagsToLifecycles grows four cases — both flags on one schema,
    both on the referent, and each cross-node direction — and keeps the same-flag precedence cases.
  • The readonly-writeonly conformance fixture gains the pairing in both spellings (one schema, and
    a $ref site carrying the opposite flag). Its golden picks up the two none: true properties and
    the two warnings, and nothing else. Deleting the new property from the fixture reddens the suite,
    so the regeneration is not a no-op.
  • unwitnessed.golden.txt loses Visibility.None: no committed spec drove that field to a
    non-zero value until now.
  • Full gate green: gofmt, go vet, golangci-lint, go build, 100% statement coverage.

Closes#276

@OmarAlJarrah
OmarAlJarrah merged commit 946db8c into mainAug 9, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/openapi-readonly-writeonly-conflict branch August 9, 2026 09:54
OmarAlJarrah added a commit that referenced this pull request Aug 9, 2026
Nine PRs landed on main after this branch was opened, and two of them added
tests to files this branch rewrites. Git reconciled the two edits without a
textual conflict, so the merged tree compiled nowhere: the new tests call the
package-local helpers this branch deletes.
Resolved by pointing those call sites at the package that now owns them —
26 in operations_test.go and schema_test.go across requireNoErrorDiags,
findOp, hasDiagCodeAt, pathsSpec, componentSpec and propsByWire. The one real
conflict, conformance_test.go's import block, keeps both: the branch added
openapitest and #322 added diag, and the file needs each.
No test was lost in the merge — the name list gains the new package's 22 and
loses nothing against main.
Sign up for freeto 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: a schema declaring both readOnly and writeOnly keeps only readOnly

1 participant

@OmarAlJarrah