Merge pull request #2263 from authzed/OS-222/disallow-def-partial-same-name

OS-222: Require definitions and partials to have distinct names

Author: tstirrat15

pkg/composableschemadsl/compiler/compiler.go

@@ -130,14 +130,16 @@ func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*Co
 		return nil, err
 	}
 
+	initialCompiledPartials := make(map[string][]*core.Relation)
+
 	compiled, err := translate(&translationContext{
 		objectTypePrefix:   cfg.objectTypePrefix,
 		mapper:             mapper,
 		schemaString:       schema.SchemaString,
 		skipValidate:       cfg.skipValidation,
 		allowedFlags:       cfg.allowedFlags,
 		existingNames:      mapz.NewSet[string](),
-		compiledPartials:   make(map[string][]*core.Relation),
+		compiledPartials:   initialCompiledPartials,
 		unresolvedPartials: mapz.NewMultiMap[string, *dslNode](),
 	}, root)
 	if err != nil {

pkg/composableschemadsl/compiler/compiler_test.go

@@ -50,7 +50,7 @@ func TestCompile(t *testing.T) {
 			"nested parse error",
 			withTenantPrefix,
 			`definition foo {
-				relation something: rela | relb + relc	
+				relation something: rela | relb + relc
 			}`,
 			"parse error in `nested parse error`, line 2, column 37: Expected end of statement or definition, found: TokenTypePlus",
 			[]SchemaDefinition{},
@@ -307,6 +307,36 @@ func TestCompile(t *testing.T) {
 			"could not find partial reference",
 			[]SchemaDefinition{},
 		},
+		{
+			"definition with same name as partial",
+			withTenantPrefix,
+			`
+			partial simple {
+				relation user: user
+			}
+			definition simple {
+				...simple
+			}
+			`,
+			"found definition with same name as existing partial",
+			[]SchemaDefinition{},
+		},
+		{
+			"caveat with same name as partial",
+			withTenantPrefix,
+			`
+			caveat some_caveat(someparam int) { someparam == 42}
+
+			partial some_caveat {
+				relation user: user
+			}
+			definition simple {
+				...some_caveat
+			}
+			`,
+			"found caveat with same name as existing partial",
+			[]SchemaDefinition{},
+		},
 		{
 			"definition reference to another definition errors",
 			withTenantPrefix,
@@ -925,7 +955,7 @@ func TestCompile(t *testing.T) {
 						"thirdParam":   caveattypes.MustListType(caveattypes.IntType),
 					},
 				), "sometenant/foo",
-					`someParam == 42 && someParam != 43 && someParam < 12 && someParam > 56 
+					`someParam == 42 && someParam != 43 && someParam < 12 && someParam > 56
 					&& anotherParam == "hi there" && 42 in thirdParam`),
 			},
 		},
@@ -1210,7 +1240,7 @@ func TestCompile(t *testing.T) {
 			"relation with expiration trait",
 			withTenantPrefix,
 			`use expiration
-			
+
 			definition simple {
 				relation viewer: user with expiration
 			}`,
@@ -1250,7 +1280,7 @@ func TestCompile(t *testing.T) {
 			"relation with expiration trait and caveat",
 			withTenantPrefix,
 			`use expiration
-			
+
 			definition simple {
 				relation viewer: user with somecaveat and expiration
 			}`,

pkg/composableschemadsl/compiler/translator.go

@@ -95,6 +95,10 @@ func translate(tctx *translationContext, root *dslNode) (*CompiledSchema, error)
 				return nil, topLevelNode.WithSourceErrorf(name, "found name reused between multiple definitions and/or caveats: %s", name)
 			}
 
+			if _, found := tctx.compiledPartials[name]; found {
+				return nil, topLevelNode.WithSourceErrorf(name, "found caveat with same name as existing partial: %s", name)
+			}
+
 			orderedDefinitions = append(orderedDefinitions, def)
 
 		case dslshape.NodeTypeDefinition:
@@ -111,6 +115,10 @@ func translate(tctx *translationContext, root *dslNode) (*CompiledSchema, error)
 				return nil, topLevelNode.WithSourceErrorf(name, "found name reused between multiple definitions and/or caveats: %s", name)
 			}
 
+			if _, found := tctx.compiledPartials[name]; found {
+				return nil, topLevelNode.WithSourceErrorf(name, "found definition with same name as existing partial: %s", name)
+			}
+
 			orderedDefinitions = append(orderedDefinitions, def)
 		}
 	}
@@ -864,6 +872,11 @@ func translatePartial(tctx *translationContext, partialNode *dslNode) error {
 	if err != nil {
 		return err
 	}
+	// Use the prefixed path to align with namespace and definition compilation
+	partialPath, err := tctx.prefixedPath(partialName)
+	if err != nil {
+		return err
+	}
 	// This needs to return the unresolved name.
 	errorOnMissingReference := false
 	relationsAndPermissions, unresolvedPartial, err := translateRelationsAndPermissions(tctx, partialNode, errorOnMissingReference)
@@ -875,22 +888,22 @@ func translatePartial(tctx *translationContext, partialNode *dslNode) error {
 		return nil
 	}
 
-	tctx.compiledPartials[partialName] = relationsAndPermissions
+	tctx.compiledPartials[partialPath] = relationsAndPermissions
 
 	// Since we've successfully compiled a partial, check the unresolved partials to see if any other partial was
 	// waiting on this partial
 	// NOTE: we're making an assumption here that a partial can't end up back in the same
 	// list of unresolved partials - if it hangs again in a different spot, it will end up in a different
 	// list of unresolved partials.
-	waitingPartials, _ := tctx.unresolvedPartials.Get(partialName)
+	waitingPartials, _ := tctx.unresolvedPartials.Get(partialPath)
 	for _, waitingPartialNode := range waitingPartials {
 		err := translatePartial(tctx, waitingPartialNode)
 		if err != nil {
 			return err
 		}
 	}
 	// Clear out this partial's key from the unresolved partials if it's not already empty.
-	tctx.unresolvedPartials.RemoveKey(partialName)
+	tctx.unresolvedPartials.RemoveKey(partialPath)
 	return nil
 }
 
@@ -902,14 +915,18 @@ func translatePartialReference(tctx *translationContext, partialReferenceNode *d
 	if err != nil {
 		return nil, "", err
 	}
-	relationsAndPermissions, ok := tctx.compiledPartials[name]
+	path, err := tctx.prefixedPath(name)
+	if err != nil {
+		return nil, "", err
+	}
+	relationsAndPermissions, ok := tctx.compiledPartials[path]
 	if !ok {
 		if errorOnMissingReference {
-			return nil, "", partialReferenceNode.Errorf("could not find partial reference with name %s", name)
+			return nil, "", partialReferenceNode.Errorf("could not find partial reference with name %s", path)
 		}
 		// If the partial isn't present and we're not throwing an error, we return the name of the missing partial
 		// This behavior supports partial collection
-		return nil, name, nil
+		return nil, path, nil
 	}
 	return relationsAndPermissions, "", nil
 }