From 418b8d589245446ff0408eaaec0994159339d39c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 28 Mar 2024 18:47:27 +0100 Subject: [PATCH 1/6] add failing test --- tests/unit/onyxTest.js | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index a75fd6bb1..ff8ed1e56 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -2,6 +2,7 @@ import _ from 'underscore'; import Onyx from '../../lib'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import OnyxUtils from '../../lib/OnyxUtils'; +import StorageMock from '../../lib/storage/__mocks__'; const ONYX_KEYS = { TEST_KEY: 'test', @@ -1071,4 +1072,62 @@ describe('Onyx', () => { expect(testKeyValue).toEqual(null); }); }); + + it('(nested) nullish values should be removed when changes are batched during merge (SQLite)', () => { + let result; + + const initialData = { + a: 'a', + b: 'b', + c: { + d: 'd', + e: 'e', + }, + }; + const change1 = null; + const change2 = { + f: 'f', + c: { + g: 'g', + }, + }; + const changes = [change1, change2]; + + const batchedChanges = OnyxUtils.applyMerge(undefined, changes, false); + + console.log('Batched changes\n', JSON.stringify(batchedChanges, null, 2)); + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => (result = value), + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, initialData) + .then(() => { + expect(result).toEqual(initialData); + Onyx.merge(ONYX_KEYS.TEST_KEY, changes[0]); + Onyx.merge(ONYX_KEYS.TEST_KEY, changes[1]); + }) + .then(waitForPromisesToResolve) + .then(() => { + expect(result).toEqual({ + f: 'f', + c: { + g: 'g', + }, + }); + + console.log('Result in storage\n', JSON.stringify(result, null, 2)); + + // We would need to mock SQLite here to actually see what happens + // The current storage mock will just use the "modifiedData" from Onyx and just set it instead of actually applying the delta change. + expect(StorageMock.getItem(ONYX_KEYS.TEST_KEY)).toEqual({ + f: 'f', + c: { + g: 'g', + }, + }); + }); + }); }); From 1538e885bcee92cb8cdc229dec040b7ad43658fc Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 29 Mar 2024 11:40:44 +0100 Subject: [PATCH 2/6] update test --- tests/unit/onyxTest.js | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index ff8ed1e56..617a237b5 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -1089,9 +1089,15 @@ describe('Onyx', () => { f: 'f', c: { g: 'g', + h: 'h', }, }; - const changes = [change1, change2]; + const change3 = { + c: { + g: null, + }, + }; + const changes = [change1, change2, change3]; const batchedChanges = OnyxUtils.applyMerge(undefined, changes, false); @@ -1106,26 +1112,29 @@ describe('Onyx', () => { return Onyx.set(ONYX_KEYS.TEST_KEY, initialData) .then(() => { expect(result).toEqual(initialData); - Onyx.merge(ONYX_KEYS.TEST_KEY, changes[0]); - Onyx.merge(ONYX_KEYS.TEST_KEY, changes[1]); + + _.map(changes, (change) => Onyx.merge(ONYX_KEYS.TEST_KEY, change)); }) .then(waitForPromisesToResolve) .then(() => { + const valueInStorage = StorageMock.getItem(ONYX_KEYS.TEST_KEY); + + console.log('Result from Onyx (most likely from cache)\n', JSON.stringify(result, null, 2)); + console.log('Result in storage\n', JSON.stringify(valueInStorage, null, 2)); + expect(result).toEqual({ f: 'f', c: { - g: 'g', + h: 'h', }, }); - console.log('Result in storage\n', JSON.stringify(result, null, 2)); - // We would need to mock SQLite here to actually see what happens // The current storage mock will just use the "modifiedData" from Onyx and just set it instead of actually applying the delta change. - expect(StorageMock.getItem(ONYX_KEYS.TEST_KEY)).toEqual({ + expect(valueInStorage).toEqual({ f: 'f', c: { - g: 'g', + h: 'h', }, }); }); From b0365a693ec2e9ca2c23e5be3789065064a0d6b5 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 29 Mar 2024 12:27:28 +0100 Subject: [PATCH 3/6] fix: batched changes --- lib/Onyx.ts | 41 +++++++++------------ lib/storage/providers/IDBKeyValProvider.ts | 4 +- lib/storage/providers/MemoryOnlyProvider.ts | 4 +- lib/storage/providers/SQLiteProvider.ts | 8 +++- lib/storage/providers/types.ts | 6 +-- tests/unit/onyxTest.js | 19 ++++++---- 6 files changed, 43 insertions(+), 39 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 98b9413c2..075f81741 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -303,48 +303,43 @@ function merge(key: TKey, changes: OnyxEntry { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, modifiedData); + return Storage.mergeItem(key, batchedDeltaChanges, preMergedValue, shouldSetValue).then(() => { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, preMergedValue); return updatePromise; }); } catch (error) { diff --git a/lib/storage/providers/IDBKeyValProvider.ts b/lib/storage/providers/IDBKeyValProvider.ts index 3a385d228..f3539b500 100644 --- a/lib/storage/providers/IDBKeyValProvider.ts +++ b/lib/storage/providers/IDBKeyValProvider.ts @@ -40,9 +40,9 @@ const provider: StorageProvider = { return Promise.all(upsertMany); }); }), - mergeItem(key, _changes, modifiedData) { + mergeItem(key, _deltaChanges, preMergedValue) { // Since Onyx also merged the existing value with the changes, we can just set the value directly - return provider.setItem(key, modifiedData); + return provider.setItem(key, preMergedValue); }, multiSet: (pairs) => setMany(pairs, idbKeyValStore), clear: () => clear(idbKeyValStore), diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index 74d9e3a84..00583a55a 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -78,9 +78,9 @@ const provider: StorageProvider = { /** * Merging an existing value with a new one */ - mergeItem(key, _changes, modifiedData) { + mergeItem(key, _deltaChanges, preMergedValue) { // Since Onyx already merged the existing value with the changes, we can just set the value directly - return this.setItem(key, modifiedData); + return this.setItem(key, preMergedValue); }, /** diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index ad5324ea9..a4771a34c 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -76,8 +76,12 @@ const provider: StorageProvider = { return db.executeBatchAsync([[query, queryArguments]]); }, - mergeItem(key, changes) { - return this.multiMerge([[key, changes]]) as Promise; + mergeItem(key, deltaChanges, preMergedValue, shouldSetValue = false) { + if (shouldSetValue) { + return this.setItem(key, preMergedValue) as Promise; + } + + return this.multiMerge([[key, deltaChanges]]) as Promise; }, getAllKeys: () => db.executeAsync('SELECT record_key FROM keyvaluepairs;').then(({rows}) => { diff --git a/lib/storage/providers/types.ts b/lib/storage/providers/types.ts index 80cf88ba7..969118004 100644 --- a/lib/storage/providers/types.ts +++ b/lib/storage/providers/types.ts @@ -43,10 +43,10 @@ type StorageProvider = { /** * Merges an existing value with a new one by leveraging JSON_PATCH - * @param changes - the delta for a specific key - * @param modifiedData - the pre-merged data from `Onyx.applyMerge` + * @param deltaChanges - the delta for a specific key + * @param preMergedValue - the pre-merged data from `Onyx.applyMerge` */ - mergeItem: (key: TKey, changes: OnyxValue, modifiedData: OnyxValue) => Promise; + mergeItem: (key: TKey, deltaChanges: OnyxValue, preMergedValue: OnyxValue, shouldSetValue?: boolean) => Promise; /** * Returns all keys available in storage diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 617a237b5..b5fbe5a06 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -1075,6 +1075,7 @@ describe('Onyx', () => { it('(nested) nullish values should be removed when changes are batched during merge (SQLite)', () => { let result; + let valueInStorage; const initialData = { a: 'a', @@ -1084,20 +1085,23 @@ describe('Onyx', () => { e: 'e', }, }; - const change1 = null; - const change2 = { + const change1 = { + b: null, + }; + const change2 = null; + const change3 = { f: 'f', c: { g: 'g', h: 'h', }, }; - const change3 = { + const change4 = { c: { g: null, }, }; - const changes = [change1, change2, change3]; + const changes = [change1, change2, change3, change4]; const batchedChanges = OnyxUtils.applyMerge(undefined, changes, false); @@ -1113,12 +1117,13 @@ describe('Onyx', () => { .then(() => { expect(result).toEqual(initialData); - _.map(changes, (change) => Onyx.merge(ONYX_KEYS.TEST_KEY, change)); + return Promise.all(_.map(changes, (change) => Onyx.merge(ONYX_KEYS.TEST_KEY, change))); }) .then(waitForPromisesToResolve) .then(() => { - const valueInStorage = StorageMock.getItem(ONYX_KEYS.TEST_KEY); - + StorageMock.getItem(ONYX_KEYS.TEST_KEY).then((v) => (valueInStorage = v)); + }) + .then(() => { console.log('Result from Onyx (most likely from cache)\n', JSON.stringify(result, null, 2)); console.log('Result in storage\n', JSON.stringify(valueInStorage, null, 2)); From efa02509812a99dbc0860749fb899f7087382284 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 7 Apr 2024 13:30:32 +0200 Subject: [PATCH 4/6] fix: pass new param through storage layer --- lib/storage/index.ts | 4 ++-- lib/storage/providers/IDBKeyValProvider.ts | 2 +- lib/storage/providers/SQLiteProvider.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/storage/index.ts b/lib/storage/index.ts index cf8c575bc..c9b797b10 100644 --- a/lib/storage/index.ts +++ b/lib/storage/index.ts @@ -114,9 +114,9 @@ const Storage: Storage = { /** * Merging an existing value with a new one */ - mergeItem: (key, changes, modifiedData) => + mergeItem: (key, deltaChanges, preMergedValue, shouldSetValue = false) => tryOrDegradePerformance(() => { - const promise = provider.mergeItem(key, changes, modifiedData); + const promise = provider.mergeItem(key, deltaChanges, preMergedValue, shouldSetValue); if (shouldKeepInstancesSync) { return promise.then(() => InstanceSync.mergeItem(key)); diff --git a/lib/storage/providers/IDBKeyValProvider.ts b/lib/storage/providers/IDBKeyValProvider.ts index f3539b500..f13657800 100644 --- a/lib/storage/providers/IDBKeyValProvider.ts +++ b/lib/storage/providers/IDBKeyValProvider.ts @@ -40,7 +40,7 @@ const provider: StorageProvider = { return Promise.all(upsertMany); }); }), - mergeItem(key, _deltaChanges, preMergedValue) { + mergeItem(key, _deltaChanges, preMergedValue, _shouldSetValue) { // Since Onyx also merged the existing value with the changes, we can just set the value directly return provider.setItem(key, preMergedValue); }, diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index a4771a34c..461c4b8ca 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -76,7 +76,7 @@ const provider: StorageProvider = { return db.executeBatchAsync([[query, queryArguments]]); }, - mergeItem(key, deltaChanges, preMergedValue, shouldSetValue = false) { + mergeItem(key, deltaChanges, preMergedValue, shouldSetValue) { if (shouldSetValue) { return this.setItem(key, preMergedValue) as Promise; } From 9778ebebf7a4a3413166ed44dc16a2b66fd50a52 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 7 Apr 2024 13:38:33 +0200 Subject: [PATCH 5/6] remove test --- tests/unit/onyxTest.js | 72 ------------------------------------------ 1 file changed, 72 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index b5fbe5a06..b8199087a 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -1072,76 +1072,4 @@ describe('Onyx', () => { expect(testKeyValue).toEqual(null); }); }); - - it('(nested) nullish values should be removed when changes are batched during merge (SQLite)', () => { - let result; - let valueInStorage; - - const initialData = { - a: 'a', - b: 'b', - c: { - d: 'd', - e: 'e', - }, - }; - const change1 = { - b: null, - }; - const change2 = null; - const change3 = { - f: 'f', - c: { - g: 'g', - h: 'h', - }, - }; - const change4 = { - c: { - g: null, - }, - }; - const changes = [change1, change2, change3, change4]; - - const batchedChanges = OnyxUtils.applyMerge(undefined, changes, false); - - console.log('Batched changes\n', JSON.stringify(batchedChanges, null, 2)); - - connectionID = Onyx.connect({ - key: ONYX_KEYS.TEST_KEY, - initWithStoredValues: false, - callback: (value) => (result = value), - }); - - return Onyx.set(ONYX_KEYS.TEST_KEY, initialData) - .then(() => { - expect(result).toEqual(initialData); - - return Promise.all(_.map(changes, (change) => Onyx.merge(ONYX_KEYS.TEST_KEY, change))); - }) - .then(waitForPromisesToResolve) - .then(() => { - StorageMock.getItem(ONYX_KEYS.TEST_KEY).then((v) => (valueInStorage = v)); - }) - .then(() => { - console.log('Result from Onyx (most likely from cache)\n', JSON.stringify(result, null, 2)); - console.log('Result in storage\n', JSON.stringify(valueInStorage, null, 2)); - - expect(result).toEqual({ - f: 'f', - c: { - h: 'h', - }, - }); - - // We would need to mock SQLite here to actually see what happens - // The current storage mock will just use the "modifiedData" from Onyx and just set it instead of actually applying the delta change. - expect(valueInStorage).toEqual({ - f: 'f', - c: { - h: 'h', - }, - }); - }); - }); }); From ef2201eed3263e3bc2f47e8044b1c5a38bdd0e38 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 7 Apr 2024 13:39:08 +0200 Subject: [PATCH 6/6] add test for SQLite batched changes --- tests/unit/onyxTest.js | 72 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index b8199087a..b5fbe5a06 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -1072,4 +1072,76 @@ describe('Onyx', () => { expect(testKeyValue).toEqual(null); }); }); + + it('(nested) nullish values should be removed when changes are batched during merge (SQLite)', () => { + let result; + let valueInStorage; + + const initialData = { + a: 'a', + b: 'b', + c: { + d: 'd', + e: 'e', + }, + }; + const change1 = { + b: null, + }; + const change2 = null; + const change3 = { + f: 'f', + c: { + g: 'g', + h: 'h', + }, + }; + const change4 = { + c: { + g: null, + }, + }; + const changes = [change1, change2, change3, change4]; + + const batchedChanges = OnyxUtils.applyMerge(undefined, changes, false); + + console.log('Batched changes\n', JSON.stringify(batchedChanges, null, 2)); + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => (result = value), + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, initialData) + .then(() => { + expect(result).toEqual(initialData); + + return Promise.all(_.map(changes, (change) => Onyx.merge(ONYX_KEYS.TEST_KEY, change))); + }) + .then(waitForPromisesToResolve) + .then(() => { + StorageMock.getItem(ONYX_KEYS.TEST_KEY).then((v) => (valueInStorage = v)); + }) + .then(() => { + console.log('Result from Onyx (most likely from cache)\n', JSON.stringify(result, null, 2)); + console.log('Result in storage\n', JSON.stringify(valueInStorage, null, 2)); + + expect(result).toEqual({ + f: 'f', + c: { + h: 'h', + }, + }); + + // We would need to mock SQLite here to actually see what happens + // The current storage mock will just use the "modifiedData" from Onyx and just set it instead of actually applying the delta change. + expect(valueInStorage).toEqual({ + f: 'f', + c: { + h: 'h', + }, + }); + }); + }); });