Skip to content

Conversation

@ioanatia
Copy link
Contributor

closes #123429

when we retrieve the query ruleset, for each MultiGetItemResponse that represents a query rule, we need to explicitly check if retrieving the item failed.

@ioanatia ioanatia added >non-issue auto-backport Automatically create backport pull requests when merged :SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.0.0 v8.18.0 v8.19.0 labels Feb 27, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@Mikep86 Mikep86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for the failure case?

@ioanatia
Copy link
Contributor Author

ioanatia commented Mar 3, 2025

I am not sure how to add a test for this.
I looked into RuleQueryBuilderTests whether I can change the current simulateMethod, but it does not seem doable.
For one, simulateMethod is reused in tests - and also it does not receive an argument that would us differentiate when we should return a failed MultiGetItemResponse vs a valid query ruleset.

I also looked at SemanticQueryBuilderTests thinking that it will test for some of the exceptions that can be raised in SemanticQueryBuilder, like when we fail to retrieve the inference result or deal with multiple inference results.
I hoped that SemanticQueryBuilderTests would have a model on how to mock these calls to the inference service, but I couldn't find them.

Copy link
Member

@kderusso kderusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing!

- skip:
features: [ headers ]
- do:
headers:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick - we probably can omit the headers for this test

@ioanatia ioanatia merged commit 5011a47 into elastic:main Mar 3, 2025
17 checks passed
@ioanatia ioanatia deleted the query_rules_response_null_check branch March 3, 2025 18:21
ioanatia added a commit to ioanatia/elasticsearch that referenced this pull request Mar 3, 2025
ioanatia added a commit to ioanatia/elasticsearch that referenced this pull request Mar 3, 2025
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
9.0
8.18
8.x

ioanatia added a commit to ioanatia/elasticsearch that referenced this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >non-issue :SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v8.18.0 v8.19.0 v9.0.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RuleQuery Builder: NullPointerException in doRewrite()

4 participants