Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper #77258

Merged
merged 8 commits into from
Sep 15, 2020
Original file line number Diff line number Diff line change
@@ -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' },
]);
}
});
});
Original file line number Diff line number Diff line change
@@ -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<IErrorResponse>) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wanted to raise that I removed the onErrorCallback arg because I thought it was cleaner to have the helper be in charge of setting FlashMessages vs. passing in setFlashMessages each time.

It's possible we'll want/need to pass a custom callback in the future, but I can't think of a really useful case for a passed callback currently (esp with try/catch syntax) - can anyone else?

Another thought: since I'm making this handler specific to flash messages (I placed it in shared/flash_messages because I thought it made a lot of sense in there), should we consider renaming this to something like flashAPIErrors instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine since its coupled to the messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rad! I went ahead renamed handleAPIError to flashAPIErrors here: ad4f8fd

CC @byronhulcher as a heads up!

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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually pretty blown away that this Just Worked(™️) within both a Logic file:

listeners: ({ actions }) => ({
initializeOverview: async () => {
try {
const response = await HttpLogic.values.http.get('/api/workplace_search/overview');
actions.setServerData(response);
} catch (e) {
handleAPIError(e);
}

and a React component:

const getEnginesData = async ({ type, pageIndex }: IGetEnginesParams) => {
try {
return await http.get('/api/app_search/engines', {
query: { type, pageIndex },
});
} catch (e) {
setIsLoading(false);
handleAPIError(e);
}
};

Kea is amazing! 💞


// 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;
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ export const HttpLogic = kea<MakeLogicType<IHttpValues, IHttpActions>>({
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);
Expand Down
Loading