Remove unconditional null pruning from Onyx write operations

lib/OnyxCache.ts

@@ -226,7 +226,6 @@ class OnyxCache {
 
         this.storageMap = {
             ...utils.fastMerge(this.storageMap, data, {
-                shouldRemoveNestedNulls: true,
                 objectRemovalMode: 'replace',
             }).result,
         };

lib/OnyxUtils.ts

@@ -1018,7 +1018,6 @@
  */
 function prepareKeyValuePairsForStorage(
     data: Record<OnyxKey, OnyxInput<OnyxKey>>,
-    shouldRemoveNestedNulls?: boolean,
     replaceNullPatches?: MultiMergeReplaceNullPatches,
     isProcessingCollectionUpdate?: boolean,
 ): StorageKeyValuePair[] {
@@ -1030,10 +1029,8 @@
             continue;
         }
 
-        const valueWithoutNestedNullValues = shouldRemoveNestedNulls ?? true ? utils.removeNestedNullValues(value) : value;
-
-        if (valueWithoutNestedNullValues !== undefined) {
-            pairs.push([key, valueWithoutNestedNullValues, replaceNullPatches?.[key]]);
+        if (value !== undefined) {
+            pairs.push([key, value, replaceNullPatches?.[key]]);
         }
     }
 
@@ -1085,7 +1082,7 @@
         // Object values are then merged one after the other
         return changes.reduce<FastMergeResult<TChange>>(
             (modifiedData, change) => {
-                const options: FastMergeOptions = mode === 'merge' ? {shouldRemoveNestedNulls: true, objectRemovalMode: 'replace'} : {objectRemovalMode: 'mark'};
+                const options: FastMergeOptions = mode === 'merge' ? {objectRemovalMode: 'replace'} : {objectRemovalMode: 'mark'};
                 const {result, replaceNullPatches} = utils.fastMerge(modifiedData.result, change, options);
 
                 // eslint-disable-next-line no-param-reassign
@@ -1117,9 +1114,7 @@
     return Storage.multiGet(keysToFetch).then((pairs) => {
         const existingDataAsObject = Object.fromEntries(pairs) as Record<string, unknown>;
 
-        const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, {
-            shouldRemoveNestedNulls: true,
-        }).result;
+        const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates).result;
         cache.merge(merged ?? {});
 
         for (const [key, value] of Object.entries(merged ?? {})) keyChanged(key, value);
@@ -1393,13 +1388,12 @@
         return Promise.resolve();
     }
 
-    const valueWithoutNestedNullValues = utils.removeNestedNullValues(value) as OnyxValue<TKey>;
-    const hasChanged = options?.skipCacheCheck ? true : cache.hasValueChanged(key, valueWithoutNestedNullValues);
+    const hasChanged = options?.skipCacheCheck ? true : cache.hasValueChanged(key, value);
 
     OnyxUtils.logKeyChanged(OnyxUtils.METHOD.SET, key, value, hasChanged);
 
     // This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
-    const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged);
+    const updatePromise = OnyxUtils.broadcastUpdate(key, value, hasChanged);
 
     // If the value has not changed and this isn't a retry attempt, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
     if (!hasChanged && !retryAttempt) {
@@ -1408,14 +1402,14 @@
 
     // If a key is a RAM-only key or a member of RAM-only collection, we skip the step that modifies the storage
     if (isRamOnlyKey(key)) {
-        OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues);
+        OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, value);
         return updatePromise;
     }
 
-    return Storage.setItem(key, valueWithoutNestedNullValues)
-        .catch((error) => OnyxUtils.retryOperation(error, setWithRetry, {key, value: valueWithoutNestedNullValues, options}, retryAttempt))
+    return Storage.setItem(key, value)
+        .catch((error) => OnyxUtils.retryOperation(error, setWithRetry, {key, value, options}, retryAttempt))
         .then(() => {
-            OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues);
+            OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, value);
             return updatePromise;
         });
 }
@@ -1448,7 +1442,7 @@
         }, {});
     }
 
-    const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true);
+    const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData);
 
     const updatePromises = keyValuePairsToSet.map(([key, value]) => {
         // When we use multiSet to set a key we want to clear the current delta changes from Onyx.merge that were queued
@@ -1530,7 +1524,7 @@
             mutableCollection[key] = null;
         }
 
-        const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true);
+        const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, undefined, true);
         const previousCollection = OnyxUtils.getCachedCollection(collectionKey);
 
         for (const [key, value] of keyValuePairs) cache.set(key, value);
@@ -1636,14 +1630,8 @@
                 newCollection[key] = resultCollection[key];
             }
 
-            // 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 = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches);
-
-            // We can safely remove nested null values when using (multi-)set,
-            // because we will simply overwrite the existing values in storage.
-            const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection, true);
+            const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection, mergeReplaceNullPatches);
+            const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection);
 
             const promises = [];
 
@@ -1732,7 +1720,7 @@
         const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection};
         const existingKeys = resultCollectionKeys.filter((key) => persistedKeys.has(key));
         const previousCollection = getCachedCollection(collectionKey, existingKeys);
