From 1673cd523d4cfb5ddc59e8f7587c8d1fd0702156 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 13 Aug 2023 22:48:49 -0400 Subject: [PATCH] thema: Fix backwards compatibility checking bugs The entire fix here was: - Checking a closed def (`_#schema`) instead of the original, typically open `schema` field - Getting rid of cue.Schema() and cue.Final() as options to Subsume(), because they weren't doing what we thought they did Then all the backwards compatibility checks just started working, exactly like we'd originally hoped. --- bind.go | 2 +- internal/compat/compat.go | 8 +++- lineage_test.go | 8 ++-- .../compat/change-default.txtar | 19 +++++++- .../compat/remove-optional.txtar | 4 +- .../compat/remove-required.txtar | 4 +- .../compat/union-reduction.txtar | 43 +++++++++++++++++++ 7 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 testdata/invalidlineage/compat/union-reduction.txtar diff --git a/bind.go b/bind.go index f5df1f86..572cfc3a 100644 --- a/bind.go +++ b/bind.go @@ -95,7 +95,7 @@ func (ml *maybeLineage) checkGoValidity(cfg *bindConfig) error { sch.ref = schiter.Value() sch.def = sch.ref.LookupPath(pathSchDef) if previous != nil && !cfg.skipbuggychecks { - compaterr := compat.ThemaCompatible(previous.ref.LookupPath(pathSch), sch.ref.LookupPath(pathSch)) + compaterr := compat.ThemaCompatible(previous.def, sch.def) if sch.v[1] == 0 && compaterr == nil { // Major version change, should be backwards incompatible return errors.Mark(mkerror(sch.ref.LookupPath(pathSch), "schema %s must be backwards incompatible with schema %s: introduce a breaking change, or redeclare as version %s", sch.v, previous.v, synv(previous.v[0], previous.v[1]+1)), terrors.ErrInvalidLineage) diff --git a/internal/compat/compat.go b/internal/compat/compat.go index bddaad06..0255c1a6 100644 --- a/internal/compat/compat.go +++ b/internal/compat/compat.go @@ -5,9 +5,13 @@ import ( ) // ThemaCompatible is the canonical Thema algorithm for checking that the -// cue.Value s is (backwards) compatible with p. +// [cue.Value] s is (backwards) compatible with p. A nil return indicates +// compatibility. +// +// The behavior of this function is undefined if s and p are not closed +// structs. TODO check this and error if conditions aren't met func ThemaCompatible(p, s cue.Value) error { - return s.Subsume(p, cue.Raw(), cue.Schema(), cue.Definitions(true), cue.All(), cue.Final()) + return s.Subsume(p, cue.Raw(), cue.All()) } // type CompatInvariantError struct { diff --git a/lineage_test.go b/lineage_test.go index 7f150003..4455252a 100644 --- a/lineage_test.go +++ b/lineage_test.go @@ -10,6 +10,7 @@ import ( "cuelang.org/go/cue/cuecontext" "cuelang.org/go/cue/errors" "cuelang.org/go/cue/load" + "github.com/grafana/thema/internal/txtartest/vanilla" ) @@ -64,11 +65,8 @@ func TestInvalidLineages(t *testing.T) { Name: "bindfail", ThemaFS: CueJointFS, ToDo: map[string]string{ - "invalidlineage/joindef": "no invariant checker written to disallow definitions from joinSchema", - "invalidlineage/onlydef": "Lineage schema non-emptiness constraints are temporarily suspended while migrating grafana to flattened lineage structure", - "invalidlineage/compat/change-default": "Thema compat analyzer fails to classify changes to default values as breaking", - "invalidlineage/compat/remove-required": "Required field removal is not detected as breaking changes", - "invalidlineage/compat/remove-optional": "Optional field removal is not detected as breaking changes", + "invalidlineage/joindef": "no invariant checker written to disallow definitions from joinSchema", + "invalidlineage/onlydef": "Lineage schema non-emptiness constraints are temporarily suspended while migrating grafana to flattened lineage structure", }, } diff --git a/testdata/invalidlineage/compat/change-default.txtar b/testdata/invalidlineage/compat/change-default.txtar index 0b461725..a65c7df3 100644 --- a/testdata/invalidlineage/compat/change-default.txtar +++ b/testdata/invalidlineage/compat/change-default.txtar @@ -14,6 +14,12 @@ schemas: [ }, { version: [0, 1] + schema: { + aunion: *"foo" | "bar" | "baz" + } + }, + { + version: [0, 2] schema: { aunion: "foo" | "bar" | *"baz" } @@ -29,6 +35,17 @@ lenses: [ aunion: input.aunion } }, + { + to: [0, 1] + from: [0, 2] + input: _ + result: { + aunion: input.aunion + } + }, ] -- out/bindfail -- -schema 0.1 must be backwards compatible with schema 0.0 +schema 0.2 is not backwards compatible with schema 0.1: +field aunion not present in {aunion:*"foo" | "bar" | "baz"}: + /cue.mod/pkg/github.com/grafana/thema/lineage.cue:234:12 +missing field "aunion" \ No newline at end of file diff --git a/testdata/invalidlineage/compat/remove-optional.txtar b/testdata/invalidlineage/compat/remove-optional.txtar index 45dcbf03..258eb307 100644 --- a/testdata/invalidlineage/compat/remove-optional.txtar +++ b/testdata/invalidlineage/compat/remove-optional.txtar @@ -29,4 +29,6 @@ lin: lenses: [{ } }] -- out/bindfail -- -schema 0.1 must be backwards compatible with schema 0.0 +schema 0.1 is not backwards compatible with schema 0.0: +field not allowed in closed struct: getsRemoved +value not an instance diff --git a/testdata/invalidlineage/compat/remove-required.txtar b/testdata/invalidlineage/compat/remove-required.txtar index 452c1b35..9e8d1f51 100644 --- a/testdata/invalidlineage/compat/remove-required.txtar +++ b/testdata/invalidlineage/compat/remove-required.txtar @@ -30,4 +30,6 @@ lin: lenses: [{ } }] -- out/bindfail -- -schema 0.1 must be backwards compatible with schema 0.0 +schema 0.1 is not backwards compatible with schema 0.0: +field not allowed in closed struct: getsRemoved +value not an instance \ No newline at end of file diff --git a/testdata/invalidlineage/compat/union-reduction.txtar b/testdata/invalidlineage/compat/union-reduction.txtar new file mode 100644 index 00000000..58b4f418 --- /dev/null +++ b/testdata/invalidlineage/compat/union-reduction.txtar @@ -0,0 +1,43 @@ +# reducing the permitted options in a union/disjunction is backwards incompatible +#lineagePath: lin +-- in.cue -- + +import "github.com/grafana/thema" + +lin: thema.#Lineage +lin: name: "union-reduction" +lin: schemas: [{ + version: [0, 0] + schema: { + concreteCross: "foo" | "bar" | 42 + concreteString: "foo" | "bar" | "baz" + crossKind3: string | int32 | bytes + crossKind2: string | int32 + } +}, +{ + version: [0, 1] + schema: { + concreteCross: "foo" | 42 + concreteString: "foo" | "bar" + crossKind3: string | int32 + crossKind2: string + } +}] + +lin: lenses: [{ + from: [0, 1] + to: [0, 0] + input: _ + result: { + concreteCross: input.concreteCross + concreteString: input.concreteString + crossKind3: input.crossKind3 + crossKind2: input.crossKind2 + } +}] +-- out/bindfail -- +schema 0.1 is not backwards compatible with schema 0.0: +field concreteCross not present in {concreteCross:"foo" | "bar" | 42,concreteString:"foo" | "bar" | "baz",crossKind3:string | >=-2147483648 & <=2147483647 & int | bytes,crossKind2:string | >=-2147483648 & <=2147483647 & int}: + /cue.mod/pkg/github.com/grafana/thema/lineage.cue:234:12 +missing field "concreteCross" \ No newline at end of file