[Resolve Comment] remove service side error handling to avoid depende…

…ncy on UI config Signed-off-by: Xinrui Bai <[email protected]>
opensearch-project · Feb 15, 2024 · 21b0b4e · 21b0b4e
1 parent 9af54a5
commit 21b0b4e
Show file tree

Hide file tree

Showing 6 changed files with 14 additions and 122 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -67,7 +67,6 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [BUG] Remove duplicate sample data as id 90943e30-9a47-11e8-b64d-95841ca0b247 ([5668](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5668))
 - [BUG][Multiple Datasource] Fix datasource testing connection unexpectedly passed with wrong endpoint [#5663](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5663)
 - [Table Visualization] Fix filter action buttons for split table aggregations ([#5619](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5619))
-- [BUG][Multiple Datasource] Datasource id is required if multiple datasource is enabled and no default cluster supported [#5751](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5751)
 
 ### 🚞 Infrastructure
 

diff --git a/src/plugins/data/server/search/opensearch_search/decide_client.ts b/src/plugins/data/server/search/opensearch_search/decide_client.ts
@@ -9,14 +9,14 @@ import { IOpenSearchSearchRequest } from '..';
 export const decideClient = async (
   context: RequestHandlerContext,
   request: IOpenSearchSearchRequest,
-  withDataSourceEnabled: boolean = false,
   withLongNumeralsSupport: boolean = false
 ): Promise<OpenSearchClient> => {
-  const defaultOpenSearchClient = withLongNumeralsSupport
-    ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport
-    : context.core.opensearch.client.asCurrentUser;
-
-  return withDataSourceEnabled && request.dataSourceId && context.dataSource
-    ? await context.dataSource.opensearch.getClient(request.dataSourceId)
-    : defaultOpenSearchClient;
+  // 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;
 };
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
@@ -33,20 +33,11 @@ import { pluginInitializerContextConfigMock } from '../../../../../core/server/m
 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 body = {
     body: {
       _shards: {
@@ -140,21 +131,8 @@ describe('OpenSearch search strategy', () => {
     expect(response).toHaveProperty('rawResponse');
   });
 
-  it('dataSource enabled and local cluster hidden, send request with dataSourceId get data source client', async () => {
-    const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = {
-      createDataSourceError(err: any): DataSourceError {
-        return new DataSourceError({});
-      },
-      dataSourceEnabled: jest.fn(() => true),
-      hideLocalCluster: jest.fn(() => true),
-    };
-
-    const opensearchSearch = await opensearchSearchStrategyProvider(
-      mockConfig$,
-      mockLogger,
-      mockSearchUsage,
-      mockDataSourcePluginSetupWithDataSourceEnabled
-    );
+  it('dataSource enabled, send request with dataSourceId get data source client', async () => {
+    const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
 
     await opensearchSearch.search(
       (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
@@ -166,35 +144,6 @@ describe('OpenSearch search strategy', () => {
     expect(mockOpenSearchApiCaller).not.toBeCalled();
   });
 
-  it('dataSource enabled and local cluster hidden, send request with empty dataSourceId should throw exception', async () => {
-    const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = {
-      createDataSourceError(err: any): DataSourceError {
-        return new DataSourceError({});
-      },
-      dataSourceEnabled: jest.fn(() => true),
-      hideLocalCluster: jest.fn(() => true),
-    };
-
-    try {
-      const opensearchSearch = opensearchSearchStrategyProvider(
-        mockConfig$,
-        mockLogger,
-        mockSearchUsage,
-        mockDataSourcePluginSetupWithDataSourceEnabled
-      );
-
-      await opensearchSearch.search(
-        (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
-        {
-          dataSourceId: '',
-        }
-      );
-    } catch (e) {
-      expect(e).toBeTruthy();
-      expect(e).toBeInstanceOf(DataSourceError);
-    }
-  });
-
   it('dataSource disabled, send request with dataSourceId get default client', async () => {
     const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
 
@@ -205,47 +154,8 @@ describe('OpenSearch search strategy', () => {
     expect(mockDataSourceApiCaller).not.toBeCalled();
   });
 
-  it('dataSource enabled and local cluster not hidden, send request with dataSourceId get datasource client', async () => {
-    const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = {
-      createDataSourceError(err: any): DataSourceError {
-        return new DataSourceError({});
-      },
-      dataSourceEnabled: jest.fn(() => true),
-      hideLocalCluster: jest.fn(() => false),
-    };
-
-    const opensearchSearch = await opensearchSearchStrategyProvider(
-      mockConfig$,
-      mockLogger,
-      mockSearchUsage,
-      mockDataSourcePluginSetupWithDataSourceEnabled
-    );
-
-    await opensearchSearch.search(
-      (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
-      {
-        dataSourceId,
-      }
-    );
-    expect(mockDataSourceApiCaller).toBeCalled();
-    expect(mockOpenSearchApiCaller).not.toBeCalled();
-  });
-
-  it('dataSource enabled and local cluster not hidden, send request without dataSourceId get default client', async () => {
-    const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = {
-      createDataSourceError(err: any): DataSourceError {
-        return new DataSourceError({});
-      },
-      dataSourceEnabled: jest.fn(() => true),
-      hideLocalCluster: jest.fn(() => false),
-    };
-
-    const opensearchSearch = await opensearchSearchStrategyProvider(
-      mockConfig$,
-      mockLogger,
-      mockSearchUsage,
-      mockDataSourcePluginSetupWithDataSourceEnabled
-    );
+  it('dataSource enabled, send request without dataSourceId get default client', async () => {
+    const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
 
     await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {});
     expect(mockOpenSearchApiCaller).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
@@ -73,20 +73,7 @@ export const opensearchSearchStrategyProvider = (
       });
 
       try {
-        if (
-          dataSource?.dataSourceEnabled() &&
-          dataSource?.hideLocalCluster() &&
-          !request.dataSourceId
-        ) {
-          throw new Error(`Data source id is required when no openseach hosts config provided`);
-        }
-
-        const client = await decideClient(
-          context,
-          request,
-          dataSource?.dataSourceEnabled(),
-          withLongNumeralsSupport
-        );
+        const client = await decideClient(context, request, withLongNumeralsSupport);
         const promise = shimAbortSignal(client.search(params), options?.abortSignal);
 
         const { body: rawResponse } = (await promise) as ApiResponse<SearchResponse<any>>;
@@ -105,7 +92,7 @@ export const opensearchSearchStrategyProvider = (
       } catch (e) {
         if (usage) usage.trackError();
 
-        if (dataSource?.dataSourceEnabled()) {
+        if (dataSource && request.dataSourceId) {
           throw dataSource.createDataSourceError(e);
         }
         throw e;

diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts
@@ -134,8 +134,6 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
 
     return {
       createDataSourceError: (e: any) => createDataSourceError(e),
-      dataSourceEnabled: () => config.enabled,
-      hideLocalCluster: () => config.hideLocalCluster,
       registerCredentialProvider,
     };
   }

diff --git a/src/plugins/data_source/server/types.ts b/src/plugins/data_source/server/types.ts
@@ -76,8 +76,6 @@ declare module 'src/core/server' {
 
 export interface DataSourcePluginSetup {
   createDataSourceError: (err: any) => DataSourceError;
-  dataSourceEnabled: () => boolean;
-  hideLocalCluster: () => boolean;
   registerCredentialProvider: (method: AuthenticationMethod) => void;
 }