Skip unchanged cached subtrees when removing nested nulls

lib/Onyx.ts

@@ -64,13 +64,15 @@ function init({
             const individual: Array<[OnyxKey, OnyxEntry<KeyValueMapping[OnyxKey]>]> = [];
             const collectionBatches = new Map<string, {partial: NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>>; previous: NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>>}>();
 
-            for (const [key, value] of pairs) {
+            for (const [key, valueFromStorage] of pairs) {
                 // RAM-only keys should never sync from storage as they may have stale persisted data
                 // from before the key was migrated to RAM-only.
                 if (OnyxKeys.isRamOnlyKey(key)) {
                     continue;
                 }
 
+                const value = utils.needsNormalization(valueFromStorage) ? utils.removeNestedNullValues(valueFromStorage) : valueFromStorage;
+
                 const collectionKey = OnyxKeys.getCollectionKey(key);
                 const isCollectionMember = !!collectionKey && OnyxKeys.isCollectionMemberKey(collectionKey, key);
 
@@ -489,7 +491,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
                         //      since collection key subscribers need to be updated differently
                         if (!isKeyToPreserve) {
                             const oldValue = cache.get(key);
-                            const newValue = defaultKeyStates[key] ?? null;
+                            const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null);
                             if (newValue !== oldValue) {
                                 cache.set(key, newValue);
 

lib/OnyxUtils.ts

@@ -402,10 +402,15 @@ function multiGet<TKey extends OnyxKey>(keys: CollectionKeyBase[]): Promise<Map<
                         continue;
                     }
 
-                    dataMap.set(key, value as OnyxValue<TKey>);
                     temp[key] = value as OnyxValue<TKey>;
                 }
                 cache.merge(temp);
+
+                // Return the cached values, which have nested nulls removed.
+                for (const key of Object.keys(temp)) {
+                    const cachedValue = cache.get(key);
+                    dataMap.set(key as TKey, (cachedValue !== undefined ? cachedValue : temp[key]) as OnyxValue<TKey>);
+                }
                 return dataMap;
             })
     );
@@ -781,7 +786,7 @@ function prepareKeyValuePairsForStorage(
             continue;
         }
 
