Merge pull request #1788 from josephschorr/caveat-diff

Have caveat diffs properly check if an expression has changed

pkg/caveats/compile.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/authzed/cel-go/cel"
 	"github.com/authzed/cel-go/common"
+	"google.golang.org/protobuf/proto"
 
 	"github.com/authzed/spicedb/pkg/caveats/types"
 	"github.com/authzed/spicedb/pkg/genutil/mapz"
@@ -52,7 +53,9 @@ func (cc CompiledCaveat) Serialize() ([]byte, error) {
 		Name: cc.name,
 	}
 
-	return caveat.MarshalVT()
+	// TODO(jschorr): change back to MarshalVT once stable is supported.
+	// See: https://github.com/planetscale/vtprotobuf/pull/133
+	return proto.MarshalOptions{Deterministic: true}.Marshal(caveat)
 }
 
 // ReferencedParameters returns the names of the parameters referenced in the expression.

pkg/diff/caveats/diff.go

@@ -34,10 +34,8 @@ const (
 	// ParameterTypeChanged indicates that the type of the parameter was changed.
 	ParameterTypeChanged DeltaType = "parameter-type-changed"
 
-	// CaveatExpressionMayHaveChanged indicates that the expression of the caveat *may* have changed.
-	// This uses a direct byte comparison which can return that a change occurred, even when it has
-	// not.
-	CaveatExpressionMayHaveChanged DeltaType = "expression-may-have-changed"
+	// CaveatExpressionChanged indicates that the expression of the caveat has changed.
+	CaveatExpressionChanged DeltaType = "expression-has-changed"
 )
 
 // Diff holds the diff between two caveats.
@@ -154,7 +152,7 @@ func DiffCaveats(existing *core.CaveatDefinition, updated *core.CaveatDefinition
 
 	if !bytes.Equal(existing.SerializedExpression, updated.SerializedExpression) {
 		deltas = append(deltas, Delta{
-			Type: CaveatExpressionMayHaveChanged,
+			Type: CaveatExpressionChanged,
 		})
 	}
 

pkg/diff/caveats/diff_test.go

@@ -187,7 +187,7 @@ func TestCaveatDiff(t *testing.T) {
 				"false",
 			),
 			[]Delta{
-				{Type: CaveatExpressionMayHaveChanged},
+				{Type: CaveatExpressionChanged},
 			},
 		},
 		{
@@ -207,9 +207,65 @@ func TestCaveatDiff(t *testing.T) {
 				"someparam <= 1",
 			),
 			[]Delta{
-				{Type: CaveatExpressionMayHaveChanged},
+				{Type: CaveatExpressionChanged},
 			},
 		},
+		{
+			"third changed expression",
+			ns.MustCaveatDefinition(
+				caveats.MustEnvForVariables(map[string]types.VariableType{
+					"someparam": types.IntType,
+				}),
+				"somecaveat",
+				"someparam == 1",
+			),
+			ns.MustCaveatDefinition(
+				caveats.MustEnvForVariables(map[string]types.VariableType{
+					"someparam": types.IntType,
+				}),
+				"somecaveat",
+				"someparam == 2",
+			),
+			[]Delta{
+				{Type: CaveatExpressionChanged},
+			},
+		},
+		{
+			"unchanged expression",
+			ns.MustCaveatDefinition(
+				caveats.MustEnvForVariables(map[string]types.VariableType{
+					"someparam": types.IntType,
+				}),
+				"somecaveat",
+				"someparam == 1",
+			),
+			ns.MustCaveatDefinition(
+				caveats.MustEnvForVariables(map[string]types.VariableType{
+					"someparam": types.IntType,
+				}),
+				"somecaveat",
+				"someparam == 1",
+			),
+			[]Delta{},
+		},
+		{
+			"unchanged slightly more complex expression",
+			ns.MustCaveatDefinition(
+				caveats.MustEnvForVariables(map[string]types.VariableType{
+					"someparam": types.IntType,
+				}),
+				"somecaveat",
+				"someparam == {\"a\": 42}.a",
+			),
+			ns.MustCaveatDefinition(
+				caveats.MustEnvForVariables(map[string]types.VariableType{
+					"someparam": types.IntType,
+				}),
+				"somecaveat",
+				"someparam == {\"a\": 42}.a",
+			),
+			[]Delta{},
+		},
 	}
 
 	for _, tc := range testCases {