Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Onyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ function init({

// Initialize all of our keys with data provided then give green light to any pending connections.
// addEvictableKeysToRecentlyAccessedList must run after initializeWithDefaultKeyStates because
// eager cache loading populates the key index (cache.setAllKeys) inside initializeWithDefaultKeyStates,
// eager cache loading populates the key index (cache.hydrate) inside initializeWithDefaultKeyStates,
// and the evictable keys list depends on that index being populated.
OnyxUtils.initializeWithDefaultKeyStates()
.then(() => cache.addEvictableKeysToRecentlyAccessedList(OnyxKeys.isCollectionKey, OnyxUtils.getAllKeys))
Expand Down
95 changes: 90 additions & 5 deletions lib/OnyxCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {deepEqual} from 'fast-equals';
import bindAll from 'lodash/bindAll';
import type {ValueOf} from 'type-fest';
import utils from './utils';
import type {FastMergeOptions} from './utils';
import type {CollectionKeyBase, KeyValueMapping, NonUndefined, OnyxCollection, OnyxKey, OnyxValue} from './types';
import OnyxKeys from './OnyxKeys';

Expand All @@ -15,6 +16,16 @@ type CollectionSnapshot = Readonly<NonUndefined<OnyxCollection<KeyValueMapping[O
*/
const FROZEN_EMPTY_COLLECTION: Readonly<NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>>> = Object.freeze({});

/**
* Merge options shared by every cache write path (`merge()` and `hydrate()`'s fallback), so the
* three call sites can't drift apart. Cached values must never hold nested nulls, and a source
* object carrying the replace mark must replace the target object rather than merge into it.
*/
const CACHE_MERGE_OPTIONS: FastMergeOptions = {
shouldRemoveNestedNulls: true,
objectRemovalMode: 'replace',
};

// Task constants
const TASK = {
GET: 'get',
Expand Down Expand Up @@ -77,6 +88,7 @@ class OnyxCache {
'set',
'drop',
'merge',
'hydrate',
'hasPendingTask',
'getTaskPromise',
'captureTask',
Expand Down Expand Up @@ -199,6 +211,79 @@ class OnyxCache {
OnyxKeys.deregisterMemberKey(key);
}

/**
* Bulk-loads values into a cache that's expected to be empty, skipping merge()'s per-key clone when
* safe. Falls back to a real merge for any key that already has a value, in case the cache wasn't
* empty after all. Used only by `Onyx.init()`.
* @param data - a map of (cache) key - values
*/
hydrate(data: Record<OnyxKey, OnyxValue<OnyxKey>>): void {
if (data === null || typeof data !== 'object' || Array.isArray(data)) {
throw new Error('data passed to cache.hydrate() must be an Object of onyx key/value pairs');
}

const affectedCollections = new Set<OnyxKey>();

// Use for-in loop to avoid an unnecessary array allocation from Object.keys()
// eslint-disable-next-line no-restricted-syntax, guard-for-in
for (const key in data) {
Comment thread
WojtekBoman marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Ignore inherited properties when hydrating cache batches

When storage contains a key named __proto__ with an object value, the assignment in initializeWithDefaultKeyStates() changes allDataFromStorage's prototype; this unguarded for...in then treats every enumerable property of that stored value as a separate Onyx key and registers/caches data that does not exist under those keys. The previous Object.entries() path only processed own properties, so guard the loop with Object.hasOwn(data, key) (and likewise for the matching loop in merge()).

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is correct, but I don't think the guard is necessary here, and it was intentionally removed in review #821 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I dont think this kind of situation would ever happen

const value = data[key];
this.addKey(key);

if (value === undefined) {
this.addNullishStorageKey(key);
// undefined means "no change" — skip storageMap modification
continue;
Comment thread
WojtekBoman marked this conversation as resolved.
}

const collectionKey = OnyxKeys.getCollectionKey(key);

if (value === null) {
this.addNullishStorageKey(key);
delete this.storageMap[key];

if (collectionKey) {
affectedCollections.add(collectionKey);
}
} else {
this.nullishStorageKeys.delete(key);

const existing = this.storageMap[key];

if (existing !== undefined) {
// Key already has a value, so the empty-cache assumption doesn't hold here (e.g. a write
// landed while storage was still being read). Fall back to a real merge, which has exactly
// the same semantics as the old `cache.merge(allDataFromStorage)` init path: the value
// loaded from disk is the merge source, so it wins on any overlapping leaf key.
Comment thread
WojtekBoman marked this conversation as resolved.
// Note: this only covers non-null writes. A `cache.set(key, null)` that lands before hydrate
// clears the nullish marker too, so a deletion racing init is still undone - same as before.
const merged = utils.fastMerge(existing, value, CACHE_MERGE_OPTIONS).result;

// fastMerge is reference-stable: returns the original target when
// nothing changed, so a simple === check detects no-ops.
if (merged === existing) {
continue;
}

this.storageMap[key] = merged;
} else if (utils.needsNormalization(value)) {
this.storageMap[key] = utils.fastMerge(undefined, value, CACHE_MERGE_OPTIONS).result;
} else {
this.storageMap[key] = value;
}

if (collectionKey) {
affectedCollections.add(collectionKey);
}
}
}

// Mark affected collections as dirty — snapshots will be lazily rebuilt on next read
for (const collectionKey of affectedCollections) {
Comment thread
WojtekBoman marked this conversation as resolved.
this.dirtyCollections.add(collectionKey);
}
}

/**
* Deep merge data to cache, any non existing keys will be created
* @param data - a map of (cache) key - values
Expand All @@ -210,7 +295,10 @@ class OnyxCache {

const affectedCollections = new Set<OnyxKey>();

for (const [key, value] of Object.entries(data)) {
// Use for-in loop to avoid an unnecessary array allocation from Object.entries()
// eslint-disable-next-line no-restricted-syntax, guard-for-in
for (const key in data) {
const value = data[key];
this.addKey(key);

const collectionKey = OnyxKeys.getCollectionKey(key);
Expand All @@ -233,10 +321,7 @@ class OnyxCache {

// Per-key merge instead of spreading the entire storageMap
const existing = this.storageMap[key];
const merged = utils.fastMerge(existing, value, {
shouldRemoveNestedNulls: true,
objectRemovalMode: 'replace',
}).result;
const merged = utils.fastMerge(existing, value, CACHE_MERGE_OPTIONS).result;

// fastMerge is reference-stable: returns the original target when
// nothing changed, so a simple === check detects no-ops.
Expand Down
9 changes: 6 additions & 3 deletions lib/OnyxUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1039,9 +1039,12 @@ function initializeWithDefaultKeyStates(): Promise<void> {
allDataFromStorage[key] = value;
}

// Load all storage data into cache silently (no subscriber notifications)
cache.setAllKeys(Object.keys(allDataFromStorage));
cache.merge(allDataFromStorage);
// Load all storage data into cache silently (no subscriber notifications).
// hydrate() rather than merge(): the cache is empty at this point, so a per-key fastMerge
// would only deep-clone every row it was handed.
// No setAllKeys() call is needed: hydrate() calls addKey() for every key, which populates the
// key index and registers collection member keys itself.
cache.hydrate(allDataFromStorage);

// For keys that have a developer-defined default (via `initialKeyStates`), merge the
// persisted value with the default so new properties added in code updates are applied
Expand Down
31 changes: 31 additions & 0 deletions lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,36 @@ function isMergeableObject<TObject extends Record<string, unknown>>(value: unkno
return isNonNullObject && !(value instanceof RegExp) && !(value instanceof Date) && !Array.isArray(value);
}

/**
* Reports whether a value needs cleaning (nested null/undefined, or the replace-object mark) before it's
* safe to store by reference. Read-only, non-allocating.
*/
function needsNormalization(value: unknown): boolean {
Comment thread
WojtekBoman marked this conversation as resolved.
if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) {
return false;
}

// Use for-in loop to avoid an unnecessary array allocation from Object.keys()
// eslint-disable-next-line no-restricted-syntax, guard-for-in
Comment thread
WojtekBoman marked this conversation as resolved.
for (const key in value) {
if (key === ONYX_INTERNALS__REPLACE_OBJECT_MARK) {
return true;
}

const propertyValue = (value as Record<string, unknown>)[key];

if (propertyValue === null || propertyValue === undefined) {
return true;
}

if (typeof propertyValue === 'object' && !Array.isArray(propertyValue) && needsNormalization(propertyValue)) {
return true;
}
}

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 {
if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) {
Expand Down Expand Up @@ -346,6 +376,7 @@ export default {
isEmptyObject,
formatActionName,
removeNestedNullValues,
needsNormalization,
checkCompatibilityWithExistingValue,
pick,
omit,
Expand Down
Loading
Loading