Prevent Query Rule Creation with Invalid Numeric Match Criteria

x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json

@@ -0,0 +1,32 @@
+{
+  "entsearch.put_query_ruleset": {
+    "documentation": {
+      "description": "Creates or updates a query ruleset",
+      "url": "..."
+    },
+    "stability": "stable",
+    "visibility": "public",
+    "headers": {
+      "accept": ["application/json"],
+      "content_type": ["application/json"]
+    },
+    "url": {
+      "paths": [
+        {
+          "path": "/_query_rules/ruleset/{ruleset_id}",
+          "methods": ["PUT"],
+          "parts": {
+            "ruleset_id": {
+              "type": "string",
+              "description": "The unique identifier of the ruleset"
+            }
+          }
+        }
+      ]
+    },
+    "body": {
+      "description": "The query ruleset to create or update",
+      "required": true
+    }
+  }
+} 

x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml

@@ -0,0 +1,138 @@
+---
+"Test numeric comparison rule validation":
+  - skip:
+      features: headers
+
+  - do:
+      indices.create:
+        index: test-index
+        body:
+          settings:
+            number_of_shards: 1
+            number_of_replicas: 0
+
+  - do:
+      query_rules.put_ruleset:
+        ruleset_id: numeric_validation_test
+        body:
+          rules:
+            - rule_id: valid_numeric_rule
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: price
+                  values: ["100"]
+              actions:
+                ids: ["1"]
+
+  - match: { result: "created" }
+
+  - do:
+      catch: bad_request
+      query_rules.put_ruleset:
+        ruleset_id: invalid_numeric_test
+        body:
+          rules:
+            - rule_id: invalid_numeric_rule
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: price
+                  values: ["not_a_number"]
+              actions:
+                ids: ["1"]
+
+  - match: { error.type: "x_content_parse_exception" }
+  - match: { error.reason: /.*/ }
+
+---
+"Test multiple numeric comparison rules":
+  - do:
+      query_rules.put_ruleset:
+        ruleset_id: multiple_numeric_test
+        body:
+          rules:
+            - rule_id: multiple_numeric_rule
+              type: pinned
+              criteria:
+                - type: gte
+                  metadata: price
+                  values: ["50"]
+                - type: lte
+                  metadata: price
+                  values: ["100"]
+              actions:
+                ids: ["1"]
+
+  - match: { result: "created" }
+
+---
+"Test mixed numeric and non-numeric rules":
+  - do:
+      query_rules.put_ruleset:
+        ruleset_id: mixed_rules_test
+        body:
+          rules:
+            - rule_id: mixed_rule
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: price
+                  values: ["100"]
+                - type: exact
+                  metadata: category
+                  values: ["electronics"]
+              actions:
+                ids: ["1"]
+
+  - match: { result: "created" }
+
+---
+"Test numeric comparison with multiple values":
+  - do:
+      catch: bad_request
+      query_rules.put_ruleset:
+        ruleset_id: multiple_values_test
+        body:
+          rules:
+            - rule_id: multiple_values_rule
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: price
+                  values: ["100", "not_a_number"]
+              actions:
+                ids: ["1"]
+
+  - match: { error.type: "x_content_parse_exception" }
+  - match: { error.reason: /.*/ }
+
+---
+teardown:
+  - do:
+      indices.delete:
+        index: test-index
+        ignore: 404
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: numeric_validation_test
+        ignore: 404
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: invalid_numeric_test
+        ignore: 404
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: multiple_numeric_test
+        ignore: 404
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: mixed_rules_test
+        ignore: 404
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: multiple_values_test
+        ignore: 404
+
+
+

x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java

@@ -140,7 +140,6 @@ public QueryRule(StreamInput in) throws IOException {
     }
 
     private void validate() {
-
         if (priority != null && (priority < MIN_PRIORITY || priority > MAX_PRIORITY)) {
             throw new IllegalArgumentException("Priority was " + priority + ", must be between " + MIN_PRIORITY + " and " + MAX_PRIORITY);
         }
@@ -155,6 +154,27 @@ private void validate() {
                 throw new ElasticsearchParseException(type.toString() + " query rule actions must contain only one of either ids or docs");
             }
         }
