[data.search] Expose SearchSource on the server. #78383

lukeelmers · 2020-09-24T04:32:26Z

Summary

This is the final PR which exposes SearchSource on the server:

Refactors msearch route that's used in the legacy "default" search strategy so that the business logic is extracted into a helper function which can be reused in SearchSource
Adds contract to search service on the server
- API is await data.search.searchSource.asScoped(kibanaRequest);, which returns the same API as is available on the client.
Adds a plugin functional test suite to verify the service on the server

Testing

This PR relies on the added functional tests to validate that everything works, as there is not currently anywhere in Kibana which uses this service on the server.

Dev Docs

The high-level search API SearchSource is now available on the server. Usage:

function async myRouteHandler(context, request, response) {
  const searchSource = await data.search.searchSource.asScoped(request);
  searchSource.createEmpty(); // API after calling `asScoped` matches the client-side service
}

lukeelmers · 2020-09-24T04:36:39Z

src/plugins/data/server/search/routes/call_msearch.ts

+    if (params.signal) {
+      params.signal.addEventListener('abort', () => promise.abort());
+    }


@lukasolson Let me know if I got this abort logic right. I tested and AFAICT it all works as expected.

lukeelmers · 2020-09-24T04:37:18Z

src/plugins/data/server/search/routes/msearch.ts

+      const callMsearch = getCallMsearch({
+        esClient: context.core.elasticsearch.client,
+        globalConfig$: deps.globalConfig$,
+        uiSettings: context.core.uiSettings.client,
+      });


This got simplified to a helper which could also be called outside of a route handler

lukeelmers · 2020-09-24T04:38:03Z

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

+              /**
+               * Unless we want all SearchSource users to provide both a KibanaRequest
+               * (needed for index patterns) AND the RequestHandlerContext (needed for
+               * low-level search), we need to fake the context as it can be derived
+               * from the request object anyway. This will pose problems for folks who
+               * are registering custom search strategies as they are only getting a
+               * subset of the entire context. Ideally low-level search should be
+               * refactored to only require the needed dependencies: esClient & uiSettings.
+               */
+              const fakeRequestHandlerContext = {
+                core: {
+                  elasticsearch: {
+                    client: esClient,
+                  },
+                  uiSettings: {
+                    client: uiSettingsClient,
+                  },
+                },
+              } as RequestHandlerContext;
+              return this.search(fakeRequestHandlerContext, searchRequest, options);
+            },


@lukasolson @lizozom @ppisljar We should discuss the fakeRequestHandlerContext here -- I added it to our list to cover offline.

i am fine moving forward with this but lets open an issue for refactoring we want to do

Takeaway from our offline discussion: we will keep as-is for now as @ppisljar suggests, and discuss the proposed low-level search service changes separately.

Here's the issue for discussing changes to the low-level search API: #78461

lukeelmers · 2020-09-24T04:38:42Z

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

+                globalConfig$: this.initializerContext.config.legacy.globalConfig$,
+                uiSettings: uiSettingsClient,
+              }),
+              loadingCount$: new BehaviorSubject(0),


loadingCount$ isn't really necessary on the server, so we just initialize BehaviorSubject that never changes

lukeelmers · 2020-09-24T04:40:03Z

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

@@ -140,6 +152,58 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
      ) => {
        return this.search(context, searchRequest, options);
      },
