feat: Test if rule format is valid (#2795)

* feat: Test if rule format is valid Signed-off-by: Thomas Poignant <[email protected]> * chore: simplify query Signed-off-by: Thomas Poignant <[email protected]> * fix: invalid rule in a valid flag file Signed-off-by: Thomas Poignant <[email protected]> --------- Signed-off-by: Thomas Poignant <[email protected]>
thomaspoignant · Dec 13, 2024 · 473c8f3 · 473c8f3
1 parent 8ae0c6d
commit 473c8f3
Show file tree

Hide file tree

Showing 5 changed files with 176 additions and 135 deletions.
diff --git a/cmd/relayproxy/testdata/controller/config_flags.yaml b/cmd/relayproxy/testdata/controller/config_flags.yaml
@@ -1,136 +1,136 @@
 array-flag:
-    variations:
-        Default:
-            - batmanDefault
-            - supermanDefault
-            - superherosDefault
-        "False":
-            - batmanFalse
-            - supermanFalse
-            - superherosFalse
-        "True":
-            - batmanTrue
-            - supermanTrue
-            - superherosTrue
-    targeting:
-        - name: rule1
-          query: anonymous eq true
-          percentage:
-            "False": 90
-            "True": 10
-    defaultRule:
-        name: defaultRule
-        variation: Default
+  variations:
+    Default:
+      - batmanDefault
+      - supermanDefault
+      - superherosDefault
+    "False":
+      - batmanFalse
+      - supermanFalse
+      - superherosFalse
+    "True":
+      - batmanTrue
+      - supermanTrue
+      - superherosTrue
+  targeting:
+    - name: rule1
+      query: anonymous eq true
+      percentage:
+        "False": 90
+        "True": 10
+  defaultRule:
+    name: defaultRule
+    variation: Default
 disable-flag:
-    variations:
-        Default: default
-        "False": "false"
-        "True": "true"
-    targeting:
-        - name: rule1
-          query: admin eq true
-          percentage:
-            "False": 0
-            "True": 100
-    defaultRule:
-        name: defaultRule
-        variation: Default
-    disable: true
+  variations:
+    Default: default
+    "False": "false"
+    "True": "true"
+  targeting:
+    - name: rule1
+      query: admin eq true
+      percentage:
+        "False": 0
+        "True": 100
+  defaultRule:
+    name: defaultRule
+    variation: Default
+  disable: true
 flag-only-for-admin:
-    variations:
-        Default: false
-        "False": false
-        "True": true
-    targeting:
-        - name: rule1
-          query: admin eq true
-          percentage:
-            "False": 0
-            "True": 100
-    defaultRule:
-        name: defaultRule
-        variation: Default
+  variations:
+    Default: false
+    "False": false
+    "True": true
+  targeting:
+    - name: rule1
+      query: admin eq true
+      percentage:
+        "False": 0
+        "True": 100
+  defaultRule:
+    name: defaultRule
+    variation: Default
 new-admin-access:
-    variations:
-        Default: false
-        "False": false
-        "True": true
-    defaultRule:
-        name: defaultRule
-        percentage:
-            "False": 70
-            "True": 30
+  variations:
+    Default: false
+    "False": false
+    "True": true
+  defaultRule:
+    name: defaultRule
+    percentage:
+      "False": 70
+      "True": 30
 number-flag:
-    variations:
-        Default: 1
-        "False": 3
-        "True": 2
-    targeting:
-        - name: rule1
-          query: anonymous eq true
-          percentage:
-            "False": 0
-            "True": 100
-    defaultRule:
-        name: defaultRule
-        variation: Default
+  variations:
+    Default: 1
+    "False": 3
+    "True": 2
+  targeting:
+    - name: rule1
+      query: anonymous eq true
+      percentage:
+        "False": 0
+        "True": 100
+  defaultRule:
+    name: defaultRule
+    variation: Default
 targeting-key-rule:
-    variations:
-        false_var: false
-        true_var: true
-    targeting:
-        - query: targetingKey eq "specific-targeting-key"
-          variation: true_var
-    defaultRule:
-        variation: false_var
+  variations:
+    false_var: false
+    true_var: true
+  targeting:
+    - query: targetingKey eq "specific-targeting-key"
+      variation: true_var
+  defaultRule:
+    variation: false_var
 test-flag-rule-apply:
-    variations:
-        Default:
-            test: test
-        "False":
-            test3: test
-        "True":
-            test2: test
-    targeting:
-        - name: rule1
-          query: key eq "random-key"
-          percentage:
-            "False": 0
-            "True": 100
-    defaultRule:
-        name: defaultRule
-        variation: Default
+  variations:
+    Default:
+      test: test
+    "False":
+      test3: test
+    "True":
+      test2: test
+  targeting:
+    - name: rule1
+      query: key eq "random-key"
+      percentage:
+        "False": 0
+        "True": 100
+  defaultRule:
+    name: defaultRule
+    variation: Default
 test-flag-rule-apply-false:
-    variations:
-        Default:
-            test: test
-        "False":
-            test3: test
-        "True":
-            test2: test
-    targeting:
-        - name: rule1
-          query: anonymous eq true
-          percentage:
-            "False": 90
-            "True": 10
-    defaultRule:
-        name: defaultRule
-        variation: Default
+  variations:
+    Default:
+      test: test
+    "False":
+      test3: test
+    "True":
+      test2: test
+  targeting:
+    - name: rule1
+      query: anonymous eq true
+      percentage:
+        "False": 90
+        "True": 10
+  defaultRule:
+    name: defaultRule
+    variation: Default
 test-flag-rule-not-apply:
-    variations:
-        Default:
-            test: test
-        "False":
-            test3: test
-        "True":
-            test2: test
-    targeting:
-        - name: rule1
-          query: key eq \"key\"
-          percentage:
-            "False": 0
-            "True": 100
-    defaultRule:
-        name: defaultRule
-        variation: Default
+  variations:
+    Default:
+      test: test
+    "False":
+      test3: test
+    "True":
+      test2: test
+  targeting:
+    - name: rule1
+      query: key eq "key"
+      percentage:
+        "False": 0
+        "True": 100
+  defaultRule:
+    name: defaultRule
+    variation: Default
diff --git a/cmd/relayproxy/testdata/controller/flag_change/flag_change_with_config_change.json b/cmd/relayproxy/testdata/controller/flag_change/flag_change_with_config_change.json
@@ -1,5 +1,5 @@
 {
-  "hash": 2343199996,
+  "hash": 2781898380,
   "flags": {
     "array-flag": 1051133493,
     "disable-flag": 903048676,
@@ -9,6 +9,6 @@
     "targeting-key-rule": 4223863808,
     "test-flag-rule-apply": 3859871038,
     "test-flag-rule-apply-false": 1652367510,
-    "test-flag-rule-not-apply": 1389723366
+    "test-flag-rule-not-apply": 370621622
   }
-}
+}
diff --git a/cmd/relayproxy/testdata/controller/flag_change/flag_change_without_config_change.json b/cmd/relayproxy/testdata/controller/flag_change/flag_change_without_config_change.json
@@ -1,5 +1,5 @@
 {
-  "hash": 2343199996,
+  "hash": 2781898380,
   "flags": {
     "array-flag": 1051133493,
     "disable-flag": 903048676,
@@ -9,6 +9,6 @@
     "targeting-key-rule": 4223863808,
     "test-flag-rule-apply": 3859871038,
     "test-flag-rule-apply-false": 1652367510,
-    "test-flag-rule-not-apply": 1389723366
+    "test-flag-rule-not-apply": 370621622
   }
-}
+}
diff --git a/internal/flag/internal_flag_test.go b/internal/flag/internal_flag_test.go
@@ -2556,6 +2556,26 @@ func TestInternalFlag_IsValid(t *testing.T) {
 			errorMsg: "invalid progressive rollout, end variation C does not exist",
 			wantErr:  assert.Error,
 		},
