Skip to content

Commit

Permalink
[vtadmin-web] Add client-side error handling interface + Bugsnag impl…
Browse files Browse the repository at this point in the history 
…ementation

Signed-off-by: Sara Bee <[email protected]>
  • Loading branch information
@doeg
doeg committed Jun 9, 2021
1 parent 50365bb commit 7257e15
Show file tree
Hide file tree
Showing 12 changed files with 629 additions and 77 deletions.
70 changes: 70 additions & 0 deletions web/vtadmin/package-lock.json
View file

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
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
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
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 {
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

0 comments on commit 7257e15

Please sign in to comment.