Merge pull request #519 from margelo/@fix-nested-null-removal-inconsistency-between-single-and-multi-functions

tgolen · web-flow · commit 2e7bf82c292e · 2024-04-23T15:43:01.000-06:00
Fix inconsistencies between single key operations (`merge`, `set`) and multi key operations (`mergeCollection`, `multiSet`)
diff --git a/.gitignore b/.gitignore
@@ -18,5 +18,9 @@ dist/
 # Published package
 *.tgz
 
+# Yalc
+.yalc
+yalc.lock
+
 # Perf tests
-.reassure
+.reassure
diff --git a/lib/Onyx.ts b/lib/Onyx.ts
@@ -245,7 +245,7 @@ function set<TKey extends OnyxKey>(key: TKey, value: OnyxEntry<KeyValueMapping[T
  * @param data object keyed by ONYXKEYS and the values to set
  */
 function multiSet(data: Partial<NullableKeyValueMapping>): Promise<void> {
-    const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data);
+    const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data, true);
 
     const updatePromises = keyValuePairs.map(([key, value]) => {
         const prevValue = cache.getValue(key, false);
@@ -285,7 +285,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxEntry<NullishDeep<K
     const mergeQueuePromise = OnyxUtils.getMergeQueuePromise();
 
     // Top-level undefined values are ignored
-    // Therefore we need to prevent adding them to the merge queue
+    // Therefore, we need to prevent adding them to the merge queue
     if (changes === undefined) {
         return mergeQueue[key] ? mergeQueuePromise[key] : Promise.resolve();
     }
@@ -419,8 +419,15 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
                 obj[key] = mergedCollection[key];
                 return obj;
             }, {});
-            const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection);
-            const keyValuePairsForNewCollection = OnyxUtils.prepareKeyValuePairsForStorage(newCollection);
+
+            // When (multi-)merging the values with the existing values in storage,
+            // we don't want to remove nested null values from the data that we pass to the storage layer,
+            // because the storage layer uses them to remove nested keys from storage natively.
+            const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection, false);
+
+            // We can safely remove nested null values when using (multi-)set,
+            // because we will simply overwrite the existing values in storage.
+            const keyValuePairsForNewCollection = OnyxUtils.prepareKeyValuePairsForStorage(newCollection, true);
 
             const promises = [];
 
diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts
@@ -453,6 +453,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
         // We prepare the "cached collection" which is the entire collection + the new partial data that
         // was merged in via mergeCollection().
         const cachedCollection = getCachedCollection(collectionKey);
+        const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;
 
         // Regular Onyx.connect() subscriber found.
         if (typeof subscriber.callback === 'function') {
@@ -464,7 +465,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
             // send the whole cached collection.
             if (isSubscribedToCollectionKey) {
                 if (subscriber.waitForCollectionCallback) {
-                    subscriber.callback(cachedCollection);
+                    subscriber.callback(cachedCollectionWithoutNestedNulls);
                     continue;
                 }
 
@@ -473,7 +474,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
                 const dataKeys = Object.keys(partialCollection ?? {});
                 for (let j = 0; j < dataKeys.length; j++) {
                     const dataKey = dataKeys[j];
-                    subscriber.callback(cachedCollection[dataKey], dataKey);
+                    subscriber.callback(cachedCollectionWithoutNestedNulls[dataKey], dataKey);
                 }
                 continue;
             }
@@ -482,7 +483,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
             // notify them with the cached data for that key only.
             if (isSubscribedToCollectionMemberKey) {
                 const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
-                subscriberCallback(cachedCollection[subscriber.key], subscriber.key as TKey);
+                subscriberCallback(cachedCollectionWithoutNestedNulls[subscriber.key], subscriber.key as TKey);
                 continue;
             }
 
@@ -621,13 +622,16 @@ function keyChanged<TKey extends OnyxKey>(
             }
             if (isCollectionKey(subscriber.key) && subscriber.waitForCollectionCallback) {
                 const cachedCollection = getCachedCollection(subscriber.key);
-                cachedCollection[key] = data;
-                subscriber.callback(cachedCollection);
+                const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;
+
+                cachedCollectionWithoutNestedNulls[key] = data;
+                subscriber.callback(cachedCollectionWithoutNestedNulls);
                 continue;
             }
 
+            const dataWithoutNestedNulls = utils.removeNestedNullValues(data);
             const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
-            subscriberCallback(data, key);
+            subscriberCallback(dataWithoutNestedNulls, key);
             continue;
         }
 
@@ -752,7 +756,8 @@ function sendDataToConnection<TKey extends OnyxKey>(mapping: Mapping<TKey>, val:
         return;
     }
 
-    (mapping as DefaultConnectOptions<TKey>).callback?.(val, matchedKey as TKey);
+    const valuesWithoutNestedNulls = utils.removeNestedNullValues(val);
+    (mapping as DefaultConnectOptions<TKey>).callback?.(valuesWithoutNestedNulls, matchedKey as TKey);
 }
 
 /**
@@ -963,11 +968,12 @@ type RemoveNullValuesOutput = {
 
 /**
  * Removes a key from storage if the value is null.
- * Otherwise removes all nested null values in objects and returns the object
+ * Otherwise removes all nested null values in objects,
+ * if shouldRemoveNestedNulls is true and returns the object.
  *
  * @returns The value without null values and a boolean "wasRemoved", which indicates if the key got removed completely
  */
-function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>): RemoveNullValuesOutput {
+function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>, shouldRemoveNestedNulls = true): RemoveNullValuesOutput {
     if (value === null) {
         remove(key);
         return {value, wasRemoved: true};
@@ -976,7 +982,7 @@ function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>): RemoveNullVa
     // We can remove all null values in an object by merging it with itself
     // utils.fastMerge recursively goes through the object and removes all null values
     // Passing two identical objects as source and target to fastMerge will not change it, but only remove the null values
-    return {value: utils.removeNestedNullValues(value as Record<string, unknown>), wasRemoved: false};
+    return {value: shouldRemoveNestedNulls ? utils.removeNestedNullValues(value as Record<string, unknown>) : (value as Record<string, unknown>), wasRemoved: false};
 }
 
 /**
@@ -986,28 +992,24 @@ function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>): RemoveNullVa
 
 * @return an array of key - value pairs <[key, value]>
  */
-function prepareKeyValuePairsForStorage(data: Record<OnyxKey, OnyxValue<OnyxKey>>): Array<[OnyxKey, OnyxValue<OnyxKey>]> {
-    const keyValuePairs: Array<[OnyxKey, OnyxValue<OnyxKey>]> = [];
-
-    Object.entries(data).forEach(([key, value]) => {
-        const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value);
+function prepareKeyValuePairsForStorage(data: Record<OnyxKey, OnyxValue<OnyxKey>>, shouldRemoveNestedNulls: boolean): Array<[OnyxKey, OnyxValue<OnyxKey>]> {
+    return Object.entries(data).reduce<Array<[OnyxKey, OnyxValue<OnyxKey>]>>((pairs, [key, value]) => {
+        const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value, shouldRemoveNestedNulls);
 
-        if (wasRemoved) {
-            return;
+        if (!wasRemoved) {
+            pairs.push([key, valueAfterRemoving]);
         }
 
-        keyValuePairs.push([key, valueAfterRemoving]);
-    });
-
-    return keyValuePairs;
+        return pairs;
+    }, []);
 }
 
 /**
  * Merges an array of changes with an existing value
  *
  * @param changes Array of changes that should be applied to the existing value
  */
