Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistencies between single key operations (merge, set) and multi key operations (mergeCollection, multiSet) #519

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
b6b0dd9
add mergeCollection test
chrispader Mar 25, 2024
1fa7b46
Merge branch 'main' into @fix-nested-null-removal-inconsistency-betwe…
chrispader Mar 27, 2024
2452f72
don't remove null values when merging with multiMerge directly
chrispader Mar 27, 2024
32e441e
add missing property
chrispader Mar 28, 2024
86ccc68
fix: test
chrispader Mar 28, 2024
209784e
simplify return in MemoryOnlyProvider
chrispader Mar 28, 2024
9c72a6d
remove log
chrispader Mar 28, 2024
c089aa5
remove nulls from cached collections
chrispader Mar 28, 2024
c403842
fix: remove invalid null removals (for comparison)
chrispader Mar 28, 2024
325e205
fix remaining callback calls
chrispader Mar 28, 2024
ef053d3
never return null values to the user in withOnyx
chrispader Mar 28, 2024
0727ec9
rename parameter name
chrispader Mar 28, 2024
f0c2668
simplify prepareKeyValuePairsForStorage function based on @paultsimur…
chrispader Mar 28, 2024
28ca063
fix: comments
chrispader Mar 28, 2024
dde6f21
simplify tests and add one for mergeCollection
chrispader Mar 28, 2024
60766dd
rename parameter
chrispader Mar 28, 2024
0ceca8c
remove invalid test
chrispader Mar 28, 2024
bbad000
remove unused import
chrispader Mar 28, 2024
4404737
fix: circular fastMerge
chrispader Mar 28, 2024
a3a8cdd
reset onyxTest
chrispader Mar 28, 2024
32beb34
add new tests
chrispader Mar 28, 2024
8c41e94
fix: test
chrispader Mar 28, 2024
a2b3ae8
improve comment
chrispader Mar 28, 2024
7764fb3
remove unused line
chrispader Mar 28, 2024
5f2e012
fix: early return
chrispader Apr 1, 2024
2c7e69f
Revert "fix: early return"
chrispader Apr 1, 2024
d717810
Merge branch 'main' into @fix-nested-null-removal-inconsistency-betwe…
chrispader Apr 15, 2024
8ec20ce
Merge branch 'main' into @fix-nested-null-removal-inconsistency-betwe…
chrispader Apr 17, 2024
86202a8
fix: removeNestedNullValues
chrispader Apr 17, 2024
e657cf6
Merge branch 'main' into @fix-nested-null-removal-inconsistency-betwe…
chrispader Apr 20, 2024
5d35172
rename variable
chrispader Apr 23, 2024
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
6 changes: 5 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,9 @@ dist/
# Published package
*.tgz

# Yalc
.yalc
yalc.lock

# Perf tests
.reassure
.reassure
15 changes: 11 additions & 4 deletions lib/Onyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ function set<TKey extends OnyxKey>(key: TKey, value: OnyxEntry<KeyValueMapping[T
* @param data object keyed by ONYXKEYS and the values to set
*/
function multiSet(data: Partial<NullableKeyValueMapping>): Promise<void> {
const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data);
const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data, true);

const updatePromises = keyValuePairs.map(([key, value]) => {
const prevValue = cache.getValue(key, false);
Expand Down Expand Up @@ -285,7 +285,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxEntry<NullishDeep<K
const mergeQueuePromise = OnyxUtils.getMergeQueuePromise();

// Top-level undefined values are ignored
// Therefore we need to prevent adding them to the merge queue
// Therefore, we need to prevent adding them to the merge queue
if (changes === undefined) {
return mergeQueue[key] ? mergeQueuePromise[key] : Promise.resolve();
}
Expand Down Expand Up @@ -419,8 +419,15 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
obj[key] = mergedCollection[key];
return obj;
}, {});
const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection);
const keyValuePairsForNewCollection = OnyxUtils.prepareKeyValuePairsForStorage(newCollection);

// When (multi-)merging the values with the existing values in storage,
// we don't want to remove nested null values from the data that we pass to the storage layer,
// because the storage layer uses them to remove nested keys from storage natively.
const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection, false);

// We can safely remove nested null values when using (multi-)set,
// because we will simply overwrite the existing values in storage.
const keyValuePairsForNewCollection = OnyxUtils.prepareKeyValuePairsForStorage(newCollection, true);

const promises = [];

Expand Down
48 changes: 25 additions & 23 deletions lib/OnyxUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
// We prepare the "cached collection" which is the entire collection + the new partial data that
// was merged in via mergeCollection().
const cachedCollection = getCachedCollection(collectionKey);
const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;

