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.

Sign up for GitHub

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

[vtadmin-web] Add client-side error handling interface + Bugsnag implementation #8287

Merged
merged 1 commit into from
Jun 10, 2021
Merged

[vtadmin-web] Add client-side error handling interface + Bugsnag implementation #8287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions web/vtadmin/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions web/vtadmin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"npm": ">=6.14.9"
},
"dependencies": {
"@bugsnag/js": "^7.10.1",
"@testing-library/user-event": "^12.6.0",
"@types/classnames": "^2.2.11",
"@types/jest": "^26.0.19",
Expand Down
62 changes: 47 additions & 15 deletions web/vtadmin/src/api/http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ import { rest } from 'msw';
import { setupServer } from 'msw/node';

import * as api from './http';
import { vtadmin as pb } from '../proto/vtadmin';
import { HTTP_RESPONSE_NOT_OK_ERROR, MALFORMED_HTTP_RESPONSE_ERROR } from './http';
import { HTTP_RESPONSE_NOT_OK_ERROR, MALFORMED_HTTP_RESPONSE_ERROR } from '../errors/errorTypes';
import * as errorHandler from '../errors/errorHandler';

jest.mock('../errors/errorHandler');

// This test suite uses Mock Service Workers (https://github.com/mswjs/msw)
// to mock HTTP responses from vtadmin-api.
Expand Down Expand Up @@ -56,15 +58,17 @@ const TEST_PROCESS_ENV = {
};

beforeAll(() => {
process.env = { ...TEST_PROCESS_ENV };
// TypeScript can get a little cranky with the automatic
// string/boolean type conversions, hence this cast.
process.env = { ...TEST_PROCESS_ENV } as NodeJS.ProcessEnv;

// Enable API mocking before tests.
server.listen();
});

afterEach(() => {
// Reset the process.env to clear out any changes made in the tests.
process.env = { ...TEST_PROCESS_ENV };
process.env = { ...TEST_PROCESS_ENV } as NodeJS.ProcessEnv;

jest.restoreAllMocks();

Expand Down Expand Up @@ -92,34 +96,52 @@ describe('api/http', () => {

it('throws an error if response.ok is false', async () => {
const endpoint = `/api/tablets`;
const response = { ok: false };
mockServerJson(endpoint, response);
const response = {
ok: false,
error: {
code: 'oh_no',
message: 'something went wrong',
},
};

expect.assertions(3);
// See https://mswjs.io/docs/recipes/mocking-error-responses
server.use(rest.get(endpoint, (req, res, ctx) => res(ctx.status(500), ctx.json(response))));

expect.assertions(5);

try {
await api.fetchTablets();
} catch (e) {
/* eslint-disable jest/no-conditional-expect */
expect(e.name).toEqual(HTTP_RESPONSE_NOT_OK_ERROR);
expect(e.message).toEqual(endpoint);
expect(e.message).toEqual('[status 500] /api/tablets: oh_no something went wrong');
expect(e.response).toEqual(response);

expect(errorHandler.notify).toHaveBeenCalledTimes(1);
expect(errorHandler.notify).toHaveBeenCalledWith(e);
/* eslint-enable jest/no-conditional-expect */
}
});

it('throws an error on malformed JSON', async () => {
const endpoint = `/api/tablets`;
server.use(rest.get(endpoint, (req, res, ctx) => res(ctx.body('this is fine'))));
server.use(
rest.get(endpoint, (req, res, ctx) =>
res(ctx.status(504), ctx.body('<html><head><title>504 Gateway Time-out</title></head></html>'))
)
);

expect.assertions(2);
expect.assertions(4);

try {
await api.vtfetch(endpoint);
} catch (e) {
/* eslint-disable jest/no-conditional-expect */
expect(e.name).toEqual('SyntaxError');
expect(e.message.startsWith('Unexpected token')).toBeTruthy();
expect(e.name).toEqual(MALFORMED_HTTP_RESPONSE_ERROR);
expect(e.message).toEqual('[status 504] /api/tablets: Unexpected token < in JSON at position 0');

expect(errorHandler.notify).toHaveBeenCalledTimes(1);
expect(errorHandler.notify).toHaveBeenCalledWith(e);
/* eslint-enable jest/no-conditional-expect */
}
});
Expand Down Expand Up @@ -151,7 +173,9 @@ describe('api/http', () => {

await api.vtfetch(endpoint);
expect(global.fetch).toHaveBeenCalledTimes(1);
expect(global.fetch).toHaveBeenCalledWith(endpoint, { credentials: 'include' });
expect(global.fetch).toHaveBeenCalledWith(endpoint, {
credentials: 'include',
});

jest.restoreAllMocks();
});
Expand All @@ -165,7 +189,9 @@ describe('api/http', () => {

await api.vtfetch(endpoint);
expect(global.fetch).toHaveBeenCalledTimes(1);
expect(global.fetch).toHaveBeenCalledWith(endpoint, { credentials: undefined });
expect(global.fetch).toHaveBeenCalledWith(endpoint, {
credentials: undefined,
});

jest.restoreAllMocks();
});
Expand All @@ -187,6 +213,9 @@ describe('api/http', () => {
'Invalid fetch credentials property: nope. Must be undefined or one of omit, same-origin, include'
);
expect(global.fetch).toHaveBeenCalledTimes(0);

expect(errorHandler.notify).toHaveBeenCalledTimes(1);
expect(errorHandler.notify).toHaveBeenCalledWith(e);
/* eslint-enable jest/no-conditional-expect */
}

Expand All @@ -200,7 +229,7 @@ describe('api/http', () => {
const endpoint = '/api/foos';
mockServerJson(endpoint, { ok: true, result: { foos: null } });

expect.assertions(1);
expect.assertions(3);

try {
await api.vtfetchEntities({
Expand All @@ -211,6 +240,9 @@ describe('api/http', () => {
} catch (e) {
/* eslint-disable jest/no-conditional-expect */
expect(e.message).toMatch('expected entities to be an array, got null');

expect(errorHandler.notify).toHaveBeenCalledTimes(1);
expect(errorHandler.notify).toHaveBeenCalledWith(e);
/* eslint-enable jest/no-conditional-expect */
}
});
Expand Down
125 changes: 64 additions & 61 deletions web/vtadmin/src/api/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,68 +15,66 @@
*/

import { vtadmin as pb } from '../proto/vtadmin';
import * as errorHandler from '../errors/errorHandler';
import { HttpFetchError, HttpResponseNotOkError, MalformedHttpResponseError } from '../errors/errorTypes';
import { HttpOkResponse } from './responseTypes';

interface HttpOkResponse {
ok: true;
result: any;
}

interface HttpErrorResponse {
ok: false;
}

export const MALFORMED_HTTP_RESPONSE_ERROR = 'MalformedHttpResponseError';

// MalformedHttpResponseError is thrown when the JSON response envelope
// is an unexpected shape.
class MalformedHttpResponseError extends Error {
responseJson: object;

constructor(message: string, responseJson: object) {
super(message);
this.name = MALFORMED_HTTP_RESPONSE_ERROR;
this.responseJson = responseJson;
}
}

export const HTTP_RESPONSE_NOT_OK_ERROR = 'HttpResponseNotOkError';

// HttpResponseNotOkError is throw when the `ok` is false in
// the JSON response envelope.
class HttpResponseNotOkError extends Error {
response: HttpErrorResponse | null;

constructor(endpoint: string, response: HttpErrorResponse) {
super(endpoint);
this.name = HTTP_RESPONSE_NOT_OK_ERROR;
this.response = response;
}
}

// vtfetch makes HTTP requests against the given vtadmin-api endpoint
// and returns the parsed response.
//
// HttpResponse envelope types are not defined in vtadmin.proto (nor should they be)
// thus we have to validate the shape of the API response with more care.
//
// Note that this only validates the HttpResponse envelope; it does not
// do any type checking or validation on the result.
/**
* vtfetch makes HTTP requests against the given vtadmin-api endpoint
* and returns the parsed response.
*
* HttpResponse envelope types are not defined in vtadmin.proto (nor should they be)
* thus we have to validate the shape of the API response with more care.
*
* Note that this only validates the HttpResponse envelope; it does not
* do any type checking or validation on the result.
*/
export const vtfetch = async (endpoint: string): Promise<HttpOkResponse> => {
const { REACT_APP_VTADMIN_API_ADDRESS } = process.env;

const url = `${REACT_APP_VTADMIN_API_ADDRESS}${endpoint}`;
const opts = vtfetchOpts();

const response = await global.fetch(url, opts);

const json = await response.json();
if (!('ok' in json)) throw new MalformedHttpResponseError('invalid http envelope', json);

// Throw "not ok" responses so that react-query correctly interprets them as errors.
// See https://react-query.tanstack.com/guides/query-functions#handling-and-throwing-errors
if (!json.ok) throw new HttpResponseNotOkError(endpoint, json);

return json as HttpOkResponse;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this diff looks much worse than it is, since I had to surround everything in a try/catch. Logic changes here are minimal and well-tested. 🙏

const { REACT_APP_VTADMIN_API_ADDRESS } = process.env;

const url = `${REACT_APP_VTADMIN_API_ADDRESS}${endpoint}`;
const opts = vtfetchOpts();

let response = null;
try {
response = await global.fetch(url, opts);
} catch (error) {
// Capture fetch() promise rejections and rethrow as HttpFetchError.
// fetch() promises will reject with a TypeError when a network error is
// encountered or CORS is misconfigured, in which case the request never
// makes it to the server.
// See https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#checking_that_the_fetch_was_successful
throw new HttpFetchError(url);
}

let json = null;
try {
json = await response.json();
} catch (error) {
throw new MalformedHttpResponseError(error.message, endpoint, json, response);
}

if (!('ok' in json)) {
throw new MalformedHttpResponseError('invalid HTTP envelope', endpoint, json, response);
}

if (!json.ok) {
throw new HttpResponseNotOkError(endpoint, json, response);
}

return json as HttpOkResponse;
} catch (error) {
// Most commonly, react-query is the downstream consumer of
// errors thrown in vtfetch. Because react-query "handles" errors
// by propagating them to components (as it should!), any errors thrown
// from vtfetch are _not_ automatically logged as "unhandled errors".
// Instead, we catch errors and manually notify our error handling serivce(s),
// and then rethrow the error for react-query to propagate the usual way.
// See https://react-query.tanstack.com/guides/query-functions#handling-and-throwing-errors
errorHandler.notify(error);
throw error;
}
};

export const vtfetchOpts = (): RequestInit => {
Expand Down Expand Up @@ -106,7 +104,12 @@ export const vtfetchEntities = async <T>(opts: {

const entities = opts.extract(res);
if (!Array.isArray(entities)) {
throw Error(`expected entities to be an array, got ${entities}`);
// Since react-query is the downstream consumer of vtfetch + vtfetchEntities,
// errors thrown in either function will be "handled" and will not automatically
// propagate as "unhandled" errors, meaning we have to log them manually.
const error = Error(`expected entities to be an array, got ${entities}`);
errorHandler.notify(error);
throw error;
}

return entities.map(opts.transform);
Expand Down
Loading