From 912f72e9121e3fa9337df637cb4ddcb0934b81b7 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 03:53:47 +0300 Subject: [PATCH 1/9] fix(compilers/openapi): keep the tighter of two co-declared bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../internal/annotation/constraints.go | 107 +++++++++++++++++- 1 file changed, 101 insertions(+), 6 deletions(-) diff --git a/compilers/openapi/internal/annotation/constraints.go b/compilers/openapi/internal/annotation/constraints.go index a1c4e48..d4a5a0e 100644 --- a/compilers/openapi/internal/annotation/constraints.go +++ b/compilers/openapi/internal/annotation/constraints.go @@ -1,6 +1,8 @@ package annotation import ( + "math/big" + oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" "github.com/dexpace/morphic/compilers/openapi/internal/diag" @@ -15,7 +17,9 @@ import ( // are List-owned and read elsewhere. A non-finite bound literal yields an // error-severity diag.NumericPrecision diagnostic and is skipped; nil is // returned when no constraint is present. exclusiveBoolean selects the -// exclusiveMinimum/exclusiveMaximum dialect (see applyExclusive). +// exclusiveMinimum/exclusiveMaximum dialect (see applyExclusive), and under the +// 2020-12 one a side that declares both of its keywords is settled by +// reconcileBound rather than by whichever ran last. // // It reads beside the other readers here for the reason they are here at all: // what a schema says about the values admitted at a position is read the same @@ -81,8 +85,9 @@ func boundLiteralDiag(prop, literal string, err error) ir.Diagnostic { // applyExclusive handles exclusiveMinimum/exclusiveMaximum in both dialects: the // 3.0 boolean arm flags the corresponding Min/Max as exclusive; the 2020-12 // numeric arm (3.1/3.2) carries the bound value itself, read from the raw node to -// avoid the float64 trap, and sets the exclusive flag. exclusiveBoolean selects -// the dialect (true for 3.0). Because load suppresses the library's type-mismatch +// avoid the float64 trap, and hands it to reconcileBound, which decides how it +// meets any minimum/maximum declared beside it. exclusiveBoolean selects the +// dialect (true for 3.0). Because load suppresses the library's type-mismatch // on these keywords, a value in the wrong form for the dialect is reported and // dropped here rather than silently accepted. func applyExclusive(c *ir.Constraints, s *oas3.Schema, isMin, exclusiveBoolean bool) []ir.Diagnostic { @@ -110,8 +115,95 @@ func applyExclusive(c *ir.Constraints, s *oas3.Schema, isMin, exclusiveBoolean b if err != nil { return []ir.Diagnostic{boundLiteralDiag(prop, node.Value, err)} } - setExclusiveBound(c, isMin, &v) - return nil + return reconcileBound(c, isMin, v) +} + +// reconcileBound settles one side's bound when the 2020-12 dialect declares +// both keywords for it: the inclusive minimum/maximum numericBounds has already +// put in c, and the exclusive bound excl read alongside it. +// +// The two are independent and conjunctive there — "x >= m and x > e" — so the +// tighter of them is the effective bound and the other adds nothing. ir.Constraints +// holds one bound plus one exclusivity flag per side, so the tighter one is kept; +// taking the exclusive bound unconditionally, as this did before, published a +// constraint weaker than the source wherever minimum was the tighter (GitHub #33). +// +// The discarded keyword is implied by the kept one, so no value the source admits +// or excludes changes — but it is still a keyword the source wrote and the IR does +// not carry, so it is named in a diagnostic rather than dropped in silence. It is +// not also preserved verbatim: Constraints has no Unmodeled channel, and its three +// callers route annotations to three different carriers, so plumbing one for a +// keyword the kept bound already implies is out of scope here. +func reconcileBound(c *ir.Constraints, isMin bool, excl ir.BigVal) []ir.Diagnostic { + incl, inclProp, exclProp := c.Max, "maximum", "exclusiveMaximum" + if isMin { + incl, inclProp, exclProp = c.Min, "minimum", "exclusiveMinimum" + } + if incl == nil { + setExclusiveBound(c, isMin, &excl) + return nil + } + + tighter, compared := inclusiveIsTighter(*incl, excl, isMin) + if tighter { + return []ir.Diagnostic{redundantBoundDiag(inclProp, *incl, exclProp, excl, compared)} + } + + dropped := *incl + setExclusiveBound(c, isMin, &excl) + return []ir.Diagnostic{redundantBoundDiag(exclProp, excl, inclProp, dropped, compared)} +} + +// inclusiveIsTighter reports whether the inclusive bound incl admits fewer +// values than the exclusive bound excl written on the same side, and whether +// the two could be compared at all. +// +// A minimum is tighter when it is the greater of the two, a maximum when it is +// the lesser; equal magnitudes are never tighter, which is what gives the +// exclusive bound the tie on both sides ("x >= 5 and x > 5" is "x > 5", +// "x <= 5 and x < 5" is "x < 5"). +// +// The comparison goes through math/big, never float64: these are the literals +// BigVal exists to keep intact, so rounding them to compare would let two values +// that differ past float64's precision — or one beyond its range — pick the +// wrong bound, reintroducing the defect this reconciliation exists to fix. A +// rational is exact for every literal it accepts and rejects only an exponent +// past math/big's own limit for one, which is the incomparable case: nothing +// can be said about which bound is tighter, so the caller keeps the exclusive +// bound and says so. +func inclusiveIsTighter(incl, excl ir.BigVal, isMin bool) (tighter, compared bool) { + inclRat, inclOK := new(big.Rat).SetString(incl.String()) + exclRat, exclOK := new(big.Rat).SetString(excl.String()) + if !inclOK || !exclOK { + return false, false + } + order := inclRat.Cmp(exclRat) + if order == 0 { + return false, true + } + return (order > 0) == isMin, true +} + +// redundantBoundDiag reports the co-declared 2020-12 bound that did not reach +// the IR, naming both keywords and both exact literals so a reader can see what +// was dropped without going back to the source. +// +// compared tells the two cases apart. When the magnitudes did compare, the kept +// bound is provably the tighter and the dropped one is redundant, which costs +// the consumer nothing — hence info severity. When they did not, the kept bound +// is the exclusive one by fallback and may be the looser of the two, so the +// message says so and the severity rises to warning. +func redundantBoundDiag(keptProp string, kept ir.BigVal, dropProp string, dropped ir.BigVal, compared bool) ir.Diagnostic { + if !compared { + return diag.Newf(ir.SeverityWarning, diag.DegradedConstruct, ir.Provenance{}, + "%s %s and %s %s both bound this value but their magnitudes could not be compared; "+ + "kept %s and dropped %s, which may be the tighter of the two", + keptProp, kept, dropProp, dropped, keptProp, dropProp) + } + return diag.Newf(ir.SeverityInfo, diag.DegradedConstruct, ir.Provenance{}, + "%s %s and %s %s both bound this value and the IR holds one bound per side; "+ + "kept %s as the tighter of the two and dropped %s, which it implies", + keptProp, kept, dropProp, dropped, keptProp, dropProp) } // exclusiveFormDiag reports an exclusiveMinimum/exclusiveMaximum whose value form @@ -137,7 +229,10 @@ func setExclusiveFlag(c *ir.Constraints, isMin bool) { c.ExclusiveMax = true } -// setExclusiveBound sets an exclusive numeric bound (2020-12 arm) on Min or Max. +// setExclusiveBound sets an exclusive numeric bound (2020-12 arm) on Min or Max, +// replacing whatever minimum/maximum put there. Only reconcileBound may call it, +// which is where the replacement is decided; calling it directly is the shape of +// GitHub #33. func setExclusiveBound(c *ir.Constraints, isMin bool, v *ir.BigVal) { if isMin { c.Min = v From 12254434bd2536bf91d2214f99b9f46f43a35bf5 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 04:04:40 +0300 Subject: [PATCH 2/9] test(compilers/openapi): cover the co-declared bound matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../constraints_readers_internal_test.go | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index ee657cd..a9cc5d9 100644 --- a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go +++ b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go @@ -1,8 +1,12 @@ package annotation import ( + "strings" "testing" + "github.com/google/go-cmp/cmp" + oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" + "github.com/speakeasy-api/openapi/marshaller" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -10,6 +14,24 @@ import ( "github.com/dexpace/morphic/ir" ) +// schemaFromYAMLUnvalidated parses body like schemaFromYAML but keeps the +// library's own validation findings instead of requiring none. +// +// A bound beyond float64 range is the case it exists for: the library types +// these keywords as float64 and reports the literal as a string it could not +// convert, while the compiler's loader suppresses exactly that finding because +// such a bound is valid and must survive. Requiring a clean parse here would +// put every out-of-float64-range bound out of this package's reach. +func schemaFromYAMLUnvalidated(t *testing.T, body string) *oas3.Schema { + t.Helper() + var js oas3.JSONSchema[oas3.Referenceable] + _, err := marshaller.Unmarshal(t.Context(), strings.NewReader(body), &js) + require.NoError(t, err) + s := js.GetSchema() + require.NotNil(t, s, "the fixture is a schema, not a bare boolean") + return s +} + // bigOf returns a pointer to v as a BigVal, which every numeric bound is. func bigOf(v string) *ir.BigVal { b := ir.BigVal(v) @@ -185,3 +207,153 @@ func TestApplyExclusive_AMalformedNumericBoundIsReported(t *testing.T) { assert.Contains(t, diags[0].Message, "exclusiveMaximum") assert.Nil(t, got) } + +// TestReconcileBound_KeepsTheTighterOfTwoCoDeclaredBounds pins the 2020-12 rule +// that a side's two keywords are independent and conjunctive, so one bound slot +// must hold the tighter of them. Keeping the looser is a constraint weaker than +// the source wrote, which is a wrong answer rather than an incomplete one +// (GitHub #33) — {minimum: 10, exclusiveMinimum: 0} once compiled to "> 0". +// +// The tie rows are the reason each side is spelled out rather than derived from +// the other: "x >= 5 and x > 5" is "x > 5" and "x <= 5 and x < 5" is "x < 5", so +// the exclusive bound wins a tie on both sides even though "tighter" runs the +// opposite way on each. +func TestReconcileBound_KeepsTheTighterOfTwoCoDeclaredBounds(t *testing.T) { + t.Parallel() + tests := []struct { + name string + body string + want ir.Constraints + wantSays []string + }{ + { + name: "minimum is the tighter of the pair", + body: "minimum: 10\nexclusiveMinimum: 0\n", + want: ir.Constraints{Min: bigOf("10")}, + wantSays: []string{"minimum 10", "exclusiveMinimum 0", + "kept minimum as the tighter", "dropped exclusiveMinimum"}, + }, + { + name: "exclusiveMinimum is the tighter of the pair", + body: "minimum: 0\nexclusiveMinimum: 10\n", + want: ir.Constraints{Min: bigOf("10"), ExclusiveMin: true}, + wantSays: []string{"exclusiveMinimum 10", "minimum 0", + "kept exclusiveMinimum as the tighter", "dropped minimum"}, + }, + { + name: "equal minimums leave the exclusive one standing", + body: "minimum: 5\nexclusiveMinimum: 5\n", + want: ir.Constraints{Min: bigOf("5"), ExclusiveMin: true}, + wantSays: []string{"exclusiveMinimum 5", "minimum 5", + "kept exclusiveMinimum as the tighter", "dropped minimum"}, + }, + { + name: "maximum is the tighter of the pair", + body: "maximum: 10\nexclusiveMaximum: 100\n", + want: ir.Constraints{Max: bigOf("10")}, + wantSays: []string{"maximum 10", "exclusiveMaximum 100", + "kept maximum as the tighter", "dropped exclusiveMaximum"}, + }, + { + name: "exclusiveMaximum is the tighter of the pair", + body: "maximum: 100\nexclusiveMaximum: 10\n", + want: ir.Constraints{Max: bigOf("10"), ExclusiveMax: true}, + wantSays: []string{"exclusiveMaximum 10", "maximum 100", + "kept exclusiveMaximum as the tighter", "dropped maximum"}, + }, + { + name: "equal maximums leave the exclusive one standing", + body: "maximum: 5\nexclusiveMaximum: 5\n", + want: ir.Constraints{Max: bigOf("5"), ExclusiveMax: true}, + wantSays: []string{"exclusiveMaximum 5", "maximum 5", + "kept exclusiveMaximum as the tighter", "dropped maximum"}, + }, + { + name: "a bound decided by a digit float64 cannot hold", + body: "minimum: 9007199254740993\nexclusiveMinimum: 9007199254740992\n", + want: ir.Constraints{Min: bigOf("9007199254740993")}, + wantSays: []string{"minimum 9007199254740993", "exclusiveMinimum 9007199254740992", + "kept minimum as the tighter", "dropped exclusiveMinimum"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, diags := Constraints(schemaFromYAML(t, "type: number\n"+tc.body), false) + + require.NotNil(t, got) + if diff := cmp.Diff(tc.want, *got); diff != "" { + t.Errorf("constraints (-want +got):\n%s", diff) + } + require.Len(t, diags, 1, "the keyword that did not reach the IR is reported") + assert.Equal(t, ir.SeverityInfo, diags[0].Severity) + assert.Equal(t, diag.DegradedConstruct, diags[0].Code) + for _, says := range tc.wantSays { + assert.Contains(t, diags[0].Message, says) + } + }) + } +} + +// TestReconcileBound_OneKeywordPerSideIsNotReconciled pins the silent path. A +// side that writes one keyword has nothing to reconcile, so announcing a +// dropped bound there would report a loss that did not happen — and it is the +// common case, which a diagnostic on every numeric schema would drown. +func TestReconcileBound_OneKeywordPerSideIsNotReconciled(t *testing.T) { + t.Parallel() + for _, body := range []string{ + "minimum: 1\nmaximum: 9\n", + "exclusiveMinimum: 1\nexclusiveMaximum: 9\n", + "minimum: 1\nexclusiveMaximum: 9\n", + } { + t.Run(body, func(t *testing.T) { + t.Parallel() + got, diags := Constraints(schemaFromYAML(t, "type: number\n"+body), false) + + require.NotNil(t, got) + assert.Empty(t, diags) + }) + } +} + +// TestReconcileBound_ThreeZeroDialectPairIsUntouched pins the 3.0 arm against +// the 2020-12 fix. There exclusiveMinimum is a boolean modifier of the minimum +// beside it, so the two cannot be rival bounds and there is nothing to drop: +// reconciling them would invent a diagnostic and could discard the bound the +// flag modifies. +func TestReconcileBound_ThreeZeroDialectPairIsUntouched(t *testing.T) { + t.Parallel() + got, diags := Constraints(schemaFromYAML(t, + "type: number\nminimum: 10\nexclusiveMinimum: true\nmaximum: 20\nexclusiveMaximum: true\n"), true) + + require.NotNil(t, got) + assert.Empty(t, diags) + want := ir.Constraints{Min: bigOf("10"), Max: bigOf("20"), ExclusiveMin: true, ExclusiveMax: true} + if diff := cmp.Diff(want, *got); diff != "" { + t.Errorf("constraints (-want +got):\n%s", diff) + } +} + +// TestReconcileBound_IncomparableMagnitudesKeepTheExclusiveBound pins the +// fallback. ir.BigVal admits an exponent no exact rational will parse, so the +// two bounds cannot always be ordered; the reader keeps the exclusive one and +// says the discarded bound may have been the tighter, rather than claiming a +// comparison it did not make. +func TestReconcileBound_IncomparableMagnitudesKeepTheExclusiveBound(t *testing.T) { + t.Parallel() + s := schemaFromYAMLUnvalidated(t, "type: number\nminimum: 1.0e2000000\nexclusiveMinimum: 5\n") + + got, diags := Constraints(s, false) + + require.NotNil(t, got) + want := ir.Constraints{Min: bigOf("5"), ExclusiveMin: true} + if diff := cmp.Diff(want, *got); diff != "" { + t.Errorf("constraints (-want +got):\n%s", diff) + } + require.Len(t, diags, 1) + assert.Equal(t, ir.SeverityWarning, diags[0].Severity, "the kept bound may be the looser one") + assert.Equal(t, diag.DegradedConstruct, diags[0].Code) + assert.Contains(t, diags[0].Message, "could not be compared") + assert.Contains(t, diags[0].Message, "minimum 1.0e2000000") + assert.Contains(t, diags[0].Message, "exclusiveMinimum 5") +} From 0686d7ecb907d06cbaa2c4a49b8fa2a54b4badb6 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 04:06:07 +0300 Subject: [PATCH 3/9] test(compilers/openapi): put co-declared bounds in the corpus 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. --- compilers/openapi/conformance_test.go | 34 +++++- .../openapi/constraints.golden.json | 100 +++++++++++++++++- testdata/conformance/openapi/constraints.yaml | 12 +++ 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/compilers/openapi/conformance_test.go b/compilers/openapi/conformance_test.go index cba3d9b..ac29af5 100644 --- a/compilers/openapi/conformance_test.go +++ b/compilers/openapi/conformance_test.go @@ -1103,7 +1103,7 @@ func assertRawPreservedDates(t *testing.T, doc *ir.Document) { } } -func assertConstraints(t *testing.T, doc *ir.Document, _ []ir.Diagnostic) { +func assertConstraints(t *testing.T, doc *ir.Document, diags []ir.Diagnostic) { m, ok := doc.Types[namedID("S")].(*ir.Model) require.True(t, ok) ratio, ok := propByWire(m, "ratio") @@ -1127,6 +1127,38 @@ func assertConstraints(t *testing.T, doc *ir.Document, _ []ir.Diagnostic) { assert.Equal(t, int64(4), *m.Constraints.MaxProps) assertLengthAndCollectionBounds(t, doc, m) + assertCoDeclaredBounds(t, m, diags) +} + +// assertCoDeclaredBounds pins the 2020-12 rule that a side declaring both of +// its keywords keeps the tighter of the two: the property bounded below keeps +// its minimum, the one bounded above keeps its exclusiveMaximum, and each side +// names the keyword that did not reach the IR (GitHub #33). +// +// Both directions are here on purpose. A case where only the exclusive keyword +// survives passes just as well on the reader that always took it, so on its own +// it would say nothing about the fix. +func assertCoDeclaredBounds(t *testing.T, m *ir.Model, diags []ir.Diagnostic) { + t.Helper() + low, ok := propByWire(m, "atLeastTen") + require.True(t, ok) + require.NotNil(t, low.Constraints) + require.NotNil(t, low.Constraints.Min) + assert.Equal(t, ir.BigVal("10"), *low.Constraints.Min, "minimum is the tighter bound") + assert.False(t, low.Constraints.ExclusiveMin, "and it is inclusive as written") + + high, ok := propByWire(m, "underTen") + require.True(t, ok) + require.NotNil(t, high.Constraints) + require.NotNil(t, high.Constraints.Max) + assert.Equal(t, ir.BigVal("10"), *high.Constraints.Max, "exclusiveMaximum is the tighter bound") + assert.True(t, high.Constraints.ExclusiveMax) + + for _, want := range []string{"dropped exclusiveMinimum", "dropped maximum"} { + assert.True(t, slices.ContainsFunc(diags, func(d ir.Diagnostic) bool { + return strings.Contains(d.Message, want) + }), "a keyword the IR does not carry is reported, not dropped in silence: %q", want) + } } // assertLengthAndCollectionBounds pins the non-numeric bounds: a string length diff --git a/testdata/conformance/openapi/constraints.golden.json b/testdata/conformance/openapi/constraints.golden.json index bc929d6..ea750ff 100644 --- a/testdata/conformance/openapi/constraints.golden.json +++ b/testdata/conformance/openapi/constraints.golden.json @@ -93,6 +93,72 @@ "pointer": "/components/schemas/S/properties/ratio" } }, + { + "id": "p/openapi/components/schemas/S/properties/atLeastTen", + "name": { + "source": "atLeastTen", + "canonical": "at_least_ten" + }, + "wireName": "atLeastTen", + "type": { + "target": "t/prim/integer", + "nullable": false + }, + "required": false, + "clientOptional": false, + "defaultAdded": false, + "visibility": { + "none": false + }, + "constraints": { + "min": "10", + "exclusiveMin": false, + "exclusiveMax": false, + "uniqueItems": false + }, + "flatten": false, + "eventHeader": false, + "eventPayload": false, + "secret": false, + "docs": {}, + "provenance": { + "source": 0, + "pointer": "/components/schemas/S/properties/atLeastTen" + } + }, + { + "id": "p/openapi/components/schemas/S/properties/underTen", + "name": { + "source": "underTen", + "canonical": "under_ten" + }, + "wireName": "underTen", + "type": { + "target": "t/prim/integer", + "nullable": false + }, + "required": false, + "clientOptional": false, + "defaultAdded": false, + "visibility": { + "none": false + }, + "constraints": { + "max": "10", + "exclusiveMin": false, + "exclusiveMax": true, + "uniqueItems": false + }, + "flatten": false, + "eventHeader": false, + "eventPayload": false, + "secret": false, + "docs": {}, + "provenance": { + "source": 0, + "pointer": "/components/schemas/S/properties/underTen" + } + }, { "id": "p/openapi/components/schemas/S/properties/label", "name": { @@ -166,6 +232,18 @@ "positional": false, "inputOnly": false }, + "t/prim/integer": { + "kind": "primitive", + "id": "t/prim/integer", + "name": {}, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0 + }, + "prim": "integer" + }, "t/prim/number": { "kind": "primitive", "id": "t/prim/number", @@ -201,11 +279,31 @@ "auth": null } ], + "diagnostics": [ + { + "severity": "info", + "code": "openapi/degraded-construct", + "message": "minimum 10 and exclusiveMinimum 0 both bound this value and the IR holds one bound per side; kept minimum as the tighter of the two and dropped exclusiveMinimum, which it implies", + "provenance": { + "source": 0, + "pointer": "/components/schemas/S/properties/atLeastTen" + } + }, + { + "severity": "info", + "code": "openapi/degraded-construct", + "message": "exclusiveMaximum 10 and maximum 100 both bound this value and the IR holds one bound per side; kept exclusiveMaximum as the tighter of the two and dropped maximum, which it implies", + "provenance": { + "source": 0, + "pointer": "/components/schemas/S/properties/underTen" + } + } + ], "sources": [ { "format": "openapi@3.1", "path": "constraints.yaml", - "hash": "f17e97c5ad1b01b8abaf78d584c35de4fb773693441b4068f9f87db515d0f743" + "hash": "3153e80d3265e6f1d00a35b4955110a7d9b95b79984f4556f6e81edcc23801e7" } ] } diff --git a/testdata/conformance/openapi/constraints.yaml b/testdata/conformance/openapi/constraints.yaml index a724c78..ad2e148 100644 --- a/testdata/conformance/openapi/constraints.yaml +++ b/testdata/conformance/openapi/constraints.yaml @@ -13,6 +13,18 @@ components: minimum: 0.30000000000000004 maximum: 9007199254740993 multipleOf: 0.1 + # In 2020-12 the two keywords on a side are independent and both apply, + # so the effective bound is the tighter of them. Reading whichever came + # last published ">= 0" here and "< 100" below (GitHub #33). One bound + # slot per side means the other keyword is reported rather than kept. + atLeastTen: + type: integer + minimum: 10 + exclusiveMinimum: 0 + underTen: + type: integer + maximum: 100 + exclusiveMaximum: 10 label: {type: string, minLength: 2, maxLength: 8} # An array hoists a List, which is where a collection bound belongs. tags: From 8ac7d28508bc211e2d916aaaca211a5533ccce26 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 11:39:01 +0300 Subject: [PATCH 4/9] fix(compilers/openapi): compare co-declared bounds without a rational MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../internal/annotation/constraints.go | 34 +++-- .../constraints_readers_internal_test.go | 76 ++++++++- .../openapi/internal/annotation/decimal.go | 144 ++++++++++++++++++ .../annotation/decimal_internal_test.go | 69 +++++++++ 4 files changed, 299 insertions(+), 24 deletions(-) create mode 100644 compilers/openapi/internal/annotation/decimal.go create mode 100644 compilers/openapi/internal/annotation/decimal_internal_test.go diff --git a/compilers/openapi/internal/annotation/constraints.go b/compilers/openapi/internal/annotation/constraints.go index d4a5a0e..1ede7bb 100644 --- a/compilers/openapi/internal/annotation/constraints.go +++ b/compilers/openapi/internal/annotation/constraints.go @@ -1,8 +1,6 @@ package annotation import ( - "math/big" - oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" "github.com/dexpace/morphic/compilers/openapi/internal/diag" @@ -131,9 +129,10 @@ func applyExclusive(c *ir.Constraints, s *oas3.Schema, isMin, exclusiveBoolean b // The discarded keyword is implied by the kept one, so no value the source admits // or excludes changes — but it is still a keyword the source wrote and the IR does // not carry, so it is named in a diagnostic rather than dropped in silence. It is -// not also preserved verbatim: Constraints has no Unmodeled channel, and its three -// callers route annotations to three different carriers, so plumbing one for a -// keyword the kept bound already implies is out of scope here. +// not also preserved verbatim: Constraints has no Unmodeled channel, and its +// callers route what it returns to different carriers — a property, a parameter +// and a hoisted alias node — so opening one is a change of its own, tracked in +// GitHub #286 rather than made here. func reconcileBound(c *ir.Constraints, isMin bool, excl ir.BigVal) []ir.Diagnostic { incl, inclProp, exclProp := c.Max, "maximum", "exclusiveMaximum" if isMin { @@ -163,21 +162,24 @@ func reconcileBound(c *ir.Constraints, isMin bool, excl ir.BigVal) []ir.Diagnost // exclusive bound the tie on both sides ("x >= 5 and x > 5" is "x > 5", // "x <= 5 and x < 5" is "x < 5"). // -// The comparison goes through math/big, never float64: these are the literals -// BigVal exists to keep intact, so rounding them to compare would let two values -// that differ past float64's precision — or one beyond its range — pick the -// wrong bound, reintroducing the defect this reconciliation exists to fix. A -// rational is exact for every literal it accepts and rejects only an exponent -// past math/big's own limit for one, which is the incomparable case: nothing -// can be said about which bound is tighter, so the caller keeps the exclusive -// bound and says so. +// The comparison is exact and never rounds to float64: these are the literals +// BigVal exists to keep intact, so comparing them as floats would let a pair +// that differs past float64's precision — or one beyond its range — pick the +// wrong bound, reintroducing the defect this reconciliation exists to fix. It +// is also total over every magnitude a spec may legally write, which a rational +// is not: math/big will not build 1e1000001 as one, and a bound it cannot order +// is a bound it may silently widen. +// +// What it cannot order is a literal that is not decimal — BigVal still stores a +// binary exponent verbatim (GitHub #45) — and there the caller keeps the +// exclusive bound and says the other may have been the tighter. func inclusiveIsTighter(incl, excl ir.BigVal, isMin bool) (tighter, compared bool) { - inclRat, inclOK := new(big.Rat).SetString(incl.String()) - exclRat, exclOK := new(big.Rat).SetString(excl.String()) + inclDec, inclOK := parseDecimalBound(incl) + exclDec, exclOK := parseDecimalBound(excl) if !inclOK || !exclOK { return false, false } - order := inclRat.Cmp(exclRat) + order := compareDecimalBounds(inclDec, exclDec) if order == 0 { return false, true } diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index a9cc5d9..8da4959 100644 --- a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go +++ b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go @@ -275,6 +275,13 @@ func TestReconcileBound_KeepsTheTighterOfTwoCoDeclaredBounds(t *testing.T) { wantSays: []string{"minimum 9007199254740993", "exclusiveMinimum 9007199254740992", "kept minimum as the tighter", "dropped exclusiveMinimum"}, }, + { + name: "one value spelled two ways is still a tie", + body: "minimum: 1e2\nexclusiveMinimum: 100\n", + want: ir.Constraints{Min: bigOf("100"), ExclusiveMin: true}, + wantSays: []string{"exclusiveMinimum 100", "minimum 1e2", + "kept exclusiveMinimum as the tighter", "dropped minimum"}, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -334,14 +341,67 @@ func TestReconcileBound_ThreeZeroDialectPairIsUntouched(t *testing.T) { } } -// TestReconcileBound_IncomparableMagnitudesKeepTheExclusiveBound pins the -// fallback. ir.BigVal admits an exponent no exact rational will parse, so the -// two bounds cannot always be ordered; the reader keeps the exclusive one and -// says the discarded bound may have been the tighter, rather than claiming a -// comparison it did not make. -func TestReconcileBound_IncomparableMagnitudesKeepTheExclusiveBound(t *testing.T) { +// TestReconcileBound_AMagnitudeNoRationalHoldsStillCompares pins the exactness +// of the comparison at the size where the obvious way to make it gives out. +// math/big will not build 1e2000000 as a rational — the exponent is past its +// own limit for one — so reconciling through a rational had to fall back, and +// the fallback keeps the exclusive bound. Here that is the looser one: "> 5" +// where the source says ">= 1e2000000" is the wrong constraint GitHub #33 is +// about, in a rarer case and with a warning attached. +// +// These magnitudes are legal in a spec and ir.NewBigVal keeps them, so the +// comparison has to reach them; the exponent alone separates the two bounds, +// and nothing here needs the million digits it stands for. +func TestReconcileBound_AMagnitudeNoRationalHoldsStillCompares(t *testing.T) { + t.Parallel() + tests := []struct { + name string + body string + want ir.Constraints + wantSays []string + }{ + { + name: "a minimum too large for a rational is still the tighter", + body: "minimum: 1.0e2000000\nexclusiveMinimum: 5\n", + want: ir.Constraints{Min: bigOf("1.0e2000000")}, + wantSays: []string{"minimum 1.0e2000000", "exclusiveMinimum 5", + "kept minimum as the tighter", "dropped exclusiveMinimum"}, + }, + { + name: "a maximum too small for one is the tighter on its side", + body: "maximum: 1e-1000001\nexclusiveMaximum: 5\n", + want: ir.Constraints{Max: bigOf("1e-1000001")}, + wantSays: []string{"maximum 1e-1000001", "exclusiveMaximum 5", + "kept maximum as the tighter", "dropped exclusiveMaximum"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, diags := Constraints(schemaFromYAMLUnvalidated(t, "type: number\n"+tc.body), false) + + require.NotNil(t, got) + if diff := cmp.Diff(tc.want, *got); diff != "" { + t.Errorf("constraints (-want +got):\n%s", diff) + } + require.Len(t, diags, 1) + assert.Equal(t, ir.SeverityInfo, diags[0].Severity, "the pair did compare") + for _, says := range tc.wantSays { + assert.Contains(t, diags[0].Message, says) + } + }) + } +} + +// TestReconcileBound_ALiteralThatIsNotDecimalKeepsTheExclusiveBound pins what +// is left of the fallback. ir.NewBigVal still stores a binary exponent verbatim +// (GitHub #45), so "1p4" reaches this reader as a bound meaning 16 that no +// decimal reading orders. The reader keeps the exclusive bound and says the +// discarded one may have been the tighter, rather than claiming a comparison it +// did not make. +func TestReconcileBound_ALiteralThatIsNotDecimalKeepsTheExclusiveBound(t *testing.T) { t.Parallel() - s := schemaFromYAMLUnvalidated(t, "type: number\nminimum: 1.0e2000000\nexclusiveMinimum: 5\n") + s := schemaFromYAMLUnvalidated(t, "type: number\nminimum: 1p4\nexclusiveMinimum: 5\n") got, diags := Constraints(s, false) @@ -354,6 +414,6 @@ func TestReconcileBound_IncomparableMagnitudesKeepTheExclusiveBound(t *testing.T assert.Equal(t, ir.SeverityWarning, diags[0].Severity, "the kept bound may be the looser one") assert.Equal(t, diag.DegradedConstruct, diags[0].Code) assert.Contains(t, diags[0].Message, "could not be compared") - assert.Contains(t, diags[0].Message, "minimum 1.0e2000000") + assert.Contains(t, diags[0].Message, "minimum 1p4") assert.Contains(t, diags[0].Message, "exclusiveMinimum 5") } diff --git a/compilers/openapi/internal/annotation/decimal.go b/compilers/openapi/internal/annotation/decimal.go new file mode 100644 index 0000000..916969e --- /dev/null +++ b/compilers/openapi/internal/annotation/decimal.go @@ -0,0 +1,144 @@ +package annotation + +import ( + "cmp" + "math/big" + "strings" + + "github.com/dexpace/morphic/ir" +) + +// decimalBound is a numeric literal split into the three pieces an exact +// comparison needs: its sign, its significant digits with the point removed and +// the leading zeros stripped, and the power of ten the first of those digits +// carries. digits is empty exactly when the value is zero, which is what makes +// "0", "-0.0" and "0e9" one value here rather than several. +// +// The split is what keeps the comparison total. Reading a bound as a number +// instead means materializing it, and 1e1000001 — legal in a spec, and kept +// intact by ir.NewBigVal — is a magnitude math/big will not build as a rational +// at all. Nothing about ordering two bounds needs those digits anyway: the +// exponent alone separates them. +type decimalBound struct { + neg bool + digits string + msdExp *big.Int +} + +// parseDecimalBound splits the canonical form ir.NewBigVal returns — an +// optional "-", digits, an optional fraction, an optional e/E exponent — into a +// decimalBound. +// +// It reports false for a literal outside that grammar. Every bound the readers +// here produce comes through ir.NewBigVal, which still stores a binary exponent +// verbatim: "1p4" is kept as written and means 16 (GitHub #45). Reading the +// digits out of one and ordering what is left would put a bound the source never +// wrote into the IR, so it declines rather than guessing. +func parseDecimalBound(v ir.BigVal) (decimalBound, bool) { + unsigned, neg := strings.CutPrefix(v.String(), "-") + + mantissa, expText := unsigned, "0" + if i := strings.IndexAny(unsigned, "eE"); i >= 0 { + mantissa, expText = unsigned[:i], unsigned[i+1:] + } + intPart, frac := mantissa, "" + if i := strings.IndexByte(mantissa, '.'); i >= 0 { + intPart, frac = mantissa[:i], mantissa[i+1:] + } + + digits := intPart + frac + if !isDigits(digits) { + return decimalBound{}, false + } + // A big.Int rather than an int64: the exponent arrives as text the source + // wrote, and a fixed width that overflowed on it would order the two bounds + // by a number neither of them has. + exp, ok := new(big.Int).SetString(expText, 10) + if !ok { + return decimalBound{}, false + } + + // digits reads as an integer scaled by 10**-len(frac), and stripping its + // leading zeros leaves that integer alone — so what is left is significant + // digits whose first one carries 10**(exp-len(frac)+len(significant)-1). + significant := strings.TrimLeft(digits, "0") + if significant == "" { + return decimalBound{neg: neg, msdExp: new(big.Int)}, true + } + return decimalBound{ + neg: neg, + digits: significant, + msdExp: exp.Add(exp, big.NewInt(int64(len(significant)-len(frac)-1))), + }, true +} + +// sign reports the bound's sign as -1, 0 or +1. Having no digits is checked +// first because a literal can carry a minus and still be zero: "-0.0" is the +// same bound as "0", and reading its sign off the minus would order it below. +func (d decimalBound) sign() int { + switch { + case d.digits == "": + return 0 + case d.neg: + return -1 + default: + return 1 + } +} + +// compareDecimalBounds returns -1, 0 or +1 as a is less than, equal to, or +// greater than b. The comparison is exact for every literal parseDecimalBound +// accepts, however far apart the two magnitudes are: sign first, then the power +// of ten the leading digit carries, and only then the digits themselves. +func compareDecimalBounds(a, b decimalBound) int { + if order := cmp.Compare(a.sign(), b.sign()); order != 0 { + return order + } + if a.sign() == 0 { + return 0 + } + + order := a.msdExp.Cmp(b.msdExp) + if order == 0 { + order = compareDigits(a.digits, b.digits) + } + if a.neg { + return -order + } + return order +} + +// compareDigits orders two digit runs whose leading digits carry the same power +// of ten, reading a run that has ended as the trailing zeros it stands for — +// which is what puts 5e1 and 50 at one value rather than two. +func compareDigits(a, b string) int { + for i := range max(len(a), len(b)) { + if order := cmp.Compare(digitAt(a, i), digitAt(b, i)); order != 0 { + return order + } + } + return 0 +} + +// digitAt returns s[i], or '0' past the end of s. +func digitAt(s string, i int) byte { + if i < len(s) { + return s[i] + } + return '0' +} + +// isDigits reports whether s is a non-empty run of decimal digits. The empty +// run is not one: a literal with no digits at all denotes no value, and reading +// it as zero would order it against real bounds. +func isDigits(s string) bool { + if s == "" { + return false + } + for i := range len(s) { + if s[i] < '0' || s[i] > '9' { + return false + } + } + return true +} diff --git a/compilers/openapi/internal/annotation/decimal_internal_test.go b/compilers/openapi/internal/annotation/decimal_internal_test.go new file mode 100644 index 0000000..319ab01 --- /dev/null +++ b/compilers/openapi/internal/annotation/decimal_internal_test.go @@ -0,0 +1,69 @@ +package annotation + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// TestCompareDecimalBounds_OrdersEveryDecimalSpellingExactly pins the ordering +// the bound reconciliation rests on. Every row is a pair a rounding comparison +// would get wrong: two values that differ past float64's precision, two that +// differ only in spelling, magnitudes past what a rational will hold, and the +// signed zeros. Each is asserted in both directions, so a comparison that +// happens to be right one way round is not mistaken for one that orders. +func TestCompareDecimalBounds_OrdersEveryDecimalSpellingExactly(t *testing.T) { + t.Parallel() + tests := []struct { + name string + a, b string + want int + }{ + {name: "one value, one spelling", a: "1", b: "1", want: 0}, + {name: "an exponent against the digits it stands for", a: "1e2", b: "100", want: 0}, + {name: "a trailing zero the exponent moved", a: "5e1", b: "50", want: 0}, + {name: "a fraction against its exponent form", a: "0.001", b: "1e-3", want: 0}, + {name: "zero however it is written", a: "0", b: "-0.0", want: 0}, + {name: "zero with an exponent is still zero", a: "-0.0", b: "0e9", want: 0}, + {name: "a digit past float64's exact range", a: "9007199254740993", b: "9007199254740992", want: 1}, + {name: "a decimal float64 cannot hold", a: "1.0000000000000001", b: "1.0000000000000002", want: -1}, + {name: "a magnitude no rational holds", a: "1e1000001", b: "5", want: 1}, + {name: "a magnitude too small for one", a: "1e-1000001", b: "5", want: -1}, + {name: "sign beats magnitude", a: "-1e1000001", b: "5", want: -1}, + {name: "two negatives order by magnitude", a: "-5", b: "-10", want: 1}, + {name: "negative against positive", a: "-5", b: "5", want: -1}, + {name: "zero against a negative", a: "0", b: "-1", want: 1}, + {name: "a leading zero carries nothing", a: "0.5", b: "00.50", want: 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + a, aOK := parseDecimalBound(ir.BigVal(tc.a)) + b, bOK := parseDecimalBound(ir.BigVal(tc.b)) + require.True(t, aOK, "%q is a decimal literal", tc.a) + require.True(t, bOK, "%q is a decimal literal", tc.b) + + assert.Equal(t, tc.want, compareDecimalBounds(a, b), "%s against %s", tc.a, tc.b) + assert.Equal(t, -tc.want, compareDecimalBounds(b, a), "%s against %s", tc.b, tc.a) + }) + } +} + +// TestParseDecimalBound_DeclinesWhatIsNotADecimalLiteral pins the one honest +// answer for a literal outside the grammar: none. Reading digits out of one and +// ordering what is left would put a bound the source never wrote into the IR — +// "1p4" is 16, not 1, and a comparison that took the 1 would keep the wrong +// bound while reporting a comparison it never made. +func TestParseDecimalBound_DeclinesWhatIsNotADecimalLiteral(t *testing.T) { + t.Parallel() + for _, literal := range []string{"", "-", ".", "1p4", "2.5p-2", "1e", "1e1_0", "1.2.3", "+5", "0x10"} { + t.Run(literal, func(t *testing.T) { + t.Parallel() + _, ok := parseDecimalBound(ir.BigVal(literal)) + assert.False(t, ok, "%q is not a decimal literal", literal) + }) + } +} From 3c956d8002fcb889ee1765c756d9338b1ec32a0d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 11:43:19 +0300 Subject: [PATCH 5/9] test(compilers/openapi): say what the unvalidated parse helper is for --- .../annotation/constraints_readers_internal_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index 8da4959..4be425e 100644 --- a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go +++ b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go @@ -17,11 +17,12 @@ import ( // schemaFromYAMLUnvalidated parses body like schemaFromYAML but keeps the // library's own validation findings instead of requiring none. // -// A bound beyond float64 range is the case it exists for: the library types -// these keywords as float64 and reports the literal as a string it could not -// convert, while the compiler's loader suppresses exactly that finding because -// such a bound is valid and must survive. Requiring a clean parse here would -// put every out-of-float64-range bound out of this package's reach. +// A literal the library cannot read as a float64 is the case it exists for — +// a magnitude beyond that range, or a spelling that is no number to it at all. +// The library types these keywords as float64 and reports the literal as a +// string it could not convert, while the compiler's loader suppresses exactly +// that finding because such a bound may still be valid and must survive. +// Requiring a clean parse here would put every one of them out of reach. func schemaFromYAMLUnvalidated(t *testing.T, body string) *oas3.Schema { t.Helper() var js oas3.JSONSchema[oas3.Referenceable] From 4a5e74b586dd15139b5a64730c61aed28df414a7 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 18:17:05 +0300 Subject: [PATCH 6/9] test(compilers/openapi): hold the bound grammars together at the seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../internal/annotation/constraints.go | 9 +++-- .../constraints_readers_internal_test.go | 26 +++++++------ .../openapi/internal/annotation/decimal.go | 15 ++++--- .../annotation/decimal_internal_test.go | 39 +++++++++++++++++++ 4 files changed, 70 insertions(+), 19 deletions(-) diff --git a/compilers/openapi/internal/annotation/constraints.go b/compilers/openapi/internal/annotation/constraints.go index 1ede7bb..ebf5dba 100644 --- a/compilers/openapi/internal/annotation/constraints.go +++ b/compilers/openapi/internal/annotation/constraints.go @@ -170,9 +170,12 @@ func reconcileBound(c *ir.Constraints, isMin bool, excl ir.BigVal) []ir.Diagnost // is not: math/big will not build 1e1000001 as one, and a bound it cannot order // is a bound it may silently widen. // -// What it cannot order is a literal that is not decimal — BigVal still stores a -// binary exponent verbatim (GitHub #45) — and there the caller keeps the -// exclusive bound and says the other may have been the tighter. +// What it cannot order is a literal outside the decimal grammar, and there the +// caller keeps the exclusive bound and says the other may have been the tighter. +// No schema reaches that today — every bound comes through ir.NewBigVal, whose +// grammar is the narrower of the two — so it stands for the day that changes: +// a bound this cannot order is one that could be silently replaced by the looser +// of its pair, which is the defect this reconciliation exists to prevent. func inclusiveIsTighter(incl, excl ir.BigVal, isMin bool) (tighter, compared bool) { inclDec, inclOK := parseDecimalBound(incl) exclDec, exclOK := parseDecimalBound(excl) diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index 4be425e..f944ead 100644 --- a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go +++ b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go @@ -394,21 +394,25 @@ func TestReconcileBound_AMagnitudeNoRationalHoldsStillCompares(t *testing.T) { } } -// TestReconcileBound_ALiteralThatIsNotDecimalKeepsTheExclusiveBound pins what -// is left of the fallback. ir.NewBigVal still stores a binary exponent verbatim -// (GitHub #45), so "1p4" reaches this reader as a bound meaning 16 that no -// decimal reading orders. The reader keeps the exclusive bound and says the -// discarded one may have been the tighter, rather than claiming a comparison it -// did not make. -func TestReconcileBound_ALiteralThatIsNotDecimalKeepsTheExclusiveBound(t *testing.T) { +// TestReconcileBound_ABoundNoDecimalReadingOrdersKeepsTheExclusiveOne pins the +// guard standing at this reader's boundary with ir.NewBigVal. +// +// It is driven through reconcileBound rather than through a schema because no +// schema reaches it: every bound arrives via ir.NewBigVal, whose grammar +// TestBigValGrammarStaysWithinTheDecimalReading holds inside the one +// parseDecimalBound orders. The guard is what keeps a later widening of that +// grammar from widening a bound instead — a bound that cannot be ordered is one +// that could be silently replaced by the looser of the pair — so it keeps the +// exclusive bound and says the discarded one may have been the tighter, rather +// than claiming a comparison it never made. +func TestReconcileBound_ABoundNoDecimalReadingOrdersKeepsTheExclusiveOne(t *testing.T) { t.Parallel() - s := schemaFromYAMLUnvalidated(t, "type: number\nminimum: 1p4\nexclusiveMinimum: 5\n") + c := &ir.Constraints{Min: bigOf("1p4")} - got, diags := Constraints(s, false) + diags := reconcileBound(c, true, ir.BigVal("5")) - require.NotNil(t, got) want := ir.Constraints{Min: bigOf("5"), ExclusiveMin: true} - if diff := cmp.Diff(want, *got); diff != "" { + if diff := cmp.Diff(want, *c); diff != "" { t.Errorf("constraints (-want +got):\n%s", diff) } require.Len(t, diags, 1) diff --git a/compilers/openapi/internal/annotation/decimal.go b/compilers/openapi/internal/annotation/decimal.go index 916969e..f6a922b 100644 --- a/compilers/openapi/internal/annotation/decimal.go +++ b/compilers/openapi/internal/annotation/decimal.go @@ -29,11 +29,16 @@ type decimalBound struct { // optional "-", digits, an optional fraction, an optional e/E exponent — into a // decimalBound. // -// It reports false for a literal outside that grammar. Every bound the readers -// here produce comes through ir.NewBigVal, which still stores a binary exponent -// verbatim: "1p4" is kept as written and means 16 (GitHub #45). Reading the -// digits out of one and ordering what is left would put a bound the source never -// wrote into the IR, so it declines rather than guessing. +// It reports false for a literal outside that grammar rather than guessing: +// reading the digits out of one and ordering what is left would put a bound the +// source never wrote into the IR — "1p4" is 16, and a comparison that took the +// 1 would keep the wrong bound while reporting an ordering it never made. +// +// No bound a schema produces is outside it today, since they all come through +// ir.NewBigVal and its grammar is the narrower of the two; +// TestBigValGrammarStaysWithinTheDecimalReading is what holds that true. This +// stays fallible because the two grammars live in different packages and have +// already moved apart once. func parseDecimalBound(v ir.BigVal) (decimalBound, bool) { unsigned, neg := strings.CutPrefix(v.String(), "-") diff --git a/compilers/openapi/internal/annotation/decimal_internal_test.go b/compilers/openapi/internal/annotation/decimal_internal_test.go index 319ab01..cda55a0 100644 --- a/compilers/openapi/internal/annotation/decimal_internal_test.go +++ b/compilers/openapi/internal/annotation/decimal_internal_test.go @@ -67,3 +67,42 @@ func TestParseDecimalBound_DeclinesWhatIsNotADecimalLiteral(t *testing.T) { }) } } + +// TestBigValGrammarStaysWithinTheDecimalReading pins the coupling that decides +// whether reconcileBound's incomparable guard is reachable: every literal +// ir.NewBigVal accepts must be one parseDecimalBound can order. +// +// While it holds, no schema reaches that guard — which is why the test for it +// calls reconcileBound directly. The two grammars live in different packages +// and have already moved apart once, so nothing but this holds them together: +// when ir widens NewBigVal, a bound it now admits and this reader cannot order +// is a bound that would be silently replaced by the looser of its pair, and +// that has to fail here rather than in a compiled document. +func TestBigValGrammarStaysWithinTheDecimalReading(t *testing.T) { + t.Parallel() + signs := []string{"", "-", "+"} + mantissas := []string{ + "0", "00", "0.0", ".5", "5.", "1", "10", "100", "1.0", "01", "007", "0.5", + "0.05", "123456789012345678901234567890", "9007199254740993", "1.7976931348623157", + } + exponents := []string{"", "e0", "e1", "e-1", "e+1", "E5", "e308", "e-308", "e1000001", "e-1000001"} + + var accepted int + for _, sign := range signs { + for _, mantissa := range mantissas { + for _, exponent := range exponents { + literal := sign + mantissa + exponent + v, err := ir.NewBigVal(literal) + if err != nil { + continue // not a bound at all; nothing for this reader to order + } + accepted++ + _, ok := parseDecimalBound(v) + assert.True(t, ok, "NewBigVal(%q) = %q, which this reader cannot order", literal, v) + } + } + } + // Without this the loop would pass by accepting nothing at all, which is + // exactly what a widened rejection in ir would look like from here. + require.NotZero(t, accepted, "the corpus reached NewBigVal") +} From 62ecf98a5a1f8c13cad1641f9e1dab05a2804cbc Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 18:20:47 +0300 Subject: [PATCH 7/9] test(compilers/openapi): narrow the unvalidated helper's stated purpose --- .../annotation/constraints_readers_internal_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index f944ead..c92aaae 100644 --- a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go +++ b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go @@ -17,12 +17,11 @@ import ( // schemaFromYAMLUnvalidated parses body like schemaFromYAML but keeps the // library's own validation findings instead of requiring none. // -// A literal the library cannot read as a float64 is the case it exists for — -// a magnitude beyond that range, or a spelling that is no number to it at all. -// The library types these keywords as float64 and reports the literal as a +// A bound whose magnitude is beyond float64's range is the case it exists for. +// The library types these keywords as float64 and reports such a literal as a // string it could not convert, while the compiler's loader suppresses exactly -// that finding because such a bound may still be valid and must survive. -// Requiring a clean parse here would put every one of them out of reach. +// that finding because the bound is still valid and must survive. Requiring a +// clean parse here would put every one of them out of reach. func schemaFromYAMLUnvalidated(t *testing.T, body string) *oas3.Schema { t.Helper() var js oas3.JSONSchema[oas3.Referenceable] From 629c2f071aa87a70dbf6585db5aed7b715882c31 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 18:23:49 +0300 Subject: [PATCH 8/9] test(compilers/openapi): tell the 3.0 exclusive sides apart 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. --- .../constraints_readers_internal_test.go | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index c92aaae..805375c 100644 --- a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go +++ b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go @@ -341,6 +341,26 @@ func TestReconcileBound_ThreeZeroDialectPairIsUntouched(t *testing.T) { } } +// TestApplyExclusive_ThreeZeroFlagsTheSideItWasReadFrom pins which side the 3.0 +// boolean arm marks exclusive. +// +// The case above declares the keyword on both sides, and every other 3.0 case +// here does too — where flagging the wrong side is symmetric, so a reader that +// crossed them over produces exactly the expected constraints. Only a schema +// exclusive on one side can tell the two apart. +func TestApplyExclusive_ThreeZeroFlagsTheSideItWasReadFrom(t *testing.T) { + t.Parallel() + got, diags := Constraints(schemaFromYAML(t, + "type: number\nminimum: 10\nexclusiveMinimum: true\nmaximum: 20\n"), true) + + require.NotNil(t, got) + assert.Empty(t, diags) + want := ir.Constraints{Min: bigOf("10"), Max: bigOf("20"), ExclusiveMin: true} + if diff := cmp.Diff(want, *got); diff != "" { + t.Errorf("constraints (-want +got):\n%s", diff) + } +} + // TestReconcileBound_AMagnitudeNoRationalHoldsStillCompares pins the exactness // of the comparison at the size where the obvious way to make it gives out. // math/big will not build 1e2000000 as a rational — the exponent is past its From 4d8cefedc34b0e5d5840cd07dda31473b0158a5a Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 19:30:43 +0300 Subject: [PATCH 9/9] test(compilers/openapi): give the grammar seam a widening to catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../annotation/decimal_internal_test.go | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/compilers/openapi/internal/annotation/decimal_internal_test.go b/compilers/openapi/internal/annotation/decimal_internal_test.go index cda55a0..44175ce 100644 --- a/compilers/openapi/internal/annotation/decimal_internal_test.go +++ b/compilers/openapi/internal/annotation/decimal_internal_test.go @@ -78,6 +78,14 @@ func TestParseDecimalBound_DeclinesWhatIsNotADecimalLiteral(t *testing.T) { // when ir widens NewBigVal, a bound it now admits and this reader cannot order // is a bound that would be silently replaced by the looser of its pair, and // that has to fail here rather than in a compiled document. +// +// The spellings NewBigVal refuses today are the load-bearing half of the +// corpus, and are not dead weight to be tidied away. A corpus of accepted +// spellings alone can only ever confirm what is already true: every one of them +// parses, so no widening it does not already contain can redden it. A binary +// exponent is the widening that actually happened here, and it is a shape no +// sweep of decimal spellings generates. These are fed in and skipped while they +// are refused; the day one is accepted, the assertion below meets it. func TestBigValGrammarStaysWithinTheDecimalReading(t *testing.T) { t.Parallel() signs := []string{"", "-", "+"} @@ -87,21 +95,34 @@ func TestBigValGrammarStaysWithinTheDecimalReading(t *testing.T) { } exponents := []string{"", "e0", "e1", "e-1", "e+1", "E5", "e308", "e-308", "e1000001", "e-1000001"} - var accepted int + // Refused today, every one of them a way the grammar could widen: a binary + // exponent (the one it has already had), another base, a digit separator, + // and the non-numbers big.ParseFloat knows by name. + widenings := []string{ + "1p4", "1P4", "0x10", "0X1p-2", "0b11", "0o17", "1_000", "1e1_0", + "Inf", "-Inf", "NaN", "1e", "1.2.3", "--1", ".", + } + + literals := make([]string, 0, len(widenings)+len(signs)*len(mantissas)*len(exponents)) + literals = append(literals, widenings...) for _, sign := range signs { for _, mantissa := range mantissas { for _, exponent := range exponents { - literal := sign + mantissa + exponent - v, err := ir.NewBigVal(literal) - if err != nil { - continue // not a bound at all; nothing for this reader to order - } - accepted++ - _, ok := parseDecimalBound(v) - assert.True(t, ok, "NewBigVal(%q) = %q, which this reader cannot order", literal, v) + literals = append(literals, sign+mantissa+exponent) } } } + + var accepted int + for _, literal := range literals { + v, err := ir.NewBigVal(literal) + if err != nil { + continue // not a bound at all; nothing for this reader to order + } + accepted++ + _, ok := parseDecimalBound(v) + assert.True(t, ok, "NewBigVal(%q) = %q, which this reader cannot order", literal, v) + } // Without this the loop would pass by accepting nothing at all, which is // exactly what a widened rejection in ir would look like from here. require.NotZero(t, accepted, "the corpus reached NewBigVal")