[Query Rules] Fix bug where combining the same metadata with text/num…

…eric values leads to error (elastic#102891) * Fix issue where query rule criteria with matching metadata but different types returns error * Update docs/changelog/102891.yaml
carlosdelest · Dec 4, 2023 · 84dad02 · 84dad02
1 parent d3fefde
commit 84dad02
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 4 deletions.
diff --git a/docs/changelog/102891.yaml b/docs/changelog/102891.yaml
@@ -0,0 +1,7 @@
+pr: 102891
+summary: "[Query Rules] Fix bug where combining the same metadata with text/numeric\
+  \ values leads to error"
+area: Application
+type: bug
+issues:
+ - 102827
diff --git a/...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/260_rule_query_search.yml b/...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/260_rule_query_search.yml
@@ -194,4 +194,46 @@ setup:
   - match: { hits.hits.0._id: 'doc2' }
   - match: { hits.hits.1._id: 'doc3' }
 
+---
+"Perform a rule query over a ruleset with combined numeric and text rule matching":
+
+  - do:
+      query_ruleset.put:
+        ruleset_id: combined-ruleset
+        body:
+          rules:
+            - rule_id: rule1
+              type: pinned
+              criteria:
+                - type: fuzzy
+                  metadata: foo
+                  values: [ bar ]
+              actions:
+                ids:
+                  - 'doc1'
+            - rule_id: rule2
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: foo
+                  values: [ 100 ]
+              actions:
+                ids:
+                  - 'doc2'
+  - do:
+      search:
+        body:
+          query:
+            rule_query:
+              organic:
+                query_string:
+                  default_field: text
+                  query: blah blah blah
+              match_criteria:
+                foo: baz
+              ruleset_id: combined-ruleset
+
+  - match: { hits.total.value: 1 }
+  - match: { hits.hits.0._id: 'doc1' }
+
 
diff --git a/.../plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java b/.../plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java
@@ -294,7 +294,7 @@ public AppliedQueryRules applyRule(AppliedQueryRules appliedRules, Map<String, O
                 final String criteriaMetadata = criterion.criteriaMetadata();
 
                 if (criteriaType == ALWAYS || (criteriaMetadata != null && criteriaMetadata.equals(match))) {
-                    boolean singleCriterionMatches = criterion.isMatch(matchValue, criteriaType);
+                    boolean singleCriterionMatches = criterion.isMatch(matchValue, criteriaType, false);
                     isRuleMatch = (isRuleMatch == null) ? singleCriterionMatches : isRuleMatch && singleCriterionMatches;
                 }
             }

diff --git a/...ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteria.java b/...ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteria.java
@@ -191,12 +191,19 @@ public String toString() {
     }
 
     public boolean isMatch(Object matchValue, QueryRuleCriteriaType matchType) {
+        return isMatch(matchValue, matchType, true);
+    }
+
+    public boolean isMatch(Object matchValue, QueryRuleCriteriaType matchType, boolean throwOnInvalidInput) {
         if (matchType == ALWAYS) {
             return true;
         }
         final String matchString = matchValue.toString();
         for (Object criteriaValue : criteriaValues) {
-            matchType.validateInput(matchValue);
+            boolean isValid = matchType.validateInput(matchValue, throwOnInvalidInput);
+            if (isValid == false) {
+                return false;
+            }
             boolean matchFound = matchType.isMatch(matchString, criteriaValue);
             if (matchFound) {
                 return true;

diff --git a/...search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java b/...search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java
@@ -86,11 +86,16 @@ public boolean isMatch(Object input, Object criteriaValue) {
         }
     };
 
-    public void validateInput(Object input) {
+    public boolean validateInput(Object input, boolean throwOnInvalidInput) {
         boolean isValid = isValidForInput(input);
-        if (isValid == false) {
+        if (isValid == false && throwOnInvalidInput) {
             throw new IllegalArgumentException("Input [" + input + "] is not valid for CriteriaType [" + this + "]");
         }
+        return isValid;
+    }
+
+    public boolean validateInput(Object input) {
+        return validateInput(input, true);
     }
 
     public abstract boolean isMatch(Object input, Object criteriaValue);