Skip to content

Commit

Permalink
Have diffexpr handle the case of adding to a single child expression
Browse files Browse the repository at this point in the history
Note that this still doesn't apply for exclusions, and they have a different expression tree format
  • Loading branch information
josephschorr committed Aug 23, 2024
1 parent d77601b commit 29bade2
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 6 deletions.
25 changes: 19 additions & 6 deletions pkg/diff/namespace/diffexpr.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)))
Expand Down Expand Up @@ -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
}
Expand Down
31 changes: 31 additions & 0 deletions pkg/diff/namespace/diffexpr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,44 @@ 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 {
t.Run(tc.name, func(t *testing.T) {
parsedExisting, err := parseUsersetRewrite(tc.existing)
require.NoError(t, err)

fmt.Println(parsedExisting)

parsedUpdated, err := parseUsersetRewrite(tc.updated)
require.NoError(t, err)

Expand Down

0 comments on commit 29bade2

Please sign in to comment.