[WIP] Add tests for different storage platforms with actual mocks (SQLite, idb-keyval)

lib/Onyx.ts

@@ -303,48 +303,43 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxEntry<NullishDeep<K
 
         try {
             // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge)
-            // We don't want to remove null values from the "batchedChanges", because SQLite uses them to remove keys from storage natively.
-            let batchedChanges = OnyxUtils.applyMerge(undefined, mergeQueue[key], false);
+            // We don't want to remove null values from the "batchedDeltaChanges", because SQLite uses them to remove keys from storage natively.
+            const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, mergeQueue[key], false);
 
-            // The presence of a `null` in the merge queue instructs us to drop the existing value.
+            // Case (1): When there is no existing value in storage, we want to set the value instead of merge it.
+            // Case (2): The presence of a top-level`null` in the merge queue instructs us to drop the whole existing value.
             // In this case, we can't simply merge the batched changes with the existing value, because then the null in the merge queue would have no effect
-            const shouldOverwriteExistingValue = mergeQueue[key].includes(null);
+            const shouldSetValue = !existingValue || mergeQueue[key].includes(null);
 
             // Clean up the write queue, so we don't apply these changes again
             delete mergeQueue[key];
             delete mergeQueuePromise[key];
 
             // If the batched changes equal null, we want to remove the key from storage, to reduce storage size
-            const {wasRemoved} = OnyxUtils.removeNullValues(key, batchedChanges);
-
-            // After that we merge the batched changes with the existing value
-            // We can remove null values from the "modifiedData", because "null" implicates that the user wants to remove a value from storage.
-            // The "modifiedData" will be directly "set" in storage instead of being merged
-            const modifiedData = shouldOverwriteExistingValue ? batchedChanges : OnyxUtils.applyMerge(existingValue, [batchedChanges], true);
-
-            // On native platforms we use SQLite which utilises JSON_PATCH to merge changes.
-            // JSON_PATCH generally removes null values from the stored object.
-            // When there is no existing value though, SQLite will just insert the changes as a new value and thus the null values won't be removed.
-            // Therefore we need to remove null values from the `batchedChanges` which are sent to the SQLite, if no existing value is present.
-            if (!existingValue) {
-                batchedChanges = OnyxUtils.applyMerge(undefined, [batchedChanges], true);
-            }
+            const {wasRemoved} = OnyxUtils.removeNullValues(key, batchedDeltaChanges);
+
+            // For providers that can't handle delta changes, we need to merge the batched changes with the existing value beforehand.
+            // The "preMergedValue" will be directly "set" in storage instead of being merged
+            // Therefore we merge the batched changes with the existing value to get the final merged value that will be stored.
+            // We can remove null values from the "preMergedValue", because "null" implicates that the user wants to remove a value from storage.
+            const preMergedValue = OnyxUtils.applyMerge(shouldSetValue ? undefined : existingValue, [batchedDeltaChanges], true);
 
-            const hasChanged = cache.hasValueChanged(key, modifiedData);
+            // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge.
+            const hasChanged = cache.hasValueChanged(key, preMergedValue);
 
             // Logging properties only since values could be sensitive things we don't want to log
-            Logger.logInfo(`merge called for key: ${key}${_.isObject(batchedChanges) ? ` properties: ${_.keys(batchedChanges).join(',')}` : ''} hasChanged: ${hasChanged}`);
+            Logger.logInfo(`merge called for key: ${key}${_.isObject(batchedDeltaChanges) ? ` properties: ${_.keys(batchedDeltaChanges).join(',')}` : ''} hasChanged: ${hasChanged}`);
 
             // This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
-            const updatePromise = OnyxUtils.broadcastUpdate(key, modifiedData, hasChanged, wasRemoved);
+            const updatePromise = OnyxUtils.broadcastUpdate(key, preMergedValue, hasChanged, wasRemoved);
 
             // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
             if (!hasChanged || wasRemoved) {
                 return updatePromise;
             }
 
-            return Storage.mergeItem(key, batchedChanges, modifiedData).then(() => {
-                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) {

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));

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),

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);
     },
 
     /**

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<BatchQueryResult>;
+    mergeItem(key, deltaChanges, preMergedValue, shouldSetValue) {
+        if (shouldSetValue) {
+            return this.setItem(key, preMergedValue) as Promise<BatchQueryResult>;
+        }
+
+        return this.multiMerge([[key, deltaChanges]]) as Promise<BatchQueryResult>;
     },
     getAllKeys: () =>
         db.executeAsync('SELECT record_key FROM keyvaluepairs;').then(({rows}) => {

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: <TKey extends OnyxKey>(key: TKey, changes: OnyxValue<TKey>, modifiedData: OnyxValue<TKey>) => Promise<BatchQueryResult | void>;
+    mergeItem: <TKey extends OnyxKey>(key: TKey, deltaChanges: OnyxValue<TKey>, preMergedValue: OnyxValue<TKey>, shouldSetValue?: boolean) => Promise<BatchQueryResult | void>;
 
     /**
      * Returns all keys available in storage

tests/unit/onyxTest.js

@@ -2,6 +2,7 @@
 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 @@
                 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',
+                    },
+                });
+            });
+    });
 });