From 51af72bff55967ed1eb43d1c931165bf96dbf705 Mon Sep 17 00:00:00 2001 From: Juan Manuel Leflet Estrada Date: Thu, 22 Feb 2024 15:01:52 +0100 Subject: [PATCH] :bug: 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 --- parser/rule_parser.go | 22 ++- parser/rule_parser_test.go | 186 +++++++++++++++++++ parser/testdata/rule-chain-2.yaml | 10 + parser/testdata/rule-chain-same-as-from.yaml | 8 + parser/testdata/rule-chain-same-as.yaml | 10 + parser/testdata/rule-chain.yaml | 2 +- 6 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 parser/testdata/rule-chain-2.yaml create mode 100644 parser/testdata/rule-chain-same-as-from.yaml create mode 100644 parser/testdata/rule-chain-same-as.yaml diff --git a/parser/rule_parser.go b/parser/rule_parser.go index a4237479..4eabba65 100644 --- a/parser/rule_parser.go +++ b/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) diff --git a/parser/rule_parser_test.go b/parser/rule_parser_test.go index 2080099f..c91920d0 100644 --- a/parser/rule_parser_test.go +++ b/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") + } + } +} diff --git a/parser/testdata/rule-chain-2.yaml b/parser/testdata/rule-chain-2.yaml new file mode 100644 index 00000000..484a6028 --- /dev/null +++ b/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 diff --git a/parser/testdata/rule-chain-same-as-from.yaml b/parser/testdata/rule-chain-same-as-from.yaml new file mode 100644 index 00000000..3457d970 --- /dev/null +++ b/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 diff --git a/parser/testdata/rule-chain-same-as.yaml b/parser/testdata/rule-chain-same-as.yaml new file mode 100644 index 00000000..c4b1742a --- /dev/null +++ b/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 diff --git a/parser/testdata/rule-chain.yaml b/parser/testdata/rule-chain.yaml index c62b0f2f..cb662efb 100644 --- a/parser/testdata/rule-chain.yaml +++ b/parser/testdata/rule-chain.yaml @@ -8,4 +8,4 @@ ignore: true not: true - builtin.file: "*.json" - as: go-files \ No newline at end of file + as: go-files-2 \ No newline at end of file