Skip to content

Commit 139c35b

Browse files
mridula-s109mridula-s109
committed
SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria
1 parent 0534074 commit 139c35b

File tree

6 files changed

+444
-9
lines changed

6 files changed

+444
-9
lines changed

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
{
2+
"entsearch.put_query_ruleset": {
3+
"documentation": {
4+
"description": "Creates or updates a query ruleset",
5+
"url": "..."
6+
},
7+
"stability": "stable",
8+
"visibility": "public",
9+
"headers": {
10+
"accept": ["application/json"],
11+
"content_type": ["application/json"]
12+
},
13+
"url": {
14+
"paths": [
15+
{
16+
"path": "/_query_rules/ruleset/{ruleset_id}",
17+
"methods": ["PUT"],
18+
"parts": {
19+
"ruleset_id": {
20+
"type": "string",
21+
"description": "The unique identifier of the ruleset"
22+
}
23+
}
24+
}
25+
]
26+
},
27+
"body": {
28+
"description": "The query ruleset to create or update",
29+
"required": true
30+
}
31+
}
32+
}

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

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
---
2+
"Test numeric comparison rule validation":
3+
- skip:
4+
features: headers
5+
6+
- do:
7+
indices.create:
8+
index: test-index
9+
body:
10+
settings:
11+
number_of_shards: 1
12+
number_of_replicas: 0
13+
14+
- do:
15+
query_rules.put_ruleset:
16+
ruleset_id: numeric_validation_test
17+
body:
18+
rules:
19+
- rule_id: valid_numeric_rule
20+
type: pinned
21+
criteria:
22+
- type: lte
23+
metadata: price
24+
values: ["100"]
25+
actions:
26+
ids: ["1"]
27+
28+
- match: { result: "created" }
29+
30+
- do:
31+
catch: bad_request
32+
query_rules.put_ruleset:
33+
ruleset_id: invalid_numeric_test
34+
body:
35+
rules:
36+
- rule_id: invalid_numeric_rule
37+
type: pinned
38+
criteria:
39+
- type: lte
40+
metadata: price
41+
values: ["not_a_number"]
42+
actions:
43+
ids: ["1"]
44+
45+
- match: { error.type: "x_content_parse_exception" }
46+
- match: { error.reason: /.*/ }
47+
48+
---
49+
"Test multiple numeric comparison rules":
50+
- do:
51+
query_rules.put_ruleset:
52+
ruleset_id: multiple_numeric_test
53+
body:
54+
rules:
55+
- rule_id: multiple_numeric_rule
56+
type: pinned
57+
criteria:
58+
- type: gte
59+
metadata: price
60+
values: ["50"]
61+
- type: lte
62+
metadata: price
63+
values: ["100"]
64+
actions:
65+
ids: ["1"]
66+
67+
- match: { result: "created" }
68+
69+
---
70+
"Test mixed numeric and non-numeric rules":
71+
- do:
72+
query_rules.put_ruleset:
73+
ruleset_id: mixed_rules_test
74+
body:
75+
rules:
76+
- rule_id: mixed_rule
77+
type: pinned
78+
criteria:
79+
- type: lte
80+
metadata: price
81+
values: ["100"]
82+
- type: exact
83+
metadata: category
84+
values: ["electronics"]
85+
actions:
86+
ids: ["1"]
87+
88+
- match: { result: "created" }
89+
90+
---
91+
"Test numeric comparison with multiple values":
92+
- do:
93+
catch: bad_request
94+
query_rules.put_ruleset:
95+
ruleset_id: multiple_values_test
96+
body:
97+
rules:
98+
- rule_id: multiple_values_rule
99+
type: pinned
100+
criteria:
101+
- type: lte
102+
metadata: price
103+
values: ["100", "not_a_number"]
104+
actions:
105+
ids: ["1"]
106+
107+
- match: { error.type: "x_content_parse_exception" }
108+
- match: { error.reason: /.*/ }
109+
110+
---
111+
teardown:
112+
- do:
113+
indices.delete:
114+
index: test-index
115+
ignore: 404
116+
- do:
117+
query_rules.delete_ruleset:
118+
ruleset_id: numeric_validation_test
119+
ignore: 404
120+
- do:
121+
query_rules.delete_ruleset:
122+
ruleset_id: invalid_numeric_test
123+
ignore: 404
124+
- do:
125+
query_rules.delete_ruleset:
126+
ruleset_id: multiple_numeric_test
127+
ignore: 404
128+
- do:
129+
query_rules.delete_ruleset:
130+
ruleset_id: mixed_rules_test
131+
ignore: 404
132+
- do:
133+
query_rules.delete_ruleset:
134+
ruleset_id: multiple_values_test
135+
ignore: 404
136+
137+
138+

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,6 @@ public QueryRule(StreamInput in) throws IOException {
140140
}
141141