-function applyMerge(existingValue: OnyxValue<OnyxKey>, changes: Array<OnyxValue<OnyxKey>>, shouldRemoveNullObjectValues: boolean): OnyxValue<OnyxKey> {
+function applyMerge(existingValue: OnyxValue<OnyxKey>, changes: Array<OnyxValue<OnyxKey>>, shouldRemoveNestedNulls: boolean): OnyxValue<OnyxKey> {
     const lastChange = changes?.at(-1);
 
     if (Array.isArray(lastChange)) {
@@ -1017,7 +1019,7 @@ function applyMerge(existingValue: OnyxValue<OnyxKey>, changes: Array<OnyxValue<
     if (changes.some((change) => change && typeof change === 'object')) {
         // Object values are then merged one after the other
         return changes.reduce(
-            (modifiedData, change) => utils.fastMerge(modifiedData as Record<OnyxKey, unknown>, change as Record<OnyxKey, unknown>, shouldRemoveNullObjectValues),
+            (modifiedData, change) => utils.fastMerge(modifiedData as Record<OnyxKey, unknown>, change as Record<OnyxKey, unknown>, shouldRemoveNestedNulls),
             existingValue || {},
         );
     }
diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts
@@ -68,11 +68,7 @@ const provider: StorageProvider = {
     multiSet(pairs) {
         const setPromises = _.map(pairs, ([key, value]) => this.setItem(key, value));
 
-        return new Promise((resolve) => {
-            Promise.all(setPromises).then(() => {
-                resolve(undefined);
-            });
-        });
+        return Promise.all(setPromises).then(() => undefined);
     },
 
     /**
diff --git a/lib/utils.ts b/lib/utils.ts
@@ -24,15 +24,15 @@ function isMergeableObject(value: unknown): value is Record<string, unknown> {
  * Merges the source object into the target object.
  * @param target - The target object.
  * @param source - The source object.
- * @param shouldRemoveNullObjectValues - If true, null object values will be removed.
+ * @param shouldRemoveNestedNulls - If true, null object values will be removed.
  * @returns - The merged object.
  */
-function mergeObject<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject, shouldRemoveNullObjectValues = true): TObject {
+function mergeObject<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject, shouldRemoveNestedNulls = true): TObject {
     const destination: Record<string, unknown> = {};
 
     // First we want to copy over all keys from the target into the destination object,
     // in case "target" is a mergable object.
-    // If "shouldRemoveNullObjectValues" is true, we want to remove null values from the merged object
+    // If "shouldRemoveNestedNulls" is true, we want to remove null values from the merged object
     // and therefore we need to omit keys where either the source or target value is null.
     if (isMergeableObject(target)) {
         const targetKeys = Object.keys(target);
@@ -41,10 +41,10 @@ function mergeObject<TObject extends Record<string, unknown>>(target: TObject |
             const sourceValue = source?.[key];
             const targetValue = target?.[key];
 
-            // If "shouldRemoveNullObjectValues" is true, we want to remove null values from the merged object.
+            // If "shouldRemoveNestedNulls" is true, we want to remove null values from the merged object.
             // Therefore, if either target or source value is null, we want to prevent the key from being set.
             const isSourceOrTargetNull = targetValue === null || sourceValue === null;
-            const shouldOmitTargetKey = shouldRemoveNullObjectValues && isSourceOrTargetNull;
+            const shouldOmitTargetKey = shouldRemoveNestedNulls && isSourceOrTargetNull;
 
             if (!shouldOmitTargetKey) {
                 destination[key] = targetValue;
@@ -60,22 +60,22 @@ function mergeObject<TObject extends Record<string, unknown>>(target: TObject |
         const targetValue = target?.[key];
 
         // If undefined is passed as the source value for a key, we want to generally ignore it.
-        // If "shouldRemoveNullObjectValues" is set to true and the source value is null,
+        // If "shouldRemoveNestedNulls" is set to true and the source value is null,
         // we don't want to set/merge the source value into the merged object.
-        const shouldIgnoreNullSourceValue = shouldRemoveNullObjectValues && sourceValue === null;
+        const shouldIgnoreNullSourceValue = shouldRemoveNestedNulls && sourceValue === null;
         const shouldOmitSourceKey = sourceValue === undefined || shouldIgnoreNullSourceValue;
 
         if (!shouldOmitSourceKey) {
             // If the source value is a mergable object, we want to merge it into the target value.
-            // If "shouldRemoveNullObjectValues" is true, "fastMerge" will recursively
+            // If "shouldRemoveNestedNulls" is true, "fastMerge" will recursively
             // remove nested null values from the merged object.
             // If source value is any other value we need to set the source value it directly.
             if (isMergeableObject(sourceValue)) {
                 // If the target value is null or undefined, we need to fallback to an empty object,
                 // so that we can still use "fastMerge" to merge the source value,
                 // to ensure that nested null values are removed from the merged object.
                 const targetValueWithFallback = (targetValue ?? {}) as TObject;
-                destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNullObjectValues);
+                destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNestedNulls);
             } else {
                 destination[key] = sourceValue;
             }
@@ -86,30 +86,30 @@ function mergeObject<TObject extends Record<string, unknown>>(target: TObject |
 }
 
 /**
- * Merges two objects and removes null values if "shouldRemoveNullObjectValues" is set to true
+ * Merges two objects and removes null values if "shouldRemoveNestedNulls" is set to true
  *
  * We generally want to remove null 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 null values for "set" operations.
  */
-function fastMerge<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject | null, shouldRemoveNullObjectValues = true): TObject | null {
+function fastMerge<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject | null, shouldRemoveNestedNulls = true): TObject | null {
     // We have to ignore arrays and nullish values here,
     // otherwise "mergeObject" will throw an error,
     // because it expects an object as "source"
     if (Array.isArray(source) || source === null || source === undefined) {
         return source;
     }
 
-    return mergeObject(target, source, shouldRemoveNullObjectValues);
+    return mergeObject(target, source, shouldRemoveNestedNulls);
 }
 
 /** Deep removes the nested null values from the given value. */
-function removeNestedNullValues(value: unknown[] | Record<string, unknown>): Record<string, unknown> | unknown[] | null {
+function removeNestedNullValues<TObject extends Record<string, unknown>>(value: unknown | unknown[] | TObject): Record<string, unknown> | unknown[] | null {
     if (typeof value === 'object' && !Array.isArray(value)) {
-        return fastMerge(value, value);
+        return fastMerge(value as Record<string, unknown> | null, value as Record<string, unknown> | null);
     }
 
-    return value;
+    return value as Record<string, unknown> | null;
 }
 
 /** Formats the action name by uppercasing and adding the key if provided. */
diff --git a/lib/withOnyx.js b/lib/withOnyx.js
@@ -321,6 +321,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
                 // that should not be passed to a wrapped component
                 let stateToPass = _.omit(this.state, 'loading');
                 stateToPass = _.omit(stateToPass, _.isNull);
+                const stateToPassWithoutNestedNulls = utils.removeNestedNullValues(stateToPass);
 
                 // Spreading props and state is necessary in an HOC where the data cannot be predicted
                 return (
@@ -329,7 +330,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
                         // eslint-disable-next-line react/jsx-props-no-spreading
                         {...propsToPass}
                         // eslint-disable-next-line react/jsx-props-no-spreading
-                        {...stateToPass}
+                        {...stateToPassWithoutNestedNulls}
                         ref={this.props.forwardedRef}
                     />
                 );
diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js
@@ -1071,4 +1071,78 @@ describe('Onyx', () => {
                 expect(testKeyValue).toEqual(null);
             });
     });
+
+    it('should merge a non-existing key with a nested null removed', () => {
+        let testKeyValue;
+
+        connectionID = Onyx.connect({
+            key: ONYX_KEYS.TEST_KEY,
+            initWithStoredValues: false,
+            callback: (value) => {
+                testKeyValue = value;
+            },
+        });
+
+        return Onyx.merge(ONYX_KEYS.TEST_KEY, {
+            waypoints: {
+                1: 'Home',
+                2: 'Work',
+                3: null,
+            },
+        }).then(() => {
+            expect(testKeyValue).toEqual({
+                waypoints: {
+                    1: 'Home',
+                    2: 'Work',
+                },
+            });
+        });
+    });
+
+    it('mergeCollection should omit nested null values', () => {
+        let result;
+
+        const routineRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}routine`;
+        const holidayRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}holiday`;
+
+        connectionID = Onyx.connect({
+            key: ONYX_KEYS.COLLECTION.TEST_KEY,
+            initWithStoredValues: false,
+            callback: (value) => (result = value),
+            waitForCollectionCallback: true,
+        });
+
+        return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
+            [routineRoute]: {
+                waypoints: {
+                    1: 'Home',
+                    2: 'Work',
+                    3: 'Gym',
+                },
+            },
+            [holidayRoute]: {
+                waypoints: {
+                    1: 'Home',
+                    2: 'Beach',
+                    3: null,
+                },
+            },
+        }).then(() => {
+            expect(result).toEqual({
+                [routineRoute]: {
+                    waypoints: {
+                        1: 'Home',
+                        2: 'Work',
+                        3: 'Gym',
+                    },
+                },
+                [holidayRoute]: {
+                    waypoints: {
+                        1: 'Home',
+                        2: 'Beach',
+                    },
+                },
+            });
+        });
+    });
 });
