deduplicate edges when conditions exist and add conditions to the edg…

…es (#422) * deduplicate edges when conditions exist and add conditions to the edges * add more comments * Update pkg/go/graph/graph_builder.go Co-authored-by: Yamil Asusta <[email protected]> * Update pkg/go/graph/weighted_graph_edge.go Co-authored-by: Yamil Asusta <[email protected]> * Update pkg/go/graph/weighted_graph.go Co-authored-by: Yamil Asusta <[email protected]> * fix lint errors --------- Co-authored-by: Yamil Asusta <[email protected]>
openfga · Feb 20, 2025 · ed0cfba · ed0cfba
1 parent 52cd7e9
commit ed0cfba
Show file tree

Hide file tree

Showing 10 changed files with 485 additions and 263 deletions.
diff --git a/pkg/go/graph/graph.go b/pkg/go/graph/graph.go
@@ -83,7 +83,8 @@ func (g *AuthorizationModelGraph) Reversed() (*AuthorizationModelGraph, error) {
 			if !ok {
 				return nil, fmt.Errorf("%w: could not cast to AuthorizationModelEdge", ErrBuildingGraph)
 			}
-			graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.conditionedOn)
+			newEdge := graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.tuplesetRelation, "")
+			newEdge.conditions = casted.conditions
 		}
 	}
 

