From 41d640a00763e1fdceaebd4cc369c1afc8cddfde Mon Sep 17 00:00:00 2001 From: arekm213 Date: Tue, 6 Oct 2026 17:51:25 +0200 Subject: [PATCH 1/8] Skip unchanged cached subtrees in removeNestedNullValues and keep the cache null-free --- lib/Onyx.ts | 8 ++- lib/OnyxUtils.ts | 17 +++-- lib/utils.ts | 29 ++++++-- tests/unit/onyxTest.ts | 156 ++++++++++++++++++++++++++++++++++++++++ tests/unit/utilsTest.ts | 145 +++++++++++++++++++++++++++++++++++++ 5 files changed, 344 insertions(+), 11 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 72c76c608..632b3b804 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -64,13 +64,16 @@ function init({ const individual: Array<[OnyxKey, OnyxEntry]> = []; const collectionBatches = new Map>; previous: NonUndefined>}>(); - 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; } + // Storage can hold nested nulls, but cached values must not. + const value = utils.removeNestedNullValues(valueFromStorage); + const collectionKey = OnyxKeys.getCollectionKey(key); const isCollectionMember = !!collectionKey && OnyxKeys.isCollectionMemberKey(collectionKey, key); @@ -489,7 +492,8 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // since collection key subscribers need to be updated differently if (!isKeyToPreserve) { const oldValue = cache.get(key); - const newValue = defaultKeyStates[key] ?? null; + // Cached values must not hold nested nulls, so the default key state is cleaned first. + const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); if (newValue !== oldValue) { cache.set(key, newValue); diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index c7ddb5478..dda859eb2 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -300,8 +300,10 @@ function get>(key: TKey): P return undefined; } - cache.set(key, val); - return val; + // Storage can hold nested nulls (e.g. SQLite's JSON_REPLACE of marked objects), but cached values must not. + const valueWithoutNestedNullValues = utils.removeNestedNullValues(val) as TValue; + cache.set(key, valueWithoutNestedNullValues); + return valueWithoutNestedNullValues; }) .catch((err) => Logger.logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`)); @@ -402,10 +404,15 @@ function multiGet(keys: CollectionKeyBase[]): Promise); temp[key] = value as OnyxValue; } cache.merge(temp); + + // Hand out what the cache now holds, which has the nested null values removed, so callers and cache agree. + for (const key of Object.keys(temp)) { + const cachedValue = cache.get(key); + dataMap.set(key as TKey, (cachedValue !== undefined ? cachedValue : temp[key]) as OnyxValue); + } return dataMap; }) ); @@ -781,7 +788,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]]); @@ -1115,7 +1122,7 @@ function setWithRetry({key, value, options}: SetParams; + const valueWithoutNestedNullValues = utils.removeNestedNullValues(value, existingValue) as OnyxValue; const hasChanged = options?.skipCacheCheck ? true : cache.hasValueChanged(key, valueWithoutNestedNullValues); OnyxUtils.logKeyChanged(OnyxUtils.METHOD.SET, key, value, hasChanged); diff --git a/lib/utils.ts b/lib/utils.ts index 9992f577a..c5c0a194d 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -1,4 +1,5 @@ import type {OnyxInput, OnyxKey} from './types'; +import * as Logger from './Logger'; type EmptyObject = Record; type EmptyValue = EmptyObject | null | undefined; @@ -156,7 +157,8 @@ function mergeObject>( // of the merged object only. const sourcePropertyWithoutMark = {...sourceProperty}; delete sourcePropertyWithoutMark.ONYX_INTERNALS__REPLACE_OBJECT_MARK; - destination[key] = sourcePropertyWithoutMark; + // The replacement object was built in "mark" mode, so it can still hold nested nulls. + destination[key] = options.shouldRemoveNestedNulls ? removeNestedNullValues(sourcePropertyWithoutMark) : sourcePropertyWithoutMark; continue; } @@ -226,12 +228,31 @@ 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 | null>(value: TValue): TValue { +/** + * Deep removes the nested null values from the given value. Returns the original reference if no nulls were found. + * + * If `nullFreeReference` is given, any (nested) object of `value` that is the same reference as the object at the same + * path in `nullFreeReference` is returned as is, without being traversed. Callers pass the current cache value here, + * which relies on cached values never holding nested nulls. The cache write paths uphold that: `cache.set()` callers + * pass values cleaned by this function or by `fastMerge()` with `shouldRemoveNestedNulls`, `cache.merge()` and + * `cache.hydrate()` clean with `fastMerge()`, and values read from storage or default key states are cleaned before + * being cached. `nullFreeReference` must therefore be the live cache value, read synchronously right before the call. + */ +function removeNestedNullValues | null>(value: TValue, nullFreeReference?: unknown): TValue { if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) { return value; } + if (value === nullFreeReference) { + // A shared subtree only holds nulls if app code mutated a cached object in place, which Onyx doesn't support. + 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; + } + + // Cheaper than isMergeableObject: Dates and RegExps have no enumerable own keys, so a property lookup on them finds nothing. + const reference = nullFreeReference !== null && typeof nullFreeReference === 'object' && !Array.isArray(nullFreeReference) ? (nullFreeReference as Record) : undefined; let hasChanged = false; const result: Record = {}; @@ -245,7 +266,7 @@ function removeNestedNullValues | null>(value: } if (typeof propertyValue === 'object' && !Array.isArray(propertyValue)) { - const cleaned = removeNestedNullValues(propertyValue); + const cleaned = removeNestedNullValues(propertyValue, reference?.[key]); if (cleaned !== propertyValue) { hasChanged = true; } diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 954443a18..c4ab75fa4 100644 --- a/tests/unit/onyxTest.ts +++ b/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,161 @@ 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 can store them) + 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 = {}; + 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), 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 remove nested nulls from values read from storage before caching them', async () => { + await StorageMock.setItem(ONYX_KEYS.TEST_KEY, {a: {b: null, c: 1}, d: null}); + + const value = await OnyxUtils.get(ONYX_KEYS.TEST_KEY); + + expect(value).toStrictEqual({a: {c: 1}}); + expect(cache.get(ONYX_KEYS.TEST_KEY)).toStrictEqual({a: {c: 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. diff --git a/tests/unit/utilsTest.ts b/tests/unit/utilsTest.ts index a45b39a80..7a545e660 100644 --- a/tests/unit/utilsTest.ts +++ b/tests/unit/utilsTest.ts @@ -185,6 +185,16 @@ describe('utils', () => { }); }); + it('should remove nested nulls from the replacing object only when "shouldRemoveNestedNulls" is true', () => { + const source = {b: {d: {[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true, h: 'h', i: null, j: {k: null}}}}; + + const withRemoval = utils.fastMerge(testObject, source, {shouldRemoveNestedNulls: true, objectRemovalMode: 'replace'}); + expect(withRemoval.result).toStrictEqual({a: 'a', b: {c: 'c', d: {h: 'h', j: {}}, g: 'g'}}); + + const withoutRemoval = utils.fastMerge(testObject, source, {objectRemovalMode: 'replace'}); + expect(withoutRemoval.result).toStrictEqual({a: 'a', b: {c: 'c', d: {h: 'h', i: null, j: {k: null}}, g: 'g'}}); + }); + test.each([ ['a string', 'value'], ['a number', 1000], @@ -439,6 +449,66 @@ describe('utils', () => { const result = utils.removeNestedNullValues(value); expect(result).toBe(value); }); + + it('should only copy the objects along the path of a removed null', () => { + const untouched = {x: {y: 1}}; + const sibling = {z: 2}; + const value = {a: untouched, b: {c: {d: null, e: sibling}}, f: [null]}; + const result = utils.removeNestedNullValues(value) as GenericDeepRecord; + expect(result).toStrictEqual({a: {x: {y: 1}}, b: {c: {e: {z: 2}}}, f: [null]}); + expect(result.a).toBe(untouched); + expect((result.b as GenericDeepRecord).c).not.toBe(value.b.c); + expect(((result.b as GenericDeepRecord).c as GenericDeepRecord).e).toBe(sibling); + expect(result.f).toBe(value.f); + expect(value.b.c).toHaveProperty('d', null); + }); + + it('should keep the key order when the first removal happens after other keys', () => { + const value = {a: 1, b: {c: 1}, d: null, e: 'e', f: undefined, g: {h: null}}; + const result = utils.removeNestedNullValues(value); + expect(Object.keys(result)).toEqual(['a', 'b', 'e', 'g']); + expect(result).toStrictEqual({a: 1, b: {c: 1}, e: 'e', g: {}}); + expect((result as GenericDeepRecord).b).toBe(value.b); + }); + }); + + describe('nullFreeReference', () => { + it('should return the value as is when it is the reference itself', () => { + const value = {a: {b: 1}}; + expect(utils.removeNestedNullValues(value, value)).toBe(value); + }); + + it('should not traverse subtrees that are shared with the reference', () => { + let reads = 0; + const shared = { + get x() { + reads++; + return 1; + }, + }; + const value = {a: shared, b: null, c: {d: null}}; + const result = utils.removeNestedNullValues(value, {a: shared, c: {}}) as GenericDeepRecord; + expect(reads).toBe(0); + expect(result.a).toBe(shared); + expect(Object.keys(result)).toEqual(['a', 'c']); + expect(result.c).toStrictEqual({}); + }); + + it('should still remove nulls from nested subtrees that differ from the reference', () => { + const sharedD = {e: 1}; + const reference = {a: {b: {c: 1}, d: sharedD}, f: 1}; + const value = {a: {b: {c: null, g: 2}, d: sharedD}, f: null}; + const result = utils.removeNestedNullValues(value, reference) as GenericDeepRecord; + expect(result).toStrictEqual({a: {b: {g: 2}, d: {e: 1}}}); + expect((result.a as GenericDeepRecord).d).toBe(sharedD); + }); + + it('should ignore references that are not plain objects', () => { + const value = {0: {a: null}}; + expect(utils.removeNestedNullValues(value, [value[0]])).toStrictEqual({0: {}}); + expect(utils.removeNestedNullValues(value, 'string')).toStrictEqual({0: {}}); + expect(utils.removeNestedNullValues(value, null)).toStrictEqual({0: {}}); + }); }); }); @@ -521,3 +591,78 @@ describe('utils', () => { }); }); }); + +/** Small deterministic PRNG so failures are reproducible. */ +function createRandom(seed: number) { + let state = seed % 4294967296 || 1; + return () => { + state = (state * 1664525 + 1013904223) % 4294967296; + return state / 4294967296; + }; +} + +type Value = unknown; + +function randomLeaf(random: () => number): Value { + const roll = random(); + if (roll < 0.25) { + return Math.floor(random() * 100); + } + if (roll < 0.5) { + return `s${Math.floor(random() * 100)}`; + } + if (roll < 0.65) { + return [Math.floor(random() * 10), null, {a: null}]; + } + if (roll < 0.8) { + return random() < 0.5; + } + return null; +} + +function randomObject(random: () => number, depth: number): Record { + const object: Record = {}; + const size = 1 + Math.floor(random() * 6); + for (let i = 0; i < size; i++) { + object[`k${Math.floor(random() * 8)}`] = depth > 0 && random() < 0.4 ? randomObject(random, depth - 1) : randomLeaf(random); + } + return object; +} + +/** Builds a value that shares random subtrees (by reference) with `reference`, plus random new or changed parts. */ +function deriveValue(random: () => number, reference: Record, depth: number): Record { + const value: Record = {}; + for (const key of Object.keys(reference)) { + const roll = random(); + const referenceProperty = reference[key]; + if (roll < 0.5) { + value[key] = referenceProperty; + } else if (roll < 0.7 && depth > 0 && referenceProperty && typeof referenceProperty === 'object' && !Array.isArray(referenceProperty)) { + value[key] = deriveValue(random, referenceProperty as Record, depth - 1); + } else if (roll < 0.9) { + value[key] = depth > 0 && random() < 0.4 ? randomObject(random, depth - 1) : randomLeaf(random); + } + } + if (random() < 0.5) { + value[`n${Math.floor(random() * 5)}`] = depth > 0 && random() < 0.5 ? randomObject(random, depth - 1) : randomLeaf(random); + } + return value; +} + +describe('removeNestedNullValues with a null-free reference', () => { + it('returns the same result as without a reference for any null-free reference', () => { + // Given 5000 random values that share random subtrees with a null-free reference (like a cached value) + const random = createRandom(42); + for (let i = 0; i < 5000; i++) { + const reference = utils.removeNestedNullValues(randomObject(random, 3)) as Record; + const value = deriveValue(random, reference, 3); + + // When nulls are removed with and without the reference + const withReference = utils.removeNestedNullValues(value as never, reference); + const withoutReference = utils.removeNestedNullValues(value as never); + + // Then the content is identical, because skipping a shared subtree must never skip a null + expect(withReference).toStrictEqual(withoutReference); + } + }); +}); From 6839ba1c420494665c0bae5193d0dee16fa42c57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Tue, 13 Oct 2026 11:58:29 +0200 Subject: [PATCH 2/8] Keep SQLite free of nested nulls instead of cleaning every lazy read Cleaning every value read by OnyxUtils.get() walked the whole value and made the Reassure get test 41% slower. The only provider that could hand out nested nulls is SQLite, because JSON_REPLACE stores the replacement object as is. Clean the replacement objects before they are stored, and clean values that older versions already stored when they are read, skipping the walk for objects whose JSON has no null. --- lib/OnyxUtils.ts | 6 ++--- lib/storage/providers/SQLiteProvider.ts | 12 +++++++-- tests/unit/onyxTest.ts | 11 +------- .../storage/providers/SQLiteProviderTest.ts | 25 +++++++++++++++++++ 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index dda859eb2..835ee015a 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -300,10 +300,8 @@ function get>(key: TKey): P return undefined; } - // Storage can hold nested nulls (e.g. SQLite's JSON_REPLACE of marked objects), but cached values must not. - const valueWithoutNestedNullValues = utils.removeNestedNullValues(val) as TValue; - cache.set(key, valueWithoutNestedNullValues); - return valueWithoutNestedNullValues; + cache.set(key, val); + return val; }) .catch((err) => Logger.logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`)); diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index 013139120..10623c2f0 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -5,6 +5,7 @@ import type {BatchQueryCommand, NitroSQLiteConnection, QueryResult} from 'react-native-nitro-sqlite'; import {open} from 'react-native-nitro-sqlite'; import {getFreeDiskStorage} from 'react-native-device-info'; +import type {OnyxKey, OnyxValue} from '../../types'; import type {FastMergeReplaceNullPatch} from '../../utils'; import utils from '../../utils'; import type StorageProvider from './types'; @@ -92,7 +93,9 @@ function objectMarkRemover(key: string, value: unknown) { function generateJSONReplaceSQLQueries(key: string, patches: FastMergeReplaceNullPatch[]): string[][] { const queries = patches.map(([pathArray, value]) => { const jsonPath = `$.${pathArray.join('.')}`; - return [jsonPath, JSON.stringify(value), key]; + // The replacement object was built in "mark" mode, so it can still hold nested nulls. Unlike JSON_PATCH, + // JSON_REPLACE would store them as they are. + return [jsonPath, JSON.stringify(utils.removeNestedNullValues(value as OnyxValue)), key]; }); return queries; @@ -145,7 +148,12 @@ const provider: StorageProvider = { return null; } - return JSON.parse(result.valueJSON); + const value = JSON.parse(result.valueJSON); + + // Older Onyx versions could store nested nulls through JSON_REPLACE, but values handed to the cache must not hold + // any. Objects without "null" in their JSON can't hold one, so the walk is skipped for them. Numeric values come + // back as numbers because of the column's affinity, so the string check only runs for objects. + return typeof value === 'object' && value !== null && result.valueJSON.includes('null') ? utils.removeNestedNullValues(value) : value; }); }, multiGet(keys) { diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index c4ab75fa4..4c2efa34e 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -3856,7 +3856,7 @@ describe('Onyx', () => { }); it('should hand out the cleaned cached value from multiGet', async () => { - // Given a value in storage that holds nested nulls (SQLite can store them) + // 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 @@ -3964,15 +3964,6 @@ describe('Onyx', () => { expect(cache.get(member1)).toStrictEqual({a: {b: 1}}); }); - it('should remove nested nulls from values read from storage before caching them', async () => { - await StorageMock.setItem(ONYX_KEYS.TEST_KEY, {a: {b: null, c: 1}, d: null}); - - const value = await OnyxUtils.get(ONYX_KEYS.TEST_KEY); - - expect(value).toStrictEqual({a: {c: 1}}); - expect(cache.get(ONYX_KEYS.TEST_KEY)).toStrictEqual({a: {c: 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; diff --git a/tests/unit/storage/providers/SQLiteProviderTest.ts b/tests/unit/storage/providers/SQLiteProviderTest.ts index 55da98760..168fa8ded 100644 --- a/tests/unit/storage/providers/SQLiteProviderTest.ts +++ b/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 value stored by an older Onyx version, whose JSON_REPLACE patches could write nested nulls + 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,20 @@ describe('SQLiteProvider', () => { expect((stored.outer as Record).nested).not.toHaveProperty('oldKey'); expect((stored.outer as Record).nested).not.toHaveProperty('keepMe'); }); + + it('should not store the nested nulls of an object that replaces a nested object', async () => { + await SQLiteProvider.setItem(ONYXKEYS.TEST_KEY_3, {outer: {nested: {oldKey: 'gone'}}}); + + // Given a replacement object built in "mark" mode, which can still hold nested nulls + const change = {outer: {nested: {[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true, newKey: 'newValue', removedKey: null}}}; + const replaceNullPatches: Array<[string[], unknown]> = [[['outer', 'nested'], {newKey: 'newValue', removedKey: null, deeper: {removedKey: null}}]]; + + await SQLiteProvider.multiMerge([[ONYXKEYS.TEST_KEY_3, change, replaceNullPatches]]); + + // Then the stored JSON holds no nulls + const result = await SQLiteProvider.store?.executeAsync<{valueJSON: string}>('SELECT valueJSON FROM keyvaluepairs WHERE record_key = ?;', [ONYXKEYS.TEST_KEY_3]); + expect(JSON.parse(result?.rows?.item(0)?.valueJSON ?? '')).toEqual({outer: {nested: {newKey: 'newValue', deeper: {}}}}); + }); }); describe('mergeItem', () => { From 7aecfc042b79739fae5beddc8905551cdc2402d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Thu, 15 Oct 2026 13:03:27 +0200 Subject: [PATCH 3/8] Simplify comments --- lib/Onyx.ts | 2 +- lib/OnyxUtils.ts | 2 +- lib/storage/providers/SQLiteProvider.ts | 8 +++----- lib/utils.ts | 12 ++++-------- 4 files changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 632b3b804..1107b947c 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -492,7 +492,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // since collection key subscribers need to be updated differently if (!isKeyToPreserve) { const oldValue = cache.get(key); - // Cached values must not hold nested nulls, so the default key state is cleaned first. + // Cached values must not hold nested nulls. const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); if (newValue !== oldValue) { cache.set(key, newValue); diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 145a4c881..4cda28f6a 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -406,7 +406,7 @@ function multiGet(keys: CollectionKeyBase[]): Promise); diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index 10623c2f0..70d596258 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -93,8 +93,7 @@ function objectMarkRemover(key: string, value: unknown) { function generateJSONReplaceSQLQueries(key: string, patches: FastMergeReplaceNullPatch[]): string[][] { const queries = patches.map(([pathArray, value]) => { const jsonPath = `$.${pathArray.join('.')}`; - // The replacement object was built in "mark" mode, so it can still hold nested nulls. Unlike JSON_PATCH, - // JSON_REPLACE would store them as they are. + // JSON_REPLACE stores the object as is, so nested nulls are removed first. return [jsonPath, JSON.stringify(utils.removeNestedNullValues(value as OnyxValue)), key]; }); @@ -150,9 +149,8 @@ const provider: StorageProvider = { const value = JSON.parse(result.valueJSON); - // Older Onyx versions could store nested nulls through JSON_REPLACE, but values handed to the cache must not hold - // any. Objects without "null" in their JSON can't hold one, so the walk is skipped for them. Numeric values come - // back as numbers because of the column's affinity, so the string check only runs for objects. + // Stored values can hold nested nulls. Only objects whose JSON contains "null" are walked. valueJSON is a number + // for numeric values, so the string check only runs for objects. return typeof value === 'object' && value !== null && result.valueJSON.includes('null') ? utils.removeNestedNullValues(value) : value; }); }, diff --git a/lib/utils.ts b/lib/utils.ts index c5c0a194d..e6e9cf734 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -157,7 +157,7 @@ function mergeObject>( // of the merged object only. const sourcePropertyWithoutMark = {...sourceProperty}; delete sourcePropertyWithoutMark.ONYX_INTERNALS__REPLACE_OBJECT_MARK; - // The replacement object was built in "mark" mode, so it can still hold nested nulls. + // Built in "mark" mode, so it can still hold nested nulls. destination[key] = options.shouldRemoveNestedNulls ? removeNestedNullValues(sourcePropertyWithoutMark) : sourcePropertyWithoutMark; continue; } @@ -231,12 +231,8 @@ function needsNormalization(value: unknown): boolean { /** * Deep removes the nested null values from the given value. Returns the original reference if no nulls were found. * - * If `nullFreeReference` is given, any (nested) object of `value` that is the same reference as the object at the same - * path in `nullFreeReference` is returned as is, without being traversed. Callers pass the current cache value here, - * which relies on cached values never holding nested nulls. The cache write paths uphold that: `cache.set()` callers - * pass values cleaned by this function or by `fastMerge()` with `shouldRemoveNestedNulls`, `cache.merge()` and - * `cache.hydrate()` clean with `fastMerge()`, and values read from storage or default key states are cleaned before - * being cached. `nullFreeReference` must therefore be the live cache value, read synchronously right before the call. + * 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 | null>(value: TValue, nullFreeReference?: unknown): TValue { if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) { @@ -251,7 +247,7 @@ function removeNestedNullValues | null>(value: return value; } - // Cheaper than isMergeableObject: Dates and RegExps have no enumerable own keys, so a property lookup on them finds nothing. + // Dates and RegExps have no enumerable keys, so lookups on them find nothing. const reference = nullFreeReference !== null && typeof nullFreeReference === 'object' && !Array.isArray(nullFreeReference) ? (nullFreeReference as Record) : undefined; let hasChanged = false; const result: Record = {}; From b6722efa41e08cb9aa7d1a269a9e25868cf217d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Thu, 15 Oct 2026 13:16:01 +0200 Subject: [PATCH 4/8] Remove unneeded comments --- lib/Onyx.ts | 2 -- lib/storage/providers/SQLiteProvider.ts | 3 +-- lib/utils.ts | 2 -- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 1107b947c..644db7685 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -71,7 +71,6 @@ function init({ continue; } - // Storage can hold nested nulls, but cached values must not. const value = utils.removeNestedNullValues(valueFromStorage); const collectionKey = OnyxKeys.getCollectionKey(key); @@ -492,7 +491,6 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // since collection key subscribers need to be updated differently if (!isKeyToPreserve) { const oldValue = cache.get(key); - // Cached values must not hold nested nulls. const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); if (newValue !== oldValue) { cache.set(key, newValue); diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index 70d596258..2b3838bc9 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -149,8 +149,7 @@ const provider: StorageProvider = { const value = JSON.parse(result.valueJSON); - // Stored values can hold nested nulls. Only objects whose JSON contains "null" are walked. valueJSON is a number - // for numeric values, so the string check only runs for objects. + // 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; }); }, diff --git a/lib/utils.ts b/lib/utils.ts index e6e9cf734..9abfa4946 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -240,14 +240,12 @@ function removeNestedNullValues | null>(value: } if (value === nullFreeReference) { - // A shared subtree only holds nulls if app code mutated a cached object in place, which Onyx doesn't support. 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; } - // Dates and RegExps have no enumerable keys, so lookups on them find nothing. const reference = nullFreeReference !== null && typeof nullFreeReference === 'object' && !Array.isArray(nullFreeReference) ? (nullFreeReference as Record) : undefined; let hasChanged = false; const result: Record = {}; From 94edaee0195b78d675e3a5541c74d4cdad022308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Thu, 15 Oct 2026 13:38:45 +0200 Subject: [PATCH 5/8] Clean nested nulls when reading from IndexedDB --- lib/Onyx.ts | 2 +- lib/storage/providers/IDBKeyValProvider/index.ts | 9 ++++++++- tests/unit/storage/providers/IDBKeyvalProviderTest.ts | 11 +++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 644db7685..44b3fb9c7 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -71,7 +71,7 @@ function init({ continue; } - const value = utils.removeNestedNullValues(valueFromStorage); + const value = utils.needsNormalization(valueFromStorage) ? utils.removeNestedNullValues(valueFromStorage) : valueFromStorage; const collectionKey = OnyxKeys.getCollectionKey(key); const isCollectionMember = !!collectionKey && OnyxKeys.isCollectionMemberKey(collectionKey, key); diff --git a/lib/storage/providers/IDBKeyValProvider/index.ts b/lib/storage/providers/IDBKeyValProvider/index.ts index 546c9acd2..cbb50c1cf 100644 --- a/lib/storage/providers/IDBKeyValProvider/index.ts +++ b/lib/storage/providers/IDBKeyValProvider/index.ts @@ -154,7 +154,14 @@ const provider: StorageProvider = { 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) { diff --git a/tests/unit/storage/providers/IDBKeyvalProviderTest.ts b/tests/unit/storage/providers/IDBKeyvalProviderTest.ts index 0c4c9416d..921372288 100644 --- a/tests/unit/storage/providers/IDBKeyvalProviderTest.ts +++ b/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', () => { From 65f5f9dc17103acfd05fc15e27303ccbc7e0cd19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Thu, 15 Oct 2026 15:03:17 +0200 Subject: [PATCH 6/8] Keep nulls in SQLite replace patches so nested patches still apply --- lib/storage/providers/SQLiteProvider.ts | 4 +-- .../storage/providers/SQLiteProviderTest.ts | 26 ++++++++++++------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index 2b3838bc9..87a55752c 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -5,7 +5,6 @@ import type {BatchQueryCommand, NitroSQLiteConnection, QueryResult} from 'react-native-nitro-sqlite'; import {open} from 'react-native-nitro-sqlite'; import {getFreeDiskStorage} from 'react-native-device-info'; -import type {OnyxKey, OnyxValue} from '../../types'; import type {FastMergeReplaceNullPatch} from '../../utils'; import utils from '../../utils'; import type StorageProvider from './types'; @@ -93,8 +92,7 @@ function objectMarkRemover(key: string, value: unknown) { function generateJSONReplaceSQLQueries(key: string, patches: FastMergeReplaceNullPatch[]): string[][] { const queries = patches.map(([pathArray, value]) => { const jsonPath = `$.${pathArray.join('.')}`; - // JSON_REPLACE stores the object as is, so nested nulls are removed first. - return [jsonPath, JSON.stringify(utils.removeNestedNullValues(value as OnyxValue)), key]; + return [jsonPath, JSON.stringify(value), key]; }); return queries; diff --git a/tests/unit/storage/providers/SQLiteProviderTest.ts b/tests/unit/storage/providers/SQLiteProviderTest.ts index 168fa8ded..cdc7498e5 100644 --- a/tests/unit/storage/providers/SQLiteProviderTest.ts +++ b/tests/unit/storage/providers/SQLiteProviderTest.ts @@ -64,7 +64,7 @@ describe('SQLiteProvider', () => { }); it('should remove nested nulls from a stored value', async () => { - // Given a value stored by an older Onyx version, whose JSON_REPLACE patches could write nested nulls + // 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}]}), @@ -261,18 +261,26 @@ describe('SQLiteProvider', () => { expect((stored.outer as Record).nested).not.toHaveProperty('keepMe'); }); - it('should not store the nested nulls of an object that replaces a nested object', async () => { - await SQLiteProvider.setItem(ONYXKEYS.TEST_KEY_3, {outer: {nested: {oldKey: 'gone'}}}); + it('should apply a replacement nested inside an earlier replacement', async () => { + await SQLiteProvider.setItem(ONYXKEYS.TEST_KEY_3, {outer: {oldKey: 'gone'}}); - // Given a replacement object built in "mark" mode, which can still hold nested nulls - const change = {outer: {nested: {[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true, newKey: 'newValue', removedKey: null}}}; - const replaceNullPatches: Array<[string[], unknown]> = [[['outer', 'nested'], {newKey: 'newValue', removedKey: null, deeper: {removedKey: null}}]]; + // 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 stored JSON holds no nulls - const result = await SQLiteProvider.store?.executeAsync<{valueJSON: string}>('SELECT valueJSON FROM keyvaluepairs WHERE record_key = ?;', [ONYXKEYS.TEST_KEY_3]); - expect(JSON.parse(result?.rows?.item(0)?.valueJSON ?? '')).toEqual({outer: {nested: {newKey: 'newValue', deeper: {}}}}); + // 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'}}}); }); }); From ffe0a0292eda99863d9668e1cf41f1c5ab1209c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Thu, 29 Oct 2026 12:56:20 +0100 Subject: [PATCH 7/8] Address review: clean default key states once and add clear and cross-tab tests --- lib/Onyx.ts | 2 +- lib/OnyxUtils.ts | 11 ++--- lib/utils.ts | 6 +-- tests/unit/onyxTest.ts | 94 +++++++++++++++++++++++++++++++++--------- 4 files changed, 82 insertions(+), 31 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 44b3fb9c7..3480606bb 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -491,7 +491,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // since collection key subscribers need to be updated differently if (!isKeyToPreserve) { const oldValue = cache.get(key); - const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); + const newValue = defaultKeyStates[key] ?? null; if (newValue !== oldValue) { cache.set(key, newValue); diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 4cda28f6a..23efddd9e 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -189,8 +189,8 @@ function initStoreValues(keys: DeepRecord, initialKeyStates: Pa return acc; }, new Set()); - // Set our default key states to use when initializing and clearing Onyx data - defaultKeyStates = initialKeyStates; + // Default key states for init and clear(), cleaned once so clear() keeps the same references. + defaultKeyStates = Object.fromEntries(Object.entries(initialKeyStates).map(([key, value]) => [key, utils.removeNestedNullValues(value)])); DevTools.initState(initialKeyStates); @@ -402,15 +402,10 @@ function multiGet(keys: CollectionKeyBase[]): Promise); temp[key] = value as OnyxValue; } 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); - } return dataMap; }) ); diff --git a/lib/utils.ts b/lib/utils.ts index 9abfa4946..eb3196b1e 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -229,10 +229,10 @@ function needsNormalization(value: unknown): boolean { } /** - * Deep removes the nested null values from the given value. Returns the original reference if no nulls were found. + * Deep removes nested null values from the given value, leaving arrays as they are. 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. + * Objects that are the same reference as the object at the same path in `nullFreeReference` are returned without being traversed, + * so `nullFreeReference` must hold no nested nulls outside arrays (pass the live cache value). */ function removeNestedNullValues | null>(value: TValue, nullFreeReference?: unknown): TValue { if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) { diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 4c2efa34e..8f251c1b6 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -3855,18 +3855,6 @@ describe('Onyx', () => { 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}}); @@ -3876,13 +3864,16 @@ describe('Onyx', () => { 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; + try { + // When a value sharing that subtree is set + await Onyx.set(ONYX_KEYS.TEST_KEY, {...cachedValue, b: 1}); - // 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(); + // 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')); + } finally { + process.env.NODE_ENV = previousNodeEnv; + logAlertSpy.mockRestore(); + } }); it('should keep every cached value free of nested nulls across random writes', async () => { @@ -3941,7 +3932,7 @@ describe('Onyx', () => { if (step % 10 === 9) { await waitForPromisesToResolve(); - // Then no cached value holds a nested null, which the subtree skip in removeNestedNullValues relies on + // Then no cached value holds a nested null for (const key of keys) { const value = cache.get(key); expect(value === undefined || value === null || !utils.needsNormalization(value)).toBe(true); @@ -4645,3 +4636,68 @@ describe('get() should prefer cache over stale storage', () => { expect(value).toHaveProperty('lastVisitTime', '2025-01-01'); }); }); + +describe('null-free cache with init options', () => { + let cache: typeof OnyxCache; + + beforeEach(() => { + Object.assign(OnyxUtils.getDeferredInitTask(), createDeferredTask()); + cache = require('../../lib/OnyxCache').default; + }); + + afterEach(() => { + jest.restoreAllMocks(); + return Onyx.clear(); + }); + + it('should reset a key to its default without nested nulls, keeping the same reference on every clear', async () => { + // Given a default key state that holds nested nulls + Onyx.init({ + keys: ONYX_KEYS, + initialKeyStates: {[ONYX_KEYS.OTHER_TEST]: {errors: null, nested: {a: null, b: 1}, isLoading: false}}, + }); + await act(async () => waitForPromisesToResolve()); + await Onyx.merge(ONYX_KEYS.OTHER_TEST, {isLoading: true}); + const callback = jest.fn(); + const connection = Onyx.connect({key: ONYX_KEYS.OTHER_TEST, callback}); + await act(async () => waitForPromisesToResolve()); + callback.mockClear(); + + // When Onyx is cleared twice + await Onyx.clear(); + const valueAfterFirstClear = cache.get(ONYX_KEYS.OTHER_TEST); + await Onyx.clear(); + + // Then the default has no nested nulls and the second clear keeps the same reference without notifying again + expect(valueAfterFirstClear).toStrictEqual({nested: {b: 1}, isLoading: false}); + expect(cache.get(ONYX_KEYS.OTHER_TEST)).toBe(valueAfterFirstClear); + expect(callback).toHaveBeenCalledTimes(1); + + Onyx.disconnect(connection); + }); + + it('should remove nested nulls from a value synced from another instance', async () => { + // Given Onyx syncing values written by other instances + Onyx.init({keys: ONYX_KEYS, shouldSyncMultipleInstances: true}); + await act(async () => waitForPromisesToResolve()); + const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0]; + let receivedValue: unknown; + const connection = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + callback: (value) => { + receivedValue = value; + }, + }); + await act(async () => waitForPromisesToResolve()); + + // When another instance stores a value that holds nested nulls + syncCallback([[ONYX_KEYS.TEST_KEY, {a: {b: null, c: 1}, d: null}]]); + await act(async () => waitForPromisesToResolve()); + + // Then the cache and subscribers get the value without the nested nulls + expect(cache.get(ONYX_KEYS.TEST_KEY)).toStrictEqual({a: {c: 1}}); + expect(receivedValue).toStrictEqual({a: {c: 1}}); + + Onyx.disconnect(connection); + }); +}); From 7b045dde957b2503d686b98ccc82241ad472a79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Carekm213=E2=80=9D?= Date: Thu, 29 Oct 2026 13:29:12 +0100 Subject: [PATCH 8/8] Clean default key states lazily on first clear instead of on init --- lib/Onyx.ts | 2 +- lib/OnyxUtils.ts | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index e32c719a6..35be6bcf0 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -489,7 +489,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // since collection key subscribers need to be updated differently if (!isKeyToPreserve) { const oldValue = cache.get(key); - const newValue = defaultKeyStates[key] ?? null; + const newValue = OnyxUtils.getNullFreeDefaultKeyState(key); valuesSetByClear[key] = newValue; if (newValue !== oldValue) { cache.set(key, newValue); diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 23efddd9e..ed557f585 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -76,6 +76,9 @@ const mergeQueuesWithStaleRead = new WeakSet>>(); // Optional user-provided key value states set when Onyx initializes or clears let defaultKeyStates: Record> = {}; +// Cache for getNullFreeDefaultKeyState() +let nullFreeDefaultKeyStates: Record> = {}; + let snapshotKey: OnyxKey | null = null; // Connections can be made before `Onyx.init`. They would wait for this task before resolving. @@ -123,6 +126,14 @@ function getDefaultKeyStates(): Record> { return defaultKeyStates; } +/** Getter - returns the default key state without nested nulls, cached so clear() reuses the same references. */ +function getNullFreeDefaultKeyState(key: OnyxKey): OnyxValue { + if (!(key in nullFreeDefaultKeyStates)) { + nullFreeDefaultKeyStates[key] = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); + } + return nullFreeDefaultKeyStates[key]; +} + /** * Getter - returns the deffered init task. */ @@ -189,8 +200,9 @@ function initStoreValues(keys: DeepRecord, initialKeyStates: Pa return acc; }, new Set()); - // Default key states for init and clear(), cleaned once so clear() keeps the same references. - defaultKeyStates = Object.fromEntries(Object.entries(initialKeyStates).map(([key, value]) => [key, utils.removeNestedNullValues(value)])); + // Set our default key states to use when initializing and clearing Onyx data + defaultKeyStates = initialKeyStates; + nullFreeDefaultKeyStates = {}; DevTools.initState(initialKeyStates); @@ -1697,6 +1709,7 @@ const OnyxUtils = { getMergeQueue, getMergeQueuePromise, getDefaultKeyStates, + getNullFreeDefaultKeyState, getDeferredInitTask, afterInit, initStoreValues,