🐛 Fix duplicated conditions when AS exists (#515)

The parser was generating a duplicate entry in the list of conditions.

---------

Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>

parser/rule_parser.go

@@ -7,13 +7,12 @@ import (
 	"regexp"
 	"strings"
 
-	"gopkg.in/yaml.v2"
-
 	"github.com/go-logr/logr"
 	"github.com/konveyor/analyzer-lsp/engine"
 	"github.com/konveyor/analyzer-lsp/engine/labels"
 	"github.com/konveyor/analyzer-lsp/output/v1/konveyor"
 	"github.com/konveyor/analyzer-lsp/provider"
+	"gopkg.in/yaml.v2"
 )
 
 const (
@@ -555,6 +554,7 @@ func (r *RuleParser) getConditions(conditionsInterface []interface{}) ([]engine.
 	conditions := []engine.ConditionEntry{}
 	providers := map[string]provider.InternalProviderClient{}
 	chainNameToIndex := map[string]int{}
+	asFound := []string{}
 	for _, conditionInterface := range conditionsInterface {
 		// get map from interface
 		conditionMap, ok := conditionInterface.(map[interface{}]interface{})
@@ -684,15 +684,25 @@ func (r *RuleParser) getConditions(conditionsInterface []interface{}) ([]engine.
 				}
 				providers[providerKey] = provider
 			}
-			if ce.As != "" {
+			if ce.From != "" && ce.As != "" && ce.From == ce.As {
+				return nil, nil, fmt.Errorf("condition cannot have the same value for fields 'from' and 'as'")
+			} else if ce.As != "" {
+				for _, as := range asFound {
+					if as == ce.As {
+						return nil, nil, fmt.Errorf("condition cannot have multiple 'as' fields with the same name")
+					}
+				}
+				asFound = append(asFound, ce.As)
+
 				index, ok := chainNameToIndex[ce.As]
 				if !ok {
 					//prepend
 					conditions = append([]engine.ConditionEntry{ce}, conditions...)
+				} else {
+					//insert
+					conditions = append(conditions[:index+1], conditions[index:]...)
+					conditions[index] = ce
 				}
-				//insert
-				conditions = append(conditions[:index+1], conditions[index:]...)
-				conditions[index] = ce
 			} else if ce.From != "" && ce.As == "" {
 				chainNameToIndex[ce.From] = len(conditions)
 				conditions = append(conditions, ce)

parser/rule_parser_test.go

@@ -146,6 +146,7 @@ func TestLoadRules(t *testing.T) {
 									},
 								},
 							},
+							When: engine.ConditionEntry{},
 						},
 					},
 				},
@@ -183,6 +184,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -256,6 +258,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoAndJsonFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -293,6 +296,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoOrJsonFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -330,6 +334,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoOrJsonFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -406,6 +411,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoOrJsonFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -461,6 +467,7 @@ func TestLoadRules(t *testing.T) {
 								Description: "",
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoOrJsonFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -506,6 +513,7 @@ func TestLoadRules(t *testing.T) {
 								},
 								Tag: []string{"test"},
 							},
+							When: engine.ConditionEntry{},
 						},
 					},
 				},
@@ -557,6 +565,7 @@ func TestLoadRules(t *testing.T) {
 							Perform: engine.Perform{
 								Tag: []string{"test"},
 							},
+							When: engine.ConditionEntry{},
 						},
 					},
 				},
@@ -587,6 +596,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -598,6 +608,7 @@ func TestLoadRules(t *testing.T) {
 								Category:    &konveyor.Potential,
 							},
 							Perform: engine.Perform{Message: engine.Message{Text: &allGoFiles, Links: []konveyor.Link{}}},
+							When:    engine.ConditionEntry{},
 						},
 					},
 				},
@@ -668,6 +679,7 @@ func TestLoadRules(t *testing.T) {
 									},
 								},
 							},
+							When: engine.ConditionEntry{},
 						},
 					},
 				},
@@ -680,6 +692,137 @@ func TestLoadRules(t *testing.T) {
 				},
 			},
 		},
