Merge pull request #333 from margelo/@chrispader/investigate-idb-keyv…

…al-problems

Fix: nullish keys not removed/merged in IDB-Keyval (web)

lib/Onyx.js

@@ -8,6 +8,7 @@ import createDeferredTask from './createDeferredTask';
 import fastMerge from './fastMerge';
 import * as PerformanceUtils from './metrics/PerformanceUtils';
 import Storage from './storage';
+import Utils from './utils';
 import unstable_batchedUpdates from './batch';
 
 // Method constants
@@ -990,24 +991,6 @@ function hasPendingMergeForKey(key) {
     return Boolean(mergeQueue[key]);
 }
 
-/**
- * We generally want to remove top-level nullish values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
- * On native, when merging an existing value with new changes, SQLite will use  JSON_PATCH, which removes top-level nullish values.
- * To be consistent with the behaviour for merge, we'll also want to remove nullish values for "set" operations.
- * On web, IndexedDB will keep the top-level keys along with a null value and this uses up storage and memory.
- * This method will ensure that keys for null values are removed before an object is written to disk and cache so that all platforms are storing the data in the same efficient way.
- * @private
- * @param {*} value
- * @returns {*}
- */
-function removeNullObjectValues(value) {
-    if (_.isArray(value) || !_.isObject(value)) {
-        return value;
-    }
-
-    return _.omit(value, objectValue => _.isNull(objectValue));
-}
-
 /**
  * Write a value to our store with the given key
  *
@@ -1025,7 +1008,7 @@ function set(key, value) {
         Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`);
     }
 
-    const valueWithNullRemoved = removeNullObjectValues(value);
+    const valueWithNullRemoved = Utils.removeNullObjectValues(value);
 
     const hasChanged = cache.hasValueChanged(key, valueWithNullRemoved);
 
@@ -1144,14 +1127,14 @@ function merge(key, changes) {
                 delete mergeQueuePromise[key];
 
                 // After that we merge the batched changes with the existing value
-                const modifiedData = removeNullObjectValues(applyMerge(existingValue, [batchedChanges]));
+                const modifiedData = Utils.removeNullObjectValues(applyMerge(existingValue, [batchedChanges]));
 
                 // 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.
                 // When there is no existing value though, SQLite will just insert the changes as a new value and thus the top-level nullish values won't be removed.
                 // Therefore we need to remove nullish values from the `batchedChanges` which are sent to the SQLite, if no existing value is present.
                 if (!existingValue) {
-                    batchedChanges = removeNullObjectValues(batchedChanges);
+                    batchedChanges = Utils.removeNullObjectValues(batchedChanges);
                 }
 
                 const hasChanged = cache.hasValueChanged(key, modifiedData);

lib/fastMerge.js

@@ -1,3 +1,5 @@
+import _ from 'underscore';
+
 // Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1
 
 /**
@@ -54,10 +56,7 @@ 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
-    const array = Array.isArray(source);
-    if (array) {
+    if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) {
         return source;
     }
     return mergeObject(target, source);

lib/storage/providers/IDBKeyVal.js

@@ -12,6 +12,7 @@ import {
 } from 'idb-keyval';
 import _ from 'underscore';
 import fastMerge from '../../fastMerge';
+import Utils from '../../utils';
 
 // We don't want to initialize the store while the JS bundle loads as idb-keyval will try to use global.indexedDB
 // which might not be available in certain environments that load the bundle (e.g. electron main process).
@@ -56,7 +57,7 @@ const provider = {
             const upsertMany = _.map(pairs, ([key, value], index) => {
                 const prev = values[index];
                 const newValue = _.isObject(prev) ? fastMerge(prev, value) : value;
-                return promisifyRequest(store.put(newValue, key));
+                return promisifyRequest(store.put(Utils.removeNullObjectValues(newValue), key));
             });
             return Promise.all(upsertMany);
         });

lib/utils.js

@@ -0,0 +1,23 @@
+import _ from 'underscore';
+
+/**
+ * We generally want to remove top-level nullish values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
+ * On native, when merging an existing value with new changes, SQLite will use  JSON_PATCH, which removes top-level nullish values.
+ * To be consistent with the behaviour for merge, we'll also want to remove nullish values for "set" operations.
+ * On web, IndexedDB will keep the top-level keys along with a null value and this uses up storage and memory.
+ * This method will ensure that keys for null values are removed before an object is written to disk and cache so that all platforms are storing the data in the same efficient way.
+ * @private
+ * @param {*} value
+ * @returns {*}
+ */
+function removeNullObjectValues(value) {
+    if (_.isArray(value) || !_.isObject(value)) {
+        return value;
+    }
+
+    const objectWithoutNullObjectValues = _.omit(value, objectValue => _.isNull(objectValue));
+
+    return objectWithoutNullObjectValues;
+}
+
+export default {removeNullObjectValues};

tests/unit/onyxTest.js

@@ -202,6 +202,48 @@ describe('Onyx', () => {
         });
     });
 
+    it('should merge an object with 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'});
+                return Onyx.merge(ONYX_KEYS.TEST_KEY, undefined);
+            })
+            .then(() => {
+                expect(testKeyValue).toEqual(undefined);
+            });
+    });
+
+    it('should remove top-level keys that are set to null/undefined when merging', () => {
+        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', test2: 'test2'})
+            .then(() => {
+                expect(testKeyValue).toEqual({test1: 'test1', test2: 'test2'});
+                return Onyx.merge(ONYX_KEYS.TEST_KEY, {test1: null});
+            })
+            .then(() => {
+                expect(testKeyValue).toEqual({test2: 'test2'});
+            });
+    });
+
     it('should overwrite an array key nested inside an object', () => {
         let testKeyValue;
         connectionID = Onyx.connect({