142142
private void validate() {
143-
144143
if (priority != null && (priority < MIN_PRIORITY || priority > MAX_PRIORITY)) {
145144
throw new IllegalArgumentException("Priority was " + priority + ", must be between " + MIN_PRIORITY + " and " + MAX_PRIORITY);
146145
}
@@ -155,6 +154,23 @@ private void validate() {
155154
throw new ElasticsearchParseException(type.toString() + " query rule actions must contain only one of either ids or docs");
156155
}
157156
}
157+
158+
for (QueryRuleCriteria criterion : criteria) {
159+
QueryRuleCriteriaType criteriaType = criterion.criteriaType();
160+
if (criteriaType.isNumericComparison()) {
161+
List<Object> values = criterion.criteriaValues();
162+
for (Object value : values) {
163+
try {
164+
Double.parseDouble(value.toString());
165+
} catch (NumberFormatException e) {
166+
throw new IllegalArgumentException(
167+
"Numeric comparison rule type " + criteriaType + " requires numeric values, but got " + value +
168+
" for criterion " + criterion
169+
);
170+
}
171+
}
172+
}
173+
}
158174
}
159175

160176
private void validateIdOrDocAction(Object action) {

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,4 +135,8 @@ private boolean isValidForInput(Object input) {
135135
private static double parseDouble(Object input) {
136136
return (input instanceof Number) ? ((Number) input).doubleValue() : Double.parseDouble(input.toString());
137137
}
138+
139+
public boolean isNumericComparison() {
140+
return this == GT || this == GTE || this == LT || this == LTE;
141+
}
138142
}

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

Lines changed: 93 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@
3939
import static org.elasticsearch.xpack.application.rules.QueryRule.MAX_PRIORITY;
4040
import static org.elasticsearch.xpack.application.rules.QueryRule.MIN_PRIORITY;
4141
import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.ALWAYS;
42+
import static org.elasticsearch.test.ESTestCase.randomAlphaOfLength;
43+
import static org.elasticsearch.test.ESTestCase.randomDoubleBetween;
4244

4345
public final class EnterpriseSearchModuleTestUtils {
4446

@@ -87,18 +89,79 @@ public static Map<String, Object> randomSearchApplicationQueryParams() {
8789
}
8890

8991
public static QueryRuleCriteria randomQueryRuleCriteria() {
90-
// We intentionally don't allow ALWAYS criteria in this method, since we want to test parsing metadata and values
91-
QueryRuleCriteriaType type = randomFrom(Arrays.stream(QueryRuleCriteriaType.values()).filter(t -> t != ALWAYS).toList());
92-
return new QueryRuleCriteria(type, randomAlphaOfLengthBetween(1, 10), randomList(1, 5, () -> randomAlphaOfLengthBetween(1, 10)));
92+
QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values());
93+
String metadata = randomAlphaOfLength(10);
94+
List<Object> values = new ArrayList<>();
95+
int numValues = randomIntBetween(1, 3);
96+
97+
if (criteriaType.isNumericComparison()) {
98+
for (int i = 0; i < numValues; i++) {
99+
double value = Math.round(randomDoubleBetween(0, 1000, true) * 100.0) / 100.0;
100+
values.add(String.valueOf(value));
101+
}
102+
} else if (criteriaType == QueryRuleCriteriaType.ALWAYS) {
103+
metadata = null;
104+
values = null;
105+
} else {
106+
for (int i = 0; i < numValues; i++) {
107+
values.add(randomAlphaOfLength(5));
108+
}
109+
}
110+
111+
return new QueryRuleCriteria(criteriaType, metadata, values);
93112
}
94113

95114
public static QueryRule randomQueryRule() {
96-
String id = randomIdentifier();
115+
if (randomBoolean()) {
116+
return createNumericQueryRule();
117+
} else {
118+
return createNonNumericQueryRule();
119+
}
120+
}
121+
122+
private static QueryRule createNumericQueryRule() {
123+
String ruleId = randomAlphaOfLength(10);
124+
QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
125+
List<QueryRuleCriteria> criteria = new ArrayList<>();
126+
int numCriteria = randomIntBetween(1, 3);
127+
128+
for (int i = 0; i < numCriteria; i++) {
129+
QueryRuleCriteriaType criteriaType = randomFrom(
130+
QueryRuleCriteriaType.GT,
131+
QueryRuleCriteriaType.GTE,
132+
QueryRuleCriteriaType.LT,
133+
QueryRuleCriteriaType.LTE
134+
);
135+
double value = Math.round(randomDoubleBetween(0, 1000, true) * 100.0) / 100.0;
136+
criteria.add(new QueryRuleCriteria(criteriaType, randomAlphaOfLength(10), List.of(String.valueOf(value))));
137+
}
138+
139+
return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority());
140+
}
141+
142+
private static QueryRule createNonNumericQueryRule() {
143+
String ruleId = randomAlphaOfLength(10);
97144
QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
98-
List<QueryRuleCriteria> criteria = List.of(randomQueryRuleCriteria());
99-
Map<String, Object> actions = Map.of(randomFrom("ids", "docs"), List.of(randomAlphaOfLengthBetween(2, 10)));
100-
Integer priority = randomQueryRulePriority();
101-
return new QueryRule(id, type, criteria, actions, priority);
145+
List<QueryRuleCriteria> criteria = new ArrayList<>();
146+
int numCriteria = randomIntBetween(1, 3);
147+
148+
for (int i = 0; i < numCriteria; i++) {
149+
QueryRuleCriteriaType criteriaType = randomFrom(
150+
QueryRuleCriteriaType.EXACT,
151+
QueryRuleCriteriaType.PREFIX,
152+
QueryRuleCriteriaType.CONTAINS,
153+
QueryRuleCriteriaType.ALWAYS
154+
);
155+
156+
String metadata = criteriaType == QueryRuleCriteriaType.ALWAYS ? null : randomAlphaOfLength(10);
157+
List<Object> values = criteriaType == QueryRuleCriteriaType.ALWAYS ?
158+
null :
159+
List.of(randomAlphaOfLength(5));
160+
161+
criteria.add(new QueryRuleCriteria(criteriaType, metadata, values));
162+
}
163+
164+
return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority());
102165
}
103166

104167
public static Integer randomQueryRulePriority() {
@@ -119,4 +182,26 @@ public static Map<String, Object> randomMatchCriteria() {
119182
return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10)));
120183
}
121184

185+
public static Map<String, Object> randomQueryRuleActions() {
186+
QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
187+
if (type == QueryRule.QueryRuleType.PINNED) {
188+
// For PINNED type, we need either 'ids' or 'docs', but not both
189+
if (randomBoolean()) {
190+
return Map.of("ids", List.of(randomAlphaOfLength(5)));
191+
} else {
192+
return Map.of("docs", List.of(Map.of("_index", randomAlphaOfLength(5), "_id", randomAlphaOfLength(5))));
193+
}
194+
} else if (type == QueryRule.QueryRuleType.EXCLUDE) {
195+
// For EXCLUDE type, similar to PINNED
196+
if (randomBoolean()) {
197+
return Map.of("ids", List.of(randomAlphaOfLength(5)));
198+
} else {
199+
return Map.of("docs", List.of(Map.of("_index", randomAlphaOfLength(5), "_id", randomAlphaOfLength(5))));
200+
}
201+
} else {
202+
// For other types (like BOOST), use a boost value
203+
return Map.of("boost", randomDoubleBetween(0.0, 5.0, true));
204+
}
205+
}
206+
122207
}

0 commit comments

Comments
 (0)