From 5c575594681c9d2846025d52c44fbfdd1eb8ef45 Mon Sep 17 00:00:00 2001 From: Su Date: Tue, 25 Oct 2022 02:29:13 -0700 Subject: [PATCH] refactor data source error handling Signed-off-by: Su --- src/core/server/http/router/router.ts | 6 +- .../opensearch_search_strategy.ts | 12 +--- .../server/client/configure_client.ts | 10 ++-- src/plugins/data_source/server/index.ts | 2 +- .../server/legacy/configure_legacy_client.ts | 7 +-- .../data_source/server/lib/error.test.ts | 12 ++-- src/plugins/data_source/server/lib/error.ts | 57 +++++++++++++++---- 7 files changed, 68 insertions(+), 38 deletions(-) diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index 047639372454..86ca48624642 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -301,7 +301,7 @@ export class Router implements IRouter { return e; } - if (isDataSourceConfigError(e)) { + if (isDataSourceError(e)) { return hapiResponseAdapter.handle( opensearchDashboardsResponseFactory.badRequest({ body: e.message }) ); @@ -312,8 +312,8 @@ export class Router implements IRouter { } } -const isDataSourceConfigError = (error: any) => { - return error.constructor.name === 'DataSourceConfigError' && error.statusCode === 400; +const isDataSourceError = (error: any) => { + return error.constructor.name === 'DataSourceError' && error.statusCode === 400; }; const convertOpenSearchUnauthorized = ( 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 c4a8f03c0710..02e8bea9cf54 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 @@ -32,10 +32,8 @@ import { first } from 'rxjs/operators'; import { SearchResponse } from 'elasticsearch'; import { Observable } from 'rxjs'; import { ApiResponse } from '@opensearch-project/opensearch'; -// eslint-disable-next-line @osd/eslint/no-restricted-paths -import { isResponseError } from '../../../../../core/server/opensearch/client/errors'; -import { SharedGlobalConfig, Logger, SavedObjectsErrorHelpers } from '../../../../../core/server'; -import { DataSourceConfigError } from '../../../../data_source/server'; +import { createDataSourceError } from '../../../../data_source/server'; +import { SharedGlobalConfig, Logger } from '../../../../../core/server'; import { SearchUsage } from '../collectors/usage'; import { toSnakeCase } from './to_snake_case'; import { @@ -92,11 +90,7 @@ export const opensearchSearchStrategyProvider = ( } catch (e: any) { if (usage) usage.trackError(); if (request.dataSourceId) { - const errorMessage = isResponseError(e) ? JSON.stringify(e.meta.body) : undefined; - throw new DataSourceConfigError( - `Failed to search against data source id [${request.dataSourceId}]: `, - SavedObjectsErrorHelpers.decorateBadRequestError(e, errorMessage) - ); + throw createDataSourceError(e); } throw e; } diff --git a/src/plugins/data_source/server/client/configure_client.ts b/src/plugins/data_source/server/client/configure_client.ts index 0e153f89b0f9..c4d1b4eef9f7 100644 --- 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/index.ts b/src/plugins/data_source/server/index.ts index 3f5826c63704..2d92c149b3a4 100644 --- a/src/plugins/data_source/server/index.ts +++ b/src/plugins/data_source/server/index.ts @@ -17,4 +17,4 @@ export function plugin(initializerContext: PluginInitializerContext) { export { DataSourcePluginSetup, DataSourcePluginStart } from './types'; -export { DataSourceConfigError } from './lib/error'; +export { DataSourceError, createDataSourceError } from './lib/error'; diff --git a/src/plugins/data_source/server/legacy/configure_legacy_client.ts b/src/plugins/data_source/server/legacy/configure_legacy_client.ts index 2921e0cad48e..94e6b40a4d2e 100644 --- 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 index fca7efb0904c..fd73ee4d9b32 100644 --- a/src/plugins/data_source/server/lib/error.test.ts +++ b/src/plugins/data_source/server/lib/error.test.ts @@ -4,12 +4,12 @@ */ import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; -import { DataSourceConfigError } from './error'; +import { DataSourceError } from './error'; -describe('DataSourceConfigError', () => { +describe('DataSourceError', () => { it('create from savedObject bad request error should be 400 error', () => { const error = SavedObjectsErrorHelpers.createBadRequestError('test reason message'); - expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ + expect(new DataSourceError('test prefix: ', error)).toMatchObject({ statusCode: 400, message: 'test prefix: test reason message: Bad Request', }); @@ -17,14 +17,14 @@ describe('DataSourceConfigError', () => { 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); + expect(new DataSourceError('test prefix: ', error)).toHaveProperty('statusCode', 400); }); it('create from savedObject service unavailable error should be a 500 error', () => { const error = SavedObjectsErrorHelpers.decorateOpenSearchUnavailableError( new Error('test reason message') ); - expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ + expect(new DataSourceError('test prefix: ', error)).toMatchObject({ statusCode: 500, message: 'test prefix: test reason message', }); @@ -32,7 +32,7 @@ describe('DataSourceConfigError', () => { 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(new DataSourceError('test prefix: ', error)).toMatchObject({ statusCode: 400, message: 'test prefix: test reason message', }); diff --git a/src/plugins/data_source/server/lib/error.ts b/src/plugins/data_source/server/lib/error.ts index ba3e700f6740..233be65c2596 100644 --- a/src/plugins/data_source/server/lib/error.ts +++ b/src/plugins/data_source/server/lib/error.ts @@ -3,19 +3,56 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { ResponseError } from '@opensearch-project/opensearch/lib/errors'; +import { errors } from 'elasticsearch'; import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; 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) => { + // handle saved object client error, while retrieve data source meta info + if (SavedObjectsErrorHelpers.isSavedObjectsClientError(error)) { + const statusCode = SavedObjectsErrorHelpers.isOpenSearchUnavailableError(error) + ? 500 + : error.output.statusCode; + return new DataSourceError(error, error.output.payload.message, statusCode); + } + + // cast OpenSearch client 500 response error to 400 + // cast OpenSearch client 401 response error to 400, due to https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2591 + if ( + (isResponseError(error) && error.statusCode === 500) || + (isResponseError(error) && error.statusCode === 401) + ) { + return new DataSourceError(error, JSON.stringify(error.meta.body), 400); + } + + // cast legacy client 500, 401 response error to 400 + if ( + error instanceof errors.AuthenticationException || + error instanceof errors.InternalServerError + ) { + return new DataSourceError(error, error.message, 400); + } + + // 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); +};