[MD] Improved error handling for the search API when a null value is passed for the dataSourceId.

CHANGELOG.md

@@ -68,6 +68,7 @@ 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 there is no open-search hosts config ([#5882](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5882))
 
 ### 🚞 Infrastructure
 

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> => {
-  // 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 withDataSourceEnabled && request.dataSourceId && context.dataSource
+    ? await context.dataSource.opensearch.getClient(request.dataSourceId)
+    : defaultOpenSearchClient;
 };

src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts

@@ -29,15 +29,34 @@
  */
 
 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(),
+  };
   const body = {
     body: {
       _shards: {
@@ -131,20 +150,96 @@ 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 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
+    );
+
+    await opensearchSearch.search(
+      (mockDataSourceEnabledContext as unknown) as RequestHandlerContext,
+      {
+        dataSourceId: 'Some 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 throw exception', async () => {
+    const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup();
+    mockOpenSearchServiceSetup.legacy.client = {
+      callAsInternalUser: jest.fn(),
+      asScoped: jest.fn(),
+      config: {
+        hosts: [],
+      },
+    };
+
+    try {
+      const opensearchSearch = opensearchSearchStrategyProvider(
+        mockConfig$,
+        mockLogger,
+        mockSearchUsage,
+        mockDataSourcePluginSetupWithDataSourceEnabled,
+        mockOpenSearchServiceSetup
+      );
+
+      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 should get default client', async () => {
     const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger);
 
     await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {
@@ -154,11 +249,17 @@ 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();
+    }
   });
 });

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 @@
   logger: Logger,
   usage?: SearchUsage,
   dataSource?: DataSourcePluginSetup,
+  openSearchServiceSetup?: OpenSearchServiceSetup,
   withLongNumeralsSupport?: boolean
 ): ISearchStrategy => {
   return {
@@ -73,7 +74,19 @@
       });
 
       try {
-        const client = await decideClient(context, request, withLongNumeralsSupport);
+        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,
+          dataSource?.dataSourceEnabled(),
+          withLongNumeralsSupport
+        );
         const promise = shimAbortSignal(client.search(params), options?.abortSignal);
 
         const { body: rawResponse } = (await promise) as ApiResponse<SearchResponse<any>>;
@@ -92,7 +105,7 @@
       } catch (e) {
         if (usage) usage.trackError();
 
-        if (dataSource && request.dataSourceId) {
+        if (dataSource?.dataSourceEnabled()) {
           throw dataSource.createDataSourceError(e);
         }
         throw e;

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

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,
     };
   }
 

src/plugins/data_source/server/types.ts

@@ -81,6 +81,7 @@ export interface DataSourcePluginSetup {
   createDataSourceError: (err: any) => DataSourceError;
   registerCredentialProvider: (method: AuthenticationMethod) => void;
   registerCustomApiSchema: (schema: any) => void;
+  dataSourceEnabled: () => boolean;
 }
 
 export interface DataSourcePluginStart {