// Regular Onyx.connect() subscriber found.
if (typeof subscriber.callback === 'function') {
Expand All @@ -464,7 +465,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
// send the whole cached collection.
if (isSubscribedToCollectionKey) {
if (subscriber.waitForCollectionCallback) {
subscriber.callback(cachedCollection);
subscriber.callback(cachedCollectionWithoutNestedNulls);
continue;
}

Expand All @@ -473,7 +474,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
const dataKeys = Object.keys(partialCollection ?? {});
for (let j = 0; j < dataKeys.length; j++) {
const dataKey = dataKeys[j];
subscriber.callback(cachedCollection[dataKey], dataKey);
subscriber.callback(cachedCollectionWithoutNestedNulls[dataKey], dataKey);
}
continue;
}
Expand All @@ -482,7 +483,7 @@ function keysChanged<TKey extends CollectionKeyBase>(
// notify them with the cached data for that key only.
if (isSubscribedToCollectionMemberKey) {
const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
subscriberCallback(cachedCollection[subscriber.key], subscriber.key as TKey);
subscriberCallback(cachedCollectionWithoutNestedNulls[subscriber.key], subscriber.key as TKey);
continue;
}

Expand Down Expand Up @@ -621,13 +622,16 @@ function keyChanged<TKey extends OnyxKey>(
}
if (isCollectionKey(subscriber.key) && subscriber.waitForCollectionCallback) {
const cachedCollection = getCachedCollection(subscriber.key);
cachedCollection[key] = data;
subscriber.callback(cachedCollection);
const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;

cachedCollectionWithoutNestedNulls[key] = data;
subscriber.callback(cachedCollectionWithoutNestedNulls);
continue;
}

const dataWithoutNestedNulls = utils.removeNestedNullValues(data);
const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
subscriberCallback(data, key);
subscriberCallback(dataWithoutNestedNulls, key);
continue;
}

Expand Down Expand Up @@ -752,7 +756,8 @@ function sendDataToConnection<TKey extends OnyxKey>(mapping: Mapping<TKey>, val:
return;
}

(mapping as DefaultConnectOptions<TKey>).callback?.(val, matchedKey as TKey);
const valuesWithoutNestedNulls = utils.removeNestedNullValues(val);
(mapping as DefaultConnectOptions<TKey>).callback?.(valuesWithoutNestedNulls, matchedKey as TKey);
}

/**
Expand Down Expand Up @@ -963,11 +968,12 @@ type RemoveNullValuesOutput = {

/**
* Removes a key from storage if the value is null.
* Otherwise removes all nested null values in objects and returns the object
* Otherwise removes all nested null values in objects,
* if shouldRemoveNestedNulls is true and returns the object.
*
* @returns The value without null values and a boolean "wasRemoved", which indicates if the key got removed completely
*/
function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>): RemoveNullValuesOutput {
function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>, shouldRemoveNestedNulls = true): RemoveNullValuesOutput {
if (value === null) {
remove(key);
return {value, wasRemoved: true};
Expand All @@ -976,7 +982,7 @@ function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>): RemoveNullVa
// We can remove all null values in an object by merging it with itself
// utils.fastMerge recursively goes through the object and removes all null values
// Passing two identical objects as source and target to fastMerge will not change it, but only remove the null values
return {value: utils.removeNestedNullValues(value as Record<string, unknown>), wasRemoved: false};
return {value: shouldRemoveNestedNulls ? utils.removeNestedNullValues(value as Record<string, unknown>) : (value as Record<string, unknown>), wasRemoved: false};
}

/**
Expand All @@ -986,28 +992,24 @@ function removeNullValues(key: OnyxKey, value: OnyxValue<OnyxKey>): RemoveNullVa

* @return an array of key - value pairs <[key, value]>
*/
function prepareKeyValuePairsForStorage(data: Record<OnyxKey, OnyxValue<OnyxKey>>): Array<[OnyxKey, OnyxValue<OnyxKey>]> {
const keyValuePairs: Array<[OnyxKey, OnyxValue<OnyxKey>]> = [];

Object.entries(data).forEach(([key, value]) => {
const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value);
function prepareKeyValuePairsForStorage(data: Record<OnyxKey, OnyxValue<OnyxKey>>, shouldRemoveNestedNulls: boolean): Array<[OnyxKey, OnyxValue<OnyxKey>]> {
return Object.entries(data).reduce<Array<[OnyxKey, OnyxValue<OnyxKey>]>>((pairs, [key, value]) => {
const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value, shouldRemoveNestedNulls);

if (wasRemoved) {
return;
if (!wasRemoved) {
pairs.push([key, valueAfterRemoving]);
}

keyValuePairs.push([key, valueAfterRemoving]);
});

return keyValuePairs;
return pairs;
}, []);
}

