Prevent Query Rule Creation with Invalid Numeric Match Criteria by mridula-s109 · Pull Request #122823 · elastic/elasticsearch

mridula-s109 · 2025-02-18T11:24:48Z

This PR introduces validation logic for query rules that require numeric comparisons to prevent storing rules that will never match. Previously, invalid numeric criteria could be silently ignored at query time, leading to unexpected behavior.

Added pre-validation in QueryRule.java to enforce numeric values for comparison rule types (lte, gte, lt, gt).
If a rule contains invalid values, the API will return an explicit validation error instead of silently accepting the rule.
Enhanced unit tests and YAML REST tests to verify validation errors.

…h invalid match criteria

kderusso

Nice start @mridula-s109 ! I've left some feedback, please let me know if you have questions.

...search/qa/rest/src/yamlRestTest/resources/rest-api-spec/api/entsearch.put_query_ruleset.json

...rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/55_numeric_validation.yml

...earch/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java

...lugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

mridula-s109 · 2025-02-20T09:31:13Z

Nice start @mridula-s109 ! I've left some feedback, please let me know if you have questions.

kderusso

Nice progress here @mridula-s109 , thanks for iterating!

I've left a couple of very minor comments.

The next thing that you'll want to do is:

Please update the title of this PR, to remove the Jira. It's OK to refer to the ticket number in the description (no links) but we don't want this being part of the official changelog as Jira is internal 🙂
Please remove this from draft state and apply the following labels:
- v9.0.0
- v8.18.0
- auto-backport
- :SearchOrg/Relevance
- >bug

This will automatically create a backport to the appropriate branch when it is merged, so the bug fix goes into 9.0. It will also create a Changelog entry for this bug fix. Finally, it will ping the Search organization that the PR is available.

While CI failed, it looks like it failed on an unrelated change. CI will be re-run any time anything is added - so creating the changelog will run again. You may need to update from main via the button on the PR to resolve. We can't merge this PR until CI is green.

Thanks for your work on this!

.../rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml

...n/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java

...earch/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java

...lugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

elasticsearchmachine · 2025-02-20T17:12:40Z

Pinging @elastic/search-eng (Team:SearchOrg)

elasticsearchmachine · 2025-02-20T17:12:41Z

Pinging @elastic/search-relevance (Team:Search - Relevance)

kderusso

LGTM, thanks for the iterations!

Mikep86

Thanks for removing the logger! I have a suggestion about how to simplify the test code :)

...earch/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java

…/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co>

# Conflicts: # x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

elasticsearchmachine · 2025-03-12T12:57:38Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122823

...lugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

…tic#122823) * SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria * [CI] Auto commit changes from spotless * Worked on the comments given in the PR * [CI] Auto commit changes from spotless * Fixed Integration tests * [CI] Auto commit changes from spotless * Made changes from the PR * Update docs/changelog/122823.yaml * [CI] Auto commit changes from spotless * Fixed the duplicate code issue in queryRuleTests * Refactored code to clean it up based on PR comments * [CI] Auto commit changes from spotless * Logger statements were removed * Cleaned up the QueryRule tests * [CI] Auto commit changes from spotless * Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co> * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co> Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co>

mridula-s109 · 2025-03-13T14:25:05Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…tic#122823) * SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria * [CI] Auto commit changes from spotless * Worked on the comments given in the PR * [CI] Auto commit changes from spotless * Fixed Integration tests * [CI] Auto commit changes from spotless * Made changes from the PR * Update docs/changelog/122823.yaml * [CI] Auto commit changes from spotless * Fixed the duplicate code issue in queryRuleTests * Refactored code to clean it up based on PR comments * [CI] Auto commit changes from spotless * Logger statements were removed * Cleaned up the QueryRule tests * [CI] Auto commit changes from spotless * Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co> * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co> Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co> (cherry picked from commit f6538e8) # Conflicts: # x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java

mridula-s109 · 2025-03-13T14:35:51Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…tic#122823) * SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria * [CI] Auto commit changes from spotless * Worked on the comments given in the PR * [CI] Auto commit changes from spotless * Fixed Integration tests * [CI] Auto commit changes from spotless * Made changes from the PR * Update docs/changelog/122823.yaml * [CI] Auto commit changes from spotless * Fixed the duplicate code issue in queryRuleTests * Refactored code to clean it up based on PR comments * [CI] Auto commit changes from spotless * Logger statements were removed * Cleaned up the QueryRule tests * [CI] Auto commit changes from spotless * Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co> * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co> Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co>

