From 139c35bf81f7df005c86f9ab48638cf396871ced Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Tue, 18 Feb 2025 16:45:39 +0530 Subject: [PATCH 01/22] SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria --- .../api/entsearch.put_query_ruleset.json | 32 ++++ .../entsearch/rules/55_numeric_validation.yml | 138 +++++++++++++++ .../xpack/application/rules/QueryRule.java | 18 +- .../rules/QueryRuleCriteriaType.java | 4 + .../EnterpriseSearchModuleTestUtils.java | 101 ++++++++++- .../application/rules/QueryRuleTests.java | 160 ++++++++++++++++++ 6 files changed, 444 insertions(+), 9 deletions(-) create mode 100644 x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json create mode 100644 x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json new file mode 100644 index 0000000000000..ebbe280d6a813 --- /dev/null +++ b/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 + } + } +} \ No newline at end of file diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml new file mode 100644 index 0000000000000..77e986535886a --- /dev/null +++ b/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 + + + diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java index e1734268fbf46..1594b7786f18c 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java +++ b/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,23 @@ 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 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) { diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java index 5606d47697d2e..3c94b90ce0f19 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java +++ b/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; + } } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 190b3c3e53169..6d19b6ba6977d 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -39,6 +39,8 @@ 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; +import static org.elasticsearch.test.ESTestCase.randomAlphaOfLength; +import static org.elasticsearch.test.ESTestCase.randomDoubleBetween; public final class EnterpriseSearchModuleTestUtils { @@ -87,18 +89,79 @@ public static Map 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 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 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 criteria = List.of(randomQueryRuleCriteria()); - Map actions = Map.of(randomFrom("ids", "docs"), List.of(randomAlphaOfLengthBetween(2, 10))); - Integer priority = randomQueryRulePriority(); - return new QueryRule(id, type, criteria, actions, priority); + List 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 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 +182,26 @@ public static Map randomMatchCriteria() { return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10))); } + public static Map 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)); + } + } + } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 67e7f6ac7d9e9..abff43e73d12a 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -20,6 +20,8 @@ import org.elasticsearch.xpack.application.EnterpriseSearchModuleTestUtils; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; import org.junit.Before; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentFactory; import java.io.IOException; import java.util.Collections; @@ -32,6 +34,7 @@ import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.EXACT; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.PREFIX; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.SUFFIX; +import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.LTE; import static org.hamcrest.CoreMatchers.equalTo; public class QueryRuleTests extends ESTestCase { @@ -53,6 +56,73 @@ public final void testRandomSerialization() throws IOException { } } + public void testNumericValidationWithValidValues() throws IOException { + String content = XContentHelper.stripWhitespace(""" + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": ["100.50", "200"] } + ], + "actions": { + "ids": ["id1"] + } + }"""); + QueryRule queryRule = QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON); + boolean humanReadable = true; + BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable); + QueryRule parsed; + try (XContentParser parser = createParser(XContentType.JSON.xContent(), originalBytes)) { + parsed = QueryRule.fromXContent(parser); + } + assertToXContentEquivalent(originalBytes, toXContent(parsed, XContentType.JSON, humanReadable), XContentType.JSON); + } + + public void testNumericValidationWithInvalidValues() throws IOException { + String content = XContentHelper.stripWhitespace(""" + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": ["abc"] } + ], + "actions": { + "ids": ["id1"] + } + }"""); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + } + + public void testNumericValidationWithMixedValues() throws IOException { + String content = XContentHelper.stripWhitespace(""" + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": ["100", "abc", "200"] } + ], + "actions": { + "ids": ["id1"] + } + }"""); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + } + + public void testNumericValidationWithEmptyValues() throws IOException { + String content = XContentHelper.stripWhitespace(""" + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": [] } + ], + "actions": { + "ids": ["id1"] + } + }"""); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + } + public void testToXContent() throws IOException { String content = XContentHelper.stripWhitespace(""" { @@ -333,6 +403,96 @@ public void testApplyRuleWithMultipleCriteria() throws IOException { assertEquals(Collections.emptyList(), appliedQueryRules.excludedDocs()); } + public void testValidateNumericComparisonRule() { + // Test valid numeric value + QueryRuleCriteria validCriteria = new QueryRuleCriteria(LTE, "price", List.of("100")); + QueryRule validRule = new QueryRule( + "test_rule", + QueryRule.QueryRuleType.PINNED, + List.of(validCriteria), + Map.of("ids", List.of("1")), + null + ); + // Should not throw exception + assertNotNull(validRule); + + // Test invalid numeric value + QueryRuleCriteria invalidCriteria = new QueryRuleCriteria(LTE, "price", List.of("not_a_number")); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> new QueryRule( + "test_rule", + QueryRule.QueryRuleType.PINNED, + List.of(invalidCriteria), + Map.of("ids", List.of("1")), + null + ) + ); + + String expectedMessage = String.format( + "Numeric comparison rule type %s requires numeric values, but got %s for criterion %s", + "lte", + "not_a_number", + "{\"type\":\"lte\",\"metadata\":\"price\",\"values\":[\"not_a_number\"]}" + ); + assertEquals(expectedMessage, e.getMessage()); + } + + public void testParseNumericComparisonRule() throws IOException { + // Test parsing valid numeric rule + XContentBuilder builder = XContentFactory.jsonBuilder(); + builder.startObject(); + { + builder.field("rule_id", "test_rule"); + builder.field("type", "pinned"); + builder.startArray("criteria"); + { + builder.startObject(); + builder.field("type", "lte"); + builder.field("metadata", "price"); + builder.startArray("values").value("100").endArray(); + builder.endObject(); + } + builder.endArray(); + builder.startObject("actions"); + { + builder.startArray("ids").value("1").endArray(); + } + builder.endObject(); + } + builder.endObject(); + + // Get BytesReference from the builder + BytesReference bytesRef = BytesReference.bytes(builder); + QueryRule rule = QueryRule.fromXContentBytes(bytesRef, builder.contentType()); + assertNotNull(rule); + assertEquals("test_rule", rule.id()); + assertEquals(QueryRule.QueryRuleType.PINNED, rule.type()); + assertEquals(1, rule.criteria().size()); + assertEquals(LTE, rule.criteria().get(0).criteriaType()); + assertEquals("100", rule.criteria().get(0).criteriaValues().get(0)); + } + + public void testIsRuleMatchWithNumericComparison() { + QueryRuleCriteria criteria = new QueryRuleCriteria(LTE, "price", List.of("100")); + QueryRule rule = new QueryRule( + "test_rule", + QueryRule.QueryRuleType.PINNED, + List.of(criteria), + Map.of("ids", List.of("1")), + null + ); + + // Test matching value + assertTrue(rule.isRuleMatch(Map.of("price", "50"))); + + // Test non-matching value + assertFalse(rule.isRuleMatch(Map.of("price", "150"))); + + // Test with non-numeric value + assertFalse(rule.isRuleMatch(Map.of("price", "not_a_number"))); + } + private void assertXContent(QueryRule queryRule, boolean humanReadable) throws IOException { BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable); QueryRule parsed; From 085cec09c888369c22274be355a431eb78226c2d Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 18 Feb 2025 11:32:00 +0000 Subject: [PATCH 02/22] [CI] Auto commit changes from spotless --- .../xpack/application/rules/QueryRule.java | 8 +- .../EnterpriseSearchModuleTestUtils.java | 11 +- .../application/rules/QueryRuleTests.java | 106 ++++++++---------- 3 files changed, 56 insertions(+), 69 deletions(-) diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java index 1594b7786f18c..5a4578066acb6 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java @@ -164,8 +164,12 @@ private void validate() { Double.parseDouble(value.toString()); } catch (NumberFormatException e) { throw new IllegalArgumentException( - "Numeric comparison rule type " + criteriaType + " requires numeric values, but got " + value + - " for criterion " + criterion + "Numeric comparison rule type " + + criteriaType + + " requires numeric values, but got " + + value + + " for criterion " + + criterion ); } } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 6d19b6ba6977d..fd9a99d6b023c 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -21,26 +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; -import static org.elasticsearch.test.ESTestCase.randomAlphaOfLength; -import static org.elasticsearch.test.ESTestCase.randomDoubleBetween; public final class EnterpriseSearchModuleTestUtils { @@ -154,9 +151,7 @@ private static QueryRule createNonNumericQueryRule() { ); String metadata = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : randomAlphaOfLength(10); - List values = criteriaType == QueryRuleCriteriaType.ALWAYS ? - null : - List.of(randomAlphaOfLength(5)); + List values = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : List.of(randomAlphaOfLength(5)); criteria.add(new QueryRuleCriteria(criteriaType, metadata, values)); } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index abff43e73d12a..b210633f328cd 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -15,13 +15,13 @@ import org.elasticsearch.search.SearchModule; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.ToXContent; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentFactory; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.application.EnterpriseSearchModuleTestUtils; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; import org.junit.Before; -import org.elasticsearch.xcontent.XContentBuilder; -import org.elasticsearch.xcontent.XContentFactory; import java.io.IOException; import java.util.Collections; @@ -32,9 +32,9 @@ import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.EXACT; +import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.LTE; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.PREFIX; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.SUFFIX; -import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.LTE; import static org.hamcrest.CoreMatchers.equalTo; public class QueryRuleTests extends ESTestCase { @@ -58,16 +58,16 @@ public final void testRandomSerialization() throws IOException { public void testNumericValidationWithValidValues() throws IOException { String content = XContentHelper.stripWhitespace(""" - { - "rule_id": "numeric_rule", - "type": "pinned", - "criteria": [ - { "type": "lte", "metadata": "price", "values": ["100.50", "200"] } - ], - "actions": { - "ids": ["id1"] - } - }"""); + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": ["100.50", "200"] } + ], + "actions": { + "ids": ["id1"] + } + }"""); QueryRule queryRule = QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON); boolean humanReadable = true; BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable); @@ -80,46 +80,46 @@ public void testNumericValidationWithValidValues() throws IOException { public void testNumericValidationWithInvalidValues() throws IOException { String content = XContentHelper.stripWhitespace(""" - { - "rule_id": "numeric_rule", - "type": "pinned", - "criteria": [ - { "type": "lte", "metadata": "price", "values": ["abc"] } - ], - "actions": { - "ids": ["id1"] - } - }"""); + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": ["abc"] } + ], + "actions": { + "ids": ["id1"] + } + }"""); expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } public void testNumericValidationWithMixedValues() throws IOException { String content = XContentHelper.stripWhitespace(""" - { - "rule_id": "numeric_rule", - "type": "pinned", - "criteria": [ - { "type": "lte", "metadata": "price", "values": ["100", "abc", "200"] } - ], - "actions": { - "ids": ["id1"] - } - }"""); + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": ["100", "abc", "200"] } + ], + "actions": { + "ids": ["id1"] + } + }"""); expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } public void testNumericValidationWithEmptyValues() throws IOException { String content = XContentHelper.stripWhitespace(""" - { - "rule_id": "numeric_rule", - "type": "pinned", - "criteria": [ - { "type": "lte", "metadata": "price", "values": [] } - ], - "actions": { - "ids": ["id1"] - } - }"""); + { + "rule_id": "numeric_rule", + "type": "pinned", + "criteria": [ + { "type": "lte", "metadata": "price", "values": [] } + ], + "actions": { + "ids": ["id1"] + } + }"""); expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } @@ -420,13 +420,7 @@ public void testValidateNumericComparisonRule() { QueryRuleCriteria invalidCriteria = new QueryRuleCriteria(LTE, "price", List.of("not_a_number")); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> new QueryRule( - "test_rule", - QueryRule.QueryRuleType.PINNED, - List.of(invalidCriteria), - Map.of("ids", List.of("1")), - null - ) + () -> new QueryRule("test_rule", QueryRule.QueryRuleType.PINNED, List.of(invalidCriteria), Map.of("ids", List.of("1")), null) ); String expectedMessage = String.format( @@ -475,20 +469,14 @@ public void testParseNumericComparisonRule() throws IOException { public void testIsRuleMatchWithNumericComparison() { QueryRuleCriteria criteria = new QueryRuleCriteria(LTE, "price", List.of("100")); - QueryRule rule = new QueryRule( - "test_rule", - QueryRule.QueryRuleType.PINNED, - List.of(criteria), - Map.of("ids", List.of("1")), - null - ); + QueryRule rule = new QueryRule("test_rule", QueryRule.QueryRuleType.PINNED, List.of(criteria), Map.of("ids", List.of("1")), null); // Test matching value assertTrue(rule.isRuleMatch(Map.of("price", "50"))); - + // Test non-matching value assertFalse(rule.isRuleMatch(Map.of("price", "150"))); - + // Test with non-numeric value assertFalse(rule.isRuleMatch(Map.of("price", "not_a_number"))); } From da6c1131c60041b3f203fb2266eb3129a3592ac8 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Thu, 20 Feb 2025 15:00:04 +0530 Subject: [PATCH 03/22] Worked on the comments given in the PR --- .../api/entsearch.put_query_ruleset.json | 32 ----- .../entsearch/rules/55_numeric_validation.yml | 94 +-------------- .../xpack/application/rules/QueryRule.java | 24 +--- .../rules/QueryRuleCriteriaType.java | 4 - .../EnterpriseSearchModuleTestUtils.java | 114 ++++++------------ .../application/rules/QueryRuleTests.java | 38 ++---- 6 files changed, 52 insertions(+), 254 deletions(-) delete mode 100644 x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json deleted file mode 100644 index ebbe280d6a813..0000000000000 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json +++ /dev/null @@ -1,32 +0,0 @@ -{ - "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 - } - } -} \ No newline at end of file diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml index 77e986535886a..4578f36645361 100644 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml +++ b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml @@ -4,31 +4,7 @@ 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 + catch: /\[query_ruleset\] failed to parse field \[rules\]/ query_rules.put_ruleset: ruleset_id: invalid_numeric_test body: @@ -42,55 +18,10 @@ 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 + catch: /\[query_ruleset\] failed to parse field \[rules\]/ query_rules.put_ruleset: ruleset_id: multiple_values_test body: @@ -104,35 +35,16 @@ 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 - + diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java index 5a4578066acb6..e0d2f26a8a3a7 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java @@ -155,26 +155,12 @@ private void validate() { } } - for (QueryRuleCriteria criterion : criteria) { - QueryRuleCriteriaType criteriaType = criterion.criteriaType(); - if (criteriaType.isNumericComparison()) { - List 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 - ); - } - } + criteria.forEach(criterion -> { + List values = criterion.criteriaValues(); + if (values != null) { + values.forEach(criterion.criteriaType()::validateInput); } - } + }); } private void validateIdOrDocAction(Object action) { diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java index 3c94b90ce0f19..5606d47697d2e 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRuleCriteriaType.java @@ -135,8 +135,4 @@ 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; - } } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index fd9a99d6b023c..f347f4fb3ccbe 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -20,6 +20,8 @@ import org.elasticsearch.xpack.application.search.TemplateParamValidator; import org.elasticsearch.xpack.core.action.util.PageParams; +import java.math.BigDecimal; +import java.math.RoundingMode; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -85,80 +87,44 @@ public static Map randomSearchApplicationQueryParams() { return randomMap(0, 10, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10))); } + public static QueryRule randomQueryRule() { + String ruleId = randomAlphaOfLengthBetween(3, 10); + QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values()); + List criteria = new ArrayList<>(); + int numCriteria = randomIntBetween(1, 3); + + for (int i = 0; i < numCriteria; i++) { + criteria.add(randomQueryRuleCriteria()); + } + + return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority()); + } + public static QueryRuleCriteria randomQueryRuleCriteria() { QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); - String metadata = randomAlphaOfLength(10); + String metadata = randomAlphaOfLengthBetween(3, 10); List values = new ArrayList<>(); int numValues = randomIntBetween(1, 3); - if (criteriaType.isNumericComparison()) { + if (List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, + QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE).contains(criteriaType)) { for (int i = 0; i < numValues; i++) { - double value = Math.round(randomDoubleBetween(0, 1000, true) * 100.0) / 100.0; - values.add(String.valueOf(value)); + BigDecimal value = BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)) + .setScale(2, RoundingMode.HALF_UP); + values.add(value.toString()); } } else if (criteriaType == QueryRuleCriteriaType.ALWAYS) { metadata = null; values = null; } else { for (int i = 0; i < numValues; i++) { - values.add(randomAlphaOfLength(5)); + values.add(randomAlphaOfLengthBetween(3, 10)); } } return new QueryRuleCriteria(criteriaType, metadata, values); } - public static QueryRule randomQueryRule() { - if (randomBoolean()) { - return createNumericQueryRule(); - } else { - return createNonNumericQueryRule(); - } - } - - private static QueryRule createNumericQueryRule() { - String ruleId = randomAlphaOfLength(10); - QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values()); - List 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 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 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() { return randomBoolean() ? randomIntBetween(MIN_PRIORITY, MAX_PRIORITY) : null; } @@ -173,30 +139,22 @@ public static QueryRuleset randomQueryRuleset() { return new QueryRuleset(id, rules); } - public static Map randomMatchCriteria() { - return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10))); - } - public static Map 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)); + if (randomBoolean()) { + return Map.of(QueryRule.IDS_FIELD.getPreferredName(), List.of(randomAlphaOfLengthBetween(3, 10))); } + return Map.of( + QueryRule.DOCS_FIELD.getPreferredName(), + List.of( + Map.of( + QueryRule.INDEX_FIELD.getPreferredName(), randomAlphaOfLengthBetween(3, 10), + QueryRule.ID_FIELD.getPreferredName(), randomAlphaOfLengthBetween(3, 10) + ) + ) + ); } + public static Map randomMatchCriteria() { + return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10))); + } } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index b210633f328cd..9f29bb346b584 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -404,36 +404,20 @@ public void testApplyRuleWithMultipleCriteria() throws IOException { } public void testValidateNumericComparisonRule() { - // Test valid numeric value - QueryRuleCriteria validCriteria = new QueryRuleCriteria(LTE, "price", List.of("100")); - QueryRule validRule = new QueryRule( - "test_rule", - QueryRule.QueryRuleType.PINNED, - List.of(validCriteria), - Map.of("ids", List.of("1")), - null - ); - // Should not throw exception - assertNotNull(validRule); - - // Test invalid numeric value - QueryRuleCriteria invalidCriteria = new QueryRuleCriteria(LTE, "price", List.of("not_a_number")); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> new QueryRule("test_rule", QueryRule.QueryRuleType.PINNED, List.of(invalidCriteria), Map.of("ids", List.of("1")), null) + () -> new QueryRule( + "test_rule", + QueryRule.QueryRuleType.PINNED, + List.of(new QueryRuleCriteria(QueryRuleCriteriaType.LTE, "price", List.of("not_a_number"))), + Map.of("ids", List.of("1")), + null + ) ); - - String expectedMessage = String.format( - "Numeric comparison rule type %s requires numeric values, but got %s for criterion %s", - "lte", - "not_a_number", - "{\"type\":\"lte\",\"metadata\":\"price\",\"values\":[\"not_a_number\"]}" - ); - assertEquals(expectedMessage, e.getMessage()); + assertEquals("Input [not_a_number] is not valid for CriteriaType [lte]", e.getMessage()); } public void testParseNumericComparisonRule() throws IOException { - // Test parsing valid numeric rule XContentBuilder builder = XContentFactory.jsonBuilder(); builder.startObject(); { @@ -456,7 +440,6 @@ public void testParseNumericComparisonRule() throws IOException { } builder.endObject(); - // Get BytesReference from the builder BytesReference bytesRef = BytesReference.bytes(builder); QueryRule rule = QueryRule.fromXContentBytes(bytesRef, builder.contentType()); assertNotNull(rule); @@ -471,13 +454,8 @@ public void testIsRuleMatchWithNumericComparison() { QueryRuleCriteria criteria = new QueryRuleCriteria(LTE, "price", List.of("100")); QueryRule rule = new QueryRule("test_rule", QueryRule.QueryRuleType.PINNED, List.of(criteria), Map.of("ids", List.of("1")), null); - // Test matching value assertTrue(rule.isRuleMatch(Map.of("price", "50"))); - - // Test non-matching value assertFalse(rule.isRuleMatch(Map.of("price", "150"))); - - // Test with non-numeric value assertFalse(rule.isRuleMatch(Map.of("price", "not_a_number"))); } From 2161e1bd5a0a362d54c56819848c4bc124ea326e Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 20 Feb 2025 09:37:29 +0000 Subject: [PATCH 04/22] [CI] Auto commit changes from spotless --- .../EnterpriseSearchModuleTestUtils.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index f347f4fb3ccbe..76a6e65e32681 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -29,7 +29,6 @@ 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; @@ -106,11 +105,10 @@ public static QueryRuleCriteria randomQueryRuleCriteria() { List values = new ArrayList<>(); int numValues = randomIntBetween(1, 3); - if (List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, - QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE).contains(criteriaType)) { + if (List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE) + .contains(criteriaType)) { for (int i = 0; i < numValues; i++) { - BigDecimal value = BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)) - .setScale(2, RoundingMode.HALF_UP); + BigDecimal value = BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP); values.add(value.toString()); } } else if (criteriaType == QueryRuleCriteriaType.ALWAYS) { @@ -140,15 +138,17 @@ public static QueryRuleset randomQueryRuleset() { } public static Map randomQueryRuleActions() { - if (randomBoolean()) { + if (randomBoolean()) { return Map.of(QueryRule.IDS_FIELD.getPreferredName(), List.of(randomAlphaOfLengthBetween(3, 10))); } return Map.of( QueryRule.DOCS_FIELD.getPreferredName(), List.of( Map.of( - QueryRule.INDEX_FIELD.getPreferredName(), randomAlphaOfLengthBetween(3, 10), - QueryRule.ID_FIELD.getPreferredName(), randomAlphaOfLengthBetween(3, 10) + QueryRule.INDEX_FIELD.getPreferredName(), + randomAlphaOfLengthBetween(3, 10), + QueryRule.ID_FIELD.getPreferredName(), + randomAlphaOfLengthBetween(3, 10) ) ) ); From 910cda4ef7f35701ade8a418954391b67d7d922f Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Thu, 20 Feb 2025 15:27:41 +0530 Subject: [PATCH 05/22] Fixed Integration tests --- .../entsearch/rules/10_query_ruleset_put.yml | 58 +++++++++++++++++++ .../entsearch/rules/55_numeric_validation.yml | 50 ---------------- .../application/EnterpriseSearchFeatures.java | 9 ++- .../xpack/application/rules/QueryRule.java | 3 + 4 files changed, 69 insertions(+), 51 deletions(-) delete mode 100644 x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml index ee94cff46d1fb..4641629df9887 100644 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml +++ b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml @@ -15,6 +15,16 @@ teardown: ruleset_id: test-query-ruleset-recreating ignore: 404 + - do: + query_rules.delete_ruleset: + ruleset_id: invalid_numeric_test + ignore: 404 + + - do: + query_rules.delete_ruleset: + ruleset_id: multiple_values_test + ignore: 404 + --- 'Create Query Ruleset': - do: @@ -160,3 +170,51 @@ teardown: - 'id2' - match: { error.type: 'security_exception' } + +--- +"Test numeric comparison rule validation": + - skip: + features: headers + + - requires: + cluster_features: "query_rules.numeric_validation" + reason: "Query rules enforce numeric validation starting with 8.19" + + - do: + catch: /\[query_ruleset\] failed to parse field \[rules\]/ + 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"] + +--- +"Test numeric comparison with multiple values": + - skip: + features: headers + + - requires: + cluster_features: "query_rules.numeric_validation" + reason: "Query rules enforce numeric validation starting with 8.19" + + - do: + catch: /\[query_ruleset\] failed to parse field \[rules\]/ + 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"] diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml deleted file mode 100644 index 4578f36645361..0000000000000 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml +++ /dev/null @@ -1,50 +0,0 @@ ---- -"Test numeric comparison rule validation": - - skip: - features: headers - - - do: - catch: /\[query_ruleset\] failed to parse field \[rules\]/ - 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"] - ---- -"Test numeric comparison with multiple values": - - do: - catch: /\[query_ruleset\] failed to parse field \[rules\]/ - 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"] - ---- -teardown: - - do: - query_rules.delete_ruleset: - ruleset_id: invalid_numeric_test - ignore: 404 - - do: - query_rules.delete_ruleset: - ruleset_id: multiple_values_test - ignore: 404 - - - diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java index 0f5c38e6f75ed..988860eb56e32 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java @@ -9,13 +9,20 @@ import org.elasticsearch.features.FeatureSpecification; import org.elasticsearch.features.NodeFeature; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.xpack.application.rules.QueryRule; import java.util.Set; -public class EnterpriseSearchFeatures implements FeatureSpecification { +public class EnterpriseSearchFeatures extends Plugin implements FeatureSpecification { @Override public Set getFeatures() { return Set.of(); } + + @Override + public Set getTestFeatures() { + return Set.of(QueryRule.NUMERIC_VALIDATION); + } } diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java index e0d2f26a8a3a7..e0ad52f6d69b0 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java @@ -24,6 +24,7 @@ import org.elasticsearch.xcontent.XContentParserConfiguration; import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; +import org.elasticsearch.features.NodeFeature; import java.io.IOException; import java.util.ArrayList; @@ -81,6 +82,8 @@ public String toString() { } } + public static final NodeFeature NUMERIC_VALIDATION = new NodeFeature("query_rules.numeric_validation", true); + /** * Public constructor. * From da07f227c1f178d90020a1f85ecf7181edc5917e Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 20 Feb 2025 10:05:05 +0000 Subject: [PATCH 06/22] [CI] Auto commit changes from spotless --- .../org/elasticsearch/xpack/application/rules/QueryRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java index e0ad52f6d69b0..22bb1673cb638 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java @@ -16,6 +16,7 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.core.Nullable; +import org.elasticsearch.features.NodeFeature; import org.elasticsearch.xcontent.ConstructingObjectParser; import org.elasticsearch.xcontent.ParseField; import org.elasticsearch.xcontent.ToXContentObject; @@ -24,7 +25,6 @@ import org.elasticsearch.xcontent.XContentParserConfiguration; import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; -import org.elasticsearch.features.NodeFeature; import java.io.IOException; import java.util.ArrayList; From 3acf750d489c8d0ba3b5e4e328999cc458cfc5c2 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Thu, 20 Feb 2025 22:43:55 +0530 Subject: [PATCH 07/22] Made changes from the PR --- .../test/entsearch/rules/10_query_ruleset_put.yml | 8 ++++---- .../xpack/application/EnterpriseSearchFeatures.java | 2 +- .../application/EnterpriseSearchModuleTestUtils.java | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml index 4641629df9887..24887b760d445 100644 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml +++ b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml @@ -178,10 +178,10 @@ teardown: - requires: cluster_features: "query_rules.numeric_validation" - reason: "Query rules enforce numeric validation starting with 8.19" + reason: "Fixed bug to enforce numeric rule validation starting with 8.19" - do: - catch: /\[query_ruleset\] failed to parse field \[rules\]/ + catch: /Input \[not_a_number\] is not valid for CriteriaType \[lte\]/ query_rules.put_ruleset: ruleset_id: invalid_numeric_test body: @@ -202,10 +202,10 @@ teardown: - requires: cluster_features: "query_rules.numeric_validation" - reason: "Query rules enforce numeric validation starting with 8.19" + reason: "Fixed bug to enforce numeric rule validation starting with 8.19" - do: - catch: /\[query_ruleset\] failed to parse field \[rules\]/ + catch: /Input \[not_a_number\] is not valid for CriteriaType \[lte\]/ query_rules.put_ruleset: ruleset_id: multiple_values_test body: diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java index 988860eb56e32..444dc21b2fd5a 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java @@ -14,7 +14,7 @@ import java.util.Set; -public class EnterpriseSearchFeatures extends Plugin implements FeatureSpecification { +public class EnterpriseSearchFeatures implements FeatureSpecification { @Override public Set getFeatures() { diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 76a6e65e32681..39e807c2c9393 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -87,7 +87,7 @@ public static Map randomSearchApplicationQueryParams() { } public static QueryRule randomQueryRule() { - String ruleId = randomAlphaOfLengthBetween(3, 10); + String ruleId = randomIdentifier(); QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values()); List criteria = new ArrayList<>(); int numCriteria = randomIntBetween(1, 3); From 00571cd444fad78e3eba6b6942cdb451d118e57c Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 20 Feb 2025 22:50:29 +0530 Subject: [PATCH 08/22] Update docs/changelog/122823.yaml --- docs/changelog/122823.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/122823.yaml diff --git a/docs/changelog/122823.yaml b/docs/changelog/122823.yaml new file mode 100644 index 0000000000000..e0d44cc261f2c --- /dev/null +++ b/docs/changelog/122823.yaml @@ -0,0 +1,5 @@ +pr: 122823 +summary: Prevent Query Rule Creation with Invalid Numeric Match Criteria +area: Relevance +type: bug +issues: [] From 58fe5d4e5320033ebcf82133e294c7d6ec3a31bc Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 20 Feb 2025 17:28:30 +0000 Subject: [PATCH 09/22] [CI] Auto commit changes from spotless --- .../xpack/application/EnterpriseSearchFeatures.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java index 444dc21b2fd5a..905a230b7266d 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java @@ -9,7 +9,6 @@ import org.elasticsearch.features.FeatureSpecification; import org.elasticsearch.features.NodeFeature; -import org.elasticsearch.plugins.Plugin; import org.elasticsearch.xpack.application.rules.QueryRule; import java.util.Set; From 74cd5d67c1aa18d8f7090b1dcf2d4d9ed29c5332 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Fri, 21 Feb 2025 19:18:10 +0530 Subject: [PATCH 10/22] Fixed the duplicate code issue in queryRuleTests --- .../application/rules/QueryRuleTests.java | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 9f29bb346b584..261f4d4de84dd 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -68,14 +68,7 @@ public void testNumericValidationWithValidValues() throws IOException { "ids": ["id1"] } }"""); - QueryRule queryRule = QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON); - boolean humanReadable = true; - BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable); - QueryRule parsed; - try (XContentParser parser = createParser(XContentType.JSON.xContent(), originalBytes)) { - parsed = QueryRule.fromXContent(parser); - } - assertToXContentEquivalent(originalBytes, toXContent(parsed, XContentType.JSON, humanReadable), XContentType.JSON); + testToXContentRules(content); } public void testNumericValidationWithInvalidValues() throws IOException { @@ -136,15 +129,7 @@ public void testToXContent() throws IOException { }, "priority": 5 }"""); - - QueryRule queryRule = QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON); - boolean humanReadable = true; - BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable); - QueryRule parsed; - try (XContentParser parser = createParser(XContentType.JSON.xContent(), originalBytes)) { - parsed = QueryRule.fromXContent(parser); - } - assertToXContentEquivalent(originalBytes, toXContent(parsed, XContentType.JSON, humanReadable), XContentType.JSON); + testToXContentRules(content); } public void testToXContentEmptyCriteria() throws IOException { From 3f023c408fc72db2060c2779e3f03a49adcbe20d Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Wed, 26 Feb 2025 14:51:34 +0530 Subject: [PATCH 11/22] Refactored code to clean it up based on PR comments --- .../EnterpriseSearchModuleTestUtils.java | 37 ++++++++++--------- .../application/rules/QueryRuleTests.java | 34 +++++++++++++++-- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 39e807c2c9393..10c336f59eb56 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -99,27 +99,28 @@ public static QueryRule randomQueryRule() { return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority()); } - public static QueryRuleCriteria randomQueryRuleCriteria() { - QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); - String metadata = randomAlphaOfLengthBetween(3, 10); + private static List generateRandomValues(QueryRuleCriteriaType criteriaType, int numValues) { + if (criteriaType == QueryRuleCriteriaType.ALWAYS) { + return null; + } + List values = new ArrayList<>(); - int numValues = randomIntBetween(1, 3); - - if (List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE) - .contains(criteriaType)) { - for (int i = 0; i < numValues; i++) { - BigDecimal value = BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP); - values.add(value.toString()); - } - } else if (criteriaType == QueryRuleCriteriaType.ALWAYS) { - metadata = null; - values = null; - } else { - for (int i = 0; i < numValues; i++) { - values.add(randomAlphaOfLengthBetween(3, 10)); - } + for (int i = 0; i < numValues; i++) { + values.add( + List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE) + .contains(criteriaType) + ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() + : randomAlphaOfLengthBetween(3, 10) + ); } + return values; + } + public static QueryRuleCriteria randomQueryRuleCriteria() { + QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); + String metadata = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : randomAlphaOfLengthBetween(3, 10); + int numValues = randomIntBetween(1, 3); + List values = generateRandomValues(criteriaType, numValues); return new QueryRuleCriteria(criteriaType, metadata, values); } diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 261f4d4de84dd..6b27ae1233dd8 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -22,6 +22,8 @@ import org.elasticsearch.xpack.application.EnterpriseSearchModuleTestUtils; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; import org.junit.Before; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collections; @@ -39,6 +41,7 @@ public class QueryRuleTests extends ESTestCase { private NamedWriteableRegistry namedWriteableRegistry; + private static final Logger logger = LoggerFactory.getLogger(QueryRuleTests.class); @Before public void registerNamedObjects() { @@ -83,7 +86,13 @@ public void testNumericValidationWithInvalidValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + logger.info("Actual error message for invalid values: " + e.getMessage()); + assertTrue("Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]")); } public void testNumericValidationWithMixedValues() throws IOException { @@ -98,7 +107,13 @@ public void testNumericValidationWithMixedValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + logger.info("Actual error message for mixed values: " + e.getMessage()); + assertTrue("Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]")); } public void testNumericValidationWithEmptyValues() throws IOException { @@ -113,7 +128,12 @@ public void testNumericValidationWithEmptyValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + logger.info("Actual error message: " + e.getMessage()); + assertTrue(e.getMessage().contains("failed to parse field [criteria]")); } public void testToXContent() throws IOException { @@ -140,7 +160,13 @@ public void testToXContentEmptyCriteria() throws IOException { "criteria": [], "actions": {} }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + logger.info("Actual error message for empty criteria: " + e.getMessage()); + assertTrue("Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]")); } public void testToXContentValidPinnedRulesWithIds() throws IOException { From 6a77cecdada4b05dd52b53002355b5df753ceb61 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Wed, 26 Feb 2025 09:30:33 +0000 Subject: [PATCH 12/22] [CI] Auto commit changes from spotless --- .../EnterpriseSearchModuleTestUtils.java | 6 +++--- .../application/rules/QueryRuleTests.java | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 10c336f59eb56..15dfa21365071 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -103,14 +103,14 @@ private static List generateRandomValues(QueryRuleCriteriaType criteriaT if (criteriaType == QueryRuleCriteriaType.ALWAYS) { return null; } - + List values = new ArrayList<>(); for (int i = 0; i < numValues; i++) { values.add( List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE) .contains(criteriaType) - ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() - : randomAlphaOfLengthBetween(3, 10) + ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() + : randomAlphaOfLengthBetween(3, 10) ); } return values; diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 6b27ae1233dd8..92e2b94c59fc4 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -91,8 +91,10 @@ public void testNumericValidationWithInvalidValues() throws IOException { () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); logger.info("Actual error message for invalid values: " + e.getMessage()); - assertTrue("Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", - e.getMessage().contains("Failed to build [query_rule]")); + assertTrue( + "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]") + ); } public void testNumericValidationWithMixedValues() throws IOException { @@ -112,8 +114,10 @@ public void testNumericValidationWithMixedValues() throws IOException { () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); logger.info("Actual error message for mixed values: " + e.getMessage()); - assertTrue("Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", - e.getMessage().contains("Failed to build [query_rule]")); + assertTrue( + "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]") + ); } public void testNumericValidationWithEmptyValues() throws IOException { @@ -165,8 +169,10 @@ public void testToXContentEmptyCriteria() throws IOException { () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); logger.info("Actual error message for empty criteria: " + e.getMessage()); - assertTrue("Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", - e.getMessage().contains("Failed to build [query_rule]")); + assertTrue( + "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]") + ); } public void testToXContentValidPinnedRulesWithIds() throws IOException { From ab9f38359f4ad4f20eb4f6d68b0657e759822bc8 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Tue, 4 Mar 2025 11:56:30 +0530 Subject: [PATCH 13/22] Logger statements were removed --- .../xpack/application/rules/QueryRuleTests.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 92e2b94c59fc4..5b60f1ba050ff 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -22,8 +22,6 @@ import org.elasticsearch.xpack.application.EnterpriseSearchModuleTestUtils; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; import org.junit.Before; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collections; @@ -41,7 +39,6 @@ public class QueryRuleTests extends ESTestCase { private NamedWriteableRegistry namedWriteableRegistry; - private static final Logger logger = LoggerFactory.getLogger(QueryRuleTests.class); @Before public void registerNamedObjects() { @@ -90,7 +87,6 @@ public void testNumericValidationWithInvalidValues() throws IOException { IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - logger.info("Actual error message for invalid values: " + e.getMessage()); assertTrue( "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", e.getMessage().contains("Failed to build [query_rule]") @@ -113,7 +109,6 @@ public void testNumericValidationWithMixedValues() throws IOException { IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - logger.info("Actual error message for mixed values: " + e.getMessage()); assertTrue( "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", e.getMessage().contains("Failed to build [query_rule]") @@ -136,7 +131,6 @@ public void testNumericValidationWithEmptyValues() throws IOException { IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - logger.info("Actual error message: " + e.getMessage()); assertTrue(e.getMessage().contains("failed to parse field [criteria]")); } @@ -168,7 +162,6 @@ public void testToXContentEmptyCriteria() throws IOException { IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - logger.info("Actual error message for empty criteria: " + e.getMessage()); assertTrue( "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", e.getMessage().contains("Failed to build [query_rule]") From d1070fdcb2ec7b0adb01477c6c60a30b3fad7035 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Tue, 4 Mar 2025 12:02:01 +0530 Subject: [PATCH 14/22] Cleaned up the QueryRule tests --- .../application/rules/QueryRuleTests.java | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 5b60f1ba050ff..d7bcb2b74d26f 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -83,14 +83,10 @@ public void testNumericValidationWithInvalidValues() throws IOException { "ids": ["id1"] } }"""); - IllegalArgumentException e = expectThrows( + expectThrows( IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - assertTrue( - "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", - e.getMessage().contains("Failed to build [query_rule]") - ); } public void testNumericValidationWithMixedValues() throws IOException { @@ -105,14 +101,10 @@ public void testNumericValidationWithMixedValues() throws IOException { "ids": ["id1"] } }"""); - IllegalArgumentException e = expectThrows( + expectThrows( IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - assertTrue( - "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", - e.getMessage().contains("Failed to build [query_rule]") - ); } public void testNumericValidationWithEmptyValues() throws IOException { @@ -127,11 +119,10 @@ public void testNumericValidationWithEmptyValues() throws IOException { "ids": ["id1"] } }"""); - IllegalArgumentException e = expectThrows( + expectThrows( IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - assertTrue(e.getMessage().contains("failed to parse field [criteria]")); } public void testToXContent() throws IOException { @@ -158,14 +149,10 @@ public void testToXContentEmptyCriteria() throws IOException { "criteria": [], "actions": {} }"""); - IllegalArgumentException e = expectThrows( + expectThrows( IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) ); - assertTrue( - "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", - e.getMessage().contains("Failed to build [query_rule]") - ); } public void testToXContentValidPinnedRulesWithIds() throws IOException { From a2f88509033a3cacc2dc5f19978a4a0d597e3085 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 4 Mar 2025 06:38:09 +0000 Subject: [PATCH 15/22] [CI] Auto commit changes from spotless --- .../application/rules/QueryRuleTests.java | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index d7bcb2b74d26f..261f4d4de84dd 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -83,10 +83,7 @@ public void testNumericValidationWithInvalidValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows( - IllegalArgumentException.class, - () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) - ); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } public void testNumericValidationWithMixedValues() throws IOException { @@ -101,10 +98,7 @@ public void testNumericValidationWithMixedValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows( - IllegalArgumentException.class, - () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) - ); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } public void testNumericValidationWithEmptyValues() throws IOException { @@ -119,10 +113,7 @@ public void testNumericValidationWithEmptyValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows( - IllegalArgumentException.class, - () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) - ); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } public void testToXContent() throws IOException { @@ -149,10 +140,7 @@ public void testToXContentEmptyCriteria() throws IOException { "criteria": [], "actions": {} }"""); - expectThrows( - IllegalArgumentException.class, - () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) - ); + expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); } public void testToXContentValidPinnedRulesWithIds() throws IOException { From ecc27e998a3a2170f3cf4ae0f023549a1afc4e88 Mon Sep 17 00:00:00 2001 From: Mridula Date: Tue, 11 Mar 2025 11:47:45 +0000 Subject: [PATCH 16/22] Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini --- .../EnterpriseSearchModuleTestUtils.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 15dfa21365071..8d31bdfd96367 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -99,28 +99,29 @@ public static QueryRule randomQueryRule() { return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority()); } - private static List generateRandomValues(QueryRuleCriteriaType criteriaType, int numValues) { + public static QueryRuleCriteria randomQueryRuleCriteria() { + Set numericValueCriteriaType = Set.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE); + QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); + + String metadata; + List values; if (criteriaType == QueryRuleCriteriaType.ALWAYS) { - return null; + metadata = null; + values = null; + } else { + metadata = randomAlphaOfLengthBetween(3, 10); + values = new ArrayList<>(); + + int numValues = randomIntBetween(1, 3); + for (int i = 0; i < numValues; i++) { + values.add( + numericValueCriteriaType.contains(criteriaType) + ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() + : randomAlphaOfLengthBetween(3, 10) + ); + } } - List values = new ArrayList<>(); - for (int i = 0; i < numValues; i++) { - values.add( - List.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE) - .contains(criteriaType) - ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() - : randomAlphaOfLengthBetween(3, 10) - ); - } - return values; - } - - public static QueryRuleCriteria randomQueryRuleCriteria() { - QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); - String metadata = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : randomAlphaOfLengthBetween(3, 10); - int numValues = randomIntBetween(1, 3); - List values = generateRandomValues(criteriaType, numValues); return new QueryRuleCriteria(criteriaType, metadata, values); } From 5b572f2d04e77d50afa2eb1f8bd12c4162001b16 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 11 Mar 2025 11:58:57 +0000 Subject: [PATCH 17/22] [CI] Auto commit changes from spotless --- .../EnterpriseSearchModuleTestUtils.java | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 8d31bdfd96367..389ec91916a88 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -100,26 +100,31 @@ public static QueryRule randomQueryRule() { } public static QueryRuleCriteria randomQueryRuleCriteria() { - Set numericValueCriteriaType = Set.of(QueryRuleCriteriaType.GT, QueryRuleCriteriaType.GTE, QueryRuleCriteriaType.LT, QueryRuleCriteriaType.LTE); + Set numericValueCriteriaType = Set.of( + QueryRuleCriteriaType.GT, + QueryRuleCriteriaType.GTE, + QueryRuleCriteriaType.LT, + QueryRuleCriteriaType.LTE + ); QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); - + String metadata; List values; if (criteriaType == QueryRuleCriteriaType.ALWAYS) { metadata = null; values = null; } else { - metadata = randomAlphaOfLengthBetween(3, 10); - values = new ArrayList<>(); - - int numValues = randomIntBetween(1, 3); - for (int i = 0; i < numValues; i++) { - values.add( - numericValueCriteriaType.contains(criteriaType) - ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() - : randomAlphaOfLengthBetween(3, 10) - ); - } + metadata = randomAlphaOfLengthBetween(3, 10); + values = new ArrayList<>(); + + int numValues = randomIntBetween(1, 3); + for (int i = 0; i < numValues; i++) { + values.add( + numericValueCriteriaType.contains(criteriaType) + ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString() + : randomAlphaOfLengthBetween(3, 10) + ); + } } return new QueryRuleCriteria(criteriaType, metadata, values); From e3b457789727856a544fbc92a233018dede98d08 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Mon, 3 Mar 2025 20:22:10 +0530 Subject: [PATCH 18/22] resolved merge conflicts # Conflicts: # x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java --- .../xpack/application/EnterpriseSearchModuleTestUtils.java | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 389ec91916a88..7e0d72968e139 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.function.Function; import static org.elasticsearch.test.ESTestCase.generateRandomStringArray; import static org.elasticsearch.test.ESTestCase.randomAlphaOfLengthBetween; From 73f5143c5da9d06767d02bad12775d46e95ddf0e Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Mon, 3 Mar 2025 21:06:09 +0530 Subject: [PATCH 19/22] Removed logger --- .../application/rules/QueryRuleTests.java | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 261f4d4de84dd..64ba17725cd50 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -36,6 +36,7 @@ import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.PREFIX; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.SUFFIX; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.containsString; public class QueryRuleTests extends ESTestCase { private NamedWriteableRegistry namedWriteableRegistry; @@ -83,7 +84,11 @@ public void testNumericValidationWithInvalidValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + assertThat(e.getMessage(), containsString("Failed to build [query_rule]")); } public void testNumericValidationWithMixedValues() throws IOException { @@ -98,7 +103,11 @@ public void testNumericValidationWithMixedValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + assertThat(e.getMessage(), containsString("Failed to build [query_rule]")); } public void testNumericValidationWithEmptyValues() throws IOException { @@ -113,7 +122,12 @@ public void testNumericValidationWithEmptyValues() throws IOException { "ids": ["id1"] } }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + logger.info("Actual error message: " + e.getMessage()); + assertTrue(e.getMessage().contains("failed to parse field [criteria]")); } public void testToXContent() throws IOException { @@ -140,7 +154,15 @@ public void testToXContentEmptyCriteria() throws IOException { "criteria": [], "actions": {} }"""); - expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON) + ); + logger.info("Actual error message for empty criteria: " + e.getMessage()); + assertTrue( + "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'", + e.getMessage().contains("Failed to build [query_rule]") + ); } public void testToXContentValidPinnedRulesWithIds() throws IOException { From f1dc548c14af04eaabc7d652d4a1e4d40f64b8e8 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Mon, 3 Mar 2025 21:09:58 +0530 Subject: [PATCH 20/22] Trigger PR update From 2691f2371ed63b829bfd5556460a4a185e1e382b Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Wed, 12 Mar 2025 10:58:07 +0000 Subject: [PATCH 21/22] Fixed checkstyle issues --- .../xpack/application/EnterpriseSearchModuleTestUtils.java | 1 - .../elasticsearch/xpack/application/rules/QueryRuleTests.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 7e0d72968e139..389ec91916a88 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.function.Function; import static org.elasticsearch.test.ESTestCase.generateRandomStringArray; import static org.elasticsearch.test.ESTestCase.randomAlphaOfLengthBetween; diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java index 64ba17725cd50..57e8adbdff764 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java @@ -35,8 +35,8 @@ import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.LTE; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.PREFIX; import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.SUFFIX; -import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.equalTo; public class QueryRuleTests extends ESTestCase { private NamedWriteableRegistry namedWriteableRegistry; From aa5293038737467fcc16b87a3ffaa74287dd5f31 Mon Sep 17 00:00:00 2001 From: Mridula Sivanandan Date: Wed, 12 Mar 2025 11:44:30 +0000 Subject: [PATCH 22/22] resolved compile errors --- .../xpack/application/EnterpriseSearchModuleTestUtils.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java index 389ec91916a88..4aa0e337a7568 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import static org.elasticsearch.test.ESTestCase.generateRandomStringArray; import static org.elasticsearch.test.ESTestCase.randomAlphaOfLengthBetween; @@ -109,7 +110,7 @@ public static QueryRuleCriteria randomQueryRuleCriteria() { QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values()); String metadata; - List values; + List values; if (criteriaType == QueryRuleCriteriaType.ALWAYS) { metadata = null; values = null;