Merge pull request Expensify#737 from callstack-internal/JKobrynski/feat/81830-remove-try-catch-from-getcollectionkey

[Onyx Audit] Remove try/catch block from getCollectionKey

Author: roryabraham

lib/Onyx.ts

@@ -346,13 +346,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
                         if (newValue !== oldValue) {
                             cache.set(key, newValue);
 
-                            let collectionKey: string | undefined;
-                            try {
-                                collectionKey = OnyxUtils.getCollectionKey(key);
-                            } catch (e) {
-                                // If getCollectionKey() throws an error it means the key is not a collection key.
-                                collectionKey = undefined;
-                            }
+                            const collectionKey = OnyxUtils.getCollectionKey(key);
 
                             if (collectionKey) {
                                 if (!keyValuesToResetAsCollection[collectionKey]) {

lib/OnyxCache.ts

@@ -458,13 +458,13 @@ class OnyxCache {
     /**
      * Get the collection key for a given member key
      */
-    getCollectionKey(key: OnyxKey): OnyxKey | null {
+    getCollectionKey(key: OnyxKey): OnyxKey | undefined {
         for (const collectionKey of this.collectionKeys) {
             if (key.startsWith(collectionKey) && key.length > collectionKey.length) {
                 return collectionKey;
             }
         }
-        return null;
+        return undefined;
     }
 
     /**

lib/OnyxSnapshotCache.ts

@@ -131,12 +131,10 @@ class OnyxSnapshotCache {
         // Always invalidate the exact key
         this.snapshotCache.delete(keyToInvalidate);
 
-        try {
-            // Check if the key is a collection member and invalidate the collection base key
-            const collectionBaseKey = OnyxUtils.getCollectionKey(keyToInvalidate);
+        // Check if the key is a collection member and invalidate the collection base key
+        const collectionBaseKey = OnyxUtils.getCollectionKey(keyToInvalidate);
+        if (collectionBaseKey) {
             this.snapshotCache.delete(collectionBaseKey);
-        } catch (e) {
-            // do nothing - this just means the key is not a collection member
         }
     }
 

lib/OnyxUtils.ts

@@ -475,14 +475,9 @@ function isCollectionMemberKey<TCollectionKey extends CollectionKeyBase>(collect
  * @returns true if the key is a collection member, false otherwise
  */
 function isCollectionMember(key: OnyxKey): boolean {
-    try {
-        const collectionKey = getCollectionKey(key);
-        // If the key is longer than the collection key, it's a collection member
-        return key.length > collectionKey.length;
-    } catch (e) {
-        // If getCollectionKey throws, the key is not a collection member
-        return false;
-    }
+    const collectionKey = getCollectionKey(key);
+    // If the key is longer than the collection key, it's a collection member
+    return !!collectionKey && key.length > collectionKey.length;
 }
 
 /**
@@ -503,12 +498,9 @@ function isCollectionMember(key: OnyxKey): boolean {
  * @returns true if key is a RAM-only key, RAM-only collection key, or a RAM-only collection member
  */
 function isRamOnlyKey(key: OnyxKey): boolean {
-    try {
-        const collectionKey = getCollectionKey(key);
-        // If collectionKey exists for a given key, check if it's a RAM-only key
+    const collectionKey = getCollectionKey(key);
+    if (collectionKey) {
         return cache.isRamOnlyKey(collectionKey);
-    } catch {
-        // If getCollectionKey throws, the key is not a collection member
     }
 
     return cache.isRamOnlyKey(key);
@@ -530,8 +522,12 @@ function splitCollectionMemberKey<TKey extends CollectionKey, CollectionKeyType
     }
 
     if (!collectionKey) {
+        const resolvedKey = getCollectionKey(key);
+        if (!resolvedKey) {
+            throw new Error(`Invalid '${key}' key provided, only collection keys are allowed.`);
+        }
         // eslint-disable-next-line no-param-reassign
-        collectionKey = getCollectionKey(key);
+        collectionKey = resolvedKey;
     }
 
     return [collectionKey as CollectionKeyType, key.slice(collectionKey.length)];
@@ -555,9 +551,9 @@ function isKeyMatch(configKey: OnyxKey, key: OnyxKey): boolean {
  * - `getCollectionKey("sharedNVP_user_-1_something")` would return "sharedNVP_user_"
  *
  * @param key - The collection key to process.
- * @returns The plain collection key or throws an Error if the key is not a collection one.
+ * @returns The plain collection key or undefined if the key is not a collection one.
  */
-function getCollectionKey(key: CollectionKey): string {
+function getCollectionKey(key: CollectionKey): string | undefined {
     // Start by finding the position of the last underscore in the string
     let lastUnderscoreIndex = key.lastIndexOf('_');
 
@@ -575,7 +571,7 @@ function getCollectionKey(key: CollectionKey): string {
         lastUnderscoreIndex = key.lastIndexOf('_', lastUnderscoreIndex - 1);
     }
 
-    throw new Error(`Invalid '${key}' key provided, only collection keys are allowed.`);
+    return undefined;
 }
 
 /**
@@ -754,13 +750,7 @@ function keyChanged<TKey extends OnyxKey>(
     // do the same in keysChanged, because we only call that function when a collection key changes, and it doesn't happen that often.
     // For performance reason, we look for the given key and later if don't find it we look for the collection key, instead of checking if it is a collection key first.
     let stateMappingKeys = onyxKeyToSubscriptionIDs.get(key) ?? [];
-    let collectionKey: string | undefined;
-    try {
-        collectionKey = getCollectionKey(key);
-    } catch (e) {
-        // If getCollectionKey() throws an error it means the key is not a collection key.
-        collectionKey = undefined;
-    }
+    const collectionKey = getCollectionKey(key);
 
     if (collectionKey) {
         // Getting the collection key from the specific key because only collection keys were stored in the mapping.

tests/unit/OnyxSnapshotCacheTest.ts

@@ -156,6 +156,7 @@ describe('OnyxSnapshotCache', () => {
 
         it('should invalidate non-collection keys without affecting others', () => {
             mockedOnyxUtils.isCollectionKey.mockReturnValue(false);
+            mockedOnyxUtils.getCollectionKey.mockReturnValue(undefined);
 
             cache.invalidateForKey('nonCollectionKey');
 

tests/unit/onyxUtilsTest.ts

@@ -342,13 +342,9 @@ describe('OnyxUtils', () => {
             });
         });
 
-        it('should throw error if key does not contain underscore', () => {
-            expect(() => {
-                OnyxUtils.getCollectionKey(ONYXKEYS.TEST_KEY);
-            }).toThrowError("Invalid 'test' key provided, only collection keys are allowed.");
-            expect(() => {
-                OnyxUtils.getCollectionKey('');
-            }).toThrowError("Invalid '' key provided, only collection keys are allowed.");
+        it('should return undefined if key does not contain underscore', () => {
+            expect(OnyxUtils.getCollectionKey(ONYXKEYS.TEST_KEY)).toBeUndefined();
+            expect(OnyxUtils.getCollectionKey('')).toBeUndefined();
         });
     });