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); + } + }); +});