diff --git a/lib/Onyx.ts b/lib/Onyx.ts index baf7c0682..7df052318 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -320,7 +320,7 @@ function merge(key: TKey, changes: OnyxMergeInput): if (!validChanges.length) { return Promise.resolve(); } - const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, validChanges, false); + const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, validChanges, false, true); // Case (1): When there is no existing value in storage, we want to set the value instead of merge it. // Case (2): The presence of a top-level `null` in the merge queue instructs us to drop the whole existing value. @@ -350,7 +350,7 @@ function merge(key: TKey, changes: OnyxMergeInput): // The "preMergedValue" will be directly "set" in storage instead of being merged // Therefore we merge the batched changes with the existing value to get the final merged value that will be stored. // We can remove null values from the "preMergedValue", because "null" implicates that the user wants to remove a value from storage. - const preMergedValue = OnyxUtils.applyMerge(shouldSetValue ? undefined : existingValue, [batchedDeltaChanges], true); + const preMergedValue = OnyxUtils.applyMerge(shouldSetValue ? undefined : existingValue, [batchedDeltaChanges], true, false); // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. const hasChanged = cache.hasValueChanged(key, preMergedValue); @@ -765,7 +765,7 @@ function update(data: OnyxUpdate[]): Promise { // Remove the collection-related key from the updateQueue so that it won't be processed individually. delete updateQueue[key]; - const updatedValue = OnyxUtils.applyMerge(undefined, operations, false); + const updatedValue = OnyxUtils.applyMerge(undefined, operations, false, true); if (operations[0] === null) { // eslint-disable-next-line no-param-reassign queue.set[key] = updatedValue; @@ -790,7 +790,7 @@ function update(data: OnyxUpdate[]): Promise { }); Object.entries(updateQueue).forEach(([key, operations]) => { - const batchedChanges = OnyxUtils.applyMerge(undefined, operations, false); + const batchedChanges = OnyxUtils.applyMerge(undefined, operations, false, true); if (operations[0] === null) { promises.push(() => set(key, batchedChanges)); diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index a3ef656a8..dacd69f73 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1249,6 +1249,7 @@ function applyMerge | undefined, TChange exten existingValue: TValue, changes: TChange[], shouldRemoveNestedNulls: boolean, + isBatchingMergeChanges: boolean, ): TChange { const lastChange = changes?.at(-1); @@ -1258,7 +1259,7 @@ function applyMerge | undefined, TChange exten 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, change, shouldRemoveNestedNulls), (existingValue || {}) as TChange); + return changes.reduce((modifiedData, change) => utils.fastMerge(modifiedData, change, shouldRemoveNestedNulls, isBatchingMergeChanges), (existingValue || {}) as TChange); } // If we have anything else we can't merge it so we'll diff --git a/lib/utils.ts b/lib/utils.ts index cdd04a54e..011473968 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -27,7 +27,12 @@ function isMergeableObject(value: unknown): value is Record { * @param shouldRemoveNestedNulls - If true, null object values will be removed. * @returns - The merged object. */ -function mergeObject>(target: TObject | unknown | null | undefined, source: TObject, shouldRemoveNestedNulls = true): TObject { +function mergeObject>( + target: TObject | unknown | null | undefined, + source: TObject, + shouldRemoveNestedNulls = true, + isBatchingMergeChanges = false, +): TObject { const destination: Record = {}; const targetObject = isMergeableObject(target) ? target : undefined; @@ -74,11 +79,17 @@ function mergeObject>(target: TObject | // 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, shouldRemoveNestedNulls); + if (isBatchingMergeChanges && targetValue === null) { + destination[key] = null; + } else { + // 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, shouldRemoveNestedNulls, isBatchingMergeChanges); + } + } else if (isBatchingMergeChanges && destination[key] === null) { + destination[key] = null; } else { destination[key] = sourceValue; } @@ -95,7 +106,7 @@ function mergeObject>(target: TObject | * 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(target: TValue, source: TValue, shouldRemoveNestedNulls = true): TValue { +function fastMerge(target: TValue, source: TValue, shouldRemoveNestedNulls = true, isBatchingMergeChanges = false): TValue { // We have to ignore arrays and nullish values here, // otherwise "mergeObject" will throw an error, // because it expects an object as "source" @@ -103,7 +114,7 @@ function fastMerge(target: TValue, source: TValue, shouldRemoveNestedNul return source; } - return mergeObject(target, source as Record, shouldRemoveNestedNulls) as TValue; + return mergeObject(target, source as Record, shouldRemoveNestedNulls, isBatchingMergeChanges) as TValue; } /** Deep removes the nested null values from the given value. */ diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 44400e715..8dcee71a9 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -1686,7 +1686,344 @@ describe('Onyx', () => { }); }); + it('should replace the old value after a null merge in the top-level object when batching updates', async () => { + let result: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_UPDATE, + waitForCollectionCallback: true, + callback: (value) => { + result = value; + }, + }); + + await Onyx.multiSet({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: { + id: 'entry1', + someKey: 'someValue', + }, + }); + + const queuedUpdates: OnyxUpdate[] = [ + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, + onyxMethod: 'merge', + // Removing the entire object in this update. + // Any subsequent changes to this key should completely replace the old value. + value: null, + }, + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, + onyxMethod: 'merge', + // This change should completely replace `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1` old value. + value: { + someKey: 'someValueChanged', + }, + }, + ]; + + await Onyx.update(queuedUpdates); + + expect(result).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: { + someKey: 'someValueChanged', + }, + }); + }); + + it('should ignore subsequent changes after a null merge in a nested property when batching updates', async () => { + let result: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_UPDATE, + waitForCollectionCallback: true, + callback: (value) => { + result = value; + }, + }); + + await Onyx.multiSet({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: { + sub_entry1: { + id: 'sub_entry1', + someKey: 'someValue', + }, + }, + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`]: { + sub_entry2: { + id: 'sub_entry2', + someKey: 'someValue', + someNestedObject: { + someNestedKey: 'someNestedValue', + anotherNestedObject: { + anotherNestedKey: 'anotherNestedValue', + }, + }, + }, + }, + }); + + const queuedUpdates: OnyxUpdate[] = [ + // entry1 + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, + onyxMethod: 'merge', + value: { + // Removing "sub_entry1" property in this update. + // Any subsequent changes to this property should be ignored. + sub_entry1: null, + }, + }, + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, + onyxMethod: 'merge', + value: { + // This change should be ignored because we previously changed "sub_entry1" to null. + sub_entry1: { + newKey: 'newValue', + }, + }, + }, + + // entry2 + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, + onyxMethod: 'merge', + value: { + sub_entry2: { + someKey: 'someValueChanged', + }, + }, + }, + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, + onyxMethod: 'merge', + value: { + sub_entry2: { + someNestedObject: { + anotherNestedObject: { + anotherNestedKey: 'anotherNestedValueChanged', + }, + anotherNestedObject2: { + anotherNestedKey2: 'anotherNestedValue2', + }, + }, + }, + }, + }, + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, + onyxMethod: 'merge', + value: { + sub_entry2: { + someNestedObject: { + anotherNestedObject: { + // Removing "anotherNestedKey" property in this update. + // Any subsequent changes to this property should be ignored. + anotherNestedKey: null, + }, + }, + }, + }, + }, + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, + onyxMethod: 'merge', + value: { + sub_entry2: { + someNestedObject: { + anotherNestedObject: { + // This change should be ignored because we previously changed "anotherNestedKey" to null. + anotherNestedKey: 'anotherNestedValueChanged2', + }, + // Removing "anotherNestedObject2" property in this update. + // Any subsequent changes to this property should be ignored. + anotherNestedObject2: null, + }, + }, + }, + }, + { + key: `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, + onyxMethod: 'merge', + value: { + sub_entry2: { + someNestedObject: { + // This change should be ignored because we previously changed "anotherNestedObject2" to null. + anotherNestedObject2: { + anotherNestedKey2: 'anotherNestedValue2', + }, + }, + }, + }, + }, + ]; + + await Onyx.update(queuedUpdates); + + expect(result).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: {}, + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`]: { + sub_entry2: { + id: 'sub_entry2', + someKey: 'someValueChanged', + someNestedObject: { + someNestedKey: 'someNestedValue', + anotherNestedObject: {}, + }, + }, + }, + }); + }); + describe('merge', () => { + it('should replace the old value after a null merge in the top-level object when batching merges', async () => { + let result: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_UPDATE, + waitForCollectionCallback: true, + callback: (value) => { + result = value; + }, + }); + + await Onyx.multiSet({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: { + id: 'entry1', + someKey: 'someValue', + }, + }); + + // Removing the entire object in this merge. + // Any subsequent changes to this key should completely replace the old value. + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, null); + + // This change should completely replace `${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1` old value. + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, { + someKey: 'someValueChanged', + }); + + await waitForPromisesToResolve(); + + expect(result).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: { + someKey: 'someValueChanged', + }, + }); + }); + + it('should ignore subsequent changes after a null merge in a nested property when batching merges', async () => { + let result: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_UPDATE, + waitForCollectionCallback: true, + callback: (value) => { + result = value; + }, + }); + + await Onyx.multiSet({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: { + sub_entry1: { + id: 'sub_entry1', + someKey: 'someValue', + }, + }, + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`]: { + sub_entry2: { + id: 'sub_entry2', + someKey: 'someValue', + someNestedObject: { + someNestedKey: 'someNestedValue', + anotherNestedObject: { + anotherNestedKey: 'anotherNestedValue', + }, + }, + }, + }, + }); + + // entry1 + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, { + // Removing "sub_entry1" property in this merge. + // Any subsequent changes to this property should be ignored. + sub_entry1: null, + }); + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`, { + // This change should be ignored because we previously changed "sub_entry1" to null. + sub_entry1: { + pendingAction: null, + }, + }); + + // entry2 + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, { + sub_entry2: { + someKey: 'someValueChanged', + }, + }); + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, { + sub_entry2: { + someNestedObject: { + anotherNestedObject: { + anotherNestedKey: 'anotherNestedValueChanged', + }, + anotherNestedObject2: { + anotherNestedKey2: 'anotherNestedValue2', + }, + }, + }, + }); + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, { + sub_entry2: { + someNestedObject: { + anotherNestedObject: { + // Removing "anotherNestedKey" property in this merge. + // Any subsequent changes to this property should be ignored. + anotherNestedKey: null, + }, + }, + }, + }); + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, { + sub_entry2: { + someNestedObject: { + anotherNestedObject: { + // This change should be ignored because we previously changed "anotherNestedKey" to null. + anotherNestedKey: 'anotherNestedValueChanged2', + }, + // Removing "anotherNestedObject2" property in this merge. + // Any subsequent changes to this property should be ignored. + anotherNestedObject2: null, + }, + }, + }); + Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`, { + sub_entry2: { + someNestedObject: { + // This change should be ignored because we previously changed "anotherNestedObject2" to null. + anotherNestedObject2: { + anotherNestedKey2: 'anotherNestedValue2', + }, + }, + }, + }); + + await waitForPromisesToResolve(); + + expect(result).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry1`]: {}, + [`${ONYX_KEYS.COLLECTION.TEST_UPDATE}entry2`]: { + sub_entry2: { + id: 'sub_entry2', + someKey: 'someValueChanged', + someNestedObject: { + someNestedKey: 'someNestedValue', + anotherNestedObject: {}, + }, + }, + }, + }); + }); + it('should remove a deeply nested null when merging an existing key', () => { let result: unknown;