From 29bade2a7e780f10b6e4f37deb2627aeab1f9f06 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Fri, 23 Aug 2024 15:49:30 -0400
Subject: [PATCH 1/2] Have diffexpr handle the case of adding to a single child
 expression

Note that this still doesn't apply for exclusions, and they have a different expression tree format
---
 pkg/diff/namespace/diffexpr.go      | 25 +++++++++++++++++------
 pkg/diff/namespace/diffexpr_test.go | 31 +++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/pkg/diff/namespace/diffexpr.go b/pkg/diff/namespace/diffexpr.go
index fc1304e3b9..359728ce72 100644
--- a/pkg/diff/namespace/diffexpr.go
+++ b/pkg/diff/namespace/diffexpr.go
@@ -17,6 +17,11 @@ const (
 
 	// ExpressionChildrenChanged indicates that the children of the expression were changed.
 	ExpressionChildrenChanged ExpressionChangeType = "children-changed"
+
+	// ExpressionOperationExpanded indicates that the operation type of the expression was expanded
+	// from a union of a single child to multiple children under a union, intersection or another
+	// operation.
+	ExpressionOperationExpanded ExpressionChangeType = "operation-expanded"
 )
 
 // ExpressionDiff holds the diff between two expressions.
@@ -144,12 +149,20 @@ func DiffExpressions(existing *core.UsersetRewrite, updated *core.UsersetRewrite
 		return nil, spiceerrors.MustBugf("unknown operation type %T", updated.RewriteOperation)
 	}
 
+	childChangeKind := ExpressionChildrenChanged
 	if existingType != updatedType {
-		return &ExpressionDiff{
-			existing: existing,
-			updated:  updated,
-			change:   ExpressionOperationChanged,
-		}, nil
+		// If the expression has changed from a union with a single child, then
+		// treat this as a special case, since there wasn't really an operation
+		// before.
+		if existingType != "union" || len(existingOperation.Child) != 1 {
+			return &ExpressionDiff{
+				existing: existing,
+				updated:  updated,
+				change:   ExpressionOperationChanged,
+			}, nil
+		}
+
+		childChangeKind = ExpressionOperationExpanded
 	}
 
 	childDiffs := make([]*OperationDiff, 0, abs(len(updatedOperation.Child)-len(existingOperation.Child)))
@@ -186,7 +199,7 @@ func DiffExpressions(existing *core.UsersetRewrite, updated *core.UsersetRewrite
 		return &ExpressionDiff{
 			existing:   existing,
 			updated:    updated,
-			change:     ExpressionChildrenChanged,
+			change:     childChangeKind,
 			childDiffs: childDiffs,
 		}, nil
 	}
diff --git a/pkg/diff/namespace/diffexpr_test.go b/pkg/diff/namespace/diffexpr_test.go
index 59f9a1c09d..3e830ee3af 100644
--- a/pkg/diff/namespace/diffexpr_test.go
+++ b/pkg/diff/namespace/diffexpr_test.go
@@ -97,6 +97,35 @@ func TestDiffExpressions(t *testing.T) {
 			expected: `expression-unchanged
 `,
 		},
+		{
+			name:     "item added to intersection",
+			existing: `viewer & editor`,
+			updated:  `viewer & editor & admin`,
+			expected: `children-changed
+	operation-added`,
+		},
+		{
+			name:     "intersection added",
+			existing: `viewer`,
+			updated:  `viewer & editor`,
+			expected: `operation-expanded
+	operation-added`,
+		},
+		{
+			name:     "exclusion added",
+			existing: `viewer`,
+			updated:  `viewer - editor`,
+			expected: `operation-expanded
+	operation-added`,
+		},
+		{
+			name:     "item added to exclusion",
+			existing: `viewer - editor`,
+			updated:  `viewer - editor - banned`,
+			expected: `children-changed
+	operation-type-changed
+	operation-computed-userset-changed`,
+		},
 	}
 
 	for _, tc := range tcs {
@@ -104,6 +133,8 @@ func TestDiffExpressions(t *testing.T) {
 			parsedExisting, err := parseUsersetRewrite(tc.existing)
 			require.NoError(t, err)
 
+			fmt.Println(parsedExisting)
+
 			parsedUpdated, err := parseUsersetRewrite(tc.updated)
 			require.NoError(t, err)
 

From 38fbc2f7eba71a5c1bf4194932c3b6d275e51e2f Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Mon, 26 Aug 2024 10:37:09 -0400
Subject: [PATCH 2/2] Update pkg/diff/namespace/diffexpr_test.go
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Víctor Roldán Betancort <vroldanbet@users.noreply.github.com>
---
 pkg/diff/namespace/diffexpr_test.go | 2 --
 1 file changed, 2 deletions(-)

diff --git a/pkg/diff/namespace/diffexpr_test.go b/pkg/diff/namespace/diffexpr_test.go
index 3e830ee3af..1622d599a7 100644
--- a/pkg/diff/namespace/diffexpr_test.go
+++ b/pkg/diff/namespace/diffexpr_test.go
@@ -133,8 +133,6 @@ func TestDiffExpressions(t *testing.T) {
 			parsedExisting, err := parseUsersetRewrite(tc.existing)
 			require.NoError(t, err)
 
-			fmt.Println(parsedExisting)
-
 			parsedUpdated, err := parseUsersetRewrite(tc.updated)
 			require.NoError(t, err)