[data.search] Add request handler context and asScoped pattern #80775

lukasolson · 2020-10-15T22:12:08Z

Summary

Depends on #80116.
Addresses #78461.

This PR changes the APIs we expose from our server-side data plugin search service. Before this PR, we were exposing a single search function from the start contract:

search(searchRequest: SearchStrategyRequest, options: ISearchOptions, context: RequestHandlerContext): Observable<SearchStrategyResponse>;

Passing context around was problematic because of two reasons:

It provided a poor contract which exposed everything on RequestContext to implementers of search strategies
It made it difficult to use outside of the scope of a route handler (see discussion in [Discuss] Removing RequestHandlerContext from the low-level data.search API. #78461)

This PR attempts to address these by providing two mechanisms of accessing the search method:

Via a newly registered route handler context:

router.post({}, async (context, request, res) => {
   const response = await context.search!.search(searchRequest, options).toPromise();
});

Via a new asScoped method in the data plugin start contract:

async function doSearch(request: KibanaRequest) {
  const response = await data.search.asScoped(request).search(searchRequest, options).toPromise();
}

It also updates the way we register custom search strategies. Instead of accepting a context inside of the search/cancel functions, you now receive SearchStrategyDependencies:

interface SearchStrategyDependencies {
  savedObjectsClient: SavedObjectsClientContract;
  esClient: IScopedClusterClient;
  uiSettingsClient: IUiSettingsClient;
}

In this way, we can control which dependencies are being passed/used in custom search strategies. If we need to add more along the way, we can, instead of relying implicitly on what's inside context.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-10-15T22:12:11Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lukasolson · 2020-10-15T22:20:40Z

src/plugins/data/common/search/es_search/types.ts

@@ -22,17 +22,6 @@ import { IKibanaSearchRequest, IKibanaSearchResponse } from '../types';

 export const ES_SEARCH_STRATEGY = 'es';

-export interface ISearchOptions {


Moved up to common/search/types

lukasolson · 2020-10-16T20:06:15Z

Skipping CI for now since we know it won't pass until #80116 is merged.

lukasolson · 2020-10-16T20:06:43Z

x-pack/plugins/security_solution/server/search_strategy/index_fields/index.ts

-          const indexPatternsFetcher = new IndexPatternsFetcher(
-            elasticsearch.legacy.client.callAsCurrentUser
-          );
+          const indexPatternsFetcher = new IndexPatternsFetcher(esClient.asCurrentUser);


This is the portion that relies on #80116.

lukeelmers

Overall approach makes sense to me! In particular I like the addition of SearchStrategyDependencies, this feels like a good way to limit the use of implicit dependencies in strategies to a known quantity.

Still need to do some testing, but in general this LGTM

lukeelmers · 2020-10-16T21:17:36Z

src/plugins/data/server/search/search_service.ts

    return {
      aggs: this.aggsService.start({ fieldFormats, uiSettings }),
      getSearchStrategy: this.getSearchStrategy,
-      search: this.search.bind(this),
+      asScoped: getSearchClientAsScoped,


<tangent>

I was thinking about whether this should be async, but I guess it doesn't have to be.

Was trying to remember why I made SearchSource.asScoped async -- turns out it is because index patterns contract is async, because field formats contract is async, because it calls await uiSettings.getAll()

</tangent>

lukeelmers · 2020-10-16T21:18:32Z

src/plugins/data/server/search/search_service.ts

-              const fakeRequestHandlerContext = {
-                core: {
-                  elasticsearch: {
-                    client: esClient,
-                  },
-                  uiSettings: {
-                    client: uiSettingsClient,
-                  },
-                },
-              } as RequestHandlerContext;


lukasolson · 2020-10-27T15:56:44Z

@elasticmachine merge master

lukasolson · 2020-10-29T22:38:15Z

I tested out the EQL search strategy and verified that it's still working as expected 👍 , but I'm going to defer approval for SecuritySolution code to someone from the Threat Hunting team as they wrote the search strategies that are failing on this branch. It appears as though those strategies use of an id conflicts with the async search functionality, causing a verification_exception to be returned from elasticsearch.

Yep, I've fixed this now. @XavierM we should sync up about how the ID parameter is being used in the security solution, as this interferes with the async search mechanism. Right now, the security search strategy is always using the open-source ES search strategy.

lukasolson · 2020-10-29T23:58:12Z

pinging @elastic/endpoint-app-team and @elastic/kibana-app for codeowner review, please and thank you!

lizozom

LGTM

alexwizp

LGTM

stratoula

KibanaApp changes LGTM

kibanamachine · 2020-11-03T17:36:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 11517ee

Metrics [docs]

page load bundle size

id	before	after	diff
`data`	954.7KB	954.7KB	-18.0B
`dataEnhanced`	27.9KB	28.0KB	+92.0B
total			+74.0B

History

💔 Build #85539 failed b0860e7
💚 Build #85286 succeeded f1b515d
💔 Build #85256 failed 1567995
💔 Build #85275 failed 5f7c9ab
💚 Build #84911 succeeded c35fb49

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

XavierM

Everything is working as expected on the security solutions! Thanks a lot

…ic#80775) * [Search] Add request context and asScoped pattern * Update docs * Unify interface for getting search client * Update examples/search_examples/server/my_strategy.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com> * Review feedback * Fix checks * Fix CI * Fix security search * Fix test * Fix test for reals * Fix types Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

… (#82648) * [Search] Add request context and asScoped pattern * Update docs * Unify interface for getting search client * Update examples/search_examples/server/my_strategy.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com> * Review feedback * Fix checks * Fix CI * Fix security search * Fix test * Fix test for reals * Fix types Co-authored-by: Anton Dosov <dosantappdev@gmail.com> Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

* master: (127 commits) [ILM] Fix breadcrumbs (elastic#82594) [UX]Swap env filter with percentile (elastic#82246) Add platform's missing READMEs (elastic#82268) [Discover] Adding uiMetric to track Visualize link click (elastic#82344) [Search] Add used index pattern name to the search agg error field (elastic#82604) improve client-side SO client get pooling (elastic#82603) [Security Solution] Unskips Overview tests (elastic#82459) Embeddables/migrations (elastic#82296) [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663) Moving reinstall function outside of promise.all (elastic#82672) Load choropleth layer correctly (elastic#82628) Master backport elastic#81233 (elastic#82642) [Fleet] Allow snake cased Kibana assets (elastic#77515) Reduce saved objects authorization checks (elastic#82204) [data.search] Add request handler context and asScoped pattern (elastic#80775) [ML] Fixes formatting of fields in index data visualizer (elastic#82593) Usage collector readme (elastic#82548) [Lens] Visualization validation and better error messages (elastic#81439) [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490) chore(NA): install microdnf in UBI docker build only (elastic#82611) ...

lukasolson added Feature:Search Querying infrastructure in Kibana refactoring v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 15, 2020

lukasolson requested review from XavierM, rylnd, lukeelmers, lizozom and alexwizp October 15, 2020 22:12

lukasolson requested a review from a team as a code owner October 15, 2020 22:12

lukasolson requested a review from a team October 15, 2020 22:12

lukasolson requested review from a team as code owners October 15, 2020 22:12

lukasolson self-assigned this Oct 15, 2020

[Search] Add request context and asScoped pattern

26c47bd

lukasolson commented Oct 15, 2020

View reviewed changes

Update docs

36b8fcb

lukasolson force-pushed the search-as-scoped branch from 218d5be to 36b8fcb Compare October 15, 2020 22:23

Merge branch 'master' into search-as-scoped

8274226

lukasolson changed the title ~~[data.search] Add request handler context and asScoped pattern~~ [skip-ci] [data.search] Add request handler context and asScoped pattern Oct 16, 2020

lukasolson changed the title ~~[skip-ci] [data.search] Add request handler context and asScoped pattern~~ [data.search] Add request handler context and asScoped pattern Oct 16, 2020

lukasolson changed the title ~~[data.search] Add request handler context and asScoped pattern~~ [skip-ci] [data.search] Add request handler context and asScoped pattern Oct 16, 2020

lukasolson commented Oct 16, 2020

View reviewed changes

lukeelmers reviewed Oct 16, 2020

View reviewed changes

lukasolson requested a review from Dosant October 19, 2020 15:13

Unify interface for getting search client

c7296f0

lukasolson requested a review from alexwizp October 26, 2020 20:43

lukasolson changed the title ~~[skip-ci] [data.search] Add request handler context and asScoped pattern~~ [data.search] Add request handler context and asScoped pattern Oct 27, 2020

lukasolson added 5 commits October 27, 2020 09:34

Merge branch 'master' into search-as-scoped

2d0da67

Merge branch 'master' into search-as-scoped

f7cb5a1

Fix checks

6c9aec5

Fix CI

1ae55ef

Fix security search

4634fb4

Merge branch 'master' into search-as-scoped

c35fb49

lizozom approved these changes Oct 30, 2020

View reviewed changes

alexwizp approved these changes Oct 30, 2020

View reviewed changes

stratoula approved these changes Nov 2, 2020

View reviewed changes

lukasolson added 3 commits November 2, 2020 09:27

Merge branch 'master' into search-as-scoped

1567995

Fix test

5f7c9ab

Fix test for reals

f1b515d

lukasolson mentioned this pull request Nov 2, 2020

[data.search] Server-side background session service #81099

Merged

8 tasks

lukasolson added 2 commits November 3, 2020 08:26

Merge branch 'master' into search-as-scoped

b0860e7

Fix types

11517ee

lukeelmers approved these changes Nov 3, 2020

View reviewed changes

lizozom requested a review from pgayvallet November 4, 2020 13:10

XavierM approved these changes Nov 4, 2020

View reviewed changes

lukasolson merged commit 44368b0 into elastic:master Nov 4, 2020

lukasolson mentioned this pull request Nov 4, 2020

[7.x] [data.search] Add request handler context and asScoped pattern (#80775) #82648

Merged

rylnd mentioned this pull request Nov 30, 2020

[Security Solution] Upgrade Apollo dependencies #73574

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.search] Add request handler context and asScoped pattern #80775

[data.search] Add request handler context and asScoped pattern #80775

lukasolson commented Oct 15, 2020

elasticmachine commented Oct 15, 2020

lukasolson Oct 15, 2020 •

edited

Loading

lukasolson commented Oct 16, 2020

lukasolson Oct 16, 2020

lukeelmers left a comment

lukeelmers Oct 16, 2020

lukeelmers Oct 16, 2020

lukasolson commented Oct 27, 2020

lukasolson commented Oct 29, 2020

lukasolson commented Oct 29, 2020

lizozom left a comment

alexwizp left a comment

stratoula left a comment

kibanamachine commented Nov 3, 2020

XavierM left a comment

		@@ -22,17 +22,6 @@ import { IKibanaSearchRequest, IKibanaSearchResponse } from '../types';

		export const ES_SEARCH_STRATEGY = 'es';

		export interface ISearchOptions {

[data.search] Add request handler context and asScoped pattern #80775

[data.search] Add request handler context and asScoped pattern #80775

Conversation

lukasolson commented Oct 15, 2020

Summary

Checklist

For maintainers

elasticmachine commented Oct 15, 2020

lukasolson Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

lukasolson commented Oct 16, 2020

lukasolson Oct 16, 2020

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Oct 16, 2020

Choose a reason for hiding this comment

lukeelmers Oct 16, 2020

Choose a reason for hiding this comment

lukasolson commented Oct 27, 2020

lukasolson commented Oct 29, 2020

lukasolson commented Oct 29, 2020

lizozom left a comment

Choose a reason for hiding this comment

alexwizp left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 3, 2020

💚 Build Succeeded

Metrics [docs]

page load bundle size

History

XavierM left a comment

Choose a reason for hiding this comment

lukasolson Oct 15, 2020 •

edited

Loading