Ignore undefined values in merge queue when merging objects

lib/Onyx.js

@@ -1082,14 +1082,12 @@ function multiSet(data) {
 function applyMerge(existingValue, changes) {
     const lastChange = _.last(changes);
 
-    if (_.isArray(existingValue) || _.isArray(lastChange)) {
+    if (_.isArray(lastChange)) {
         return lastChange;
     }
 
-    if (_.isObject(existingValue) || _.every(changes, _.isObject)) {
-        // Object values are merged one after the other
-        // lodash adds a small overhead so we don't use it here
-        // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method
+    if (_.some(changes, _.isObject)) {
+        // Object values are then merged one after the other
         return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change),
             existingValue || {});
     }
@@ -1120,6 +1118,12 @@ function applyMerge(existingValue, changes) {
  * @returns {Promise}
  */
 function merge(key, changes) {
+    // Top-level undefined values are ignored
+    // Therefore we need to prevent adding them to the merge queue
+    if (_.isUndefined(changes)) {
+        return mergeQueue[key] ? mergeQueuePromise[key] : Promise.resolve();
+    }
+
     // Merge attempts are batched together. The delta should be applied after a single call to get() to prevent a race condition.
     // Using the initial value from storage in subsequent merge attempts will lead to an incorrect final merged value.
     if (mergeQueue[key]) {
@@ -1134,12 +1138,21 @@ function merge(key, changes) {
                 // 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)
                 let batchedChanges = applyMerge(undefined, mergeQueue[key]);
 
+                if (_.isNull(batchedChanges)) {
+                    return remove(key);
+                }
+
+                // The presence of a `null` in the merge queue instructs us to drop the 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 = _.includes(mergeQueue[key], null);
+
                 // Clean up the write queue, so we don't apply these changes again
                 delete mergeQueue[key];
                 delete mergeQueuePromise[key];
 
                 // After that we merge the batched changes with the existing value
-                const modifiedData = utils.removeNullObjectValues(applyMerge(existingValue, [batchedChanges]));
+                const updatedValue = shouldOverwriteExistingValue ? batchedChanges : applyMerge(existingValue, [batchedChanges]);
+                const modifiedData = utils.removeNullObjectValues(updatedValue);
 
                 // On native platforms we use SQLite which utilises JSON_PATCH to merge changes.
                 // JSON_PATCH generally removes top-level nullish values from the stored object.

lib/utils.js

@@ -65,8 +65,9 @@ function mergeObject(target, source) {
 * @returns {Object|Array}
 */
 function fastMerge(target, source) {
-    // lodash adds a small overhead so we don't use it here
-    // eslint-disable-next-line rulesdir/prefer-underscore-method
+    // We have to ignore arrays and nullish values here,
+    // otherwise "mergeObject" will throw an error,
+    // because it expects an object as "source"
     if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) {
         return source;
     }

tests/unit/onyxTest.js

@@ -202,7 +202,7 @@ describe('Onyx', () => {
         });
     });
 
-    it('should merge an object with undefined', () => {
+    it('should ignore top-level undefined values', () => {
         let testKeyValue;
 
         connectionID = Onyx.connect({
@@ -219,7 +219,7 @@ describe('Onyx', () => {
                 return Onyx.merge(ONYX_KEYS.TEST_KEY, undefined);
             })
             .then(() => {
-                expect(testKeyValue).toEqual(undefined);
+                expect(testKeyValue).toEqual({test1: 'test1'});
             });
     });
 
@@ -848,4 +848,59 @@ describe('Onyx', () => {
                 Onyx.disconnect(connectionIDs);
             });
     });
+
+    it('should merge an object with a batch of objects and undefined', () => {
+        let testKeyValue;
+
+        connectionID = Onyx.connect({
+            key: ONYX_KEYS.TEST_KEY,
+            initWithStoredValues: false,
+            callback: (value) => {
+                testKeyValue = value;
+            },
+        });
+
+        return Onyx.set(ONYX_KEYS.TEST_KEY, {test1: 'test1'})
+            .then(() => {
+                expect(testKeyValue).toEqual({test1: 'test1'});
+                Onyx.merge(ONYX_KEYS.TEST_KEY, {test2: 'test2'});
+                Onyx.merge(ONYX_KEYS.TEST_KEY, {test3: 'test3'});
+                Onyx.merge(ONYX_KEYS.TEST_KEY, undefined);
+                Onyx.merge(ONYX_KEYS.TEST_KEY, {test4: 'test4'});
+                Onyx.merge(ONYX_KEYS.TEST_KEY, undefined);
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                expect(testKeyValue).toEqual({
+                    test1: 'test1', test2: 'test2', test3: 'test3', test4: 'test4',
+                });
+            });
+    });
+
+    it('should merge an object with null and overwrite the value', () => {
+        let testKeyValue;
+
+        connectionID = Onyx.connect({
+            key: ONYX_KEYS.TEST_KEY,
+            initWithStoredValues: false,
+            callback: (value) => {
+                testKeyValue = value;
+            },
+        });
+
+        return Onyx.set(ONYX_KEYS.TEST_KEY, {test1: 'test1'})
+            .then(() => {
+                expect(testKeyValue).toEqual({test1: 'test1'});
+                Onyx.merge(ONYX_KEYS.TEST_KEY, null);
+                Onyx.merge(ONYX_KEYS.TEST_KEY, {test2: 'test2'});
+                Onyx.merge(ONYX_KEYS.TEST_KEY, {test3: 'test3'});
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                expect(testKeyValue).toEqual({
+                    test2: 'test2',
+                    test3: 'test3',
+                });
+            });
+    });
 });