Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update optimistic IDs in sequential queue #27996

Merged
merged 33 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
2f29985
Create deepReplaceKeysAndValues function
roryabraham Sep 22, 2023
5a4676d
Fix JSDoc comment
roryabraham Sep 22, 2023
80102ca
Convert deepReplaceKeysAndValues to TS
roryabraham Sep 25, 2023
0ebf3dc
Save draft state
roryabraham Sep 25, 2023
833ba10
Merge branch 'main' into Rory-UpdateOptimisticIDsInSequentialQueue
roryabraham Sep 26, 2023
af7094b
Merge branch 'main' into Rory-UpdateOptimisticIDsInSequentialQueue
roryabraham Sep 26, 2023
0d5f4ac
Create base test
roryabraham Sep 27, 2023
f1441a8
Implement middleware to handle unused optimistic IDs
roryabraham Sep 27, 2023
09ba949
Fix typescript types
roryabraham Sep 27, 2023
cb5888b
Don't suppress lint rule
roryabraham Sep 28, 2023
5f3110d
Remove unnecessary type check
roryabraham Sep 28, 2023
7f83936
Simplify key generation
roryabraham Sep 28, 2023
bf153a5
Turn on the middleware
roryabraham Sep 28, 2023
15756ba
Get middleware working correctly
roryabraham Sep 28, 2023
80af4cc
Fix TypeScript types
roryabraham Sep 28, 2023
f0fbf0d
Update test and fix off-by-one error
roryabraham Sep 28, 2023
224a134
Replace reportID in navigation stack
roryabraham Sep 29, 2023
1d05d9b
Start creating UI tests
roryabraham Sep 29, 2023
5489195
Merge branch 'main' into Rory-UpdateOptimisticIDsInSequentialQueue
roryabraham Sep 29, 2023
06cc7bd
Finish writing test
roryabraham Sep 29, 2023
1aad488
Remove unnecessary local reference
roryabraham Sep 29, 2023
e583ca4
Simplify PersistedRequests.update
roryabraham Sep 29, 2023
0d69f32
Only skip the first persisted request if from sequential queue
roryabraham Sep 29, 2023
ed415dc
Fix spacing
roryabraham Sep 29, 2023
e1f470f
Renamed obj to target
roryabraham Sep 29, 2023
d9863e1
Fix typescript types
roryabraham Sep 29, 2023
e72dded
Remove commented out code
roryabraham Oct 1, 2023
0d477ac
Update outdated comment
roryabraham Oct 1, 2023
8b777ea
Revert navigation changes
roryabraham Oct 2, 2023
0094aab
Updated to use lodashClone
roryabraham Oct 2, 2023
38aa8ee
Merge branch 'main' into Rory-UpdateOptimisticIDsInSequentialQueue
roryabraham Oct 2, 2023
e059f57
Merge branch 'main' into Rory-UpdateOptimisticIDsInSequentialQueue
roryabraham Oct 2, 2023
f0b1a4e
Use offset in PersistedRequests.update
roryabraham Oct 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libs/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ Request.use(Middleware.RecheckConnection);
// Reauthentication - Handles jsonCode 407 which indicates an expired authToken. We need to reauthenticate and get a new authToken with our stored credentials.
Request.use(Middleware.Reauthentication);

// If an optimistic ID is not used by the server, this will update the remaining serialized requests using that optimistic ID to use the correct ID instead.
Request.use(Middleware.HandleUnusedOptimisticID);

// SaveResponseInOnyx - Merges either the successData or failureData into Onyx depending on if the call was successful or not. This needs to be the LAST middleware we use, don't add any
// middlewares after this, because the SequentialQueue depends on the result of this middleware to pause the queue (if needed) to bring the app to an up-to-date state.
Request.use(Middleware.SaveResponseInOnyx);
Expand Down
39 changes: 39 additions & 0 deletions src/libs/Middleware/HandleUnusedOptimisticID.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import _ from 'lodash';
import ONYXKEYS from '../../ONYXKEYS';
import Report from '../../types/onyx/Report';
import {Middleware} from '../Request';
import * as PersistedRequests from '../actions/PersistedRequests';
import deepReplaceKeysAndValues from '../deepReplaceKeysAndValues';

const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSequentialQueue) =>
requestResponse.then((response) => {
const responseOnyxData = response?.onyxData ?? [];
responseOnyxData.forEach((onyxData) => {
const key = onyxData.key;
if (!key.startsWith(ONYXKEYS.COLLECTION.REPORT)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key could be undefined which caused this issue - #28748 (comment)

return;
}

if (!onyxData.value) {
return;
}

const report: Report = onyxData.value as Report;
const preexistingReportID = report.preexistingReportID;
if (!preexistingReportID) {
return;
}
const oldReportID = request.data?.reportID;
const offset = isFromSequentialQueue ? 1 : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explain this with a code comment just a little bit. Something like:

If we are in the sequential queue then we want to ignore the first request in the queue as we know that it's values are correct. If we are not in the sequential queue then all of the requests queued to run after this are safe to modify.

PersistedRequests.getAll()
.slice(offset)
.forEach((persistedRequest, index) => {
const persistedRequestClone = _.clone(persistedRequest);
persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea for future improvement: return a boolean if deepReplaceKeysAndValues made any change to the clone, only use PersistedRequests.update if something changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Onyx will also check this, but it incurs an extra object comparison so - maybe it can make some difference.

PersistedRequests.update(index + offset, persistedRequestClone);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea for future improvement: don't run PersistedRequests.update in a loop, because it will result in n calls to Onyx.set instead of potentially just 1 call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably depends on the number of pending requests. I would keep this how it is but keep an eye on it. Alternatively test it by going offline and triggering a bunch of different action. Then go online and observe how quickly the UI updates (non-scientific approach).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another school of thought is that this is gonna be pretty edge case so maybe we can get away with what we've got here..

});
});
return response;
});

export default handleUnusedOptimisticID;
3 changes: 2 additions & 1 deletion src/libs/Middleware/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import HandleUnusedOptimisticID from './HandleUnusedOptimisticID';
import Logging from './Logging';
import Reauthentication from './Reauthentication';
import RecheckConnection from './RecheckConnection';
import SaveResponseInOnyx from './SaveResponseInOnyx';

export {Logging, Reauthentication, RecheckConnection, SaveResponseInOnyx};
export {HandleUnusedOptimisticID, Logging, Reauthentication, RecheckConnection, SaveResponseInOnyx};
10 changes: 6 additions & 4 deletions src/libs/Request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import HttpUtils from './HttpUtils';
import enhanceParameters from './Network/enhanceParameters';
import * as NetworkStore from './Network/NetworkStore';
import Request from '../types/onyx/Request';
import Response from '../types/onyx/Response';

type Middleware = (response: unknown, request: Request, isFromSequentialQueue: boolean) => Promise<unknown>;
type Middleware = (response: Promise<Response>, request: Request, isFromSequentialQueue: boolean) => Promise<Response>;

let middlewares: Middleware[] = [];

function makeXHR(request: Request): Promise<unknown> {
function makeXHR(request: Request): Promise<Response> {
const finalParameters = enhanceParameters(request.command, request?.data ?? {});
return NetworkStore.hasReadRequiredDataFromStorage().then(() => {
// If we're using the Supportal token and this is not a Supportal request
Expand All @@ -16,10 +17,10 @@ function makeXHR(request: Request): Promise<unknown> {
return new Promise<void>((resolve) => resolve());
}
return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure);
});
}) as Promise<Response>;
}

function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise<unknown> {
function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise<Response> {
return middlewares.reduce((last, middleware) => middleware(last, request, isFromSequentialQueue), makeXHR(request));
}

Expand All @@ -32,3 +33,4 @@ function clearMiddlewares() {
}

export {clearMiddlewares, processWithMiddleware, use};
export type {Middleware};
17 changes: 10 additions & 7 deletions src/libs/actions/PersistedRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,21 @@ function remove(requestToRemove: Request) {
* We only remove the first matching request because the order of requests matters.
* If we were to remove all matching requests, we can end up with a final state that is different than what the user intended.
*/
const requests = [...persistedRequests];
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove));
if (index !== -1) {
requests.splice(index, 1);
const index = persistedRequests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove));
if (index === -1) {
return;
}
persistedRequests.splice(index, 1);
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

