Merge pull request #193 from grafana/sdboyer/fix-backcompat-checks

thema: Fix backwards compatibility checking bugs

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)

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 {

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",
 		},
 	}
 

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"

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

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

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"