✨ Chains are now automatically sorted before being executed (#267)

closes #226 --------- Signed-off-by: Fabian von Feilitzsch <[email protected]>
konveyor · Jul 20, 2023 · d5121b9 · d5121b9
1 parent ba6d8da
commit d5121b9
Show file tree

Hide file tree

Showing 5 changed files with 352 additions and 3 deletions.
diff --git a/demo-output.yaml b/demo-output.yaml
@@ -272,6 +272,48 @@
           tags:
           - Golang
       extras: []
+    singleton-sessionbean-00001:
+      description: ""
+      category: potential
+      incidents:
+      - uri: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+        message: condition entries should evaluate out of order
+        codeSnip: " 1  package com.example.apps;\n 2  \n 3  import javax.ejb.SessionBean;\n 4  import javax.ejb.Singleton;\n 5  \n 6  @Singleton\n 7  public class Bean implements SessionBean {\n 8  }\n"
+        lineNumber: 5
+        variables:
+          file: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+          kind: Class
+          name: Singleton
+      - uri: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+        message: condition entries should evaluate out of order
+        codeSnip: " 1  package com.example.apps;\n 2  \n 3  import javax.ejb.SessionBean;\n 4  import javax.ejb.Singleton;\n 5  \n 6  @Singleton\n 7  public class Bean implements SessionBean {\n 8  }\n"
+        lineNumber: 6
+        variables:
+          file: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+          kind: Class
+          name: Bean
+      extras: []
+    singleton-sessionbean-00002:
+      description: ""
+      category: potential
+      incidents:
+      - uri: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+        message: condition entries should evaluate in order
+        codeSnip: " 1  package com.example.apps;\n 2  \n 3  import javax.ejb.SessionBean;\n 4  import javax.ejb.Singleton;\n 5  \n 6  @Singleton\n 7  public class Bean implements SessionBean {\n 8  }\n"
+        lineNumber: 5
+        variables:
+          file: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+          kind: Class
+          name: Singleton
+      - uri: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+        message: condition entries should evaluate in order
+        codeSnip: " 1  package com.example.apps;\n 2  \n 3  import javax.ejb.SessionBean;\n 4  import javax.ejb.Singleton;\n 5  \n 6  @Singleton\n 7  public class Bean implements SessionBean {\n 8  }\n"
+        lineNumber: 6
+        variables:
+          file: file:///analyzer-lsp/examples/java/src/main/java/com/example/apps/Singleton.java
+          kind: Class
+          name: Bean
+      extras: []
     tech-tag-001:
       description: ""
       category: potential

diff --git a/engine/conditions.go b/engine/conditions.go
@@ -151,7 +151,8 @@ func (a AndCondition) Evaluate(ctx context.Context, log logr.Logger, condCtx Con
 		Incidents:       []IncidentContext{},
 		TemplateContext: map[string]interface{}{},
 	}
-	for _, c := range a.Conditions {
+	conditions := sortConditionEntries(a.Conditions)
+	for _, c := range conditions {
 		if _, ok := condCtx.Template[c.From]; !ok && c.From != "" {
 			// Short circut w/ error here
 			// TODO: determine if this is the right thing, I am assume the full rule should fail here
@@ -206,7 +207,8 @@ func (o OrCondition) Evaluate(ctx context.Context, log logr.Logger, condCtx Cond
 		Incidents:       []IncidentContext{},
 		TemplateContext: map[string]interface{}{},
 	}
-	for _, c := range o.Conditions {
+	conditions := sortConditionEntries(o.Conditions)
+	for _, c := range conditions {
 		if _, ok := condCtx.Template[c.From]; !ok && c.From != "" {
 			// Short circut w/ error here
 			// TODO: determine if this is the right thing, I am assume the full rule should fail here
@@ -268,6 +270,28 @@ func incidentsToFilepaths(incident []IncidentContext) []string {
 	return filepaths
 }
 
+func sortConditionEntries(entries []ConditionEntry) []ConditionEntry {
+	sorted := []ConditionEntry{}
+	for _, e := range entries {
+		// entries without chaining or that begin a chain come first
+		if e.From == "" {
+			sorted = append(sorted, gatherChain(e, entries)...)
+		}
+	}
+
+	return sorted
+}
+
+func gatherChain(start ConditionEntry, entries []ConditionEntry) []ConditionEntry {
+	chain := []ConditionEntry{start}
+	for _, d := range entries {
+		if start.As == d.From && start.As != "" {
+			chain = append(chain, gatherChain(d, entries)...)
+		}
+	}
+	return chain
+}
+
 // Chain Templates are used by rules and providers to pass context around during rule execution.
 type ChainTemplate struct {
 	Filepaths []string               `yaml:"filepaths"`

diff --git a/engine/conditions_test.go b/engine/conditions_test.go
@@ -0,0 +1,250 @@
+package engine
+
+import (
+	"reflect"
+	"testing"
+)
+
+func Test_sortConditionEntries(t *testing.T) {
+	tests := []struct {
+		title       string
+		entries     []ConditionEntry
+		expected    []ConditionEntry
+		shouldError bool
+	}{
+		{
+			title: "correctly sorted conditions should stay sorted",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					As:   "b",
+					From: "a",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					As:   "b",
+					From: "a",
+				},
+			},
+		}, {
+			title: "incorrectly sorted conditions should be sorted",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					As:   "b",
+					From: "a",
+				},
+				ConditionEntry{
+					As: "a",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					As:   "b",
+					From: "a",
+				},
+			},
+		}, {
+			title: "incorrectly sorted conditions that branch should be sorted",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					As:   "b",
+					From: "a",
+				},
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					As:   "c",
+					From: "b",
+				},
+				ConditionEntry{
+					As:   "e",
+					From: "d",
+				},
+				ConditionEntry{
+					As:   "d",
+					From: "b",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					As:   "b",
+					From: "a",
+				},
+				ConditionEntry{
+					As:   "c",
+					From: "b",
+				},
+				ConditionEntry{
+					As:   "d",
+					From: "b",
+				},
+				ConditionEntry{
+					As:   "e",
+					From: "d",
+				},
+			},
+		}, {
+			title: "longer chains should sort properly",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					From: "e",
+					As:   "f",
+				},
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					From: "d",
+					As:   "e",
+				},
+				ConditionEntry{
+					From: "a",
+					As:   "b",
+				},
+				ConditionEntry{
+					From: "b",
+					As:   "c",
+				},
+				ConditionEntry{
+					From: "c",
+					As:   "d",
+				},
+				ConditionEntry{
+					From: "f",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					From: "a",
+					As:   "b",
+				},
+				ConditionEntry{
+					From: "b",
+					As:   "c",
+				},
+				ConditionEntry{
+					From: "c",
+					As:   "d",
+				},
+				ConditionEntry{
+					From: "d",
+					As:   "e",
+				},
+				ConditionEntry{
+					From: "e",
+					As:   "f",
+				},
+				ConditionEntry{
+					From: "f",
+				},
+			},
+		}, {
+			title: "completely reversed chains should sort properly",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					From: "c",
+				},
+				ConditionEntry{
+					From: "b",
+					As:   "c",
+				},
+				ConditionEntry{
+					From: "a",
+					As:   "b",
+				},
+				ConditionEntry{
+					As: "a",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					From: "a",
+					As:   "b",
+				},
+				ConditionEntry{
+					From: "b",
+					As:   "c",
+				},
+				ConditionEntry{
+					From: "c",
+				},
+			},
+		}, {
+			title: "unused As should not cause error",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					From: "c",
+					As:   "d",
+				},
+				ConditionEntry{
+					From: "b",
+					As:   "c",
+				},
+				ConditionEntry{
+					From: "a",
+					As:   "b",
+				},
+				ConditionEntry{
+					As: "a",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+				ConditionEntry{
+					From: "a",
+					As:   "b",
+				},
+				ConditionEntry{
+					From: "b",
+					As:   "c",
+				},
+				ConditionEntry{
+					From: "c",
+					As:   "d",
+				},
+			},
+		}, {
+			title: "length 1 lists should not cause error",
+			entries: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+			},
+			expected: []ConditionEntry{
+				ConditionEntry{
+					As: "a",
+				},
+			},
+		}}
+	for _, tt := range tests {
+		t.Run(tt.title, func(t *testing.T) {
+			sorted := sortConditionEntries(tt.entries)
+
+			if !reflect.DeepEqual(sorted, tt.expected) {
+				t.Errorf("expected '%+v', got '%+v'", tt.expected, sorted)
+				return
+			}
+		})
+	}
+}
diff --git a/examples/java/src/main/java/com/example/apps/Singleton.java b/examples/java/src/main/java/com/example/apps/Singleton.java
@@ -0,0 +1,8 @@
+package com.example.apps;
+
+import javax.ejb.SessionBean;
+import javax.ejb.Singleton;
+
+@Singleton
+public class Bean implements SessionBean {
+}
diff --git a/rule-example.yaml b/rule-example.yaml
@@ -138,6 +138,32 @@
     java.referenced:
       location: METHOD_CALL
       pattern: com.example.apps.GenericClass.get
+- message: condition entries should evaluate out of order
+  ruleID: singleton-sessionbean-00001
+  when:
+    or:
+    - as: sessionbean
+      from: singleton
+      java.referenced:
+        location: IMPLEMENTS_TYPE
+        pattern: javax.ejb.SessionBean
+    - as: singleton
+      java.referenced:
+        location: ANNOTATION
+        pattern: javax.ejb.Singleton
+- message: condition entries should evaluate in order
+  ruleID: singleton-sessionbean-00002
+  when:
+    or:
+    - as: singleton
+      java.referenced:
+        location: ANNOTATION
+        pattern: javax.ejb.Singleton
+    - as: sessionbean
+      from: singleton
+      java.referenced:
+        location: IMPLEMENTS_TYPE
+        pattern: javax.ejb.SessionBean
 - message: "error test"
   ruleID: error-rule-001
   when:
@@ -157,4 +183,3 @@
     builtin.filecontent:
       pattern: "^FROM.*openjdk-11.*"
       filePattern: "Dockerfile"
-