-        const valueWithoutNestedNullValues = shouldRemoveNestedNulls ?? true ? utils.removeNestedNullValues(value) : value;
+        const valueWithoutNestedNullValues = shouldRemoveNestedNulls ?? true ? utils.removeNestedNullValues(value, cache.get(key)) : value;
 
         if (valueWithoutNestedNullValues !== undefined) {
             pairs.push([key, valueWithoutNestedNullValues, replaceNullPatches?.[key]]);
@@ -1119,7 +1124,7 @@ function setWithRetry<TKey extends OnyxKey>({key, value, options}: SetParams<TKe
         return Promise.resolve();
     }
 
-    const valueWithoutNestedNullValues = utils.removeNestedNullValues(value) as OnyxValue<TKey>;
+    const valueWithoutNestedNullValues = utils.removeNestedNullValues(value, existingValue) as OnyxValue<TKey>;
     const hasChanged = options?.skipCacheCheck ? true : cache.hasValueChanged(key, valueWithoutNestedNullValues);
 
     OnyxUtils.logKeyChanged(OnyxUtils.METHOD.SET, key, value, hasChanged);

lib/storage/providers/IDBKeyValProvider/index.ts

@@ -154,7 +154,14 @@ const provider: StorageProvider<UseStore | undefined> = {
         return (
             IDB.get(key, provider.store)
                 // idb-keyval returns undefined for missing items, but this needs to return null so that idb-keyval does the same thing as SQLiteStorage.
-                .then((val) => (val === undefined ? null : val))
+                .then((val) => {
+                    if (val === undefined) {
+                        return null;
+                    }
+
+                    // Stored values can hold nested nulls, but values handed to the cache must not.
+                    return utils.needsNormalization(val) ? utils.removeNestedNullValues(val) : val;
+                })
         );
     },
     removeItem(key) {

lib/storage/providers/SQLiteProvider.ts

@@ -145,7 +145,10 @@ const provider: StorageProvider<NitroSQLiteConnection | undefined> = {
                 return null;
             }
 
-            return JSON.parse(result.valueJSON);
+            const value = JSON.parse(result.valueJSON);
+
+            // valueJSON is a number for numeric values, so the type is checked before includes().
+            return typeof value === 'object' && value !== null && result.valueJSON.includes('null') ? utils.removeNestedNullValues(value) : value;
         });
     },
     multiGet(keys) {

lib/utils.ts

@@ -1,4 +1,5 @@
 import type {OnyxInput, OnyxKey} from './types';
+import * as Logger from './Logger';
 
 type EmptyObject = Record<string, never>;
 type EmptyValue = EmptyObject | null | undefined;
@@ -156,7 +157,8 @@ function mergeObject<TObject extends Record<string, unknown>>(
             // of the merged object only.
             const sourcePropertyWithoutMark = {...sourceProperty};
             delete sourcePropertyWithoutMark.ONYX_INTERNALS__REPLACE_OBJECT_MARK;
-            destination[key] = sourcePropertyWithoutMark;
+            // Built in "mark" mode, so it can still hold nested nulls.
+            destination[key] = options.shouldRemoveNestedNulls ? removeNestedNullValues(sourcePropertyWithoutMark) : sourcePropertyWithoutMark;
             continue;
         }
 
@@ -226,12 +228,25 @@ function needsNormalization(value: unknown): boolean {
     return false;
 }
 
-/** Deep removes the nested null values from the given value. Returns the original reference if no nulls were found. */
-function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value: TValue): TValue {
+/**
+ * Deep removes the nested null values from the given value. Returns the original reference if no nulls were found.
+ *
+ * Nested objects that are the same reference as the object at the same path in `nullFreeReference` are returned as is,
+ * without being traversed. Pass the live cache value here, since cached values never hold nested nulls.
+ */
+function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value: TValue, nullFreeReference?: unknown): TValue {
     if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) {
         return value;
     }
 
+    if (value === nullFreeReference) {
+        if (process.env.NODE_ENV === 'development' && needsNormalization(value)) {
+            Logger.logAlert('removeNestedNullValues skipped a cached subtree that holds nested null values. Onyx values must not be mutated in place.');
+        }
+        return value;
+    }
+
+    const reference = nullFreeReference !== null && typeof nullFreeReference === 'object' && !Array.isArray(nullFreeReference) ? (nullFreeReference as Record<string, unknown>) : undefined;
     let hasChanged = false;
     const result: Record<string, unknown> = {};
 
@@ -245,7 +260,7 @@ function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value:
         }
 
         if (typeof propertyValue === 'object' && !Array.isArray(propertyValue)) {
-            const cleaned = removeNestedNullValues(propertyValue);
+            const cleaned = removeNestedNullValues(propertyValue, reference?.[key]);
             if (cleaned !== propertyValue) {
                 hasChanged = true;
             }

tests/unit/onyxTest.ts

@@ -13,6 +13,7 @@ import createDeferredTask from '../../lib/createDeferredTask';
 import * as Logger from '../../lib/Logger';
 import onyxSubscriptionManager from '../../lib/OnyxSubscriptionManager';
 import OnyxUtils from '../../lib/OnyxUtils';
+import utils from '../../lib/utils';
 import StorageMock from '../../lib/storage';
 import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
 
@@ -3841,6 +3842,152 @@ describe('Onyx', () => {
             await expect(Onyx.multiGet([])).resolves.toEqual([]);
         });
     });
+
+    describe('null-free cache', () => {
+        it('should remove nested nulls when the native merge path replaces an object built in mark mode', () => {
+            // Given a batch that removes an object and then sets a new one holding a nested null (native applyMerge batches like this)
+            const {result: batchedChanges} = OnyxUtils.mergeAndMarkChanges([{a: null}, {a: {b: 1, c: null}}]);
+
+            // When the batched change is merged into the existing value
+            const {result: mergedValue} = OnyxUtils.mergeChanges([batchedChanges], {a: {x: 1}});
+
+            // Then the replacing object has its nested null removed, so it can be cached
+            expect(mergedValue).toStrictEqual({a: {b: 1}});
+        });
+
+        it('should hand out the cleaned cached value from multiGet', async () => {
+            // Given a value in storage that holds nested nulls (SQLite data written by older Onyx versions can)
+            await StorageMock.setItem(ONYX_KEYS.TEST_KEY, {a: {b: null, c: 1}});
+
+            // When it is read through multiGet
+            const dataMap = await OnyxUtils.multiGet([ONYX_KEYS.TEST_KEY]);
+
+            // Then the caller gets the same cleaned value the cache holds
+            expect(dataMap.get(ONYX_KEYS.TEST_KEY)).toStrictEqual({a: {c: 1}});
+            expect(dataMap.get(ONYX_KEYS.TEST_KEY)).toBe(cache.get(ONYX_KEYS.TEST_KEY));
+        });
+
+        it('should alert in development when a skipped cached subtree was mutated to hold a null', async () => {
+            // Given a cached subtree that app code mutated in place, which Onyx does not support
+            await Onyx.set(ONYX_KEYS.TEST_KEY, {a: {x: 1}});
+            const cachedValue = cache.get(ONYX_KEYS.TEST_KEY) as GenericDeepRecord;
+            (cachedValue.a as GenericDeepRecord).x = null;
+            const logAlertSpy = jest.spyOn(Logger, 'logAlert');
+            const previousNodeEnv = process.env.NODE_ENV;
+            process.env.NODE_ENV = 'development';
+
+            // When a value sharing that subtree is set
+            await Onyx.set(ONYX_KEYS.TEST_KEY, {...cachedValue, b: 1});
+            process.env.NODE_ENV = previousNodeEnv;
+
+            // Then the mutation is reported instead of silently persisting the null
+            expect(logAlertSpy).toHaveBeenCalledWith(expect.stringContaining('skipped a cached subtree that holds nested null values'));
+            logAlertSpy.mockRestore();
+        });
+
+        it('should keep every cached value free of nested nulls across random writes', async () => {
+            // Given random sequences of set, merge, multiSet, mergeCollection and update writes that carry nested nulls
+            let state = 7;
+            const random = () => {
+                state = (state * 1664525 + 1013904223) % 4294967296;
+                return state / 4294967296;
+            };
+            const randomValue = (depth: number): unknown => {
+                const roll = random();
+                if (roll < 0.2) {
+                    return null;
+                }
+                if (roll < 0.45 || depth === 0) {
+                    return Math.floor(random() * 10);
+                }
+                const value: Record<string, unknown> = {};
+                for (let i = 0; i < 1 + Math.floor(random() * 3); i++) {
+                    value[`p${Math.floor(random() * 4)}`] = randomValue(depth - 1);
+                }
+                return value;
+            };
+            const keys = [ONYX_KEYS.TEST_KEY, `${ONYX_KEYS.COLLECTION.TEST_KEY}1`, `${ONYX_KEYS.COLLECTION.TEST_KEY}2`, `${ONYX_KEYS.COLLECTION.TEST_KEY}3`];
+            const randomKey = () => keys[Math.floor(random() * keys.length)];
+            const randomObject = () => ({...(randomValue(3) as Record<string, unknown>), id: 1});
+
+            for (let step = 0; step < 300; step++) {
+                const roll = random();
+                if (roll < 0.25) {
+                    Onyx.set(randomKey(), randomObject());
+                } else if (roll < 0.5) {
+                    Onyx.merge(randomKey(), randomObject());
+                } else if (roll < 0.6) {
+                    Onyx.multiSet({[randomKey()]: randomObject()});
+                } else if (roll < 0.75) {
+                    Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
+                        [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: randomObject(),
+                        [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: randomObject(),
+                    } as GenericCollection);
+                } else if (roll < 0.88) {
+                    Onyx.update([
+                        {onyxMethod: Onyx.METHOD.MERGE, key: randomKey(), value: random() < 0.3 ? null : randomObject()},
+                        {onyxMethod: Onyx.METHOD.MERGE, key: randomKey(), value: randomObject()},
+                        {onyxMethod: random() < 0.5 ? Onyx.METHOD.SET : Onyx.METHOD.MERGE, key: randomKey(), value: randomObject()},
+                    ]);
+                } else {
+                    // Removes a nested object and replaces it with a new one holding nulls in the same batch (replace mode)
+                    const member = `${ONYX_KEYS.COLLECTION.TEST_KEY}${1 + Math.floor(random() * 3)}`;
+                    Onyx.update([
+                        {onyxMethod: Onyx.METHOD.MERGE, key: member, value: {p0: null}},
+                        {onyxMethod: Onyx.METHOD.MERGE, key: member, value: {p0: {q: 1, r: null, s: {t: null}}}},
+                        {onyxMethod: Onyx.METHOD.MERGE, key: randomKey(), value: randomObject()},
+                    ]);
+                }
+                if (step % 10 === 9) {
+                    await waitForPromisesToResolve();
+
+                    // Then no cached value holds a nested null, which the subtree skip in removeNestedNullValues relies on
+                    for (const key of keys) {
+                        const value = cache.get(key);
+                        expect(value === undefined || value === null || !utils.needsNormalization(value)).toBe(true);
+                    }
+                }
+            }
+        });
+
+        it('should not cache nested nulls of an object that replaced a removed object in a batched update', async () => {
+            const member1 = `${ONYX_KEYS.COLLECTION.TEST_KEY}1`;
+            const member2 = `${ONYX_KEYS.COLLECTION.TEST_KEY}2`;
+            await Onyx.multiSet({[member1]: {a: {x: 1}}, [member2]: {a: {x: 1}}});
+
+            await Onyx.update([
+                {onyxMethod: Onyx.METHOD.MERGE, key: member1, value: {a: null}},
+                {onyxMethod: Onyx.METHOD.MERGE, key: member1, value: {a: {b: 1, c: null}}},
+                {onyxMethod: Onyx.METHOD.MERGE, key: member2, value: {z: 1}},
+            ]);
+
+            expect(cache.get(member1)).toStrictEqual({a: {b: 1}});
+        });
+
+        it('should keep the cached subtrees and remove new nulls when setting a value built from the cached one', async () => {
+            await Onyx.set(ONYX_KEYS.TEST_KEY, {a: {x: 1}, b: {y: 2}});
+            const cachedValue = cache.get(ONYX_KEYS.TEST_KEY) as GenericDeepRecord;
+
+            await Onyx.set(ONYX_KEYS.TEST_KEY, {...cachedValue, b: {y: 3, z: null}, c: null});
+
+            const newValue = cache.get(ONYX_KEYS.TEST_KEY) as GenericDeepRecord;
+            expect(newValue).toStrictEqual({a: {x: 1}, b: {y: 3}});
+            expect(newValue.a).toBe(cachedValue.a);
+            expect(await StorageMock.getItem(ONYX_KEYS.TEST_KEY)).toStrictEqual({a: {x: 1}, b: {y: 3}});
+        });
+
+        it('should keep the cached subtrees and remove new nulls when multi-setting values built from the cached ones', async () => {
+            await Onyx.set(ONYX_KEYS.TEST_KEY, {a: {x: 1}});
+            const cachedValue = cache.get(ONYX_KEYS.TEST_KEY) as GenericDeepRecord;
+
+            await Onyx.multiSet({[ONYX_KEYS.TEST_KEY]: {...cachedValue, b: {y: null}}, [ONYX_KEYS.OTHER_TEST]: {c: {d: null}}});
+
+            const newValue = cache.get(ONYX_KEYS.TEST_KEY) as GenericDeepRecord;
+            expect(newValue).toStrictEqual({a: {x: 1}, b: {}});
+            expect(newValue.a).toBe(cachedValue.a);
+            expect(cache.get(ONYX_KEYS.OTHER_TEST)).toStrictEqual({c: {}});
+        });
+    });
 });
 
 // Separate describe block for Onyx.init to control initialization during each test.

