[MD] Refactor data source server side error handling (opensearch-project#2661)

zhongnansu · alchemist51 · commit df8a888a5fc1 · 2023-01-13T13:21:30.000+05:30
- Rename DataSourceConfigError to DataSourceError
- Add helper createDataSourceError() to create data source error from， and replace usage of new DataSourceError(e) with createDataSourceError(e)
- import createDataSourceError()  from data source plugin setup interface,
refactor to move data source error response logic from router.ts to resolve_index.ts.

Signed-off-by: Su &lt;szhongna@amazon.com&gt;
Signed-off-by: Arpit Bandejiya &lt;abandeji@amazon.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -57,6 +57,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Adding @zhongnansu as maintainer. ([#2590](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2590))
 
 ### 🪛 Refactoring
+* [MD] Refactor data source error handling ([#2661](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2661))
 
 ### 🔩 Tests
 
diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts
@@ -301,21 +301,11 @@ export class Router implements IRouter {
         return e;
       }
 
-      if (isDataSourceConfigError(e)) {
-        return hapiResponseAdapter.handle(
-          opensearchDashboardsResponseFactory.badRequest({ body: e.message })
-        );
-      }
-
       return hapiResponseAdapter.toInternalError();
     }
   }
 }
 
-const isDataSourceConfigError = (error: any) => {
-  return error.constructor.name === 'DataSourceConfigError' && error.statusCode === 400;
-};
-
 const convertOpenSearchUnauthorized = (
   e: OpenSearchNotAuthorizedError
 ): ErrorHttpResponseOptions => {
diff --git a/src/plugins/data/server/plugin.ts b/src/plugins/data/server/plugin.ts
@@ -30,6 +30,7 @@
 
 import { PluginInitializerContext, CoreSetup, CoreStart, Plugin, Logger } from 'src/core/server';
 import { ExpressionsServerSetup } from 'src/plugins/expressions/server';
+import { DataSourcePluginSetup } from 'src/plugins/data_source/server';
 import { ConfigSchema } from '../config';
 import { IndexPatternsService, IndexPatternsServiceStart } from './index_patterns';
 import { ISearchSetup, ISearchStart, SearchEnhancements } from './search';
@@ -64,6 +65,7 @@ export interface DataPluginStart {
 export interface DataPluginSetupDependencies {
   expressions: ExpressionsServerSetup;
   usageCollection?: UsageCollectionSetup;
+  dataSource?: DataSourcePluginSetup;
 }
 
 // eslint-disable-next-line @typescript-eslint/no-empty-interface
@@ -96,7 +98,7 @@ export class DataServerPlugin
 
   public setup(
     core: CoreSetup<DataPluginStartDependencies, DataPluginStart>,
-    { expressions, usageCollection }: DataPluginSetupDependencies
+    { expressions, usageCollection, dataSource }: DataPluginSetupDependencies
   ) {
     this.indexPatterns.setup(core);
     this.scriptsService.setup(core);
@@ -109,6 +111,7 @@ export class DataServerPlugin
     const searchSetup = this.searchService.setup(core, {
       registerFunction: expressions.registerFunction,
       usageCollection,
+      dataSource,
     });
 
     return {
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
@@ -33,6 +33,7 @@ import { SharedGlobalConfig, Logger } from 'opensearch-dashboards/server';
 import { SearchResponse } from 'elasticsearch';
 import { Observable } from 'rxjs';
 import { ApiResponse } from '@opensearch-project/opensearch';
+import { DataSourcePluginSetup } from 'src/plugins/data_source/server';
 import { SearchUsage } from '../collectors/usage';
 import { toSnakeCase } from './to_snake_case';
 import {
@@ -47,7 +48,8 @@ import { decideClient } from './decide_client';
 export const opensearchSearchStrategyProvider = (
   config$: Observable<SharedGlobalConfig>,
   logger: Logger,
-  usage?: SearchUsage
+  usage?: SearchUsage,
+  dataSource?: DataSourcePluginSetup
 ): ISearchStrategy => {
   return {
     search: async (context, request, options) => {
@@ -88,6 +90,10 @@ export const opensearchSearchStrategyProvider = (
         };
       } catch (e) {
         if (usage) usage.trackError();
+
+        if (dataSource && request.dataSourceId) {
+          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
@@ -42,6 +42,7 @@ import {
   StartServicesAccessor,
 } from 'src/core/server';
 import { first } from 'rxjs/operators';
+import { DataSourcePluginSetup } from 'src/plugins/data_source/server';
 import { ISearchSetup, ISearchStart, ISearchStrategy, SearchEnhancements } from './types';
 
 import { AggsService, AggsSetupDependencies } from './aggs';
@@ -78,6 +79,7 @@ type StrategyMap = Record<string, ISearchStrategy<any, any>>;
 export interface SearchServiceSetupDependencies {
   registerFunction: AggsSetupDependencies['registerFunction'];
   usageCollection?: UsageCollectionSetup;
+  dataSource?: DataSourcePluginSetup;
 }
 
 /** @internal */
@@ -105,7 +107,7 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
 
   public setup(
     core: CoreSetup<{}, DataPluginStart>,
-    { registerFunction, usageCollection }: SearchServiceSetupDependencies
+    { registerFunction, usageCollection, dataSource }: SearchServiceSetupDependencies
   ): ISearchSetup {
     const usage = usageCollection ? usageProvider(core, this.initializerContext) : undefined;
 
@@ -122,7 +124,8 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
       opensearchSearchStrategyProvider(
         this.initializerContext.config.legacy.globalConfig$,
         this.logger,
-        usage
+        usage,
+        dataSource
       )
     );
 
diff --git a/src/plugins/data_source/server/client/configure_client.ts b/src/plugins/data_source/server/client/configure_client.ts
@@ -13,7 +13,7 @@ import {
 } from '../../common/data_sources';
 import { DataSourcePluginConfigType } from '../../config';
 import { CryptographyServiceSetup } from '../cryptography_service';
-import { DataSourceConfigError } from '../lib/error';
+import { createDataSourceError, DataSourceError } from '../lib/error';
 import { DataSourceClientParams } from '../types';
 import { parseClientOptions } from './client_config';
 import { OpenSearchClientPoolSetup } from './client_pool';
@@ -32,8 +32,8 @@ export const configureClient = async (
   } catch (error: any) {
     logger.error(`Failed to get data source client for dataSourceId: [${dataSourceId}]`);
     logger.error(error);
-    // Re-throw as DataSourceConfigError
-    throw new DataSourceConfigError('Failed to get data source client: ', error);
+    // Re-throw as DataSourceError
+    throw createDataSourceError(error);
   }
 };
 
@@ -59,8 +59,8 @@ export const getCredential = async (
   const { decryptedText, encryptionContext } = await cryptography
     .decodeAndDecrypt(password)
     .catch((err: any) => {
-      // Re-throw as DataSourceConfigError
-      throw new DataSourceConfigError('Unable to decrypt "auth.credentials.password".', err);
+      // Re-throw as DataSourceError
+      throw createDataSourceError(err);
     });
 
   if (encryptionContext!.endpoint !== endpoint) {
diff --git a/src/plugins/data_source/server/legacy/configure_legacy_client.ts b/src/plugins/data_source/server/legacy/configure_legacy_client.ts
@@ -23,7 +23,7 @@ import { CryptographyServiceSetup } from '../cryptography_service';
 import { DataSourceClientParams, LegacyClientCallAPIParams } from '../types';
 import { OpenSearchClientPoolSetup, getCredential, getDataSource } from '../client';
 import { parseClientOptions } from './client_config';
-import { DataSourceConfigError } from '../lib/error';
+import { createDataSourceError, DataSourceError } from '../lib/error';
 
 export const configureLegacyClient = async (
   { dataSourceId, savedObjects, cryptography }: DataSourceClientParams,
@@ -40,8 +40,8 @@ export const configureLegacyClient = async (
   } catch (error: any) {
     logger.error(`Failed to get data source client for dataSourceId: [${dataSourceId}]`);
     logger.error(error);
-    // Re-throw as DataSourceConfigError
-    throw new DataSourceConfigError('Failed to get data source client: ', error);
+    // Re-throw as DataSourceError
+    throw createDataSourceError(error);
   }
 };
 
@@ -140,7 +140,6 @@ const callAPI = async (
     if (!options.wrap401Errors || err.statusCode !== 401) {
       throw err;
     }
-
     throw LegacyOpenSearchErrorHelpers.decorateNotAuthorizedError(err);
   }
 };
diff --git a/src/plugins/data_source/server/lib/error.test.ts b/src/plugins/data_source/server/lib/error.test.ts
@@ -3,38 +3,118 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import { ApiResponse } from '@opensearch-project/opensearch';
+import {
+  ConnectionError,
+  NoLivingConnectionsError,
+  ResponseError,
+} from '@opensearch-project/opensearch/lib/errors';
 import { SavedObjectsErrorHelpers } from '../../../../../src/core/server';
-import { DataSourceConfigError } from './error';
+import { createDataSourceError, DataSourceError } from './error';
+import { errors as LegacyErrors } from 'elasticsearch';
 
-describe('DataSourceConfigError', () => {
-  it('create from savedObject bad request error should be 400 error', () => {
+const createApiResponseError = ({
+  statusCode = 200,
+  headers = {},
+  body = {},
+}: {
+  statusCode?: number;
+  headers?: Record<string, string>;
+  body?: Record<string, any>;
+} = {}): ApiResponse => {
+  return {
+    body,
+    statusCode,
+    headers,
+    warnings: [],
+    meta: {} as any,
+  };
+};
+
+describe('CreateDataSourceError', () => {
+  it('create from savedObject bad request error should generate 400 error', () => {
     const error = SavedObjectsErrorHelpers.createBadRequestError('test reason message');
-    expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({
+    expect(createDataSourceError(error)).toMatchObject({
       statusCode: 400,
-      message: 'test prefix: test reason message: Bad Request',
+      message: 'Data Source Error: test reason message: Bad Request',
     });
   });
 
-  it('create from savedObject not found error should be 400 error', () => {
-    const error = SavedObjectsErrorHelpers.decorateNotAuthorizedError(new Error());
-    expect(new DataSourceConfigError('test prefix: ', error)).toHaveProperty('statusCode', 400);
+  it('create from savedObject not found error should have statusCode 404', () => {
+    const error = SavedObjectsErrorHelpers.createGenericNotFoundError('type', 'id');
+    expect(createDataSourceError(error)).toHaveProperty('statusCode', 404);
   });
 
-  it('create from savedObject service unavailable error should be a 500 error', () => {
+  it('create from savedObject service unavailable error should have statusCode 503', () => {
     const error = SavedObjectsErrorHelpers.decorateOpenSearchUnavailableError(
       new Error('test reason message')
     );
-    expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({
-      statusCode: 500,
-      message: 'test prefix: test reason message',
+    expect(createDataSourceError(error)).toMatchObject({
+      statusCode: 503,
+      message: 'Data Source Error: test reason message',
     });
   });
 
   it('create from non savedObject error should always be a 400 error', () => {
     const error = new Error('test reason message');
-    expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({
+    expect(createDataSourceError(error)).toMatchObject({
       statusCode: 400,
-      message: 'test prefix: test reason message',
+      message: 'Data Source Error: test reason message',
     });
   });
+
+  it('create from client response error 401 should be casted to a 400 DataSourceError', () => {
+    expect(
+      createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 401 })))
+    ).toHaveProperty('statusCode', 400);
+  });
+
+  it('create from non 401 client response error should respect original statusCode', () => {
+    expect(
+      createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 403 })))
+    ).toHaveProperty('statusCode', 403);
+    expect(
+      createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 404 })))
+    ).toHaveProperty('statusCode', 404);
+    expect(
+      createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 500 })))
+    ).toHaveProperty('statusCode', 500);
+  });
+
+  it('create from non-response client error should be casted to a 400 DataSourceError', () => {
+    expect(
+      createDataSourceError(new ConnectionError('error', createApiResponseError()))
+    ).toHaveProperty('statusCode', 400);
+    expect(
+      createDataSourceError(new NoLivingConnectionsError('error', createApiResponseError()))
+    ).toHaveProperty('statusCode', 400);
+    expect(createDataSourceError(new Error('foo'))).toHaveProperty('statusCode', 400);
+  });
+
+  it('create from legacy client 401 error should be casted to a 400 DataSourceError', () => {
+    expect(createDataSourceError(new LegacyErrors.AuthenticationException())).toEqual(
+      new DataSourceError(new Error('dummy'), 'Authentication Exception', 400)
+    );
+  });
+
+  it('create from legacy client non 401 error should respect original statusCode', () => {
+    expect(createDataSourceError(new LegacyErrors.NotFound())).toEqual(
+      new DataSourceError(new Error('dummy'), 'Not Found', 404)
+    );
+    expect(createDataSourceError(new LegacyErrors.TooManyRequests())).toEqual(
+      new DataSourceError(new Error('dummy'), 'Too Many Requests', 429)
+    );
+    expect(createDataSourceError(new LegacyErrors.InternalServerError())).toEqual(
+      new DataSourceError(new Error('dummy'), 'Internal Server Error', 400)
+    );
+  });
+
+  it('create from legacy client error should be casted to a 400 DataSourceError', () => {
+    expect(createDataSourceError(new LegacyErrors.NoConnections())).toEqual(
+      new DataSourceError(new Error('dummy'), 'No Living connections', 400)
+    );
+    expect(createDataSourceError(new LegacyErrors.ConnectionFault())).toEqual(
+      new DataSourceError(new Error('dummy'), 'Connection Failure', 400)
+    );
+  });
 });
diff --git a/src/plugins/data_source/server/lib/error.ts b/src/plugins/data_source/server/lib/error.ts
@@ -3,19 +3,47 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import { ResponseError } from '@opensearch-project/opensearch/lib/errors';
+import { errors as LegacyErrors } from 'elasticsearch';
 import { SavedObjectsErrorHelpers } from '../../../../../src/core/server';
-import { OsdError } from '../../../../../src/plugins/opensearch_dashboards_utils/common';
+import { OsdError } from '../../../opensearch_dashboards_utils/common';
 
-export class DataSourceConfigError extends OsdError {
+export class DataSourceError extends OsdError {
   // must have statusCode to avoid route handler in search.ts to return 500
   statusCode: number;
-  constructor(messagePrefix: string, error: any) {
-    const messageContent = SavedObjectsErrorHelpers.isSavedObjectsClientError(error)
-      ? error.output.payload.message
-      : error.message;
-    super(messagePrefix + messageContent);
-    // Cast all 5xx error returned by saveObjectClient to 500.
-    // Cast both savedObject client 4xx errors, and other errors to 400
-    this.statusCode = SavedObjectsErrorHelpers.isOpenSearchUnavailableError(error) ? 500 : 400;
+  constructor(error: any, message?: string, statusCode?: number) {
+    message = message ? message : error.message;
+    super('Data Source Error: ' + message);
+    if (statusCode) {
+      this.statusCode = statusCode;
+    } else if (error.statusCode) {
+      this.statusCode = error.statusCode;
+    } else {
+      this.statusCode = 400;
+    }
   }
 }
+
+export const createDataSourceError = (error: any, message?: string): DataSourceError => {
+  // handle saved object client error, while retrieve data source meta info
+  if (SavedObjectsErrorHelpers.isSavedObjectsClientError(error)) {
+    return new DataSourceError(error, error.output.payload.message, error.output.statusCode);
+  }
+
+  // cast OpenSearch client 401 response error to 400, due to https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2591
+  if (isResponseError(error) && error.statusCode === 401) {
+    return new DataSourceError(error, JSON.stringify(error.meta.body), 400);
+  }
+
+  // cast legacy client 401 response error to 400
+  if (error instanceof LegacyErrors.AuthenticationException) {
+    return new DataSourceError(error, error.message, 400);
+  }
+
+  // handle all other error that may or may not comes with statuscode
+  return new DataSourceError(error, message);
+};
+
+const isResponseError = (error: any): error is ResponseError => {
+  return Boolean(error.body && error.statusCode && error.headers);
+};
diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts
@@ -28,6 +28,7 @@ import { DATA_SOURCE_SAVED_OBJECT_TYPE } from '../common';
 
 // eslint-disable-next-line @osd/eslint/no-restricted-paths
 import { ensureRawRequest } from '../../../../src/core/server/http/router';
+import { createDataSourceError } from './lib/error';
 export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourcePluginStart> {
   private readonly logger: Logger;
   private readonly cryptographyService: CryptographyService;
@@ -102,7 +103,9 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
       )
     );
 
-    return {};
+    return {
+      createDataSourceError: (e: any) => createDataSourceError(e),
+    };
   }
 
   public start(core: CoreStart) {
diff --git a/src/plugins/data_source/server/types.ts b/src/plugins/data_source/server/types.ts
diff --git a/src/plugins/index_pattern_management/server/routes/resolve_index.ts b/src/plugins/index_pattern_management/server/routes/resolve_index.ts

Original file line number	Diff line number	Diff line change
`@@ -301,21 +301,11 @@ export class Router implements IRouter {`
`301`	`301`	`return e;`
`302`	`302`	`}`
`303`	`303`
`304`		`- if (isDataSourceConfigError(e)) {`
`305`		`- return hapiResponseAdapter.handle(`
`306`		`- opensearchDashboardsResponseFactory.badRequest({ body: e.message })`
`307`		`- );`
`308`		`- }`
`309`		`-`
`310`	`304`	`return hapiResponseAdapter.toInternalError();`
`311`	`305`	`}`
`312`	`306`	`}`
`313`	`307`	`}`
`314`	`308`
`315`		`-const isDataSourceConfigError = (error: any) => {`
`316`		`- return error.constructor.name === 'DataSourceConfigError' && error.statusCode === 400;`
`317`		`-};`
`318`		`-`
`319`	`309`	`const convertOpenSearchUnauthorized = (`
`320`	`310`	`e: OpenSearchNotAuthorizedError`
`321`	`311`	`): ErrorHttpResponseOptions => {`