/**
* Merges an array of changes with an existing value
*
* @param changes Array of changes that should be applied to the existing value
*/
function applyMerge(existingValue: OnyxValue<OnyxKey>, changes: Array<OnyxValue<OnyxKey>>, shouldRemoveNullObjectValues: boolean): OnyxValue<OnyxKey> {
function applyMerge(existingValue: OnyxValue<OnyxKey>, changes: Array<OnyxValue<OnyxKey>>, shouldRemoveNestedNulls: boolean): OnyxValue<OnyxKey> {
const lastChange = changes?.at(-1);

if (Array.isArray(lastChange)) {
Expand All @@ -1017,7 +1019,7 @@ function applyMerge(existingValue: OnyxValue<OnyxKey>, changes: Array<OnyxValue<
if (changes.some((change) => change && typeof change === 'object')) {
// Object values are then merged one after the other
return changes.reduce(
(modifiedData, change) => utils.fastMerge(modifiedData as Record<OnyxKey, unknown>, change as Record<OnyxKey, unknown>, shouldRemoveNullObjectValues),
(modifiedData, change) => utils.fastMerge(modifiedData as Record<OnyxKey, unknown>, change as Record<OnyxKey, unknown>, shouldRemoveNestedNulls),
existingValue || {},
);
}
Expand Down
6 changes: 1 addition & 5 deletions lib/storage/providers/MemoryOnlyProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,7 @@ const provider: StorageProvider = {
multiSet(pairs) {
const setPromises = _.map(pairs, ([key, value]) => this.setItem(key, value));

return new Promise((resolve) => {
Promise.all(setPromises).then(() => {
resolve(undefined);
});
});
return Promise.all(setPromises).then(() => undefined);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed this return could be simplified. Not related to this PR

},

/**
Expand Down
30 changes: 15 additions & 15 deletions lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ function isMergeableObject(value: unknown): value is Record<string, unknown> {
* Merges the source object into the target object.
* @param target - The target object.
* @param source - The source object.
* @param shouldRemoveNullObjectValues - If true, null object values will be removed.
* @param shouldRemoveNestedNulls - If true, null object values will be removed.
* @returns - The merged object.
*/
function mergeObject<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject, shouldRemoveNullObjectValues = true): TObject {
function mergeObject<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject, shouldRemoveNestedNulls = true): TObject {
const destination: Record<string, unknown> = {};

// First we want to copy over all keys from the target into the destination object,
// in case "target" is a mergable object.
// If "shouldRemoveNullObjectValues" is true, we want to remove null values from the merged object
// If "shouldRemoveNestedNulls" is true, we want to remove null values from the merged object
// and therefore we need to omit keys where either the source or target value is null.
if (isMergeableObject(target)) {
const targetKeys = Object.keys(target);
Expand All @@ -41,10 +41,10 @@ function mergeObject<TObject extends Record<string, unknown>>(target: TObject |
const sourceValue = source?.[key];
const targetValue = target?.[key];

// If "shouldRemoveNullObjectValues" is true, we want to remove null values from the merged object.
// If "shouldRemoveNestedNulls" is true, we want to remove null values from the merged object.
// Therefore, if either target or source value is null, we want to prevent the key from being set.
const isSourceOrTargetNull = targetValue === null || sourceValue === null;
const shouldOmitTargetKey = shouldRemoveNullObjectValues && isSourceOrTargetNull;
const shouldOmitTargetKey = shouldRemoveNestedNulls && isSourceOrTargetNull;

if (!shouldOmitTargetKey) {
destination[key] = targetValue;
Expand All @@ -60,22 +60,22 @@ function mergeObject<TObject extends Record<string, unknown>>(target: TObject |
const targetValue = target?.[key];

// If undefined is passed as the source value for a key, we want to generally ignore it.
// If "shouldRemoveNullObjectValues" is set to true and the source value is null,
// If "shouldRemoveNestedNulls" is set to true and the source value is null,
// we don't want to set/merge the source value into the merged object.
const shouldIgnoreNullSourceValue = shouldRemoveNullObjectValues && sourceValue === null;
const shouldIgnoreNullSourceValue = shouldRemoveNestedNulls && sourceValue === null;
const shouldOmitSourceKey = sourceValue === undefined || shouldIgnoreNullSourceValue;

if (!shouldOmitSourceKey) {
// If the source value is a mergable object, we want to merge it into the target value.
// If "shouldRemoveNullObjectValues" is true, "fastMerge" will recursively
// If "shouldRemoveNestedNulls" is true, "fastMerge" will recursively
// remove nested null values from the merged object.
// If source value is any other value we need to set the source value it directly.
if (isMergeableObject(sourceValue)) {
// If the target value is null or undefined, we need to fallback to an empty object,
// so that we can still use "fastMerge" to merge the source value,
// to ensure that nested null values are removed from the merged object.
const targetValueWithFallback = (targetValue ?? {}) as TObject;
destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNullObjectValues);
destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNestedNulls);
} else {
destination[key] = sourceValue;
}
Expand All @@ -86,30 +86,30 @@ function mergeObject<TObject extends Record<string, unknown>>(target: TObject |
}

/**
* Merges two objects and removes null values if "shouldRemoveNullObjectValues" is set to true
* Merges two objects and removes null values if "shouldRemoveNestedNulls" is set to true
*
* We generally want to remove null values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
* On native, when merging an existing value with new changes, SQLite will use JSON_PATCH, which removes top-level nullish values.
* To be consistent with the behaviour for merge, we'll also want to remove null values for "set" operations.
*/
function fastMerge<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject | null, shouldRemoveNullObjectValues = true): TObject | null {
function fastMerge<TObject extends Record<string, unknown>>(target: TObject | null, source: TObject | null, shouldRemoveNestedNulls = true): TObject | null {
// We have to ignore arrays and nullish values here,
// otherwise "mergeObject" will throw an error,
// because it expects an object as "source"
if (Array.isArray(source) || source === null || source === undefined) {
return source;
}

return mergeObject(target, source, shouldRemoveNullObjectValues);
return mergeObject(target, source, shouldRemoveNestedNulls);
}

/** Deep removes the nested null values from the given value. */
function removeNestedNullValues(value: unknown[] | Record<string, unknown>): Record<string, unknown> | unknown[] | null {
function removeNestedNullValues<TObject extends Record<string, unknown>>(value: unknown | unknown[] | TObject): Record<string, unknown> | unknown[] | null {
if (typeof value === 'object' && !Array.isArray(value)) {
return fastMerge(value, value);
return fastMerge(value as Record<string, unknown> | null, value as Record<string, unknown> | null);
}

return value;
return value as Record<string, unknown> | null;
}

/** Formats the action name by uppercasing and adding the key if provided. */
Expand Down
3 changes: 2 additions & 1 deletion lib/withOnyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
// that should not be passed to a wrapped component
let stateToPass = _.omit(this.state, 'loading');
stateToPass = _.omit(stateToPass, _.isNull);
const stateToPassWithoutNestedNulls = utils.removeNestedNullValues(stateToPass);

// Spreading props and state is necessary in an HOC where the data cannot be predicted
return (
Expand All @@ -329,7 +330,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsToPass}
// eslint-disable-next-line react/jsx-props-no-spreading
{...stateToPass}
{...stateToPassWithoutNestedNulls}
ref={this.props.forwardedRef}
/>
);
Expand Down
74 changes: 74 additions & 0 deletions tests/unit/onyxTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -1071,4 +1071,78 @@ describe('Onyx', () => {
expect(testKeyValue).toEqual(null);
});
});

it('should merge a non-existing key with a nested null removed', () => {
let testKeyValue;

connectionID = Onyx.connect({
key: ONYX_KEYS.TEST_KEY,
initWithStoredValues: false,
callback: (value) => {
testKeyValue = value;
},
});

return Onyx.merge(ONYX_KEYS.TEST_KEY, {
waypoints: {
1: 'Home',
2: 'Work',
3: null,
},
}).then(() => {
expect(testKeyValue).toEqual({
waypoints: {
1: 'Home',
2: 'Work',
},
});
});
});

it('mergeCollection should omit nested null values', () => {
let result;

const routineRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}routine`;
const holidayRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}holiday`;

connectionID = Onyx.connect({
key: ONYX_KEYS.COLLECTION.TEST_KEY,
initWithStoredValues: false,
callback: (value) => (result = value),
waitForCollectionCallback: true,
});

return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[routineRoute]: {
waypoints: {
1: 'Home',
2: 'Work',
3: 'Gym',
},
},
[holidayRoute]: {
waypoints: {
1: 'Home',
2: 'Beach',
3: null,
},
},
}).then(() => {
expect(result).toEqual({
[routineRoute]: {
waypoints: {
1: 'Home',
2: 'Work',
3: 'Gym',
},
},
[holidayRoute]: {
waypoints: {
1: 'Home',
2: 'Beach',
},
},
});
});
});
});
Loading