change to return the actual nodes that have loop

pkg/go/graph/graph.go

@@ -2,6 +2,11 @@ package graph
 
 import (
  "errors"
+ "reflect"
+ "slices"
+ "sort"
+
+ "github.com/openfga/language/pkg/go/utils"
 
  "gonum.org/v1/gonum/graph"
  "gonum.org/v1/gonum/graph/encoding"
@@ -41,14 +46,59 @@ func (g *AuthorizationModelGraph) GetDOT() string {
  return string(dotRepresentation)
 }
 
+func sortAndRemoveDuplicateAndAlgebraic(orig [][]string) [][]string {
+ newSlices := make(utils.SlicesOfSlices, 0, len(orig))
+ for _, slice := range orig {
+ newSlice := make([]string, 0, len(slice))
+ for _, item := range slice {
+ if item != union && item != intersection && item != exclusion && !slices.Contains(newSlice, item) {
+ newSlice = append(newSlice, item)
+ }
+ }
+ sort.Strings(newSlice)
+ if !slicesOfSliceContains(newSlices, newSlice) {
+ newSlices = append(newSlices, newSlice)
+ }
+ }
+ // now, sort the slices according to size
+ sort.Sort(newSlices)
+
+ return newSlices
+}
+
 // CycleInformation encapsulates whether the graph has cycles.
 type CycleInformation struct {
- // If hasCyclesAtCompileTime is true, we should block this model from ever being written.
+ // If hasCyclesAtCompileTime is non-empty, we should block this model from ever being written.
  // This is because we are trying to perform a Check on it will cause a stack overflow no matter what the tuples are.
- hasCyclesAtCompileTime bool
+ hasCyclesAtCompileTime [][]string
 
- // If canHaveCyclesAtRuntime is true, there could exist tuples that introduce a cycle.
- canHaveCyclesAtRuntime bool
+ // If canHaveCyclesAtRuntime is non-empty, there could exist tuples that introduce a cycle.
+ canHaveCyclesAtRuntime [][]string
+}
+
+// slicesOfSliceContains returns true if the newSlice is deeply equal to any of the origSlices.
+func slicesOfSliceContains(origSlices [][]string, newSlice []string) bool {
+ for _, origSlice := range origSlices {
+ if reflect.DeepEqual(newSlice, origSlice) {
+ return true
+ }
+ }
+
+ return false
+}
+
+// SortedHasCyclesAtCompileTime returns a sorted HasCyclesAtCompileTime which removed algebraic operation (such as union/intersection/exclusion)
+// The []string are sorted by length. If []string has the same length, it will return if the first/second/third/.. item is smallest
+// Within each []string, it is sorted by alphabet. In addition, the duplicate node is removed.
+func (c *CycleInformation) SortedHasCyclesAtCompileTime() [][]string {
+ return sortAndRemoveDuplicateAndAlgebraic(c.hasCyclesAtCompileTime)
+}
+
+// SortedCanHaveCyclesAtRuntime returns a sorted HasCyclesAtCompileTime which removed algebraic operation (such as union/intersection/exclusion)
+// The []string are sorted by length. If []string has the same length, it will return if the first/second/third/.. item is smallest
+// Within each []string, it is sorted by alphabet. In addition, the duplicate node is removed.
+func (c *CycleInformation) SortedCanHaveCyclesAtRuntime() [][]string {
+ return sortAndRemoveDuplicateAndAlgebraic(c.canHaveCyclesAtRuntime)
 }
 
 func (g *AuthorizationModelGraph) nodeListHasNonComputedEdge(nodeList []graph.Node) bool {
@@ -67,24 +117,36 @@ func (g *AuthorizationModelGraph) nodeListHasNonComputedEdge(nodeList []graph.No
  return false
 }
 
+func nodeListIdentifier(nodeList []graph.Node) []string {
+ labels := make([]string, 0, len(nodeList))
+ for _, node := range nodeList {
+ auth, ok := node.(*AuthorizationModelNode)
+ if ok {
+ labels = append(labels, auth.label)
+ }
+ }
+
+ return labels
+}
+
 func (g *AuthorizationModelGraph) GetCycles() CycleInformation {
- hasCyclesAtCompileTime := false
- hasCyclesAtRuntime := false
+ var nodesWithCyclesAtCompileTime [][]string
+ var nodesWithCyclesAtRuntime [][]string
 
  // TODO: investigate whether len(1) should be identified as cycle
 
  nodes := topo.DirectedCyclesIn(g)
  for _, nodeList := range nodes {
  if g.nodeListHasNonComputedEdge(nodeList) {
- hasCyclesAtRuntime = true
+ nodesWithCyclesAtRuntime = append(nodesWithCyclesAtRuntime, nodeListIdentifier(nodeList))
  } else {
- hasCyclesAtCompileTime = true
+ nodesWithCyclesAtCompileTime = append(nodesWithCyclesAtCompileTime, nodeListIdentifier(nodeList))
  }
  }
 
  return CycleInformation{
- hasCyclesAtCompileTime: hasCyclesAtCompileTime,
- canHaveCyclesAtRuntime: hasCyclesAtRuntime,
+ hasCyclesAtCompileTime: nodesWithCyclesAtCompileTime,
+ canHaveCyclesAtRuntime: nodesWithCyclesAtRuntime,
  }
 }
 

pkg/go/graph/graph_builder.go

@@ -11,6 +11,12 @@ import (
  "gonum.org/v1/gonum/graph/multi"
 )
 
+const (
+ union = "union"
+ intersection = "intersection"
+ exclusion = "exclusion"
+)
+
 type AuthorizationModelGraphBuilder struct {
  graph.DirectedMultigraphBuilder
 
@@ -92,15 +98,15 @@ func checkRewrite(graphBuilder *AuthorizationModelGraphBuilder, parentNode *Auth
 
  return
  case *openfgav1.Userset_Union:
- operator = "union"
+ operator = union
  children = rw.Union.GetChild()
 
  case *openfgav1.Userset_Intersection:
- operator = "intersection"
+ operator = intersection
  children = rw.Intersection.GetChild()
 
  case *openfgav1.Userset_Difference:
- operator = "exclusion"
+ operator = exclusion
  children = []*openfgav1.Userset{
  rw.Difference.GetBase(),
  rw.Difference.GetSubtract(),

pkg/go/graph/graph_builder_test.go

@@ -227,8 +227,8 @@ rankdir=BT
 3 -> 2 [style=dashed];
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: true,
- canHaveCyclesAtRuntime: false,
+ hasCyclesAtCompileTime: [][]string{{"folder#x", "folder#y", "folder#z", "folder#x"}},
+ canHaveCyclesAtRuntime: nil,
  },
  },
  `computed_relation_with_size_two`: {
@@ -254,8 +254,8 @@ rankdir=BT
 2 -> 1 [style=dashed];
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: true,
- canHaveCyclesAtRuntime: false,
+ hasCyclesAtCompileTime: [][]string{{"folder#x", "folder#y", "folder#x"}},
+ canHaveCyclesAtRuntime: nil,
  },
  },
  `tuple_to_userset_one_related_type`: {
@@ -317,8 +317,8 @@ rankdir=BT
 4 -> 3 [label=direct];
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: false,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: nil,
+ canHaveCyclesAtRuntime: [][]string{{"folder#viewer"}},
  },
  },
  `tuple_to_userset_two_related_types`: {
@@ -631,8 +631,8 @@ rankdir=BT
 7 -> 6;
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: false,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: nil,
+ canHaveCyclesAtRuntime: [][]string{{"folder#a", "folder#b", "folder#c"}},
  },
  },
  `multigraph`: {
@@ -820,8 +820,8 @@ rankdir=BT
 9 -> 4 [style=dashed];
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: true,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: [][]string{{"folder#x", "folder#y"}},
+ canHaveCyclesAtRuntime: [][]string{{"folder#a", "folder#b", "folder#c"}},
  },
  },
  `potential_cycle_or_but_not`: {
@@ -861,8 +861,8 @@ rankdir=BT
 7 -> 6;
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: false,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: nil,
+ canHaveCyclesAtRuntime: [][]string{{"resource#x", "resource#y", "resource#z"}},
  },
  },
  `potential_cycle_four_union`: {
@@ -916,8 +916,20 @@ rankdir=BT
 9 -> 6;
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: false,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: nil,
+ canHaveCyclesAtRuntime: [][]string{
+ {"group#member", "group#memberA"},
+ {"group#member", "group#memberB"},
+ {"group#member", "group#memberC"},
+ {"group#memberA", "group#memberB"},
+ {"group#memberA", "group#memberC"},
+ {"group#memberB", "group#memberC"},
+ {"group#member", "group#memberA", "group#memberB"},
+ {"group#member", "group#memberA", "group#memberC"},
+ {"group#member", "group#memberB", "group#memberC"},
+ {"group#memberA", "group#memberB", "group#memberC"},
+ {"group#member", "group#memberA", "group#memberB", "group#memberC"},
+ },
  },
  },
  `potential_cycle_four_union_with_one_member_no_union`: {
@@ -966,8 +978,13 @@ rankdir=BT
 8 -> 5;
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: false,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: nil,
+ canHaveCyclesAtRuntime: [][]string{
+ {"account#admin", "account#member"},
+ {"account#admin", "account#super_admin"},
+ {"account#member", "account#super_admin"},
+ {"account#admin", "account#member", "account#super_admin"},
+ },
  },
  },
  `intersection`: {
@@ -1014,8 +1031,13 @@ rankdir=BT
 8 -> 3 [label=direct];
 }`,
  cycleInformation: CycleInformation{
- hasCyclesAtCompileTime: false,
- canHaveCyclesAtRuntime: true,
+ hasCyclesAtCompileTime: nil,
+ canHaveCyclesAtRuntime: [][]string{
+ {"document#action1", "document#action2"},
+ {"document#action1", "document#action3"},
+ {"document#action2", "document#action3"},
+ {"document#action1", "document#action2", "document#action3"},
+ },
  },
  },
  }
@@ -1035,7 +1057,17 @@ rankdir=BT
  diff := cmp.Diff(expectedSorted, actualSorted)
 
  require.Empty(t, diff, "expected %s\ngot\n%s", testCase.expectedOutput, actualDOT)
- require.Equal(t, testCase.cycleInformation, graph.GetCycles())
+ cycleInfo := graph.GetCycles()
+ if len(testCase.cycleInformation.canHaveCyclesAtRuntime) > 0 {
+ require.Equal(t, testCase.cycleInformation.SortedCanHaveCyclesAtRuntime(), cycleInfo.SortedCanHaveCyclesAtRuntime())
+ } else {
+ require.Empty(t, cycleInfo.SortedCanHaveCyclesAtRuntime())
+ }
+ if len(testCase.cycleInformation.hasCyclesAtCompileTime) > 0 {
+ require.Equal(t, testCase.cycleInformation.SortedHasCyclesAtCompileTime(), cycleInfo.SortedHasCyclesAtCompileTime())
+ } else {
+ require.Empty(t, cycleInfo.SortedHasCyclesAtCompileTime())
+ }
  })
  }
 }
@@ -1114,3 +1146,90 @@ func getSorted(input string) string {
 
  return strings.Join(lines, "\n")
 }
+
+func TestSortAndRemoveDuplicateAndAlgebraic(t *testing.T) {
+ t.Parallel()
+ tests := []struct {
+ name string
+ input [][]string
+ expected [][]string
+ }{
+ {
+ name: "empty",
+ input: [][]string{},
+ expected: [][]string{},
+ },
+ {
+ name: "single_line_with_no_duplicate",
+ input: [][]string{
+ {"c", "b", "a"},
+ },
+ expected: [][]string{
+ {"a", "b", "c"},
+ },
+ },
+ {
+ name: "single_line_with_duplicate_union_intersection_exclusion",
+ input: [][]string{
+ {"c", "exclusion", "b", "union", "c", "a", "intersection", "a", "a"},
+ },
+ expected: [][]string{
+ {"a", "b", "c"},
+ },
+ },
+ {
+ name: "multiple_line_with_no_duplicate",
+ input: [][]string{
+ {"y", "x", "z"},
+ {"c", "b", "a"},
+ {"a", "c", "d", "b"},
+ },
+ expected: [][]string{
+ {"a", "b", "c"},
+ {"x", "y", "z"},
+ {"a", "b", "c", "d"},
+ },
+ },
+ {
+ name: "multiple_line_with_duplicate_items",
+ input: [][]string{
+ {"y", "x", "x", "z"},
+ {"c", "b", "a", "a"},
+ {"a", "c", "d", "b"},
+ },
+ expected: [][]string{
+ {"a", "b", "c"},
+ {"x", "y", "z"},
+ {"a", "b", "c", "d"},
+ },
+ },
+ {
+ name: "multiple_line_difference_last_item",
+ input: [][]string{
+ {"c", "b", "a", "e"},
+ {"a", "c", "d", "b"},
+ },
+ expected: [][]string{
+ {"a", "b", "c", "d"},
+ {"a", "b", "c", "e"},
+ },
+ },
+ {
+ name: "duplicate_lines",
+ input: [][]string{
+ {"c", "b", "a", "d"},
+ {"a", "c", "d", "b"},
+ },
+ expected: [][]string{
+ {"a", "b", "c", "d"},
+ },
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ t.Parallel()
+ output := sortAndRemoveDuplicateAndAlgebraic(tt.input)
+ require.Equal(t, tt.expected, output)
+ })
+ }
+}