feedback from github.com//pull/422 (#423)

* feedback from github.com//pull/422

* Update pkg/go/graph/graph_builder.go

Co-authored-by: Adrian Tam <[email protected]>

---------

Co-authored-by: Adrian Tam <[email protected]>

Author: yissellokta

pkg/go/graph/graph.go

@@ -83,8 +83,7 @@ func (g *AuthorizationModelGraph) Reversed() (*AuthorizationModelGraph, error) {
 			if !ok {
 				return nil, fmt.Errorf("%w: could not cast to AuthorizationModelEdge", ErrBuildingGraph)
 			}
-			newEdge := graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.tuplesetRelation, "")
-			newEdge.conditions = casted.conditions
+			graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.tuplesetRelation, casted.conditions)
 		}
 	}
 

pkg/go/graph/graph_builder.go

@@ -111,7 +111,7 @@ func checkRewrite(graphBuilder *AuthorizationModelGraphBuilder, parentNode *Auth
 
 	// 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, "", nil)
 	for _, child := range children {
 		checkRewrite(graphBuilder, operatorNodeParent, model, child, typeDef, relation)
 	}
@@ -160,7 +160,7 @@ func parseComputed(graphBuilder *AuthorizationModelGraphBuilder, parentNode *Aut
 	if parentNode.nodeType == SpecificTypeAndRelation && newNode.nodeType == SpecificTypeAndRelation {
 		nodeType = ComputedEdge
 	}
-	graphBuilder.AddEdge(newNode, parentNode, nodeType, "", "")
+	graphBuilder.AddEdge(newNode, parentNode, nodeType, "", nil)
 }
 
 func parseTupleToUserset(graphBuilder *AuthorizationModelGraphBuilder, parentNode graph.Node, model *openfgav1.AuthorizationModel, typeDef *openfgav1.TypeDefinition, rewrite *openfgav1.TupleToUserset) {
@@ -188,8 +188,15 @@ func parseTupleToUserset(graphBuilder *AuthorizationModelGraphBuilder, parentNod
 		typeTuplesetRelation := fmt.Sprintf("%s#%s", typeDef.GetType(), tuplesetRelation)
 
 		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
+			// we don't need to do any condition update, only de-dup the edge. In case of TTU
+			// the direct relation will have the conditions
+			// for example, in the case of
+			// type group
+			//   relations
+			// 		define rel1: [user] or rel1 from parent
+			//		define parent: [group, group with condX]
+			// In the graph we only have one TTU edge from the OR node to the group#rel1 node, but there are no conditions associated to it
+			// the conditions are associated to the edge from group#parent node to the group node. This direct edge has two conditions: none and condX
 			continue
 		}
 
@@ -231,14 +238,13 @@ func (g *AuthorizationModelGraphBuilder) getNodeByLabel(uniqueLabel string) *Aut
 	return authModelNode
 }
 
-func (g *AuthorizationModelGraphBuilder) AddEdge(from, to graph.Node, edgeType EdgeType, tuplesetRelation string, condition string) *AuthorizationModelEdge {
+func (g *AuthorizationModelGraphBuilder) AddEdge(from, to graph.Node, edgeType EdgeType, tuplesetRelation string, conditions []string) *AuthorizationModelEdge {
 	if from == nil || to == nil {
 		return nil
 	}
-	if condition == "" {
-		condition = NoCond
+	if len(conditions) == 0 {
+		conditions = []string{NoCond}
 	}
-	conditions := []string{condition}
 
 	l := g.NewLine(from, to)
 	newLine := &AuthorizationModelEdge{Line: l, edgeType: edgeType, tuplesetRelation: tuplesetRelation, conditions: conditions}
@@ -267,7 +273,10 @@ func (g *AuthorizationModelGraphBuilder) upsertEdge(from, to graph.Node, edgeTyp
 		}
 	}
 
-	g.AddEdge(from, to, edgeType, tuplesetRelation, condition)
+	if condition == "" {
+		condition = NoCond
+	}
+	g.AddEdge(from, to, edgeType, tuplesetRelation, []string{condition})
 }
 
 func (g *AuthorizationModelGraphBuilder) hasEdge(from, to graph.Node, edgeType EdgeType, tuplesetRelation string) bool {

pkg/go/graph/weighted_graph_builder_test.go

@@ -519,20 +519,27 @@ func TestValidConditionalGraphModel(t *testing.T) {
 	conditions := edges[0].conditions
 	require.Empty(t, edges[0].tuplesetRelation)
 	require.Len(t, conditions, 2)
+	require.Equal(t, "none", conditions[0])
+	require.Equal(t, "condX", conditions[1])
+
 	edges, _ = graph.GetEdgesFromNode(graph.nodes["job#can_read"])
 	require.Len(t, edges, 1)
 	conditions = edges[0].conditions
 	require.Len(t, conditions, 1)
+	require.Equal(t, "none", conditions[0])
 	require.Equal(t, "job#permission", edges[0].tuplesetRelation)
 	edges, _ = graph.GetEdgesFromNode(graph.nodes["job#permission"])
 	require.Len(t, edges, 1)
 	conditions = edges[0].conditions
 	require.Len(t, conditions, 2)
+	require.Equal(t, "none", conditions[0])
+	require.Equal(t, "condX", conditions[1])
 	require.Equal(t, "", edges[0].tuplesetRelation)
 	edges, _ = graph.GetEdgesFromNode(graph.nodes["role#assignee"])
 	require.Len(t, edges, 1)
 	conditions = edges[0].conditions
 	require.Len(t, conditions, 1)
+	require.Equal(t, "none", conditions[0])
 	require.Equal(t, "", edges[0].tuplesetRelation)
 	edges, _ = graph.GetEdgesFromNode(graph.nodes["permission#member"])
 	require.Len(t, edges, 2)
@@ -547,19 +554,25 @@ func TestValidConditionalGraphModel(t *testing.T) {
 	}
 	conditions = recursiveEdge.conditions
 	require.Len(t, conditions, 2)
+	require.Equal(t, "none", conditions[0])
+	require.Equal(t, "condX", conditions[1])
 	require.Equal(t, "", recursiveEdge.tuplesetRelation)
 	conditions = userEdge.conditions
 	require.Len(t, conditions, 1)
+	require.Equal(t, "none", conditions[0])
 	require.Equal(t, "", userEdge.tuplesetRelation)
 	edges, _ = graph.GetEdgesFromNode(graph.nodes["job#owner"])
 	require.Len(t, edges, 1)
 	conditions = edges[0].conditions
 	require.Len(t, conditions, 2)
+	require.Equal(t, "none", conditions[0])
+	require.Equal(t, "condX", conditions[1])
 	require.Equal(t, "", edges[0].tuplesetRelation)
 	edges, _ = graph.GetEdgesFromNode(graph.nodes["job#can_view"])
 	require.Len(t, edges, 1)
 	conditions = edges[0].conditions
 	require.Len(t, conditions, 1)
+	require.Equal(t, "none", conditions[0])
 	require.Equal(t, "", edges[0].tuplesetRelation)
 	edges, _ = graph.GetEdgesFromNode(edges[0].to) // OR node
 	require.Len(t, edges, 2)
@@ -572,10 +585,12 @@ func TestValidConditionalGraphModel(t *testing.T) {
 	}
 	conditions = recursiveEdge.conditions
 	require.Len(t, conditions, 1)
+	require.Equal(t, "none", conditions[0])
 	require.Equal(t, "job#owner", recursiveEdge.tuplesetRelation)
 	conditions = userEdge.conditions
 	require.Len(t, conditions, 1)
 	require.Equal(t, "", userEdge.tuplesetRelation)
+	require.Equal(t, "none", conditions[0])
 
 	require.Equal(t, 2, graph.nodes["permission#assignee"].weights["user"])
 	require.Equal(t, 3, graph.nodes["job#can_read"].weights["user"])