Merge pull request #532 from dominictb/fix/39852

Add some basic runtime type validation in Onyx
Expensify · Apr 25, 2024 · c245727 · c245727
2 parents ab8222d + 1de05cf
commit c245727
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 7 deletions.
diff --git 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<TKey extends OnyxKey>(key: TKey, value: OnyxEntry<KeyValueMapping[TKey]>): Promise<void> {
+    // 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<TKey>;
@@ -307,7 +316,18 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxEntry<NullishDeep<K
         try {
             // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge)
             // We don't want to remove null values from the "batchedDeltaChanges", because SQLite uses them to remove keys from storage natively.
-            const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, mergeQueue[key], false);
+            const validChanges = mergeQueue[key].filter((change) => {
+                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<TKey extends CollectionKeyBase, TMap>(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<TKey extends CollectionKeyBase, TMap>(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
@@ -401,10 +401,10 @@ function addAllSafeEvictionKeysToRecentlyAccessedList(): Promise<void> {
     });
 }
 
-function getCachedCollection<TKey extends CollectionKeyBase>(collectionKey: TKey): NonNullable<OnyxCollection<KeyValueMapping[TKey]>> {
-    const collectionMemberKeys = Array.from(cache.getAllKeys()).filter((storedKey) => isCollectionMemberKey(collectionKey, storedKey));
+function getCachedCollection<TKey extends CollectionKeyBase>(collectionKey: TKey, collectionMemberKeys?: string[]): NonNullable<OnyxCollection<KeyValueMapping[TKey]>> {
+    const resolvedCollectionMemberKeys = collectionMemberKeys || Array.from(cache.getAllKeys()).filter((storedKey) => isCollectionMemberKey(collectionKey, storedKey));
 
-    return collectionMemberKeys.reduce((prev: NonNullable<OnyxCollection<KeyValueMapping[TKey]>>, key) => {
+    return resolvedCollectionMemberKeys.reduce((prev: NonNullable<OnyxCollection<KeyValueMapping[TKey]>>, key) => {
         const cachedValue = cache.getValue(key);
         if (!cachedValue) {
             return prev;

diff --git a/lib/logMessages.ts 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
@@ -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
@@ -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',