From a65d7886405244e212857cb817869945664867c1 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 17 Dec 2024 19:00:49 +0100 Subject: [PATCH 01/16] feat: use OnyxUpdateManager to fetch pending updates from server, if payload was too big --- .../PushNotification/NotificationType.ts | 1 + .../subscribePushNotification/index.ts | 23 ++++++++++-- src/libs/actions/OnyxUpdateManager/index.ts | 36 +++++++++++++------ src/libs/actions/OnyxUpdates.ts | 10 ++++-- src/libs/actions/applyOnyxUpdatesReliably.ts | 33 +++++++++++++---- src/types/onyx/OnyxUpdatesFromServer.ts | 3 ++ 6 files changed, 83 insertions(+), 23 deletions(-) diff --git a/src/libs/Notification/PushNotification/NotificationType.ts b/src/libs/Notification/PushNotification/NotificationType.ts index 0ae8d9cc488e..28d98c7aec42 100644 --- a/src/libs/Notification/PushNotification/NotificationType.ts +++ b/src/libs/Notification/PushNotification/NotificationType.ts @@ -19,6 +19,7 @@ type BasePushNotificationData = { onyxData?: OnyxServerUpdate[]; lastUpdateID?: number; previousUpdateID?: number; + hasPendingOnyxUpdates?: boolean; }; type ReportActionPushNotificationData = BasePushNotificationData & { diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index 61af079f9ed1..0e49edda8bd3 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -48,7 +48,7 @@ function getLastUpdateIDAppliedToClient(): Promise { }); } -function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previousUpdateID}: ReportActionPushNotificationData): Promise { +function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previousUpdateID, hasPendingOnyxUpdates}: ReportActionPushNotificationData): Promise { Log.info(`[PushNotification] Applying onyx data in the ${Visibility.isVisible() ? 'foreground' : 'background'}`, false, {reportID, reportActionID}); if (!ActiveClientManager.isClientTheLeader()) { @@ -56,7 +56,22 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo return Promise.resolve(); } - if (!onyxData || !lastUpdateID || !previousUpdateID) { + const shouldFetchPendingUpdates = hasPendingOnyxUpdates && !!lastUpdateID && !!previousUpdateID; + if (shouldFetchPendingUpdates) { + const updates: OnyxUpdatesFromServer = { + type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, + lastUpdateID, + previousUpdateID, + shouldFetchPendingUpdates: true, + }; + + return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => + applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient, shouldFetchPendingUpdates: lastUpdateID}), + ); + } + + const isDataMissing = !onyxData || !previousUpdateID || !lastUpdateID; + if (isDataMissing) { Log.hmmm("[PushNotification] didn't apply onyx updates because some data is missing", {lastUpdateID, previousUpdateID, onyxDataCount: onyxData?.length ?? 0}); return Promise.resolve(); } @@ -79,7 +94,9 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo * lastUpdateIDAppliedToClient will NOT be populated in other libs. To workaround this, we manually read the value here * and pass it as a param */ - return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => applyOnyxUpdatesReliably(updates, true, lastUpdateIDAppliedToClient)); + return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => + applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient, shouldFetchPendingUpdates: lastUpdateID}), + ); } function navigateToReport({reportID, reportActionID}: ReportActionPushNotificationData): Promise { diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 085e05b0a449..0d88ffb001d5 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -63,13 +63,23 @@ function finalizeUpdatesAndResumeQueue() { DeferredOnyxUpdates.clear(); } +type FetchMissingUpdatesIds = { + clientLastUpdateID?: number; + shouldFetchPendingUpdates?: boolean; +}; + /** * * @param onyxUpdatesFromServer * @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient * @returns */ -function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { +function handleOnyxUpdateGap( + onyxUpdatesFromServer: OnyxEntry, + {clientLastUpdateID, shouldFetchPendingUpdates: shouldFetchPendingUpdatesProp}: FetchMissingUpdatesIds = {}, +) { + const shouldFetchPendingUpdates = shouldFetchPendingUpdatesProp ?? onyxUpdatesFromServer?.shouldFetchPendingUpdates; + // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, // we'll only have them overriten by the openApp response. So let's skip it and return. @@ -87,18 +97,21 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry { }; export {handleOnyxUpdateGap, queryPromiseWrapper as queryPromise, resetDeferralLogicVariables}; +export type {FetchMissingUpdatesIds}; diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index 52dfa9dfd742..2d3eb6c59563 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -163,19 +163,24 @@ function saveUpdateInformation(updateParams: OnyxUpdatesFromServer) { Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, updateParams); } +type ManualOnyxUpdateCheckIds = { + clientLastUpdateID?: number; + previousUpdateID?: number; +}; + /** * This function will receive the previousUpdateID from any request/pusher update that has it, compare to our current app state * and return if an update is needed * @param previousUpdateID The previousUpdateID contained in the response object * @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient */ -function doesClientNeedToBeUpdated(previousUpdateID = 0, clientLastUpdateID = 0): boolean { +function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID}: ManualOnyxUpdateCheckIds): boolean { // If no previousUpdateID is sent, this is not a WRITE request so we don't need to update our current state if (!previousUpdateID) { return false; } - const lastUpdateIDFromClient = clientLastUpdateID || lastUpdateIDAppliedToClient; + const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient; // If we don't have any value in lastUpdateIDFromClient, this is the first time we're receiving anything, so we need to do a last reconnectApp if (!lastUpdateIDFromClient) { @@ -192,3 +197,4 @@ function doesClientNeedToBeUpdated(previousUpdateID = 0, clientLastUpdateID = 0) // eslint-disable-next-line import/prefer-default-export export {apply, doesClientNeedToBeUpdated, saveUpdateInformation}; +export type {ManualOnyxUpdateCheckIds}; diff --git a/src/libs/actions/applyOnyxUpdatesReliably.ts b/src/libs/actions/applyOnyxUpdatesReliably.ts index 17754712cdc8..f1ac4d8dec48 100644 --- a/src/libs/actions/applyOnyxUpdatesReliably.ts +++ b/src/libs/actions/applyOnyxUpdatesReliably.ts @@ -1,7 +1,12 @@ import type {OnyxUpdatesFromServer} from '@src/types/onyx'; +import type {FetchMissingUpdatesIds} from './OnyxUpdateManager'; import {handleOnyxUpdateGap} from './OnyxUpdateManager'; import * as OnyxUpdates from './OnyxUpdates'; +type ApplyOnyxUpdatesReliablyOptions = FetchMissingUpdatesIds & { + shouldRunSync?: boolean; +}; + /** * Checks for and handles gaps of onyx updates between the client and the given server updates before applying them * @@ -11,16 +16,30 @@ import * as OnyxUpdates from './OnyxUpdates'; * @param shouldRunSync * @returns */ -export default function applyOnyxUpdatesReliably(updates: OnyxUpdatesFromServer, shouldRunSync = false, clientLastUpdateID = 0) { +export default function applyOnyxUpdatesReliably( + updates: OnyxUpdatesFromServer, + {shouldRunSync = false, clientLastUpdateID, shouldFetchPendingUpdates: shouldFetchPendingUpdatesProp}: ApplyOnyxUpdatesReliablyOptions = {}, +) { + const fetchMissingUpdates = (shouldFetchPendingUpdates = false) => { + if (shouldRunSync) { + handleOnyxUpdateGap(updates, {clientLastUpdateID, shouldFetchPendingUpdates}); + } else { + OnyxUpdates.saveUpdateInformation({...updates, shouldFetchPendingUpdates}); + } + }; + + // If a pendingLastUpdateID is was provided, it means that the backend didn't send updates because the payload was too big. + // In this case, we need to fetch the missing updates up to the pendingLastUpdateID. + const shouldFetchPendingUpdates = shouldFetchPendingUpdatesProp && updates == null; + if (shouldFetchPendingUpdatesProp) { + return fetchMissingUpdates(shouldFetchPendingUpdates); + } + const previousUpdateID = Number(updates.previousUpdateID) || 0; - if (!OnyxUpdates.doesClientNeedToBeUpdated(previousUpdateID, clientLastUpdateID)) { + if (!OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID})) { OnyxUpdates.apply(updates); return; } - if (shouldRunSync) { - handleOnyxUpdateGap(updates, clientLastUpdateID); - } else { - OnyxUpdates.saveUpdateInformation(updates); - } + fetchMissingUpdates(); } diff --git a/src/types/onyx/OnyxUpdatesFromServer.ts b/src/types/onyx/OnyxUpdatesFromServer.ts index 99d565ede08f..0de3c975eefb 100644 --- a/src/types/onyx/OnyxUpdatesFromServer.ts +++ b/src/types/onyx/OnyxUpdatesFromServer.ts @@ -32,6 +32,9 @@ type OnyxUpdatesFromServer = { /** Previous update ID from server */ previousUpdateID: number | string; + /** Whether the client should fetch pending updates from the server */ + shouldFetchPendingUpdates?: boolean; + /** Request data sent to the server */ request?: Request; From 1bf9367cee51e20fec604214301dd49818c6d7a6 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 13:57:37 +0100 Subject: [PATCH 02/16] fix: default value for shouldFetchPendingUpdates --- .../PushNotification/subscribePushNotification/index.ts | 6 ++---- src/libs/actions/applyOnyxUpdatesReliably.ts | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index 0e49edda8bd3..06cca15a0def 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -66,7 +66,7 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo }; return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => - applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient, shouldFetchPendingUpdates: lastUpdateID}), + applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient, shouldFetchPendingUpdates: true}), ); } @@ -94,9 +94,7 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo * lastUpdateIDAppliedToClient will NOT be populated in other libs. To workaround this, we manually read the value here * and pass it as a param */ - return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => - applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient, shouldFetchPendingUpdates: lastUpdateID}), - ); + return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient})); } function navigateToReport({reportID, reportActionID}: ReportActionPushNotificationData): Promise { diff --git a/src/libs/actions/applyOnyxUpdatesReliably.ts b/src/libs/actions/applyOnyxUpdatesReliably.ts index f1ac4d8dec48..89ec7e04d936 100644 --- a/src/libs/actions/applyOnyxUpdatesReliably.ts +++ b/src/libs/actions/applyOnyxUpdatesReliably.ts @@ -18,7 +18,7 @@ type ApplyOnyxUpdatesReliablyOptions = FetchMissingUpdatesIds & { */ export default function applyOnyxUpdatesReliably( updates: OnyxUpdatesFromServer, - {shouldRunSync = false, clientLastUpdateID, shouldFetchPendingUpdates: shouldFetchPendingUpdatesProp}: ApplyOnyxUpdatesReliablyOptions = {}, + {shouldRunSync = false, clientLastUpdateID, shouldFetchPendingUpdates: shouldFetchPendingUpdatesProp = false}: ApplyOnyxUpdatesReliablyOptions = {}, ) { const fetchMissingUpdates = (shouldFetchPendingUpdates = false) => { if (shouldRunSync) { From 6cb1d69a972d65dc9a296b8d905ce133da908df1 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 14:16:27 +0100 Subject: [PATCH 03/16] fix: previousUpdateID is undefined if flag is set --- .../subscribePushNotification/index.ts | 5 ++--- src/types/onyx/OnyxUpdatesFromServer.ts | 20 ++++++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index 06cca15a0def..d6423564acc5 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -48,7 +48,7 @@ function getLastUpdateIDAppliedToClient(): Promise { }); } -function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previousUpdateID, hasPendingOnyxUpdates}: ReportActionPushNotificationData): Promise { +function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previousUpdateID, hasPendingOnyxUpdates = false}: ReportActionPushNotificationData): Promise { Log.info(`[PushNotification] Applying onyx data in the ${Visibility.isVisible() ? 'foreground' : 'background'}`, false, {reportID, reportActionID}); if (!ActiveClientManager.isClientTheLeader()) { @@ -56,12 +56,11 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo return Promise.resolve(); } - const shouldFetchPendingUpdates = hasPendingOnyxUpdates && !!lastUpdateID && !!previousUpdateID; + const shouldFetchPendingUpdates = hasPendingOnyxUpdates && !!lastUpdateID; if (shouldFetchPendingUpdates) { const updates: OnyxUpdatesFromServer = { type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, lastUpdateID, - previousUpdateID, shouldFetchPendingUpdates: true, }; diff --git a/src/types/onyx/OnyxUpdatesFromServer.ts b/src/types/onyx/OnyxUpdatesFromServer.ts index 0de3c975eefb..e73281a04cb1 100644 --- a/src/types/onyx/OnyxUpdatesFromServer.ts +++ b/src/types/onyx/OnyxUpdatesFromServer.ts @@ -29,12 +29,6 @@ type OnyxUpdatesFromServer = { /** Last update ID from server */ lastUpdateID: number | string; - /** Previous update ID from server */ - previousUpdateID: number | string; - - /** Whether the client should fetch pending updates from the server */ - shouldFetchPendingUpdates?: boolean; - /** Request data sent to the server */ request?: Request; @@ -43,7 +37,19 @@ type OnyxUpdatesFromServer = { /** Collection of onyx updates */ updates?: OnyxUpdateEvent[]; -}; +} & ( + | { + /** Whether the client should fetch pending updates from the server */ + shouldFetchPendingUpdates?: false; + + /** Previous update ID from server */ + previousUpdateID: number | string; + } + | { + /** Whether the client should fetch pending updates from the server */ + shouldFetchPendingUpdates: true; + } +); /** * Helper function to determine if onyx update received from server is valid From 9c3381d314a2d84321e9231e54ba49907c1ce2a8 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 15:23:49 +0100 Subject: [PATCH 04/16] fix: re-write pending updates logic --- .../subscribePushNotification/index.ts | 63 ++++++++++++------- src/libs/actions/OnyxUpdateManager/index.ts | 15 +---- src/libs/actions/applyOnyxUpdatesReliably.ts | 16 ++--- src/types/onyx/OnyxUpdatesFromServer.ts | 20 +++--- 4 files changed, 55 insertions(+), 59 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index d6423564acc5..b74c366be114 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -56,38 +56,53 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo return Promise.resolve(); } - const shouldFetchPendingUpdates = hasPendingOnyxUpdates && !!lastUpdateID; + // const shouldFetchPendingUpdates = hasPendingOnyxUpdates && !!lastUpdateID; + const shouldFetchPendingUpdates = true; + lastUpdateID = 1234567890123; + + const logMissingOnyxDataInfo = (isDataMissing: boolean): boolean => { + if (isDataMissing) { + Log.hmmm("[PushNotification] didn't apply onyx updates because some data is missing", {lastUpdateID, previousUpdateID, onyxDataCount: onyxData?.length ?? 0}); + return false; + } + + Log.info('[PushNotification] reliable onyx update received', false, {lastUpdateID, previousUpdateID, onyxDataCount: onyxData?.length ?? 0}); + return true; + }; + + let updates: OnyxUpdatesFromServer; if (shouldFetchPendingUpdates) { - const updates: OnyxUpdatesFromServer = { + const isDataMissing = !lastUpdateID; + logMissingOnyxDataInfo(isDataMissing); + if (isDataMissing) { + return Promise.resolve(); + } + + updates = { type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, lastUpdateID, shouldFetchPendingUpdates: true, }; + } else { + const isDataMissing = !lastUpdateID || !onyxData || !previousUpdateID; + logMissingOnyxDataInfo(isDataMissing); + if (isDataMissing) { + return Promise.resolve(); + } - return getLastUpdateIDAppliedToClient().then((lastUpdateIDAppliedToClient) => - applyOnyxUpdatesReliably(updates, {shouldRunSync: true, clientLastUpdateID: lastUpdateIDAppliedToClient, shouldFetchPendingUpdates: true}), - ); - } - - const isDataMissing = !onyxData || !previousUpdateID || !lastUpdateID; - if (isDataMissing) { - Log.hmmm("[PushNotification] didn't apply onyx updates because some data is missing", {lastUpdateID, previousUpdateID, onyxDataCount: onyxData?.length ?? 0}); - return Promise.resolve(); + updates = { + type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, + lastUpdateID, + previousUpdateID, + updates: [ + { + eventType: '', // This is only needed for Pusher events + data: onyxData, + }, + ], + }; } - Log.info('[PushNotification] reliable onyx update received', false, {lastUpdateID, previousUpdateID, onyxDataCount: onyxData?.length ?? 0}); - const updates: OnyxUpdatesFromServer = { - type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, - lastUpdateID, - previousUpdateID, - updates: [ - { - eventType: '', // This is only needed for Pusher events - data: onyxData, - }, - ], - }; - /** * When this callback runs in the background on Android (via Headless JS), no other Onyx.connect callbacks will run. This means that * lastUpdateIDAppliedToClient will NOT be populated in other libs. To workaround this, we manually read the value here diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 0d88ffb001d5..7651e03a9033 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -63,22 +63,14 @@ function finalizeUpdatesAndResumeQueue() { DeferredOnyxUpdates.clear(); } -type FetchMissingUpdatesIds = { - clientLastUpdateID?: number; - shouldFetchPendingUpdates?: boolean; -}; - /** * * @param onyxUpdatesFromServer * @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient * @returns */ -function handleOnyxUpdateGap( - onyxUpdatesFromServer: OnyxEntry, - {clientLastUpdateID, shouldFetchPendingUpdates: shouldFetchPendingUpdatesProp}: FetchMissingUpdatesIds = {}, -) { - const shouldFetchPendingUpdates = shouldFetchPendingUpdatesProp ?? onyxUpdatesFromServer?.shouldFetchPendingUpdates; +function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { + const shouldFetchPendingUpdates = onyxUpdatesFromServer?.shouldFetchPendingUpdates ?? false; // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, @@ -111,8 +103,8 @@ function handleOnyxUpdateGap( // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const updateParams = onyxUpdatesFromServer!; const lastUpdateIDFromServer = updateParams.lastUpdateID; - const previousUpdateIDFromServer = updateParams.previousUpdateID; const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; + const previousUpdateIDFromServer = shouldFetchPendingUpdates ? lastUpdateIDFromClient : updateParams.previousUpdateID; // In cases where we received a previousUpdateID and it doesn't match our lastUpdateIDAppliedToClient // we need to perform one of the 2 possible cases: @@ -195,4 +187,3 @@ export default () => { }; export {handleOnyxUpdateGap, queryPromiseWrapper as queryPromise, resetDeferralLogicVariables}; -export type {FetchMissingUpdatesIds}; diff --git a/src/libs/actions/applyOnyxUpdatesReliably.ts b/src/libs/actions/applyOnyxUpdatesReliably.ts index 89ec7e04d936..aaae54d47dc7 100644 --- a/src/libs/actions/applyOnyxUpdatesReliably.ts +++ b/src/libs/actions/applyOnyxUpdatesReliably.ts @@ -1,9 +1,9 @@ import type {OnyxUpdatesFromServer} from '@src/types/onyx'; -import type {FetchMissingUpdatesIds} from './OnyxUpdateManager'; import {handleOnyxUpdateGap} from './OnyxUpdateManager'; import * as OnyxUpdates from './OnyxUpdates'; -type ApplyOnyxUpdatesReliablyOptions = FetchMissingUpdatesIds & { +type ApplyOnyxUpdatesReliablyOptions = { + clientLastUpdateID?: number; shouldRunSync?: boolean; }; @@ -16,13 +16,10 @@ type ApplyOnyxUpdatesReliablyOptions = FetchMissingUpdatesIds & { * @param shouldRunSync * @returns */ -export default function applyOnyxUpdatesReliably( - updates: OnyxUpdatesFromServer, - {shouldRunSync = false, clientLastUpdateID, shouldFetchPendingUpdates: shouldFetchPendingUpdatesProp = false}: ApplyOnyxUpdatesReliablyOptions = {}, -) { +export default function applyOnyxUpdatesReliably(updates: OnyxUpdatesFromServer, {shouldRunSync = false, clientLastUpdateID}: ApplyOnyxUpdatesReliablyOptions = {}) { const fetchMissingUpdates = (shouldFetchPendingUpdates = false) => { if (shouldRunSync) { - handleOnyxUpdateGap(updates, {clientLastUpdateID, shouldFetchPendingUpdates}); + handleOnyxUpdateGap({...updates, shouldFetchPendingUpdates}, clientLastUpdateID); } else { OnyxUpdates.saveUpdateInformation({...updates, shouldFetchPendingUpdates}); } @@ -30,9 +27,8 @@ export default function applyOnyxUpdatesReliably( // If a pendingLastUpdateID is was provided, it means that the backend didn't send updates because the payload was too big. // In this case, we need to fetch the missing updates up to the pendingLastUpdateID. - const shouldFetchPendingUpdates = shouldFetchPendingUpdatesProp && updates == null; - if (shouldFetchPendingUpdatesProp) { - return fetchMissingUpdates(shouldFetchPendingUpdates); + if (updates.shouldFetchPendingUpdates) { + return fetchMissingUpdates(true); } const previousUpdateID = Number(updates.previousUpdateID) || 0; diff --git a/src/types/onyx/OnyxUpdatesFromServer.ts b/src/types/onyx/OnyxUpdatesFromServer.ts index e73281a04cb1..ef82ab013e39 100644 --- a/src/types/onyx/OnyxUpdatesFromServer.ts +++ b/src/types/onyx/OnyxUpdatesFromServer.ts @@ -29,6 +29,12 @@ type OnyxUpdatesFromServer = { /** Last update ID from server */ lastUpdateID: number | string; + /** Previous update ID from server */ + previousUpdateID?: number | string; + + /** Whether the client should fetch pending updates from the server */ + shouldFetchPendingUpdates?: boolean; + /** Request data sent to the server */ request?: Request; @@ -37,19 +43,7 @@ type OnyxUpdatesFromServer = { /** Collection of onyx updates */ updates?: OnyxUpdateEvent[]; -} & ( - | { - /** Whether the client should fetch pending updates from the server */ - shouldFetchPendingUpdates?: false; - - /** Previous update ID from server */ - previousUpdateID: number | string; - } - | { - /** Whether the client should fetch pending updates from the server */ - shouldFetchPendingUpdates: true; - } -); +}; /** * Helper function to determine if onyx update received from server is valid From db2c134b5883af45d7ca209014b451494567ca40 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 15:47:06 +0100 Subject: [PATCH 05/16] fix: remove debug code --- .../PushNotification/subscribePushNotification/index.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index b74c366be114..a956792837cd 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -56,10 +56,6 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo return Promise.resolve(); } - // const shouldFetchPendingUpdates = hasPendingOnyxUpdates && !!lastUpdateID; - const shouldFetchPendingUpdates = true; - lastUpdateID = 1234567890123; - const logMissingOnyxDataInfo = (isDataMissing: boolean): boolean => { if (isDataMissing) { Log.hmmm("[PushNotification] didn't apply onyx updates because some data is missing", {lastUpdateID, previousUpdateID, onyxDataCount: onyxData?.length ?? 0}); @@ -71,7 +67,7 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo }; let updates: OnyxUpdatesFromServer; - if (shouldFetchPendingUpdates) { + if (hasPendingOnyxUpdates) { const isDataMissing = !lastUpdateID; logMissingOnyxDataInfo(isDataMissing); if (isDataMissing) { From 4b6958b223698fa9ae2324c4af74c33a2eab547b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 17:54:49 +0100 Subject: [PATCH 06/16] fix: re-write handleOnyxUpdateGap flow --- src/libs/actions/OnyxUpdateManager/index.ts | 68 ++++++++++++-------- src/libs/actions/applyOnyxUpdatesReliably.ts | 4 +- tests/unit/OnyxUpdateManagerTest.ts | 42 ++++++------ 3 files changed, 64 insertions(+), 50 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 7651e03a9033..5848e2498666 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -64,12 +64,12 @@ function finalizeUpdatesAndResumeQueue() { } /** - * - * @param onyxUpdatesFromServer + * Triggers the fetching process of either pending or missing updates. + * @param onyxUpdatesFromServer the current update that is supposed to be applied * @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient * @returns */ -function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { +function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { const shouldFetchPendingUpdates = onyxUpdatesFromServer?.shouldFetchPendingUpdates ?? false; // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case @@ -89,12 +89,7 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), + ); + } else if (!lastUpdateIDFromClient) { + // This is the first time we're receiving an lastUpdateID, so we need to do a final ReconnectApp query before + // This flow is setting the promise to a ReconnectApp query. + // If there is a ReconnectApp query in progress, we should not start another one. if (DeferredOnyxUpdates.getMissingOnyxUpdatesQueryPromise()) { return; @@ -126,7 +134,13 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry { console.debug('[OnyxUpdateManager] Listening for updates from the server'); Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_FROM_SERVER, - callback: (value) => handleOnyxUpdateGap(value), + callback: (value) => handleMissingOnyxUpdates(value), }); }; -export {handleOnyxUpdateGap, queryPromiseWrapper as queryPromise, resetDeferralLogicVariables}; +export {handleMissingOnyxUpdates, queryPromiseWrapper as queryPromise, resetDeferralLogicVariables}; diff --git a/src/libs/actions/applyOnyxUpdatesReliably.ts b/src/libs/actions/applyOnyxUpdatesReliably.ts index aaae54d47dc7..5bf54b92b4c3 100644 --- a/src/libs/actions/applyOnyxUpdatesReliably.ts +++ b/src/libs/actions/applyOnyxUpdatesReliably.ts @@ -1,5 +1,5 @@ import type {OnyxUpdatesFromServer} from '@src/types/onyx'; -import {handleOnyxUpdateGap} from './OnyxUpdateManager'; +import {handleMissingOnyxUpdates} from './OnyxUpdateManager'; import * as OnyxUpdates from './OnyxUpdates'; type ApplyOnyxUpdatesReliablyOptions = { @@ -19,7 +19,7 @@ type ApplyOnyxUpdatesReliablyOptions = { export default function applyOnyxUpdatesReliably(updates: OnyxUpdatesFromServer, {shouldRunSync = false, clientLastUpdateID}: ApplyOnyxUpdatesReliablyOptions = {}) { const fetchMissingUpdates = (shouldFetchPendingUpdates = false) => { if (shouldRunSync) { - handleOnyxUpdateGap({...updates, shouldFetchPendingUpdates}, clientLastUpdateID); + handleMissingOnyxUpdates({...updates, shouldFetchPendingUpdates}, clientLastUpdateID); } else { OnyxUpdates.saveUpdateInformation({...updates, shouldFetchPendingUpdates}); } diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 7b0446641458..c106e992c292 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -49,9 +49,9 @@ describe('OnyxUpdateManager', () => { }); it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. @@ -75,9 +75,9 @@ describe('OnyxUpdateManager', () => { }); it('should only apply deferred updates that are newer than the last locally applied update (pending updates)', async () => { - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, @@ -113,9 +113,9 @@ describe('OnyxUpdateManager', () => { }); it('should re-fetch missing updates if the deferred updates have a gap', async () => { - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. @@ -148,10 +148,10 @@ describe('OnyxUpdateManager', () => { }); it('should re-fetch missing deferred updates only once per batch', async () => { - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate8); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate8); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. @@ -179,10 +179,10 @@ describe('OnyxUpdateManager', () => { }); it('should not re-fetch missing updates if the lastUpdateIDFromClient has been updated', async () => { - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate7); ApplyUpdates.mockValues.onApplyUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, @@ -220,8 +220,8 @@ describe('OnyxUpdateManager', () => { }); it('should re-fetch missing updates if the lastUpdateIDFromClient has increased, but there are still gaps after the locally applied update', async () => { - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate7); ApplyUpdates.mockValues.onApplyUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 4, to simulate local updates being applied, @@ -263,8 +263,8 @@ describe('OnyxUpdateManager', () => { }); it('should only fetch missing updates that are not outdated (older than already locally applied update)', () => { - OnyxUpdateManager.handleOnyxUpdateGap(offsetedMockUpdate3); - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); + OnyxUpdateManager.handleMissingOnyxUpdates(offsetedMockUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 4. From 523ecaa6a8f3a9751fb82519537d7c111be6ce72 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 18:39:36 +0100 Subject: [PATCH 07/16] fix: lint and TS and re-write --- src/libs/Middleware/SaveResponseInOnyx.ts | 2 +- .../subscribePushNotification/index.ts | 1 + src/libs/actions/OnyxUpdateManager/index.ts | 26 ++++++++----------- src/libs/actions/OnyxUpdates.ts | 6 ++--- src/libs/actions/User.ts | 2 +- 5 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/libs/Middleware/SaveResponseInOnyx.ts b/src/libs/Middleware/SaveResponseInOnyx.ts index 85f95c146dac..e5c129e5bccf 100644 --- a/src/libs/Middleware/SaveResponseInOnyx.ts +++ b/src/libs/Middleware/SaveResponseInOnyx.ts @@ -32,7 +32,7 @@ const SaveResponseInOnyx: Middleware = (requestResponse, request) => response: response ?? {}, }; - if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response?.previousUpdateID ?? 0))) { + if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: Number(response?.previousUpdateID ?? 0)})) { return OnyxUpdates.apply(responseToApply); } diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index a956792837cd..6d5dbab151a9 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -78,6 +78,7 @@ function applyOnyxData({reportID, reportActionID, onyxData, lastUpdateID, previo type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, lastUpdateID, shouldFetchPendingUpdates: true, + updates: [], }; } else { const isDataMissing = !lastUpdateID || !onyxData || !previousUpdateID; diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 5848e2498666..0dbdaa0fb0f7 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -89,16 +89,17 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry Date: Wed, 18 Dec 2024 18:49:26 +0100 Subject: [PATCH 08/16] fix: use default number id from CONST --- src/libs/Middleware/SaveResponseInOnyx.ts | 4 ++-- .../PushNotification/subscribePushNotification/index.ts | 2 +- src/libs/actions/OnyxUpdateManager/index.ts | 7 ++++--- .../OnyxUpdateManager/utils/DeferredOnyxUpdates.ts | 4 +++- src/libs/actions/OnyxUpdateManager/utils/index.ts | 9 +++++---- src/libs/actions/User.ts | 4 ++-- 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/libs/Middleware/SaveResponseInOnyx.ts b/src/libs/Middleware/SaveResponseInOnyx.ts index e5c129e5bccf..0eb7144e50a8 100644 --- a/src/libs/Middleware/SaveResponseInOnyx.ts +++ b/src/libs/Middleware/SaveResponseInOnyx.ts @@ -26,8 +26,8 @@ const SaveResponseInOnyx: Middleware = (requestResponse, request) => const responseToApply = { type: CONST.ONYX_UPDATE_TYPES.HTTPS, - lastUpdateID: Number(response?.lastUpdateID ?? 0), - previousUpdateID: Number(response?.previousUpdateID ?? 0), + lastUpdateID: Number(response?.lastUpdateID ?? CONST.DEFAULT_NUMBER_ID), + previousUpdateID: Number(response?.previousUpdateID ?? CONST.DEFAULT_NUMBER_ID), request, response: response ?? {}, }; diff --git a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts index 6d5dbab151a9..237a615b570a 100644 --- a/src/libs/Notification/PushNotification/subscribePushNotification/index.ts +++ b/src/libs/Notification/PushNotification/subscribePushNotification/index.ts @@ -43,7 +43,7 @@ function getLastUpdateIDAppliedToClient(): Promise { return new Promise((resolve) => { Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => resolve(value ?? 0), + callback: (value) => resolve(value ?? CONST.DEFAULT_NUMBER_ID), }); }); } diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 0dbdaa0fb0f7..53d0bb982cfb 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -6,6 +6,7 @@ import * as NetworkStore from '@libs/Network/NetworkStore'; import * as SequentialQueue from '@libs/Network/SequentialQueue'; import * as App from '@userActions/App'; import updateSessionAuthTokens from '@userActions/Session/updateSessionAuthTokens'; +import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer, Session} from '@src/types/onyx'; import {isValidOnyxUpdateFromServer} from '@src/types/onyx/OnyxUpdatesFromServer'; @@ -27,10 +28,10 @@ import * as DeferredOnyxUpdates from './utils/DeferredOnyxUpdates'; // The circular dependency happens because this file calls API.GetMissingOnyxUpdates() which uses the SaveResponseInOnyx.js file (as a middleware). // Therefore, SaveResponseInOnyx.js can't import and use this file directly. -let lastUpdateIDAppliedToClient = 0; +let lastUpdateIDAppliedToClient: number = CONST.DEFAULT_NUMBER_ID; Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), + callback: (value) => (lastUpdateIDAppliedToClient = value ?? CONST.DEFAULT_NUMBER_ID), }); let isLoadingApp = false; @@ -100,7 +101,7 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry | undefined; @@ -40,7 +42,7 @@ function getUpdates(options?: GetDeferredOnyxUpdatesOptiosn) { } return Object.entries(deferredUpdates).reduce((acc, [lastUpdateID, update]) => { - if (Number(lastUpdateID) > (options.minUpdateID ?? 0)) { + if (Number(lastUpdateID) > (options.minUpdateID ?? CONST.DEFAULT_NUMBER_ID)) { acc[Number(lastUpdateID)] = update; } return acc; diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index 9a527308034e..d41064a4ab3a 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -2,15 +2,16 @@ import Onyx from 'react-native-onyx'; import Log from '@libs/Log'; import * as App from '@userActions/App'; import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@userActions/OnyxUpdateManager/types'; +import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import {applyUpdates} from './applyUpdates'; // eslint-disable-next-line import/no-cycle import * as DeferredOnyxUpdates from './DeferredOnyxUpdates'; -let lastUpdateIDAppliedToClient = 0; +let lastUpdateIDAppliedToClient: number = CONST.DEFAULT_NUMBER_ID; Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), + callback: (value) => (lastUpdateIDAppliedToClient = value ?? CONST.DEFAULT_NUMBER_ID), }); /** @@ -114,7 +115,7 @@ function detectGapsAndSplit(lastUpdateIDFromClient: number): DetectGapAndSplitRe * apply the updates in order after the missing updates are fetched and applied */ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousParams?: {newLastUpdateIDFromClient: number; latestMissingUpdateID: number}): Promise { - const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; + const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? CONST.DEFAULT_NUMBER_ID; Log.info('[DeferredUpdates] Processing deferred updates', false, {lastUpdateIDFromClient, previousParams}); @@ -140,7 +141,7 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, // as long as there was no new gap detected. Otherwise repeat the process. - const newLastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; + const newLastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? CONST.DEFAULT_NUMBER_ID; DeferredOnyxUpdates.enqueue(updatesAfterGaps, {shouldPauseSequentialQueue: false}); diff --git a/src/libs/actions/User.ts b/src/libs/actions/User.ts index 3af970ec730c..1fee3f748e3e 100644 --- a/src/libs/actions/User.ts +++ b/src/libs/actions/User.ts @@ -911,9 +911,9 @@ function subscribeToUserEvents() { // Example: {lastUpdateID: 1, previousUpdateID: 0, updates: [{onyxMethod: 'whatever', key: 'foo', value: 'bar'}]} const updates = { type: CONST.ONYX_UPDATE_TYPES.PUSHER, - lastUpdateID: Number(pushJSON.lastUpdateID || 0), + lastUpdateID: Number(pushJSON.lastUpdateID ?? CONST.DEFAULT_NUMBER_ID), updates: pushJSON.updates ?? [], - previousUpdateID: Number(pushJSON.previousUpdateID ?? 0), + previousUpdateID: Number(pushJSON.previousUpdateID ?? CONST.DEFAULT_NUMBER_ID), }; applyOnyxUpdatesReliably(updates); }); From 148de1f261b4dae995bceb3cdf77793c385ad0e0 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 18:56:57 +0100 Subject: [PATCH 09/16] fix: don't always apply shouldFetchPendingUpdates prop --- src/libs/actions/OnyxUpdateManager/index.ts | 3 +-- src/libs/actions/applyOnyxUpdatesReliably.ts | 12 +++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 53d0bb982cfb..89f86a6e8e6e 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -71,8 +71,6 @@ function finalizeUpdatesAndResumeQueue() { * @returns */ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { - const shouldFetchPendingUpdates = onyxUpdatesFromServer?.shouldFetchPendingUpdates ?? false; - // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, // we'll only have them overriten by the openApp response. So let's skip it and return. @@ -99,6 +97,7 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry { + const fetchMissingUpdates = () => { if (shouldRunSync) { - handleMissingOnyxUpdates({...updates, shouldFetchPendingUpdates}, clientLastUpdateID); + handleMissingOnyxUpdates(updates, clientLastUpdateID); } else { - OnyxUpdates.saveUpdateInformation({...updates, shouldFetchPendingUpdates}); + OnyxUpdates.saveUpdateInformation(updates); } }; // If a pendingLastUpdateID is was provided, it means that the backend didn't send updates because the payload was too big. // In this case, we need to fetch the missing updates up to the pendingLastUpdateID. if (updates.shouldFetchPendingUpdates) { - return fetchMissingUpdates(true); + fetchMissingUpdates(); + return; } - const previousUpdateID = Number(updates.previousUpdateID) || 0; + const previousUpdateID = Number(updates.previousUpdateID) ?? CONST.DEFAULT_NUMBER_ID; if (!OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID})) { OnyxUpdates.apply(updates); return; From 12982cf2486cb55285045c9ddd7c87af97fe99da Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 19:07:52 +0100 Subject: [PATCH 10/16] fix: lint and ts --- src/libs/actions/OnyxUpdateManager/utils/DeferredOnyxUpdates.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/DeferredOnyxUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/DeferredOnyxUpdates.ts index ac17b7c7e2cc..8b3bf5d9af86 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/DeferredOnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/DeferredOnyxUpdates.ts @@ -6,7 +6,6 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; import {isValidOnyxUpdateFromServer} from '@src/types/onyx/OnyxUpdatesFromServer'; // eslint-disable-next-line import/no-cycle -import CONST from '@src/CONST'; import * as OnyxUpdateManagerUtils from '.'; let missingOnyxUpdatesQueryPromise: Promise | undefined; From a681d0b0068239ae89f156b896507888e451fcc8 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 18 Dec 2024 19:15:05 +0100 Subject: [PATCH 11/16] fix: more ESLint changed files workflow issues --- src/libs/Middleware/SaveResponseInOnyx.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libs/Middleware/SaveResponseInOnyx.ts b/src/libs/Middleware/SaveResponseInOnyx.ts index 0eb7144e50a8..b3baf278a477 100644 --- a/src/libs/Middleware/SaveResponseInOnyx.ts +++ b/src/libs/Middleware/SaveResponseInOnyx.ts @@ -32,7 +32,10 @@ const SaveResponseInOnyx: Middleware = (requestResponse, request) => response: response ?? {}, }; - if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: Number(response?.previousUpdateID ?? 0)})) { + if ( + requestsToIgnoreLastUpdateID.includes(request.command) || + !OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: Number(response?.previousUpdateID ?? CONST.DEFAULT_NUMBER_ID)}) + ) { return OnyxUpdates.apply(responseToApply); } From 9dc03810711ab3c8b8fc1d55ee7414251ca18b1e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 19 Dec 2024 12:30:06 +0100 Subject: [PATCH 12/16] fix: deprecated code about ids --- src/libs/actions/User.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/User.ts b/src/libs/actions/User.ts index 1fee3f748e3e..2d56f8a0650a 100644 --- a/src/libs/actions/User.ts +++ b/src/libs/actions/User.ts @@ -57,7 +57,7 @@ let currentEmail = ''; Onyx.connect({ key: ONYXKEYS.SESSION, callback: (value) => { - currentUserAccountID = value?.accountID ?? -1; + currentUserAccountID = value?.accountID ?? CONST.DEFAULT_NUMBER_ID; currentEmail = value?.email ?? ''; }, }); From c0cdfb442508ceac3aa65343e72c70d4e3aac931 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 7 Jan 2025 10:39:32 +0100 Subject: [PATCH 13/16] refactor: early-return if no pending updates and refactor handleMissingOnyxUpdates code --- src/libs/actions/OnyxUpdateManager/index.ts | 130 +++++++++++--------- 1 file changed, 73 insertions(+), 57 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 89f86a6e8e6e..a29744f127a8 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -102,67 +102,83 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), - ); - } else if (!lastUpdateIDFromClient) { - // This is the first time we're receiving an lastUpdateID, so we need to do a final ReconnectApp query before - // This flow is setting the promise to a ReconnectApp query. - - // If there is a ReconnectApp query in progress, we should not start another one. - if (DeferredOnyxUpdates.getMissingOnyxUpdatesQueryPromise()) { - return; + // Check if the client needs to send a backend request to fetch missing or pending updates and/or queue deferred updates. + // Returns a boolean indicating whether we should execute the finally block after the promise is done, + // in which the OnyxUpdateManager finishes its work and the SequentialQueue will is unpaused. + const checkIfClientNeedsToBeUpdated = (): boolean => { + // The OnyxUpdateManager can handle different types of re-fetch processes. Either there are pending updates, + // that we need to fetch manually or we detected gaps in the previously fetched updates. + // Each of the flows below sets a promise through `DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise`, which we further process. + if (shouldFetchPendingUpdates) { + // This flow handles the case where the server didn't send updates because the payload was too big. + // We need to call the GetMissingOnyxUpdates query to fetch the missing updates up to the pendingLastUpdateID. + const pendingUpdateID = Number(lastUpdateIDFromServer); + + // If the pendingUpdateID is not newer than the last locally applied update, we don't need to fetch the missing updates. + if (pendingUpdateID <= lastUpdateIDFromClient) { + DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise(Promise.resolve()); + return true; + } + + console.debug(`[OnyxUpdateManager] Client is fetching pending updates from the server, from updates ${lastUpdateIDFromClient} to ${Number(pendingUpdateID)}`); + Log.info('There are pending updates from the server, so fetching incremental updates', true, { + pendingUpdateID, + lastUpdateIDFromClient, + }); + + // Get the missing Onyx updates from the server and afterwards validate and apply the deferred updates. + // This will trigger recursive calls to "validateAndApplyDeferredUpdates" if there are gaps in the deferred updates. + DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise( + App.getMissingOnyxUpdates(lastUpdateIDFromClient, lastUpdateIDFromServer).then(() => OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), + ); + } else if (!lastUpdateIDFromClient) { + // This is the first time we're receiving an lastUpdateID, so we need to do a final ReconnectApp query before + // This flow is setting the promise to a ReconnectApp query. + + // If there is a ReconnectApp query in progress, we should not start another one. + if (DeferredOnyxUpdates.getMissingOnyxUpdatesQueryPromise()) { + return false; + } + + Log.info('Client has not gotten reliable updates before so reconnecting the app to start the process'); + + // Since this is a full reconnectApp, we'll not apply the updates we received - those will come in the reconnect app request. + DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise(App.finalReconnectAppAfterActivatingReliableUpdates()); + } else { + // This client already has the reliable updates mode enabled, but it's missing some updates and it needs to fetch those. + // Therefore, we are calling the GetMissingOnyxUpdates query, to fetch the missing updates. + + const areDeferredUpdatesQueued = !DeferredOnyxUpdates.isEmpty(); + + // Add the new update to the deferred updates + DeferredOnyxUpdates.enqueue(onyxUpdatesFromServer, {shouldPauseSequentialQueue: false}); + + // If there are deferred updates already, we don't need to fetch the missing updates again. + if (areDeferredUpdatesQueued) { + return false; + } + + console.debug(`[OnyxUpdateManager] Client is fetching missing updates from the server, from updates ${lastUpdateIDFromClient} to ${Number(previousUpdateIDFromServer)}`); + Log.info('Gap detected in update IDs from the server so fetching incremental updates', true, { + lastUpdateIDFromClient, + lastUpdateIDFromServer, + previousUpdateIDFromServer, + }); + + // Get the missing Onyx updates from the server and afterwards validate and apply the deferred updates. + // This will trigger recursive calls to "validateAndApplyDeferredUpdates" if there are gaps in the deferred updates. + DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise( + App.getMissingOnyxUpdates(lastUpdateIDFromClient, previousUpdateIDFromServer).then(() => OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), + ); } - Log.info('Client has not gotten reliable updates before so reconnecting the app to start the process'); - - // Since this is a full reconnectApp, we'll not apply the updates we received - those will come in the reconnect app request. - DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise(App.finalReconnectAppAfterActivatingReliableUpdates()); - } else { - // This client already has the reliable updates mode enabled, but it's missing some updates and it needs to fetch those. - // Therefore, we are calling the GetMissingOnyxUpdates query, to fetch the missing updates. - - const areDeferredUpdatesQueued = !DeferredOnyxUpdates.isEmpty(); - - // Add the new update to the deferred updates - DeferredOnyxUpdates.enqueue(onyxUpdatesFromServer, {shouldPauseSequentialQueue: false}); + return true; + }; + const shouldFinalizeAndResume = checkIfClientNeedsToBeUpdated(); - // If there are deferred updates already, we don't need to fetch the missing updates again. - if (areDeferredUpdatesQueued) { - return; - } - - console.debug(`[OnyxUpdateManager] Client is fetching missing updates from the server, from updates ${lastUpdateIDFromClient} to ${Number(previousUpdateIDFromServer)}`); - Log.info('Gap detected in update IDs from the server so fetching incremental updates', true, { - lastUpdateIDFromClient, - lastUpdateIDFromServer, - previousUpdateIDFromServer, - }); - - // Get the missing Onyx updates from the server and afterwards validate and apply the deferred updates. - // This will trigger recursive calls to "validateAndApplyDeferredUpdates" if there are gaps in the deferred updates. - DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise( - App.getMissingOnyxUpdates(lastUpdateIDFromClient, previousUpdateIDFromServer).then(() => OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), - ); + if (shouldFinalizeAndResume) { + DeferredOnyxUpdates.getMissingOnyxUpdatesQueryPromise()?.finally(finalizeUpdatesAndResumeQueue); } - - DeferredOnyxUpdates.getMissingOnyxUpdatesQueryPromise()?.finally(finalizeUpdatesAndResumeQueue); } function updateAuthTokenIfNecessary(onyxUpdatesFromServer: OnyxEntry): void { From 4da1e574cbf583eb5e6ccc43f78146719fb9a32a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 7 Jan 2025 10:40:53 +0100 Subject: [PATCH 14/16] fix: always pause SequentialQueue, also if `shouldRunSync` is true --- src/libs/actions/OnyxUpdates.ts | 6 ------ src/libs/actions/applyOnyxUpdatesReliably.ts | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index 29bfd617d662..d2a78eb4a2bb 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -2,7 +2,6 @@ import type {OnyxUpdate} from 'react-native-onyx'; import Onyx from 'react-native-onyx'; import type {Merge} from 'type-fest'; import Log from '@libs/Log'; -import * as SequentialQueue from '@libs/Network/SequentialQueue'; import Performance from '@libs/Performance'; import PusherUtils from '@libs/PusherUtils'; import CONST from '@src/CONST'; @@ -154,11 +153,6 @@ function apply({lastUpdateID, type, request, response, updates}: OnyxUpdatesFrom * @param [updateParams.updates] Exists if updateParams.type === 'pusher' */ function saveUpdateInformation(updateParams: OnyxUpdatesFromServer) { - // If we got here, that means we are missing some updates on our local storage. To - // guarantee that we're not fetching more updates before our local data is up to date, - // let's stop the sequential queue from running until we're done catching up. - SequentialQueue.pause(); - // Always use set() here so that the updateParams are never merged and always unique to the request that came in Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, updateParams); } diff --git a/src/libs/actions/applyOnyxUpdatesReliably.ts b/src/libs/actions/applyOnyxUpdatesReliably.ts index 85dce5e53b02..d8475c55042a 100644 --- a/src/libs/actions/applyOnyxUpdatesReliably.ts +++ b/src/libs/actions/applyOnyxUpdatesReliably.ts @@ -1,3 +1,4 @@ +import * as SequentialQueue from '@libs/Network/SequentialQueue'; import CONST from '@src/CONST'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import {handleMissingOnyxUpdates} from './OnyxUpdateManager'; @@ -19,6 +20,11 @@ type ApplyOnyxUpdatesReliablyOptions = { */ export default function applyOnyxUpdatesReliably(updates: OnyxUpdatesFromServer, {shouldRunSync = false, clientLastUpdateID}: ApplyOnyxUpdatesReliablyOptions = {}) { const fetchMissingUpdates = () => { + // If we got here, that means we are missing some updates on our local storage. To + // guarantee that we're not fetching more updates before our local data is up to date, + // let's stop the sequential queue from running until we're done catching up. + SequentialQueue.pause(); + if (shouldRunSync) { handleMissingOnyxUpdates(updates, clientLastUpdateID); } else { From 8b8d0d17bdd623e0ba70e0eb5acb60f1faa3e9bb Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 7 Jan 2025 19:56:30 +0100 Subject: [PATCH 15/16] test: re-structure OnyxUpdateManager tests and add tests for pending updates --- src/libs/actions/OnyxUpdateManager/index.ts | 62 +++-- .../utils/__mocks__/applyUpdates.ts | 33 ++- .../utils/__mocks__/index.ts | 16 +- .../actions/OnyxUpdateManager/utils/index.ts | 6 +- src/libs/actions/__mocks__/App.ts | 34 ++- src/libs/actions/__mocks__/OnyxUpdates.ts | 40 +++ tests/actions/OnyxUpdateManagerTest.ts | 18 +- tests/unit/OnyxUpdateManagerTest.ts | 254 +++++++++++++----- tests/utils/OnyxUpdateMockUtils.ts | 36 +++ tests/utils/createOnyxMockUpdate.ts | 23 -- 10 files changed, 373 insertions(+), 149 deletions(-) create mode 100644 src/libs/actions/__mocks__/OnyxUpdates.ts create mode 100644 tests/utils/OnyxUpdateMockUtils.ts delete mode 100644 tests/utils/createOnyxMockUpdate.ts diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index a29744f127a8..dad3e4b15f35 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -49,6 +49,7 @@ const createQueryPromiseWrapper = () => }); // eslint-disable-next-line import/no-mutable-exports let queryPromiseWrapper = createQueryPromiseWrapper(); +let isFetchingForPendingUpdates = false; const resetDeferralLogicVariables = () => { DeferredOnyxUpdates.clear({shouldUnpauseSequentialQueue: false}); @@ -62,6 +63,7 @@ function finalizeUpdatesAndResumeQueue() { queryPromiseWrapper = createQueryPromiseWrapper(); DeferredOnyxUpdates.clear(); + isFetchingForPendingUpdates = false; } /** @@ -72,8 +74,8 @@ function finalizeUpdatesAndResumeQueue() { */ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case - // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, - // we'll only have them overriten by the openApp response. So let's skip it and return. + // we don't have base state of the app (reports, policies, etc.) setup. If we apply this update, + // we'll only have them overwritten by the openApp response. So let's skip it and return. if (isLoadingApp) { // When ONYX_UPDATES_FROM_SERVER is set, we pause the queue. Let's unpause // it so the app is not stuck forever without processing requests. @@ -107,13 +109,15 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry { // The OnyxUpdateManager can handle different types of re-fetch processes. Either there are pending updates, - // that we need to fetch manually or we detected gaps in the previously fetched updates. + // that we need to fetch manually, or we detected gaps in the previously fetched updates. // Each of the flows below sets a promise through `DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise`, which we further process. if (shouldFetchPendingUpdates) { // This flow handles the case where the server didn't send updates because the payload was too big. // We need to call the GetMissingOnyxUpdates query to fetch the missing updates up to the pendingLastUpdateID. const pendingUpdateID = Number(lastUpdateIDFromServer); + isFetchingForPendingUpdates = true; + // If the pendingUpdateID is not newer than the last locally applied update, we don't need to fetch the missing updates. if (pendingUpdateID <= lastUpdateIDFromClient) { DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise(Promise.resolve()); @@ -126,12 +130,16 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), ); - } else if (!lastUpdateIDFromClient) { + + return true; + } + + if (!lastUpdateIDFromClient) { // This is the first time we're receiving an lastUpdateID, so we need to do a final ReconnectApp query before // This flow is setting the promise to a ReconnectApp query. @@ -144,34 +152,36 @@ function handleMissingOnyxUpdates(onyxUpdatesFromServer: OnyxEntry OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), - ); + // If there are deferred updates already, we don't need to fetch the missing updates again. + if (areDeferredUpdatesQueued || isFetchingForPendingUpdates) { + return false; } + console.debug(`[OnyxUpdateManager] Client is fetching missing updates from the server, from updates ${lastUpdateIDFromClient} to ${Number(previousUpdateIDFromServer)}`); + Log.info('Gap detected in update IDs from the server so fetching incremental updates', true, { + lastUpdateIDFromClient, + lastUpdateIDFromServer, + previousUpdateIDFromServer, + }); + + // Get the missing Onyx updates from the server and afterwards validate and apply the deferred updates. + // This will trigger recursive calls to "validateAndApplyDeferredUpdates" if there are gaps in the deferred updates. + DeferredOnyxUpdates.setMissingOnyxUpdatesQueryPromise( + App.getMissingOnyxUpdates(lastUpdateIDFromClient, previousUpdateIDFromServer).then(() => OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)), + ); + return true; }; const shouldFinalizeAndResume = checkIfClientNeedsToBeUpdated(); diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts index 5cd66df6b0b0..019821c7f215 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -1,16 +1,11 @@ -import Onyx from 'react-native-onyx'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; -import ONYXKEYS from '@src/ONYXKEYS'; +import * as OnyxUpdates from '@userActions/OnyxUpdates'; import createProxyForObject from '@src/utils/createProxyForObject'; -let lastUpdateIDAppliedToClient = 0; -Onyx.connect({ - key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), -}); +jest.mock('@userActions/OnyxUpdates'); type ApplyUpdatesMockValues = { - onApplyUpdates: ((updates: DeferredUpdatesDictionary) => Promise) | undefined; + beforeApplyUpdates: ((updates: DeferredUpdatesDictionary) => Promise) | undefined; }; type ApplyUpdatesMock = { @@ -19,15 +14,27 @@ type ApplyUpdatesMock = { }; const mockValues: ApplyUpdatesMockValues = { - onApplyUpdates: undefined, + beforeApplyUpdates: undefined, }; const mockValuesProxy = createProxyForObject(mockValues); const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { - const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); - return (mockValuesProxy.onApplyUpdates === undefined ? Promise.resolve() : mockValuesProxy.onApplyUpdates(updates)).then(() => - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Math.max(lastUpdateIDAppliedToClient, lastUpdateIdFromUpdates)), - ); + const createChain = () => { + let chain = Promise.resolve(); + Object.values(updates).forEach((update) => { + chain = chain.then(() => { + return OnyxUpdates.apply(update).then(() => undefined); + }); + }); + + return chain; + }; + + if (mockValuesProxy.beforeApplyUpdates === undefined) { + return createChain(); + } + + return mockValuesProxy.beforeApplyUpdates(updates).then(() => createChain()); }); export {applyUpdates, mockValuesProxy as mockValues}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts index f66e059ff7f6..a10c265cf569 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts @@ -6,7 +6,7 @@ import {applyUpdates} from './applyUpdates'; const UtilsImplementation = jest.requireActual('@libs/actions/OnyxUpdateManager/utils'); type OnyxUpdateManagerUtilsMockValues = { - onValidateAndApplyDeferredUpdates: ((clientLastUpdateID?: number) => Promise) | undefined; + beforeValidateAndApplyDeferredUpdates: ((clientLastUpdateID?: number) => Promise) | undefined; }; type OnyxUpdateManagerUtilsMock = typeof UtilsImplementation & { @@ -16,17 +16,19 @@ type OnyxUpdateManagerUtilsMock = typeof UtilsImplementation & { }; const mockValues: OnyxUpdateManagerUtilsMockValues = { - onValidateAndApplyDeferredUpdates: undefined, + beforeValidateAndApplyDeferredUpdates: undefined, }; const mockValuesProxy = createProxyForObject(mockValues); const detectGapsAndSplit = jest.fn(UtilsImplementation.detectGapsAndSplit); -const validateAndApplyDeferredUpdates = jest.fn((clientLastUpdateID?: number) => - (mockValuesProxy.onValidateAndApplyDeferredUpdates === undefined ? Promise.resolve() : mockValuesProxy.onValidateAndApplyDeferredUpdates(clientLastUpdateID)).then(() => - UtilsImplementation.validateAndApplyDeferredUpdates(clientLastUpdateID), - ), -); +const validateAndApplyDeferredUpdates = jest.fn((clientLastUpdateID?: number) => { + if (mockValuesProxy.beforeValidateAndApplyDeferredUpdates === undefined) { + return UtilsImplementation.validateAndApplyDeferredUpdates(clientLastUpdateID); + } + + return mockValuesProxy.beforeValidateAndApplyDeferredUpdates(clientLastUpdateID).then(() => UtilsImplementation.validateAndApplyDeferredUpdates(clientLastUpdateID)); +}); export {applyUpdates, detectGapsAndSplit, validateAndApplyDeferredUpdates, mockValuesProxy as mockValues}; export type {OnyxUpdateManagerUtilsMock}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index d41064a4ab3a..bf9be862c029 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -121,7 +121,7 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(lastUpdateIDFromClient); - // If there are no applicable deferred updates and no missing deferred updates, + // If there are no applicably deferred updates and no missing deferred updates, // we don't need to apply or re-fetch any updates. We can just unpause the queue by resolving. if (Object.values(applicableUpdates).length === 0 && latestMissingUpdateID === undefined) { return Promise.resolve(); @@ -139,13 +139,13 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa // After we have applied the applicable updates, there might have been new deferred updates added. // In the next (recursive) call of "validateAndApplyDeferredUpdates", // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, - // as long as there was no new gap detected. Otherwise repeat the process. + // as long as there was no new gap detected. Otherwise, repeat the process. const newLastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? CONST.DEFAULT_NUMBER_ID; DeferredOnyxUpdates.enqueue(updatesAfterGaps, {shouldPauseSequentialQueue: false}); - // If lastUpdateIDAppliedToClient got updated, we will just retrigger the validation + // If lastUpdateIDAppliedToClient got updated, we will just re-trigger the validation // and application of the current deferred updates. if (latestMissingUpdateID <= newLastUpdateIDFromClient) { validateAndApplyDeferredUpdates(undefined, {newLastUpdateIDFromClient, latestMissingUpdateID}) diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts index 09fd553a87f3..0b2b098feefb 100644 --- a/src/libs/actions/__mocks__/App.ts +++ b/src/libs/actions/__mocks__/App.ts @@ -1,10 +1,11 @@ -import Onyx from 'react-native-onyx'; import type * as AppImport from '@libs/actions/App'; -import type * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; -import ONYXKEYS from '@src/ONYXKEYS'; +import * as OnyxUpdates from '@userActions/OnyxUpdates'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import createProxyForObject from '@src/utils/createProxyForObject'; +jest.mock('@libs/actions/OnyxUpdates'); +jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); + const AppImplementation = jest.requireActual('@libs/actions/App'); const { setLocale, @@ -39,13 +40,30 @@ const mockValues: AppMockValues = { }; const mockValuesProxy = createProxyForObject(mockValues); -const ApplyUpdatesImplementation = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); -const getMissingOnyxUpdates = jest.fn((_fromID: number, toID: number) => { - if (mockValuesProxy.missingOnyxUpdatesToBeApplied === undefined) { - return Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, toID); +const getMissingOnyxUpdates = jest.fn((updateIDFrom: number, updateIDTo: number) => { + const updates = mockValuesProxy.missingOnyxUpdatesToBeApplied ?? []; + if (updates.length === 0) { + for (let i = updateIDFrom + 1; i <= updateIDTo; i++) { + updates.push({ + lastUpdateID: i, + previousUpdateID: i - 1, + } as OnyxUpdatesFromServer); + } } - return ApplyUpdatesImplementation.applyUpdates(mockValuesProxy.missingOnyxUpdatesToBeApplied); + let chain = Promise.resolve(); + updates.forEach((update) => { + chain = chain.then(() => { + if (!OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: Number(update.previousUpdateID)})) { + return OnyxUpdates.apply(update).then(() => undefined); + } + + OnyxUpdates.saveUpdateInformation(update); + return Promise.resolve(); + }); + }); + + return chain; }); export { diff --git a/src/libs/actions/__mocks__/OnyxUpdates.ts b/src/libs/actions/__mocks__/OnyxUpdates.ts new file mode 100644 index 000000000000..0c7a9df4f6ab --- /dev/null +++ b/src/libs/actions/__mocks__/OnyxUpdates.ts @@ -0,0 +1,40 @@ +import Onyx from 'react-native-onyx'; +import type * as OnyxUpdatesImport from '@userActions/OnyxUpdates'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; + +jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); + +const OnyxUpdatesImplementation = jest.requireActual('@libs/actions/OnyxUpdates'); +const {doesClientNeedToBeUpdated, saveUpdateInformation} = OnyxUpdatesImplementation; + +type OnyxUpdatesMock = typeof OnyxUpdatesImport & { + apply: jest.Mock, [OnyxUpdatesFromServer]>; +}; + +let lastUpdateIDAppliedToClient: number | undefined = 0; +Onyx.connect({ + key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, + callback: (val) => (lastUpdateIDAppliedToClient = val), +}); + +const apply = jest.fn(({lastUpdateID}: OnyxUpdatesFromServer): Promise | undefined => { + if (lastUpdateID && (lastUpdateIDAppliedToClient === undefined || Number(lastUpdateID) > lastUpdateIDAppliedToClient)) { + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Number(lastUpdateID)); + } + + return Promise.resolve(); +}); + +export { + // Mocks + apply, + + // Actual OnyxUpdates implementation + doesClientNeedToBeUpdated, + saveUpdateInformation, +}; + +type ManualOnyxUpdateCheckIds = OnyxUpdatesImport.ManualOnyxUpdateCheckIds; +export type {ManualOnyxUpdateCheckIds}; +export type {OnyxUpdatesMock}; diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts index e1008064163b..3039d1999a79 100644 --- a/tests/actions/OnyxUpdateManagerTest.ts +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -15,7 +15,7 @@ import CONST from '@src/CONST'; import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager'; import ONYXKEYS from '@src/ONYXKEYS'; import type * as OnyxTypes from '@src/types/onyx'; -import createOnyxMockUpdate from '../utils/createOnyxMockUpdate'; +import OnyxUpdateMockUtils from '../utils/OnyxUpdateMockUtils'; jest.mock('@libs/actions/App'); jest.mock('@libs/actions/OnyxUpdateManager/utils'); @@ -53,14 +53,14 @@ const exampleReportAction = { const initialData = {report1: exampleReportAction, report2: exampleReportAction, report3: exampleReportAction} as unknown as OnyxTypes.ReportActions; -const mockUpdate1 = createOnyxMockUpdate(1, [ +const mockUpdate1 = OnyxUpdateMockUtils.createUpdate(1, [ { onyxMethod: OnyxUtils.METHOD.SET, key: ONYX_KEY, value: initialData, }, ]); -const mockUpdate2 = createOnyxMockUpdate(2, [ +const mockUpdate2 = OnyxUpdateMockUtils.createUpdate(2, [ { onyxMethod: OnyxUtils.METHOD.MERGE, key: ONYX_KEY, @@ -79,7 +79,7 @@ const report2PersonDiff = { const report3AvatarDiff: Partial = { avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_5.png', }; -const mockUpdate3 = createOnyxMockUpdate(3, [ +const mockUpdate3 = OnyxUpdateMockUtils.createUpdate(3, [ { onyxMethod: OnyxUtils.METHOD.MERGE, key: ONYX_KEY, @@ -89,7 +89,7 @@ const mockUpdate3 = createOnyxMockUpdate(3, [ }, }, ]); -const mockUpdate4 = createOnyxMockUpdate(4, [ +const mockUpdate4 = OnyxUpdateMockUtils.createUpdate(4, [ { onyxMethod: OnyxUtils.METHOD.MERGE, key: ONYX_KEY, @@ -106,7 +106,7 @@ const report4 = { ...exampleReportAction, automatic: true, } satisfies Partial; -const mockUpdate5 = createOnyxMockUpdate(5, [ +const mockUpdate5 = OnyxUpdateMockUtils.createUpdate(5, [ { onyxMethod: OnyxUtils.METHOD.MERGE, key: ONYX_KEY, @@ -135,7 +135,7 @@ describe('actions/OnyxUpdateManager', () => { await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); await Onyx.set(ONYX_KEY, initialData); - OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; + OnyxUpdateManagerUtils.mockValues.beforeValidateAndApplyDeferredUpdates = undefined; App.mockValues.missingOnyxUpdatesToBeApplied = undefined; OnyxUpdateManagerExports.resetDeferralLogicVariables(); }); @@ -194,7 +194,7 @@ describe('actions/OnyxUpdateManager', () => { finishFirstCall = resolve; }); - OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = () => { + OnyxUpdateManagerUtils.mockValues.beforeValidateAndApplyDeferredUpdates = () => { // After the first GetMissingOnyxUpdates call has been resolved, // we have to set the mocked results of for the second call. App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3, mockUpdate4]; @@ -261,7 +261,7 @@ describe('actions/OnyxUpdateManager', () => { }; let firstCallFinished = false; - OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = () => { + OnyxUpdateManagerUtils.mockValues.beforeValidateAndApplyDeferredUpdates = () => { if (firstCallFinished) { assertAfterSecondGetMissingOnyxUpdates(); return Promise.resolve(); diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index c106e992c292..cd0922416a62 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,17 +1,20 @@ import Onyx from 'react-native-onyx'; -import type {AppActionsMock} from '@libs/actions/__mocks__/App'; -import * as AppImport from '@libs/actions/App'; -import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; -import * as OnyxUpdateManagerUtilsImport from '@libs/actions/OnyxUpdateManager/utils'; -import type {OnyxUpdateManagerUtilsMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__'; -import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; -import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; +import type {AppActionsMock} from '@userActions/__mocks__/App'; +import type {OnyxUpdatesMock} from '@userActions/__mocks__/OnyxUpdates'; +import * as AppImport from '@userActions/App'; +import * as OnyxUpdateManager from '@userActions/OnyxUpdateManager'; +import * as OnyxUpdateManagerUtilsImport from '@userActions/OnyxUpdateManager/utils'; +import type {OnyxUpdateManagerUtilsMock} from '@userActions/OnyxUpdateManager/utils/__mocks__'; +import type {ApplyUpdatesMock} from '@userActions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; +import * as ApplyUpdatesImport from '@userActions/OnyxUpdateManager/utils/applyUpdates'; +import * as OnyxUpdatesImport from '@userActions/OnyxUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; -import createOnyxMockUpdate from '../utils/createOnyxMockUpdate'; +import OnyxUpdateMockUtils from '../utils/OnyxUpdateMockUtils'; -jest.mock('@libs/actions/App'); -jest.mock('@libs/actions/OnyxUpdateManager/utils'); -jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); +jest.mock('@userActions/OnyxUpdates'); +jest.mock('@userActions/App'); +jest.mock('@userActions/OnyxUpdateManager/utils'); +jest.mock('@userActions/OnyxUpdateManager/utils/applyUpdates'); jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ default: () => ({ @@ -19,17 +22,23 @@ jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ }), })); +const OnyxUpdates = OnyxUpdatesImport as OnyxUpdatesMock; const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; -const mockUpdate3 = createOnyxMockUpdate(3); -const offsetedMockUpdate3 = createOnyxMockUpdate(3, undefined, 2); -const mockUpdate4 = createOnyxMockUpdate(4); -const mockUpdate5 = createOnyxMockUpdate(5); -const mockUpdate6 = createOnyxMockUpdate(6); -const mockUpdate7 = createOnyxMockUpdate(7); -const mockUpdate8 = createOnyxMockUpdate(8); +const update2 = OnyxUpdateMockUtils.createUpdate(2); +const pendingUpdateUpTo2 = OnyxUpdateMockUtils.createPendingUpdate(2); + +const update3 = OnyxUpdateMockUtils.createUpdate(3); +const pendingUpdateUpTo3 = OnyxUpdateMockUtils.createPendingUpdate(3); +const offsetUpdate3 = OnyxUpdateMockUtils.createUpdate(3, undefined, 1); + +const update4 = OnyxUpdateMockUtils.createUpdate(4); +const update5 = OnyxUpdateMockUtils.createUpdate(5); +const update6 = OnyxUpdateMockUtils.createUpdate(6); +const update7 = OnyxUpdateMockUtils.createUpdate(7); +const update8 = OnyxUpdateMockUtils.createUpdate(8); describe('OnyxUpdateManager', () => { let lastUpdateIDAppliedToClient = 1; @@ -43,20 +52,26 @@ describe('OnyxUpdateManager', () => { beforeEach(async () => { jest.clearAllMocks(); await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); - OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; - ApplyUpdates.mockValues.onApplyUpdates = undefined; + OnyxUpdateManagerUtils.mockValues.beforeValidateAndApplyDeferredUpdates = undefined; + ApplyUpdates.mockValues.beforeApplyUpdates = undefined; + App.mockValues.missingOnyxUpdatesToBeApplied = undefined; OnyxUpdateManager.resetDeferralLogicVariables(); }); it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); + OnyxUpdateManager.handleMissingOnyxUpdates(update3); + OnyxUpdateManager.handleMissingOnyxUpdates(update4); + OnyxUpdateManager.handleMissingOnyxUpdates(update5); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(5); + // OnyxUpdates.apply should have been called 4 times, + // - once for the fetched missing update (2) + // - three times for the deferred updates (3-5) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(4); + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); @@ -70,27 +85,32 @@ describe('OnyxUpdateManager', () => { // since the locally applied updates have changed in the meantime. expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: update3, 4: update4, 5: update5}); }); }); - it('should only apply deferred updates that are newer than the last locally applied update (pending updates)', async () => { - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); + it('should only apply deferred updates that are newer than the last locally applied update (pending deferred updates)', async () => { + OnyxUpdateManager.handleMissingOnyxUpdates(update4); + OnyxUpdateManager.handleMissingOnyxUpdates(update5); + OnyxUpdateManager.handleMissingOnyxUpdates(update6); - OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = async () => { + OnyxUpdateManagerUtils.mockValues.beforeValidateAndApplyDeferredUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, // while we are waiting for the missing updates to be fetched. // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; + OnyxUpdateManagerUtils.mockValues.beforeValidateAndApplyDeferredUpdates = undefined; }; return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(6); + // OnyxUpdates.apply should have been called 2 times, + // - twice for the fetched missing updates (2-3) + // - once for the deferred update (6) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(3); + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); @@ -108,19 +128,25 @@ describe('OnyxUpdateManager', () => { expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {6: update6}); }); }); it('should re-fetch missing updates if the deferred updates have a gap', async () => { - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); + OnyxUpdateManager.handleMissingOnyxUpdates(update3); + OnyxUpdateManager.handleMissingOnyxUpdates(update5); + OnyxUpdateManager.handleMissingOnyxUpdates(update6); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(6); + // OnyxUpdates.apply should have been called 5 times, + // - once for the first fetched missing update (2) + // - once for the second fetched missing update (4) + // - three times for the deferred updates (3, 5, 6) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(5); + // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); @@ -136,27 +162,34 @@ describe('OnyxUpdateManager', () => { // After the initial missing updates have been applied, the applicable updates (3) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: update3}); // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates. 3-4 expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); // After the gap in the deferred updates has been resolved, the remaining deferred updates (5, 6) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: update5, 6: update6}); }); }); it('should re-fetch missing deferred updates only once per batch', async () => { - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate8); + OnyxUpdateManager.handleMissingOnyxUpdates(update3); + OnyxUpdateManager.handleMissingOnyxUpdates(update4); + OnyxUpdateManager.handleMissingOnyxUpdates(update6); + OnyxUpdateManager.handleMissingOnyxUpdates(update8); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(8); + // OnyxUpdates.apply should have been called 7 times, + // - once for the first fetched missing update (2) + // - once for the second fetched missing update (5) + // - once for the third fetched missing update (7) + // - four times for the deferred updates (3, 4, 6, 8) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(7); + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); @@ -167,36 +200,42 @@ describe('OnyxUpdateManager', () => { // After the initial missing updates have been applied, the applicable updates (3-4) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3, 4: mockUpdate4}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: update3, 4: update4}); // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap (4-7) in the deferred updates. expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 7); // After the gap in the deferred updates has been resolved, the remaining deferred updates (8) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {8: mockUpdate8}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {8: update8}); }); }); it('should not re-fetch missing updates if the lastUpdateIDFromClient has been updated', async () => { - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate5); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate6); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate7); + OnyxUpdateManager.handleMissingOnyxUpdates(update3); + OnyxUpdateManager.handleMissingOnyxUpdates(update5); + OnyxUpdateManager.handleMissingOnyxUpdates(update6); + OnyxUpdateManager.handleMissingOnyxUpdates(update7); - ApplyUpdates.mockValues.onApplyUpdates = async () => { + ApplyUpdates.mockValues.beforeApplyUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, // while the applicable updates have been applied. // When this happens, the OnyxUpdateManager should trigger another validation of the deferred updates, // without triggering another re-fetching of missing updates from the server. await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - ApplyUpdates.mockValues.onApplyUpdates = undefined; + ApplyUpdates.mockValues.beforeApplyUpdates = undefined; }; return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(7); + // OnyxUpdates.apply should have been called 4 times, + // - once for the first fetched missing update (2) + // - updates 3-5 are not fetched, because they are set externally + // - two times for the remaining deferred updates (6, 7) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(4); + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); @@ -211,31 +250,38 @@ describe('OnyxUpdateManager', () => { // After the initial missing updates have been applied, the applicable updates (3) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: update3}); // Since the lastUpdateIDAppliedToClient has changed to 5 in the meantime, we only need to apply the remaining deferred updates (6-7). // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {6: mockUpdate6, 7: mockUpdate7}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {6: update6, 7: update7}); }); }); it('should re-fetch missing updates if the lastUpdateIDFromClient has increased, but there are still gaps after the locally applied update', async () => { - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate3); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate7); + OnyxUpdateManager.handleMissingOnyxUpdates(update3); + OnyxUpdateManager.handleMissingOnyxUpdates(update7); - ApplyUpdates.mockValues.onApplyUpdates = async () => { + ApplyUpdates.mockValues.beforeApplyUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 4, to simulate local updates being applied, // while the applicable updates have been applied. // When this happens, the OnyxUpdateManager should trigger another validation of the deferred updates, // without triggering another re-fetching of missing updates from the server. await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 4); - ApplyUpdates.mockValues.onApplyUpdates = undefined; + ApplyUpdates.mockValues.beforeApplyUpdates = undefined; }; return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(7); + // OnyxUpdates.apply should have been called 6 times, + // - once for the first fetched missing update (2) + // - updates 3-4 are not fetched, because they are set externally + // - three times for the second fetched missing updates (5-6) + // - once for the remaining deferred update (7) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(5); + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); @@ -250,26 +296,32 @@ describe('OnyxUpdateManager', () => { // After the initial missing updates have been applied, the applicable updates (3) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: update3}); // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates, // that are later than the locally applied update (4-6). (including the last locally applied update) expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 6); - // Since the lastUpdateIDAppliedToClient has changed to 4 in the meantime and we're fetching updates 5-6 we only need to apply the remaining deferred updates (7). + // Since the lastUpdateIDAppliedToClient has changed to 4 in the meantime, and we're fetching updates 5-6 we only need to apply the remaining deferred updates (7). // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: mockUpdate7}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: update7}); }); }); it('should only fetch missing updates that are not outdated (older than already locally applied update)', () => { - OnyxUpdateManager.handleMissingOnyxUpdates(offsetedMockUpdate3); - OnyxUpdateManager.handleMissingOnyxUpdates(mockUpdate4); + OnyxUpdateManager.handleMissingOnyxUpdates(offsetUpdate3); + OnyxUpdateManager.handleMissingOnyxUpdates(update4); return OnyxUpdateManager.queryPromise.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 4. expect(lastUpdateIDAppliedToClient).toBe(4); + // OnyxUpdates.apply should have been called 2 times, + // - once for the first fetched missing update (2) + // - the offset update is omitted + // - three times for the deferred update (4) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(2); + // validateAndApplyDeferredUpdates should be called once for the initial deferred updates // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. // The intended assertion would look like this: @@ -279,7 +331,89 @@ describe('OnyxUpdateManager', () => { // since the locally applied updates have changed in the meantime. expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: offsetedMockUpdate3, 4: mockUpdate4}); + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: offsetUpdate3, 4: update4}); + + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + }); + }); + + it('should fetch a single pending update if `hasPendingOnyxUpdates flag is true`', () => { + App.mockValues.missingOnyxUpdatesToBeApplied = [update2]; + + OnyxUpdateManager.handleMissingOnyxUpdates(pendingUpdateUpTo2); + + return OnyxUpdateManager.queryPromise.then(() => { + // After all the pending update has been applied, the lastUpdateIDAppliedToClient should be 2. + expect(lastUpdateIDAppliedToClient).toBe(2); + + // OnyxUpdates.apply should have been called 1 times, + // - once for the pending update (2) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(1); + + // validateAndApplyDeferredUpdates should be called once for the initial deferred updates + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // There should be no call to applyUpdates, because there are no deferred updates + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(0); + + // There are no deferred updates, so this should only be called once for the pending update. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + }); + }); + + it('should fetch multiple pending updates if `hasPendingOnyxUpdates flag is true`', () => { + App.mockValues.missingOnyxUpdatesToBeApplied = [update2, update3]; + + OnyxUpdateManager.handleMissingOnyxUpdates(pendingUpdateUpTo3); + + return OnyxUpdateManager.queryPromise.then(() => { + // After all the pending update has been applied, the lastUpdateIDAppliedToClient should be 3. + expect(lastUpdateIDAppliedToClient).toBe(3); + + // OnyxUpdates.apply should have been called 2 times, + // - twice for the pending updates (2, 3) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(2); + + // validateAndApplyDeferredUpdates should be called once for the initial deferred updates + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // There should be no call to applyUpdates, because there are no deferred updates + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(0); + + // There are no deferred updates, so this should only be called once for the pending update. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + }); + }); + + it('should apply deferred updates after fetching pending updates', () => { + App.mockValues.missingOnyxUpdatesToBeApplied = [update2, update3]; + + OnyxUpdateManager.handleMissingOnyxUpdates(pendingUpdateUpTo3); + OnyxUpdateManager.handleMissingOnyxUpdates(update4); + + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 4. + expect(lastUpdateIDAppliedToClient).toBe(4); + + // OnyxUpdates.apply should have been called 3 times, + // - twice for the pending updates (2, 3) + // - once for the deferred update (4) + expect(OnyxUpdates.apply).toHaveBeenCalledTimes(3); + + // validateAndApplyDeferredUpdates should be called once for the initial deferred updates + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // There should be only one call to applyUpdates. The call should contain the deferred updates. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {4: update4}); // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); diff --git a/tests/utils/OnyxUpdateMockUtils.ts b/tests/utils/OnyxUpdateMockUtils.ts new file mode 100644 index 000000000000..90d738a413cc --- /dev/null +++ b/tests/utils/OnyxUpdateMockUtils.ts @@ -0,0 +1,36 @@ +import type {OnyxUpdate} from 'react-native-onyx'; +import CONST from '@src/CONST'; +import type {OnyxUpdatesFromServer} from '@src/types/onyx'; + +const createUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = [], previousUpdateID?: number): OnyxUpdatesFromServer => ({ + type: CONST.ONYX_UPDATE_TYPES.HTTPS, + lastUpdateID, + previousUpdateID: previousUpdateID ?? lastUpdateID - 1, + request: { + command: 'TestCommand', + successData, + failureData: [], + finallyData: [], + optimisticData: [], + }, + response: { + jsonCode: 200, + lastUpdateID, + previousUpdateID: previousUpdateID ?? lastUpdateID - 1, + onyxData: successData, + }, +}); + +const createPendingUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ + type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, + lastUpdateID, + shouldFetchPendingUpdates: true, + updates: [], +}); + +const OnyxUpdateMockUtils = { + createUpdate, + createPendingUpdate, +}; + +export default OnyxUpdateMockUtils; diff --git a/tests/utils/createOnyxMockUpdate.ts b/tests/utils/createOnyxMockUpdate.ts deleted file mode 100644 index e50918b01faf..000000000000 --- a/tests/utils/createOnyxMockUpdate.ts +++ /dev/null @@ -1,23 +0,0 @@ -import type {OnyxUpdate} from 'react-native-onyx'; -import type {OnyxUpdatesFromServer} from '@src/types/onyx'; - -const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = [], previousUpdateIDOffset = 1): OnyxUpdatesFromServer => ({ - type: 'https', - lastUpdateID, - previousUpdateID: lastUpdateID - previousUpdateIDOffset, - request: { - command: 'TestCommand', - successData, - failureData: [], - finallyData: [], - optimisticData: [], - }, - response: { - jsonCode: 200, - lastUpdateID, - previousUpdateID: lastUpdateID - previousUpdateIDOffset, - onyxData: successData, - }, -}); - -export default createOnyxMockUpdate; From 3114a022d37bf6ebb7d1c66b57ce26d9b577f347 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 7 Jan 2025 20:11:51 +0100 Subject: [PATCH 16/16] fix: actions/OnyxUpdateManagerTest --- src/libs/actions/OnyxUpdates.ts | 2 +- src/libs/actions/__mocks__/OnyxUpdates.ts | 10 +++++++--- tests/actions/OnyxUpdateManagerTest.ts | 9 +++++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index d2a78eb4a2bb..a09159993ad8 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -190,5 +190,5 @@ function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID}: DoesC } // eslint-disable-next-line import/prefer-default-export -export {apply, doesClientNeedToBeUpdated, saveUpdateInformation}; +export {apply, doesClientNeedToBeUpdated, saveUpdateInformation, applyHTTPSOnyxUpdates as INTERNAL_DO_NOT_USE_applyHTTPSOnyxUpdates}; export type {DoesClientNeedToBeUpdatedParams as ManualOnyxUpdateCheckIds}; diff --git a/src/libs/actions/__mocks__/OnyxUpdates.ts b/src/libs/actions/__mocks__/OnyxUpdates.ts index 0c7a9df4f6ab..3e4cb10d7f9e 100644 --- a/src/libs/actions/__mocks__/OnyxUpdates.ts +++ b/src/libs/actions/__mocks__/OnyxUpdates.ts @@ -6,7 +6,7 @@ import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); const OnyxUpdatesImplementation = jest.requireActual('@libs/actions/OnyxUpdates'); -const {doesClientNeedToBeUpdated, saveUpdateInformation} = OnyxUpdatesImplementation; +const {doesClientNeedToBeUpdated, saveUpdateInformation, INTERNAL_DO_NOT_USE_applyHTTPSOnyxUpdates: applyHTTPSOnyxUpdates} = OnyxUpdatesImplementation; type OnyxUpdatesMock = typeof OnyxUpdatesImport & { apply: jest.Mock, [OnyxUpdatesFromServer]>; @@ -18,9 +18,13 @@ Onyx.connect({ callback: (val) => (lastUpdateIDAppliedToClient = val), }); -const apply = jest.fn(({lastUpdateID}: OnyxUpdatesFromServer): Promise | undefined => { +const apply = jest.fn(({lastUpdateID, request, response}: OnyxUpdatesFromServer): Promise | undefined => { if (lastUpdateID && (lastUpdateIDAppliedToClient === undefined || Number(lastUpdateID) > lastUpdateIDAppliedToClient)) { - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Number(lastUpdateID)); + Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Number(lastUpdateID)); + } + + if (request && response) { + return applyHTTPSOnyxUpdates(request, response).then(() => undefined); } return Promise.resolve(); diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts index 3039d1999a79..7366c13fa8b3 100644 --- a/tests/actions/OnyxUpdateManagerTest.ts +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -17,10 +17,11 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type * as OnyxTypes from '@src/types/onyx'; import OnyxUpdateMockUtils from '../utils/OnyxUpdateMockUtils'; -jest.mock('@libs/actions/App'); -jest.mock('@libs/actions/OnyxUpdateManager/utils'); -jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates', () => { - const ApplyUpdatesImplementation = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); +jest.mock('@userActions/OnyxUpdates'); +jest.mock('@userActions/App'); +jest.mock('@userActions/OnyxUpdateManager/utils'); +jest.mock('@userActions/OnyxUpdateManager/utils/applyUpdates', () => { + const ApplyUpdatesImplementation = jest.requireActual('@userActions/OnyxUpdateManager/utils/applyUpdates'); return { applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => ApplyUpdatesImplementation.applyUpdates(updates)),