tests/unit/storage/providers/IDBKeyvalProviderTest.ts

@@ -81,6 +81,17 @@ describe('IDBKeyValProvider', () => {
         it('should return null if there is no stored value for the key', async () => {
             expect(await IDBKeyValProvider.getItem(ONYXKEYS.TEST_KEY)).toBeNull();
         });
+
+        it('should remove nested nulls from a stored value', async () => {
+            // Given a value in storage that holds nested nulls
+            await IDB.set(ONYXKEYS.TEST_KEY_3, {a: 1, b: null, nested: {c: null, d: 'd'}, list: [null, {e: null}]}, IDBKeyValProvider.store);
+
+            // When it is read
+            const value = await IDBKeyValProvider.getItem(ONYXKEYS.TEST_KEY_3);
+
+            // Then the nested nulls are removed, and arrays are left as they are
+            expect(value).toEqual({a: 1, nested: {d: 'd'}, list: [null, {e: null}]});
+        });
     });
 
     describe('multiGet', () => {

tests/unit/storage/providers/SQLiteProviderTest.ts

@@ -62,6 +62,17 @@ describe('SQLiteProvider', () => {
         it('should return null if there is no stored value for the key', async () => {
             expect(await SQLiteProvider.getItem(ONYXKEYS.TEST_KEY)).toBeNull();
         });
+
+        it('should remove nested nulls from a stored value', async () => {
+            // Given a stored value with nested nulls, which JSON_REPLACE patches can write
+            await SQLiteProvider.store?.executeAsync('REPLACE INTO keyvaluepairs (record_key, valueJSON) VALUES (?, ?);', [
+                ONYXKEYS.TEST_KEY_3,
+                JSON.stringify({a: 1, b: null, nested: {c: null, d: 'd'}, list: [null, {e: null}]}),
+            ]);
+
+            // Then the value is handed out without the nested nulls, and arrays are left as they are
+            expect(await SQLiteProvider.getItem(ONYXKEYS.TEST_KEY_3)).toEqual({a: 1, nested: {d: 'd'}, list: [null, {e: null}]});
+        });
     });
 
     describe('multiGet', () => {
@@ -249,6 +260,28 @@ describe('SQLiteProvider', () => {
             expect((stored.outer as Record<string, unknown>).nested).not.toHaveProperty('oldKey');
             expect((stored.outer as Record<string, unknown>).nested).not.toHaveProperty('keepMe');
         });
+
+        it('should apply a replacement nested inside an earlier replacement', async () => {
+            await SQLiteProvider.setItem(ONYXKEYS.TEST_KEY_3, {outer: {oldKey: 'gone'}});
+
+            // Given the patches from a batch of merges {outer: null}, {outer: {nested: null}}, {outer: {nested: {newKey: 'newValue'}}}
+            const change = {
+                outer: {
+                    [utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true,
+                    nested: {[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true, newKey: 'newValue'},
+                },
+            };
+            const replaceNullPatches: Array<[string[], unknown]> = [
+                [['outer'], {nested: null}],
+                [['outer', 'nested'], {newKey: 'newValue'}],
+            ];
+
+            // When the patches are applied
+            await SQLiteProvider.multiMerge([[ONYXKEYS.TEST_KEY_3, change, replaceNullPatches]]);
+
+            // Then the second patch finds the path kept by the first one
+            expect(await SQLiteProvider.getItem(ONYXKEYS.TEST_KEY_3)).toEqual({outer: {nested: {newKey: 'newValue'}}});
+        });
     });
 
     describe('mergeItem', () => {