From b8d5799e75124239eadf485cb06d1521e0fb150b Mon Sep 17 00:00:00 2001 From: Ben Wheeler Date: Sat, 14 Dec 2019 22:44:26 -0500 Subject: [PATCH 1/5] refactor lib/session so that it can be used arbitrary number of times --- src/lib/session.js | 56 ++++++++++++++++++-- src/redux/session.js | 118 +++++++++++++++++-------------------------- 2 files changed, 96 insertions(+), 78 deletions(-) diff --git a/src/lib/session.js b/src/lib/session.js index 361eb01f9..dd16a5fe4 100644 --- a/src/lib/session.js +++ b/src/lib/session.js @@ -2,7 +2,45 @@ const api = require('./api'); module.exports = {}; -module.exports.requestSessionWithRetry = (count, resolve, reject) => { +/* +requestSessionWithRetry() + +Retries the session api call until it has either reached the limit of number of +retries, or received a successful response with that contains a 'user' field in +its body. + +Each time it retries, it will double its previous waiting time, and subtract +that time from the totalDelayMS parameter. + +example of what this might look like: + +1st call: +receives params: retriesLeft=3 and totalDelayMS=3500 +performs api call +delay until next call: 3500 / (2^3 - 1) = 500ms +next call params: retriesLeft=2, totalDelayMS=3000 + +2nd call: +receives params: retriesLeft=2 and totalDelayMS=3000 +performs api call +delay until next call: 3000 / (2^2 - 1) = 1000ms +next call: retriesLeft=1, totalDelayMS=2000 + +3rd call: +receives params: retriesLeft=1, totalDelayMS=2000 +performs api call +delay until next call: 2000 / (2^1 - 1) = 2000ms +next call: retriesLeft=0, totalDelayMS=0 + +4th call: +receives params: retriesLeft=0, totalDelayMS=0 +performs api call +returns the response, even if it is undefined or empty + +total api calls: 4 +total delay time: 3500ms +*/ +module.exports.requestSessionWithRetry = (resolve, reject, retriesLeft, totalDelayMS) => { api({ host: '', uri: '/session/' @@ -11,13 +49,21 @@ module.exports.requestSessionWithRetry = (count, resolve, reject) => { return reject(err); } if (typeof body === 'undefined' || !body.user) { - // Retry after 500ms, 1.5s, 3.5s, 7.5s and then stop. - if (count > 4) { + if (retriesLeft < 1) { return resolve(body); } - return setTimeout(module.exports.requestSessionWithRetry.bind(null, count + 1, resolve, reject), - 250 * Math.pow(2, count)); + const nextTimeout = totalDelayMS / (Math.pow(2, retriesLeft) - 1); + return setTimeout( + module.exports.requestSessionWithRetry.bind( + null, resolve, reject, retriesLeft - 1, totalDelayMS - nextTimeout + ), + nextTimeout + ); } return resolve(body); }); }; + +module.exports.requestSessionOnce = (resolve, reject) => ( + module.exports.requestSessionWithRetry(resolve, reject, 0, 0) +); diff --git a/src/redux/session.js b/src/redux/session.js index af209363e..68f0b9edc 100644 --- a/src/redux/session.js +++ b/src/redux/session.js @@ -62,87 +62,59 @@ module.exports.setStatus = status => ({ status: status }); +const handleSessionResponse = (dispatch, body) => { + if (typeof body === 'undefined') return dispatch(module.exports.setSessionError('No session content')); + if ( + body.user && + body.user.banned && + banWhitelistPaths.indexOf(window.location.pathname) === -1 + ) { + window.location = '/accounts/banned-response/'; + return; + } else if ( + body.flags && + body.flags.must_complete_registration && + window.location.pathname !== '/classes/complete_registration' + ) { + window.location = '/classes/complete_registration'; + return; + } else if ( + body.flags && + body.flags.must_reset_password && + !body.flags.must_complete_registration && + window.location.pathname !== '/classes/student_password_reset/' + ) { + window.location = '/classes/student_password_reset/'; + return; + } + dispatch(module.exports.setSession(body)); + dispatch(module.exports.setStatus(module.exports.Status.FETCHED)); + + // get the permissions from the updated session + dispatch(permissionsActions.storePermissions(body.permissions)); + if (typeof body.user !== 'undefined') { + dispatch(messageCountActions.getCount(body.user.username)); + } + return; +}; + module.exports.refreshSession = () => (dispatch => { dispatch(module.exports.setStatus(module.exports.Status.FETCHING)); - api({ - host: '', - uri: '/session/' - }, (err, body) => { - if (err) return dispatch(module.exports.setSessionError(err)); - if (typeof body === 'undefined') return dispatch(module.exports.setSessionError('No session content')); - if ( - body.user && - body.user.banned && - banWhitelistPaths.indexOf(window.location.pathname) === -1 - ) { - window.location = '/accounts/banned-response/'; - return; - } else if ( - body.flags && - body.flags.must_complete_registration && - window.location.pathname !== '/classes/complete_registration' - ) { - window.location = '/classes/complete_registration'; - return; - } else if ( - body.flags && - body.flags.must_reset_password && - !body.flags.must_complete_registration && - window.location.pathname !== '/classes/student_password_reset/' - ) { - window.location = '/classes/student_password_reset/'; - return; - } - dispatch(module.exports.setSession(body)); - dispatch(module.exports.setStatus(module.exports.Status.FETCHED)); - - // get the permissions from the updated session - dispatch(permissionsActions.storePermissions(body.permissions)); - if (typeof body.user !== 'undefined') { - dispatch(messageCountActions.getCount(body.user.username)); - } - return; - }); + return new Promise((resolve, reject) => ( + sessionLib.requestSessionOnce(resolve, reject).then(body => { + handleSessionResponse(dispatch, body); + }, err => { + dispatch(module.exports.setSessionError(err)); + }) + )); }); module.exports.refreshSessionWithRetry = () => (dispatch => { dispatch(module.exports.setStatus(module.exports.Status.FETCHING)); return new Promise((resolve, reject) => ( - sessionLib.requestSessionWithRetry(1, resolve, reject) + sessionLib.requestSessionWithRetry(4, resolve, reject) )).then(body => { - if (typeof body === 'undefined') return dispatch(module.exports.setSessionError('No session content')); - if ( - body.user && - body.user.banned && - banWhitelistPaths.indexOf(window.location.pathname) === -1 - ) { - window.location = '/accounts/banned-response/'; - return; - } else if ( - body.flags && - body.flags.must_complete_registration && - window.location.pathname !== '/classes/complete_registration' - ) { - window.location = '/classes/complete_registration'; - return; - } else if ( - body.flags && - body.flags.must_reset_password && - !body.flags.must_complete_registration && - window.location.pathname !== '/classes/student_password_reset/' - ) { - window.location = '/classes/student_password_reset/'; - return; - } - dispatch(module.exports.setSession(body)); - dispatch(module.exports.setStatus(module.exports.Status.FETCHED)); - - // get the permissions from the updated session - dispatch(permissionsActions.storePermissions(body.permissions)); - if (typeof body.user !== 'undefined') { - dispatch(messageCountActions.getCount(body.user.username)); - } - return; + handleSessionResponse(dispatch, body); }, err => { dispatch(module.exports.setSessionError(err)); }); From 0c71121540cc85ccd3fbf05f141c55a3ea698ab5 Mon Sep 17 00:00:00 2001 From: Ben Wheeler Date: Sat, 14 Dec 2019 22:44:38 -0500 Subject: [PATCH 2/5] session testing WIP --- test/unit/lib/session.test.js | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 test/unit/lib/session.test.js diff --git a/test/unit/lib/session.test.js b/test/unit/lib/session.test.js new file mode 100644 index 000000000..120365742 --- /dev/null +++ b/test/unit/lib/session.test.js @@ -0,0 +1,64 @@ +describe('session library', () => { +// let mockAPIRequest = {}; + let mockAPIRequestNoSession = jest.fn((opts, callback) => { + console.log(`mocked has been called ${mockAPIRequestNoSession.mock.calls.length} times`); + callback(null, {}, {statusCode: 200}); + }); + // let mockAPIRequestYesSession = jest.fn((opts, callback) => { + // console.log(`Yes Session mocked has been called ${mockAPIRequestNoSession.mock.calls.length} times`); + // callback(null, {}, {statusCode: 200}); + // }); + // let whichMockAPIRequest = mockAPIRequestNoSession; + // const mockCurrentAPIRequest = (() => { + // whichMockAPIRequest(); + // })(); + jest.mock('../../../src/lib/api', () => ( + mockAPIRequestNoSession + )); + const sessionLib = require('../../../src/lib/session'); // eslint-disable-line global-require + afterEach(() => { + jest.clearAllMocks(); + }); + + test('requestSessionOnce calls api 1 time', done => { + //whichMockAPIRequest = mockAPIRequestNoSession; + // mockAPIRequest = jest.fn((opts, callback) => { + // callback(null, {}, {statusCode: 200}); + // }); + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionOnce(resolve, reject); + }).then(() => { + expect(mockAPIRequestNoSession).toHaveBeenCalledTimes(1); + done(); + }, () => { + console.log('sessionLib test: err'); + }); + }); + + test('requestSessionWithRetry can call api multiple times', done => { + // mockAPIRequest = jest.fn((opts, callback) => { + // callback(null, {}, {statusCode: 200}); + // }); + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); + }).then(() => { + expect(mockAPIRequestNoSession).toHaveBeenCalledTimes(3); + done(); + }); + }); + + test('requestSessionWithRetry respects total delay time param within a reasonable tolerance', done => { + // mockAPIRequest = jest.fn((opts, callback) => { + // callback(null, {}, {statusCode: 200}); + // }); + const startTime = new Date().getTime(); + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionWithRetry(resolve, reject, 2, 2500); + }).then(() => { + const endTime = new Date().getTime(); + expect(endTime - startTime > 2000).toBeTruthy(); + expect(endTime - startTime < 3000).toBeTruthy(); + done(); + }); + }); +}); From 27a7324da0fb861efa4251784f9f257151764ad1 Mon Sep 17 00:00:00 2001 From: Ben Wheeler Date: Mon, 16 Dec 2019 09:34:51 -0500 Subject: [PATCH 3/5] treat null body in session response as unsuccessful, without dereferencing body --- src/lib/session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/session.js b/src/lib/session.js index dd16a5fe4..8094ab3f1 100644 --- a/src/lib/session.js +++ b/src/lib/session.js @@ -48,7 +48,7 @@ module.exports.requestSessionWithRetry = (resolve, reject, retriesLeft, totalDel if (err || (response && response.statusCode === 404)) { return reject(err); } - if (typeof body === 'undefined' || !body.user) { + if (typeof body === 'undefined' || body === null || !body.user) { if (retriesLeft < 1) { return resolve(body); } From 69e519286d03324b91a0fddcf0074f34db718e48 Mon Sep 17 00:00:00 2001 From: Ben Wheeler Date: Mon, 16 Dec 2019 09:35:11 -0500 Subject: [PATCH 4/5] session tests mock api response various ways --- src/redux/session.js | 1 - test/unit/lib/session.test.js | 129 ++++++++++++++++++++++++++-------- 2 files changed, 99 insertions(+), 31 deletions(-) diff --git a/src/redux/session.js b/src/redux/session.js index 68f0b9edc..c2dffa1da 100644 --- a/src/redux/session.js +++ b/src/redux/session.js @@ -1,7 +1,6 @@ const keyMirror = require('keymirror'); const defaults = require('lodash.defaults'); -const api = require('../lib/api'); const sessionLib = require('../lib/session'); const messageCountActions = require('./message-count.js'); const permissionsActions = require('./permissions.js'); diff --git a/test/unit/lib/session.test.js b/test/unit/lib/session.test.js index 120365742..3b1bbe7a2 100644 --- a/test/unit/lib/session.test.js +++ b/test/unit/lib/session.test.js @@ -1,56 +1,92 @@ describe('session library', () => { -// let mockAPIRequest = {}; - let mockAPIRequestNoSession = jest.fn((opts, callback) => { - console.log(`mocked has been called ${mockAPIRequestNoSession.mock.calls.length} times`); + // respond to session requests with empty session object + let sessionNoUser = jest.fn((opts, callback) => { callback(null, {}, {statusCode: 200}); }); - // let mockAPIRequestYesSession = jest.fn((opts, callback) => { - // console.log(`Yes Session mocked has been called ${mockAPIRequestNoSession.mock.calls.length} times`); - // callback(null, {}, {statusCode: 200}); - // }); - // let whichMockAPIRequest = mockAPIRequestNoSession; - // const mockCurrentAPIRequest = (() => { - // whichMockAPIRequest(); - // })(); - jest.mock('../../../src/lib/api', () => ( - mockAPIRequestNoSession - )); + // respond to session requests with session object that indicates + // successfully logged-in user + let sessionYesUser = jest.fn((opts, callback) => { + callback(null, {user: {username: 'test_username'}}, {statusCode: 200}); + }); + // respond to first two requests with empty session object; after that, + // respond with user in object + let sessionNoThenYes = jest.fn((opts, callback) => { + if (sessionNoThenYes.mock.calls.length <= 2) { + callback(null, {}, {statusCode: 200}); + } else { + callback(null, {user: {username: 'test_username'}}, {statusCode: 200}); + } + }); + // respond to session requests with response code 404, indicating no session + // found for that user + let sessionNotFound = jest.fn((opts, callback) => { + callback(null, null, {statusCode: 404}); + }); + // respond to session requests with response code 503, indicating connection failure + let sessionConnectFailure = jest.fn((opts, callback) => { + callback(null, null, {statusCode: 503}); + }); + + // by changing whichMockAPIRequest, we can simulate different api responses + let whichMockAPIRequest = null; + let mockAPIRequest = (opts, callback) => { + whichMockAPIRequest(opts, callback); + }; + + // mock lib/api.js, and include our mocked version in lib/session.js + jest.mock('../../../src/lib/api', () => { + return mockAPIRequest; + }); const sessionLib = require('../../../src/lib/session'); // eslint-disable-line global-require + afterEach(() => { jest.clearAllMocks(); }); - test('requestSessionOnce calls api 1 time', done => { - //whichMockAPIRequest = mockAPIRequestNoSession; - // mockAPIRequest = jest.fn((opts, callback) => { - // callback(null, {}, {statusCode: 200}); - // }); + test('requestSessionOnce can call api 1 time, when session found', done => { + whichMockAPIRequest = sessionYesUser; new Promise((resolve, reject) => { // eslint-disable-line no-undef sessionLib.requestSessionOnce(resolve, reject); - }).then(() => { - expect(mockAPIRequestNoSession).toHaveBeenCalledTimes(1); + }).then(body => { + expect(sessionYesUser).toHaveBeenCalledTimes(1); + expect(body).toEqual({user: {username: 'test_username'}}); + done(); + }); + }); + + test('requestSessionOnce can call api 1 time, when session not found', done => { + whichMockAPIRequest = sessionNoUser; + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionOnce(resolve, reject); + }).then(body => { + expect(sessionNoUser).toHaveBeenCalledTimes(1); + expect(body).toEqual({}); + done(); + }); + }); + + test('requestSessionWithRetry can call api once', done => { + whichMockAPIRequest = sessionNoUser; + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionWithRetry(resolve, reject, 0, 0); + }).then(() => { + expect(sessionNoUser).toHaveBeenCalledTimes(1); done(); - }, () => { - console.log('sessionLib test: err'); }); }); test('requestSessionWithRetry can call api multiple times', done => { - // mockAPIRequest = jest.fn((opts, callback) => { - // callback(null, {}, {statusCode: 200}); - // }); + whichMockAPIRequest = sessionNoUser; new Promise((resolve, reject) => { // eslint-disable-line no-undef sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); }).then(() => { - expect(mockAPIRequestNoSession).toHaveBeenCalledTimes(3); + expect(sessionNoUser).toHaveBeenCalledTimes(3); done(); }); }); test('requestSessionWithRetry respects total delay time param within a reasonable tolerance', done => { - // mockAPIRequest = jest.fn((opts, callback) => { - // callback(null, {}, {statusCode: 200}); - // }); + whichMockAPIRequest = sessionNoUser; const startTime = new Date().getTime(); new Promise((resolve, reject) => { // eslint-disable-line no-undef sessionLib.requestSessionWithRetry(resolve, reject, 2, 2500); @@ -61,4 +97,37 @@ describe('session library', () => { done(); }); }); + + test('requestSessionWithRetry will retry if no user found, then stop when user is found', done => { + whichMockAPIRequest = sessionNoThenYes; + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionWithRetry(resolve, reject, 4, 3000); + }).then(body => { + expect(body).toEqual({user: {username: 'test_username'}}); + expect(sessionNoThenYes).toHaveBeenCalledTimes(3); + done(); + }); + }); + + test('requestSessionWithRetry handles session not found as immediate error', done => { + whichMockAPIRequest = sessionNotFound; + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); + }).then(() => {}, err => { + expect(err).toBeFalsy(); + done(); + }); + }); + + test('requestSessionWithRetry handles connection failure by retrying', done => { + whichMockAPIRequest = sessionConnectFailure; + new Promise((resolve, reject) => { // eslint-disable-line no-undef + sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); + }).then(body => { + expect(sessionConnectFailure).toHaveBeenCalledTimes(3); + expect(body).toBeFalsy(); + done(); + }); + }); + }); From d53fe4324176c28b245a0598f237250f3e944a84 Mon Sep 17 00:00:00 2001 From: Ben Wheeler Date: Mon, 16 Dec 2019 17:53:30 -0500 Subject: [PATCH 5/5] fix bug, updating session request retry code per rschamp's feedback --- src/lib/session.js | 2 +- src/redux/session.js | 6 +++--- test/unit/lib/session.test.js | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lib/session.js b/src/lib/session.js index 8094ab3f1..22490a010 100644 --- a/src/lib/session.js +++ b/src/lib/session.js @@ -64,6 +64,6 @@ module.exports.requestSessionWithRetry = (resolve, reject, retriesLeft, totalDel }); }; -module.exports.requestSessionOnce = (resolve, reject) => ( +module.exports.requestSession = (resolve, reject) => ( module.exports.requestSessionWithRetry(resolve, reject, 0, 0) ); diff --git a/src/redux/session.js b/src/redux/session.js index c2dffa1da..a0d951a07 100644 --- a/src/redux/session.js +++ b/src/redux/session.js @@ -1,7 +1,7 @@ const keyMirror = require('keymirror'); const defaults = require('lodash.defaults'); -const sessionLib = require('../lib/session'); +const {requestSession, requestSessionWithRetry} = require('../lib/session'); const messageCountActions = require('./message-count.js'); const permissionsActions = require('./permissions.js'); @@ -100,7 +100,7 @@ const handleSessionResponse = (dispatch, body) => { module.exports.refreshSession = () => (dispatch => { dispatch(module.exports.setStatus(module.exports.Status.FETCHING)); return new Promise((resolve, reject) => ( - sessionLib.requestSessionOnce(resolve, reject).then(body => { + requestSession(resolve, reject).then(body => { handleSessionResponse(dispatch, body); }, err => { dispatch(module.exports.setSessionError(err)); @@ -111,7 +111,7 @@ module.exports.refreshSession = () => (dispatch => { module.exports.refreshSessionWithRetry = () => (dispatch => { dispatch(module.exports.setStatus(module.exports.Status.FETCHING)); return new Promise((resolve, reject) => ( - sessionLib.requestSessionWithRetry(4, resolve, reject) + requestSessionWithRetry(resolve, reject, 4, 7500) )).then(body => { handleSessionResponse(dispatch, body); }, err => { diff --git a/test/unit/lib/session.test.js b/test/unit/lib/session.test.js index 3b1bbe7a2..324cf6f1f 100644 --- a/test/unit/lib/session.test.js +++ b/test/unit/lib/session.test.js @@ -43,10 +43,10 @@ describe('session library', () => { jest.clearAllMocks(); }); - test('requestSessionOnce can call api 1 time, when session found', done => { + test('requestSession can call api 1 time, when session found', done => { whichMockAPIRequest = sessionYesUser; new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionOnce(resolve, reject); + sessionLib.requestSession(resolve, reject); }).then(body => { expect(sessionYesUser).toHaveBeenCalledTimes(1); expect(body).toEqual({user: {username: 'test_username'}}); @@ -54,10 +54,10 @@ describe('session library', () => { }); }); - test('requestSessionOnce can call api 1 time, when session not found', done => { + test('requestSession can call api 1 time, when session not found', done => { whichMockAPIRequest = sessionNoUser; new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionOnce(resolve, reject); + sessionLib.requestSession(resolve, reject); }).then(body => { expect(sessionNoUser).toHaveBeenCalledTimes(1); expect(body).toEqual({});