Add a query rules tester API call #114168

kderusso · 2024-10-04T19:50:57Z

Adds a tester call to the query ruleset API.

Example call:

POST _query_rules/my-ruleset/_test
{
  "match_criteria": {
    "foo": "bar"
  }
}

Example response:

{
  "total_matched_rules": 1,
  "matched_rules": [
    { 
      "ruleset_id": "my-ruleset", 
      "rule_id": "my-rule" 
    }
  ]
}

elasticsearchmachine · 2024-10-08T18:34:58Z

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

elasticsearchmachine · 2024-10-08T18:34:58Z

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

elasticsearchmachine · 2024-10-08T18:34:59Z

Hi @kderusso, I've created a changelog YAML for you.

Samiul-TheSoccerFan

LGTM

...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/70_query_rule_test.yml

ioanatia · 2024-10-10T11:07:29Z

...n/java/org/elasticsearch/xpack/application/rules/action/TransportTestQueryRulesetAction.java

+    @Override
+    protected void doExecute(Task task, TestQueryRulesetAction.Request request, ActionListener<TestQueryRulesetAction.Response> listener) {
+        GetQueryRulesetAction.Request getQueryRulesetRequest = new GetQueryRulesetAction.Request(request.rulesetId());
+        originSettingClient.execute(GetQueryRulesetAction.INSTANCE, getQueryRulesetRequest, new ActionListener<>() {


If we ever want to open this API to not require the manage_search_query_rules privilege like we discussed, if changing this similarly to #105667 to use executeAsyncWithOrigin would be the solution

Thanks, I will look into that!

Believe it or not, that doesn't work either!

I updated the call to use executeAsyncWithOrigin and I still get yaml tests failing with the following error:

- type: "security_exception" reason: "action [indices:data/read/xpack/query_rules/test] is unauthorized for\ \ user [entsearch-admin] with effective roles [admin] on restricted indices\ \ [.query-rules], this action is granted by the index privileges [read,all]"

I am stumped why this doesn't work, as it definitely does when we use the rule query. I'm inclined to punt and figure it out later before we GA the call, just to get something out there. That would require us keeping the manage query rules privilege for now. What are your thoughts?

ioanatia · 2024-10-10T11:10:02Z

...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/70_query_rule_test.yml

+
+  - do:
+      catch: forbidden
+      headers: { Authorization: "Basic ZW50c2VhcmNoLXVzZXI6ZW50c2VhcmNoLXVzZXItcGFzc3dvcmQ=" }  # user


Is this checking that we are requiring the manage_search_query_rules privilege?

Correct, that's the permission we're using right now.

ioanatia · 2024-10-10T11:12:31Z

docs/reference/query-rules/apis/index.asciidoc

 include::put-query-rule.asciidoc[]
 include::get-query-rule.asciidoc[]
 include::delete-query-rule.asciidoc[]
+include::test-query-ruleset.asciidoc[]


are there other docs/guides where we want to refer to this API as a way to test/troubleshoot a query rules set? (can be done as a follow up)

I'd like to do that as a followup. This is being launched in preview right now, because there may be evolving requirements. I think when it gets closer to beta or GA is when it would be most helpful to promote this more, WDYT?

ioanatia · 2024-10-10T11:16:44Z

...h/src/main/java/org/elasticsearch/xpack/application/rules/action/TestQueryRulesetAction.java

+        private final int totalMatchedRules;
+        private final List<MatchedRule> matchedRules;
+
+        private static final ParseField TOTAL_MATCHED_RULES_FIELD = new ParseField("total_matched_rules");


Why do we need a total_matched_rules in the response? could we just return the matched_rules? just trying to understand

We could just return the matched rules. I was thinking it could be convenient in the event there were 1000 rules returned, or we want to provide pagination in the future.

++ for keeping total_matched_rules

Mikep86

Looks good overall, great job 🙌 ! I only have minor comments.

...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/70_query_rule_test.yml

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

...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/70_query_rule_test.yml

Mikep86 · 2024-10-10T17:32:14Z

...h/src/main/java/org/elasticsearch/xpack/application/rules/action/TestQueryRulesetAction.java

+        private final int totalMatchedRules;
+        private final List<MatchedRule> matchedRules;
+
+        private static final ParseField TOTAL_MATCHED_RULES_FIELD = new ParseField("total_matched_rules");


++ for keeping total_matched_rules

...h/src/main/java/org/elasticsearch/xpack/application/rules/action/TestQueryRulesetAction.java

...csearch/xpack/application/rules/action/TestQueryRulesetActionRequestBWCSerializingTests.java

…eatures

Mikep86

LGTM, thanks for the changes!

elasticsearchmachine · 2024-10-14T16:56:38Z

💚 Backport successful

Status	Branch	Result
✅	8.x

* Add a query rules tester API call * Update docs/changelog/114168.yaml * Wrap client call in async with origin * Remove unused param * PR feedback * Remove redundant test * CI workaround - add ent-search as ml dependency so it can find node features

elasticsearchmachine added the v9.0.0 label Oct 4, 2024

kderusso added auto-backport Automatically create backport pull requests when merged v8.16.0 labels Oct 4, 2024

kderusso force-pushed the kderusso/query-rules-tester-api branch 2 times, most recently from c73a6bf to 3613634 Compare October 8, 2024 18:17

Add a query rules tester API call

df6dfa8

kderusso force-pushed the kderusso/query-rules-tester-api branch from 3613634 to df6dfa8 Compare October 8, 2024 18:30

kderusso marked this pull request as ready for review October 8, 2024 18:31

kderusso requested review from a team and ioanatia October 8, 2024 18:31

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Oct 8, 2024

kderusso added :SearchOrg/Relevance Label for the Search (solution/org) Relevance team >enhancement and removed needs:triage Requires assignment of a team area label labels Oct 8, 2024

Update docs/changelog/114168.yaml

1bc085b

Samiul-TheSoccerFan approved these changes Oct 10, 2024

View reviewed changes

...qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/70_query_rule_test.yml Show resolved Hide resolved

ioanatia reviewed Oct 10, 2024

View reviewed changes

kderusso added 2 commits October 10, 2024 11:31

Wrap client call in async with origin

2d2c24b

Remove unused param

e46ae97

Mikep86 reviewed Oct 10, 2024

View reviewed changes

kderusso added 4 commits October 10, 2024 16:13

PR feedback

339297b

Merge from main

9784406

Remove redundant test

c5fa25e

CI workaround - add ent-search as ml dependency so it can find node f…

ba4708f

…eatures

kderusso requested a review from a team as a code owner October 11, 2024 19:57

Merge from main

0bf2bfd

kderusso requested a review from ioanatia October 14, 2024 12:27

kderusso requested review from Mikep86 and Samiul-TheSoccerFan October 14, 2024 12:27

Mikep86 approved these changes Oct 14, 2024

View reviewed changes

kderusso merged commit 69b4a9f into elastic:main Oct 14, 2024
16 checks passed

kderusso deleted the kderusso/query-rules-tester-api branch October 14, 2024 16:55

kderusso mentioned this pull request Oct 14, 2024

[8.x] Add a query rules tester API call (#114168) #114747

Merged

kderusso mentioned this pull request Oct 18, 2024

Add specification for query rule test API elastic/elasticsearch-specification#3029

Merged

Add a query rules tester API call #114168

Add a query rules tester API call #114168

Uh oh!

Conversation

kderusso commented Oct 4, 2024

Uh oh!

elasticsearchmachine commented Oct 8, 2024

Uh oh!

elasticsearchmachine commented Oct 8, 2024

Uh oh!

elasticsearchmachine commented Oct 8, 2024

Uh oh!

Samiul-TheSoccerFan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikep86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mikep86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 14, 2024

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants