From e57009ce0374b0315d3c43cb4f2b33ba2a3af8bb Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Sep 2020 17:02:28 -0700 Subject: [PATCH 1/7] Update/refactor EnterpriseSearchRequestHandler to manage internal endpoint error behavior + pull out / refactor all errors into their own methods instead of relying on a single 'error connecting' message --- .../enterprise_search_request_handler.test.ts | 235 ++++++++++++------ .../lib/enterprise_search_request_handler.ts | 133 ++++++++-- 2 files changed, 280 insertions(+), 88 deletions(-) diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts index 34f83ef3a3fd2..68db0e4486230 100644 --- a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts +++ b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts @@ -30,109 +30,183 @@ describe('EnterpriseSearchRequestHandler', () => { fetchMock.mockReset(); }); - it('makes an API call and returns the response', async () => { - const responseBody = { - results: [{ name: 'engine1' }], - meta: { page: { total_results: 1 } }, - }; + describe('createRequest()', () => { + it('makes an API call and returns the response', async () => { + const responseBody = { + results: [{ name: 'engine1' }], + meta: { page: { total_results: 1 } }, + }; - EnterpriseSearchAPI.mockReturn(responseBody); + EnterpriseSearchAPI.mockReturn(responseBody); - const requestHandler = enterpriseSearchRequestHandler.createRequest({ - path: '/as/credentials/collection', - }); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/as/credentials/collection', + }); - await makeAPICall(requestHandler, { - query: { - type: 'indexed', - pageIndex: 1, - }, - }); + await makeAPICall(requestHandler, { + query: { + type: 'indexed', + pageIndex: 1, + }, + }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith( - 'http://localhost:3002/as/credentials/collection?type=indexed&pageIndex=1', - { method: 'GET' } - ); + EnterpriseSearchAPI.shouldHaveBeenCalledWith( + 'http://localhost:3002/as/credentials/collection?type=indexed&pageIndex=1', + { method: 'GET' } + ); - expect(responseMock.custom).toHaveBeenCalledWith({ - body: responseBody, - statusCode: 200, + expect(responseMock.custom).toHaveBeenCalledWith({ + body: responseBody, + statusCode: 200, + }); }); - }); - describe('request passing', () => { - it('passes route method', async () => { - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' }); + describe('request passing', () => { + it('passes route method', async () => { + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + }); + + await makeAPICall(requestHandler, { route: { method: 'POST' } }); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { + method: 'POST', + }); - await makeAPICall(requestHandler, { route: { method: 'POST' } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { - method: 'POST', + await makeAPICall(requestHandler, { route: { method: 'DELETE' } }); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { + method: 'DELETE', + }); }); - await makeAPICall(requestHandler, { route: { method: 'DELETE' } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { - method: 'DELETE', + it('passes request body', async () => { + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + }); + await makeAPICall(requestHandler, { body: { bodacious: true } }); + + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { + body: '{"bodacious":true}', + }); }); - }); - it('passes request body', async () => { - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' }); - await makeAPICall(requestHandler, { body: { bodacious: true } }); + it('passes custom params set by the handler, which override request params', async () => { + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + params: { someQuery: true }, + }); + await makeAPICall(requestHandler, { query: { someQuery: false } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { - body: '{"bodacious":true}', + EnterpriseSearchAPI.shouldHaveBeenCalledWith( + 'http://localhost:3002/api/example?someQuery=true' + ); }); }); - it('passes custom params set by the handler, which override request params', async () => { - const requestHandler = enterpriseSearchRequestHandler.createRequest({ - path: '/api/example', - params: { someQuery: true }, + describe('response passing', () => { + it('returns the response status code from Enterprise Search', async () => { + EnterpriseSearchAPI.mockReturn({}, { status: 201 }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + }); + await makeAPICall(requestHandler); + + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example'); + expect(responseMock.custom).toHaveBeenCalledWith({ body: {}, statusCode: 201 }); }); - await makeAPICall(requestHandler, { query: { someQuery: false } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith( - 'http://localhost:3002/api/example?someQuery=true' - ); + // TODO: It's possible we may also pass back headers at some point + // from Enterprise Search, e.g. the x-read-only mode header }); }); - describe('response passing', () => { - it('returns the response status code from Enterprise Search', async () => { - EnterpriseSearchAPI.mockReturn({}, { status: 404 }); + describe('error responses', () => { + describe('handleClientError()', () => { + afterEach(() => { + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/4xx'); + expect(mockLogger.error).not.toHaveBeenCalled(); + }); - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' }); - await makeAPICall(requestHandler); + it('passes back json.error', async () => { + const error = 'some error message'; + EnterpriseSearchAPI.mockReturn({ error }, { status: 404, headers: JSON_HEADER }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example'); - expect(responseMock.custom).toHaveBeenCalledWith({ body: {}, statusCode: 404 }); - }); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); - // TODO: It's possible we may also pass back headers at some point - // from Enterprise Search, e.g. the x-read-only mode header - }); + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 404, + body: { + message: 'some error message', + attributes: { errors: ['some error message'] }, + }, + }); + }); - describe('error handling', () => { - afterEach(() => { - expect(mockLogger.error).toHaveBeenCalledWith( - expect.stringContaining('Error connecting to Enterprise Search') - ); + it('passes back json.errors', async () => { + const errors = ['one', 'two', 'three']; + EnterpriseSearchAPI.mockReturn({ errors }, { status: 400, headers: JSON_HEADER }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { + message: 'one,two,three', + attributes: { errors: ['one', 'two', 'three'] }, + }, + }); + }); + + it('handles empty json', async () => { + EnterpriseSearchAPI.mockReturn({}, { status: 400, headers: JSON_HEADER }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { + message: 'Bad Request', + attributes: { errors: ['Bad Request'] }, + }, + }); + }); + + it('handles blank bodies', async () => { + EnterpriseSearchAPI.mockReturn(undefined as any, { status: 404 }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 404, + body: { + message: 'Not Found', + attributes: { errors: ['Not Found'] }, + }, + }); + }); }); - it('returns an error when an API request fails', async () => { - EnterpriseSearchAPI.mockReturnError(); - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/failed' }); + it('handleServerError()', async () => { + EnterpriseSearchAPI.mockReturn('something crashed!' as any, { status: 500 }); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/5xx' }); await makeAPICall(requestHandler); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/failed'); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/5xx'); expect(responseMock.customError).toHaveBeenCalledWith({ - body: 'Error connecting to Enterprise Search: Failed', statusCode: 502, + body: expect.stringContaining('Enterprise Search encountered an internal server error'), }); + expect(mockLogger.error).toHaveBeenCalledWith( + 'Enterprise Search Server Error 500 at : "something crashed!"' + ); }); - it('returns an error when `hasValidData` fails', async () => { + it('handleInvalidDataError()', async () => { EnterpriseSearchAPI.mockReturn({ results: false }); const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/invalid', @@ -143,15 +217,29 @@ describe('EnterpriseSearchRequestHandler', () => { EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/invalid'); expect(responseMock.customError).toHaveBeenCalledWith({ - body: 'Error connecting to Enterprise Search: Invalid data received', statusCode: 502, + body: 'Invalid data received from Enterprise Search', }); - expect(mockLogger.debug).toHaveBeenCalledWith( + expect(mockLogger.error).toHaveBeenCalledWith( 'Invalid data received from : {"results":false}' ); }); - describe('user authentication errors', () => { + it('handleConnectionError()', async () => { + EnterpriseSearchAPI.mockReturnError(); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/failed' }); + + await makeAPICall(requestHandler); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/failed'); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 502, + body: 'Error connecting to Enterprise Search: Failed', + }); + expect(mockLogger.error).toHaveBeenCalled(); + }); + + describe('handleAuthenticationError()', () => { afterEach(async () => { const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/unauthenticated', @@ -160,9 +248,10 @@ describe('EnterpriseSearchRequestHandler', () => { EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/unauthenticated'); expect(responseMock.customError).toHaveBeenCalledWith({ - body: 'Error connecting to Enterprise Search: Cannot authenticate Enterprise Search user', statusCode: 502, + body: 'Cannot authenticate Enterprise Search user', }); + expect(mockLogger.error).toHaveBeenCalled(); }); it('errors when redirected to /login', async () => { @@ -175,7 +264,7 @@ describe('EnterpriseSearchRequestHandler', () => { }); }); - it('has a helper for checking empty objects', async () => { + it('isEmptyObj', async () => { expect(enterpriseSearchRequestHandler.isEmptyObj({})).toEqual(true); expect(enterpriseSearchRequestHandler.isEmptyObj({ empty: false })).toEqual(false); }); diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts index 18f10c590847c..00d5eaf5d6a83 100644 --- a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts +++ b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import fetch from 'node-fetch'; +import fetch, { Response } from 'node-fetch'; import querystring from 'querystring'; import { RequestHandler, @@ -25,6 +25,12 @@ interface IRequestParams { params?: object; hasValidData?: (body?: ResponseBody) => boolean; } +interface IErrorResponse { + message: string; + attributes: { + errors: string[]; + }; +} export interface IEnterpriseSearchRequestHandler { createRequest(requestParams?: object): RequestHandler; } @@ -71,34 +77,131 @@ export class EnterpriseSearchRequestHandler { ? JSON.stringify(request.body) : undefined; - // Call the Enterprise Search API and pass back response to the front-end + // Call the Enterprise Search API const apiResponse = await fetch(url, { method, headers, body }); + // Handle authentication redirects if (apiResponse.url.endsWith('/login') || apiResponse.url.endsWith('/ent/select')) { - throw new Error('Cannot authenticate Enterprise Search user'); + return this.handleAuthenticationError(response); } + // Handle 400-500+ responses from the Enterprise Search server const { status } = apiResponse; - const json = await apiResponse.json(); + if (status >= 500) { + return this.handleServerError(response, apiResponse, url); + } else if (status >= 400) { + return this.handleClientError(response, apiResponse); + } - if (hasValidData(json)) { - return response.custom({ statusCode: status, body: json }); - } else { - this.log.debug(`Invalid data received from <${url}>: ${JSON.stringify(json)}`); - throw new Error('Invalid data received'); + // Check returned data + const json = await apiResponse.json(); + if (!hasValidData(json)) { + return this.handleInvalidDataError(response, url, json); } + + // Pass successful responses back to the front-end + return response.custom({ statusCode: status, body: json }); } catch (e) { - const errorMessage = `Error connecting to Enterprise Search: ${e?.message || e.toString()}`; + // Catch connection/auth errors + return this.handleConnectionError(response, e); + } + }; + } - this.log.error(errorMessage); - if (e instanceof Error) this.log.debug(e.stack as string); + /** + * Attempt to grab a usable error body from Enterprise Search - this isn't + * always possible because some of our internal endpoints send back blank + * bodies, and sometimes the server sends back Ruby on Rails error pages + */ + async getErrorResponseBody(apiResponse: Response) { + const { statusText } = apiResponse; + const contentType = apiResponse.headers.get('content-type') || ''; - return response.customError({ statusCode: 502, body: errorMessage }); - } + // Default response + let body: IErrorResponse = { + message: statusText, + attributes: { errors: [statusText] }, }; + + try { + if (contentType.includes('application/json')) { + // Try parsing body as JSON + const json = await apiResponse.json(); + + // Some of our internal endpoints return either an `error` or `errors` key, + // which can both return either a string or array of strings ¯\_(ツ)_/¯ + const errors = json.error || json.errors || [statusText]; + body = { + message: errors.toString(), + attributes: { errors: Array.isArray(errors) ? errors : [errors] }, + }; + } else { + // Try parsing body as text/html + const text = await apiResponse.text(); + if (text) { + body = { + message: text, + attributes: { errors: [text] }, + }; + } + } + } catch { + // Fail silently + } + + return body; + } + + /** + * Error response helpers + */ + + async handleClientError(response: KibanaResponseFactory, apiResponse: Response) { + const { status } = apiResponse; + const body = await this.getErrorResponseBody(apiResponse); + + return response.customError({ statusCode: status, body }); + } + + async handleServerError(response: KibanaResponseFactory, apiResponse: Response, url: string) { + const { status } = apiResponse; + const { message } = await this.getErrorResponseBody(apiResponse); + + // Don't expose server errors to the front-end, as they may contain sensitive stack traces + const errorMessage = + 'Enterprise Search encountered an internal server error. Please contact your system administrator if the problem persists.'; + + this.log.error(`Enterprise Search Server Error ${status} at <${url}>: ${message}`); + return response.customError({ statusCode: 502, body: errorMessage }); + } + + handleInvalidDataError(response: KibanaResponseFactory, url: string, json: object) { + const errorMessage = 'Invalid data received from Enterprise Search'; + + this.log.error(`Invalid data received from <${url}>: ${JSON.stringify(json)}`); + return response.customError({ statusCode: 502, body: errorMessage }); } - // Small helper + handleConnectionError(response: KibanaResponseFactory, e: Error) { + const errorMessage = `Error connecting to Enterprise Search: ${e?.message || e.toString()}`; + + this.log.error(errorMessage); + if (e instanceof Error) this.log.debug(e.stack as string); + + return response.customError({ statusCode: 502, body: errorMessage }); + } + + handleAuthenticationError(response: KibanaResponseFactory) { + const errorMessage = 'Cannot authenticate Enterprise Search user'; + + this.log.error(errorMessage); + return response.customError({ statusCode: 502, body: errorMessage }); + } + + /** + * Misc helpers + */ + isEmptyObj(obj: object) { return Object.keys(obj).length === 0; } From 0e1f2ebd8fcde8b61e07b29b5bfb25e2010042f3 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Sep 2020 17:49:00 -0700 Subject: [PATCH 2/7] Fix http interceptor bug preventing 4xx/5xx responses from being caught --- .../public/applications/shared/http/http_logic.test.ts | 9 ++++++--- .../public/applications/shared/http/http_logic.ts | 4 +++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.test.ts index a6957340d33d3..c032e3b04ebe6 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.test.ts @@ -74,21 +74,24 @@ describe('HttpLogic', () => { describe('errorConnectingInterceptor', () => { it('handles errors connecting to Enterprise Search', async () => { const { responseError } = mockHttp.intercept.mock.calls[0][0] as any; - await responseError({ response: { url: '/api/app_search/engines', status: 502 } }); + const httpResponse = { response: { url: '/api/app_search/engines', status: 502 } }; + await expect(responseError(httpResponse)).rejects.toEqual(httpResponse); expect(HttpLogic.actions.setErrorConnecting).toHaveBeenCalled(); }); it('does not handle non-502 Enterprise Search errors', async () => { const { responseError } = mockHttp.intercept.mock.calls[0][0] as any; - await responseError({ response: { url: '/api/workplace_search/overview', status: 404 } }); + const httpResponse = { response: { url: '/api/workplace_search/overview', status: 404 } }; + await expect(responseError(httpResponse)).rejects.toEqual(httpResponse); expect(HttpLogic.actions.setErrorConnecting).not.toHaveBeenCalled(); }); it('does not handle errors for unrelated calls', async () => { const { responseError } = mockHttp.intercept.mock.calls[0][0] as any; - await responseError({ response: { url: '/api/some_other_plugin/', status: 502 } }); + const httpResponse = { response: { url: '/api/some_other_plugin/', status: 502 } }; + await expect(responseError(httpResponse)).rejects.toEqual(httpResponse); expect(HttpLogic.actions.setErrorConnecting).not.toHaveBeenCalled(); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts b/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts index fb2d9b1061723..5b694a49d0656 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts @@ -68,7 +68,9 @@ export const HttpLogic = kea>({ if (isApiResponse && hasErrorConnecting) { actions.setErrorConnecting(true); } - return httpResponse; + + // Re-throw error so that downstream catches work as expected + return Promise.reject(httpResponse); }, }); httpInterceptors.push(errorConnectingInterceptor); From 2b949c5eb3bad3cab465c61d7c162c6e1ea26012 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Sep 2020 17:51:36 -0700 Subject: [PATCH 3/7] Add handleAPIError helper - New and improved from ent-search - this one automatically sets flash messages for you so you don't have to pass in a callback! --- .../flash_messages/handle_api_errors.test.ts | 50 +++++++++++++++++++ .../flash_messages/handle_api_errors.ts | 46 +++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts create mode 100644 x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts new file mode 100644 index 0000000000000..b04c8aff772f1 --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts @@ -0,0 +1,50 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +jest.mock('./', () => ({ + FlashMessagesLogic: { actions: { setFlashMessages: jest.fn() } }, +})); +import { FlashMessagesLogic } from './'; + +import { handleAPIError } from './handle_api_errors'; + +describe('handleAPIError', () => { + const mockHttpError = { + body: { + statusCode: 404, + error: 'Not Found', + message: 'Could not find X,Could not find Y,Something else bad happened', + attributes: { + errors: ['Could not find X', 'Could not find Y', 'Something else bad happened'], + }, + }, + } as any; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('converts API errors into flash messages', () => { + handleAPIError(mockHttpError); + + expect(FlashMessagesLogic.actions.setFlashMessages).toHaveBeenCalledWith([ + { type: 'error', message: 'Could not find X' }, + { type: 'error', message: 'Could not find Y' }, + { type: 'error', message: 'Something else bad happened' }, + ]); + }); + + it('displays a generic error message and re-throws non-API errors', () => { + try { + handleAPIError(Error('whatever') as any); + } catch (e) { + expect(e.message).toEqual('whatever'); + expect(FlashMessagesLogic.actions.setFlashMessages).toHaveBeenCalledWith([ + { type: 'error', message: 'An unexpected error occurred' }, + ]); + } + }); +}); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts new file mode 100644 index 0000000000000..52744d67bf36c --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts @@ -0,0 +1,46 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { HttpResponse } from 'src/core/public'; + +import { FlashMessagesLogic, IFlashMessage } from './'; + +/** + * The API errors we are handling can come from one of two ways: + * - When our http calls recieve a response containing an error code, such as a 404 or 500 + * - Our own JS while handling a successful response + * + * In the first case, if it is a purposeful error (like a 404) we will receive an + * `errors` property in the response's data, which will contain messages we can + * display to the user. + */ +interface IErrorResponse { + statusCode: number; + error: string; + message: string; + attributes: { + errors: string[]; + }; +} + +/** + * Converts API/HTTP errors into user-facing Flash Messages + */ +export const handleAPIError = (error: HttpResponse) => { + const defaultErrorMessage = 'An unexpected error occurred'; + + const errorFlashMessages: IFlashMessage[] = Array.isArray(error?.body?.attributes?.errors) + ? error.body!.attributes.errors.map((message) => ({ type: 'error', message })) + : [{ type: 'error', message: defaultErrorMessage }]; + + FlashMessagesLogic.actions.setFlashMessages(errorFlashMessages); + + // If this was a programming error or a failed request (such as a CORS) error, + // we rethrow the error so it shows up in the developer console + if (!error?.body?.message) { + throw error; + } +}; From ab889eed3e75d5125c7710b3057fe7298918da53 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Sep 2020 19:03:42 -0700 Subject: [PATCH 4/7] Fix type check --- .../public/applications/shared/http/http_logic.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts b/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts index 5b694a49d0656..ec9db30ddef3b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts @@ -6,7 +6,7 @@ import { kea, MakeLogicType } from 'kea'; -import { HttpSetup } from 'src/core/public'; +import { HttpSetup, HttpInterceptorResponseError } from 'src/core/public'; export interface IHttpValues { http: HttpSetup; @@ -70,7 +70,7 @@ export const HttpLogic = kea>({ } // Re-throw error so that downstream catches work as expected - return Promise.reject(httpResponse); + return Promise.reject(httpResponse) as Promise; }, }); httpInterceptors.push(errorConnectingInterceptor); From dcdd872bdd26fce151a651a846adc15939a9264a Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Sep 2020 19:20:39 -0700 Subject: [PATCH 5/7] [Feedback] Add option to queue flash messages to handleAPIError - Should be hopefully useful for calls that need to queue an error and then redirect to another page --- .../flash_messages/handle_api_errors.test.ts | 17 ++++++++++++++++- .../shared/flash_messages/handle_api_errors.ts | 14 ++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts index b04c8aff772f1..45fc6749e3426 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts @@ -5,7 +5,12 @@ */ jest.mock('./', () => ({ - FlashMessagesLogic: { actions: { setFlashMessages: jest.fn() } }, + FlashMessagesLogic: { + actions: { + setFlashMessages: jest.fn(), + setQueuedMessages: jest.fn(), + }, + }, })); import { FlashMessagesLogic } from './'; @@ -37,6 +42,16 @@ describe('handleAPIError', () => { ]); }); + it('queues messages when isQueued is passed', () => { + handleAPIError(mockHttpError, { isQueued: true }); + + expect(FlashMessagesLogic.actions.setQueuedMessages).toHaveBeenCalledWith([ + { type: 'error', message: 'Could not find X' }, + { type: 'error', message: 'Could not find Y' }, + { type: 'error', message: 'Something else bad happened' }, + ]); + }); + it('displays a generic error message and re-throws non-API errors', () => { try { handleAPIError(Error('whatever') as any); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts index 52744d67bf36c..9089aba147da2 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts @@ -25,18 +25,28 @@ interface IErrorResponse { errors: string[]; }; } +interface IOptions { + isQueued?: boolean; +} /** * Converts API/HTTP errors into user-facing Flash Messages */ -export const handleAPIError = (error: HttpResponse) => { +export const handleAPIError = ( + error: HttpResponse, + { isQueued }: IOptions = {} +) => { const defaultErrorMessage = 'An unexpected error occurred'; const errorFlashMessages: IFlashMessage[] = Array.isArray(error?.body?.attributes?.errors) ? error.body!.attributes.errors.map((message) => ({ type: 'error', message })) : [{ type: 'error', message: defaultErrorMessage }]; - FlashMessagesLogic.actions.setFlashMessages(errorFlashMessages); + if (isQueued) { + FlashMessagesLogic.actions.setQueuedMessages(errorFlashMessages); + } else { + FlashMessagesLogic.actions.setFlashMessages(errorFlashMessages); + } // If this was a programming error or a failed request (such as a CORS) error, // we rethrow the error so it shows up in the developer console From 9bf80691dd228d336064eb1202fcc9eb9a1d86c5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Mon, 14 Sep 2020 08:12:50 -0700 Subject: [PATCH 6/7] PR feedback: Add test case for bodies flagged as JSON which are not --- .../lib/enterprise_search_request_handler.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts index 68db0e4486230..0c1e81e3aba46 100644 --- a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts +++ b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts @@ -174,6 +174,21 @@ describe('EnterpriseSearchRequestHandler', () => { }); }); + it('handles invalid json', async () => { + EnterpriseSearchAPI.mockReturn('invalid' as any, { status: 400, headers: JSON_HEADER }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { + message: 'Bad Request', + attributes: { errors: ['Bad Request'] }, + }, + }); + }); + it('handles blank bodies', async () => { EnterpriseSearchAPI.mockReturn(undefined as any, { status: 404 }); From ad4f8fdd0b057b53257b3116cb2baaabb818acd5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Mon, 14 Sep 2020 08:55:09 -0700 Subject: [PATCH 7/7] Rename handleAPIError to flashAPIErrors --- .../shared/flash_messages/handle_api_errors.test.ts | 10 +++++----- .../shared/flash_messages/handle_api_errors.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts index 45fc6749e3426..6f2aec757eb37 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.test.ts @@ -14,9 +14,9 @@ jest.mock('./', () => ({ })); import { FlashMessagesLogic } from './'; -import { handleAPIError } from './handle_api_errors'; +import { flashAPIErrors } from './handle_api_errors'; -describe('handleAPIError', () => { +describe('flashAPIErrors', () => { const mockHttpError = { body: { statusCode: 404, @@ -33,7 +33,7 @@ describe('handleAPIError', () => { }); it('converts API errors into flash messages', () => { - handleAPIError(mockHttpError); + flashAPIErrors(mockHttpError); expect(FlashMessagesLogic.actions.setFlashMessages).toHaveBeenCalledWith([ { type: 'error', message: 'Could not find X' }, @@ -43,7 +43,7 @@ describe('handleAPIError', () => { }); it('queues messages when isQueued is passed', () => { - handleAPIError(mockHttpError, { isQueued: true }); + flashAPIErrors(mockHttpError, { isQueued: true }); expect(FlashMessagesLogic.actions.setQueuedMessages).toHaveBeenCalledWith([ { type: 'error', message: 'Could not find X' }, @@ -54,7 +54,7 @@ describe('handleAPIError', () => { it('displays a generic error message and re-throws non-API errors', () => { try { - handleAPIError(Error('whatever') as any); + flashAPIErrors(Error('whatever') as any); } catch (e) { expect(e.message).toEqual('whatever'); expect(FlashMessagesLogic.actions.setFlashMessages).toHaveBeenCalledWith([ diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts index 9089aba147da2..5e56c4fb0bd22 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts @@ -32,7 +32,7 @@ interface IOptions { /** * Converts API/HTTP errors into user-facing Flash Messages */ -export const handleAPIError = ( +export const flashAPIErrors = ( error: HttpResponse, { isQueued }: IOptions = {} ) => {