+
+        for (QueryRuleCriteria criterion : criteria) {
+            QueryRuleCriteriaType criteriaType = criterion.criteriaType();
+            if (criteriaType.isNumericComparison()) {
+                List<Object> values = criterion.criteriaValues();
+                for (Object value : values) {
+                    try {
+                        Double.parseDouble(value.toString());
+                    } catch (NumberFormatException e) {
+                        throw new IllegalArgumentException(
+                            "Numeric comparison rule type "
+                                + criteriaType
+                                + " requires numeric values, but got "
+                                + value
+                                + " for criterion "
+                                + criterion
+                        );
+                    }
+                }
+            }
+        }
     }
 
     private void validateIdOrDocAction(Object action) {

x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java

@@ -135,4 +135,8 @@ private boolean isValidForInput(Object input) {
     private static double parseDouble(Object input) {
         return (input instanceof Number) ? ((Number) input).doubleValue() : Double.parseDouble(input.toString());
     }
+
+    public boolean isNumericComparison() {
+        return this == GT || this == GTE || this == LT || this == LTE;
+    }
 }

x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java

@@ -21,24 +21,23 @@
 import org.elasticsearch.xpack.core.action.util.PageParams;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 
 import static org.elasticsearch.test.ESTestCase.generateRandomStringArray;
+import static org.elasticsearch.test.ESTestCase.randomAlphaOfLength;
 import static org.elasticsearch.test.ESTestCase.randomAlphaOfLengthBetween;
 import static org.elasticsearch.test.ESTestCase.randomBoolean;
+import static org.elasticsearch.test.ESTestCase.randomDoubleBetween;
 import static org.elasticsearch.test.ESTestCase.randomFrom;
 import static org.elasticsearch.test.ESTestCase.randomIdentifier;
 import static org.elasticsearch.test.ESTestCase.randomIntBetween;
-import static org.elasticsearch.test.ESTestCase.randomList;
 import static org.elasticsearch.test.ESTestCase.randomLongBetween;
 import static org.elasticsearch.test.ESTestCase.randomMap;
 import static org.elasticsearch.xpack.application.rules.QueryRule.MAX_PRIORITY;
 import static org.elasticsearch.xpack.application.rules.QueryRule.MIN_PRIORITY;
-import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.ALWAYS;
 
 public final class EnterpriseSearchModuleTestUtils {
 
@@ -87,18 +86,77 @@ public static Map<String, Object> randomSearchApplicationQueryParams() {
     }
 
     public static QueryRuleCriteria randomQueryRuleCriteria() {
-        // We intentionally don't allow ALWAYS criteria in this method, since we want to test parsing metadata and values
-        QueryRuleCriteriaType type = randomFrom(Arrays.stream(QueryRuleCriteriaType.values()).filter(t -> t != ALWAYS).toList());
-        return new QueryRuleCriteria(type, randomAlphaOfLengthBetween(1, 10), randomList(1, 5, () -> randomAlphaOfLengthBetween(1, 10)));
+        QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values());
+        String metadata = randomAlphaOfLength(10);
+        List<Object> values = new ArrayList<>();
+        int numValues = randomIntBetween(1, 3);
+
+        if (criteriaType.isNumericComparison()) {
+            for (int i = 0; i < numValues; i++) {
+                double value = Math.round(randomDoubleBetween(0, 1000, true) * 100.0) / 100.0;
+                values.add(String.valueOf(value));
+            }
+        } else if (criteriaType == QueryRuleCriteriaType.ALWAYS) {
+            metadata = null;
+            values = null;
+        } else {
+            for (int i = 0; i < numValues; i++) {
+                values.add(randomAlphaOfLength(5));
+            }
+        }
+
+        return new QueryRuleCriteria(criteriaType, metadata, values);
     }
 
     public static QueryRule randomQueryRule() {
-        String id = randomIdentifier();
+        if (randomBoolean()) {
+            return createNumericQueryRule();
+        } else {
+            return createNonNumericQueryRule();
+        }
+    }
+
+    private static QueryRule createNumericQueryRule() {
+        String ruleId = randomAlphaOfLength(10);
         QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
-        List<QueryRuleCriteria> criteria = List.of(randomQueryRuleCriteria());
-        Map<String, Object> actions = Map.of(randomFrom("ids", "docs"), List.of(randomAlphaOfLengthBetween(2, 10)));
-        Integer priority = randomQueryRulePriority();
-        return new QueryRule(id, type, criteria, actions, priority);
+        List<QueryRuleCriteria> criteria = new ArrayList<>();
+        int numCriteria = randomIntBetween(1, 3);
+
+        for (int i = 0; i < numCriteria; i++) {
+            QueryRuleCriteriaType criteriaType = randomFrom(
+                QueryRuleCriteriaType.GT,
+                QueryRuleCriteriaType.GTE,
+                QueryRuleCriteriaType.LT,
+                QueryRuleCriteriaType.LTE
+            );
+            double value = Math.round(randomDoubleBetween(0, 1000, true) * 100.0) / 100.0;
+            criteria.add(new QueryRuleCriteria(criteriaType, randomAlphaOfLength(10), List.of(String.valueOf(value))));
+        }
+
+        return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority());
+    }
+
+    private static QueryRule createNonNumericQueryRule() {
+        String ruleId = randomAlphaOfLength(10);
+        QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
+        List<QueryRuleCriteria> criteria = new ArrayList<>();
+        int numCriteria = randomIntBetween(1, 3);
+
+        for (int i = 0; i < numCriteria; i++) {
+            QueryRuleCriteriaType criteriaType = randomFrom(
+                QueryRuleCriteriaType.EXACT,
+                QueryRuleCriteriaType.PREFIX,
+                QueryRuleCriteriaType.CONTAINS,
+                QueryRuleCriteriaType.ALWAYS
+            );
+
+            String metadata = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : randomAlphaOfLength(10);
+            List<Object> values = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : List.of(randomAlphaOfLength(5));
+
+            criteria.add(new QueryRuleCriteria(criteriaType, metadata, values));
+        }
+
+        return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority());
     }
 
     public static Integer randomQueryRulePriority() {
@@ -119,4 +177,26 @@ public static Map<String, Object> randomMatchCriteria() {
         return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10)));
     }
 
+    public static Map<String, Object> randomQueryRuleActions() {
+        QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
+        if (type == QueryRule.QueryRuleType.PINNED) {
+            // For PINNED type, we need either 'ids' or 'docs', but not both
+            if (randomBoolean()) {
+                return Map.of("ids", List.of(randomAlphaOfLength(5)));
+            } else {
+                return Map.of("docs", List.of(Map.of("_index", randomAlphaOfLength(5), "_id", randomAlphaOfLength(5))));
+            }
+        } else if (type == QueryRule.QueryRuleType.EXCLUDE) {
+            // For EXCLUDE type, similar to PINNED
+            if (randomBoolean()) {
+                return Map.of("ids", List.of(randomAlphaOfLength(5)));
+            } else {
+                return Map.of("docs", List.of(Map.of("_index", randomAlphaOfLength(5), "_id", randomAlphaOfLength(5))));
+            }
+        } else {
+            // For other types (like BOOST), use a boost value
+            return Map.of("boost", randomDoubleBetween(0.0, 5.0, true));
+        }
+    }
+
 }