From 63e216557f554c9830cf44d40eee6017ddf433e4 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Thu, 6 May 2021 17:41:37 +0200 Subject: [PATCH] review improvements --- .../search_source/search_source.test.ts | 53 ++++++++++++++++++- .../search/search_source/search_source.ts | 11 ++-- .../rollup_wrong_index_exception.json | 13 +++++ .../ese_search/ese_search_strategy.test.ts | 3 -- .../rollup_search_strategy.test.ts | 44 ++++++++++----- .../rollup_search/rollup_search_strategy.ts | 1 + 6 files changed, 103 insertions(+), 22 deletions(-) create mode 100644 src/plugins/data/common/search/test_data/rollup_wrong_index_exception.json diff --git a/src/plugins/data/common/search/search_source/search_source.test.ts b/src/plugins/data/common/search/search_source/search_source.test.ts index ef3e020747f7a..0bac841077ab3 100644 --- a/src/plugins/data/common/search/search_source/search_source.test.ts +++ b/src/plugins/data/common/search/search_source/search_source.test.ts @@ -10,7 +10,12 @@ import { of } from 'rxjs'; import { IndexPattern } from '../../index_patterns'; import { GetConfigFn } from '../../types'; import { SearchSource, SearchSourceDependencies, SortDirection } from './'; -import { AggConfigs, AggTypesRegistryStart, ES_SEARCH_STRATEGY } from '../../'; +import { + AggConfigs, + AggTypesRegistryStart, + ES_SEARCH_STRATEGY, + ROLLUP_SEARCH_STRATEGY, +} from '../../'; import { mockAggTypesRegistry } from '../aggs/test_helpers'; import { RequestResponder } from 'src/plugins/inspector/common'; import { switchMap } from 'rxjs/operators'; @@ -32,6 +37,14 @@ const indexPattern = ({ getSourceFiltering: () => mockSource, } as unknown) as IndexPattern; +const rollupIndexPattern = ({ + title: 'foo', + type: 'rollup', + fields: [{ name: 'foo-bar' }, { name: 'field1' }, { name: 'field2' }], + getComputedFields, + getSourceFiltering: () => mockSource, +} as unknown) as IndexPattern; + const indexPattern2 = ({ title: 'foo', getComputedFields, @@ -895,6 +908,44 @@ describe('SearchSource', () => { const [, callOptions] = mockSearchMethod.mock.calls[0]; expect(callOptions.strategy).toBe('banana'); }); + + test('should use rollup search if rollup index', async () => { + searchSource = new SearchSource({ index: rollupIndexPattern }, searchSourceDependencies); + const options = {}; + await searchSource.fetch$(options).toPromise(); + + const [, callOptions] = mockSearchMethod.mock.calls[0]; + expect(callOptions.strategy).toBe(ROLLUP_SEARCH_STRATEGY); + }); + + test('should not use rollup search if overriden', async () => { + searchSource = new SearchSource({ index: rollupIndexPattern }, searchSourceDependencies); + const options = { strategy: 'banana' }; + await searchSource.fetch$(options).toPromise(); + + const [, callOptions] = mockSearchMethod.mock.calls[0]; + expect(callOptions.strategy).toBe('banana'); + }); + }); + + describe('Rollup search', () => { + test('should use rollup search if rollup index', async () => { + searchSource = new SearchSource({ index: rollupIndexPattern }, searchSourceDependencies); + const options = {}; + await searchSource.fetch$(options).toPromise(); + + const [, callOptions] = mockSearchMethod.mock.calls[0]; + expect(callOptions.strategy).toBe(ROLLUP_SEARCH_STRATEGY); + }); + + test('should not use rollup search if overriden', async () => { + searchSource = new SearchSource({ index: rollupIndexPattern }, searchSourceDependencies); + const options = { strategy: 'banana' }; + await searchSource.fetch$(options).toPromise(); + + const [, callOptions] = mockSearchMethod.mock.calls[0]; + expect(callOptions.strategy).toBe('banana'); + }); }); describe('responses', () => { diff --git a/src/plugins/data/common/search/search_source/search_source.ts b/src/plugins/data/common/search/search_source/search_source.ts index add8f5ebb64b2..2dd0171347423 100644 --- a/src/plugins/data/common/search/search_source/search_source.ts +++ b/src/plugins/data/common/search/search_source/search_source.ts @@ -277,10 +277,11 @@ export class SearchSource { ): Observable>> { const { getConfig } = this.dependencies; const syncSearchByDefault = getConfig(UI_SETTINGS.COURIER_BATCH_SEARCHES); + const hasExplicitStrategy = !!options.strategy; // Use the sync search strategy if legacy search is enabled. // This still uses bfetch for batching. - if (!options?.strategy && syncSearchByDefault) { + if (!hasExplicitStrategy && syncSearchByDefault) { options.strategy = ES_SEARCH_STRATEGY; } @@ -290,6 +291,9 @@ export class SearchSource { this.history = [searchRequest]; if (searchRequest.index) { options.indexPattern = searchRequest.index; + if (searchRequest.indexType === 'rollup' && !hasExplicitStrategy) { + options.strategy = ROLLUP_SEARCH_STRATEGY; + } } return this.fetchSearch$(searchRequest, options); @@ -446,11 +450,6 @@ export class SearchSource { getConfig, }); - // force a rollup strategy in case this is a rollup index - if (!options.strategy && searchRequest.indexType === 'rollup') { - options.strategy = ROLLUP_SEARCH_STRATEGY; - } - return search({ params }, options).pipe( switchMap((response) => { return new Observable>((obs) => { diff --git a/src/plugins/data/common/search/test_data/rollup_wrong_index_exception.json b/src/plugins/data/common/search/test_data/rollup_wrong_index_exception.json new file mode 100644 index 0000000000000..9c3e82cad0b7d --- /dev/null +++ b/src/plugins/data/common/search/test_data/rollup_wrong_index_exception.json @@ -0,0 +1,13 @@ +{ + "error": { + "root_cause": [ + { + "type": "illegal_argument_exception", + "reason": "Must specify at least one concrete index." + } + ], + "type": "illegal_argument_exception", + "reason": "Must specify at least one concrete index." + }, + "status": 400 +} diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts index 178f3bf5bf4ba..1c636ebe5a14b 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts @@ -30,7 +30,6 @@ const mockAsyncResponse = { }; describe('ES search strategy', () => { - const mockApiCaller = jest.fn(); const mockGetCaller = jest.fn(); const mockSubmitCaller = jest.fn(); const mockDeleteCaller = jest.fn(); @@ -48,7 +47,6 @@ describe('ES search strategy', () => { submit: mockSubmitCaller, delete: mockDeleteCaller, }, - transport: { request: mockApiCaller }, }, }, searchSessionsClient: createSearchSessionsClientMock(), @@ -64,7 +62,6 @@ describe('ES search strategy', () => { }); beforeEach(() => { - mockApiCaller.mockClear(); mockGetCaller.mockClear(); mockSubmitCaller.mockClear(); mockDeleteCaller.mockClear(); diff --git a/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.test.ts b/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.test.ts index 26c509edb3475..b276811f42d33 100644 --- a/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.test.ts +++ b/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.test.ts @@ -10,6 +10,9 @@ import { BehaviorSubject } from 'rxjs'; import { SearchStrategyDependencies } from '../../types'; import { rollupSearchStrategyProvider } from './rollup_search_strategy'; import { createSearchSessionsClientMock } from '../../mocks'; +import { ResponseError } from '@elastic/elasticsearch/lib/errors'; +import * as rollupWrongIndexException from '../../../../common/search/test_data/rollup_wrong_index_exception.json'; +import { KbnServerError } from '../../../../../kibana_utils/server'; const mockRollupResponse = { body: { @@ -22,11 +25,8 @@ const mockRollupResponse = { }, }; -describe('ES search strategy', () => { +describe('Rollup search strategy', () => { const mockApiCaller = jest.fn(); - const mockGetCaller = jest.fn(); - const mockSubmitCaller = jest.fn(); - const mockDeleteCaller = jest.fn(); const mockLogger: any = { debug: () => {}, }; @@ -36,11 +36,6 @@ describe('ES search strategy', () => { }, esClient: { asCurrentUser: { - asyncSearch: { - get: mockGetCaller, - submit: mockSubmitCaller, - delete: mockDeleteCaller, - }, transport: { request: mockApiCaller }, }, }, @@ -58,9 +53,6 @@ describe('ES search strategy', () => { beforeEach(() => { mockApiCaller.mockClear(); - mockGetCaller.mockClear(); - mockSubmitCaller.mockClear(); - mockDeleteCaller.mockClear(); }); it('calls the rollup API', async () => { @@ -84,4 +76,32 @@ describe('ES search strategy', () => { expect(method).toBe('POST'); expect(path).toBe('/foo-%E7%A8%8B/_rollup_search'); }); + + it('throws normalized error on ResponseError', async () => { + const errResponse = new ResponseError({ + body: rollupWrongIndexException, + statusCode: 400, + headers: {}, + warnings: [], + meta: {} as any, + }); + + mockApiCaller.mockRejectedValueOnce(errResponse); + + const params = { index: 'non-rollup-index', body: {} }; + const rollupSearch = await rollupSearchStrategyProvider(mockLegacyConfig$, mockLogger); + + let err: KbnServerError | undefined; + try { + await rollupSearch.search({ params }, {}, mockDeps).toPromise(); + } catch (e) { + err = e; + } + + expect(mockApiCaller).toBeCalled(); + expect(err).toBeInstanceOf(KbnServerError); + expect(err?.statusCode).toBe(400); + expect(err?.message).toBe(errResponse.message); + expect(err?.errBody).toBe(rollupWrongIndexException); + }); }); diff --git a/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.ts b/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.ts index f912362ae5b1a..906472ce65095 100644 --- a/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.ts @@ -53,6 +53,7 @@ export const rollupSearchStrategyProvider = ( }; try { + // TODO: use esClient.asCurrentUser.rollup.rollupSearch const promise = esClient.asCurrentUser.transport.request({ method, path,