[MD] Improved error handling for the search API when a null value is …

…passed for the dataSourceId. (#5882) (#5969) * [MD] Using legacy client cofig instead of globle config * [UT] add test cases to test different hostconfig and datasourceId combinations of scenario * Update changelog file * Resolve comments * [UT] update test cases and resolve comments * [UT] Update unit test --------- (cherry picked from commit 7d77b9e) Signed-off-by: Xinrui Bai <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Feb 27, 2024 · b6468fa · b6468fa
1 parent e59e506
commit b6468fa
Show file tree

Hide file tree

Showing 6 changed files with 190 additions and 20 deletions.
diff --git a/src/plugins/data/server/search/opensearch_search/decide_client.ts b/src/plugins/data/server/search/opensearch_search/decide_client.ts
@@ -11,12 +11,11 @@ export const decideClient = async (
   request: IOpenSearchSearchRequest,
   withLongNumeralsSupport: boolean = false
 ): Promise<OpenSearchClient> => {
-  // if data source feature is disabled, return default opensearch client of current user
-  const client =
-    request.dataSourceId && context.dataSource
-      ? await context.dataSource.opensearch.getClient(request.dataSourceId)
-      : withLongNumeralsSupport
-      ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport
-      : context.core.opensearch.client.asCurrentUser;
-  return client;
+  const defaultOpenSearchClient = withLongNumeralsSupport
+    ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport
+    : context.core.opensearch.client.asCurrentUser;
+
+  return request.dataSourceId && context.dataSource
+    ? await context.dataSource.opensearch.getClient(request.dataSourceId)
+    : defaultOpenSearchClient;
 };
diff --git a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts
@@ -29,15 +29,47 @@
  */
 
 import { RequestHandlerContext } from '../../../../../core/server';
-import { pluginInitializerContextConfigMock } from '../../../../../core/server/mocks';
+import {
+  opensearchServiceMock,
+  pluginInitializerContextConfigMock,
+} from '../../../../../core/server/mocks';
 import { opensearchSearchStrategyProvider } from './opensearch_search_strategy';
 import { DataSourceError } from '../../../../data_source/server/lib/error';
 import { DataSourcePluginSetup } from '../../../../data_source/server';
+import { SearchUsage } from '../collectors';
 
 describe('OpenSearch search strategy', () => {
   const mockLogger: any = {
     debug: () => {},
   };
+  const mockSearchUsage: SearchUsage = {
+    trackError(): Promise<void> {
+      return Promise.resolve(undefined);
+    },
+    trackSuccess(duration: number): Promise<void> {
+      return Promise.resolve(undefined);
+    },
+  };
+  const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = {
+    createDataSourceError(err: any): DataSourceError {
+      return new DataSourceError({});
+    },
+    dataSourceEnabled: jest.fn(() => true),
+    registerCredentialProvider: jest.fn(),
+    registerCustomApiSchema(schema: any): void {
+      throw new Error('Function not implemented.');
+    },
+  };
+  const mockDataSourcePluginSetupWithDataSourceDisabled: DataSourcePluginSetup = {
+    createDataSourceError(err: any): DataSourceError {
+      return new DataSourceError({});
+    },
+    dataSourceEnabled: jest.fn(() => false),
+    registerCredentialProvider: jest.fn(),
+    registerCustomApiSchema(schema: any): void {
+      throw new Error('Function not implemented.');
+    },
+  };
   const body = {
     body: {
       _shards: {
@@ -50,6 +82,7 @@ describe('OpenSearch search strategy', () => {
   };
   const mockOpenSearchApiCaller = jest.fn().mockResolvedValue(body);
   const mockDataSourceApiCaller = jest.fn().mockResolvedValue(body);
+  const mockOpenSearchApiCallerWithLongNumeralsSupport = jest.fn().mockResolvedValue(body);
   const dataSourceId = 'test-data-source-id';
   const mockDataSourceContext = {
     dataSource: {
@@ -67,7 +100,14 @@ describe('OpenSearch search strategy', () => {
           get: () => {},
         },
       },
-      opensearch: { client: { asCurrentUser: { search: mockOpenSearchApiCaller } } },
+      opensearch: {
+        client: {
+          asCurrentUser: { search: mockOpenSearchApiCaller },
+          asCurrentUserWithLongNumeralsSupport: {
+            search: mockOpenSearchApiCallerWithLongNumeralsSupport,
+          },
+        },
+      },
     },
   };
   const mockDataSourceEnabledContext = {
@@ -131,20 +171,110 @@ describe('OpenSearch search strategy', () => {
     expect(response).toHaveProperty('rawResponse');
   });
 
-  it('dataSource enabled, send request with dataSourceId get data source client', async () => {
-    const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
+  it('dataSource enabled, config host exist, send request with dataSourceId should get data source client', async () => {
+    const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup();
+    mockOpenSearchServiceSetup.legacy.client = {
+      callAsInternalUser: jest.fn(),
+      asScoped: jest.fn(),
+      config: {
+        hosts: ['some host'],
+      },
+    };
+
+    const opensearchSearch = opensearchSearchStrategyProvider(
+      mockConfig$,
+      mockLogger,
+      mockSearchUsage,
+      mockDataSourcePluginSetupWithDataSourceEnabled,
+      mockOpenSearchServiceSetup
+    );
 
     await opensearchSearch.search(
       (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
       {
         dataSourceId,
       }
     );
+
     expect(mockDataSourceApiCaller).toBeCalled();
     expect(mockOpenSearchApiCaller).not.toBeCalled();
   });
 
-  it('dataSource disabled, send request with dataSourceId get default client', async () => {
+  it('dataSource enabled, config host exist, send request without dataSourceId should get default client', async () => {
+    const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup();
+    mockOpenSearchServiceSetup.legacy.client = {
+      callAsInternalUser: jest.fn(),
+      asScoped: jest.fn(),
+      config: {
+        hosts: ['some host'],
+      },
+    };
+
+    const opensearchSearch = opensearchSearchStrategyProvider(
+      mockConfig$,
+      mockLogger,
+      mockSearchUsage,
+      mockDataSourcePluginSetupWithDataSourceEnabled,
+      mockOpenSearchServiceSetup
+    );
+
+    const dataSourceIdToBeTested = [undefined, ''];
+
+    dataSourceIdToBeTested.forEach(async (id) => {
+      const testRequest = id === undefined ? {} : { dataSourceId: id };
+
+      await opensearchSearch.search(
+        (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
+        testRequest
+      );
+      expect(mockOpenSearchApiCaller).toBeCalled();
+      expect(mockDataSourceApiCaller).not.toBeCalled();
+    });
+  });
+
+  it('dataSource enabled, config host is empty / undefined, send request with / without dataSourceId should both throw DataSourceError exception', async () => {
+    const hostsTobeTested = [undefined, []];
+    const dataSourceIdToBeTested = [undefined, '', dataSourceId];
+
+    hostsTobeTested.forEach((host) => {
+      const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup();
+
+      if (host !== undefined) {
+        mockOpenSearchServiceSetup.legacy.client = {
+          callAsInternalUser: jest.fn(),
+          asScoped: jest.fn(),
+          config: {
+            hosts: [],
+          },
+        };
+      }
+
+      dataSourceIdToBeTested.forEach(async (id) => {
+        const testRequest = id === undefined ? {} : { dataSourceId: id };
+
+        try {
+          const opensearchSearch = opensearchSearchStrategyProvider(
+            mockConfig$,
+            mockLogger,
+            mockSearchUsage,
+            mockDataSourcePluginSetupWithDataSourceEnabled,
+            mockOpenSearchServiceSetup
+          );
+
+          await opensearchSearch.search(
+            (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
+            testRequest
+          );
+        } catch (e) {
+          expect(e).toBeTruthy();
+          expect(e).toBeInstanceOf(DataSourceError);
+          expect(e.statusCode).toEqual(400);
+        }
+      });
+    });
+  });
+
+  it('dataSource disabled, send request with dataSourceId should get default client', async () => {
     const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
 
     await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {
@@ -154,11 +284,40 @@ describe('OpenSearch search strategy', () => {
     expect(mockDataSourceApiCaller).not.toBeCalled();
   });
 
-  it('dataSource enabled, send request without dataSourceId get default client', async () => {
+  it('dataSource disabled, send request without dataSourceId should get default client', async () => {
     const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
 
-    await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {});
-    expect(mockOpenSearchApiCaller).toBeCalled();
-    expect(mockDataSourceApiCaller).not.toBeCalled();
+    const dataSourceIdToBeTested = [undefined, ''];
+
+    for (const testDataSourceId of dataSourceIdToBeTested) {
+      await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {
+        dataSourceId: testDataSourceId,
+      });
+      expect(mockOpenSearchApiCaller).toBeCalled();
+      expect(mockDataSourceApiCaller).not.toBeCalled();
+    }
+  });
+
+  it('dataSource disabled and longNumeralsSupported, send request without dataSourceId should get longNumeralsSupport client', async () => {
+    const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup();
+    const opensearchSearch = await opensearchSearchStrategyProvider(
+      mockConfig$,
+      mockLogger,
+      mockSearchUsage,
+      mockDataSourcePluginSetupWithDataSourceDisabled,
+      mockOpenSearchServiceSetup,
+      true
+    );
+
+    const dataSourceIdToBeTested = [undefined, ''];
+
+    for (const testDataSourceId of dataSourceIdToBeTested) {
+      await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {
+        dataSourceId: testDataSourceId,
+      });
+      expect(mockOpenSearchApiCallerWithLongNumeralsSupport).toBeCalled();
+      expect(mockOpenSearchApiCaller).not.toBeCalled();
+      expect(mockDataSourceApiCaller).not.toBeCalled();
+    }
   });
 });
diff --git a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts
@@ -29,7 +29,7 @@
  */
 
 import { first } from 'rxjs/operators';
-import { SharedGlobalConfig, Logger } from 'opensearch-dashboards/server';
+import { SharedGlobalConfig, Logger, OpenSearchServiceSetup } from 'opensearch-dashboards/server';
 import { SearchResponse } from 'elasticsearch';
 import { Observable } from 'rxjs';
 import { ApiResponse } from '@opensearch-project/opensearch';
@@ -50,6 +50,7 @@ export const opensearchSearchStrategyProvider = (
   logger: Logger,
   usage?: SearchUsage,
   dataSource?: DataSourcePluginSetup,
+  openSearchServiceSetup?: OpenSearchServiceSetup,
   withLongNumeralsSupport?: boolean
 ): ISearchStrategy => {
   return {
@@ -73,6 +74,13 @@ export const opensearchSearchStrategyProvider = (
       });
 
       try {
+        const isOpenSearchHostsEmpty =
+          openSearchServiceSetup?.legacy?.client?.config?.hosts?.length === 0;
+
+        if (dataSource?.dataSourceEnabled() && isOpenSearchHostsEmpty && !request.dataSourceId) {
+          throw new Error(`Data source id is required when no openseach hosts config provided`);
+        }
+
         const client = await decideClient(context, request, withLongNumeralsSupport);
         const promise = shimAbortSignal(client.search(params), options?.abortSignal);
 
@@ -92,7 +100,7 @@ export const opensearchSearchStrategyProvider = (
       } catch (e) {
         if (usage) usage.trackError();
 
-        if (dataSource && request.dataSourceId) {
+        if (dataSource?.dataSourceEnabled()) {
           throw dataSource.createDataSourceError(e);
         }
         throw e;

diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts
@@ -130,7 +130,8 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
         this.initializerContext.config.legacy.globalConfig$,
         this.logger,
         usage,
-        dataSource
+        dataSource,
+        core.opensearch
       )
     );
 
@@ -141,6 +142,7 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
         this.logger,
         usage,
         dataSource,
+        core.opensearch,
         true
       )
     );

diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts
@@ -144,6 +144,7 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
       createDataSourceError: (e: any) => createDataSourceError(e),
       registerCredentialProvider,
       registerCustomApiSchema: (schema: any) => this.customApiSchemaRegistry.register(schema),
+      dataSourceEnabled: () => config.enabled,
     };
   }
 

diff --git a/src/plugins/data_source/server/types.ts b/src/plugins/data_source/server/types.ts
@@ -85,6 +85,7 @@ export interface DataSourcePluginSetup {
   createDataSourceError: (err: any) => DataSourceError;
   registerCredentialProvider: (method: AuthenticationMethod) => void;
   registerCustomApiSchema: (schema: any) => void;
+  dataSourceEnabled: () => boolean;
 }
 
 export interface DataSourcePluginStart {