+		{
+			name: "should error if rule query is not valid for the parser",
+			fields: fields{
+				Variations: &map[string]*interface{}{
+					"A": testconvert.Interface("A"),
+					"B": testconvert.Interface("B"),
+				},
+				Rules: &[]flag.Rule{
+					{
+						Query:           testconvert.String("invalid"),
+						VariationResult: testconvert.String("A"),
+					},
+				},
+				DefaultRule: &flag.Rule{
+					VariationResult: testconvert.String("A"),
+				},
+			},
+			errorMsg: "invalid query: Invalid rule",
+			wantErr:  assert.Error,
+		},
 	}
 
 	for _, tt := range tests {

diff --git a/internal/flag/rule.go b/internal/flag/rule.go
@@ -233,8 +233,8 @@ func (r *Rule) IsValid(defaultRule bool, variations map[string]*interface{}) err
 	}
 
 	// targeting without query
-	if !defaultRule && r.Query == nil {
-		return fmt.Errorf("each targeting should have a query")
+	if err := r.isQueryValid(defaultRule); err != nil {
+		return err
 	}
 
 	// Validate the percentage of the rule
@@ -284,6 +284,27 @@ func (r *Rule) IsValid(defaultRule bool, variations map[string]*interface{}) err
 	return nil
 }
 
+func (r *Rule) isQueryValid(defaultRule bool) error {
+	if defaultRule {
+		return nil
+	}
+
+	if r.Query == nil {
+		return fmt.Errorf("each targeting should have a query")
+	}
+
+	// Validate the query with the parser
+	ev, err := parser.NewEvaluator(r.GetTrimmedQuery())
+	if err != nil {
+		return err
+	}
+	_, err = ev.Process(map[string]interface{}{})
+	if err != nil {
+		return fmt.Errorf("invalid query: %w", err)
+	}
+	return nil
+}
+
 // GetTrimmedQuery is removing the break lines and return
 func (r *Rule) GetTrimmedQuery() string {
 	splitQuery := strings.Split(r.GetQuery(), "\n")