Skip to content

Commit

Permalink
Merge pull request #6266 from Expensify/marcaaron-fixLogDependency
Browse files Browse the repository at this point in the history 
[No QA] Fix dependency cycles
  • Loading branch information
@marcaaron
marcaaron authored Nov 23, 2021
2 parents 1f114cc + 13e72e5 commit 0dfdf36
Show file tree
Hide file tree
Showing 20 changed files with 216 additions and 198 deletions.
90 changes: 35 additions & 55 deletions src/libs/API.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@ import Onyx from 'react-native-onyx';
import CONST from '../CONST';
import CONFIG from '../CONFIG';
import ONYXKEYS from '../ONYXKEYS';
// eslint-disable-next-line import/no-cycle
import redirectToSignIn from './actions/SignInRedirect';
import isViaExpensifyCashNative from './isViaExpensifyCashNative';
// eslint-disable-next-line import/no-cycle
import requireParameters from './requireParameters';
import Log from './Log';
import * as Network from './Network';
// eslint-disable-next-line import/no-cycle
import LogUtil from './Log';
// eslint-disable-next-line import/no-cycle
import * as Session from './actions/Session';
import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens';
import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError';

let isAuthenticating;
let credentials;
Expand Down Expand Up @@ -89,35 +87,6 @@ function addDefaultValuesToParameters(command, parameters) {
// Tie into the network layer to add auth token to the parameters of all requests
Network.registerParameterEnhancer(addDefaultValuesToParameters);

/**
* @throws {Error} If the "parameters" object has a null or undefined value for any of the given parameterNames
*
* @param {String[]} parameterNames Array of the required parameter names
* @param {Object} parameters A map from available parameter names to their values
* @param {String} commandName The name of the API command
*/
function requireParameters(parameterNames, parameters, commandName) {
parameterNames.forEach((parameterName) => {
if (_(parameters).has(parameterName)
&& parameters[parameterName] !== null
&& parameters[parameterName] !== undefined
) {
return;
}

const propertiesToRedact = ['authToken', 'password', 'partnerUserSecret', 'twoFactorAuthCode'];
const parametersCopy = _.chain(parameters)
.clone()
.mapObject((val, key) => (_.contains(propertiesToRedact, key) ? '<redacted>' : val))
.value();
const keys = _(parametersCopy).keys().join(', ') || 'none';

let error = `Parameter ${parameterName} is required for "${commandName}". `;
error += `Supplied parameters: ${keys}`;
throw new Error(error);
});
}

/**
* Function used to handle expired auth tokens. It re-authenticates with the API and
* then replays the original request
Expand Down Expand Up @@ -153,7 +122,34 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp
));
}

Network.registerRequestHandler((queuedRequest, finalParameters) => {
if (queuedRequest.command === 'Log') {
return;
}

Log.info('Making API request', false, {
command: queuedRequest.command,
type: queuedRequest.type,
shouldUseSecure: queuedRequest.type,
rvl: finalParameters.returnValueList,
});
});

Network.registerRequestSkippedHandler((parameters) => {
Log.hmmm('Trying to make a request when Network is not ready', parameters);
});

Network.registerResponseHandler((queuedRequest, response) => {
if (queuedRequest.command !== 'Log') {
Log.info('Finished API request', false, {
command: queuedRequest.command,
type: queuedRequest.type,
shouldUseSecure: queuedRequest.shouldUseSecure,
jsonCode: response.jsonCode,
requestID: response.requestID,
});
}

if (response.jsonCode === 407) {
// Credentials haven't been initialized. We will not be able to re-authenticates with the API
const unableToReauthenticate = (!credentials || !credentials.autoGeneratedLogin
Expand Down Expand Up @@ -186,13 +182,13 @@ Network.registerResponseHandler((queuedRequest, response) => {

Network.registerErrorHandler((queuedRequest, error) => {
if (queuedRequest.command !== 'Log') {
LogUtil.hmmm('[API] Handled error when making request', error);
Log.hmmm('[API] Handled error when making request', error);
} else {
console.debug('[API] There was an error in the Log API command, unable to log to server!', error);
}

// Set an error state and signify we are done loading
Session.setSessionLoadingAndError(false, 'Cannot connect to server');
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));
Expand Down Expand Up @@ -293,7 +289,7 @@ function reauthenticate(command = '') {

// Update authToken in Onyx and in our local variables so that API requests will use the
// new authToken
Session.updateSessionAuthTokens(response.authToken, response.encryptedAuthToken);
updateSessionAuthTokens(response.authToken, response.encryptedAuthToken);
authToken = response.authToken;

// The authentication process is finished so the network can be unpaused to continue
Expand All @@ -317,7 +313,7 @@ function reauthenticate(command = '') {
// If we experience something other than a network error then redirect the user to sign in
redirectToSignIn(error.message);

LogUtil.hmmm('Redirecting to Sign In because we failed to reauthenticate', {
Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', {
command,
error: error.message,
});
Expand Down Expand Up @@ -516,21 +512,6 @@ function GetRequestCountryCode() {
return Network.post(commandName);
}

/**
* @param {Object} parameters
* @param {String} parameters.expensifyCashAppVersion
* @param {Object[]} parameters.logPacket
* @returns {Promise}
*/
function Log(parameters) {
const commandName = 'Log';
requireParameters(['logPacket', 'expensifyCashAppVersion'],
parameters, commandName);

// Note: We are forcing Log to run since it requires no authToken and should only be queued when we are offline.
return Network.post(commandName, {...parameters, forceNetworkRequest: true});
}

/**
* @param {Object} parameters
* @param {String} parameters.name
Expand Down Expand Up @@ -1155,7 +1136,6 @@ export {
GetRequestCountryCode,
Graphite_Timer,
Inbox_CallUser,
Log,
PayIOU,
PayWithWallet,
PersonalDetails_GetForEmails,
Expand Down
30 changes: 1 addition & 29 deletions src/libs/HttpUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ import CONFIG from '../CONFIG';
import CONST from '../CONST';
import ONYXKEYS from '../ONYXKEYS';

// To avoid a circular dependency, we can't include Log here, so instead, we define an empty logging method and expose the setLogger method to set the logger from outside this file
let info = () => {};

let shouldUseSecureStaging = false;
Onyx.connect({
key: ONYXKEYS.USER,
Expand Down Expand Up @@ -39,14 +36,6 @@ function processHTTPRequest(url, method = 'get', body = null) {
* @returns {Promise}
*/
function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) {
if (command !== 'Log') {
info('Making API request', false, {
command,
type,
shouldUseSecure,
rvl: data.returnValueList,
});
}
const formData = new FormData();
_.each(data, (val, key) => formData.append(key, val));
let apiRoot = shouldUseSecure ? CONFIG.EXPENSIFY.URL_EXPENSIFY_SECURE : CONFIG.EXPENSIFY.URL_API_ROOT;
Expand All @@ -55,19 +44,7 @@ function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure =
apiRoot = CONST.STAGING_SECURE_URL;
}

return processHTTPRequest(`${apiRoot}api?command=${command}`, type, formData)
.then((response) => {
if (command !== 'Log') {
info('Finished API request', false, {
command,
type,
shouldUseSecure,
jsonCode: response.jsonCode,
requestID: response.requestID,
});
}
return response;
});
return processHTTPRequest(`${apiRoot}api?command=${command}`, type, formData);
}

/**
Expand All @@ -87,12 +64,7 @@ function download(relativePath) {
return processHTTPRequest(`${siteRoot}${strippedRelativePath}`);
}

function setLogger(logger) {
info = logger.info;
}

export default {
setLogger,
download,
xhr,
};
33 changes: 20 additions & 13 deletions src/libs/Log.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,26 @@ import Logger from 'expensify-common/lib/Logger';
import CONFIG from '../CONFIG';
import getPlatform from './getPlatform';
import {version} from '../../package.json';
import NetworkConnection from './NetworkConnection';
import HttpUtils from './HttpUtils';

// eslint-disable-next-line import/no-cycle
import * as API from './API';
import requireParameters from './requireParameters';
import * as Network from './Network';

let timeout = null;

/**
* @param {Object} parameters
* @param {String} parameters.expensifyCashAppVersion
* @param {Object[]} parameters.logPacket
* @returns {Promise}
*/
function LogCommand(parameters) {
const commandName = 'Log';
requireParameters(['logPacket', 'expensifyCashAppVersion'],
parameters, commandName);

// Note: We are forcing Log to run since it requires no authToken and should only be queued when we are offline.
return Network.post(commandName, {...parameters, forceNetworkRequest: true});
}

/**
* Network interface for logger.
*
Expand All @@ -31,13 +43,11 @@ function serverLoggingCallback(logger, params) {
}
clearTimeout(timeout);
timeout = setTimeout(() => logger.info('Flushing logs older than 10 minutes', true, {}, true), 10 * 60 * 1000);
return API.Log(requestParams);
return LogCommand(requestParams);
}

// Note: We are importing Logger from expensify-common because it is
// used by other platforms. The server and client logging
// callback methods are passed in here so we can decouple
// the logging library from the logging methods.
// Note: We are importing Logger from expensify-common because it is used by other platforms. The server and client logging
// callback methods are passed in here so we can decouple the logging library from the logging methods.
const Log = new Logger({
serverLoggingCallback,
clientLoggingCallback: (message) => {
Expand All @@ -47,7 +57,4 @@ const Log = new Logger({
});
timeout = setTimeout(() => Log.info('Flushing logs older than 10 minutes', true, {}, true), 10 * 60 * 1000);

NetworkConnection.registerLogInfoCallback(Log.info);
HttpUtils.setLogger(Log);

export default Log;
1 change: 0 additions & 1 deletion src/libs/Navigation/Navigation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
} from '@react-navigation/native';
import PropTypes from 'prop-types';
import Onyx from 'react-native-onyx';
// eslint-disable-next-line import/no-cycle
import Log from '../Log';
import linkTo from './linkTo';
import ROUTES from '../../ROUTES';
Expand Down
35 changes: 11 additions & 24 deletions src/libs/Network.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import HttpUtils from './HttpUtils';
import ONYXKEYS from '../ONYXKEYS';
import * as ActiveClientManager from './ActiveClientManager';
import CONST from '../CONST';
// eslint-disable-next-line import/no-cycle
import LogUtil from './Log';
import createCallback from './createCallback';
import * as NetworkRequestQueue from './actions/NetworkRequestQueue';

let isReady = false;
Expand All @@ -20,10 +19,12 @@ let networkRequestQueue = [];
// parameters such as authTokens or CSRF tokens, etc.
let enhanceParameters;

// These handlers must be registered in order to process the response or network errors returned from the queue.
// The first argument passed will be the queuedRequest object and the second will be either the response or error.
let onResponse = () => {};
let onError = () => {};
// 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();
const [onResponse, registerResponseHandler] = createCallback();
const [onError, registerErrorHandler] = createCallback();
const [onRequestSkipped, registerRequestSkippedHandler] = createCallback();

let didLoadPersistedRequests;
let isOffline;
Expand Down Expand Up @@ -120,7 +121,7 @@ function setIsReady(val) {
*/
function canMakeRequest(request) {
if (!isReady) {
LogUtil.hmmm('Trying to make a request when Network is not ready', {command: request.command, type: request.type});
onRequestSkipped({command: request.command, type: request.type});
return false;
}

Expand Down Expand Up @@ -221,6 +222,7 @@ function processNetworkRequestQueue() {
? enhanceParameters(queuedRequest.command, requestData)
: requestData;

onRequest(queuedRequest, finalParameters);
HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure)
.then(response => onResponse(queuedRequest, response))
.catch(error => onError(queuedRequest, error));
Expand Down Expand Up @@ -323,23 +325,6 @@ function clearRequestQueue() {
networkRequestQueue = [];
}

/**
* Register a method to call when the authToken expires
* @param {Function} callback
*/
function registerResponseHandler(callback) {
onResponse = callback;
}

/**
* The error handler will handle fetch() errors. Not used for successful responses that might send expected error codes
* e.g. jsonCode: 407.
* @param {Function} callback
*/
function registerErrorHandler(callback) {
onError = callback;
}

export {
post,
pauseRequestQueue,
Expand All @@ -349,5 +334,7 @@ export {
clearRequestQueue,
registerResponseHandler,
registerErrorHandler,
registerRequestHandler,
setIsReady,
registerRequestSkippedHandler,
};
Loading

0 comments on commit 0dfdf36

Please sign in to comment.