… Criteria (#122823) | Removed logger and also fixed the nitpick comments (#124650) (#124758) * Prevent Query Rule Creation with Invalid Numeric Match Criteria (#122823) * SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria * [CI] Auto commit changes from spotless * Worked on the comments given in the PR * [CI] Auto commit changes from spotless * Fixed Integration tests * [CI] Auto commit changes from spotless * Made changes from the PR * Update docs/changelog/122823.yaml * [CI] Auto commit changes from spotless * Fixed the duplicate code issue in queryRuleTests * Refactored code to clean it up based on PR comments * [CI] Auto commit changes from spotless * Logger statements were removed * Cleaned up the QueryRule tests * [CI] Auto commit changes from spotless * Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co> * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co> Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co> (cherry picked from commit f6538e8) # Conflicts: # x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java * Removed logger and also fixed the nitpick comments (#124650) (cherry picked from commit 44a3ac4) * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

SEARCH-802 - bug fixed - Query rules allows for creation of rules wit…

139c35b

…h invalid match criteria

mridula-s109 requested a review from kderusso February 18, 2025 11:24

Merge branch 'main' into numeric-bugfix

5e1a815

elasticsearchmachine added the v9.1.0 label Feb 18, 2025

elasticsearchmachine and others added 2 commits February 18, 2025 11:32

[CI] Auto commit changes from spotless

085cec0

Merge branch 'main' into numeric-bugfix

0240723

kderusso reviewed Feb 18, 2025

View reviewed changes

Worked on the comments given in the PR

da6c113

mridula-s109 closed this Feb 20, 2025

mridula-s109 reopened this Feb 20, 2025

elasticsearchmachine and others added 3 commits February 20, 2025 09:37

[CI] Auto commit changes from spotless

2161e1b

Fixed Integration tests

910cda4

[CI] Auto commit changes from spotless

da07f22

kderusso reviewed Feb 20, 2025

View reviewed changes

Merge branch 'main' into numeric-bugfix

1887bb0

mridula-s109 changed the title ~~[SEARCH-802] Prevent Query Rule Creation with Invalid Numeric Match Criteria~~ Feb 20, 2025

mridula-s109 added v9.0.0 v8.18.0 auto-backport Automatically create backport pull requests when merged :SearchOrg/Relevance Label for the Search (solution/org) Relevance team >bug labels Feb 20, 2025

Merge branch 'main' into numeric-bugfix

7f9399d

mridula-s109 marked this pull request as ready for review February 20, 2025 17:12

mridula-s109 enabled auto-merge (squash) February 20, 2025 17:12

mridula-s109 requested a review from a team February 20, 2025 17:12

mridula-s109 added 2 commits February 20, 2025 22:45

Made changes from the PR

3acf750

Update docs/changelog/122823.yaml

00571cd

Merge branch 'main' into numeric-bugfix

ede5605

mridula-s109 requested a review from kderusso March 4, 2025 06:39

kderusso approved these changes Mar 4, 2025

View reviewed changes

Mikep86 reviewed Mar 5, 2025

View reviewed changes

...earch/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java Outdated Show resolved Hide resolved

mridula-s109 and others added 6 commits March 11, 2025 11:47

Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack…

ecc27e9

…/application/EnterpriseSearchModuleTestUtils.java Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co>

Merge branch 'main' into numeric-bugfix

96e1fa4

[CI] Auto commit changes from spotless

5b572f2

Merge branch 'main' into numeric-bugfix

fc17b83

Merge branch 'main' into numeric-bugfix

c26535c

Merge branch 'main' into numeric-bugfix

5878c9c

mridula-s109 enabled auto-merge (squash) March 12, 2025 11:06

mridula-s109 added 5 commits March 12, 2025 11:15

resolved merge conflicts

e3b4577

# Conflicts: # x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

Removed logger

73f5143

Trigger PR update

f1dc548

Fixed checkstyle issues

2691f23

resolved compile errors

aa52930

mridula-s109 merged commit f6538e8 into elastic:main Mar 12, 2025
17 checks passed

elasticsearchmachine added the backport pending label Mar 12, 2025

Mikep86 reviewed Mar 12, 2025

View reviewed changes

mridula-s109 mentioned this pull request Mar 12, 2025

Follow-up: Remove Logger Calls in Tests #124650

Merged

mridula-s109 mentioned this pull request Mar 13, 2025

[8.x] Prevent Query Rule Creation with Invalid Numeric Match Criteria (#122823) #124756

Closed

mridula-s109 mentioned this pull request Mar 13, 2025

[Backport 8.x] Prevent Query Rule Creation with Invalid Numeric Match Criteria (#122823) | Removed logger and also fixed the nitpick comments (#124650) #124758

Merged

mridula-s109 self-assigned this Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent Query Rule Creation with Invalid Numeric Match Criteria#122823

Prevent Query Rule Creation with Invalid Numeric Match Criteria#122823
mridula-s109 merged 36 commits intoelastic:mainfrom
mridula-s109:numeric-bugfix

mridula-s109 commented Feb 18, 2025

kderusso left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mridula-s109 commented Feb 20, 2025

kderusso left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 20, 2025

elasticsearchmachine commented Feb 20, 2025

kderusso left a comment

Mikep86 left a comment

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 12, 2025

Uh oh!

Uh oh!

Uh oh!

mridula-s109 commented Mar 13, 2025

mridula-s109 commented Mar 13, 2025

Labels

5 participants

Conversation

mridula-s109 commented Feb 18, 2025

kderusso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mridula-s109 commented Feb 20, 2025

kderusso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 20, 2025

elasticsearchmachine commented Feb 20, 2025

kderusso left a comment

Choose a reason for hiding this comment

Mikep86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 12, 2025

💔 Backport failed

Uh oh!

Uh oh!

Uh oh!

mridula-s109 commented Mar 13, 2025

💚 All backports created successfully

Questions ?

mridula-s109 commented Mar 13, 2025

💚 All backports created successfully

Questions ?

Labels

5 participants

kderusso left a comment •

edited

Loading