From 8127041acb66067f1e0f1e545718269e0d98a7ac Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Sun, 26 Feb 2017 13:40:07 +0200 Subject: [PATCH] #303: properly handling json and request errors. Added InternalServerError --- src/components/accounts/actions.js | 3 +- src/components/ui/bsod/BsodMiddleware.js | 11 ++-- .../middlewares/refreshTokenMiddleware.js | 3 +- src/services/request/InternalServerError.js | 27 +++++++++ src/services/request/index.js | 3 + src/services/request/request.js | 24 ++++++-- tests/components/accounts/actions.test.js | 5 +- .../refreshTokenMiddleware.test.js | 7 +-- tests/services/request/request.test.js | 55 +++++++++++++++++++ 9 files changed, 120 insertions(+), 18 deletions(-) create mode 100644 src/services/request/InternalServerError.js create mode 100644 tests/services/request/request.test.js diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index c2d065e..2ec6b5d 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -5,6 +5,7 @@ import { updateUser, setGuest } from 'components/user/actions'; import { setLocale } from 'components/i18n/actions'; import { setAccountSwitcher } from 'components/auth/actions'; import logger from 'services/logger'; +import { InternalServerError } from 'services/request'; import { add, @@ -36,7 +37,7 @@ export function authenticate({token, refreshToken}) { return (dispatch, getState) => authentication.validateToken({token, refreshToken}) .catch((resp = {}) => { - if (resp.originalResponse && resp.originalResponse.status >= 500) { + if (resp instanceof InternalServerError) { // delegate error recovering to the later logic return Promise.reject(resp); } diff --git a/src/components/ui/bsod/BsodMiddleware.js b/src/components/ui/bsod/BsodMiddleware.js index 84c827c..a38f7f7 100644 --- a/src/components/ui/bsod/BsodMiddleware.js +++ b/src/components/ui/bsod/BsodMiddleware.js @@ -1,9 +1,12 @@ +import { InternalServerError } from 'services/request'; + export default function BsodMiddleware(dispatchBsod, logger) { return { - catch(resp) { - if (resp - && resp.originalResponse - && /404|5\d\d/.test(resp.originalResponse.status) + catch(resp = {}) { + if (resp instanceof InternalServerError + || (resp.originalResponse + && /404|5\d\d/.test(resp.originalResponse.status) + ) ) { dispatchBsod(); diff --git a/src/components/user/middlewares/refreshTokenMiddleware.js b/src/components/user/middlewares/refreshTokenMiddleware.js index bfc6427..b630b73 100644 --- a/src/components/user/middlewares/refreshTokenMiddleware.js +++ b/src/components/user/middlewares/refreshTokenMiddleware.js @@ -1,6 +1,7 @@ import { getJwtPayload } from 'functions'; import authentication from 'services/api/authentication'; import logger from 'services/logger'; +import { InternalServerError } from 'services/request'; import { updateToken, logoutAll } from 'components/accounts/actions'; /** @@ -72,7 +73,7 @@ function requestAccessToken(refreshToken, dispatch) { return authentication.requestToken(refreshToken) .then(({token}) => dispatch(updateToken(token))) .catch((resp = {}) => { - if (resp.originalResponse && resp.originalResponse.status >= 500) { + if (resp instanceof InternalServerError) { return Promise.reject(resp); } diff --git a/src/services/request/InternalServerError.js b/src/services/request/InternalServerError.js new file mode 100644 index 0000000..c8ec86a --- /dev/null +++ b/src/services/request/InternalServerError.js @@ -0,0 +1,27 @@ +function InternalServerError(error, resp) { + error = error || {}; + + this.name = 'InternalServerError'; + this.message = 'InternalServerError'; + this.stack = (new Error()).stack; + + if (resp) { + this.originalResponse = resp; + } + + if (error.message) { + this.message = error.message; + } + + if (typeof error === 'string') { + this.message = error; + } else { + this.error = error; + Object.assign(this, error); + } +} +InternalServerError.prototype = Object.create(Error.prototype); +InternalServerError.prototype.constructor = InternalServerError; + + +export default InternalServerError; diff --git a/src/services/request/index.js b/src/services/request/index.js index 174888e..7d9c662 100644 --- a/src/services/request/index.js +++ b/src/services/request/index.js @@ -1,3 +1,6 @@ import request from './request'; +import InternalServerError from './InternalServerError'; export default request; + +export { InternalServerError }; diff --git a/src/services/request/request.js b/src/services/request/request.js index 7ea6640..355cc40 100644 --- a/src/services/request/request.js +++ b/src/services/request/request.js @@ -1,4 +1,5 @@ import PromiseMiddlewareLayer from './PromiseMiddlewareLayer'; +import InternalServerError from './InternalServerError'; const middlewareLayer = new PromiseMiddlewareLayer(); @@ -65,12 +66,27 @@ export default { const checkStatus = (resp) => Promise[resp.status >= 200 && resp.status < 300 ? 'resolve' : 'reject'](resp); -const toJSON = (resp) => resp.json().then((json) => { - json.originalResponse = resp; +const toJSON = (resp = {}) => { + if (!resp.json) { + // e.g. 'TypeError: Failed to fetch' due to CORS + throw new InternalServerError(resp); + } - return json; + return resp.json().then((json) => { + json.originalResponse = resp; + + return json; + }, (error) => Promise.reject( + new InternalServerError(error, resp) + )); +}; +const rejectWithJSON = (resp) => toJSON(resp).then((resp) => { + if (resp.originalResponse.status >= 500) { + throw new InternalServerError(resp, resp.originalResponse); + } + + throw resp; }); -const rejectWithJSON = (resp) => toJSON(resp).then((resp) => {throw resp;}); const handleResponseSuccess = (resp) => Promise[resp.success || typeof resp.success === 'undefined' ? 'resolve' : 'reject'](resp); function doFetch(url, options = {}) { diff --git a/tests/components/accounts/actions.test.js b/tests/components/accounts/actions.test.js index 6f8d274..6877cde 100644 --- a/tests/components/accounts/actions.test.js +++ b/tests/components/accounts/actions.test.js @@ -4,6 +4,7 @@ import sinon from 'sinon'; import { routeActions } from 'react-router-redux'; import logger from 'services/logger'; +import { InternalServerError } from 'services/request'; import authentication from 'services/api/authentication'; import { authenticate, @@ -133,9 +134,7 @@ describe('components/accounts/actions', () => { }); it('rejects when 5xx without logouting', () => { - const resp = { - originalResponse: {status: 500} - }; + const resp = new InternalServerError(null, {status: 500}); authentication.validateToken.returns(Promise.reject(resp)); diff --git a/tests/components/user/middlewares/refreshTokenMiddleware.test.js b/tests/components/user/middlewares/refreshTokenMiddleware.test.js index 2b0cecc..980d28d 100644 --- a/tests/components/user/middlewares/refreshTokenMiddleware.test.js +++ b/tests/components/user/middlewares/refreshTokenMiddleware.test.js @@ -4,6 +4,7 @@ import sinon from 'sinon'; import refreshTokenMiddleware from 'components/user/middlewares/refreshTokenMiddleware'; import authentication from 'services/api/authentication'; +import { InternalServerError } from 'services/request'; import { updateToken } from 'components/accounts/actions'; const refreshToken = 'foo'; @@ -145,11 +146,7 @@ describe('refreshTokenMiddleware', () => { }); it('should not logout if request failed with 5xx', () => { - const resp = { - originalResponse: { - status: 500 - } - }; + const resp = new InternalServerError(null, {status: 500}); authentication.requestToken.returns(Promise.reject(resp)); diff --git a/tests/services/request/request.test.js b/tests/services/request/request.test.js new file mode 100644 index 0000000..1a65172 --- /dev/null +++ b/tests/services/request/request.test.js @@ -0,0 +1,55 @@ +import expect from 'unexpected'; +import sinon from 'sinon'; + +import request from 'services/request'; +import { InternalServerError } from 'services/request'; + +describe('services/request', () => { + beforeEach(() => { + sinon.stub(window, 'fetch').named('fetch'); + }); + + afterEach(() => { + window.fetch.restore(); + }); + + describe('InternalServerError', () => { + it('should wrap fetch error', () => { + const resp = new TypeError('Fetch error'); + + fetch.returns(Promise.reject(resp)); + + return expect(request.get('/foo'), 'to be rejected') + .then((error) => { + expect(error, 'to be an', InternalServerError); + expect(error.originalResponse, 'to be undefined'); + expect(error.message, 'to equal', resp.message); + }); + }); + + it('should wrap json errors', () => { + const resp = new Response('bad resp format', {status: 200}); + + fetch.returns(Promise.resolve(resp)); + + return expect(request.get('/foo'), 'to be rejected') + .then((error) => { + expect(error, 'to be an', InternalServerError); + expect(error.originalResponse, 'to be', resp); + expect(error.message, 'to contain', 'JSON'); + }); + }); + + it('should wrap 5xx errors', () => { + const resp = new Response('{}', {status: 500}); + + fetch.returns(Promise.resolve(resp)); + + return expect(request.get('/foo'), 'to be rejected') + .then((error) => { + expect(error, 'to be an', InternalServerError); + expect(error.originalResponse, 'to be', resp); + }); + }); + }); +});