From 6914dba2d8fc9fd6a70ddc577cd9d59954d895f8 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Fri, 19 Dec 2025 17:02:17 -0500 Subject: [PATCH 01/10] feat: add warning for mixed operators without parentheses --- pkg/development/devcontext.go | 4 + pkg/development/warningdefs.go | 117 ++++++++++++ pkg/development/warnings.go | 121 ++++++++++++- pkg/development/warnings_misc_test.go | 249 ++++++++++++++++++++++++++ pkg/development/warnings_test.go | 154 ++++++++++++++++ 5 files changed, 643 insertions(+), 2 deletions(-) diff --git a/pkg/development/devcontext.go b/pkg/development/devcontext.go index 2ec0f5131..0b06a9580 100644 --- a/pkg/development/devcontext.go +++ b/pkg/development/devcontext.go @@ -48,6 +48,9 @@ type DevContext struct { Revision datastore.Revision CompiledSchema *compiler.CompiledSchema Dispatcher dispatch.Dispatcher + + // SchemaString is the original schema source text, used for source-scanning warnings. + SchemaString string } // NewDevContext creates a new DevContext from the specified request context, parsing and populating @@ -154,6 +157,7 @@ func newDevContextWithDatastore(ctx context.Context, requestContext *devinterfac CompiledSchema: compiled, Revision: currentRevision, Dispatcher: graph.MustNewLocalOnlyDispatcher(params), + SchemaString: requestContext.Schema, }, nil, nil } diff --git a/pkg/development/warningdefs.go b/pkg/development/warningdefs.go index b995933db..97eb7be3a 100644 --- a/pkg/development/warningdefs.go +++ b/pkg/development/warningdefs.go @@ -230,3 +230,120 @@ var lintArrowReferencingRelation = ttuCheck{ return nil, nil }, } + +// CheckExpressionForMixedOperators checks a permission expression for mixed operators +// at the same parenthetical scope. This is a source-scanning check that detects +// cases where users mix +, -, and & operators without explicit parentheses. +// +// For example: +// - "foo + bar" - OK (single operator type) +// - "foo - bar & baz" - WARN (mixed - and & at same scope) +// - "(foo - bar) & baz" - OK (operators in different scopes) +// - "(foo + bar) - (baz & qux)" - OK (operators in different scopes) +// - "(foo + bar - baz)" - WARN (mixed inside parentheses) +func CheckExpressionForMixedOperators( + permissionName string, + expressionText string, + sourcePosition *corev1.SourcePosition, +) *devinterface.DeveloperWarning { + // Use a stack-based approach to track operators in each parenthetical scope. + // Each scope is a map of operators seen at that level. + type scope struct { + operators map[rune]bool + } + + // Helper to check if a scope has mixed operators and return warning if so + checkScope := func(s scope) *devinterface.DeveloperWarning { + if len(s.operators) > 1 { + // Build a human-readable list of the mixed operators + var opList []string + if s.operators['+'] { + opList = append(opList, "union (+)") + } + if s.operators['-'] { + opList = append(opList, "exclusion (-)") + } + if s.operators['&'] { + opList = append(opList, "intersection (&)") + } + + return warningForPosition( + "mixed-operators-without-parentheses", + fmt.Sprintf( + "Permission %q mixes %s at the same level of nesting; consider adding parentheses to clarify precedence", + permissionName, + strings.Join(opList, " and "), + ), + permissionName, + sourcePosition, + ) + } + return nil + } + + // Stack of scopes - start with the root scope + scopeStack := []scope{{operators: make(map[rune]bool)}} + + runes := []rune(expressionText) + for i := 0; i < len(runes); i++ { + r := runes[i] + + // Skip whitespace + if isWhitespace(r) { + continue + } + + // Enter a new scope on open parenthesis + if r == '(' { + scopeStack = append(scopeStack, scope{operators: make(map[rune]bool)}) + continue + } + + // Exit scope on close parenthesis - check for mixed operators before popping + if r == ')' { + if len(scopeStack) > 1 { + // Check the scope we're about to pop for mixed operators + if warning := checkScope(scopeStack[len(scopeStack)-1]); warning != nil { + return warning + } + scopeStack = scopeStack[:len(scopeStack)-1] + } + continue + } + + // Skip arrow operator (->) + if r == '-' && i+1 < len(runes) && runes[i+1] == '>' { + i++ // Skip the '>' + continue + } + + // Skip dot notation for function calls (e.g., parent.all) + if r == '.' { + continue + } + + // Check for operators: +, -, & + // Only count operators that are not part of an identifier + if r == '+' || r == '-' || r == '&' { + // Look back to see if the previous non-whitespace char was an identifier char + // If so, this might be part of a different construct, but in SpiceDB schema + // these operators should always be surrounded by whitespace or parens + currentScope := &scopeStack[len(scopeStack)-1] + currentScope.operators[r] = true + continue + } + } + + // Check remaining scopes (including root) for mixed operators + for _, s := range scopeStack { + if warning := checkScope(s); warning != nil { + return warning + } + } + + return nil +} + +func isWhitespace(r rune) bool { + return r == ' ' || r == '\t' || r == '\n' || r == '\r' +} diff --git a/pkg/development/warnings.go b/pkg/development/warnings.go index 5a6b5a789..a45418d69 100644 --- a/pkg/development/warnings.go +++ b/pkg/development/warnings.go @@ -3,6 +3,7 @@ package development import ( "context" "fmt" + "strings" "github.com/ccoveille/go-safecast/v2" @@ -64,8 +65,11 @@ func GetWarnings(ctx context.Context, devCtx *DevContext) ([]*devinterface.Devel res := schema.ResolverForCompiledSchema(*devCtx.CompiledSchema) ts := schema.NewTypeSystem(res) + // Pre-split schema string once for performance when checking multiple permissions + schemaLines := strings.Split(devCtx.SchemaString, "\n") + for _, def := range devCtx.CompiledSchema.ObjectDefinitions { - found, err := addDefinitionWarnings(ctx, def, ts) + found, err := addDefinitionWarnings(ctx, def, ts, schemaLines) if err != nil { return nil, err } @@ -79,7 +83,7 @@ type contextKey string var relationKey = contextKey("relation") -func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinition, ts *schema.TypeSystem) ([]*devinterface.DeveloperWarning, error) { +func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinition, ts *schema.TypeSystem, schemaLines []string) ([]*devinterface.DeveloperWarning, error) { def, err := schema.NewDefinition(ts, nsDef) if err != nil { return nil, err @@ -111,12 +115,125 @@ func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinitio } warnings = append(warnings, found...) + + // Check for mixed operators without parentheses using source scanning + if !shouldSkipCheck(rel.Metadata, "mixed-operators-without-parentheses") { + expressionText := extractPermissionExpression(schemaLines, rel.Name, rel.GetSourcePosition()) + if expressionText != "" { + if warning := CheckExpressionForMixedOperators(rel.Name, expressionText, rel.GetSourcePosition()); warning != nil { + warnings = append(warnings, warning) + } + } + } } } return warnings, nil } +// extractPermissionExpression extracts the expression text for a permission from the schema source. +// It uses the source position to locate the permission and extracts the text after the '=' sign. +// The schemaLines parameter should be pre-split for performance when checking multiple permissions. +func extractPermissionExpression(schemaLines []string, _ string, sourcePosition *corev1.SourcePosition) string { + if sourcePosition == nil || len(schemaLines) == 0 { + return "" + } + + lineIdx, err := safecast.Convert[int](sourcePosition.ZeroIndexedLineNumber) + if err != nil || lineIdx < 0 || lineIdx >= len(schemaLines) { + return "" + } + + // Start from the permission's line and look for the expression + // The expression is everything after '=' until end of statement + var expressionBuilder strings.Builder + foundEquals := false + parenDepth := 0 + inBlockComment := false + lastNonWhitespaceWasOperator := false + + for i := lineIdx; i < len(schemaLines); i++ { + line := schemaLines[i] + + // If this is the first line, start from the column position + startCol := 0 + if i == lineIdx { + colPos, err := safecast.Convert[int](sourcePosition.ZeroIndexedColumnPosition) + if err == nil && colPos < len(line) { + startCol = colPos + } + } + + lineHasContent := false + for j := startCol; j < len(line); j++ { + ch := line[j] + + // Handle block comment state + if inBlockComment { + if ch == '*' && j+1 < len(line) && line[j+1] == '/' { + inBlockComment = false + j++ // Skip the '/' + } + continue + } + + // Check for start of block comment + if ch == '/' && j+1 < len(line) && line[j+1] == '*' { + inBlockComment = true + j++ // Skip the '*' + continue + } + + // Check for line comment - skip rest of line + if ch == '/' && j+1 < len(line) && line[j+1] == '/' { + break + } + + if !foundEquals { + if ch == '=' { + foundEquals = true + } + continue + } + + // Track parenthesis depth for multi-line expressions + switch ch { + case '(': + parenDepth++ + case ')': + parenDepth-- + } + + // Track if this is an operator (for multi-line continuation detection) + isWhitespaceChar := ch == ' ' || ch == '\t' + if !isWhitespaceChar { + lineHasContent = true + lastNonWhitespaceWasOperator = (ch == '+' || ch == '-' || ch == '&') + } + + expressionBuilder.WriteByte(ch) + } + + // Determine if expression continues on next line: + // 1. We're inside parentheses (parenDepth > 0) + // 2. We're inside a block comment + // 3. The line ended with an operator (suggesting continuation) + // 4. The line had no content yet after '=' (e.g., '= \n foo + bar') + if foundEquals { + continueToNextLine := parenDepth > 0 || inBlockComment || lastNonWhitespaceWasOperator || !lineHasContent + if !continueToNextLine { + break + } + // Add space for multi-line expressions + if i < len(schemaLines)-1 { + expressionBuilder.WriteByte(' ') + } + } + } + + return strings.TrimSpace(expressionBuilder.String()) +} + func shouldSkipCheck(metadata *corev1.Metadata, name string) bool { if metadata == nil { return false diff --git a/pkg/development/warnings_misc_test.go b/pkg/development/warnings_misc_test.go index a5191d5d7..c835d82c4 100644 --- a/pkg/development/warnings_misc_test.go +++ b/pkg/development/warnings_misc_test.go @@ -1,6 +1,7 @@ package development import ( + "strings" "testing" "github.com/stretchr/testify/require" @@ -209,3 +210,251 @@ func TestWrappedFunctionedTTUUnknownFunction(t *testing.T) { _, _ = wrapped.GetArrowString() }) } + +func TestCheckExpressionForMixedOperators(t *testing.T) { + testCases := []struct { + name string + expression string + expectWarning bool + expectedInMsg string + }{ + { + name: "single union operator", + expression: "foo + bar", + expectWarning: false, + }, + { + name: "single intersection operator", + expression: "foo & bar", + expectWarning: false, + }, + { + name: "single exclusion operator", + expression: "foo - bar", + expectWarning: false, + }, + { + name: "multiple same operators - union", + expression: "foo + bar + baz", + expectWarning: false, + }, + { + name: "multiple same operators - intersection", + expression: "foo & bar & baz", + expectWarning: false, + }, + { + name: "multiple same operators - exclusion", + expression: "foo - bar - baz", + expectWarning: false, + }, + { + name: "mixed union and exclusion at same depth", + expression: "foo + bar - baz", + expectWarning: true, + expectedInMsg: "union (+) and exclusion (-)", + }, + { + name: "mixed exclusion and intersection at same depth", + expression: "foo - bar & baz", + expectWarning: true, + expectedInMsg: "exclusion (-) and intersection (&)", + }, + { + name: "mixed union and intersection at same depth", + expression: "foo + bar & baz", + expectWarning: true, + expectedInMsg: "union (+) and intersection (&)", + }, + { + name: "all three operators at same depth", + expression: "foo + bar - baz & qux", + expectWarning: true, + expectedInMsg: "union (+)", + }, + { + name: "mixed operators with parentheses - different depths", + expression: "(foo + bar) - baz", + expectWarning: false, + }, + { + name: "mixed operators with parentheses - right side", + expression: "foo - (bar & baz)", + expectWarning: false, + }, + { + name: "complex parenthesized expression - no warning", + expression: "(foo + bar) - (baz & qux)", + expectWarning: false, + }, + { + name: "arrow expression should not trigger warning", + expression: "parent->view + editor", + expectWarning: false, + }, + { + name: "arrow expression with mixed operators should trigger", + expression: "parent->view + editor - blocked", + expectWarning: true, + expectedInMsg: "union (+) and exclusion (-)", + }, + { + name: "function call expression", + expression: "parent.all(view) + editor", + expectWarning: false, + }, + { + name: "empty expression", + expression: "", + expectWarning: false, + }, + { + name: "only identifier", + expression: "viewer", + expectWarning: false, + }, + { + name: "nested parentheses all same operator", + expression: "(foo + bar) + (baz + qux)", + expectWarning: false, + }, + { + name: "deeply nested mixed operators at different depths", + expression: "((foo + bar) - baz) & qux", + expectWarning: false, + }, + { + name: "mixed operators inside parentheses should warn", + expression: "(foo + bar - baz)", + expectWarning: true, + expectedInMsg: "union (+) and exclusion (-)", + }, + { + name: "mixed operators in nested parens should warn", + expression: "qux & (foo + bar - baz)", + expectWarning: true, + expectedInMsg: "union (+) and exclusion (-)", + }, + } + + sourcePos := &core.SourcePosition{ + ZeroIndexedLineNumber: 0, + ZeroIndexedColumnPosition: 0, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + warning := CheckExpressionForMixedOperators("test_perm", tc.expression, sourcePos) + if tc.expectWarning { + require.NotNil(t, warning, "expected warning but got nil") + require.Contains(t, warning.Message, tc.expectedInMsg, "warning message should contain expected text") + require.Contains(t, warning.Message, "mixed-operators-without-parentheses") + } else { + require.Nil(t, warning, "expected no warning but got: %v", warning) + } + }) + } +} + +func TestCheckExpressionForMixedOperatorsNilSourcePosition(t *testing.T) { + warning := CheckExpressionForMixedOperators("test_perm", "foo + bar - baz", nil) + require.NotNil(t, warning, "should still produce warning with nil source position") + require.Equal(t, uint32(0), warning.Line) + require.Equal(t, uint32(0), warning.Column) +} + +func TestExtractPermissionExpression(t *testing.T) { + testCases := []struct { + name string + schema string + permissionName string + lineNumber uint64 + expectedExpression string + }{ + { + name: "simple permission", + schema: `definition test { + permission view = foo + bar + }`, + permissionName: "view", + lineNumber: 1, + expectedExpression: "foo + bar", + }, + { + name: "permission with comment after", + schema: `definition test { + permission view = foo + bar // some comment + }`, + permissionName: "view", + lineNumber: 1, + expectedExpression: "foo + bar", + }, + { + name: "empty schema", + schema: "", + permissionName: "view", + lineNumber: 0, + expectedExpression: "", + }, + { + name: "multi-line expression with operator at end of line", + schema: "definition test {\n\tpermission view = foo +\n\t\tbar - baz\n}", + permissionName: "view", + lineNumber: 1, + expectedExpression: "foo + \t\tbar - baz", + }, + { + name: "multi-line expression with parentheses", + schema: "definition test {\n\tpermission view = (foo +\n\t\tbar) - baz\n}", + permissionName: "view", + lineNumber: 1, + expectedExpression: "(foo + \t\tbar) - baz", + }, + { + name: "expression with block comment", + schema: `definition test { + permission view = foo /* this is a comment */ + bar + }`, + permissionName: "view", + lineNumber: 1, + expectedExpression: "foo + bar", + }, + { + name: "expression with block comment spanning content", + schema: `definition test { + permission view = foo + /* comment */ bar - baz + }`, + permissionName: "view", + lineNumber: 1, + expectedExpression: "foo + bar - baz", + }, + { + name: "multi-line expression without operator at end - single line", + schema: `definition test { + permission view = foo + bar + permission edit = baz + }`, + permissionName: "view", + lineNumber: 1, + expectedExpression: "foo + bar", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + sourcePos := &core.SourcePosition{ + ZeroIndexedLineNumber: tc.lineNumber, + ZeroIndexedColumnPosition: 0, + } + schemaLines := strings.Split(tc.schema, "\n") + result := extractPermissionExpression(schemaLines, tc.permissionName, sourcePos) + require.Equal(t, tc.expectedExpression, result) + }) + } +} + +func TestExtractPermissionExpressionNilSourcePosition(t *testing.T) { + schemaLines := strings.Split("definition test { permission view = foo }", "\n") + result := extractPermissionExpression(schemaLines, "view", nil) + require.Empty(t, result) +} diff --git a/pkg/development/warnings_test.go b/pkg/development/warnings_test.go index 5cb7ab7ab..b381eafec 100644 --- a/pkg/development/warnings_test.go +++ b/pkg/development/warnings_test.go @@ -249,6 +249,160 @@ func TestWarnings(t *testing.T) { `, expectedWarning: nil, }, + // Mixed operators tests + { + name: "mixed operators without parentheses - exclusion and intersection", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = foo - bar & baz + } + `, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Permission \"view\" mixes exclusion (-) and intersection (&) at the same level of nesting; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Line: 7, + Column: 5, + SourceCode: "view", + }, + }, + { + name: "mixed operators without parentheses - union and exclusion", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = foo + bar - baz + } + `, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Permission \"view\" mixes union (+) and exclusion (-) at the same level of nesting; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Line: 7, + Column: 5, + SourceCode: "view", + }, + }, + { + name: "mixed operators with parentheses - no warning", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = (foo - bar) & baz + } + `, + expectedWarning: nil, + }, + { + name: "mixed operators with parentheses on right - no warning", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = foo - (bar & baz) + } + `, + expectedWarning: nil, + }, + { + name: "single operator type - no warning", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = foo + bar + baz + } + `, + expectedWarning: nil, + }, + { + name: "arrow with mixed operators - no false positive on arrow", + schema: `definition user {} + + definition group { + relation member: user + permission view = member + } + + definition document { + relation parent: group + relation viewer: user + permission view = parent->view + viewer + } + `, + expectedWarning: nil, + }, + { + name: "mixed operators warning disabled", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + + // spicedb-ignore-warning: mixed-operators-without-parentheses + permission view = foo - bar & baz + } + `, + expectedWarning: nil, + }, + // Multi-line expression tests + { + name: "multi-line mixed operators with operator at end of line", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = foo + + bar - baz + } + `, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Permission \"view\" mixes union (+) and exclusion (-) at the same level of nesting; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Line: 7, + Column: 5, + SourceCode: "view", + }, + }, + { + name: "multi-line expression with parentheses - no warning", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + relation baz: user + permission view = (foo + + bar) - baz + } + `, + expectedWarning: nil, + }, + { + name: "expression with block comment - no warning for same operator", + schema: `definition user {} + + definition document { + relation foo: user + relation bar: user + permission view = foo /* comment */ + bar + } + `, + expectedWarning: nil, + }, } for _, tc := range tcs { From 7028342808226c527c2c6500726a3a3553dce338 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Wed, 7 Jan 2026 15:51:41 -0500 Subject: [PATCH 02/10] feat: add warning for mixed operators without parentheses --- .gitignore | 5 +- go.sum | 1 + .../compiler/compiler_test.go | 17 +- .../compiler/translator.go | 107 ++++++++ pkg/composableschemadsl/dslshape/dslshape.go | 14 +- .../dslshape/zz_generated.nodetype_string.go | 13 +- pkg/composableschemadsl/parser/parser.go | 15 +- .../parser/tests/basic.zed.expected | 31 ++- .../parser/tests/multiparen.zed.expected | 94 ++++--- .../parser/tests/nil.zed.expected | 39 +-- .../parser/tests/parens.zed.expected | 31 ++- pkg/development/warningdefs.go | 130 ++------- pkg/development/warnings.go | 114 +------- pkg/development/warnings_misc_test.go | 249 ------------------ pkg/development/warnings_test.go | 145 ++-------- pkg/namespace/builder.go | 13 + pkg/namespace/metadata.go | 91 +++++++ pkg/proto/impl/v1/impl.pb.go | 46 +++- pkg/proto/impl/v1/impl.pb.validate.go | 31 +++ pkg/proto/impl/v1/impl_vtproto.pb.go | 128 +++++++++ pkg/schemadsl/compiler/compiler_test.go | 17 +- pkg/schemadsl/compiler/translator.go | 107 ++++++++ pkg/schemadsl/dslshape/dslshape.go | 14 +- .../dslshape/zz_generated.nodetype_string.go | 7 +- pkg/schemadsl/parser/parser.go | 15 +- pkg/schemadsl/parser/tests/basic.zed.expected | 31 ++- .../parser/tests/multiparen.zed.expected | 94 ++++--- pkg/schemadsl/parser/tests/nil.zed.expected | 39 +-- .../parser/tests/parens.zed.expected | 31 ++- .../tests/permission_edge_cases.zed.expected | 31 ++- proto/internal/impl/v1/impl.proto | 8 + 31 files changed, 886 insertions(+), 822 deletions(-) diff --git a/.gitignore b/.gitignore index 148a4040d..af0d44fe0 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,7 @@ coverage*.txt .claude/ e2e/newenemy/spicedb e2e/newenemy/cockroach -e2e/newenemy/chaosd \ No newline at end of file +e2e/newenemy/chaosd + +# Auto Claude data directory +.auto-claude/ diff --git a/go.sum b/go.sum index a7bb9679d..20ec5bd75 100644 --- a/go.sum +++ b/go.sum @@ -2503,6 +2503,7 @@ google.golang.org/grpc v1.54.0/go.mod h1:PUSEXI6iWghWaB6lXM4knEgpJNu2qUcKfDtNci3 google.golang.org/grpc v1.56.3/go.mod h1:I9bI3vqKfayGqPUAwGdOSu7kt6oIJLixfffKrpXqQ9s= google.golang.org/grpc v1.77.0 h1:wVVY6/8cGA6vvffn+wWK5ToddbgdU3d8MNENr4evgXM= google.golang.org/grpc v1.77.0/go.mod h1:z0BY1iVj0q8E1uSQCjL9cppRj+gnZjzDnzV0dHhrNig= +google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0 h1:M1YKkFIboKNieVO5DLUEVzQfGwJD30Nv2jfUgzb5UcE= google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0= diff --git a/pkg/composableschemadsl/compiler/compiler_test.go b/pkg/composableschemadsl/compiler/compiler_test.go index 64429892c..a10834949 100644 --- a/pkg/composableschemadsl/compiler/compiler_test.go +++ b/pkg/composableschemadsl/compiler/compiler_test.go @@ -573,16 +573,19 @@ func TestCompile(t *testing.T) { "", []SchemaDefinition{ namespace.Namespace("sometenant/complex", - namespace.MustRelation("foos", - namespace.Exclusion( - namespace.Rewrite( - namespace.Union( - namespace.ComputedUserset("bars"), - namespace.ComputedUserset("bazs"), + namespace.WithMixedOperators( + namespace.MustRelation("foos", + namespace.Exclusion( + namespace.Rewrite( + namespace.Union( + namespace.ComputedUserset("bars"), + namespace.ComputedUserset("bazs"), + ), ), + namespace.ComputedUserset("mehs"), ), - namespace.ComputedUserset("mehs"), ), + 1, 22, // line 1, column 22 (0-indexed) ), ), }, diff --git a/pkg/composableschemadsl/compiler/translator.go b/pkg/composableschemadsl/compiler/translator.go index bb00ebb11..3b6047789 100644 --- a/pkg/composableschemadsl/compiler/translator.go +++ b/pkg/composableschemadsl/compiler/translator.go @@ -437,6 +437,9 @@ func translatePermission(tctx *translationContext, permissionNode *dslNode) (*co return nil, permissionNode.Errorf("invalid permission expression: %w", err) } + // Detect mixed operators without parentheses before translation (which may flatten the AST). + mixedOpsPosition := detectMixedOperatorsWithoutParens(tctx, expressionNode) + rewrite, err := translateExpression(tctx, expressionNode) if err != nil { return nil, err @@ -447,6 +450,14 @@ func translatePermission(tctx *translationContext, permissionNode *dslNode) (*co return nil, err } + // Store mixed operators flag in metadata + if mixedOpsPosition != nil { + err = namespace.SetMixedOperatorsWithoutParens(permission, true, mixedOpsPosition) + if err != nil { + return nil, permissionNode.Errorf("error adding mixed operators flag to metadata: %w", err) + } + } + if !tctx.skipValidate { if err := permission.Validate(); err != nil { return nil, permissionNode.Errorf("error in permission %s: %w", permissionName, err) @@ -579,6 +590,14 @@ func translateExpressionOperationDirect(tctx *translationContext, expressionOpNo case dslshape.NodeTypeSelfExpression: return namespace.Self(), nil + case dslshape.NodeTypeParenthesizedExpression: + // Unwrap the parenthesized expression and translate its inner expression. + innerExprNode, err := expressionOpNode.Lookup(dslshape.NodeParenthesizedExpressionPredicateInnerExpr) + if err != nil { + return nil, err + } + return translateExpressionOperation(tctx, innerExprNode) + case dslshape.NodeTypeArrowExpression: leftChild, err := expressionOpNode.Lookup(dslshape.NodeExpressionPredicateLeftExpr) if err != nil { @@ -948,3 +967,91 @@ func translateUseFlag(tctx *translationContext, useFlagNode *dslNode) error { tctx.enabledFlags.Add(flagName) return nil } + +// operatorType represents the type of set operator in an expression. +type operatorType int + +const ( + operatorTypeUnknown operatorType = iota + operatorTypeUnion + operatorTypeIntersection + operatorTypeExclusion +) + +// getOperatorType returns the operator type for a given node type, or operatorTypeUnknown if not a set operator. +func getOperatorType(nodeType dslshape.NodeType) operatorType { + switch nodeType { + case dslshape.NodeTypeUnionExpression: + return operatorTypeUnion + case dslshape.NodeTypeIntersectExpression: + return operatorTypeIntersection + case dslshape.NodeTypeExclusionExpression: + return operatorTypeExclusion + default: + return operatorTypeUnknown + } +} + +// detectMixedOperatorsWithoutParens walks the expression AST and detects if there are mixed +// operators (union, intersection, exclusion) at the same scope level without explicit parentheses. +// Returns the source position of the first mixed operator found, or nil if none. +// Parenthesized expressions act as boundaries - mixing inside parens does not trigger a warning +// since the user explicitly grouped the expression. However, top-level parentheses are unwrapped +// since they don't clarify internal operator precedence. +func detectMixedOperatorsWithoutParens(tctx *translationContext, node *dslNode) *core.SourcePosition { + // Unwrap top-level parenthesized expressions - they don't clarify internal precedence. + // e.g., (a + b - c) should still warn about mixed operators. + for node.GetType() == dslshape.NodeTypeParenthesizedExpression { + innerNode, err := node.Lookup(dslshape.NodeParenthesizedExpressionPredicateInnerExpr) + if err != nil { + break + } + node = innerNode + } + return detectMixedOperatorsInScope(tctx, node, operatorTypeUnknown) +} + +// detectMixedOperatorsInScope recursively checks for mixed operators within a scope. +// parentOp is the operator type seen so far at this scope level. +func detectMixedOperatorsInScope(tctx *translationContext, node *dslNode, parentOp operatorType) *core.SourcePosition { + nodeType := node.GetType() + + // Parenthesized expressions act as a boundary - don't propagate operator checking into them. + // The user explicitly grouped the expression, so we don't warn about mixing inside. + if nodeType == dslshape.NodeTypeParenthesizedExpression { + return nil + } + + currentOp := getOperatorType(nodeType) + + // If this is a set operator and we've seen a different operator at this scope, it's mixed. + if currentOp != operatorTypeUnknown && parentOp != operatorTypeUnknown && currentOp != parentOp { + return getSourcePosition(node, tctx.mapper) + } + + // If this is a set operator, check children with this operator as the scope's operator. + if currentOp != operatorTypeUnknown { + effectiveOp := currentOp + if parentOp != operatorTypeUnknown { + effectiveOp = parentOp // Keep the first operator seen at this scope + } + + // Check left child + leftChild, err := node.Lookup(dslshape.NodeExpressionPredicateLeftExpr) + if err == nil { + if pos := detectMixedOperatorsInScope(tctx, leftChild, effectiveOp); pos != nil { + return pos + } + } + + // Check right child + rightChild, err := node.Lookup(dslshape.NodeExpressionPredicateRightExpr) + if err == nil { + if pos := detectMixedOperatorsInScope(tctx, rightChild, effectiveOp); pos != nil { + return pos + } + } + } + + return nil +} diff --git a/pkg/composableschemadsl/dslshape/dslshape.go b/pkg/composableschemadsl/dslshape/dslshape.go index 3d1d63add..b6a007cc0 100644 --- a/pkg/composableschemadsl/dslshape/dslshape.go +++ b/pkg/composableschemadsl/dslshape/dslshape.go @@ -32,9 +32,10 @@ const ( NodeTypeArrowExpression // A TTU in arrow form. - NodeTypeIdentifier // An identifier under an expression. - NodeTypeNilExpression // A nil keyword - NodeTypeSelfExpression // A self keyword + NodeTypeIdentifier // An identifier under an expression. + NodeTypeNilExpression // A nil keyword + NodeTypeSelfExpression // A self keyword + NodeTypeParenthesizedExpression // A parenthesized expression wrapper. NodeTypeCaveatTypeReference // A type reference for a caveat parameter. @@ -212,6 +213,13 @@ const ( NodeExpressionPredicateLeftExpr = "left-expr" NodeExpressionPredicateRightExpr = "right-expr" + // + // NodeTypeParenthesizedExpression + // + + // The inner expression wrapped by parentheses. + NodeParenthesizedExpressionPredicateInnerExpr = "inner-expr" + // // NodeTypeImport // diff --git a/pkg/composableschemadsl/dslshape/zz_generated.nodetype_string.go b/pkg/composableschemadsl/dslshape/zz_generated.nodetype_string.go index 318cc0873..d477c9dc7 100644 --- a/pkg/composableschemadsl/dslshape/zz_generated.nodetype_string.go +++ b/pkg/composableschemadsl/dslshape/zz_generated.nodetype_string.go @@ -29,15 +29,16 @@ func _() { _ = x[NodeTypeIdentifier-18] _ = x[NodeTypeNilExpression-19] _ = x[NodeTypeSelfExpression-20] - _ = x[NodeTypeCaveatTypeReference-21] - _ = x[NodeTypeImport-22] - _ = x[NodeTypePartial-23] - _ = x[NodeTypePartialReference-24] + _ = x[NodeTypeParenthesizedExpression-21] + _ = x[NodeTypeCaveatTypeReference-22] + _ = x[NodeTypeImport-23] + _ = x[NodeTypePartial-24] + _ = x[NodeTypePartialReference-25] } -const _NodeType_name = "NodeTypeErrorNodeTypeFileNodeTypeCommentNodeTypeUseFlagNodeTypeDefinitionNodeTypeCaveatDefinitionNodeTypeCaveatParameterNodeTypeCaveatExpressionNodeTypeRelationNodeTypePermissionNodeTypeTypeReferenceNodeTypeSpecificTypeReferenceNodeTypeCaveatReferenceNodeTypeTraitReferenceNodeTypeUnionExpressionNodeTypeIntersectExpressionNodeTypeExclusionExpressionNodeTypeArrowExpressionNodeTypeIdentifierNodeTypeNilExpressionNodeTypeSelfExpressionNodeTypeCaveatTypeReferenceNodeTypeImportNodeTypePartialNodeTypePartialReference" +const _NodeType_name = "NodeTypeErrorNodeTypeFileNodeTypeCommentNodeTypeUseFlagNodeTypeDefinitionNodeTypeCaveatDefinitionNodeTypeCaveatParameterNodeTypeCaveatExpressionNodeTypeRelationNodeTypePermissionNodeTypeTypeReferenceNodeTypeSpecificTypeReferenceNodeTypeCaveatReferenceNodeTypeTraitReferenceNodeTypeUnionExpressionNodeTypeIntersectExpressionNodeTypeExclusionExpressionNodeTypeArrowExpressionNodeTypeIdentifierNodeTypeNilExpressionNodeTypeSelfExpressionNodeTypeParenthesizedExpressionNodeTypeCaveatTypeReferenceNodeTypeImportNodeTypePartialNodeTypePartialReference" -var _NodeType_index = [...]uint16{0, 13, 25, 40, 55, 73, 97, 120, 144, 160, 178, 199, 228, 251, 273, 296, 323, 350, 373, 391, 412, 434, 461, 475, 490, 514} +var _NodeType_index = [...]uint16{0, 13, 25, 40, 55, 73, 97, 120, 144, 160, 178, 199, 228, 251, 273, 296, 323, 350, 373, 391, 412, 434, 465, 492, 506, 521, 545} func (i NodeType) String() string { idx := int(i) - 0 diff --git a/pkg/composableschemadsl/parser/parser.go b/pkg/composableschemadsl/parser/parser.go index e59731638..5ac73f633 100644 --- a/pkg/composableschemadsl/parser/parser.go +++ b/pkg/composableschemadsl/parser/parser.go @@ -652,18 +652,23 @@ func (p *sourceParser) tryConsumeArrowExpression() (AstNode, bool) { // ```nil``` func (p *sourceParser) tryConsumeBaseExpression() (AstNode, bool) { switch { - // Nested expression. + // Nested expression - wrap in NodeTypeParenthesizedExpression to preserve parentheses info. case p.isToken(lexer.TokenTypeLeftParen): comments := p.currentToken.comments + wrapperNode := p.startNode(dslshape.NodeTypeParenthesizedExpression) + p.consume(lexer.TokenTypeLeftParen) - exprNode := p.consumeComputeExpression() + innerExprNode := p.consumeComputeExpression() p.consume(lexer.TokenTypeRightParen) - // Attach any comments found to the consumed expression. - p.decorateComments(exprNode, comments) + wrapperNode.Connect(dslshape.NodeParenthesizedExpressionPredicateInnerExpr, innerExprNode) + p.mustFinishNode() - return exprNode, true + // Attach any comments found to the wrapper node. + p.decorateComments(wrapperNode, comments) + + return wrapperNode, true // Self expression. case p.isKeyword("self"): diff --git a/pkg/composableschemadsl/parser/tests/basic.zed.expected b/pkg/composableschemadsl/parser/tests/basic.zed.expected index a98c1030b..aaf6b2b87 100644 --- a/pkg/composableschemadsl/parser/tests/basic.zed.expected +++ b/pkg/composableschemadsl/parser/tests/basic.zed.expected @@ -84,22 +84,27 @@ NodeTypeFile input-source = basic definition test start-rune = 208 left-expr => - NodeTypeExclusionExpression - end-rune = 217 + NodeTypeParenthesizedExpression + end-rune = 218 input-source = basic definition test - start-rune = 209 - left-expr => - NodeTypeIdentifier - end-rune = 211 - identifier-value = foo - input-source = basic definition test - start-rune = 209 - right-expr => - NodeTypeIdentifier + start-rune = 208 + inner-expr => + NodeTypeExclusionExpression end-rune = 217 - identifier-value = meh input-source = basic definition test - start-rune = 215 + start-rune = 209 + left-expr => + NodeTypeIdentifier + end-rune = 211 + identifier-value = foo + input-source = basic definition test + start-rune = 209 + right-expr => + NodeTypeIdentifier + end-rune = 217 + identifier-value = meh + input-source = basic definition test + start-rune = 215 right-expr => NodeTypeIdentifier end-rune = 224 diff --git a/pkg/composableschemadsl/parser/tests/multiparen.zed.expected b/pkg/composableschemadsl/parser/tests/multiparen.zed.expected index 5bf967dd3..d7424687b 100644 --- a/pkg/composableschemadsl/parser/tests/multiparen.zed.expected +++ b/pkg/composableschemadsl/parser/tests/multiparen.zed.expected @@ -42,58 +42,68 @@ NodeTypeFile input-source = multiple parens test start-rune = 44 right-expr => - NodeTypeExclusionExpression - end-rune = 61 + NodeTypeParenthesizedExpression + end-rune = 62 input-source = multiple parens test - start-rune = 51 - left-expr => - NodeTypeArrowExpression - end-rune = 54 + start-rune = 50 + inner-expr => + NodeTypeExclusionExpression + end-rune = 61 input-source = multiple parens test start-rune = 51 left-expr => - NodeTypeIdentifier - end-rune = 51 - identifier-value = a - input-source = multiple parens test - start-rune = 51 - right-expr => - NodeTypeIdentifier + NodeTypeArrowExpression end-rune = 54 - identifier-value = b - input-source = multiple parens test - start-rune = 54 - right-expr => - NodeTypeArrowExpression - end-rune = 61 - input-source = multiple parens test - start-rune = 58 - left-expr => - NodeTypeIdentifier - end-rune = 58 - identifier-value = c input-source = multiple parens test - start-rune = 58 + start-rune = 51 + left-expr => + NodeTypeIdentifier + end-rune = 51 + identifier-value = a + input-source = multiple parens test + start-rune = 51 + right-expr => + NodeTypeIdentifier + end-rune = 54 + identifier-value = b + input-source = multiple parens test + start-rune = 54 right-expr => - NodeTypeIdentifier + NodeTypeArrowExpression end-rune = 61 - identifier-value = d input-source = multiple parens test - start-rune = 61 + start-rune = 58 + left-expr => + NodeTypeIdentifier + end-rune = 58 + identifier-value = c + input-source = multiple parens test + start-rune = 58 + right-expr => + NodeTypeIdentifier + end-rune = 61 + identifier-value = d + input-source = multiple parens test + start-rune = 61 right-expr => - NodeTypeIntersectExpression - end-rune = 75 + NodeTypeParenthesizedExpression + end-rune = 76 input-source = multiple parens test - start-rune = 67 - left-expr => - NodeTypeIdentifier - end-rune = 69 - identifier-value = maz - input-source = multiple parens test - start-rune = 67 - right-expr => - NodeTypeIdentifier + start-rune = 66 + inner-expr => + NodeTypeIntersectExpression end-rune = 75 - identifier-value = beh input-source = multiple parens test - start-rune = 73 \ No newline at end of file + start-rune = 67 + left-expr => + NodeTypeIdentifier + end-rune = 69 + identifier-value = maz + input-source = multiple parens test + start-rune = 67 + right-expr => + NodeTypeIdentifier + end-rune = 75 + identifier-value = beh + input-source = multiple parens test + start-rune = 73 \ No newline at end of file diff --git a/pkg/composableschemadsl/parser/tests/nil.zed.expected b/pkg/composableschemadsl/parser/tests/nil.zed.expected index 90bc5578b..2c6ec7081 100644 --- a/pkg/composableschemadsl/parser/tests/nil.zed.expected +++ b/pkg/composableschemadsl/parser/tests/nil.zed.expected @@ -67,32 +67,37 @@ NodeTypeFile input-source = nil test start-rune = 117 left-expr => - NodeTypeUnionExpression - end-rune = 128 + NodeTypeParenthesizedExpression + end-rune = 129 input-source = nil test - start-rune = 118 - left-expr => + start-rune = 117 + inner-expr => NodeTypeUnionExpression - end-rune = 122 + end-rune = 128 input-source = nil test start-rune = 118 left-expr => - NodeTypeIdentifier - end-rune = 118 - identifier-value = a + NodeTypeUnionExpression + end-rune = 122 input-source = nil test start-rune = 118 + left-expr => + NodeTypeIdentifier + end-rune = 118 + identifier-value = a + input-source = nil test + start-rune = 118 + right-expr => + NodeTypeIdentifier + end-rune = 122 + identifier-value = b + input-source = nil test + start-rune = 122 right-expr => - NodeTypeIdentifier - end-rune = 122 - identifier-value = b + NodeTypeNilExpression + end-rune = 128 input-source = nil test - start-rune = 122 - right-expr => - NodeTypeNilExpression - end-rune = 128 - input-source = nil test - start-rune = 126 + start-rune = 126 right-expr => NodeTypeIdentifier end-rune = 133 diff --git a/pkg/composableschemadsl/parser/tests/parens.zed.expected b/pkg/composableschemadsl/parser/tests/parens.zed.expected index cbd5b3b45..79eb54e54 100644 --- a/pkg/composableschemadsl/parser/tests/parens.zed.expected +++ b/pkg/composableschemadsl/parser/tests/parens.zed.expected @@ -15,19 +15,24 @@ NodeTypeFile relation-name = bar start-rune = 21 compute-expression => - NodeTypeIntersectExpression - end-rune = 47 + NodeTypeParenthesizedExpression + end-rune = 48 input-source = parens test - start-rune = 39 - left-expr => - NodeTypeIdentifier - end-rune = 41 - identifier-value = maz - input-source = parens test - start-rune = 39 - right-expr => - NodeTypeIdentifier + start-rune = 38 + inner-expr => + NodeTypeIntersectExpression end-rune = 47 - identifier-value = beh input-source = parens test - start-rune = 45 \ No newline at end of file + start-rune = 39 + left-expr => + NodeTypeIdentifier + end-rune = 41 + identifier-value = maz + input-source = parens test + start-rune = 39 + right-expr => + NodeTypeIdentifier + end-rune = 47 + identifier-value = beh + input-source = parens test + start-rune = 45 \ No newline at end of file diff --git a/pkg/development/warningdefs.go b/pkg/development/warningdefs.go index 97eb7be3a..2ca26230f 100644 --- a/pkg/development/warningdefs.go +++ b/pkg/development/warningdefs.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/authzed/spicedb/pkg/namespace" corev1 "github.com/authzed/spicedb/pkg/proto/core/v1" devinterface "github.com/authzed/spicedb/pkg/proto/developer/v1" "github.com/authzed/spicedb/pkg/schema" @@ -231,119 +232,32 @@ var lintArrowReferencingRelation = ttuCheck{ }, } -// CheckExpressionForMixedOperators checks a permission expression for mixed operators -// at the same parenthetical scope. This is a source-scanning check that detects -// cases where users mix +, -, and & operators without explicit parentheses. -// -// For example: -// - "foo + bar" - OK (single operator type) -// - "foo - bar & baz" - WARN (mixed - and & at same scope) -// - "(foo - bar) & baz" - OK (operators in different scopes) -// - "(foo + bar) - (baz & qux)" - OK (operators in different scopes) -// - "(foo + bar - baz)" - WARN (mixed inside parentheses) -func CheckExpressionForMixedOperators( - permissionName string, - expressionText string, - sourcePosition *corev1.SourcePosition, -) *devinterface.DeveloperWarning { - // Use a stack-based approach to track operators in each parenthetical scope. - // Each scope is a map of operators seen at that level. - type scope struct { - operators map[rune]bool - } - - // Helper to check if a scope has mixed operators and return warning if so - checkScope := func(s scope) *devinterface.DeveloperWarning { - if len(s.operators) > 1 { - // Build a human-readable list of the mixed operators - var opList []string - if s.operators['+'] { - opList = append(opList, "union (+)") - } - if s.operators['-'] { - opList = append(opList, "exclusion (-)") - } - if s.operators['&'] { - opList = append(opList, "intersection (&)") - } +var lintMixedOperatorsWithoutParentheses = relationCheck{ + "mixed-operators-without-parentheses", + func( + ctx context.Context, + relation *corev1.Relation, + def *schema.Definition, + ) (*devinterface.DeveloperWarning, error) { + // Only check permissions, not relations. + if !def.IsPermission(relation.Name) { + return nil, nil + } + // Check if the permission has mixed operators without parentheses. + if namespace.HasMixedOperatorsWithoutParens(relation) { + position := namespace.GetMixedOperatorsPosition(relation) return warningForPosition( "mixed-operators-without-parentheses", fmt.Sprintf( - "Permission %q mixes %s at the same level of nesting; consider adding parentheses to clarify precedence", - permissionName, - strings.Join(opList, " and "), + "Permission %q mixes operators (union, intersection, exclusion) at the same level without explicit parentheses; consider adding parentheses to clarify precedence", + relation.Name, ), - permissionName, - sourcePosition, - ) - } - return nil - } - - // Stack of scopes - start with the root scope - scopeStack := []scope{{operators: make(map[rune]bool)}} - - runes := []rune(expressionText) - for i := 0; i < len(runes); i++ { - r := runes[i] - - // Skip whitespace - if isWhitespace(r) { - continue - } - - // Enter a new scope on open parenthesis - if r == '(' { - scopeStack = append(scopeStack, scope{operators: make(map[rune]bool)}) - continue - } - - // Exit scope on close parenthesis - check for mixed operators before popping - if r == ')' { - if len(scopeStack) > 1 { - // Check the scope we're about to pop for mixed operators - if warning := checkScope(scopeStack[len(scopeStack)-1]); warning != nil { - return warning - } - scopeStack = scopeStack[:len(scopeStack)-1] - } - continue - } - - // Skip arrow operator (->) - if r == '-' && i+1 < len(runes) && runes[i+1] == '>' { - i++ // Skip the '>' - continue - } - - // Skip dot notation for function calls (e.g., parent.all) - if r == '.' { - continue - } - - // Check for operators: +, -, & - // Only count operators that are not part of an identifier - if r == '+' || r == '-' || r == '&' { - // Look back to see if the previous non-whitespace char was an identifier char - // If so, this might be part of a different construct, but in SpiceDB schema - // these operators should always be surrounded by whitespace or parens - currentScope := &scopeStack[len(scopeStack)-1] - currentScope.operators[r] = true - continue - } - } - - // Check remaining scopes (including root) for mixed operators - for _, s := range scopeStack { - if warning := checkScope(s); warning != nil { - return warning + relation.Name, + position, + ), nil } - } - return nil -} - -func isWhitespace(r rune) bool { - return r == ' ' || r == '\t' || r == '\n' || r == '\r' + return nil, nil + }, } diff --git a/pkg/development/warnings.go b/pkg/development/warnings.go index a45418d69..048855a78 100644 --- a/pkg/development/warnings.go +++ b/pkg/development/warnings.go @@ -18,6 +18,7 @@ import ( var allChecks = checks{ relationChecks: []relationCheck{ lintRelationReferencesParentType, + lintMixedOperatorsWithoutParentheses, }, computedUsersetChecks: []computedUsersetCheck{ lintPermissionReferencingItself, @@ -115,125 +116,12 @@ func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinitio } warnings = append(warnings, found...) - - // Check for mixed operators without parentheses using source scanning - if !shouldSkipCheck(rel.Metadata, "mixed-operators-without-parentheses") { - expressionText := extractPermissionExpression(schemaLines, rel.Name, rel.GetSourcePosition()) - if expressionText != "" { - if warning := CheckExpressionForMixedOperators(rel.Name, expressionText, rel.GetSourcePosition()); warning != nil { - warnings = append(warnings, warning) - } - } - } } } return warnings, nil } -// extractPermissionExpression extracts the expression text for a permission from the schema source. -// It uses the source position to locate the permission and extracts the text after the '=' sign. -// The schemaLines parameter should be pre-split for performance when checking multiple permissions. -func extractPermissionExpression(schemaLines []string, _ string, sourcePosition *corev1.SourcePosition) string { - if sourcePosition == nil || len(schemaLines) == 0 { - return "" - } - - lineIdx, err := safecast.Convert[int](sourcePosition.ZeroIndexedLineNumber) - if err != nil || lineIdx < 0 || lineIdx >= len(schemaLines) { - return "" - } - - // Start from the permission's line and look for the expression - // The expression is everything after '=' until end of statement - var expressionBuilder strings.Builder - foundEquals := false - parenDepth := 0 - inBlockComment := false - lastNonWhitespaceWasOperator := false - - for i := lineIdx; i < len(schemaLines); i++ { - line := schemaLines[i] - - // If this is the first line, start from the column position - startCol := 0 - if i == lineIdx { - colPos, err := safecast.Convert[int](sourcePosition.ZeroIndexedColumnPosition) - if err == nil && colPos < len(line) { - startCol = colPos - } - } - - lineHasContent := false - for j := startCol; j < len(line); j++ { - ch := line[j] - - // Handle block comment state - if inBlockComment { - if ch == '*' && j+1 < len(line) && line[j+1] == '/' { - inBlockComment = false - j++ // Skip the '/' - } - continue - } - - // Check for start of block comment - if ch == '/' && j+1 < len(line) && line[j+1] == '*' { - inBlockComment = true - j++ // Skip the '*' - continue - } - - // Check for line comment - skip rest of line - if ch == '/' && j+1 < len(line) && line[j+1] == '/' { - break - } - - if !foundEquals { - if ch == '=' { - foundEquals = true - } - continue - } - - // Track parenthesis depth for multi-line expressions - switch ch { - case '(': - parenDepth++ - case ')': - parenDepth-- - } - - // Track if this is an operator (for multi-line continuation detection) - isWhitespaceChar := ch == ' ' || ch == '\t' - if !isWhitespaceChar { - lineHasContent = true - lastNonWhitespaceWasOperator = (ch == '+' || ch == '-' || ch == '&') - } - - expressionBuilder.WriteByte(ch) - } - - // Determine if expression continues on next line: - // 1. We're inside parentheses (parenDepth > 0) - // 2. We're inside a block comment - // 3. The line ended with an operator (suggesting continuation) - // 4. The line had no content yet after '=' (e.g., '= \n foo + bar') - if foundEquals { - continueToNextLine := parenDepth > 0 || inBlockComment || lastNonWhitespaceWasOperator || !lineHasContent - if !continueToNextLine { - break - } - // Add space for multi-line expressions - if i < len(schemaLines)-1 { - expressionBuilder.WriteByte(' ') - } - } - } - - return strings.TrimSpace(expressionBuilder.String()) -} - func shouldSkipCheck(metadata *corev1.Metadata, name string) bool { if metadata == nil { return false diff --git a/pkg/development/warnings_misc_test.go b/pkg/development/warnings_misc_test.go index c835d82c4..a5191d5d7 100644 --- a/pkg/development/warnings_misc_test.go +++ b/pkg/development/warnings_misc_test.go @@ -1,7 +1,6 @@ package development import ( - "strings" "testing" "github.com/stretchr/testify/require" @@ -210,251 +209,3 @@ func TestWrappedFunctionedTTUUnknownFunction(t *testing.T) { _, _ = wrapped.GetArrowString() }) } - -func TestCheckExpressionForMixedOperators(t *testing.T) { - testCases := []struct { - name string - expression string - expectWarning bool - expectedInMsg string - }{ - { - name: "single union operator", - expression: "foo + bar", - expectWarning: false, - }, - { - name: "single intersection operator", - expression: "foo & bar", - expectWarning: false, - }, - { - name: "single exclusion operator", - expression: "foo - bar", - expectWarning: false, - }, - { - name: "multiple same operators - union", - expression: "foo + bar + baz", - expectWarning: false, - }, - { - name: "multiple same operators - intersection", - expression: "foo & bar & baz", - expectWarning: false, - }, - { - name: "multiple same operators - exclusion", - expression: "foo - bar - baz", - expectWarning: false, - }, - { - name: "mixed union and exclusion at same depth", - expression: "foo + bar - baz", - expectWarning: true, - expectedInMsg: "union (+) and exclusion (-)", - }, - { - name: "mixed exclusion and intersection at same depth", - expression: "foo - bar & baz", - expectWarning: true, - expectedInMsg: "exclusion (-) and intersection (&)", - }, - { - name: "mixed union and intersection at same depth", - expression: "foo + bar & baz", - expectWarning: true, - expectedInMsg: "union (+) and intersection (&)", - }, - { - name: "all three operators at same depth", - expression: "foo + bar - baz & qux", - expectWarning: true, - expectedInMsg: "union (+)", - }, - { - name: "mixed operators with parentheses - different depths", - expression: "(foo + bar) - baz", - expectWarning: false, - }, - { - name: "mixed operators with parentheses - right side", - expression: "foo - (bar & baz)", - expectWarning: false, - }, - { - name: "complex parenthesized expression - no warning", - expression: "(foo + bar) - (baz & qux)", - expectWarning: false, - }, - { - name: "arrow expression should not trigger warning", - expression: "parent->view + editor", - expectWarning: false, - }, - { - name: "arrow expression with mixed operators should trigger", - expression: "parent->view + editor - blocked", - expectWarning: true, - expectedInMsg: "union (+) and exclusion (-)", - }, - { - name: "function call expression", - expression: "parent.all(view) + editor", - expectWarning: false, - }, - { - name: "empty expression", - expression: "", - expectWarning: false, - }, - { - name: "only identifier", - expression: "viewer", - expectWarning: false, - }, - { - name: "nested parentheses all same operator", - expression: "(foo + bar) + (baz + qux)", - expectWarning: false, - }, - { - name: "deeply nested mixed operators at different depths", - expression: "((foo + bar) - baz) & qux", - expectWarning: false, - }, - { - name: "mixed operators inside parentheses should warn", - expression: "(foo + bar - baz)", - expectWarning: true, - expectedInMsg: "union (+) and exclusion (-)", - }, - { - name: "mixed operators in nested parens should warn", - expression: "qux & (foo + bar - baz)", - expectWarning: true, - expectedInMsg: "union (+) and exclusion (-)", - }, - } - - sourcePos := &core.SourcePosition{ - ZeroIndexedLineNumber: 0, - ZeroIndexedColumnPosition: 0, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - warning := CheckExpressionForMixedOperators("test_perm", tc.expression, sourcePos) - if tc.expectWarning { - require.NotNil(t, warning, "expected warning but got nil") - require.Contains(t, warning.Message, tc.expectedInMsg, "warning message should contain expected text") - require.Contains(t, warning.Message, "mixed-operators-without-parentheses") - } else { - require.Nil(t, warning, "expected no warning but got: %v", warning) - } - }) - } -} - -func TestCheckExpressionForMixedOperatorsNilSourcePosition(t *testing.T) { - warning := CheckExpressionForMixedOperators("test_perm", "foo + bar - baz", nil) - require.NotNil(t, warning, "should still produce warning with nil source position") - require.Equal(t, uint32(0), warning.Line) - require.Equal(t, uint32(0), warning.Column) -} - -func TestExtractPermissionExpression(t *testing.T) { - testCases := []struct { - name string - schema string - permissionName string - lineNumber uint64 - expectedExpression string - }{ - { - name: "simple permission", - schema: `definition test { - permission view = foo + bar - }`, - permissionName: "view", - lineNumber: 1, - expectedExpression: "foo + bar", - }, - { - name: "permission with comment after", - schema: `definition test { - permission view = foo + bar // some comment - }`, - permissionName: "view", - lineNumber: 1, - expectedExpression: "foo + bar", - }, - { - name: "empty schema", - schema: "", - permissionName: "view", - lineNumber: 0, - expectedExpression: "", - }, - { - name: "multi-line expression with operator at end of line", - schema: "definition test {\n\tpermission view = foo +\n\t\tbar - baz\n}", - permissionName: "view", - lineNumber: 1, - expectedExpression: "foo + \t\tbar - baz", - }, - { - name: "multi-line expression with parentheses", - schema: "definition test {\n\tpermission view = (foo +\n\t\tbar) - baz\n}", - permissionName: "view", - lineNumber: 1, - expectedExpression: "(foo + \t\tbar) - baz", - }, - { - name: "expression with block comment", - schema: `definition test { - permission view = foo /* this is a comment */ + bar - }`, - permissionName: "view", - lineNumber: 1, - expectedExpression: "foo + bar", - }, - { - name: "expression with block comment spanning content", - schema: `definition test { - permission view = foo + /* comment */ bar - baz - }`, - permissionName: "view", - lineNumber: 1, - expectedExpression: "foo + bar - baz", - }, - { - name: "multi-line expression without operator at end - single line", - schema: `definition test { - permission view = foo + bar - permission edit = baz - }`, - permissionName: "view", - lineNumber: 1, - expectedExpression: "foo + bar", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - sourcePos := &core.SourcePosition{ - ZeroIndexedLineNumber: tc.lineNumber, - ZeroIndexedColumnPosition: 0, - } - schemaLines := strings.Split(tc.schema, "\n") - result := extractPermissionExpression(schemaLines, tc.permissionName, sourcePos) - require.Equal(t, tc.expectedExpression, result) - }) - } -} - -func TestExtractPermissionExpressionNilSourcePosition(t *testing.T) { - schemaLines := strings.Split("definition test { permission view = foo }", "\n") - result := extractPermissionExpression(schemaLines, "view", nil) - require.Empty(t, result) -} diff --git a/pkg/development/warnings_test.go b/pkg/development/warnings_test.go index b381eafec..3eacbc6b9 100644 --- a/pkg/development/warnings_test.go +++ b/pkg/development/warnings_test.go @@ -240,7 +240,7 @@ func TestWarnings(t *testing.T) { { name: "exclusion operation", schema: `definition user {} - + definition document { relation viewer: user relation editor: user @@ -249,95 +249,33 @@ func TestWarnings(t *testing.T) { `, expectedWarning: nil, }, - // Mixed operators tests - { - name: "mixed operators without parentheses - exclusion and intersection", - schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = foo - bar & baz - } - `, - expectedWarning: &developerv1.DeveloperWarning{ - Message: "Permission \"view\" mixes exclusion (-) and intersection (&) at the same level of nesting; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", - Line: 7, - Column: 5, - SourceCode: "view", - }, - }, { - name: "mixed operators without parentheses - union and exclusion", + name: "mixed operators without parentheses", schema: `definition user {} - + definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = foo + bar - baz + relation viewer: user + relation editor: user + relation admin: user + permission view = viewer + editor - admin } `, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Permission \"view\" mixes union (+) and exclusion (-) at the same level of nesting; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Message: "Permission \"view\" mixes operators (union, intersection, exclusion) at the same level without explicit parentheses; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", Line: 7, - Column: 5, + Column: 23, SourceCode: "view", }, }, { - name: "mixed operators with parentheses - no warning", + name: "mixed operators with parentheses does not warn", schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = (foo - bar) & baz - } - `, - expectedWarning: nil, - }, - { - name: "mixed operators with parentheses on right - no warning", - schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = foo - (bar & baz) - } - `, - expectedWarning: nil, - }, - { - name: "single operator type - no warning", - schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = foo + bar + baz - } - `, - expectedWarning: nil, - }, - { - name: "arrow with mixed operators - no false positive on arrow", - schema: `definition user {} - - definition group { - relation member: user - permission view = member - } - + definition document { - relation parent: group relation viewer: user - permission view = parent->view + viewer + relation editor: user + relation admin: user + permission view = (viewer + editor) - admin } `, expectedWarning: nil, @@ -345,64 +283,35 @@ func TestWarnings(t *testing.T) { { name: "mixed operators warning disabled", schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - relation baz: user + definition document { + relation viewer: user + relation editor: user + relation admin: user // spicedb-ignore-warning: mixed-operators-without-parentheses - permission view = foo - bar & baz + permission view = viewer + editor - admin } `, expectedWarning: nil, }, - // Multi-line expression tests { - name: "multi-line mixed operators with operator at end of line", + name: "mixed operators with outer parentheses still warns", schema: `definition user {} - + definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = foo + - bar - baz + relation viewer: user + relation editor: user + relation admin: user + permission view = (viewer + editor - admin) } `, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Permission \"view\" mixes union (+) and exclusion (-) at the same level of nesting; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Message: "Permission \"view\" mixes operators (union, intersection, exclusion) at the same level without explicit parentheses; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", Line: 7, - Column: 5, + Column: 24, SourceCode: "view", }, }, - { - name: "multi-line expression with parentheses - no warning", - schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - relation baz: user - permission view = (foo + - bar) - baz - } - `, - expectedWarning: nil, - }, - { - name: "expression with block comment - no warning for same operator", - schema: `definition user {} - - definition document { - relation foo: user - relation bar: user - permission view = foo /* comment */ + bar - } - `, - expectedWarning: nil, - }, } for _, tc := range tcs { diff --git a/pkg/namespace/builder.go b/pkg/namespace/builder.go index 88a91d83b..81dc59153 100644 --- a/pkg/namespace/builder.go +++ b/pkg/namespace/builder.go @@ -73,6 +73,19 @@ func MustRelationWithComment(name string, comment string, rewrite *core.UsersetR return rel } +// WithMixedOperators marks the relation as having mixed operators without parentheses. +// This should only be used for test expectations. +func WithMixedOperators(rel *core.Relation, line uint64, column uint64) *core.Relation { + position := &core.SourcePosition{ + ZeroIndexedLineNumber: line, + ZeroIndexedColumnPosition: column, + } + if err := SetMixedOperatorsWithoutParens(rel, true, position); err != nil { + panic(err) + } + return rel +} + // AllowedRelation creates a relation reference to an allowed relation. func AllowedRelation(namespaceName string, relationName string) *core.AllowedRelation { return &core.AllowedRelation{ diff --git a/pkg/namespace/metadata.go b/pkg/namespace/metadata.go index 27cfa5255..976ed352a 100644 --- a/pkg/namespace/metadata.go +++ b/pkg/namespace/metadata.go @@ -193,3 +193,94 @@ func SetTypeAnnotations(relation *core.Relation, typeAnnotations []string) error relation.Metadata.MetadataMessage = append(relation.Metadata.MetadataMessage, metadataAny) return nil } + +// HasMixedOperatorsWithoutParens returns whether the permission expression contains mixed operators +// (union, intersection, exclusion) at the same scope level without explicit parentheses. +func HasMixedOperatorsWithoutParens(relation *core.Relation) bool { + if relation.Metadata == nil { + return false + } + + for _, metadataAny := range relation.Metadata.MetadataMessage { + var relationMetadata iv1.RelationMetadata + if err := metadataAny.UnmarshalTo(&relationMetadata); err != nil { + continue + } + + if relationMetadata.Kind == iv1.RelationMetadata_PERMISSION { + return relationMetadata.HasMixedOperatorsWithoutParentheses + } + } + + return false +} + +// GetMixedOperatorsPosition returns the source position of the first mixed operator found, +// or nil if none. +func GetMixedOperatorsPosition(relation *core.Relation) *core.SourcePosition { + if relation.Metadata == nil { + return nil + } + + for _, metadataAny := range relation.Metadata.MetadataMessage { + var relationMetadata iv1.RelationMetadata + if err := metadataAny.UnmarshalTo(&relationMetadata); err != nil { + continue + } + + if relationMetadata.Kind == iv1.RelationMetadata_PERMISSION { + return relationMetadata.MixedOperatorsPosition + } + } + + return nil +} + +// SetMixedOperatorsWithoutParens sets the mixed operators flag and position for a permission relation. +func SetMixedOperatorsWithoutParens(relation *core.Relation, hasMixed bool, position *core.SourcePosition) error { + if relation.Metadata == nil { + if !hasMixed { + return nil // Nothing to set + } + relation.Metadata = &core.Metadata{} + } + + // Find existing PERMISSION RelationMetadata and update it + for i, metadataAny := range relation.Metadata.MetadataMessage { + var relationMetadata iv1.RelationMetadata + if err := metadataAny.UnmarshalTo(&relationMetadata); err != nil { + continue + } + + if relationMetadata.Kind == iv1.RelationMetadata_PERMISSION { + relationMetadata.HasMixedOperatorsWithoutParentheses = hasMixed + relationMetadata.MixedOperatorsPosition = position + + updatedAny, err := anypb.New(&relationMetadata) + if err != nil { + return err + } + + relation.Metadata.MetadataMessage[i] = updatedAny + return nil + } + } + + // If no existing PERMISSION RelationMetadata found and we need to set the flag + if hasMixed { + relationMetadata := &iv1.RelationMetadata{ + Kind: iv1.RelationMetadata_PERMISSION, + HasMixedOperatorsWithoutParentheses: hasMixed, + MixedOperatorsPosition: position, + } + + metadataAny, err := anypb.New(relationMetadata) + if err != nil { + return err + } + + relation.Metadata.MetadataMessage = append(relation.Metadata.MetadataMessage, metadataAny) + } + + return nil +} diff --git a/pkg/proto/impl/v1/impl.pb.go b/pkg/proto/impl/v1/impl.pb.go index 8a2b2eec2..a48fd944e 100644 --- a/pkg/proto/impl/v1/impl.pb.go +++ b/pkg/proto/impl/v1/impl.pb.go @@ -7,6 +7,7 @@ package implv1 import ( + v1 "github.com/authzed/spicedb/pkg/proto/core/v1" v1alpha1 "google.golang.org/genproto/googleapis/api/expr/v1alpha1" protoreflect "google.golang.org/protobuf/reflect/protoreflect" protoimpl "google.golang.org/protobuf/runtime/protoimpl" @@ -571,8 +572,13 @@ type RelationMetadata struct { state protoimpl.MessageState `protogen:"open.v1"` Kind RelationMetadata_RelationKind `protobuf:"varint,1,opt,name=kind,proto3,enum=impl.v1.RelationMetadata_RelationKind" json:"kind,omitempty"` TypeAnnotations *TypeAnnotations `protobuf:"bytes,2,opt,name=type_annotations,json=typeAnnotations,proto3" json:"type_annotations,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // Indicates whether the permission expression contains mixed operators (union, intersection, + // exclusion) at the same scope level without explicit parentheses. + HasMixedOperatorsWithoutParentheses bool `protobuf:"varint,3,opt,name=has_mixed_operators_without_parentheses,json=hasMixedOperatorsWithoutParentheses,proto3" json:"has_mixed_operators_without_parentheses,omitempty"` + // The source position of the first mixed operator found, if any. + MixedOperatorsPosition *v1.SourcePosition `protobuf:"bytes,4,opt,name=mixed_operators_position,json=mixedOperatorsPosition,proto3" json:"mixed_operators_position,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *RelationMetadata) Reset() { @@ -619,6 +625,20 @@ func (x *RelationMetadata) GetTypeAnnotations() *TypeAnnotations { return nil } +func (x *RelationMetadata) GetHasMixedOperatorsWithoutParentheses() bool { + if x != nil { + return x.HasMixedOperatorsWithoutParentheses + } + return false +} + +func (x *RelationMetadata) GetMixedOperatorsPosition() *v1.SourcePosition { + if x != nil { + return x.MixedOperatorsPosition + } + return nil +} + type NamespaceAndRevision struct { state protoimpl.MessageState `protogen:"open.v1"` NamespaceName string `protobuf:"bytes,1,opt,name=namespace_name,json=namespaceName,proto3" json:"namespace_name,omitempty"` @@ -905,7 +925,7 @@ var File_impl_v1_impl_proto protoreflect.FileDescriptor const file_impl_v1_impl_proto_rawDesc = "" + "\n" + - "\x12impl/v1/impl.proto\x12\aimpl.v1\x1a&google/api/expr/v1alpha1/checked.proto\"l\n" + + "\x12impl/v1/impl.proto\x12\aimpl.v1\x1a&google/api/expr/v1alpha1/checked.proto\x1a\x12core/v1/core.proto\"l\n" + "\rDecodedCaveat\x129\n" + "\x03cel\x18\x01 \x01(\v2%.google.api.expr.v1alpha1.CheckedExprH\x00R\x03cel\x12\x12\n" + "\x04name\x18\x02 \x01(\tR\x04nameB\f\n" + @@ -948,10 +968,12 @@ const file_impl_v1_impl_proto_rawDesc = "" + "DocComment\x12\x18\n" + "\acomment\x18\x01 \x01(\tR\acomment\"'\n" + "\x0fTypeAnnotations\x12\x14\n" + - "\x05types\x18\x01 \x03(\tR\x05types\"\xd3\x01\n" + + "\x05types\x18\x01 \x03(\tR\x05types\"\xfc\x02\n" + "\x10RelationMetadata\x12:\n" + "\x04kind\x18\x01 \x01(\x0e2&.impl.v1.RelationMetadata.RelationKindR\x04kind\x12C\n" + - "\x10type_annotations\x18\x02 \x01(\v2\x18.impl.v1.TypeAnnotationsR\x0ftypeAnnotations\">\n" + + "\x10type_annotations\x18\x02 \x01(\v2\x18.impl.v1.TypeAnnotationsR\x0ftypeAnnotations\x12T\n" + + "'has_mixed_operators_without_parentheses\x18\x03 \x01(\bR#hasMixedOperatorsWithoutParentheses\x12Q\n" + + "\x18mixed_operators_position\x18\x04 \x01(\v2\x17.core.v1.SourcePositionR\x16mixedOperatorsPosition\">\n" + "\fRelationKind\x12\x10\n" + "\fUNKNOWN_KIND\x10\x00\x12\f\n" + "\bRELATION\x10\x01\x12\x0e\n" + @@ -996,6 +1018,7 @@ var file_impl_v1_impl_proto_goTypes = []any{ (*DecodedZedToken_V1ZedToken)(nil), // 14: impl.v1.DecodedZedToken.V1ZedToken nil, // 15: impl.v1.V1Cursor.FlagsEntry (*v1alpha1.CheckedExpr)(nil), // 16: google.api.expr.v1alpha1.CheckedExpr + (*v1.SourcePosition)(nil), // 17: core.v1.SourcePosition } var file_impl_v1_impl_proto_depIdxs = []int32{ 16, // 0: impl.v1.DecodedCaveat.cel:type_name -> google.api.expr.v1alpha1.CheckedExpr @@ -1007,12 +1030,13 @@ var file_impl_v1_impl_proto_depIdxs = []int32{ 15, // 6: impl.v1.V1Cursor.flags:type_name -> impl.v1.V1Cursor.FlagsEntry 0, // 7: impl.v1.RelationMetadata.kind:type_name -> impl.v1.RelationMetadata.RelationKind 7, // 8: impl.v1.RelationMetadata.type_annotations:type_name -> impl.v1.TypeAnnotations - 9, // 9: impl.v1.V1Alpha1Revision.ns_revisions:type_name -> impl.v1.NamespaceAndRevision - 10, // [10:10] is the sub-list for method output_type - 10, // [10:10] is the sub-list for method input_type - 10, // [10:10] is the sub-list for extension type_name - 10, // [10:10] is the sub-list for extension extendee - 0, // [0:10] is the sub-list for field type_name + 17, // 9: impl.v1.RelationMetadata.mixed_operators_position:type_name -> core.v1.SourcePosition + 9, // 10: impl.v1.V1Alpha1Revision.ns_revisions:type_name -> impl.v1.NamespaceAndRevision + 11, // [11:11] is the sub-list for method output_type + 11, // [11:11] is the sub-list for method input_type + 11, // [11:11] is the sub-list for extension type_name + 11, // [11:11] is the sub-list for extension extendee + 0, // [0:11] is the sub-list for field type_name } func init() { file_impl_v1_impl_proto_init() } diff --git a/pkg/proto/impl/v1/impl.pb.validate.go b/pkg/proto/impl/v1/impl.pb.validate.go index 6811f6333..f2d10aa2c 100644 --- a/pkg/proto/impl/v1/impl.pb.validate.go +++ b/pkg/proto/impl/v1/impl.pb.validate.go @@ -1068,6 +1068,37 @@ func (m *RelationMetadata) validate(all bool) error { } } + // no validation rules for HasMixedOperatorsWithoutParentheses + + if all { + switch v := interface{}(m.GetMixedOperatorsPosition()).(type) { + case interface{ ValidateAll() error }: + if err := v.ValidateAll(); err != nil { + errors = append(errors, RelationMetadataValidationError{ + field: "MixedOperatorsPosition", + reason: "embedded message failed validation", + cause: err, + }) + } + case interface{ Validate() error }: + if err := v.Validate(); err != nil { + errors = append(errors, RelationMetadataValidationError{ + field: "MixedOperatorsPosition", + reason: "embedded message failed validation", + cause: err, + }) + } + } + } else if v, ok := interface{}(m.GetMixedOperatorsPosition()).(interface{ Validate() error }); ok { + if err := v.Validate(); err != nil { + return RelationMetadataValidationError{ + field: "MixedOperatorsPosition", + reason: "embedded message failed validation", + cause: err, + } + } + } + if len(errors) > 0 { return RelationMetadataMultiError(errors) } diff --git a/pkg/proto/impl/v1/impl_vtproto.pb.go b/pkg/proto/impl/v1/impl_vtproto.pb.go index d5323399c..708b68193 100644 --- a/pkg/proto/impl/v1/impl_vtproto.pb.go +++ b/pkg/proto/impl/v1/impl_vtproto.pb.go @@ -6,6 +6,7 @@ package implv1 import ( fmt "fmt" + v1 "github.com/authzed/spicedb/pkg/proto/core/v1" protohelpers "github.com/planetscale/vtprotobuf/protohelpers" v1alpha1 "google.golang.org/genproto/googleapis/api/expr/v1alpha1" proto "google.golang.org/protobuf/proto" @@ -312,6 +313,14 @@ func (m *RelationMetadata) CloneVT() *RelationMetadata { r := new(RelationMetadata) r.Kind = m.Kind r.TypeAnnotations = m.TypeAnnotations.CloneVT() + r.HasMixedOperatorsWithoutParentheses = m.HasMixedOperatorsWithoutParentheses + if rhs := m.MixedOperatorsPosition; rhs != nil { + if vtpb, ok := interface{}(rhs).(interface{ CloneVT() *v1.SourcePosition }); ok { + r.MixedOperatorsPosition = vtpb.CloneVT() + } else { + r.MixedOperatorsPosition = proto.Clone(rhs).(*v1.SourcePosition) + } + } if len(m.unknownFields) > 0 { r.unknownFields = make([]byte, len(m.unknownFields)) copy(r.unknownFields, m.unknownFields) @@ -822,6 +831,16 @@ func (this *RelationMetadata) EqualVT(that *RelationMetadata) bool { if !this.TypeAnnotations.EqualVT(that.TypeAnnotations) { return false } + if this.HasMixedOperatorsWithoutParentheses != that.HasMixedOperatorsWithoutParentheses { + return false + } + if equal, ok := interface{}(this.MixedOperatorsPosition).(interface{ EqualVT(*v1.SourcePosition) bool }); ok { + if !equal.EqualVT(that.MixedOperatorsPosition) { + return false + } + } else if !proto.Equal(this.MixedOperatorsPosition, that.MixedOperatorsPosition) { + return false + } return string(this.unknownFields) == string(that.unknownFields) } @@ -1579,6 +1598,38 @@ func (m *RelationMetadata) MarshalToSizedBufferVT(dAtA []byte) (int, error) { i -= len(m.unknownFields) copy(dAtA[i:], m.unknownFields) } + if m.MixedOperatorsPosition != nil { + if vtmsg, ok := interface{}(m.MixedOperatorsPosition).(interface { + MarshalToSizedBufferVT([]byte) (int, error) + }); ok { + size, err := vtmsg.MarshalToSizedBufferVT(dAtA[:i]) + if err != nil { + return 0, err + } + i -= size + i = protohelpers.EncodeVarint(dAtA, i, uint64(size)) + } else { + encoded, err := proto.Marshal(m.MixedOperatorsPosition) + if err != nil { + return 0, err + } + i -= len(encoded) + copy(dAtA[i:], encoded) + i = protohelpers.EncodeVarint(dAtA, i, uint64(len(encoded))) + } + i-- + dAtA[i] = 0x22 + } + if m.HasMixedOperatorsWithoutParentheses { + i-- + if m.HasMixedOperatorsWithoutParentheses { + dAtA[i] = 1 + } else { + dAtA[i] = 0 + } + i-- + dAtA[i] = 0x18 + } if m.TypeAnnotations != nil { size, err := m.TypeAnnotations.MarshalToSizedBufferVT(dAtA[:i]) if err != nil { @@ -1978,6 +2029,19 @@ func (m *RelationMetadata) SizeVT() (n int) { l = m.TypeAnnotations.SizeVT() n += 1 + l + protohelpers.SizeOfVarint(uint64(l)) } + if m.HasMixedOperatorsWithoutParentheses { + n += 2 + } + if m.MixedOperatorsPosition != nil { + if size, ok := interface{}(m.MixedOperatorsPosition).(interface { + SizeVT() int + }); ok { + l = size.SizeVT() + } else { + l = proto.Size(m.MixedOperatorsPosition) + } + n += 1 + l + protohelpers.SizeOfVarint(uint64(l)) + } n += len(m.unknownFields) return n } @@ -3446,6 +3510,70 @@ func (m *RelationMetadata) UnmarshalVT(dAtA []byte) error { return err } iNdEx = postIndex + case 3: + if wireType != 0 { + return fmt.Errorf("proto: wrong wireType = %d for field HasMixedOperatorsWithoutParentheses", wireType) + } + var v int + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return protohelpers.ErrIntOverflow + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + v |= int(b&0x7F) << shift + if b < 0x80 { + break + } + } + m.HasMixedOperatorsWithoutParentheses = bool(v != 0) + case 4: + if wireType != 2 { + return fmt.Errorf("proto: wrong wireType = %d for field MixedOperatorsPosition", wireType) + } + var msglen int + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return protohelpers.ErrIntOverflow + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + msglen |= int(b&0x7F) << shift + if b < 0x80 { + break + } + } + if msglen < 0 { + return protohelpers.ErrInvalidLength + } + postIndex := iNdEx + msglen + if postIndex < 0 { + return protohelpers.ErrInvalidLength + } + if postIndex > l { + return io.ErrUnexpectedEOF + } + if m.MixedOperatorsPosition == nil { + m.MixedOperatorsPosition = &v1.SourcePosition{} + } + if unmarshal, ok := interface{}(m.MixedOperatorsPosition).(interface { + UnmarshalVT([]byte) error + }); ok { + if err := unmarshal.UnmarshalVT(dAtA[iNdEx:postIndex]); err != nil { + return err + } + } else { + if err := proto.Unmarshal(dAtA[iNdEx:postIndex], m.MixedOperatorsPosition); err != nil { + return err + } + } + iNdEx = postIndex default: iNdEx = preIndex skippy, err := protohelpers.Skip(dAtA[iNdEx:]) diff --git a/pkg/schemadsl/compiler/compiler_test.go b/pkg/schemadsl/compiler/compiler_test.go index c249baed2..cd591fff5 100644 --- a/pkg/schemadsl/compiler/compiler_test.go +++ b/pkg/schemadsl/compiler/compiler_test.go @@ -313,16 +313,19 @@ func TestCompile(t *testing.T) { "", []SchemaDefinition{ namespace.Namespace("sometenant/complex", - namespace.MustRelation("foos", - namespace.Exclusion( - namespace.Rewrite( - namespace.Union( - namespace.ComputedUserset("bars"), - namespace.ComputedUserset("bazs"), + namespace.WithMixedOperators( + namespace.MustRelation("foos", + namespace.Exclusion( + namespace.Rewrite( + namespace.Union( + namespace.ComputedUserset("bars"), + namespace.ComputedUserset("bazs"), + ), ), + namespace.ComputedUserset("mehs"), ), - namespace.ComputedUserset("mehs"), ), + 1, 22, // line 1, column 22 (0-indexed) ), ), }, diff --git a/pkg/schemadsl/compiler/translator.go b/pkg/schemadsl/compiler/translator.go index bef09c425..13033276d 100644 --- a/pkg/schemadsl/compiler/translator.go +++ b/pkg/schemadsl/compiler/translator.go @@ -401,6 +401,9 @@ func translatePermission(tctx *translationContext, permissionNode *dslNode) (*co return nil, permissionNode.Errorf("invalid permission expression: %w", err) } + // Detect mixed operators without parentheses before translation (which may flatten the AST). + mixedOpsPosition := detectMixedOperatorsWithoutParens(tctx, expressionNode) + rewrite, err := translateExpression(tctx, expressionNode) if err != nil { return nil, err @@ -419,6 +422,14 @@ func translatePermission(tctx *translationContext, permissionNode *dslNode) (*co } } + // Store mixed operators flag in metadata + if mixedOpsPosition != nil { + err = namespace.SetMixedOperatorsWithoutParens(permission, true, mixedOpsPosition) + if err != nil { + return nil, permissionNode.Errorf("error adding mixed operators flag to metadata: %w", err) + } + } + if !tctx.skipValidate { if err := permission.Validate(); err != nil { return nil, permissionNode.Errorf("error in permission %s: %w", permissionName, err) @@ -569,6 +580,14 @@ func translateExpressionOperationDirect(tctx *translationContext, expressionOpNo case dslshape.NodeTypeSelfExpression: return namespace.Self(), nil + case dslshape.NodeTypeParenthesizedExpression: + // Unwrap the parenthesized expression and translate its inner expression. + innerExprNode, err := expressionOpNode.Lookup(dslshape.NodeParenthesizedExpressionPredicateInnerExpr) + if err != nil { + return nil, err + } + return translateExpressionOperation(tctx, innerExprNode) + case dslshape.NodeTypeArrowExpression: leftChild, err := expressionOpNode.Lookup(dslshape.NodeExpressionPredicateLeftExpr) if err != nil { @@ -760,3 +779,91 @@ func translateUseFlag(tctx *translationContext, useFlagNode *dslNode) error { tctx.enabledFlags = append(tctx.enabledFlags, flagName) return nil } + +// operatorType represents the type of set operator in an expression. +type operatorType int + +const ( + operatorTypeUnknown operatorType = iota + operatorTypeUnion + operatorTypeIntersection + operatorTypeExclusion +) + +// getOperatorType returns the operator type for a given node type, or operatorTypeUnknown if not a set operator. +func getOperatorType(nodeType dslshape.NodeType) operatorType { + switch nodeType { + case dslshape.NodeTypeUnionExpression: + return operatorTypeUnion + case dslshape.NodeTypeIntersectExpression: + return operatorTypeIntersection + case dslshape.NodeTypeExclusionExpression: + return operatorTypeExclusion + default: + return operatorTypeUnknown + } +} + +// detectMixedOperatorsWithoutParens walks the expression AST and detects if there are mixed +// operators (union, intersection, exclusion) at the same scope level without explicit parentheses. +// Returns the source position of the first mixed operator found, or nil if none. +// Parenthesized expressions act as boundaries - mixing inside parens does not trigger a warning +// since the user explicitly grouped the expression. However, top-level parentheses are unwrapped +// since they don't clarify internal operator precedence. +func detectMixedOperatorsWithoutParens(tctx *translationContext, node *dslNode) *core.SourcePosition { + // Unwrap top-level parenthesized expressions - they don't clarify internal precedence. + // e.g., (a + b - c) should still warn about mixed operators. + for node.GetType() == dslshape.NodeTypeParenthesizedExpression { + innerNode, err := node.Lookup(dslshape.NodeParenthesizedExpressionPredicateInnerExpr) + if err != nil { + break + } + node = innerNode + } + return detectMixedOperatorsInScope(tctx, node, operatorTypeUnknown) +} + +// detectMixedOperatorsInScope recursively checks for mixed operators within a scope. +// parentOp is the operator type seen so far at this scope level. +func detectMixedOperatorsInScope(tctx *translationContext, node *dslNode, parentOp operatorType) *core.SourcePosition { + nodeType := node.GetType() + + // Parenthesized expressions act as a boundary - don't propagate operator checking into them. + // The user explicitly grouped the expression, so we don't warn about mixing inside. + if nodeType == dslshape.NodeTypeParenthesizedExpression { + return nil + } + + currentOp := getOperatorType(nodeType) + + // If this is a set operator and we've seen a different operator at this scope, it's mixed. + if currentOp != operatorTypeUnknown && parentOp != operatorTypeUnknown && currentOp != parentOp { + return getSourcePosition(node, tctx.mapper) + } + + // If this is a set operator, check children with this operator as the scope's operator. + if currentOp != operatorTypeUnknown { + effectiveOp := currentOp + if parentOp != operatorTypeUnknown { + effectiveOp = parentOp // Keep the first operator seen at this scope + } + + // Check left child + leftChild, err := node.Lookup(dslshape.NodeExpressionPredicateLeftExpr) + if err == nil { + if pos := detectMixedOperatorsInScope(tctx, leftChild, effectiveOp); pos != nil { + return pos + } + } + + // Check right child + rightChild, err := node.Lookup(dslshape.NodeExpressionPredicateRightExpr) + if err == nil { + if pos := detectMixedOperatorsInScope(tctx, rightChild, effectiveOp); pos != nil { + return pos + } + } + } + + return nil +} diff --git a/pkg/schemadsl/dslshape/dslshape.go b/pkg/schemadsl/dslshape/dslshape.go index 331917ea6..027150091 100644 --- a/pkg/schemadsl/dslshape/dslshape.go +++ b/pkg/schemadsl/dslshape/dslshape.go @@ -33,9 +33,10 @@ const ( NodeTypeArrowExpression // A TTU in arrow form. - NodeTypeIdentifier // An identifier under an expression. - NodeTypeNilExpression // A nil keyword - NodeTypeSelfExpression // A self keyword + NodeTypeIdentifier // An identifier under an expression. + NodeTypeNilExpression // A nil keyword + NodeTypeSelfExpression // A self keyword + NodeTypeParenthesizedExpression // A parenthesized expression wrapper. NodeTypeCaveatTypeReference // A type reference for a caveat parameter. ) @@ -218,4 +219,11 @@ const ( // NodeExpressionPredicateLeftExpr = "left-expr" NodeExpressionPredicateRightExpr = "right-expr" + + // + // NodeTypeParenthesizedExpression + // + + // The inner expression wrapped by parentheses. + NodeParenthesizedExpressionPredicateInnerExpr = "inner-expr" ) diff --git a/pkg/schemadsl/dslshape/zz_generated.nodetype_string.go b/pkg/schemadsl/dslshape/zz_generated.nodetype_string.go index f3659d8fc..77a8944b9 100644 --- a/pkg/schemadsl/dslshape/zz_generated.nodetype_string.go +++ b/pkg/schemadsl/dslshape/zz_generated.nodetype_string.go @@ -30,12 +30,13 @@ func _() { _ = x[NodeTypeIdentifier-19] _ = x[NodeTypeNilExpression-20] _ = x[NodeTypeSelfExpression-21] - _ = x[NodeTypeCaveatTypeReference-22] + _ = x[NodeTypeParenthesizedExpression-22] + _ = x[NodeTypeCaveatTypeReference-23] } -const _NodeType_name = "NodeTypeErrorNodeTypeFileNodeTypeCommentNodeTypeUseFlagNodeTypeDefinitionNodeTypeCaveatDefinitionNodeTypeCaveatParameterNodeTypeCaveatExpressionNodeTypeRelationNodeTypePermissionNodeTypeTypeAnnotationNodeTypeTypeReferenceNodeTypeSpecificTypeReferenceNodeTypeCaveatReferenceNodeTypeTraitReferenceNodeTypeUnionExpressionNodeTypeIntersectExpressionNodeTypeExclusionExpressionNodeTypeArrowExpressionNodeTypeIdentifierNodeTypeNilExpressionNodeTypeSelfExpressionNodeTypeCaveatTypeReference" +const _NodeType_name = "NodeTypeErrorNodeTypeFileNodeTypeCommentNodeTypeUseFlagNodeTypeDefinitionNodeTypeCaveatDefinitionNodeTypeCaveatParameterNodeTypeCaveatExpressionNodeTypeRelationNodeTypePermissionNodeTypeTypeAnnotationNodeTypeTypeReferenceNodeTypeSpecificTypeReferenceNodeTypeCaveatReferenceNodeTypeTraitReferenceNodeTypeUnionExpressionNodeTypeIntersectExpressionNodeTypeExclusionExpressionNodeTypeArrowExpressionNodeTypeIdentifierNodeTypeNilExpressionNodeTypeSelfExpressionNodeTypeParenthesizedExpressionNodeTypeCaveatTypeReference" -var _NodeType_index = [...]uint16{0, 13, 25, 40, 55, 73, 97, 120, 144, 160, 178, 200, 221, 250, 273, 295, 318, 345, 372, 395, 413, 434, 456, 483} +var _NodeType_index = [...]uint16{0, 13, 25, 40, 55, 73, 97, 120, 144, 160, 178, 200, 221, 250, 273, 295, 318, 345, 372, 395, 413, 434, 456, 487, 514} func (i NodeType) String() string { idx := int(i) - 0 diff --git a/pkg/schemadsl/parser/parser.go b/pkg/schemadsl/parser/parser.go index fc0d2ce4f..0c2300757 100644 --- a/pkg/schemadsl/parser/parser.go +++ b/pkg/schemadsl/parser/parser.go @@ -650,18 +650,23 @@ func (p *sourceParser) tryConsumeArrowExpression() (AstNode, bool) { // ```self``` func (p *sourceParser) tryConsumeBaseExpression() (AstNode, bool) { switch { - // Nested expression. + // Nested expression - wrap in NodeTypeParenthesizedExpression to preserve parentheses info. case p.isToken(lexer.TokenTypeLeftParen): comments := p.currentToken.comments + wrapperNode := p.startNode(dslshape.NodeTypeParenthesizedExpression) + p.consume(lexer.TokenTypeLeftParen) - exprNode := p.consumeComputeExpression() + innerExprNode := p.consumeComputeExpression() p.consume(lexer.TokenTypeRightParen) - // Attach any comments found to the consumed expression. - p.decorateComments(exprNode, comments) + wrapperNode.Connect(dslshape.NodeParenthesizedExpressionPredicateInnerExpr, innerExprNode) + p.mustFinishNode() + + // Attach any comments found to the wrapper node. + p.decorateComments(wrapperNode, comments) - return exprNode, true + return wrapperNode, true // Self expression. case p.isKeyword("self"): diff --git a/pkg/schemadsl/parser/tests/basic.zed.expected b/pkg/schemadsl/parser/tests/basic.zed.expected index a98c1030b..aaf6b2b87 100644 --- a/pkg/schemadsl/parser/tests/basic.zed.expected +++ b/pkg/schemadsl/parser/tests/basic.zed.expected @@ -84,22 +84,27 @@ NodeTypeFile input-source = basic definition test start-rune = 208 left-expr => - NodeTypeExclusionExpression - end-rune = 217 + NodeTypeParenthesizedExpression + end-rune = 218 input-source = basic definition test - start-rune = 209 - left-expr => - NodeTypeIdentifier - end-rune = 211 - identifier-value = foo - input-source = basic definition test - start-rune = 209 - right-expr => - NodeTypeIdentifier + start-rune = 208 + inner-expr => + NodeTypeExclusionExpression end-rune = 217 - identifier-value = meh input-source = basic definition test - start-rune = 215 + start-rune = 209 + left-expr => + NodeTypeIdentifier + end-rune = 211 + identifier-value = foo + input-source = basic definition test + start-rune = 209 + right-expr => + NodeTypeIdentifier + end-rune = 217 + identifier-value = meh + input-source = basic definition test + start-rune = 215 right-expr => NodeTypeIdentifier end-rune = 224 diff --git a/pkg/schemadsl/parser/tests/multiparen.zed.expected b/pkg/schemadsl/parser/tests/multiparen.zed.expected index 5bf967dd3..d7424687b 100644 --- a/pkg/schemadsl/parser/tests/multiparen.zed.expected +++ b/pkg/schemadsl/parser/tests/multiparen.zed.expected @@ -42,58 +42,68 @@ NodeTypeFile input-source = multiple parens test start-rune = 44 right-expr => - NodeTypeExclusionExpression - end-rune = 61 + NodeTypeParenthesizedExpression + end-rune = 62 input-source = multiple parens test - start-rune = 51 - left-expr => - NodeTypeArrowExpression - end-rune = 54 + start-rune = 50 + inner-expr => + NodeTypeExclusionExpression + end-rune = 61 input-source = multiple parens test start-rune = 51 left-expr => - NodeTypeIdentifier - end-rune = 51 - identifier-value = a - input-source = multiple parens test - start-rune = 51 - right-expr => - NodeTypeIdentifier + NodeTypeArrowExpression end-rune = 54 - identifier-value = b - input-source = multiple parens test - start-rune = 54 - right-expr => - NodeTypeArrowExpression - end-rune = 61 - input-source = multiple parens test - start-rune = 58 - left-expr => - NodeTypeIdentifier - end-rune = 58 - identifier-value = c input-source = multiple parens test - start-rune = 58 + start-rune = 51 + left-expr => + NodeTypeIdentifier + end-rune = 51 + identifier-value = a + input-source = multiple parens test + start-rune = 51 + right-expr => + NodeTypeIdentifier + end-rune = 54 + identifier-value = b + input-source = multiple parens test + start-rune = 54 right-expr => - NodeTypeIdentifier + NodeTypeArrowExpression end-rune = 61 - identifier-value = d input-source = multiple parens test - start-rune = 61 + start-rune = 58 + left-expr => + NodeTypeIdentifier + end-rune = 58 + identifier-value = c + input-source = multiple parens test + start-rune = 58 + right-expr => + NodeTypeIdentifier + end-rune = 61 + identifier-value = d + input-source = multiple parens test + start-rune = 61 right-expr => - NodeTypeIntersectExpression - end-rune = 75 + NodeTypeParenthesizedExpression + end-rune = 76 input-source = multiple parens test - start-rune = 67 - left-expr => - NodeTypeIdentifier - end-rune = 69 - identifier-value = maz - input-source = multiple parens test - start-rune = 67 - right-expr => - NodeTypeIdentifier + start-rune = 66 + inner-expr => + NodeTypeIntersectExpression end-rune = 75 - identifier-value = beh input-source = multiple parens test - start-rune = 73 \ No newline at end of file + start-rune = 67 + left-expr => + NodeTypeIdentifier + end-rune = 69 + identifier-value = maz + input-source = multiple parens test + start-rune = 67 + right-expr => + NodeTypeIdentifier + end-rune = 75 + identifier-value = beh + input-source = multiple parens test + start-rune = 73 \ No newline at end of file diff --git a/pkg/schemadsl/parser/tests/nil.zed.expected b/pkg/schemadsl/parser/tests/nil.zed.expected index 90bc5578b..2c6ec7081 100644 --- a/pkg/schemadsl/parser/tests/nil.zed.expected +++ b/pkg/schemadsl/parser/tests/nil.zed.expected @@ -67,32 +67,37 @@ NodeTypeFile input-source = nil test start-rune = 117 left-expr => - NodeTypeUnionExpression - end-rune = 128 + NodeTypeParenthesizedExpression + end-rune = 129 input-source = nil test - start-rune = 118 - left-expr => + start-rune = 117 + inner-expr => NodeTypeUnionExpression - end-rune = 122 + end-rune = 128 input-source = nil test start-rune = 118 left-expr => - NodeTypeIdentifier - end-rune = 118 - identifier-value = a + NodeTypeUnionExpression + end-rune = 122 input-source = nil test start-rune = 118 + left-expr => + NodeTypeIdentifier + end-rune = 118 + identifier-value = a + input-source = nil test + start-rune = 118 + right-expr => + NodeTypeIdentifier + end-rune = 122 + identifier-value = b + input-source = nil test + start-rune = 122 right-expr => - NodeTypeIdentifier - end-rune = 122 - identifier-value = b + NodeTypeNilExpression + end-rune = 128 input-source = nil test - start-rune = 122 - right-expr => - NodeTypeNilExpression - end-rune = 128 - input-source = nil test - start-rune = 126 + start-rune = 126 right-expr => NodeTypeIdentifier end-rune = 133 diff --git a/pkg/schemadsl/parser/tests/parens.zed.expected b/pkg/schemadsl/parser/tests/parens.zed.expected index cbd5b3b45..79eb54e54 100644 --- a/pkg/schemadsl/parser/tests/parens.zed.expected +++ b/pkg/schemadsl/parser/tests/parens.zed.expected @@ -15,19 +15,24 @@ NodeTypeFile relation-name = bar start-rune = 21 compute-expression => - NodeTypeIntersectExpression - end-rune = 47 + NodeTypeParenthesizedExpression + end-rune = 48 input-source = parens test - start-rune = 39 - left-expr => - NodeTypeIdentifier - end-rune = 41 - identifier-value = maz - input-source = parens test - start-rune = 39 - right-expr => - NodeTypeIdentifier + start-rune = 38 + inner-expr => + NodeTypeIntersectExpression end-rune = 47 - identifier-value = beh input-source = parens test - start-rune = 45 \ No newline at end of file + start-rune = 39 + left-expr => + NodeTypeIdentifier + end-rune = 41 + identifier-value = maz + input-source = parens test + start-rune = 39 + right-expr => + NodeTypeIdentifier + end-rune = 47 + identifier-value = beh + input-source = parens test + start-rune = 45 \ No newline at end of file diff --git a/pkg/schemadsl/parser/tests/permission_edge_cases.zed.expected b/pkg/schemadsl/parser/tests/permission_edge_cases.zed.expected index 104caac02..fef3337bf 100644 --- a/pkg/schemadsl/parser/tests/permission_edge_cases.zed.expected +++ b/pkg/schemadsl/parser/tests/permission_edge_cases.zed.expected @@ -135,22 +135,27 @@ NodeTypeFile input-source = permission edge cases test start-rune = 340 right-expr => - NodeTypeExclusionExpression - end-rune = 364 + NodeTypeParenthesizedExpression + end-rune = 365 input-source = permission edge cases test - start-rune = 350 - left-expr => - NodeTypeIdentifier - end-rune = 355 - identifier-value = viewer - input-source = permission edge cases test - start-rune = 350 - right-expr => - NodeTypeIdentifier + start-rune = 349 + inner-expr => + NodeTypeExclusionExpression end-rune = 364 - identifier-value = viewer input-source = permission edge cases test - start-rune = 359 + start-rune = 350 + left-expr => + NodeTypeIdentifier + end-rune = 355 + identifier-value = viewer + input-source = permission edge cases test + start-rune = 350 + right-expr => + NodeTypeIdentifier + end-rune = 364 + identifier-value = viewer + input-source = permission edge cases test + start-rune = 359 type-annotations => NodeTypeTypeAnnotation end-rune = 336 diff --git a/proto/internal/impl/v1/impl.proto b/proto/internal/impl/v1/impl.proto index cd85ece0c..af705d149 100644 --- a/proto/internal/impl/v1/impl.proto +++ b/proto/internal/impl/v1/impl.proto @@ -2,6 +2,7 @@ syntax = "proto3"; package impl.v1; import "google/api/expr/v1alpha1/checked.proto"; +import "core/v1/core.proto"; option go_package = "github.com/authzed/spicedb/pkg/proto/impl/v1"; @@ -90,6 +91,13 @@ message RelationMetadata { RelationKind kind = 1; TypeAnnotations type_annotations = 2; + + // Indicates whether the permission expression contains mixed operators (union, intersection, + // exclusion) at the same scope level without explicit parentheses. + bool has_mixed_operators_without_parentheses = 3; + + // The source position of the first mixed operator found, if any. + core.v1.SourcePosition mixed_operators_position = 4; } message NamespaceAndRevision { From 882008f870c313939e6736744bd3a1de58ea285f Mon Sep 17 00:00:00 2001 From: ivanauth Date: Wed, 7 Jan 2026 16:00:59 -0500 Subject: [PATCH 03/10] fix: lint issues - proto import order and panic check --- pkg/namespace/builder.go | 2 +- proto/internal/impl/v1/impl.proto | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/namespace/builder.go b/pkg/namespace/builder.go index 81dc59153..cb8af8ed0 100644 --- a/pkg/namespace/builder.go +++ b/pkg/namespace/builder.go @@ -81,7 +81,7 @@ func WithMixedOperators(rel *core.Relation, line uint64, column uint64) *core.Re ZeroIndexedColumnPosition: column, } if err := SetMixedOperatorsWithoutParens(rel, true, position); err != nil { - panic(err) + panic(spiceerrors.MustBugf("failed to set mixed operators: %s", err.Error())) } return rel } diff --git a/proto/internal/impl/v1/impl.proto b/proto/internal/impl/v1/impl.proto index af705d149..3f847ac67 100644 --- a/proto/internal/impl/v1/impl.proto +++ b/proto/internal/impl/v1/impl.proto @@ -1,8 +1,8 @@ syntax = "proto3"; package impl.v1; -import "google/api/expr/v1alpha1/checked.proto"; import "core/v1/core.proto"; +import "google/api/expr/v1alpha1/checked.proto"; option go_package = "github.com/authzed/spicedb/pkg/proto/impl/v1"; From 7972d3415f348c10dfbae8ea0f103e446dc4d28b Mon Sep 17 00:00:00 2001 From: ivanauth Date: Wed, 7 Jan 2026 16:36:02 -0500 Subject: [PATCH 04/10] fix: rename WithMixedOperators to MustWithMixedOperators Satisfies panic check linter which allows panics in Must* functions. Also regenerates proto files after import order fix. --- pkg/composableschemadsl/compiler/compiler_test.go | 2 +- pkg/namespace/builder.go | 4 ++-- pkg/proto/impl/v1/impl.pb.go | 2 +- pkg/schemadsl/compiler/compiler_test.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/composableschemadsl/compiler/compiler_test.go b/pkg/composableschemadsl/compiler/compiler_test.go index a10834949..d41468850 100644 --- a/pkg/composableschemadsl/compiler/compiler_test.go +++ b/pkg/composableschemadsl/compiler/compiler_test.go @@ -573,7 +573,7 @@ func TestCompile(t *testing.T) { "", []SchemaDefinition{ namespace.Namespace("sometenant/complex", - namespace.WithMixedOperators( + namespace.MustWithMixedOperators( namespace.MustRelation("foos", namespace.Exclusion( namespace.Rewrite( diff --git a/pkg/namespace/builder.go b/pkg/namespace/builder.go index cb8af8ed0..cb116a752 100644 --- a/pkg/namespace/builder.go +++ b/pkg/namespace/builder.go @@ -73,9 +73,9 @@ func MustRelationWithComment(name string, comment string, rewrite *core.UsersetR return rel } -// WithMixedOperators marks the relation as having mixed operators without parentheses. +// MustWithMixedOperators marks the relation as having mixed operators without parentheses. // This should only be used for test expectations. -func WithMixedOperators(rel *core.Relation, line uint64, column uint64) *core.Relation { +func MustWithMixedOperators(rel *core.Relation, line uint64, column uint64) *core.Relation { position := &core.SourcePosition{ ZeroIndexedLineNumber: line, ZeroIndexedColumnPosition: column, diff --git a/pkg/proto/impl/v1/impl.pb.go b/pkg/proto/impl/v1/impl.pb.go index a48fd944e..9e3b37b51 100644 --- a/pkg/proto/impl/v1/impl.pb.go +++ b/pkg/proto/impl/v1/impl.pb.go @@ -925,7 +925,7 @@ var File_impl_v1_impl_proto protoreflect.FileDescriptor const file_impl_v1_impl_proto_rawDesc = "" + "\n" + - "\x12impl/v1/impl.proto\x12\aimpl.v1\x1a&google/api/expr/v1alpha1/checked.proto\x1a\x12core/v1/core.proto\"l\n" + + "\x12impl/v1/impl.proto\x12\aimpl.v1\x1a\x12core/v1/core.proto\x1a&google/api/expr/v1alpha1/checked.proto\"l\n" + "\rDecodedCaveat\x129\n" + "\x03cel\x18\x01 \x01(\v2%.google.api.expr.v1alpha1.CheckedExprH\x00R\x03cel\x12\x12\n" + "\x04name\x18\x02 \x01(\tR\x04nameB\f\n" + diff --git a/pkg/schemadsl/compiler/compiler_test.go b/pkg/schemadsl/compiler/compiler_test.go index cd591fff5..ef6f98b1f 100644 --- a/pkg/schemadsl/compiler/compiler_test.go +++ b/pkg/schemadsl/compiler/compiler_test.go @@ -313,7 +313,7 @@ func TestCompile(t *testing.T) { "", []SchemaDefinition{ namespace.Namespace("sometenant/complex", - namespace.WithMixedOperators( + namespace.MustWithMixedOperators( namespace.MustRelation("foos", namespace.Exclusion( namespace.Rewrite( From edcb2ee2e68f693557f8ea18318916cab1ad2e42 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Wed, 7 Jan 2026 16:42:14 -0500 Subject: [PATCH 05/10] fix: gofmt formatting in metadata.go --- pkg/namespace/metadata.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/namespace/metadata.go b/pkg/namespace/metadata.go index 976ed352a..2ef2ae70a 100644 --- a/pkg/namespace/metadata.go +++ b/pkg/namespace/metadata.go @@ -269,7 +269,7 @@ func SetMixedOperatorsWithoutParens(relation *core.Relation, hasMixed bool, posi // If no existing PERMISSION RelationMetadata found and we need to set the flag if hasMixed { relationMetadata := &iv1.RelationMetadata{ - Kind: iv1.RelationMetadata_PERMISSION, + Kind: iv1.RelationMetadata_PERMISSION, HasMixedOperatorsWithoutParentheses: hasMixed, MixedOperatorsPosition: position, } From c693b77b4d3f4f85a903886e89aad5cda5a6ac3e Mon Sep 17 00:00:00 2001 From: ivanauth Date: Thu, 8 Jan 2026 10:10:38 -0500 Subject: [PATCH 06/10] ci: re-run tests From be4dcfe48cc00b94a951d83354e6697439fe5d00 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Thu, 8 Jan 2026 10:50:01 -0500 Subject: [PATCH 07/10] ci: re-run tests (attempt 2) From 5c2b616eb4da1e672103a8d324ace3ae006a2a61 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Fri, 23 Jan 2026 12:55:47 -0500 Subject: [PATCH 08/10] test: add coverage for mixed operators metadata helpers and warning lint Add TestMixedOperatorsMetadata to pkg/namespace/metadata_test.go covering all code paths in HasMixedOperatorsWithoutParens, GetMixedOperatorsPosition, and SetMixedOperatorsWithoutParens (nil metadata, empty metadata, non-permission relation, permission with/without flag, set/clear operations, creation of new metadata entries). Add additional warning test cases to pkg/development/warnings_test.go for mixed intersection+exclusion, intersection+union operator combinations, same-operators-repeated (no warning), and relation-referencing-parent for a non-permission relation. --- pkg/development/warnings_test.go | 64 +++++++ pkg/namespace/metadata_test.go | 276 +++++++++++++++++++++++++++++++ 2 files changed, 340 insertions(+) diff --git a/pkg/development/warnings_test.go b/pkg/development/warnings_test.go index 3eacbc6b9..f21e4aaa7 100644 --- a/pkg/development/warnings_test.go +++ b/pkg/development/warnings_test.go @@ -312,6 +312,70 @@ func TestWarnings(t *testing.T) { SourceCode: "view", }, }, + { + name: "mixed intersection and exclusion without parentheses", + schema: `definition user {} + + definition document { + relation viewer: user + relation editor: user + relation admin: user + permission view = viewer & editor - admin + } + `, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Permission \"view\" mixes operators (union, intersection, exclusion) at the same level without explicit parentheses; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Line: 7, + Column: 23, + SourceCode: "view", + }, + }, + { + name: "mixed intersection and union without parentheses", + schema: `definition user {} + + definition document { + relation viewer: user + relation editor: user + relation admin: user + permission view = viewer & editor + admin + } + `, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Permission \"view\" mixes operators (union, intersection, exclusion) at the same level without explicit parentheses; consider adding parentheses to clarify precedence (mixed-operators-without-parentheses)", + Line: 7, + Column: 32, + SourceCode: "view", + }, + }, + { + name: "same operators repeated does not warn", + schema: `definition user {} + + definition document { + relation viewer: user + relation editor: user + relation admin: user + permission view = viewer + editor + admin + } + `, + expectedWarning: nil, + }, + { + name: "relation name referencing parent is a relation not permission", + schema: `definition user {} + + definition document { + relation viewer_document: user + permission view = viewer_document + }`, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Relation \"viewer_document\" references parent type \"document\" in its name; it is recommended to drop the suffix (relation-name-references-parent)", + Line: 4, + Column: 5, + SourceCode: "viewer_document", + }, + }, } for _, tc := range tcs { diff --git a/pkg/namespace/metadata_test.go b/pkg/namespace/metadata_test.go index 73def5f3a..9de5ec07e 100644 --- a/pkg/namespace/metadata_test.go +++ b/pkg/namespace/metadata_test.go @@ -61,6 +61,282 @@ func TestMetadata(t *testing.T) { require.Equal(iv1.RelationMetadata_PERMISSION, GetRelationKind(ns.Relation[0])) } +func TestMixedOperatorsMetadata(t *testing.T) { + tests := []struct { + name string + setupRelation func() *core.Relation + performSet bool + setMixed bool + setPosition *core.SourcePosition + expectedHasMixed bool + expectedPosition *core.SourcePosition + }{ + { + name: "has mixed returns false for nil metadata", + setupRelation: func() *core.Relation { + return &core.Relation{Name: "test"} + }, + expectedHasMixed: false, + expectedPosition: nil, + }, + { + name: "has mixed returns false for empty metadata", + setupRelation: func() *core.Relation { + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{}, + } + }, + expectedHasMixed: false, + expectedPosition: nil, + }, + { + name: "has mixed returns false for non-permission relation", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{Kind: iv1.RelationMetadata_RELATION} + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + expectedHasMixed: false, + expectedPosition: nil, + }, + { + name: "has mixed returns false for permission without mixed flag", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{Kind: iv1.RelationMetadata_PERMISSION} + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + expectedHasMixed: false, + expectedPosition: nil, + }, + { + name: "has mixed returns true when flag is set", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{ + Kind: iv1.RelationMetadata_PERMISSION, + HasMixedOperatorsWithoutParentheses: true, + MixedOperatorsPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 5, + ZeroIndexedColumnPosition: 10, + }, + } + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + expectedHasMixed: true, + expectedPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 5, + ZeroIndexedColumnPosition: 10, + }, + }, + { + name: "has mixed returns true with nil position", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{ + Kind: iv1.RelationMetadata_PERMISSION, + HasMixedOperatorsWithoutParentheses: true, + } + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + expectedHasMixed: true, + expectedPosition: nil, + }, + { + name: "set mixed on relation with no metadata", + setupRelation: func() *core.Relation { + return &core.Relation{Name: "test"} + }, + performSet: true, + setMixed: true, + setPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 3, + ZeroIndexedColumnPosition: 7, + }, + expectedHasMixed: true, + expectedPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 3, + ZeroIndexedColumnPosition: 7, + }, + }, + { + name: "set mixed false on relation with no metadata is no-op", + setupRelation: func() *core.Relation { + return &core.Relation{Name: "test"} + }, + performSet: true, + setMixed: false, + setPosition: nil, + expectedHasMixed: false, + expectedPosition: nil, + }, + { + name: "set mixed updates existing permission metadata", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{ + Kind: iv1.RelationMetadata_PERMISSION, + } + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + performSet: true, + setMixed: true, + setPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 2, + ZeroIndexedColumnPosition: 15, + }, + expectedHasMixed: true, + expectedPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 2, + ZeroIndexedColumnPosition: 15, + }, + }, + { + name: "set mixed on relation with non-permission metadata creates new entry", + setupRelation: func() *core.Relation { + docComment := &iv1.DocComment{Comment: "test comment"} + docAny, _ := anypb.New(docComment) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{docAny}, + }, + } + }, + performSet: true, + setMixed: true, + setPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 1, + ZeroIndexedColumnPosition: 5, + }, + expectedHasMixed: true, + expectedPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 1, + ZeroIndexedColumnPosition: 5, + }, + }, + { + name: "set mixed false clears existing flag", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{ + Kind: iv1.RelationMetadata_PERMISSION, + HasMixedOperatorsWithoutParentheses: true, + MixedOperatorsPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 5, + ZeroIndexedColumnPosition: 10, + }, + } + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + performSet: true, + setMixed: false, + setPosition: nil, + expectedHasMixed: false, + expectedPosition: nil, + }, + { + name: "metadata with doc comment before relation metadata", + setupRelation: func() *core.Relation { + docComment := &iv1.DocComment{Comment: "a comment"} + docAny, _ := anypb.New(docComment) + relationMetadata := &iv1.RelationMetadata{ + Kind: iv1.RelationMetadata_PERMISSION, + HasMixedOperatorsWithoutParentheses: true, + MixedOperatorsPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 8, + ZeroIndexedColumnPosition: 20, + }, + } + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{docAny, metadataAny}, + }, + } + }, + expectedHasMixed: true, + expectedPosition: &core.SourcePosition{ + ZeroIndexedLineNumber: 8, + ZeroIndexedColumnPosition: 20, + }, + }, + { + name: "set mixed false on relation with non-permission metadata only", + setupRelation: func() *core.Relation { + relationMetadata := &iv1.RelationMetadata{Kind: iv1.RelationMetadata_RELATION} + metadataAny, _ := anypb.New(relationMetadata) + return &core.Relation{ + Name: "test", + Metadata: &core.Metadata{ + MetadataMessage: []*anypb.Any{metadataAny}, + }, + } + }, + performSet: true, + setMixed: false, + setPosition: nil, + expectedHasMixed: false, + expectedPosition: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + relation := tt.setupRelation() + + if tt.performSet { + err := SetMixedOperatorsWithoutParens(relation, tt.setMixed, tt.setPosition) + require.NoError(t, err) + } + + hasMixed := HasMixedOperatorsWithoutParens(relation) + require.Equal(t, tt.expectedHasMixed, hasMixed) + + position := GetMixedOperatorsPosition(relation) + if tt.expectedPosition == nil { + require.Nil(t, position) + } else { + require.NotNil(t, position) + require.Equal(t, tt.expectedPosition.ZeroIndexedLineNumber, position.ZeroIndexedLineNumber) + require.Equal(t, tt.expectedPosition.ZeroIndexedColumnPosition, position.ZeroIndexedColumnPosition) + } + }) + } +} + func TestTypeAnnotations(t *testing.T) { tests := []struct { name string From 2a7fb8d267ff2cd694a82a1b80d46ce51e0ff8c6 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Thu, 29 Jan 2026 11:19:23 -0500 Subject: [PATCH 09/10] test: update expected parser output for self_test Update the expected AST output for the self_test to reflect the new NodeTypeParenthesizedExpression wrapper that is now emitted when parsing parenthesized expressions. Co-Authored-By: Claude Opus 4.5 --- .../parser/tests/use_self.zed.expected | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/pkg/schemadsl/parser/tests/use_self.zed.expected b/pkg/schemadsl/parser/tests/use_self.zed.expected index bb772b1ea..7485fabd5 100644 --- a/pkg/schemadsl/parser/tests/use_self.zed.expected +++ b/pkg/schemadsl/parser/tests/use_self.zed.expected @@ -72,32 +72,37 @@ NodeTypeFile input-source = self test start-rune = 125 left-expr => - NodeTypeUnionExpression - end-rune = 137 + NodeTypeParenthesizedExpression + end-rune = 138 input-source = self test - start-rune = 126 - left-expr => + start-rune = 125 + inner-expr => NodeTypeUnionExpression - end-rune = 130 + end-rune = 137 input-source = self test start-rune = 126 left-expr => - NodeTypeIdentifier - end-rune = 126 - identifier-value = a + NodeTypeUnionExpression + end-rune = 130 input-source = self test start-rune = 126 + left-expr => + NodeTypeIdentifier + end-rune = 126 + identifier-value = a + input-source = self test + start-rune = 126 + right-expr => + NodeTypeIdentifier + end-rune = 130 + identifier-value = b + input-source = self test + start-rune = 130 right-expr => - NodeTypeIdentifier - end-rune = 130 - identifier-value = b + NodeTypeSelfExpression + end-rune = 137 input-source = self test - start-rune = 130 - right-expr => - NodeTypeSelfExpression - end-rune = 137 - input-source = self test - start-rune = 134 + start-rune = 134 right-expr => NodeTypeIdentifier end-rune = 142 From ebb6aa63e51e0ee768e0e773859452073ff46d1f Mon Sep 17 00:00:00 2001 From: ivanauth Date: Thu, 29 Jan 2026 12:00:26 -0500 Subject: [PATCH 10/10] fix: update composableschemadsl self_test expected output --- .../parser/tests/use_self.zed.expected | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/pkg/composableschemadsl/parser/tests/use_self.zed.expected b/pkg/composableschemadsl/parser/tests/use_self.zed.expected index bb772b1ea..7485fabd5 100644 --- a/pkg/composableschemadsl/parser/tests/use_self.zed.expected +++ b/pkg/composableschemadsl/parser/tests/use_self.zed.expected @@ -72,32 +72,37 @@ NodeTypeFile input-source = self test start-rune = 125 left-expr => - NodeTypeUnionExpression - end-rune = 137 + NodeTypeParenthesizedExpression + end-rune = 138 input-source = self test - start-rune = 126 - left-expr => + start-rune = 125 + inner-expr => NodeTypeUnionExpression - end-rune = 130 + end-rune = 137 input-source = self test start-rune = 126 left-expr => - NodeTypeIdentifier - end-rune = 126 - identifier-value = a + NodeTypeUnionExpression + end-rune = 130 input-source = self test start-rune = 126 + left-expr => + NodeTypeIdentifier + end-rune = 126 + identifier-value = a + input-source = self test + start-rune = 126 + right-expr => + NodeTypeIdentifier + end-rune = 130 + identifier-value = b + input-source = self test + start-rune = 130 right-expr => - NodeTypeIdentifier - end-rune = 130 - identifier-value = b + NodeTypeSelfExpression + end-rune = 137 input-source = self test - start-rune = 130 - right-expr => - NodeTypeSelfExpression - end-rune = 137 - input-source = self test - start-rune = 134 + start-rune = 134 right-expr => NodeTypeIdentifier end-rune = 142