-        const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true);
+        const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, undefined, true);
 
         for (const [key, value] of keyValuePairs) cache.set(key, value);
 

lib/storage/providers/IDBKeyValProvider/index.ts

@@ -62,7 +62,6 @@ const provider: StorageProvider<UseStore | undefined> = {
                         store.delete(key);
                     } else {
                         const newValue = utils.fastMerge(values[index] as Record<string, unknown>, value as Record<string, unknown>, {
-                            shouldRemoveNestedNulls: true,
                             objectRemovalMode: 'replace',
                         }).result;
 

lib/storage/providers/MemoryOnlyProvider.ts

@@ -82,14 +82,12 @@ const provider: StorageProvider<Store> = {
 
     /**
      * Multiple merging of existing and new values in a batch
-     * This function also removes all nested null values from an object.
      */
     multiMerge(pairs) {
         for (const [key, value] of pairs) {
             const existingValue = provider.store[key] as Record<string, unknown>;
 
             const newValue = utils.fastMerge(existingValue, value as Record<string, unknown>, {
-                shouldRemoveNestedNulls: true,
                 objectRemovalMode: 'replace',
             }).result;
 

lib/utils.ts

@@ -1,4 +1,4 @@
-import type {OnyxInput, OnyxKey} from './types';
+import type {OnyxKey} from './types';
 
 type EmptyObject = Record<string, never>;
 type EmptyValue = EmptyObject | null | undefined;
@@ -13,9 +13,6 @@ type EmptyValue = EmptyObject | null | undefined;
 type FastMergeReplaceNullPatch = [string[], unknown];
 
 type FastMergeOptions = {
-    /** If true, null object values will be removed. */
-    shouldRemoveNestedNulls?: boolean;
-
     /**
      * If set to "mark", we will mark objects that are set to null instead of simply removing them,
      * so that we can batch changes together, without losing information about the object removal.
@@ -40,9 +37,7 @@ type FastMergeResult<TValue> = {
 const ONYX_INTERNALS__REPLACE_OBJECT_MARK = 'ONYX_INTERNALS__REPLACE_OBJECT_MARK';
 
 /**
- * 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.
+ * Merges two objects.
  */
 function fastMerge<TValue>(target: TValue, source: TValue, options?: FastMergeOptions, metadata?: FastMergeMetadata, basePath: string[] = []): FastMergeResult<TValue> {
     if (!metadata) {
@@ -60,7 +55,6 @@ function fastMerge<TValue>(target: TValue, source: TValue, options?: FastMergeOp
     }
 
     const optionsWithDefaults: FastMergeOptions = {
-        shouldRemoveNestedNulls: options?.shouldRemoveNestedNulls ?? false,
         objectRemovalMode: options?.objectRemovalMode ?? 'none',
     };
 
@@ -91,18 +85,11 @@ function mergeObject<TObject extends 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 "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 (targetObject) {
         for (const key of Object.keys(targetObject)) {
             const targetProperty = targetObject?.[key];
-            const sourceProperty = source?.[key];
-
-            // If "shouldRemoveNestedNulls" is true, we want to remove (nested) null values from the merged object.
-            // If either the source or target value is null, we want to omit the key from the merged object.
-            const shouldOmitNullishProperty = options.shouldRemoveNestedNulls && (targetProperty === null || sourceProperty === null);
 
-            if (targetProperty === undefined || shouldOmitNullishProperty) {
+            if (targetProperty === undefined) {
                 continue;
             }
 
@@ -115,11 +102,7 @@ function mergeObject<TObject extends Record<string, unknown>>(
         let targetProperty = targetObject?.[key];
         const sourceProperty = source?.[key] as Record<string, unknown>;
 
-        // If "shouldRemoveNestedNulls" is true, we want to remove (nested) null values from the merged object.
-        // If the source value is null, we want to omit the key from the merged object.
-        const shouldOmitNullishProperty = options.shouldRemoveNestedNulls && sourceProperty === null;
-
-        if (sourceProperty === undefined || shouldOmitNullishProperty) {
+        if (sourceProperty === undefined) {
             continue;
         }
 
@@ -170,37 +153,6 @@ function isMergeableObject<TObject extends Record<string, unknown>>(value: unkno
     return isNonNullObject && !(value instanceof RegExp) && !(value instanceof Date) && !Array.isArray(value);
 }
 
-/** Deep removes the nested null values from the given value. */
-function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value: TValue): TValue {
-    if (value === null || value === undefined || typeof value !== 'object') {
-        return value;
-    }
-
-    if (Array.isArray(value)) {
-        return [...value] as TValue;
-    }
-
-    const result: Record<string, unknown> = {};
-
-    // eslint-disable-next-line no-restricted-syntax, guard-for-in
-    for (const key in value) {
-        const propertyValue = value[key];
-
-        if (propertyValue === null || propertyValue === undefined) {
-            continue;
-        }
-
-        if (typeof propertyValue === 'object' && !Array.isArray(propertyValue)) {
-            const valueWithoutNestedNulls = removeNestedNullValues(propertyValue);
-            result[key] = valueWithoutNestedNulls;
-        } else {
-            result[key] = propertyValue;
-        }
-    }
-
-    return result as TValue;
-}
-
 /** Formats the action name by uppercasing and adding the key if provided. */
 function formatActionName(method: string, key?: OnyxKey): string {
     return key ? `${method.toUpperCase()}/${key}` : method.toUpperCase();
@@ -284,7 +236,6 @@ export default {
     fastMerge,
     isEmptyObject,
     formatActionName,
-    removeNestedNullValues,
     checkCompatibilityWithExistingValue,
     pick,
     omit,

tests/perf-test/utils.perf-test.ts

@@ -23,11 +23,7 @@ describe('utils', () => {
         test('one call', async () => {
             const target = getRandomReportActions(collectionKey, 1000);
             const source = getRandomReportActions(collectionKey, 500);
-            await measureFunction(() =>
-                utils.fastMerge(target, source, {
-                    shouldRemoveNestedNulls: true,
-                }),
-            );
+            await measureFunction(() => utils.fastMerge(target, source));
         });
     });
 
@@ -37,19 +33,6 @@ describe('utils', () => {
         });
     });
 
-    describe('removeNestedNullValues', () => {
-        test('one call', async () => {
-            const reportAction = createRandomReportAction(0);
-            reportAction.actorAccountID = null;
-            reportAction.person.push(null);
-            reportAction.message[0].style = null;
-            reportAction.originalMessage.whisperedTo = null;
-            reportAction.lastModified = null;
-
-            await measureFunction(() => utils.removeNestedNullValues(reportAction));
-        });
-    });
-
     describe('checkCompatibilityWithExistingValue', () => {
         test('one call', async () => {
             const value = {};

tests/unit/fastMergeTest.ts

@@ -27,15 +27,6 @@ const testObjectWithNullishValues: DeepObject = {
     },
 };
 
-const testObjectWithNullValuesRemoved: DeepObject = {
-    b: {
-        c: {
-            h: 'h',
-        },
-        d: {},
-    },
-};
-
 const testMergeChanges: DeepObject[] = [
     {
         b: {
@@ -78,24 +69,7 @@ describe('fastMerge', () => {
     });
 
     describe('objects', () => {
-        it('should merge an object with another object and remove nested null values', () => {
-            const result = utils.fastMerge(testObject, testObjectWithNullishValues, {shouldRemoveNestedNulls: true});
-
-            expect(result.result).toEqual({
-                a: 'a',
-                b: {
-                    c: {
-                        h: 'h',
-                    },
-                    d: {
-                        f: 'f',
-                    },
-                    g: 'g',
-                },
-            });
-        });
-
-        it('should merge an object with another object and not remove nested null values', () => {
+        it('should merge an object with another object and keep nested null values', () => {
             const result = utils.fastMerge(testObject, testObjectWithNullishValues);
 
             expect(result.result).toEqual({
@@ -113,20 +87,6 @@ describe('fastMerge', () => {
             });
         });
 
-        it('should merge an object with an empty object and remove deeply nested null values', () => {
-            const result = utils.fastMerge({}, testObjectWithNullishValues, {
-                shouldRemoveNestedNulls: true,
-            });
-
-            expect(result.result).toEqual(testObjectWithNullValuesRemoved);
-        });
-
-        it('should remove null values by merging two identical objects with fastMerge', () => {
-            const result = utils.removeNestedNullValues(testObjectWithNullishValues);
-
-            expect(result).toEqual(testObjectWithNullValuesRemoved);
-        });
-
         it('should replace Date objects', () => {
             const oldDate = new Date('2024-01-01');
             const newDate = new Date('2025-01-01');
@@ -143,7 +103,6 @@ describe('fastMerge', () => {
 
         it('should add the "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag to the merged object when the change is set to null and "objectRemovalMode" is set to "mark"', () => {
             const result = utils.fastMerge(testMergeChanges[1], testMergeChanges[0], {
-                shouldRemoveNestedNulls: true,
                 objectRemovalMode: 'mark',
             });
 
@@ -172,7 +131,6 @@ describe('fastMerge', () => {
                     },
                 },
                 {
-                    shouldRemoveNestedNulls: true,
                     objectRemovalMode: 'replace',
                 },
             );

tests/unit/onyxCacheTest.tsx

@@ -352,13 +352,13 @@ describe('Onyx', () => {
                 expect(() => cache.merge({})).not.toThrow();
             });
 
-            it('Should remove `null` values when merging', () => {
+            it('Should keep `null` values when merging', () => {
                 cache.set('mockKey', {ID: 5});
                 cache.set('mockNullKey', null);
 
                 cache.merge({mockKey: null});
 
-                expect(cache.get('mockKey')).toEqual(undefined);
+                expect(cache.get('mockKey')).toEqual(null);
                 expect(cache.get('mockNullKey')).toEqual(undefined);
             });
         });