From 3f77ff2de885050ea0b833378891311ad5b7fc87 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Wed, 21 Feb 2024 16:31:18 -0500
Subject: [PATCH] Small improvement in tuple package to remove TODO

---
 pkg/tuple/tuple.go      | 17 +++++++++++++++--
 pkg/tuple/tuple_test.go | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/pkg/tuple/tuple.go b/pkg/tuple/tuple.go
index 7b97cbd72b..6929f6c8ea 100644
--- a/pkg/tuple/tuple.go
+++ b/pkg/tuple/tuple.go
@@ -10,6 +10,7 @@ import (
 
 	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
 	"github.com/jzelinskie/stringz"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/structpb"
 
 	core "github.com/authzed/spicedb/pkg/proto/core/v1"
@@ -271,9 +272,21 @@ func Delete(tpl *core.RelationTuple) *core.RelationTupleUpdate {
 	}
 }
 
+// Equal returns true if the two relationships are exactly the same.
 func Equal(lhs, rhs *core.RelationTuple) bool {
-	// TODO(jschorr): Use a faster method then string comparison for caveats.
-	return OnrEqual(lhs.ResourceAndRelation, rhs.ResourceAndRelation) && OnrEqual(lhs.Subject, rhs.Subject) && MustStringCaveat(lhs.Caveat) == MustStringCaveat(rhs.Caveat)
+	return OnrEqual(lhs.ResourceAndRelation, rhs.ResourceAndRelation) && OnrEqual(lhs.Subject, rhs.Subject) && caveatEqual(lhs.Caveat, rhs.Caveat)
+}
+
+func caveatEqual(lhs, rhs *core.ContextualizedCaveat) bool {
+	if lhs == nil && rhs == nil {
+		return true
+	}
+
+	if lhs == nil || rhs == nil {
+		return false
+	}
+
+	return lhs.CaveatName == rhs.CaveatName && proto.Equal(lhs.Context, rhs.Context)
 }
 
 // MustToRelationship converts a RelationTuple into a Relationship. Will panic if
diff --git a/pkg/tuple/tuple_test.go b/pkg/tuple/tuple_test.go
index f8a37a9d3b..26a81592e4 100644
--- a/pkg/tuple/tuple_test.go
+++ b/pkg/tuple/tuple_test.go
@@ -583,12 +583,17 @@ func TestEqual(t *testing.T) {
 				},
 			},
 		),
+		MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":\"there\"}]"),
+		MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":123}}]"),
+		MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":{\"hey\":true}}, \"hi2\":{\"yo2\":{\"hey2\":false}}}]"),
+		MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":{\"hey\":true}}, \"hi2\":{\"yo2\":{\"hey2\":[1,2,3]}}}]"),
 	}
 
 	for _, tc := range equalTestCases {
 		t.Run(MustString(tc), func(t *testing.T) {
 			require := require.New(t)
 			require.True(Equal(tc, tc.CloneVT()))
+			require.True(Equal(tc, MustParse(MustString(tc))))
 		})
 	}
 
@@ -725,12 +730,40 @@ func TestEqual(t *testing.T) {
 				},
 			),
 		},
+		{
+			name: "missing caveat context via string",
+			lhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":\"there\"}]"),
+			rhs:  MustParse("document:foo#viewer@user:tom[somecaveat]"),
+		},
+		{
+			name: "mismatch caveat context via string",
+			lhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":\"there\"}]"),
+			rhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":\"there2\"}]"),
+		},
+		{
+			name: "mismatch caveat name",
+			lhs:  MustParse("document:foo#viewer@user:tom[somecaveat]"),
+			rhs:  MustParse("document:foo#viewer@user:tom[somecaveat2]"),
+		},
+		{
+			name: "mismatch caveat context, deeply nested",
+			lhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":123}}]"),
+			rhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":124}}]"),
+		},
+		{
+			name: "mismatch caveat context, deeply nested with array",
+			lhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":[1,2,3]}}]"),
+			rhs:  MustParse("document:foo#viewer@user:tom[somecaveat:{\"hi\":{\"yo\":[1,2,4]}}]"),
+		},
 	}
 
 	for _, tc := range notEqualTestCases {
 		t.Run(tc.name, func(t *testing.T) {
 			require := require.New(t)
 			require.False(Equal(tc.lhs, tc.rhs))
+			require.False(Equal(tc.rhs, tc.lhs))
+			require.False(Equal(tc.lhs, MustParse(MustString(tc.rhs))))
+			require.False(Equal(tc.rhs, MustParse(MustString(tc.lhs))))
 		})
 	}
 }