+		{
+			Name:         "chaining should yield the same number of conditions as the rule",
+			testFileName: "rule-chain-2.yaml",
+			providerNameClient: map[string]provider.InternalProviderClient{
+				"builtin": testProvider{
+					caps: []provider.Capability{{
+						Name: "filecontent",
+					}},
+				},
+			},
+			ExpectedProvider: map[string]provider.InternalProviderClient{
+				"builtin": testProvider{
+					caps: []provider.Capability{{
+						Name: "filecontent",
+					}},
+				},
+			},
+			ExpectedRuleSet: map[string]engine.RuleSet{
+				"konveyor-analysis": {
+					Rules: []engine.Rule{
+						{
+							RuleMeta: engine.RuleMeta{
+								RuleID:      "chaining-rule",
+								Description: "",
+								Category:    &konveyor.Potential,
+							},
+							Perform: engine.Perform{
+								Message: engine.Message{
+									Text:  &allGoFiles,
+									Links: []konveyor.Link{},
+								},
+							},
+							When: engine.AndCondition{
+								Conditions: []engine.ConditionEntry{
+									{
+										From:      "",
+										As:        "file",
+										Ignorable: false,
+										Not:       false,
+									},
+									{
+										From:      "file",
+										As:        "",
+										Ignorable: false,
+										Not:       false,
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			Name:         "no two conditions should have the same 'as' field within the same block",
+			testFileName: "rule-chain-same-as.yaml",
+			ShouldErr:    true,
+			ErrorMessage: "condition cannot have multiple 'as' fields with the same name",
+			providerNameClient: map[string]provider.InternalProviderClient{
+				"builtin": testProvider{
+					caps: []provider.Capability{{
+						Name: "filecontent",
+					}},
+				},
+			},
+			ExpectedProvider: map[string]provider.InternalProviderClient{
+				"builtin": testProvider{
+					caps: []provider.Capability{{
+						Name: "filecontent",
+					}},
+				},
+			},
+			ExpectedRuleSet: map[string]engine.RuleSet{
+				"konveyor-analysis": {
+					Rules: []engine.Rule{
+						{
+							RuleMeta: engine.RuleMeta{
+								RuleID:      "chaining-rule",
+								Description: "",
+								Category:    &konveyor.Potential,
+							},
+							Perform: engine.Perform{
+								Message: engine.Message{
+									Text:  &allGoFiles,
+									Links: []konveyor.Link{},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			Name:         "a condition should not have the same 'as' and 'from' fields",
+			testFileName: "rule-chain-same-as-from.yaml",
+			ShouldErr:    true,
+			ErrorMessage: "condition cannot have the same value for fields 'from' and 'as'",
+			providerNameClient: map[string]provider.InternalProviderClient{
+				"builtin": testProvider{
+					caps: []provider.Capability{{
+						Name: "filecontent",
+					}},
+				},
+			},
+			ExpectedProvider: map[string]provider.InternalProviderClient{
+				"builtin": testProvider{
+					caps: []provider.Capability{{
+						Name: "filecontent",
+					}},
+				},
+			},
+			ExpectedRuleSet: map[string]engine.RuleSet{
+				"konveyor-analysis": {
+					Rules: []engine.Rule{
+						{
+							RuleMeta: engine.RuleMeta{
+								RuleID:      "chaining-rule",
+								Description: "",
+								Category:    &konveyor.Potential,
+							},
+							Perform: engine.Perform{
+								Message: engine.Message{
+									Text:  &allGoFiles,
+									Links: []konveyor.Link{},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -740,6 +883,7 @@ func TestLoadRules(t *testing.T) {
 								foundRule = true
 							}
 						}
+						compareWhens(expectedRule.When, rule.When, t)
 					}
 					if !foundRule {
 						t.Errorf("not have matching rule go: %#v, expected rules: %#v", rule, expectedSet.Rules)
@@ -750,3 +894,45 @@ func TestLoadRules(t *testing.T) {
 		})
 	}
 }
+
+func compareWhens(w1 engine.Conditional, w2 engine.Conditional, t *testing.T) {
+	if (w1 == nil && w2 != nil) || (w1 != nil && w2 == nil) {
+		t.Errorf("rulesets did not have matching when field")
+	}
+	if and1, ok := w1.(engine.AndCondition); ok {
+		and2, ok := w2.(engine.AndCondition)
+		if !ok {
+			t.Errorf("rulesets did not have matching when field")
+		}
+		compareConditions(and1.Conditions, and2.Conditions, t)
+	} else if or1, ok := w1.(engine.OrCondition); ok {
+		or2, ok := w2.(engine.OrCondition)
+		if !ok {
+			t.Errorf("rulesets did not have matching when field")
+		}
+		compareConditions(or1.Conditions, or2.Conditions, t)
+	}
+
+}
+
+func compareConditions(cs1 []engine.ConditionEntry, cs2 []engine.ConditionEntry, t *testing.T) {
+	if len(cs1) != len(cs2) {
+		t.Errorf("rulesets did not have the same number of conditions")
+	}
+	for i := 0; i < len(cs1); i++ {
+		c1 := cs1[i]
+		c2 := cs2[i]
+		if c1.As != c2.As {
+			t.Errorf("rulesets did not have the same As field")
+		}
+		if c1.From != c2.From {
+			t.Errorf("rulesets did not have the same From field")
+		}
+		if c1.Ignorable != c2.Ignorable {
+			t.Errorf("rulesets did not have the same Ignorable field")
+		}
+		if c1.Not != c2.Not {
+			t.Errorf("rulesets did not have the same Not field")
+		}
+	}
+}

parser/testdata/rule-chain-2.yaml

@@ -0,0 +1,10 @@
+- message: "all go files"
+  ruleID: chaining-rule
+  when:
+    and:
+      - builtin.filecontent:
+          pattern: spring\.datasource
+        as: file
+      - builtin.filecontent:
+          pattern: value
+        from: file

parser/testdata/rule-chain-same-as-from.yaml

@@ -0,0 +1,8 @@
+- message: "all go files"
+  ruleID: chaining-rule
+  when:
+    and:
+      - builtin.filecontent:
+          pattern: spring\.datasource
+        as: file
+        from: file

parser/testdata/rule-chain-same-as.yaml

@@ -0,0 +1,10 @@
+- message: "all go files"
+  ruleID: chaining-rule
+  when:
+    and:
+      - builtin.filecontent:
+          pattern: spring\.datasource
+        as: file
+      - builtin.filecontent:
+          pattern: value
+        as: file

parser/testdata/rule-chain.yaml

@@ -8,4 +8,4 @@
       ignore: true
       not: true
     - builtin.file: "*.json"
-      as: go-files
+      as: go-files-2