diff --git a/pkg/go/graph/graph_builder.go b/pkg/go/graph/graph_builder.go
@@ -46,7 +46,7 @@ func parseModel(model *openfgav1.AuthorizationModel) (*multi.DirectedGraph, Node
 	})
 
 	for _, typeDef := range sortedTypeDefs {
-		graphBuilder.GetOrAddNode(typeDef.GetType(), typeDef.GetType(), SpecificType)
+		graphBuilder.getOrAddNode(typeDef.GetType(), typeDef.GetType(), SpecificType)
 
 		// sort relations by name to guarantee stable output
 		sortedRelations := make([]string, 0, len(typeDef.GetRelations()))
@@ -58,7 +58,7 @@ func parseModel(model *openfgav1.AuthorizationModel) (*multi.DirectedGraph, Node
 
 		for _, relation := range sortedRelations {
 			uniqueLabel := fmt.Sprintf("%s#%s", typeDef.GetType(), relation)
-			parentNode := graphBuilder.GetOrAddNode(uniqueLabel, uniqueLabel, SpecificTypeAndRelation)
+			parentNode := graphBuilder.getOrAddNode(uniqueLabel, uniqueLabel, SpecificTypeAndRelation)
 			rewrite := typeDef.GetRelations()[relation]
 			checkRewrite(graphBuilder, parentNode, model, rewrite, typeDef, relation)
 		}
@@ -107,11 +107,11 @@ func checkRewrite(graphBuilder *AuthorizationModelGraphBuilder, parentNode *Auth
 	}
 
 	operatorNode := fmt.Sprintf("%s:%s", operator, ulid.Make().String())
-	operatorNodeParent := graphBuilder.GetOrAddNode(operatorNode, operator, OperatorNode)
+	operatorNodeParent := graphBuilder.getOrAddNode(operatorNode, operator, OperatorNode)
 
 	// add one edge "operator" -> "relation that defined the operator"
 	// Note: if this is a composition of operators, operationNode will be nil and this edge won't be added.
-	graphBuilder.AddEdge(operatorNodeParent, parentNode, RewriteEdge, "")
+	graphBuilder.AddEdge(operatorNodeParent, parentNode, RewriteEdge, "", "")
 	for _, child := range children {
 		checkRewrite(graphBuilder, operatorNodeParent, model, child, typeDef, relation)
 	}
@@ -129,42 +129,38 @@ func parseThis(graphBuilder *AuthorizationModelGraphBuilder, parentNode graph.No
 		if directlyRelatedDef.GetRelationOrWildcard() == nil {
 			// direct assignment to concrete type
 			assignableType := directlyRelatedDef.GetType()
-			curNode = graphBuilder.GetOrAddNode(assignableType, assignableType, SpecificType)
+			curNode = graphBuilder.getOrAddNode(assignableType, assignableType, SpecificType)
 		}
 
 		if directlyRelatedDef.GetWildcard() != nil {
 			// direct assignment to wildcard
 			assignableWildcard := directlyRelatedDef.GetType() + ":*"
-			curNode = graphBuilder.GetOrAddNode(assignableWildcard, assignableWildcard, SpecificTypeWildcard)
+			curNode = graphBuilder.getOrAddNode(assignableWildcard, assignableWildcard, SpecificTypeWildcard)
 		}
 
 		if directlyRelatedDef.GetRelation() != "" {
 			// direct assignment to userset
 			assignableUserset := directlyRelatedDef.GetType() + "#" + directlyRelatedDef.GetRelation()
-			curNode = graphBuilder.GetOrAddNode(assignableUserset, assignableUserset, SpecificTypeAndRelation)
+			curNode = graphBuilder.getOrAddNode(assignableUserset, assignableUserset, SpecificTypeAndRelation)
 		}
 
-		if graphBuilder.HasEdge(curNode, parentNode, DirectEdge, "") {
-			// de-dup types that are conditioned, e.g. if define viewer: [user, user with condX]
-			// we only draw one edge from user to x#viewer
-			continue
-		}
-
-		graphBuilder.AddEdge(curNode, parentNode, DirectEdge, "")
+		// de-dup types that are conditioned, e.g. if define viewer: [user, user with condX]
+		// we only draw one edge from user to x#viewer, but with two conditions: none and condX
+		graphBuilder.upsertEdge(curNode, parentNode, DirectEdge, "", directlyRelatedDef.GetCondition())
 	}
 }
 
 func parseComputed(graphBuilder *AuthorizationModelGraphBuilder, parentNode *AuthorizationModelNode, typeDef *openfgav1.TypeDefinition, relation string) {
 	nodeType := RewriteEdge
 	// e.g. define x: y. Here y is the rewritten relation
 	rewrittenNodeName := fmt.Sprintf("%s#%s", typeDef.GetType(), relation)
-	newNode := graphBuilder.GetOrAddNode(rewrittenNodeName, rewrittenNodeName, SpecificTypeAndRelation)
+	newNode := graphBuilder.getOrAddNode(rewrittenNodeName, rewrittenNodeName, SpecificTypeAndRelation)
 	// new edge from y to x
 
 	if parentNode.nodeType == SpecificTypeAndRelation && newNode.nodeType == SpecificTypeAndRelation {
 		nodeType = ComputedEdge
 	}
-	graphBuilder.AddEdge(newNode, parentNode, nodeType, "")
+	graphBuilder.AddEdge(newNode, parentNode, nodeType, "", "")
 }
 
 func parseTupleToUserset(graphBuilder *AuthorizationModelGraphBuilder, parentNode graph.Node, model *openfgav1.AuthorizationModel, typeDef *openfgav1.TypeDefinition, rewrite *openfgav1.TupleToUserset) {
@@ -188,16 +184,22 @@ func parseTupleToUserset(graphBuilder *AuthorizationModelGraphBuilder, parentNod
 		}
 
 		rewrittenNodeName := fmt.Sprintf("%s#%s", tuplesetType, computedRelation)
-		nodeSource := graphBuilder.GetOrAddNode(rewrittenNodeName, rewrittenNodeName, SpecificTypeAndRelation)
-		conditionedOnNodeName := fmt.Sprintf("%s#%s", typeDef.GetType(), tuplesetRelation)
+		nodeSource := graphBuilder.getOrAddNode(rewrittenNodeName, rewrittenNodeName, SpecificTypeAndRelation)
+		typeTuplesetRelation := fmt.Sprintf("%s#%s", typeDef.GetType(), tuplesetRelation)
 
-		// new edge from "xxx#admin" to "yyy#viewer" conditioned on "yyy#parent"
-		graphBuilder.AddEdge(nodeSource, parentNode, TTUEdge, conditionedOnNodeName)
+		if graphBuilder.hasEdge(nodeSource, parentNode, TTUEdge, typeTuplesetRelation) {
+			// de-dup types that are conditioned, e.g. if define viewer: [user, user with condX]
+			// we only draw one edge from user to x#viewer
+			continue
+		}
+
+		// new edge from "xxx#admin" to "yyy#viewer" tuplesetRelation on "yyy#parent"
+		graphBuilder.upsertEdge(nodeSource, parentNode, TTUEdge, typeTuplesetRelation, relatedType.GetCondition())
 	}
 }
 
