From 538bcab9fb9bced71227ba98306b8e9c863fdc7f Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 23 Mar 2022 09:49:50 -1000 Subject: [PATCH 1/4] Move Network into folder so we can organize things better Fix tests. Move enhanceParameters to its own method add docs fix bad comment clean up some logic that does not quite make sense Add more logging Only treat "Failed to fetch" errors as retryable for now. Curious to see what other kinds of errors we get. Throw any fetch errors when the response is not ok Make tests pass. Stop retrying non 200 jsonCode. fix up tests Add a few more tests Fix tests --- src/CONST.js | 7 +- src/libs/API.js | 112 +++----------- src/libs/HttpUtils.js | 8 +- src/libs/Network/NetworkStore.js | 85 +++++++++++ src/libs/Network/enhanceParameters.js | 52 +++++++ src/libs/{Network.js => Network/index.js} | 131 +++++++++-------- src/libs/actions/Session/index.js | 8 +- tests/actions/ReimbursementAccountTest.js | 3 +- tests/actions/SessionTest.js | 5 +- tests/unit/NetworkTest.js | 169 ++++++++++++++++------ tests/unit/enhanceParametersTest.js | 44 ++++++ 11 files changed, 418 insertions(+), 206 deletions(-) create mode 100644 src/libs/Network/NetworkStore.js create mode 100644 src/libs/Network/enhanceParameters.js rename src/libs/{Network.js => Network/index.js} (71%) create mode 100644 tests/unit/enhanceParametersTest.js diff --git a/src/CONST.js b/src/CONST.js index 77491a18d469..9b186163611f 100755 --- a/src/CONST.js +++ b/src/CONST.js @@ -299,6 +299,8 @@ const CONST = { API_OFFLINE: 'session.offlineMessageRetry', UNKNOWN_ERROR: 'Unknown error', REQUEST_CANCELLED: 'AbortError', + FAILED_TO_FETCH: 'Failed to fetch', + ENSURE_BUGBOT: 'ENSURE_BUGBOT', }, NETWORK: { METHOD: { @@ -308,10 +310,9 @@ const CONST = { PROCESS_REQUEST_DELAY_MS: 1000, MAX_PENDING_TIME_MS: 10 * 1000, }, - HTTP_STATUS_CODE: { + JSON_CODE: { SUCCESS: 200, - BAD_REQUEST: 400, - UNAUTHORIZED: 401, + NOT_AUTHENTICATED: 407, }, NVP: { IS_FIRST_TIME_NEW_EXPENSIFY_USER: 'isFirstTimeNewExpensifyUser', diff --git a/src/libs/API.js b/src/libs/API.js index 948f2a64b2e5..21f440c97cd3 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -1,9 +1,7 @@ import _ from 'underscore'; import lodashGet from 'lodash/get'; -import Onyx from 'react-native-onyx'; import CONST from '../CONST'; import CONFIG from '../CONFIG'; -import ONYXKEYS from '../ONYXKEYS'; import getPlaidLinkTokenParameters from './getPlaidLinkTokenParameters'; import redirectToSignIn from './actions/SignInRedirect'; import isViaExpensifyCashNative from './isViaExpensifyCashNative'; @@ -12,87 +10,10 @@ import Log from './Log'; import * as Network from './Network'; import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens'; import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError'; +import * as NetworkStore from './Network/NetworkStore'; +import enhanceParameters from './Network/enhanceParameters'; let isAuthenticating; -let credentials; -let authToken; -let currentUserEmail; - -function checkRequiredDataAndSetNetworkReady() { - if (_.isUndefined(authToken) || _.isUndefined(credentials)) { - return; - } - - Network.setIsReady(true); -} - -Onyx.connect({ - key: ONYXKEYS.CREDENTIALS, - callback: (val) => { - credentials = val || null; - checkRequiredDataAndSetNetworkReady(); - }, -}); - -Onyx.connect({ - key: ONYXKEYS.SESSION, - callback: (val) => { - authToken = lodashGet(val, 'authToken', null); - currentUserEmail = lodashGet(val, 'email', null); - checkRequiredDataAndSetNetworkReady(); - }, -}); - -/** - * Does this command require an authToken? - * - * @param {String} command - * @return {Boolean} - */ -function isAuthTokenRequired(command) { - return !_.contains([ - 'Log', - 'Graphite_Timer', - 'Authenticate', - 'GetAccountStatus', - 'SetPassword', - 'User_SignUp', - 'ResendValidateCode', - 'ResetPassword', - 'User_ReopenAccount', - 'ValidateEmail', - ], command); -} - -/** - * Adds default values to our request data - * - * @param {String} command - * @param {Object} parameters - * @returns {Object} - */ -function addDefaultValuesToParameters(command, parameters) { - const finalParameters = {...parameters}; - - if (isAuthTokenRequired(command) && !parameters.authToken) { - finalParameters.authToken = authToken; - } - - finalParameters.referer = CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER; - - // This application does not save its authToken in cookies like the classic Expensify app. - // Setting api_setCookie to false will ensure that the Expensify API doesn't set any cookies - // and prevents interfering with the cookie authToken that Expensify classic uses. - finalParameters.api_setCookie = false; - - // Unless email is already set include current user's email in every request and the server logs - finalParameters.email = lodashGet(parameters, 'email', currentUserEmail); - - return finalParameters; -} - -// Tie into the network layer to add auth token to the parameters of all requests -Network.registerParameterEnhancer(addDefaultValuesToParameters); /** * Function used to handle expired auth tokens. It re-authenticates with the API and @@ -118,7 +39,7 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp return reauthenticate(originalCommand) .then(() => { // Now that the API is authenticated, make the original request again with the new authToken - const params = addDefaultValuesToParameters(originalCommand, originalParameters); + const params = enhanceParameters(originalCommand, originalParameters); return Network.post(originalCommand, params, originalType); }) .catch(() => ( @@ -159,8 +80,10 @@ Network.registerResponseHandler((queuedRequest, response) => { }); } - if (response.jsonCode === 407) { - // Credentials haven't been initialized. We will not be able to re-authenticates with the API + if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { + const credentials = NetworkStore.getCredentials(); + + // Credentials haven't been initialized. We will not be able to re-authenticate with the API const unableToReauthenticate = (!credentials || !credentials.autoGeneratedLogin || !credentials.autoGeneratedPassword); @@ -169,13 +92,19 @@ Network.registerResponseHandler((queuedRequest, response) => { // of the new response created by handleExpiredAuthToken. const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); if (!shouldRetry || unableToReauthenticate) { - queuedRequest.resolve(response); + if (queuedRequest.resolve) { + queuedRequest.resolve(response); + } return; } handleExpiredAuthToken(queuedRequest.command, queuedRequest.data, queuedRequest.type) - .then(queuedRequest.resolve) - .catch(queuedRequest.reject); + .then(queuedRequest.resolve || (() => Promise.resolve())) + .catch(queuedRequest.reject || (() => Promise.resolve())); + return; + } + + if (!queuedRequest.resolve) { return; } @@ -197,8 +126,10 @@ Network.registerErrorHandler((queuedRequest, error) => { // Set an error state and signify we are done loading setSessionLoadingAndError(false, 'Cannot connect to server'); - // Reject the queued request with an API offline error so that the original caller can handle it. - queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); + // Reject the queued request with an API offline error so that the original caller can handle it + if (queuedRequest.reject) { + queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); + } }); /** @@ -280,6 +211,7 @@ function Authenticate(parameters) { * @returns {Promise} */ function reauthenticate(command = '') { + const credentials = NetworkStore.getCredentials(); return Authenticate({ useExpensifyLogin: false, partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, @@ -297,7 +229,7 @@ function reauthenticate(command = '') { // Update authToken in Onyx and in our local variables so that API requests will use the // new authToken updateSessionAuthTokens(response.authToken, response.encryptedAuthToken); - authToken = response.authToken; + NetworkStore.setAuthToken(response.authToken); // The authentication process is finished so the network can be unpaused to continue // processing requests diff --git a/src/libs/HttpUtils.js b/src/libs/HttpUtils.js index b58ff52e4dcd..f377ad4ae95b 100644 --- a/src/libs/HttpUtils.js +++ b/src/libs/HttpUtils.js @@ -30,7 +30,13 @@ function processHTTPRequest(url, method = 'get', body = null, canCancel = true) method, body, }) - .then(response => response.json()); + .then((response) => { + if (!response.ok) { + throw Error(response.statusText); + } + + return response.json(); + }); } /** diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js new file mode 100644 index 000000000000..3d7880b942e8 --- /dev/null +++ b/src/libs/Network/NetworkStore.js @@ -0,0 +1,85 @@ +import lodashGet from 'lodash/get'; +import Onyx from 'react-native-onyx'; +import _ from 'underscore'; +import ONYXKEYS from '../../ONYXKEYS'; + +let credentials; +let authToken; +let currentUserEmail; +let networkReady = false; + +/** + * @param {Boolean} ready + */ +function setIsReady(ready) { + networkReady = ready; +} + +function checkRequiredDataAndSetNetworkReady() { + if (_.isUndefined(authToken) || _.isUndefined(credentials)) { + return; + } + + setIsReady(true); +} + +Onyx.connect({ + key: ONYXKEYS.SESSION, + callback: (val) => { + authToken = lodashGet(val, 'authToken', null); + currentUserEmail = lodashGet(val, 'email', null); + checkRequiredDataAndSetNetworkReady(); + }, +}); + +Onyx.connect({ + key: ONYXKEYS.CREDENTIALS, + callback: (val) => { + credentials = val || null; + checkRequiredDataAndSetNetworkReady(); + }, +}); + +/** + * @returns {String} + */ +function getAuthToken() { + return authToken; +} + +/** + * @param {String} newAuthToken + */ +function setAuthToken(newAuthToken) { + authToken = newAuthToken; +} + +/** + * @returns {Object} + */ +function getCredentials() { + return credentials; +} + +/** + * @returns {String} + */ +function getCurrentUserEmail() { + return currentUserEmail; +} + +/** + * @returns {Boolean} + */ +function isReady() { + return networkReady; +} + +export { + getAuthToken, + setAuthToken, + getCredentials, + getCurrentUserEmail, + isReady, + setIsReady, +}; diff --git a/src/libs/Network/enhanceParameters.js b/src/libs/Network/enhanceParameters.js new file mode 100644 index 000000000000..4bf55e77b51c --- /dev/null +++ b/src/libs/Network/enhanceParameters.js @@ -0,0 +1,52 @@ +import lodashGet from 'lodash/get'; +import _ from 'underscore'; +import CONFIG from '../../CONFIG'; +import * as NetworkStore from './NetworkStore'; + +/** + * Does this command require an authToken? + * + * @param {String} command + * @return {Boolean} + */ +function isAuthTokenRequired(command) { + return !_.contains([ + 'Log', + 'Graphite_Timer', + 'Authenticate', + 'GetAccountStatus', + 'SetPassword', + 'User_SignUp', + 'ResendValidateCode', + 'ResetPassword', + 'User_ReopenAccount', + 'ValidateEmail', + ], command); +} + +/** + * Adds default values to our request data + * + * @param {String} command + * @param {Object} parameters + * @returns {Object} + */ +export default function enhanceParameters(command, parameters) { + const finalParameters = {...parameters}; + + if (isAuthTokenRequired(command) && !parameters.authToken) { + finalParameters.authToken = NetworkStore.getAuthToken(); + } + + finalParameters.referer = CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER; + + // This application does not save its authToken in cookies like the classic Expensify app. + // Setting api_setCookie to false will ensure that the Expensify API doesn't set any cookies + // and prevents interfering with the cookie authToken that Expensify classic uses. + finalParameters.api_setCookie = false; + + // Include current user's email in every request and the server logs + finalParameters.email = lodashGet(parameters, 'email', NetworkStore.getCurrentUserEmail()); + + return finalParameters; +} diff --git a/src/libs/Network.js b/src/libs/Network/index.js similarity index 71% rename from src/libs/Network.js rename to src/libs/Network/index.js index 16ce0acc0203..bccf4b4c3d6f 100644 --- a/src/libs/Network.js +++ b/src/libs/Network/index.js @@ -1,14 +1,15 @@ import _ from 'underscore'; import lodashGet from 'lodash/get'; import Onyx from 'react-native-onyx'; -import HttpUtils from './HttpUtils'; -import ONYXKEYS from '../ONYXKEYS'; -import * as ActiveClientManager from './ActiveClientManager'; -import CONST from '../CONST'; -import createCallback from './createCallback'; -import * as NetworkRequestQueue from './actions/NetworkRequestQueue'; - -let isReady = false; +import HttpUtils from '../HttpUtils'; +import ONYXKEYS from '../../ONYXKEYS'; +import * as ActiveClientManager from '../ActiveClientManager'; +import CONST from '../../CONST'; +import createCallback from '../createCallback'; +import * as NetworkRequestQueue from '../actions/NetworkRequestQueue'; +import * as NetworkStore from './NetworkStore'; +import enhanceParameters from './enhanceParameters'; + let isOffline = false; let isQueuePaused = false; let persistedRequestsQueueRunning = false; @@ -16,11 +17,6 @@ let persistedRequestsQueueRunning = false; // Queue for network requests so we don't lose actions done by the user while offline let networkRequestQueue = []; -// This is an optional function that this lib can be configured with (via registerParameterEnhancer()) -// that accepts all request parameters and returns a new copy of them. This allows other code to inject -// parameters such as authTokens or CSRF tokens, etc. -let enhanceParameters; - // These handlers must be registered so we can process the request, response, and errors returned from the queue. // The first argument passed will be the queuedRequest object and the second will be either the parameters, response, or error. const [onRequest, registerRequestHandler] = createCallback(); @@ -39,9 +35,7 @@ const [recheckConnectivity, registerConnectionCheckCallback] = createCallback(); * @returns {Promise} */ function processRequest(request) { - const finalParameters = _.isFunction(enhanceParameters) - ? enhanceParameters(request.command, request.data) - : request.data; + const finalParameters = enhanceParameters(request.command, request.data); // If request is still in processing after this time, we might be offline const timerId = setTimeout(recheckConnectivity, CONST.NETWORK.MAX_PENDING_TIME_MS); @@ -51,6 +45,14 @@ function processRequest(request) { .finally(() => clearTimeout(timerId)); } +/** + * This method will get any persisted requests and fire them off in parallel to retry them. + * If we get any jsonCode besides 407 the request is a success. It doesn't make sense to + * continually retry things that have returned a response. However, we can retry any requests + * with known networking errors like "Failed to fetch". + * + * @returns {Promise} + */ function processPersistedRequestsQueue() { const persistedRequests = NetworkRequestQueue.getPersistedRequests(); @@ -61,17 +63,32 @@ function processPersistedRequestsQueue() { const tasks = _.map(persistedRequests, request => processRequest(request) .then((response) => { - if (response.jsonCode !== CONST.HTTP_STATUS_CODE.SUCCESS) { - throw new Error(`Persisted request failed due to jsonCode: ${response.jsonCode}`); + if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { + getLogger().info('Persisted optimistic request needs authentication'); + } else { + getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.'); } - + onResponse(request, response); NetworkRequestQueue.removeRetryableRequest(request); }) .catch((error) => { - const retryCount = NetworkRequestQueue.incrementRetries(request); - getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); - if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { - // Request failed too many times removing from persisted storage + // If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left + if (error.message === CONST.ERROR.FAILED_TO_FETCH) { + const retryCount = NetworkRequestQueue.incrementRetries(request); + getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); + if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { + getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message}); + NetworkRequestQueue.removeRetryableRequest(request); + } + } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { + getLogger().info('Persisted request was cancelled. Not retrying.'); + onError(request); + NetworkRequestQueue.removeRetryableRequest(request); + } else { + getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, { + command: request.command, + error: error.message, + }); NetworkRequestQueue.removeRetryableRequest(request); } })); @@ -123,13 +140,6 @@ Onyx.connect({ }, }); -/** - * @param {Boolean} val - */ -function setIsReady(val) { - isReady = val; -} - /** * Checks to see if a request can be made. * @@ -141,7 +151,7 @@ function setIsReady(val) { * @return {Boolean} */ function canMakeRequest(request) { - if (!isReady) { + if (!NetworkStore.isReady()) { onRequestSkipped({command: request.command, type: request.type}); return false; } @@ -236,27 +246,41 @@ function processNetworkRequestQueue() { processRequest(queuedRequest) .then(response => onResponse(queuedRequest, response)) .catch((error) => { + // Cancelled requests should not be retried + if (error.name === CONST.ERROR.REQUEST_CANCELLED) { + onError(queuedRequest, error); + return; + } + recheckConnectivity(); - // When the request did not reach its destination add it back the queue to be retried - const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); - if (shouldRetry && error.name !== CONST.ERROR.REQUEST_CANCELLED) { - const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest); - getLogger().info('A retrieable request failed', false, { - retryCount, + // Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. + if (error.message === CONST.ERROR.FAILED_TO_FETCH) { + // When the request did not reach its destination add it back the queue to be retried if we can + const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); + if (shouldRetry) { + const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest); + getLogger().info('A retryable request failed', false, { + retryCount, + command: queuedRequest.command, + error: error.message, + }); + + if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) { + networkRequestQueue.push(queuedRequest); + return; + } + + getLogger().info('Request was retried too many times with no success. No more retries left'); + } + + onError(queuedRequest, error); + } else { + getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { command: queuedRequest.command, error: error.message, }); - - if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) { - networkRequestQueue.push(queuedRequest); - return; - } - - getLogger().info('Request was retried too many times with no success. No more retries left'); } - - onError(queuedRequest, error); }); }); @@ -337,23 +361,12 @@ function unpauseRequestQueue() { isQueuePaused = false; } -/** - * Register a function that will accept all the parameters being sent in a request - * and will return a new set of parameters to send instead. Useful for adding data to every request - * like auth or CRSF tokens. - * - * @param {Function} callback - */ -function registerParameterEnhancer(callback) { - enhanceParameters = callback; -} - /** * Clear the queue and cancels all pending requests * Non-cancellable requests like Log would not be cleared */ function clearRequestQueue() { - networkRequestQueue = _.filter(networkRequestQueue, r => !r.data.canCancel); + networkRequestQueue = _.filter(networkRequestQueue, request => !request.data.canCancel); HttpUtils.cancelPendingRequests(); } @@ -361,12 +374,10 @@ export { post, pauseRequestQueue, unpauseRequestQueue, - registerParameterEnhancer, clearRequestQueue, registerResponseHandler, registerErrorHandler, registerRequestHandler, - setIsReady, registerRequestSkippedHandler, registerLogHandler, registerConnectionCheckCallback, diff --git a/src/libs/actions/Session/index.js b/src/libs/actions/Session/index.js index d58583855e66..c508ca2a72ef 100644 --- a/src/libs/actions/Session/index.js +++ b/src/libs/actions/Session/index.js @@ -385,13 +385,13 @@ function changePasswordAndSignIn(authToken, password) { signIn(password); return; } - if (responsePassword.jsonCode === 407 && !credentials.login) { + if (responsePassword.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED && !credentials.login) { // authToken has expired, and we don't have the email set to request a new magic link. // send user to login page to enter email. Navigation.navigate(ROUTES.HOME); return; } - if (responsePassword.jsonCode === 407) { + if (responsePassword.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { // authToken has expired, and we have the account email, so we request a new magic link. Onyx.merge(ONYXKEYS.ACCOUNT, {accountExists: true, validateCodeExpired: true, error: null}); resetPassword(); @@ -478,7 +478,7 @@ function authenticatePusher(socketID, channelName, callback) { forceNetworkRequest: true, }) .then((data) => { - if (data.jsonCode === 407) { + if (data.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { callback(new Error('Expensify session expired'), {auth: ''}); // Attempt to refresh the authToken then reconnect to Pusher @@ -486,7 +486,7 @@ function authenticatePusher(socketID, channelName, callback) { return; } - if (data.jsonCode !== 200) { + if (data.jsonCode !== CONST.JSON_CODE.SUCCESS) { Log.hmmm('[PusherConnectionManager] Unable to authenticate Pusher for some reason other than expired session'); return; } diff --git a/tests/actions/ReimbursementAccountTest.js b/tests/actions/ReimbursementAccountTest.js index c711b0fbf35f..c04c49e290aa 100644 --- a/tests/actions/ReimbursementAccountTest.js +++ b/tests/actions/ReimbursementAccountTest.js @@ -7,6 +7,7 @@ import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import CONST from '../../src/CONST'; import BankAccount from '../../src/libs/models/BankAccount'; import * as Network from '../../src/libs/Network'; +import * as NetworkStore from '../../src/libs/Network/NetworkStore'; const TEST_BANK_ACCOUNT_ID = 1; const TEST_BANK_ACCOUNT_CITY = 'Opa-locka'; @@ -35,7 +36,7 @@ Onyx.connect({ beforeEach(() => Onyx.clear() .then(() => { - Network.setIsReady(true); + NetworkStore.setIsReady(true); TestHelper.signInWithTestUser(); return waitForPromisesToResolve(); })); diff --git a/tests/actions/SessionTest.js b/tests/actions/SessionTest.js index 62604fd77c67..cf03d8611405 100644 --- a/tests/actions/SessionTest.js +++ b/tests/actions/SessionTest.js @@ -5,6 +5,7 @@ import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ONYXKEYS from '../../src/ONYXKEYS'; import * as TestHelper from '../utils/TestHelper'; +import CONST from '../../src/CONST'; // We are mocking this method so that we can later test to see if it was called and what arguments it was called with. // We test HttpUtils.xhr() since this means that our API command turned into a network request and isn't only queued. @@ -57,12 +58,12 @@ test('Authenticate is called with saved credentials when a session expires', () // This will make the call to API.Get() below return with an expired session code .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 407, + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, })) // The next call should be Authenticate since we are reauthenticating .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, accountID: TEST_USER_ACCOUNT_ID, authToken: TEST_REFRESHED_AUTH_TOKEN, email: TEST_USER_LOGIN, diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 5dc1bb9f7980..c587f220b69a 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -11,8 +11,10 @@ import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ONYXKEYS from '../../src/ONYXKEYS'; import CONST from '../../src/CONST'; import * as Network from '../../src/libs/Network'; +import * as NetworkStore from '../../src/libs/Network/NetworkStore'; import * as Session from '../../src/libs/actions/Session'; -import * as NetworkQueue from '../../src/libs/actions/NetworkRequestQueue'; +import * as NetworkRequestQueue from '../../src/libs/actions/NetworkRequestQueue'; +import Log from '../../src/libs/Log'; // Set up manual mocks for methods used in the actions so our test does not fail. jest.mock('../../src/libs/Notification/PushNotification', () => ({ @@ -27,10 +29,17 @@ Onyx.init({ keys: ONYXKEYS, }); -beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); +const originalXHR = HttpUtils.xhr; + +beforeEach(() => { + HttpUtils.xhr = originalXHR; + NetworkRequestQueue.clearPersistedRequests(); + Network.clearRequestQueue(); + return Onyx.clear().then(waitForPromisesToResolve); +}); afterEach(() => { - Network.setIsReady(false); + NetworkStore.setIsReady(false); Onyx.addDelayToConnectCallback(0); jest.clearAllMocks(); }); @@ -61,7 +70,7 @@ test('failing to reauthenticate while offline should not log out user', () => { HttpUtils.xhr = jest.fn(); HttpUtils.xhr .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 407, + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, })) // Fail the call to re-authenticate @@ -69,18 +78,18 @@ test('failing to reauthenticate while offline should not log out user', () => { // The next call should still be using the old authToken .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 407, + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, })) // Succeed the call to set a new authToken .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, authToken: 'qwerty12345', })) // All remaining requests should succeed .mockImplementation(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, })); // This should first trigger re-authentication and then an API is offline error @@ -158,39 +167,39 @@ test('consecutive API calls eventually succeed when authToken is expired', () => // This will make the first call to API.Get() return with an expired session code .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 407, + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, })) // The next 2 API calls will also fire and also return a 407 .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 407, + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, })) .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 407, + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, })) // The request to Authenticate should succeed and we mock the responses for the remaining calls .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, authToken: 'qwerty12345', })) // Get&returnValueList=personalDetailsList .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, personalDetailsList: TEST_PERSONAL_DETAILS, })) // Get&returnValueList=account .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, account: TEST_ACCOUNT_DATA, })) // Get&returnValueList=chatList .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, + jsonCode: CONST.JSON_CODE.SUCCESS, chatList: TEST_CHAT_LIST, })); @@ -224,11 +233,8 @@ test('consecutive API calls eventually succeed when authToken is expired', () => }); test('retry network request if auth and credentials are not read from Onyx yet', () => { - // For this test we're having difficulty creating a situation where Onyx.connect() has not yet run - // because some Onyx.connect callbacks are already registered in API.js (which happens before this - // unit test is setup), so in order to test an scenario where the auth token and credentials hasn't - // been read from storage we set Network.setIsReady(false) and trigger an update in the Onyx.connect - // callbacks registered in API.js merging an empty object. + // In order to test an scenario where the auth token and credentials hasn't been read from storage we set + // NetworkStore.setIsReady(false) and set the session and credentials to "ready" the Network // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; @@ -238,15 +244,14 @@ test('retry network request if auth and credentials are not read from Onyx yet', Onyx.addDelayToConnectCallback(ONYX_DELAY_MS); // Given initial state to Network - Network.setIsReady(false); + NetworkStore.setIsReady(false); - // Given any initial value to trigger an update - Onyx.merge(ONYXKEYS.CREDENTIALS, {}); - Onyx.merge(ONYXKEYS.SESSION, {}); + // Given an initial value to trigger an update + Onyx.merge(ONYXKEYS.CREDENTIALS, {login: 'test-login'}); + Onyx.merge(ONYXKEYS.SESSION, {authToken: 'test-auth-token'}); // Given some mock functions to track the isReady // flag in Network and the http requests made - const spyNetworkSetIsReady = jest.spyOn(Network, 'setIsReady'); const spyHttpUtilsXhr = jest.spyOn(HttpUtils, 'xhr').mockImplementation(() => Promise.resolve({})); // When we make an arbitrary request that can be retried @@ -254,7 +259,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', Session.fetchAccountDetails(TEST_USER_LOGIN); return waitForPromisesToResolve().then(() => { // Then we expect not having the Network ready and not making an http request - expect(spyNetworkSetIsReady).not.toHaveBeenCalled(); + expect(NetworkStore.isReady()).toBe(false); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // When we resolve Onyx.connect callbacks @@ -262,7 +267,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', // Then we should expect call Network.setIsReady(true) // And We should expect not making an http request yet - expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); + expect(NetworkStore.isReady()).toBe(true); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // When we run processNetworkRequestQueue in the setInterval of Network.js @@ -278,11 +283,11 @@ test('retry network request if connection is lost while request is running', () const xhr = jest.spyOn(HttpUtils, 'xhr') .mockImplementationOnce(() => { Onyx.merge(ONYXKEYS.NETWORK, {isOffline: true}); - return Promise.reject(new Error('Network request failed')); + return Promise.reject(new Error(CONST.ERROR.FAILED_TO_FETCH)); }) - .mockResolvedValue({jsonCode: 200, fromRetriedResult: true}); + .mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS, fromRetriedResult: true}); - // Given a regular "retriable" request (that is bound to fail) + // Given a regular "retryable" request (that is bound to fail) const promise = Network.post('Get'); return waitForPromisesToResolve() @@ -301,7 +306,7 @@ test('retry network request if connection is lost while request is running', () expect(xhr).toHaveBeenCalledTimes(2); // And the promise should be resolved with the 2nd call that succeeded - return expect(promise).resolves.toEqual({jsonCode: 200, fromRetriedResult: true}); + return expect(promise).resolves.toEqual({jsonCode: CONST.JSON_CODE.SUCCESS, fromRetriedResult: true}); }); }); @@ -322,7 +327,7 @@ test('requests should be persisted while offline', () => { // Then `xhr` should not be used and requests should be persisted to storage expect(xhr).not.toHaveBeenCalled(); - const persisted = NetworkQueue.getPersistedRequests(); + const persisted = NetworkRequestQueue.getPersistedRequests(); expect(persisted).toEqual([ expect.objectContaining({command: 'mock command', data: expect.objectContaining({param1: 'value1'})}), expect.objectContaining({command: 'mock command', data: expect.objectContaining({param3: 'value3'})}), @@ -332,7 +337,7 @@ test('requests should be persisted while offline', () => { test('requests should resume when we are online', () => { // We're setting up a basic case where all requests succeed when we resume connectivity - const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValue({jsonCode: 200}); + const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS}); // Given we have some requests made while we're offline return Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}) @@ -354,12 +359,12 @@ test('requests should resume when we are online', () => { expect.arrayContaining(['mock command', expect.objectContaining({param2: 'value2'})]), ]); - const persisted = NetworkQueue.getPersistedRequests(); + const persisted = NetworkRequestQueue.getPersistedRequests(); expect(persisted).toEqual([]); }); }); -test('persisted request should not be cleared unit a backend response', () => { +test('persisted request should not be cleared until a backend response occurs', () => { // We're setting up xhr handler that will resolve calls programmatically const xhrCalls = []; const promises = []; @@ -386,41 +391,41 @@ test('persisted request should not be cleared unit a backend response', () => { .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) .then(() => { // Then requests should remain persisted until the xhr call is resolved - expect(_.size(NetworkQueue.getPersistedRequests())).toEqual(2); + expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(2); - xhrCalls[0].resolve({jsonCode: 200}); + xhrCalls[0].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); return waitForPromisesToResolve(); }) .then(waitForPromisesToResolve) .then(() => { - expect(_.size(NetworkQueue.getPersistedRequests())).toEqual(1); - expect(NetworkQueue.getPersistedRequests()).toEqual([ + expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(1); + expect(NetworkRequestQueue.getPersistedRequests()).toEqual([ expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})}), ]); // When a request fails it should be retried - xhrCalls[1].resolve({jsonCode: 401}); + xhrCalls[1].reject(new Error(CONST.ERROR.FAILED_TO_FETCH)); return waitForPromisesToResolve(); }) .then(() => { - expect(_.size(NetworkQueue.getPersistedRequests())).toEqual(1); - expect(NetworkQueue.getPersistedRequests()).toEqual([ + expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(1); + expect(NetworkRequestQueue.getPersistedRequests()).toEqual([ expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})}), ]); // Finally, after it succeeds the queue should be empty - xhrCalls[2].resolve({jsonCode: 200}); + xhrCalls[2].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); return waitForPromisesToResolve(); }) .then(() => { - expect(_.size(NetworkQueue.getPersistedRequests())).toEqual(0); + expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(0); }); }); test(`persisted request should be retried up to ${CONST.NETWORK.MAX_REQUEST_RETRIES} times`, () => { - // We're setting up xhr handler that always returns an error response + // We're setting up xhr handler that always rejects with a fetch error const xhr = jest.spyOn(HttpUtils, 'xhr') - .mockResolvedValue({jsonCode: 401}); + .mockRejectedValue(new Error(CONST.ERROR.FAILED_TO_FETCH)); // Given we have a request made while we're offline return Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}) @@ -443,3 +448,77 @@ test(`persisted request should be retried up to ${CONST.NETWORK.MAX_REQUEST_RETR }); }); }); + +test('test bad response will log alert', () => { + global.fetch = jest.fn() + .mockResolvedValueOnce({ok: false, status: 502, statusText: 'Bad Gateway'}); + + const logAlertSpy = jest.spyOn(Log, 'alert'); + + // Given we have a request made while online + return Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}) + .then(() => { + Network.post('MockBadNetworkResponse', {param1: 'value1'}); + return waitForPromisesToResolve(); + }) + .then(() => { + expect(logAlertSpy).toHaveBeenCalled(); + }); +}); + +test('test Failed to fetch error for requests not flagged with shouldRetry will throw API OFFLINE error', () => { + // Setup xhr handler that rejects once with a 502 Bad Gateway + global.fetch = jest.fn(() => new Promise((_resolve, reject) => reject(new Error(CONST.ERROR.FAILED_TO_FETCH)))); + + const onRejected = jest.fn(); + + // Given we have a request made while online + return Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}) + .then(() => { + // When network calls with are made + Network.post('mock command', {param1: 'value1', shouldRetry: false}) + .catch(onRejected); + + return waitForPromisesToResolve(); + }) + .then(() => { + const error = onRejected.mock.calls[0][0]; + expect(onRejected).toHaveBeenCalled(); + expect(error.message).toBe(CONST.ERROR.API_OFFLINE); + }); +}); + +test('persisted request can trigger reauthentication for anything retryable', () => { + // We're setting up xhr handler that rejects once with a 407 code and again with success + const xhr = jest.spyOn(HttpUtils, 'xhr') + .mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS}) // Default + .mockResolvedValueOnce({jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED}) // Initial call to test command return 407 + .mockResolvedValueOnce({jsonCode: CONST.JSON_CODE.SUCCESS}) // Call to Authenticate return 200 + .mockResolvedValueOnce({jsonCode: CONST.JSON_CODE.SUCCESS}); // Original command return 200 + + // Given we have a request made while we're offline and we have credentials available to reauthenticate + Onyx.merge(ONYXKEYS.CREDENTIALS, {autoGeneratedLogin: 'caca', autoGeneratedPassword: 'caca'}); + return waitForPromisesToResolve() + .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: true})) + .then(() => { + Network.post('Mock', {param1: 'value1', persist: true, shouldRetry: true}); + return waitForPromisesToResolve(); + }) + + // When we resume connectivity + .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) + .then(waitForPromisesToResolve) + .then(() => { + const nonLogCalls = _.filter(xhr.mock.calls, ([commandName]) => commandName !== 'Log'); + + // The request should be retried once and reauthenticate should be called the second time + // expect(xhr).toHaveBeenCalledTimes(3); + const [call1, call2, call3] = nonLogCalls; + const [commandName1] = call1; + const [commandName2] = call2; + const [commandName3] = call3; + expect(commandName1).toBe('Mock'); + expect(commandName2).toBe('Authenticate'); + expect(commandName3).toBe('Mock'); + }); +}); diff --git a/tests/unit/enhanceParametersTest.js b/tests/unit/enhanceParametersTest.js new file mode 100644 index 000000000000..a4fc9f70a9cf --- /dev/null +++ b/tests/unit/enhanceParametersTest.js @@ -0,0 +1,44 @@ +import Onyx from 'react-native-onyx'; +import ONYXKEYS from '../../src/ONYXKEYS'; +import enhanceParameters from '../../src/libs/Network/enhanceParameters'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import CONFIG from '../../src/CONFIG'; + +beforeEach(() => Onyx.clear()); + +test('Enhance parameters adds correct parameters for Log command with no authToken', () => { + const command = 'Log'; + const parameters = {testParameter: 'test'}; + const email = 'test-user@test.com'; + const authToken = 'test-token'; + Onyx.merge(ONYXKEYS.SESSION, {email, authToken}); + return waitForPromisesToResolve() + .then(() => { + const finalParameters = enhanceParameters(command, parameters); + expect(finalParameters).toEqual({ + testParameter: 'test', + api_setCookie: false, + email, + referer: CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER, + }); + }); +}); + +test('Enhance parameters adds correct parameters for a command that requires authToken', () => { + const command = 'Report_AddComment'; + const parameters = {testParameter: 'test'}; + const email = 'test-user@test.com'; + const authToken = 'test-token'; + Onyx.merge(ONYXKEYS.SESSION, {email, authToken}); + return waitForPromisesToResolve() + .then(() => { + const finalParameters = enhanceParameters(command, parameters); + expect(finalParameters).toEqual({ + testParameter: 'test', + api_setCookie: false, + email, + authToken, + referer: CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER, + }); + }); +}); From 1bd755465dbb6cbff04a12ba54acd0da5488b4dd Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 11:01:55 -1000 Subject: [PATCH 2/4] remove unused import --- tests/actions/ReimbursementAccountTest.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/actions/ReimbursementAccountTest.js b/tests/actions/ReimbursementAccountTest.js index c04c49e290aa..05db25959fcc 100644 --- a/tests/actions/ReimbursementAccountTest.js +++ b/tests/actions/ReimbursementAccountTest.js @@ -6,7 +6,6 @@ import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import CONST from '../../src/CONST'; import BankAccount from '../../src/libs/models/BankAccount'; -import * as Network from '../../src/libs/Network'; import * as NetworkStore from '../../src/libs/Network/NetworkStore'; const TEST_BANK_ACCOUNT_ID = 1; From cbe2b6eb6e8958af48adb1cbf2a0e5188a1cfcf2 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 13:58:32 -1000 Subject: [PATCH 3/4] Add clarifying comments --- src/libs/API.js | 8 ++++++++ src/libs/Network/NetworkStore.js | 4 ++++ src/libs/Network/index.js | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/libs/API.js b/src/libs/API.js index 21f440c97cd3..0dd443b93792 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -92,6 +92,8 @@ Network.registerResponseHandler((queuedRequest, response) => { // of the new response created by handleExpiredAuthToken. const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); if (!shouldRetry || unableToReauthenticate) { + // Check to see if queuedRequest has a resolve method as this could be a persisted request which had it's promise handling logic stripped + // from it when persisted to storage if (queuedRequest.resolve) { queuedRequest.resolve(response); } @@ -127,6 +129,8 @@ Network.registerErrorHandler((queuedRequest, error) => { setSessionLoadingAndError(false, 'Cannot connect to server'); // Reject the queued request with an API offline error so that the original caller can handle it + // Note: We are checking for the reject method as this could be a persisted request which had it's promise handling logic stripped + // from it when persisted to storage if (queuedRequest.reject) { queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); } @@ -229,6 +233,10 @@ function reauthenticate(command = '') { // Update authToken in Onyx and in our local variables so that API requests will use the // new authToken updateSessionAuthTokens(response.authToken, response.encryptedAuthToken); + + // Note: It is important to manually set the authToken that is in the store here since any requests that are hooked into + // reauthenticate .then() will immediate post and use the local authToken. Onyx updates subscribers lately so it is not + // enough to do the updateSessionAuthTokens() call above. NetworkStore.setAuthToken(response.authToken); // The authentication process is finished so the network can be unpaused to continue diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index 3d7880b942e8..bb3725d67d0c 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -15,6 +15,10 @@ function setIsReady(ready) { networkReady = ready; } +/** + * This is a hack to workaround the fact that Onyx may not yet have read these values from storage by the time Network starts processing requests. + * If the values are undefined we haven't read them yet. If they are null or have a value then we have and the network is "ready". + */ function checkRequiredDataAndSetNetworkReady() { if (_.isUndefined(authToken) || _.isUndefined(credentials)) { return; diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index bccf4b4c3d6f..8e526920724f 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -254,7 +254,7 @@ function processNetworkRequestQueue() { recheckConnectivity(); - // Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. + // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. if (error.message === CONST.ERROR.FAILED_TO_FETCH) { // When the request did not reach its destination add it back the queue to be retried if we can const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); From fb3904c9c90b7d3e643abb7d77fe0da487f969bf Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 14:01:46 -1000 Subject: [PATCH 4/4] Fix comment --- src/libs/Network/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index 8e526920724f..f27e418dbaf9 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -254,7 +254,8 @@ function processNetworkRequestQueue() { recheckConnectivity(); - // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. + // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario + // like incorrect url, bad cors headers returned by the server, DNS lookup failure etc. if (error.message === CONST.ERROR.FAILED_TO_FETCH) { // When the request did not reach its destination add it back the queue to be retried if we can const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry');