review improvements

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', () => {

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

@@ -277,10 +277,11 @@ export class SearchSource {
   ): Observable<IKibanaSearchResponse<estypes.SearchResponse<any>>> {
     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<IKibanaSearchResponse<any>>((obs) => {

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
+}

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();

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);
+  });
 });

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,