-func (g *AuthorizationModelGraphBuilder) GetOrAddNode(uniqueLabel, label string, nodeType NodeType) *AuthorizationModelNode {
-	if existingNode := g.GetNodeFor(uniqueLabel); existingNode != nil {
+func (g *AuthorizationModelGraphBuilder) getOrAddNode(uniqueLabel, label string, nodeType NodeType) *AuthorizationModelNode {
+	if existingNode := g.getNodeByLabel(uniqueLabel); existingNode != nil {
 		return existingNode
 	}
 
@@ -215,7 +217,7 @@ func (g *AuthorizationModelGraphBuilder) GetOrAddNode(uniqueLabel, label string,
 	return newNode
 }
 
-func (g *AuthorizationModelGraphBuilder) GetNodeFor(uniqueLabel string) *AuthorizationModelNode {
+func (g *AuthorizationModelGraphBuilder) getNodeByLabel(uniqueLabel string) *AuthorizationModelNode {
 	id, ok := g.ids[uniqueLabel]
 	if !ok {
 		return nil
@@ -229,31 +231,55 @@ func (g *AuthorizationModelGraphBuilder) GetNodeFor(uniqueLabel string) *Authori
 	return authModelNode
 }
 
-func (g *AuthorizationModelGraphBuilder) AddEdge(from, to graph.Node, edgeType EdgeType, conditionedOn string) *AuthorizationModelEdge {
+func (g *AuthorizationModelGraphBuilder) AddEdge(from, to graph.Node, edgeType EdgeType, tuplesetRelation string, condition string) *AuthorizationModelEdge {
 	if from == nil || to == nil {
 		return nil
 	}
+	if condition == "" {
+		condition = NoCond
+	}
+	conditions := []string{condition}
 
 	l := g.NewLine(from, to)
-	newLine := &AuthorizationModelEdge{Line: l, edgeType: edgeType, conditionedOn: conditionedOn}
+	newLine := &AuthorizationModelEdge{Line: l, edgeType: edgeType, tuplesetRelation: tuplesetRelation, conditions: conditions}
 	g.SetLine(newLine)
 
 	return newLine
 }
 
-func (g *AuthorizationModelGraphBuilder) HasEdge(from, to graph.Node, edgeType EdgeType, conditionedOn string) bool {
+func (g *AuthorizationModelGraphBuilder) upsertEdge(from, to graph.Node, edgeType EdgeType, tuplesetRelation string, condition string) {
 	if from == nil || to == nil {
-		return false
+		return
 	}
 
 	iter := g.Lines(from.ID(), to.ID())
 	for iter.Next() {
 		l := iter.Line()
-		edge, ok := l.(*AuthorizationModelEdge)
-		if !ok {
-			return false
+		edge, _ := l.(*AuthorizationModelEdge)
+		if edge.edgeType == edgeType && edge.tuplesetRelation == tuplesetRelation {
+			for _, cond := range edge.conditions {
+				if cond == condition {
+					return
+				}
+			}
+			edge.conditions = append(edge.conditions, condition)
+			return
 		}
-		if edge.edgeType == edgeType && edge.conditionedOn == conditionedOn {
+	}
+
+	g.AddEdge(from, to, edgeType, tuplesetRelation, condition)
+}
+
+func (g *AuthorizationModelGraphBuilder) hasEdge(from, to graph.Node, edgeType EdgeType, tuplesetRelation string) bool {
+	if from == nil || to == nil {
+		return false
+	}
+
+	iter := g.Lines(from.ID(), to.ID())
+	for iter.Next() {
+		l := iter.Line()
+		edge, _ := l.(*AuthorizationModelEdge)
+		if edge.edgeType == edgeType && edge.tuplesetRelation == tuplesetRelation {
 			return true
 		}
 	}

diff --git a/pkg/go/graph/graph_builder_test.go b/pkg/go/graph/graph_builder_test.go
@@ -287,6 +287,71 @@ rankdir=BT
 2 -> 1 [label=direct];
 4 -> 3 [headlabel="(document#parent)"];
 5 -> 4 [label=direct];
+}`,
+		},
+		`tuple_to_userset_conditional`: {
+			model: `
+				model
+					schema 1.1
+				type user
+				type document
+					relations
+						define parentt: [folder, folder with condX]
+						define viewer: admin from parentt
+				type folder
+					relations
+						define admin: [user]
+				condition condX (x:int) {
+					x > 0
+				}`,
+			expectedOutput: `digraph {
+graph [
+rankdir=BT
+];
+
+// Node definitions.
+0 [label=document];
+1 [label="document#parentt"];
+2 [label=folder];
+3 [label="document#viewer"];
+4 [label="folder#admin"];
+5 [label=user];
+
+// Edge definitions.
+2 -> 1 [label=direct];
+4 -> 3 [headlabel="(document#parentt)"];
+5 -> 4 [label=direct];
+}`,
+		},
+		`userset_conditional`: {
+			model: `
+				model
+					schema 1.1
+				type folder
+					relations
+						define viewer: [group#member, group#member with condX]
+				type group
+					relations
+						define member: [user]
+				type user
+				condition condX (x:int) {
+					x > 0
+				}`,
+			expectedOutput: `digraph {
+graph [
+rankdir=BT
+];
+
+// Node definitions.
+0 [label=folder];
+1 [label="folder#viewer"];
+2 [label="group#member"];
+3 [label=group];
+4 [label=user];
+
+// Edge definitions.
+2 -> 1 [label=direct];
+4 -> 2 [label=direct];
 }`,
 		},
 		`tuple_to_userset_recursive`: {

diff --git a/pkg/go/graph/graph_edge.go b/pkg/go/graph/graph_edge.go
@@ -12,6 +12,12 @@ const (
 	RewriteEdge  EdgeType = 1
 	TTUEdge      EdgeType = 2
 	ComputedEdge EdgeType = 3
+	// When an edge does not have cond in the model, it will have a condition with value none.
+	// This is required to differentiate when an edge need to support condition and no condition
+	// like define rel1: [user, user with condX], in this case the edge will have [none, condX]
+	// or an edge needs to support only condition like define rel1: [user with condX], the edge will have [condX]
+	// in the case the edge does not have any condition like define rel1: [user], the edge will have [none].
+	NoCond string = "none"
 )
 
 type AuthorizationModelEdge struct {
@@ -21,7 +27,15 @@ type AuthorizationModelEdge struct {
 	edgeType EdgeType
 
 	// only when edgeType == TTUEdge
-	conditionedOn string
+	tuplesetRelation string
+
+	// conditions on the edge. This is a flattened graph with dedupx edges,
+	// if you have a node with multiple edges to another node will be deduplicate and instead
+	// only one edge but with multiple conditions,
+	// define rel1: [user, user with condX]
+	// then the node rel1 will have an edge pointing to the node user and with two conditions
+	// one that will be none and another one that will be condX
+	conditions []string
 }
 
 var _ encoding.Attributer = (*AuthorizationModelEdge)(nil)
@@ -30,12 +44,12 @@ func (n *AuthorizationModelEdge) EdgeType() EdgeType {
 	return n.edgeType
 }
 
-// ConditionedOn returns the TTU relation. For example, relation
+// TuplesetRelation returns the TTU relation. For example, relation
 // define viewer: viewer from parent
 // gives the graph "document#viewer" -> "document#viewer" and the edge
 // is conditioned on "document#parent".
-func (n *AuthorizationModelEdge) ConditionedOn() string {
-	return n.conditionedOn
+func (n *AuthorizationModelEdge) TuplesetRelation() string {
+	return n.tuplesetRelation
 }
 
 func (n *AuthorizationModelEdge) Attributes() []encoding.Attribute {
@@ -55,7 +69,7 @@ func (n *AuthorizationModelEdge) Attributes() []encoding.Attribute {
 			},
 		}
 	case TTUEdge:
-		headLabelAttrValue := n.conditionedOn
+		headLabelAttrValue := n.tuplesetRelation
 		if headLabelAttrValue == "" {
 			headLabelAttrValue = "missing"
 		}

diff --git a/pkg/go/graph/graph_edge_test.go b/pkg/go/graph/graph_edge_test.go
@@ -41,5 +41,5 @@ func TestEdgeConditionedOn(t *testing.T) {
 	edge, ok := edges.Line().(*AuthorizationModelEdge)
 	require.True(t, ok)
 
-	require.Equal(t, "document#parent", edge.ConditionedOn())
+	require.Equal(t, "document#parent", edge.TuplesetRelation())
 }
diff --git a/pkg/go/graph/weighted_graph.go b/pkg/go/graph/weighted_graph.go
@@ -57,11 +57,14 @@ func (wg *WeightedAuthorizationModelGraph) AddNode(uniqueLabel, label string, no
 	wg.nodes[uniqueLabel] = &WeightedAuthorizationModelNode{uniqueLabel: uniqueLabel, label: label, nodeType: nodeType, wildcards: wildcards}
 }
 
-func (wg *WeightedAuthorizationModelGraph) AddEdge(fromID, toID string, edgeType EdgeType, condition string) {
+func (wg *WeightedAuthorizationModelGraph) AddEdge(fromID, toID string, edgeType EdgeType, tuplesetRelation string, conditions []string) {
 	wildcards := make([]string, 0)
 	fromNode := wg.nodes[fromID]
 	toNode := wg.nodes[toID]
-	edge := &WeightedAuthorizationModelEdge{from: fromNode, to: toNode, edgeType: edgeType, conditionedOn: condition, wildcards: wildcards}
+	if len(conditions) == 0 {
+		conditions = []string{NoCond}
+	}
+	edge := &WeightedAuthorizationModelEdge{from: fromNode, to: toNode, edgeType: edgeType, tuplesetRelation: tuplesetRelation, wildcards: wildcards, conditions: conditions}
 	wg.edges[fromID] = append(wg.edges[fromID], edge)
 }
 

diff --git a/pkg/go/graph/weighted_graph_builder.go b/pkg/go/graph/weighted_graph_builder.go
@@ -64,7 +64,7 @@ func (wgb *WeightedAuthorizationModelGraphBuilder) Build(model *openfgav1.Author
 			if !ok {
 				return nil, fmt.Errorf("%w: could not cast %v to AuthorizationModelNode", ErrBuildingGraph, castedEdge.To())
 			}
-			wb.AddEdge(castedFromNode.uniqueLabel, castedToNode.uniqueLabel, castedEdge.edgeType, castedEdge.conditionedOn)
+			wb.AddEdge(castedFromNode.uniqueLabel, castedToNode.uniqueLabel, castedEdge.edgeType, castedEdge.tuplesetRelation, castedEdge.conditions)
 		}
 	}