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/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 3a385d228..f13657800 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, _shouldSetValue) { // 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..461c4b8ca 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) { + 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 a75fd6bb1..b5fbe5a06 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,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', + }, + }); + }); + }); });