diff --git a/lib/Onyx.ts b/lib/Onyx.ts index ff9927c1d..8d1b2832f 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -23,6 +23,7 @@ import type { OnyxValue, } from './types'; import OnyxUtils from './OnyxUtils'; +import logMessages from './logMessages'; // Keeps track of the last connectionID that was used so we can keep incrementing it let lastConnectionID = 0; @@ -208,6 +209,14 @@ function disconnect(connectionID: number, keyToRemoveFromEvictionBlocklist?: Ony * @param value value to store */ function set(key: TKey, value: OnyxEntry): Promise { + // check if the value is compatible with the existing value in the storage + const existingValue = cache.getValue(key, false); + const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(value, existingValue); + if (!isCompatible) { + Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'set', existingValueType, newValueType)); + return Promise.resolve(); + } + // If the value is null, we remove the key from storage const {value: valueAfterRemoving, wasRemoved} = OnyxUtils.removeNullValues(key, value); const valueWithoutNullValues = valueAfterRemoving as OnyxValue; @@ -307,7 +316,18 @@ function merge(key: TKey, changes: OnyxEntry { + const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(change, existingValue); + if (!isCompatible) { + Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'merge', existingValueType, newValueType)); + } + return isCompatible; + }); + + if (!validChanges.length) { + return undefined; + } + const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, validChanges, false); // 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. @@ -406,9 +426,17 @@ function mergeCollection(collectionKey: TK }); const existingKeys = keys.filter((key) => persistedKeys.has(key)); + + const cachedCollectionForExistingKeys = OnyxUtils.getCachedCollection(collectionKey, existingKeys); + const newKeys = keys.filter((key) => !persistedKeys.has(key)); const existingKeyCollection = existingKeys.reduce((obj: NullableKeyValueMapping, key) => { + const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(mergedCollection[key], cachedCollectionForExistingKeys[key]); + if (!isCompatible) { + Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'mergeCollection', existingValueType, newValueType)); + return obj; + } // eslint-disable-next-line no-param-reassign obj[key] = mergedCollection[key]; return obj; @@ -441,11 +469,14 @@ function mergeCollection(collectionKey: TK promises.push(Storage.multiSet(keyValuePairsForNewCollection)); } + // finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates + const finalMergedCollection = {...existingKeyCollection, ...newCollection}; + // Prefill cache if necessary by calling get() on any existing keys and then merge original data to cache // and update all subscribers const promiseUpdate = Promise.all(existingKeys.map(OnyxUtils.get)).then(() => { - cache.merge(mergedCollection); - return OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mergedCollection); + cache.merge(finalMergedCollection); + return OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection); }); return Promise.all(promises) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index fe1b9b3f7..71a3af7ea 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -401,10 +401,10 @@ function addAllSafeEvictionKeysToRecentlyAccessedList(): Promise { }); } -function getCachedCollection(collectionKey: TKey): NonNullable> { - const collectionMemberKeys = Array.from(cache.getAllKeys()).filter((storedKey) => isCollectionMemberKey(collectionKey, storedKey)); +function getCachedCollection(collectionKey: TKey, collectionMemberKeys?: string[]): NonNullable> { + const resolvedCollectionMemberKeys = collectionMemberKeys || Array.from(cache.getAllKeys()).filter((storedKey) => isCollectionMemberKey(collectionKey, storedKey)); - return collectionMemberKeys.reduce((prev: NonNullable>, key) => { + return resolvedCollectionMemberKeys.reduce((prev: NonNullable>, key) => { const cachedValue = cache.getValue(key); if (!cachedValue) { return prev; diff --git a/lib/logMessages.ts b/lib/logMessages.ts new file mode 100644 index 000000000..97a249da7 --- /dev/null +++ b/lib/logMessages.ts @@ -0,0 +1,7 @@ +const logMessages = { + incompatibleUpdateAlert: (key: string, operation: string, existingValueType?: string, newValueType?: string) => { + return `Warning: Trying to apply "${operation}" with ${newValueType ?? 'unknown'} type to ${existingValueType ?? 'unknown'} type in the key "${key}"`; + }, +}; + +export default logMessages; diff --git a/lib/utils.ts b/lib/utils.ts index e29e472fe..64c56c0da 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -117,4 +117,25 @@ function formatActionName(method: string, key?: OnyxKey): string { return key ? `${method.toUpperCase()}/${key}` : method.toUpperCase(); } -export default {isEmptyObject, fastMerge, formatActionName, removeNestedNullValues}; +/** validate that the update and the existing value are compatible */ +function checkCompatibilityWithExistingValue(value: unknown, existingValue: unknown): {isCompatible: boolean; existingValueType?: string; newValueType?: string} { + if (!existingValue || !value) { + return { + isCompatible: true, + }; + } + const existingValueType = Array.isArray(existingValue) ? 'array' : 'non-array'; + const newValueType = Array.isArray(value) ? 'array' : 'non-array'; + if (existingValueType !== newValueType) { + return { + isCompatible: false, + existingValueType, + newValueType, + }; + } + return { + isCompatible: true, + }; +} + +export default {isEmptyObject, fastMerge, formatActionName, removeNestedNullValues, checkCompatibilityWithExistingValue}; diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index faeda0681..e1a2c3957 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -72,6 +72,27 @@ describe('Onyx', () => { }); }); + it('should not set the key if the value is incompatible (array vs object)', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, ['test']) + .then(() => { + expect(testKeyValue).toStrictEqual(['test']); + return Onyx.set(ONYX_KEYS.TEST_KEY, {test: 'test'}); + }) + .then(() => { + expect(testKeyValue).toStrictEqual(['test']); + }); + }); + it('should merge an object with another object', () => { let testKeyValue; @@ -93,6 +114,27 @@ describe('Onyx', () => { }); }); + it('should not merge if the value is incompatible (array vs object)', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.merge(ONYX_KEYS.TEST_KEY, ['test']) + .then(() => { + expect(testKeyValue).toStrictEqual(['test']); + return Onyx.merge(ONYX_KEYS.TEST_KEY, {test2: 'test2'}); + }) + .then(() => { + expect(testKeyValue).toStrictEqual(['test']); + }); + }); + it('should notify subscribers when data has been cleared', () => { let testKeyValue; connectionID = Onyx.connect({ @@ -389,6 +431,7 @@ describe('Onyx', () => { ID: 234, value: 'four', }, + test_3: ['abc', 'xyz'], // This shouldn't be merged since it's an array, and the original value is an object {ID, value} test_4: { ID: 456, value: 'two',