fix: update-storage-client-error-handling

.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/package.json

@@ -27,6 +27,7 @@
     "@forgerock/sdk-oidc": "workspace:*",
     "@forgerock/sdk-request-middleware": "workspace:*",
     "@forgerock/sdk-types": "workspace:*",
+    "@forgerock/sdk-utilities": "workspace:*",
     "@forgerock/storage": "workspace:*",
     "@reduxjs/toolkit": "catalog:",
     "effect": "catalog:effect",

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

@@ -9,6 +9,7 @@
  */
 import { CustomLogger, logger as loggerFn, LogLevel } from '@forgerock/sdk-logger';
 import { createStorage } from '@forgerock/storage';
+import { isGenericError } from '@forgerock/sdk-utilities';
 
 import { createClientStore, handleUpdateValidateError, RootState } from './client.store.utils.js';
 import { nodeSlice } from './node.slice.js';
@@ -126,7 +127,17 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
 
       if (serverSlice && serverSlice.status === 'continue') {
         return async () => {
-          await serverInfo.set(serverSlice);
+          const setResult = await serverInfo.set(serverSlice);
+          if (isGenericError(setResult)) {
+            log.error(setResult.message ?? setResult.error);
+            return {
+              error: {
+                message: setResult.message ?? 'Failed to store server info for external IDP flow',
+                type: 'internal_error',
+              },
+            } as InternalErrorResponse;
+          }
+          return;
         };
       }
       return async () => {
@@ -196,21 +207,53 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
       continueToken: string;
     }): Promise<InternalErrorResponse | NodeStates> => {
       try {
-        const storedServerInfo = (await serverInfo.get()) as ContinueNode['server'];
+        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 (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',
+          };
+        }
+
         await store.dispatch(
           davinciApi.endpoints.resume.initiate({ continueToken, serverInfo: storedServerInfo }),
         );
-        await serverInfo.remove();
+
+        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 {
-        // logger.error('No url found in collector, social login needs a url in the collector');
+      } catch (err) {
+        const error = err as Error;
+        log.error(error.message);
         return {
           error: {
-            message:
-              'No url found in storage, social login needs a continue url which is saved in local storage. You may have cleared your browser data',
+            message: error.message ?? 'An unexpected error occurred during resume operation',
             type: 'internal_error',
           },
           type: 'internal_error',

packages/davinci-client/tsconfig.json

@@ -13,6 +13,9 @@
     {
       "path": "../sdk-effects/storage"
     },
+    {
+      "path": "../sdk-utilities"
+    },
     {
       "path": "../sdk-types"
     },

packages/davinci-client/tsconfig.lib.json

@@ -34,6 +34,9 @@
     {
       "path": "../sdk-effects/storage/tsconfig.lib.json"
     },
+    {
+      "path": "../sdk-utilities/tsconfig.lib.json"
+    },
     {
       "path": "../sdk-types/tsconfig.lib.json"
     },

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/package.json

@@ -36,6 +36,9 @@
     "test": "pnpm nx nxTest",
     "test:watch": "pnpm nx nxTest --watch"
   },
+  "dependencies": {
+    "@forgerock/sdk-types": "workspace:*"
+  },
   "nx": {
     "tags": ["scope:sdk-utilities"]
   }

packages/sdk-utilities/src/index.ts

@@ -7,6 +7,7 @@
  *
  */
 
+export * from './lib/error/index.js';
 export * from './lib/oidc/index.js';
 export * from './lib/strings/index.js';
 export * from './lib/url/index.js';

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

@@ -0,0 +1,199 @@
+/*
+ * Copyright (c) 2025 Ping Identity Corporation. All rights reserved.
+ *
+ * This software may be modified and distributed under the terms
+ * of the MIT license. See the LICENSE file for details.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { isGenericError } from './error.utils.js';
+import type { GenericError } from '@forgerock/sdk-types';
+
+describe('isGenericError', () => {
+  describe('success cases', () => {
+    it('isGenericError_ValidGenericErrorWithRequiredFields_ReturnsTrue', () => {
+      // Arrange
+      const error: GenericError = {
+        error: 'storage_error',
+        type: 'unknown_error',
+      };
+
+      // Act
+      const result = isGenericError(error);
+
+      // Assert
+      expect(result).toBe(true);
+    });
+
+    it('isGenericError_ValidGenericErrorWithAllFields_ReturnsTrue', () => {
+      // Arrange
+      const error: GenericError = {
+        code: 500,
+        error: 'storage_error',
+        message: 'Failed to store value',
+        status: 500,
+        type: 'unknown_error',
+      };
+
+      // Act
+      const result = isGenericError(error);
+
+      // Assert
+      expect(result).toBe(true);
+    });
+
+    it('isGenericError_ValidGenericErrorWithParseErrorType_ReturnsTrue', () => {
+      // Arrange
+      const error: GenericError = {
+        error: 'Parsing_error',
+        message: 'Error parsing value from session storage',
+        type: 'parse_error',
+      };
+
+      // Act
+      const result = isGenericError(error);
+
+      // Assert
+      expect(result).toBe(true);
+    });
+  });
+
+  describe('failure cases', () => {
+    it('isGenericError_NullValue_ReturnsFalse', () => {
+      // Arrange
+      const value = null;
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_UndefinedValue_ReturnsFalse', () => {
+      // Arrange
+      const value = undefined;
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_PrimitiveString_ReturnsFalse', () => {
+      // Arrange
+      const value = 'error string';
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_PrimitiveNumber_ReturnsFalse', () => {
+      // Arrange
+      const value = 500;
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_EmptyObject_ReturnsFalse', () => {
+      // Arrange
+      const value = {};
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_ObjectMissingErrorProperty_ReturnsFalse', () => {
+      // Arrange
+      const value = {
+        type: 'unknown_error',
+        message: 'Some error message',
+      };
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_ObjectMissingTypeProperty_ReturnsFalse', () => {
+      // Arrange
+      const value = {
+        error: 'storage_error',
+        message: 'Some error message',
+      };
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_ObjectWithNonStringError_ReturnsFalse', () => {
+      // Arrange
+      const value = {
+        error: 123,
+        type: 'unknown_error',
+      };
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      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'];
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+
+    it('isGenericError_ValidDataObject_ReturnsFalse', () => {
+      // Arrange
+      const value = {
+        id: '123',
+        name: 'test',
+        data: { nested: 'value' },
+      };
+
+      // Act
+      const result = isGenericError(value);
+
+      // Assert
+      expect(result).toBe(false);
+    });
+  });
+});