From 21b0b4e6f468606dd18375a774f2ad53cf8f06c6 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Thu, 15 Feb 2024 19:49:37 +0000 Subject: [PATCH] [Resolve Comment] remove service side error handling to avoid dependency on UI config Signed-off-by: Xinrui Bai --- CHANGELOG.md | 1 - .../search/opensearch_search/decide_client.ts | 16 +-- .../opensearch_search_strategy.test.ts | 98 +------------------ .../opensearch_search_strategy.ts | 17 +--- src/plugins/data_source/server/plugin.ts | 2 - src/plugins/data_source/server/types.ts | 2 - 6 files changed, 14 insertions(+), 122 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d8c0349046e..655418cf8897 100644 --- 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 index 9e0306d19269..2ff2339add44 100644 --- 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 => { - 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 index 4423d90f63b1..39c367a04a41 100644 --- 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 { - return Promise.resolve(undefined); - }, - trackSuccess(duration: number): Promise { - 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 index ab46177d0b43..5eb290517792 100644 --- 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>; @@ -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 index 1d6e390362eb..5eaafc290000 100644 --- 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 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 index bf34865c95c2..146a28eb8cfa 100644 --- 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; }