Merge pull request #2038 from josephschorr/diffexpr-add-single-child

Have diffexpr handle the case of adding to a single child expression

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
 	}

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 {