+      searchSource: {
+        asScoped: async (request: KibanaRequest) => {


Accepting a KibanaRequest was pretty much my only option for the API since it's what index patterns require on the server. The question (as outlined in the code comment below) is whether it's necessary to also require RequestHandlerContext for the low-level search, when all of those items can already be derived from a KibanaRequest

lukeelmers · 2020-09-24T04:41:19Z

test/plugin_functional/plugins/data_search/server/plugin.ts

+    const router = core.http.createRouter();
+
+    router.get(
+      { path: '/api/data_search_plugin/search_source/as_scoped', validate: false },


These tests create a few dummy routes that can be called from the client to execute various pieces of the searchSource service on the server.

lukeelmers · 2020-09-24T04:41:53Z

test/plugin_functional/test_suites/data_plugin/index_patterns.ts

-    before(async () => {
-      await esArchiver.loadIfNeeded(
-        '../functional/fixtures/es_archiver/getting_started/shakespeare'
-      );
-      await PageObjects.common.navigateToApp('settings');
-      await PageObjects.settings.createIndexPattern('shakespeare', '');
-    });


relocated to top-level before that loads before both index patterns and search

elasticmachine · 2020-09-24T04:43:12Z

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

ppisljar

code LGTM

lukeelmers · 2020-09-26T01:05:51Z

@elasticmachine merge upstream

lukeelmers · 2020-09-26T01:06:11Z

@elasticmachine merge upstream

elasticmachine · 2020-09-26T01:06:12Z

merge conflict between base and head

kibanamachine · 2020-09-29T18:29:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5aebda3

Metrics [docs]

@kbn/optimizer bundle module count

id	value	diff	baseline
`data`	569	+1	568

distributable file count

id	value	diff	baseline
`default`	45826	+2	45824
`oss`	26576	+2	26574

page load bundle size

id	value	diff	baseline
`data`	1.3MB	+949.0B	1.3MB

History

💚 Build #77955 succeeded b75a84b481d038dfaf47a68b0139d353841ed7c7
💔 Build #77477 failed 5fca7111f6a6f165dafcac5a629f3388f0fb326a
💔 Build #77236 failed 7a0b5c0076aef2901c04ba8c105e5e92fe44fc0a
💚 Build #77000 succeeded f48dda8446ee51869bbd4e602e3fc500241eb19f
💚 Build #76807 succeeded 52036713a98533f5448b1c458da72e41198650fa

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

# Conflicts: # src/plugins/data/server/server.api.md

lizozom · 2020-09-30T09:19:06Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -108,7 +101,7 @@ export const searchSourceRequiredUiSettings = [
 ];

 export interface SearchSourceDependencies extends FetchHandlers {
-  search: ISearchGeneric;
+  search: (request: IEsSearchRequest, options: ISearchOptions) => Promise<IEsSearchResponse>;


@lukeelmers why did you do this change here?
ISearchGeneric is the exact equivalent of this more explicit typescript.

Was the intention here to limit the usage of SearchSource to DSL only?

It's not quite the exact equivalent -- ISearchGeneric returns an observable, however on the server, the low-level search returns a promise.

In order to ensure the same behavior of SearchSource on both client & server, I had to wrap the dependency into something that always returns a promise.

That said, I did not include the ability to provide generic type params like ISearchGeneric does, and probably should have. I will open a PR to update this.

(Also per our offline discussion, if low-level search on the server eventually moves to returning an observable as it does on the client, we can come back and adjust this)

PR adding the generic type params: #79608

lukeelmers self-assigned this Sep 24, 2020

lukeelmers added Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. release_note:enhancement v7.10.0 v8.0.0 review labels Sep 24, 2020

lukeelmers commented Sep 24, 2020

View reviewed changes

lukeelmers requested review from lizozom, ppisljar and lukasolson September 24, 2020 04:42

lukeelmers added the Team:AppArch label Sep 24, 2020

lukeelmers marked this pull request as ready for review September 24, 2020 04:43

lukeelmers requested a review from a team as a code owner September 24, 2020 04:43

lukeelmers mentioned this pull request Sep 24, 2020

[data.search.searchSource]: Expose SearchSource on server #65069

Closed

28 tasks

ppisljar approved these changes Sep 24, 2020

View reviewed changes

lukeelmers force-pushed the feat/ssource-server branch from 5203671 to f48dda8 Compare September 24, 2020 16:15

lukeelmers mentioned this pull request Sep 24, 2020

[Discuss] Removing RequestHandlerContext from the low-level data.search API. #78461

Closed

lukeelmers force-pushed the feat/ssource-server branch 2 times, most recently from 5fca711 to b75a84b Compare September 29, 2020 16:23

lukeelmers added 6 commits September 29, 2020 10:58

Add common SearchSourceService.

9f1f162

Move logic for msearch route into separate helper.

2f33854

Add unit tests for injectReferences.

a797855

Expose SearchSource from server.

461bc0b

Add plugin functional tests.

f252234

Update generated docs.

5aebda3

lukeelmers force-pushed the feat/ssource-server branch from b75a84b to 5aebda3 Compare September 29, 2020 17:00

lukeelmers merged commit ccb789d into elastic:master Sep 29, 2020

lukeelmers deleted the feat/ssource-server branch September 29, 2020 18:38

lukeelmers mentioned this pull request Sep 29, 2020

[7.x] [data.search] Expose SearchSource on the server. (#78383) #78891

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Sep 29, 2020

[data.search] Expose SearchSource on the server. (elastic#78383)

10a9b6b

# Conflicts: # src/plugins/data/server/server.api.md

lukeelmers added a commit that referenced this pull request Sep 30, 2020

[data.search] Expose SearchSource on the server. (#78383) (#78891)

a95cdeb

# Conflicts: # src/plugins/data/server/server.api.md

lizozom reviewed Sep 30, 2020

View reviewed changes

lukeelmers mentioned this pull request Oct 6, 2020

[data.search.searchSource] Add generic type params to search dependency. #79608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.search] Expose SearchSource on the server. #78383

[data.search] Expose SearchSource on the server. #78383

lukeelmers commented Sep 24, 2020 •

edited

Loading

lukeelmers Sep 24, 2020

lukeelmers Sep 24, 2020

lukeelmers Sep 24, 2020

ppisljar Sep 24, 2020

lukeelmers Sep 24, 2020

lukeelmers Sep 24, 2020

lukeelmers Sep 24, 2020

lukeelmers Sep 24, 2020 •

edited

Loading

lukeelmers Sep 24, 2020

lukeelmers Sep 24, 2020

elasticmachine commented Sep 24, 2020

ppisljar left a comment

lukeelmers commented Sep 26, 2020

lukeelmers commented Sep 26, 2020

elasticmachine commented Sep 26, 2020

kibanamachine commented Sep 29, 2020

lizozom Sep 30, 2020 •

edited

Loading

lukeelmers Sep 30, 2020

lukeelmers Sep 30, 2020

lukeelmers Oct 6, 2020

[data.search] Expose SearchSource on the server. #78383

[data.search] Expose SearchSource on the server. #78383

Conversation

lukeelmers commented Sep 24, 2020 • edited Loading

Summary

Testing

Dev Docs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 24, 2020

ppisljar left a comment

Choose a reason for hiding this comment

lukeelmers commented Sep 26, 2020

lukeelmers commented Sep 26, 2020

elasticmachine commented Sep 26, 2020

kibanamachine commented Sep 29, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

distributable file count

page load bundle size

History

lizozom Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers commented Sep 24, 2020 •

edited

Loading

lukeelmers Sep 24, 2020 •

edited

Loading

lizozom Sep 30, 2020 •

edited

Loading