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/compilers/openapi/internal/annotation/constraints.go b/compilers/openapi/internal/annotation/constraints.go index a1c4e48..ebf5dba 100644 --- a/compilers/openapi/internal/annotation/constraints.go +++ b/compilers/openapi/internal/annotation/constraints.go @@ -15,7 +15,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 +83,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 +113,102 @@ 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 +// 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 { + 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 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 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) + if !inclOK || !exclOK { + return false, false + } + order := compareDecimalBounds(inclDec, exclDec) + 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 +234,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 diff --git a/compilers/openapi/internal/annotation/constraints_readers_internal_test.go b/compilers/openapi/internal/annotation/constraints_readers_internal_test.go index ee657cd..805375c 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 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 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] + _, 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,237 @@ 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"}, + }, + { + 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) { + 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) + } +} + +// 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 +// 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_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() + c := &ir.Constraints{Min: bigOf("1p4")} + + diags := reconcileBound(c, true, ir.BigVal("5")) + + want := ir.Constraints{Min: bigOf("5"), ExclusiveMin: true} + if diff := cmp.Diff(want, *c); 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 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..f6a922b --- /dev/null +++ b/compilers/openapi/internal/annotation/decimal.go @@ -0,0 +1,149 @@ +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 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(), "-") + + 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..44175ce --- /dev/null +++ b/compilers/openapi/internal/annotation/decimal_internal_test.go @@ -0,0 +1,129 @@ +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) + }) + } +} + +// 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. +// +// 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{"", "-", "+"} + 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"} + + // 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 { + 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") +} 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: