Refactor save method in PersistedRequests #30425
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
WojtekBoman
force-pushed
the
refactor-persisted-requests
branch
from
b338fba
to
2166dfb
Compare
WojtekBoman
force-pushed
the
refactor-persisted-requests
branch
from
2166dfb
to
f214161
Compare
melvin-bot
bot
requested review from
alitoshmatov
and removed request for
a team
|
@alitoshmatov Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Reviewing |
Reviewer Checklist
Screenshots/VideosAndroid: Nativerefactoring-android.movAndroid: mWeb ChromeiOS: Nativerefactoring-ios.mp4iOS: mWeb Safarirefactoring-safari.mp4MacOS: Chrome / Safarirefactoring-web.movMacOS: Desktoprefactoring-desktop.mov |
There was a problem hiding this comment.
I started toying with this locally while reviewing and this is the implementation I landed on:
diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts
index d4aee4a221..5da032baaf 100644
--- a/src/libs/Network/SequentialQueue.ts
+++ b/src/libs/Network/SequentialQueue.ts
@@ -160,7 +160,7 @@ NetworkStore.onReconnection(flush);
function push(request: OnyxRequest) {
// Add request to Persisted Requests so that it can be retried if it fails
- PersistedRequests.save([request]);
+ PersistedRequests.save(request);
// If we are offline we don't need to trigger the queue to empty as it will happen when we come back online
if (NetworkStore.isOffline()) {
diff --git a/src/libs/Network/enhanceParameters.ts b/src/libs/Network/enhanceParameters.ts
index 6ff54f94bc..3fadeea744 100644
--- a/src/libs/Network/enhanceParameters.ts
+++ b/src/libs/Network/enhanceParameters.ts
@@ -37,5 +37,8 @@ export default function enhanceParameters(command: string, parameters: Record<st
// Include current user's email in every request and the server logs
finalParameters.email = parameters.email ?? NetworkStore.getCurrentUserEmail();
+ // idempotencyKey declared in JS is front-end-only. We delete it here so it doesn't interfere with idempotency in other layers.
+ delete finalParameters.idempotencyKey;
+
return finalParameters;
}
diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts
index c35de9ee94..8661c4acec 100644
--- a/src/libs/actions/PersistedRequests.ts
+++ b/src/libs/actions/PersistedRequests.ts
@@ -1,4 +1,5 @@
import isEqual from 'lodash/isEqual';
+import merge from 'lodash/merge';
import Onyx from 'react-native-onyx';
import ONYXKEYS from '@src/ONYXKEYS';
import {Request} from '@src/types/onyx';
@@ -17,15 +18,17 @@ function clear() {
return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []);
}
-function save(requestsToPersist: Request[]) {
- let requests: Request[] = [];
- if (persistedRequests.length) {
- requests = persistedRequests.concat(requestsToPersist);
+function save(requestToPersist: Request) {
+ // Check for a request w/ matching idempotencyKey in the queue
+ const existingRequestIndex = persistedRequests.findIndex((request) => request.idempotencyKey && request.idempotencyKey === requestToPersist.idempotencyKey);
+ if (existingRequestIndex > 0) {
+ // Merge the new request into the existing one, keeping its place in the queue
+ persistedRequests.splice(existingRequestIndex, 1, merge(persistedRequests[existingRequestIndex], requestToPersist));
} else {
- requests = requestsToPersist;
+ // If not, push the new request to the end of the queue
+ persistedRequests.push(requestToPersist);
}
- persistedRequests = requests;
- Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests);
+ Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}
function remove(requestToRemove: Request) {
diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 1de15c1184..75d97e09cd 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -463,6 +463,7 @@ function reportActionsExist(reportID) {
* @param {Array} participantAccountIDList The list of accountIDs that are included in a new chat, not including the user creating it
*/
function openReport(reportID, participantLoginList = [], newReportObject = {}, parentReportActionID = '0', isFromDeepLink = false, participantAccountIDList = []) {
+ const commandName = 'OpenReport';
const optimisticReportData = [
{
onyxMethod: Onyx.METHOD.MERGE,
@@ -528,6 +529,7 @@ function openReport(reportID, participantLoginList = [], newReportObject = {}, p
emailList: participantLoginList ? participantLoginList.join(',') : '',
accountIDList: participantAccountIDList ? participantAccountIDList.join(',') : '',
parentReportActionID,
+ idempotencyKey: `${commandName}_${reportID}`,
};
if (isFromDeepLink) {
@@ -625,12 +627,12 @@ function openReport(reportID, participantLoginList = [], newReportObject = {}, p
if (isFromDeepLink) {
// eslint-disable-next-line rulesdir/no-api-side-effects-method
- API.makeRequestWithSideEffects('OpenReport', params, onyxData).finally(() => {
+ API.makeRequestWithSideEffects(commandName, params, onyxData).finally(() => {
Onyx.set(ONYXKEYS.IS_CHECKING_PUBLIC_ROOM, false);
});
} else {
// eslint-disable-next-line rulesdir/no-multiple-api-calls
- API.write('OpenReport', params, onyxData);
+ API.write(commandName, params, onyxData);
}
}
diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts
index 836138ca99..c97a5a21f4 100644
--- a/src/types/onyx/Request.ts
+++ b/src/types/onyx/Request.ts
@@ -15,6 +15,7 @@ type RequestData = {
shouldUseSecure?: boolean;
successData?: OnyxUpdate[];
failureData?: OnyxUpdate[];
+ idempotencyKey?: string;
resolve?: (value: Response) => void;
reject?: (value?: unknown) => void;If it works just as well, I think this is a fair deal simpler.
|
Also, let's plan on adding some basic unit tests to cover this change before merging |
|
@roryabraham I've added some adjustments according to your suggestions. Additionally I've written some basic tests to check if behaviour of PersistedRequests save method is proper. I have one question about merging data from requests. What should be the result of this test: If we use the lodashMerge function then length of both arrays inside the mergedRequest object will be equal to 1. Is it expected behaviour of merging requests? I created method mergeOnyxUpdateData, because I thought that it should work in the way presented above in the test. This method merges data of arrays: successData, failureData and optimisticData by key property of the elements in arrays. If it shouldn't work this way and we can use only lodashMerge to merge this requests I will fix the tests and remove unnecessary code. |
There was a problem hiding this comment.
Oh, thanks for pointing out that difference and great work so far.
I seriously considered recommending a bit more subtle behavior we could achieve with regards to merging Onyx updates, according to the following logic:
- If the
keyis different, always concat the new updates into the array - If there's a matching key, and
onyxMethodisset, we replace the value, because that's the same end result as amergethenset - If there's a matching key, and
onyxMethodismergeormergeCollection, we merge the values (alternatively would could just concat, but it's be more efficient to collapse Onyx updates so the subscribers to the matching key are only updated once, not twice) - Any time
onyxMethodis clear, the end result is just an array with oneclearupdate, because that's the end result we'll have.
My code sample looked like this:
import merge from 'lodash/merge';
import mergeWith from 'lodash/mergeWith';
function createUpdatedRequest(oldRequest: Request, newRequest: Request): Request {
return mergeWith(oldRequest, newRequest, (objValue, srcValue) => {
if (!Array.isArray(objValue) || !objValue.some((obj) => 'onyxMethod' in obj)) {
return;
}
for (const onyxUpdate of srcValue) {
switch (onyxUpdate.onyxMethod) {
case Onyx.METHOD.MERGE:
case Onyx.METHOD.MERGE_COLLECTION: {
// Find any existing merge on the same key
const matchingUpdateIndex = objValue.findIndex((existingUpdate) => existingUpdate.onyxMethod === Onyx.METHOD.MERGE && existingUpdate.key === onyxUpdate.key);
if (matchingUpdateIndex) {
// Merge the new value into the existing one
const matchingUpdate = objValue[matchingUpdateIndex];
objValue.splice(matchingUpdateIndex, 1, merge(matchingUpdate, onyxUpdate));
} else {
// Otherwise, add the new merge operation to the onyx updates
objValue.push(onyxUpdate);
}
break;
}
case Onyx.METHOD.SET: {
// Remove any existing updates for the same key
const objValue = objValue.filter((existingUpdate) => existingUpdate.key !== onyxUpdate.key);
// Then add the new SET update
objValue.push(onyxUpdate);
break;
}
case Onyx.METHOD.CLEAR: {
// All that matters is the one clear operation, so that's all we'll do.
// Any updates that come after should still be applied
// eslint-disable-next-line no-param-reassign
objValue = [onyxUpdate];
break;
}
default:
// Not possible, do nothing
}
}
return objValue;
});
}The idea being that we can collapse similar Onyx updates together and minimize the number of Onyx updates, eliminating any unnecessary work. That said, I feel that it's a bit over-engineered and a pre-optimization. After all, today we would apply all the Onyx updates and perform all the separate network requests.
I think we can consider that in the context of a follow-up improvement – maybe building that directly into Onyx. It also might not matter since Onyx has batched updates. For now, let's be conservative and go with something much simpler to land us in the correct final state:
import mergeWith from 'lodash/mergeWith';
function createUpdatedRequest(oldRequest: Request, newRequest: Request): Request {
// Merge the requests together, but concat Onyx update arrays together
return mergeWith(oldRequest, newRequest, (objValue, srcValue) => {
if (!Array.isArray(objValue) || !objValue.some((obj) => 'onyxMethod' in obj)) {
return;
}
return objValue.concat(srcValue);
});
}
|
Brought this up in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1699039930496579 Seems like there's general consensus that this isn't a bad idea, so that's good |
|
I refactored createUpdatedRequest method. I did it without using lodashMergeWith function because in that implementation objValue and srcValue have types any. Currently it works but we should be aware that now we can store duplicates in failureData, successData and optimisticData arrays. |
There was a problem hiding this comment.
Recognized a problem with the current approach: https://expensify.slack.com/archives/C01GTK53T8Q/p1699893959272759?thread_ts=1699039930.496579&cid=C01GTK53T8Q
|
Nice and simple where we landed |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.2-0 🚀
|
|
@roryabraham The issue connected with this PR is mentioned above. I've checked it and previously in the save method in PersistedRequests we used a copy of persistedRequests. Now we modify this array directly in the function and save to the Onyx. I think that this issue can be fixed by using a copy of persistedRequests inside the save function. diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts
index c788d69de7..bf98c19a5e 100644
--- a/src/libs/actions/PersistedRequests.ts
+++ b/src/libs/actions/PersistedRequests.ts
@@ -18,16 +18,18 @@ function clear() {
}
function save(requestToPersist: Request) {
+ const requests = [...persistedRequests]
// Check for a request w/ matching idempotencyKey in the queue
- const existingRequestIndex = persistedRequests.findIndex((request) => request.data?.idempotencyKey && request.data?.idempotencyKey === requestToPersist.data?.idempotencyKey);
+ const existingRequestIndex = requests.findIndex((request) => request.data?.idempotencyKey && request.data?.idempotencyKey === requestToPersist.data?.idempotencyKey);
if (existingRequestIndex > -1) {
// Merge the new request into the existing one, keeping its place in the queue
- persistedRequests.splice(existingRequestIndex, 1, requestToPersist);
+ requests.splice(existingRequestIndex, 1, requestToPersist);
} else {
// If not, push the new request to the end of the queue
- persistedRequests.push(requestToPersist);
+ requests.push(requestToPersist);
}
- Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
+ persistedRequests = requests
+ Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests);
}
|
|
@WojtekBoman I tried your solution above but I get the following error in the |
|
I'm reverting this PR since it introduced this blocker and other issues #31733 |
Could you please give me the steps to reproduce? I tried to reproduce it but I didn't get this error Screen.Recording.2023-11-23.at.11.03.23.mov |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.2-3 🚀
|
Sounds good 👍🏼 |
I'm not sure if there are any other steps other than just log in and log out with the account. That's all I did to reproduce that issue. I can test again once the new PR is up and see if I still have the issue. |
@roryabraham @luacmartins The new PR is ready #32246. I've tried to reproduce other issues that occured after merging v1 to the main branch and now it should work fine |
Details
This PR fixes storing duplicated data in
PersistedRequestsarray. This solution based on addingidempotencyKeyparam toOnyxDatatype. There cannot be two requests with the sameidempotencyKeyin thePersistedRequestsarray.cc @roryabraham
After fix in PersistedRequests.ts file.
Screen.Recording.2023-10-26.at.10.10.29.mov
Fixed Issues
$ #28172
PROPOSAL:
$ #28172 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2023-10-26.at.12.31.29.mov
Android: mWeb Chrome
Screen.Recording.2023-10-26.at.12.32.32.mov
iOS: Native
Screen.Recording.2023-10-26.at.12.53.09.mov
iOS: mWeb Safari
Screen.Recording.2023-10-26.at.12.56.40.mov
MacOS: Chrome / Safari
Screen.Recording.2023-10-26.at.10.10.29.mov
MacOS: Desktop
Screen.Recording.2023-10-26.at.12.21.51.mov