chore: pr-comments

Author: ryanbas21

.changeset/fix-storage-error-handling.md

@@ -0,0 +1,11 @@
+---
+'@forgerock/davinci-client': patch
+'@forgerock/sdk-utilities': patch
+'@forgerock/storage': patch
+---
+
+Fix error handling in storage client and davinci-client
+
+- Add `isGenericError` type guard to sdk-utilities for runtime error validation
+- Fix storage client to properly catch errors from custom storage implementations, honoring the errors-as-values contract
+- Improve davinci-client error handling to use explicit error checks instead of try-catch

packages/davinci-client/src/lib/client.store.ts

@@ -206,45 +206,59 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
     }: {
       continueToken: string;
     }): Promise<InternalErrorResponse | NodeStates> => {
-      const storedServerInfo = await serverInfo.get();
+      try {
+        const storedServerInfo = await serverInfo.get();
 
-      if (storedServerInfo === null) {
-        log.error('No server info found in storage for resume operation');
-        return {
-          error: {
-            message:
-              'No server info found in storage. Social login needs server info which is saved in local storage. You may have cleared your browser data.',
-            type: 'state_error',
-          },
-          type: 'internal_error',
-        };
-      }
+        if (storedServerInfo === null) {
+          log.error('No server info found in storage for resume operation');
+          return {
+            error: {
+              message:
+                'No server info found in storage. Social login needs server info which is saved in local storage. You may have cleared your browser data.',
+              type: 'state_error',
+            },
+            type: 'internal_error',
+          };
+        }
+
+        if (isGenericError(storedServerInfo)) {
+          log.error(storedServerInfo.message ?? storedServerInfo.error);
+          return {
+            error: {
+              message:
+                storedServerInfo.message ??
+                'Failed to retrieve server info from storage for resume operation',
+              type: 'internal_error',
+            },
+            type: 'internal_error',
+          };
+        }
 
-      if (isGenericError(storedServerInfo)) {
-        log.error(storedServerInfo.message ?? storedServerInfo.error);
+        await store.dispatch(
+          davinciApi.endpoints.resume.initiate({ continueToken, serverInfo: storedServerInfo }),
+        );
+
+        const removeResult = await serverInfo.remove();
+        if (isGenericError(removeResult)) {
+          log.warn(
+            removeResult.message ?? 'Failed to remove server info from storage after resume',
+          );
+        }
+
+        const node = nodeSlice.selectSlice(store.getState());
+
+        return node;
+      } catch (err) {
+        const error = err as Error;
+        log.error(error.message);
         return {
           error: {
-            message:
-              storedServerInfo.message ??
-              'Failed to retrieve server info from storage for resume operation',
+            message: error.message ?? 'An unexpected error occurred during resume operation',
             type: 'internal_error',
           },
           type: 'internal_error',
         };
       }
-
-      await store.dispatch(
-        davinciApi.endpoints.resume.initiate({ continueToken, serverInfo: storedServerInfo }),
-      );
-
-      const removeResult = await serverInfo.remove();
-      if (isGenericError(removeResult)) {
-        log.warn(removeResult.message ?? 'Failed to remove server info from storage after resume');
-      }
-
-      const node = nodeSlice.selectSlice(store.getState());
-
-      return node;
     },
 
     /**

packages/sdk-effects/storage/src/lib/storage.effects.ts

@@ -68,7 +68,13 @@ export function createStorage<Value>(config: StorageConfig): StorageClient<Value
   return {
     get: async function storageGet(): Promise<Value | GenericError | null> {
       if (storeType === 'custom') {
-        const value = await config.custom.get(key);
+        let value: Awaited<ReturnType<typeof config.custom.get>>;
+        try {
+          value = await config.custom.get(key);
+        } catch {
+          return createStorageError(storeType, 'Retrieving');
+        }
+
         if (value === null || (typeof value === 'object' && 'error' in value)) {
           return value;
         }
@@ -101,8 +107,12 @@ export function createStorage<Value>(config: StorageConfig): StorageClient<Value
     set: async function storageSet(value: Value): Promise<GenericError | null> {
       const valueToStore = JSON.stringify(value);
       if (storeType === 'custom') {
-        const value = await config.custom.set(key, valueToStore);
-        return Promise.resolve(value ?? null);
+        try {
+          const result = await config.custom.set(key, valueToStore);
+          return result ?? null;
+        } catch {
+          return createStorageError(storeType, 'Storing');
+        }
       }
 
       try {
@@ -114,8 +124,12 @@ export function createStorage<Value>(config: StorageConfig): StorageClient<Value
     },
     remove: async function storageRemove(): Promise<GenericError | null> {
       if (storeType === 'custom') {
-        const value = await config.custom.remove(key);
-        return Promise.resolve(value ?? null);
+        try {
+          const result = await config.custom.remove(key);
+          return result ?? null;
+        } catch {
+          return createStorageError(storeType, 'Removing');
+        }
       }
 
       try {

packages/sdk-utilities/src/lib/error/error.utils.test.ts

@@ -156,6 +156,20 @@ describe('isGenericError', () => {
       expect(result).toBe(false);
     });
 
+    it('isGenericError_ObjectWithNonStringType_ReturnsFalse', () => {
+      // Arrange
+      const value = {
+        error: 'storage_error',
+        type: 123,
+      };
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
     it('isGenericError_ArrayValue_ReturnsFalse', () => {
       // Arrange
       const value = ['error', 'type'];

packages/sdk-utilities/src/lib/error/error.utils.ts

@@ -19,6 +19,7 @@ export function isGenericError(value: unknown): value is GenericError {
     value !== null &&
     'error' in value &&
     typeof (value as GenericError).error === 'string' &&
-    'type' in value
+    'type' in value &&
+    typeof (value as GenericError).type === 'string'
   );
 }