persistedRequests = requests;
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests);
function update(oldRequestIndex: number, newRequest: Request) {
persistedRequests.splice(oldRequestIndex, 1, newRequest);
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

function getAll(): Request[] {
return persistedRequests;
}

export {clear, save, getAll, remove};
export {clear, save, getAll, remove, update};
50 changes: 50 additions & 0 deletions src/libs/deepReplaceKeysAndValues.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
type ReplaceableValue = Record<string, unknown> | unknown[] | string | number | boolean | undefined | null;

/**
* @param target the object or value to transform
* @param oldVal the value to search for
* @param newVal the replacement value
*/
function deepReplaceKeysAndValues<T extends ReplaceableValue>(target: T, oldVal: string, newVal: string): T {
if (!target) {
return target;
}

if (typeof target === 'string') {
return target.replace(oldVal, newVal) as T;
}

if (typeof target !== 'object') {
return target;
}

if (Array.isArray(target)) {
return target.map((item) => deepReplaceKeysAndValues(item as T, oldVal, newVal)) as T;
}

const newObj: Record<string, unknown> = {};
Object.entries(target).forEach(([key, val]) => {
const newKey = key.replace(oldVal, newVal);

if (typeof val === 'object') {
newObj[newKey] = deepReplaceKeysAndValues(val as T, oldVal, newVal);
return;
}

if (val === oldVal) {
newObj[newKey] = newVal;
return;
}

if (typeof val === 'string') {
newObj[newKey] = val.replace(oldVal, newVal);
return;
}

newObj[newKey] = val;
});

return newObj as T;
}

export default deepReplaceKeysAndValues;
3 changes: 2 additions & 1 deletion src/types/onyx/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ type Report = {
reportName?: string;

/** ID of the report */
reportID?: string;
reportID: string;

/** The state that the report is currently in */
stateNum?: ValueOf<typeof CONST.REPORT.STATE_NUM>;
Expand Down Expand Up @@ -77,6 +77,7 @@ type Report = {
participantAccountIDs?: number[];
total?: number;
currency?: string;
preexistingReportID?: string;
};

export default Report;
102 changes: 102 additions & 0 deletions tests/unit/MiddlewareTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import Onyx from 'react-native-onyx';
import * as NetworkStore from '../../src/libs/Network/NetworkStore';
import * as Request from '../../src/libs/Request';
import * as SequentialQueue from '../../src/libs/Network/SequentialQueue';
import * as TestHelper from '../utils/TestHelper';
import waitForNetworkPromises from '../utils/waitForNetworkPromises';
import ONYXKEYS from '../../src/ONYXKEYS';
import * as MainQueue from '../../src/libs/Network/MainQueue';
import HttpUtils from '../../src/libs/HttpUtils';

Onyx.init({
keys: ONYXKEYS,
});

beforeAll(() => {
global.fetch = TestHelper.getGlobalFetchMock();
});

beforeEach(async () => {
SequentialQueue.pause();
MainQueue.clear();
HttpUtils.cancelPendingRequests();
NetworkStore.checkRequiredData();
await waitForNetworkPromises();
jest.clearAllMocks();
Request.clearMiddlewares();
});

describe('Middleware', () => {
describe('HandleUnusedOptimisticID', () => {
test('Normal request', async () => {
const actual = jest.requireActual('../../src/libs/Middleware/HandleUnusedOptimisticID');
const handleUnusedOptimisticID = jest.spyOn(actual, 'default');
Request.use(handleUnusedOptimisticID);
const requests = [
{
command: 'OpenReport',
data: {authToken: 'testToken', reportID: '1234'},
},
{
command: 'AddComment',
data: {authToken: 'testToken', reportID: '1234', reportActionID: '5678', reportComment: 'foo'},
},
];
for (const request of requests) {
SequentialQueue.push(request);
}
SequentialQueue.unpause();
await waitForNetworkPromises();

expect(global.fetch).toHaveBeenCalledTimes(2);
expect(global.fetch).toHaveBeenLastCalledWith('https://www.expensify.com.dev/api?command=AddComment', expect.anything());
TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[1][1].body, {reportID: '1234', reportActionID: '5678', reportComment: 'foo'});
expect(global.fetch).toHaveBeenNthCalledWith(1, 'https://www.expensify.com.dev/api?command=OpenReport', expect.anything());
TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[0][1].body, {reportID: '1234'});
});

test('Request with preexistingReportID', async () => {
const actual = jest.requireActual('../../src/libs/Middleware/HandleUnusedOptimisticID');
const handleUnusedOptimisticID = jest.spyOn(actual, 'default');
Request.use(handleUnusedOptimisticID);
const requests = [
{
command: 'OpenReport',
data: {authToken: 'testToken', reportID: '1234'},
},
{
command: 'AddComment',
data: {authToken: 'testToken', reportID: '1234', reportActionID: '5678', reportComment: 'foo'},
},
];
for (const request of requests) {
SequentialQueue.push(request);
}

global.fetch.mockImplementationOnce(async () => ({
ok: true,
json: async () => ({
jsonCode: 200,
onyxData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}1234`,
value: {
preexistingReportID: '5555',
},
},
],
}),
}));

SequentialQueue.unpause();
await waitForNetworkPromises();

expect(global.fetch).toHaveBeenCalledTimes(2);
expect(global.fetch).toHaveBeenLastCalledWith('https://www.expensify.com.dev/api?command=AddComment', expect.anything());
TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[1][1].body, {reportID: '5555', reportActionID: '5678', reportComment: 'foo'});
expect(global.fetch).toHaveBeenNthCalledWith(1, 'https://www.expensify.com.dev/api?command=OpenReport', expect.anything());
TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[0][1].body, {reportID: '1234'});
});
});
});
Loading
Loading