From 6188c742862a8a5db205c06fc35fde4134e2959e Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Wed, 29 Jan 2020 12:26:23 -0700 Subject: [PATCH 1/2] Move loadingCount to search strategy --- .../create_app_mount_context_search.test.ts | 50 ++++++++----------- .../search/create_app_mount_context_search.ts | 19 ++----- .../public/search/es_client/get_es_client.ts | 6 ++- .../data/public/search/search_service.ts | 10 +--- .../public/search/sync_search_strategy.ts | 16 +++--- 5 files changed, 41 insertions(+), 60 deletions(-) diff --git a/src/plugins/data/public/search/create_app_mount_context_search.test.ts b/src/plugins/data/public/search/create_app_mount_context_search.test.ts index 15b85ee270bed..fa7cdbcda3082 100644 --- a/src/plugins/data/public/search/create_app_mount_context_search.test.ts +++ b/src/plugins/data/public/search/create_app_mount_context_search.test.ts @@ -18,35 +18,32 @@ */ import { createAppMountSearchContext } from './create_app_mount_context_search'; -import { from, BehaviorSubject } from 'rxjs'; +import { from } from 'rxjs'; describe('Create app mount search context', () => { it('Returns search fn when there are no strategies', () => { - const context = createAppMountSearchContext({}, new BehaviorSubject(0)); + const context = createAppMountSearchContext({}); expect(context.search).toBeDefined(); }); it(`Search throws an error when the strategy doesn't exist`, () => { - const context = createAppMountSearchContext({}, new BehaviorSubject(0)); + const context = createAppMountSearchContext({}); expect(() => context.search({}, {}, 'noexist').toPromise()).toThrowErrorMatchingInlineSnapshot( `"Strategy with name noexist does not exist"` ); }); it(`Search fn is called on appropriate strategy name`, done => { - const context = createAppMountSearchContext( - { - mysearch: search => - Promise.resolve({ - search: () => from(Promise.resolve({ percentComplete: 98 })), - }), - anothersearch: search => - Promise.resolve({ - search: () => from(Promise.resolve({ percentComplete: 0 })), - }), - }, - new BehaviorSubject(0) - ); + const context = createAppMountSearchContext({ + mysearch: search => + Promise.resolve({ + search: () => from(Promise.resolve({ percentComplete: 98 })), + }), + anothersearch: search => + Promise.resolve({ + search: () => from(Promise.resolve({ percentComplete: 0 })), + }), + }); context.search({}, {}, 'mysearch').subscribe(response => { expect(response).toEqual({ percentComplete: 98 }); @@ -55,19 +52,16 @@ describe('Create app mount search context', () => { }); it(`Search fn is called with the passed in request object`, done => { - const context = createAppMountSearchContext( - { - mysearch: search => { - return Promise.resolve({ - search: request => { - expect(request).toEqual({ greeting: 'hi' }); - return from(Promise.resolve({})); - }, - }); - }, + const context = createAppMountSearchContext({ + mysearch: search => { + return Promise.resolve({ + search: request => { + expect(request).toEqual({ greeting: 'hi' }); + return from(Promise.resolve({})); + }, + }); }, - new BehaviorSubject(0) - ); + }); context.search({ greeting: 'hi' } as any, {}, 'mysearch').subscribe( response => {}, () => {}, diff --git a/src/plugins/data/public/search/create_app_mount_context_search.ts b/src/plugins/data/public/search/create_app_mount_context_search.ts index f480b8f3e042e..7a617e0bab837 100644 --- a/src/plugins/data/public/search/create_app_mount_context_search.ts +++ b/src/plugins/data/public/search/create_app_mount_context_search.ts @@ -17,8 +17,8 @@ * under the License. */ -import { mergeMap, tap } from 'rxjs/operators'; -import { from, BehaviorSubject } from 'rxjs'; +import { mergeMap } from 'rxjs/operators'; +import { from } from 'rxjs'; import { ISearchAppMountContext } from './i_search_app_mount_context'; import { ISearchGeneric } from './i_search'; import { @@ -30,8 +30,7 @@ import { TStrategyTypes } from './strategy_types'; import { DEFAULT_SEARCH_STRATEGY } from '../../common/search'; export const createAppMountSearchContext = ( - searchStrategies: TSearchStrategiesMap, - loadingCount$: BehaviorSubject + searchStrategies: TSearchStrategiesMap ): ISearchAppMountContext => { const getSearchStrategy = ( strategyName?: K @@ -47,17 +46,7 @@ export const createAppMountSearchContext = ( const search: ISearchGeneric = (request, options, strategyName) => { const strategyPromise = getSearchStrategy(strategyName); - return from(strategyPromise).pipe( - mergeMap(strategy => { - loadingCount$.next(loadingCount$.getValue() + 1); - return strategy.search(request, options).pipe( - tap( - error => loadingCount$.next(loadingCount$.getValue() - 1), - complete => loadingCount$.next(loadingCount$.getValue() - 1) - ) - ); - }) - ); + return from(strategyPromise).pipe(mergeMap(strategy => strategy.search(request, options))); }; return { search }; diff --git a/src/plugins/data/public/search/es_client/get_es_client.ts b/src/plugins/data/public/search/es_client/get_es_client.ts index 6c271643ba012..93d9d24920271 100644 --- a/src/plugins/data/public/search/es_client/get_es_client.ts +++ b/src/plugins/data/public/search/es_client/get_es_client.ts @@ -25,8 +25,7 @@ import { BehaviorSubject } from 'rxjs'; export function getEsClient( injectedMetadata: CoreStart['injectedMetadata'], http: CoreStart['http'], - packageInfo: PackageInfo, - loadingCount$: BehaviorSubject + packageInfo: PackageInfo ) { const esRequestTimeout = injectedMetadata.getInjectedVar('esRequestTimeout') as number; const esApiVersion = injectedMetadata.getInjectedVar('esApiVersion') as string; @@ -39,6 +38,9 @@ export function getEsClient( apiVersion: esApiVersion, }); + const loadingCount$ = new BehaviorSubject(0); + http.addLoadingCountSource(loadingCount$); + return { search: wrapEsClientMethod(client, 'search', loadingCount$), msearch: wrapEsClientMethod(client, 'msearch', loadingCount$), diff --git a/src/plugins/data/public/search/search_service.ts b/src/plugins/data/public/search/search_service.ts index dad2b3d078aa0..dd67c04f1d7bd 100644 --- a/src/plugins/data/public/search/search_service.ts +++ b/src/plugins/data/public/search/search_service.ts @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { BehaviorSubject } from 'rxjs'; import { Plugin, CoreSetup, @@ -80,22 +79,17 @@ export class SearchService implements Plugin { private contextContainer?: IContextContainer>; private esClient?: LegacyApiCaller; private search?: ISearchGeneric; - private readonly loadingCount$ = new BehaviorSubject(0); constructor(private initializerContext: PluginInitializerContext) {} public setup(core: CoreSetup, packageInfo: PackageInfo): ISearchSetup { - core.http.addLoadingCountSource(this.loadingCount$); - const search = (this.search = createAppMountSearchContext( - this.searchStrategies, - this.loadingCount$ - ).search); + const search = (this.search = createAppMountSearchContext(this.searchStrategies).search); core.application.registerMountContext<'search'>('search', () => { return { search }; }); this.contextContainer = core.context.createContextContainer(); - this.esClient = getEsClient(core.injectedMetadata, core.http, packageInfo, this.loadingCount$); + this.esClient = getEsClient(core.injectedMetadata, core.http, packageInfo); const registerSearchStrategyProvider: TRegisterSearchStrategyProvider = < T extends TStrategyTypes diff --git a/src/plugins/data/public/search/sync_search_strategy.ts b/src/plugins/data/public/search/sync_search_strategy.ts index 65fe10f39aaa0..352e19b5875e8 100644 --- a/src/plugins/data/public/search/sync_search_strategy.ts +++ b/src/plugins/data/public/search/sync_search_strategy.ts @@ -17,7 +17,7 @@ * under the License. */ -import { from } from 'rxjs'; +import { BehaviorSubject, from } from 'rxjs'; import { IKibanaSearchRequest, IKibanaSearchResponse } from '../../common/search'; import { ISearchContext } from './i_search_context'; import { ISearch, ISearchOptions } from './i_search'; @@ -31,11 +31,15 @@ export interface ISyncSearchRequest extends IKibanaSearchRequest { export const syncSearchStrategyProvider: TSearchStrategyProvider = ( context: ISearchContext -) => { +): ISearchStrategy => { const search: ISearch = ( request: ISyncSearchRequest, options: ISearchOptions = {} ) => { + const loadingCount$ = new BehaviorSubject(0); + context.core.http.addLoadingCountSource(loadingCount$); + loadingCount$.next(loadingCount$.getValue() + 1); + const response: Promise = context.core.http.fetch({ path: `/internal/search/${request.serverStrategy}`, method: 'POST', @@ -43,12 +47,10 @@ export const syncSearchStrategyProvider: TSearchStrategyProvider loadingCount$.next(loadingCount$.getValue() - 1)); - const strategy: ISearchStrategy = { - search, + return from(response); }; - return strategy; + return { search }; }; From e5d7d5851154fe2f0fcc1f8b8a89db6e8747ca39 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Mon, 3 Feb 2020 13:16:13 -0700 Subject: [PATCH 2/2] Only register observable once --- src/plugins/data/public/search/sync_search_strategy.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/public/search/sync_search_strategy.ts b/src/plugins/data/public/search/sync_search_strategy.ts index 352e19b5875e8..b895a177d8608 100644 --- a/src/plugins/data/public/search/sync_search_strategy.ts +++ b/src/plugins/data/public/search/sync_search_strategy.ts @@ -32,12 +32,13 @@ export interface ISyncSearchRequest extends IKibanaSearchRequest { export const syncSearchStrategyProvider: TSearchStrategyProvider = ( context: ISearchContext ): ISearchStrategy => { + const loadingCount$ = new BehaviorSubject(0); + context.core.http.addLoadingCountSource(loadingCount$); + const search: ISearch = ( request: ISyncSearchRequest, options: ISearchOptions = {} ) => { - const loadingCount$ = new BehaviorSubject(0); - context.core.http.addLoadingCountSource(loadingCount$); loadingCount$.next(loadingCount$.getValue() + 1); const response: Promise = context.core.http.fetch({