fix(compilers/openapi): keep the tighter of co-declared bounds - #274
Merged
Conversation
In the 2020-12 dialect (OAS 3.1/3.2) minimum and exclusiveMinimum are
independent keywords that may both apply to one value, and so are
maximum and exclusiveMaximum. The reader filled Min/Max from
minimum/maximum and then let the exclusive arm overwrite whichever it
found there, so a schema writing both published whichever keyword
happened to be read last rather than the bound the source means.
Where minimum was the tighter of the pair that is a wrong constraint,
not merely a lossy one: {minimum: 10, exclusiveMinimum: 0} compiled to
"> 0", so generated validation would admit 1 through 9. Nothing was
reported either way.
The two keywords are conjunctive, so the tighter one is the effective
bound and the other is implied by it. reconcileBound now picks that one,
with the tie going to the exclusive bound on both sides ("x >= 5 and
x > 5" is "x > 5", "x <= 5 and x < 5" is "x < 5"). The comparison runs
through math/big rather than float64: these are exactly the literals
BigVal exists to keep intact, and rounding them to compare could pick
the wrong bound for a pair that differs past float64's precision. A pair
math/big cannot compare exactly — an exponent past its limit for a
rational — keeps the exclusive bound and says the other may have been
tighter.
The keyword that does not reach the IR is named, with its exact value,
in a degraded-construct diagnostic. It is not also preserved verbatim:
the reader has no Unmodeled channel and its three callers route
annotations to three different carriers, and the dropped keyword is
implied by the kept one, so no admitted or excluded value changes.
The 3.0 arm, where exclusiveMinimum is a boolean modifier of minimum and
the two cannot disagree, is untouched.
One row per side per outcome — the inclusive keyword tighter, the exclusive keyword tighter, and the tie the exclusive one wins — plus a pair only an exact comparison can order, the sides that declare a single keyword and reconcile nothing, and the 3.0 spelling, where the boolean modifies the bound beside it and the two can never be rival bounds. Each row was checked against a planted defect: reverting the reconciliation, flipping the tie, flipping the comparison direction, silencing the diagnostic, claiming an incomparable pair was compared, and letting the 3.0 arm fall through to the 2020-12 one. schemaFromYAMLUnvalidated parses a fixture whose bound is beyond float64 range, which the library reports as a type mismatch and the loader suppresses for exactly that reason; requiring a clean parse would put those bounds out of this package's reach.
No committed spec wrote both keywords for one side, so the whole corpus agreed with the reader that took whichever came last. The constraints capability spec now declares a property bounded by minimum and exclusiveMinimum together and another bounded by maximum and exclusiveMaximum, and the golden records the effective bound plus the diagnostic naming the keyword that did not reach it. The two run opposite ways deliberately: a side where the exclusive keyword happens to be the tighter one compiles identically on the old reader, so a corpus holding only that shape would go green either way.
Reconciling a side's two 2020-12 bound keywords compared them as big.Rat
values, which is exact but not total: math/big refuses to build a rational
once a literal's decimal exponent passes its own limit, and the reader then
kept the exclusive bound and warned that the other may have been tighter.
Where the discarded one was tighter that is a constraint weaker than the
source, which is the defect the reconciliation exists to prevent, in a rarer
case. {minimum: 1.0e2000000, exclusiveMinimum: 5} compiled to "> 5" against
a source that says ">= 1e2000000", and {maximum: 1e-1000001, exclusiveMaximum:
5} to "< 5" against "<= 1e-1000001". Both magnitudes are legal in a spec and
ir.NewBigVal keeps them intact, so the reader has to order them.
Order them on the literal instead: sign, then the power of ten the leading
digit carries, then the digits. That is exact for every decimal spelling and
total over every magnitude, needs no float64, and never materializes the
value — the million zeros behind 1e1000001 say nothing about which bound is
the tighter. It also drops the ~14 ms and ~400 KB each such rational cost.
What is left uncomparable is a literal that is not decimal at all: BigVal
still stores a binary exponent verbatim, so "1p4" reaches the reader meaning
16 (#45). That keeps the exclusive bound and the warning, as before.
The incomparable fallback was justified by ir.NewBigVal storing a binary exponent verbatim, and covered by driving "1p4" through a schema. #269 closed that: NewBigVal refuses a p/P exponent now, so no schema reaches the fallback and the case that covered it reports an unreadable literal instead. The guard stays. What it defends is the seam between two grammars in different packages — parseDecimalBound's has to stay the wider of the two — and that seam has now moved once already. A bound this reader cannot order is one that could be silently replaced by the looser of its pair, which is the defect the reconciliation exists to prevent. So cover it where it lives, by calling reconcileBound directly, and add a property test asserting every literal NewBigVal accepts is one parseDecimalBound orders. Widening NewBigVal now reddens at the seam rather than in a compiled document. The comments that cited #45 as open say what the guard is for instead.
OmarAlJarrah
force-pushed
the
fix/openapi-exclusive-bound-overwrite
branch
from
August 6, 2026 15:17
81cf3c5 to
4a5e74b
Compare
Every 3.0 case declared exclusiveMinimum and exclusiveMaximum together, so a reader that crossed the two sides over produced exactly the constraints each case expected. Crossing them in setExclusiveFlag left the whole suite green. Add the one shape that can see it: exclusive on the minimum side only.
The seam test swept decimal spellings only, so it could confirm what was already true and nothing else: every spelling it generated was one NewBigVal already accepts, which no widening it does not contain can redden. Restoring the pre-#269 p/P grammar left it green — the widening that actually happened here, missed by the test written for it. Feed it the spellings NewBigVal refuses today: a binary exponent, other bases, a digit separator, the named non-numbers. They are skipped while refused, and met by the assertion the day one is accepted.
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
In the 2020-12 dialect (OpenAPI 3.1 and 3.2)
minimumandexclusiveMinimumareindependent keywords that may both apply to one value, and so are
maximumandexclusiveMaximum. The schema reader filledConstraints.Min/Maxfromminimum/maximumand then let the exclusive arm overwrite whatever it found there,so a schema writing both keywords for one side published whichever happened to be read
last rather than the bound the source means.
Where the inclusive keyword is the tighter of the pair, that is a wrong constraint
rather than a merely lossy one. Compiling
produced
min: "0", exclusiveMin: true— "> 0" where the source says "≥ 10", sogenerated validation would admit 1 through 9. The same on the maximum side:
{maximum: 10, exclusiveMaximum: 100}produced "< 100" where the source says "≤ 10".Neither reported anything.
The two keywords are conjunctive ("x ≥ m and x > e"), so the tighter of them is the
effective bound and the other is implied by it.
reconcileBoundnow keeps that one. Atie goes to the exclusive bound on both sides — "x ≥ 5 and x > 5" is "x > 5", and
"x ≤ 5 and x < 5" is "x < 5" — even though "tighter" runs in opposite directions on the
two sides.
The 3.0 arm is untouched. There
exclusiveMinimumis a boolean modifier of theminimumbeside it, so the two can never be rival bounds and there is nothing toreconcile.
Ordering the two bounds
The comparison decides which constraint ships, so it has to be exact and it has to
work on every literal a spec may legally write.
float64is out on both counts: theseare the values
ir.BigValexists to keep intact, and rounding them to compare picksthe wrong bound for a pair that differs past
float64's precision or sits beyond itsrange — the same defect from the other end.
A rational is exact but not total.
math/bigrefuses to build one once a literal'sdecimal exponent passes its own limit, so
1e1000001— legal, and stored intact byir.NewBigVal— cannot be ordered that way, and a bound that cannot be ordered is abound that may be silently widened.
So the ordering runs on the literal itself: sign, then the power of ten the leading
digit carries, then the digits, with the exponent held as a
big.Int. That is exactfor every decimal spelling (
1e2and100are one value,-0.0and0are onevalue), total over every magnitude, and it never materializes the value — the zeros
behind
1e1000001say nothing about which of two bounds is the tighter.What no decimal reading can order is a literal outside the decimal grammar. No spec
produces one today: every bound arrives through
ir.NewBigVal, whose grammar is thenarrower of the two since #269 taught it to refuse a
p/Pbinary exponent. The guardstays anyway, because what it defends is a seam between two grammars in different
packages, and that seam has already moved once — a bound this reader cannot order is one
that could be silently replaced by the looser of its pair, which is the defect the
reconciliation exists to prevent. It keeps the exclusive bound and says in a warning that
the discarded one may have been the tighter, rather than claiming a comparison it did not
make.
TestBigValGrammarStaysWithinTheDecimalReadingis what holds the seam: every literalNewBigValaccepts must be oneparseDecimalBoundcan order. WideningNewBigValreddens there rather than quietly widening a bound in a compiled document. The guard
itself is covered by calling
reconcileBounddirectly, since no schema reaches it.Half that test's corpus is spellings
NewBigValrefuses today — a binary exponent,other bases, a digit separator, the named non-numbers — and they are the half that does
the work. A sweep of accepted spellings can only confirm what is already true, since
every one of them parses by construction; it cannot redden on a widening it does not
already contain, and a binary exponent is not a shape any sweep of decimal spellings
generates. They are skipped while refused, and met by the assertion the day one is
accepted. Restoring the pre-#269
p/Pgrammar reddens it; without them it did not.Deliberate limitation
The keyword that does not reach the IR is named — with its exact value — in a
openapi/degraded-constructdiagnostic, but it is not also preserved verbatim underUnmodeled.annotation.Constraintshas noUnmodeledreturn channel and its callersroute what it returns to different carriers — a property, a parameter and a hoisted
alias node — so opening one is a change to that seam rather than to the reconciliation.
No value the source admits or excludes changes, since the discarded keyword is implied
by the kept one; what is lost is the record that the source spelled the bound twice.
That is filed as #286 and recorded in the code beside
reconcileBound.Test plan
Reproduced first with the CLI rather than by reading. Before the change,
morphic compileon the spec above emitted"min": "0", "exclusiveMin": trueand anempty diagnostic list; after it,
"min": "10", "exclusiveMin": falseplus a diagnosticnaming
exclusiveMinimum 0as dropped. The maximum-side property behaves the same way.Compiling the same schema with the two keywords in the opposite declaration order
produces a byte-identical document — checked as a two-order
cmp.Diffover both theconstraints and the diagnostics, across every co-declared shape here (each side's three
outcomes, a magnitude no rational holds, and a pair only an exact comparison orders).
Nothing in this reader is order-sensitive to begin with: the two keywords are read by
name, in an order the code fixes rather than the source. Note that the corpus golden
records the source hash, so reordering the fixture's keys reddens it regardless — that
is the hash moving, not the IR.
New unit coverage in
compilers/openapi/internal/annotationwalks the matrix: on eachside, the inclusive keyword tighter, the exclusive keyword tighter, and the tie; the
3.0 dialect exclusive on one side only, which is the only shape that can tell the two
sides apart; a pair
only an exact comparison can order (
9007199254740993against9007199254740992); onevalue spelled two ways (
1e2against100); magnitudes no rational holds on bothsides; the sides that declare one keyword and reconcile nothing; a bound no decimal
reading orders, driven straight into
reconcileBound; and the 3.0 spelling.TestCompareDecimalBounds_...pins theordering itself in both directions per row, including the signed zeros and the pairs
that differ only past
float64's precision.Every behavioural assertion was checked against a planted defect, and each mutation
killed the rows it should:
constraints.go/decimal.goNewBigValwidened back to the pre-#269p/PgrammarparseDecimalBoundnarrowed to refuse exponentsparseDecimalBoundwidened to read a non-literal as zerobig.RatThe capability corpus had no spec declaring both keywords for one side, so it agreed
with the old reader everywhere.
testdata/conformance/openapi/constraints.yamlnowdeclares one property bounded by
minimum+exclusiveMinimumand another bymaximum+exclusiveMaximum, running in opposite directions on purpose: a side wherethe exclusive keyword happens to be the tighter one compiles identically on the old
reader, so a corpus holding only that shape would stay green either way. Reverting the
fix reddens the regenerated golden, and so does deleting either keyword from the spec.
Beyond the table above, the ordering was differential-tested against
math/big.Ratover500,000 random pairs drawn from ~6,200 literal spellings — signed zeros,
1e2against100, values differing pastfloat64's precision, exponents acrosse-600..e308—with no mismatch, and no literal
NewBigValaccepts thatparseDecimalBoundrefused.That run is a review instrument rather than a committed test: it can only cover the
subset
big.Ratrepresents, and the magnitudes past it are exactly what the table rowsare for.
Full gate green:
gofmtclean,go vetclean,golangci-lint0 issues,go build,and
scripts/check-coverage.shat 100% of statements. Rebased ontomain, and the gateabove was run on the rebased tree rather than the tree the branch was written against.
Closes #33