Skip to content

Commit 159f743

Browse files
ryanbas21ryanbas21
authored
Merge pull request #510 from ForgeRock/SDKS-4446
fix: update-storage-client-error-handling
2 parents 9949f4b + 3c63979 commit 159f743

File tree

14 files changed

+339
-14
lines changed

14 files changed

+339
-14
lines changed

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@forgerock/davinci-client': patch
3+
'@forgerock/sdk-utilities': patch
4+
'@forgerock/storage': patch
5+
---
6+
7+
Fix error handling in storage client and davinci-client
8+
9+
- Add `isGenericError` type guard to sdk-utilities for runtime error validation
10+
- Fix storage client to properly catch errors from custom storage implementations, honoring the errors-as-values contract
11+
- Improve davinci-client error handling to use explicit error checks instead of try-catch

‎packages/davinci-client/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
"@forgerock/sdk-oidc": "workspace:*",
2828
"@forgerock/sdk-request-middleware": "workspace:*",
2929
"@forgerock/sdk-types": "workspace:*",
30+
"@forgerock/sdk-utilities": "workspace:*",
3031
"@forgerock/storage": "workspace:*",
3132
"@reduxjs/toolkit": "catalog:",
3233
"effect": "catalog:effect",

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

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010
import { CustomLogger, logger as loggerFn, LogLevel } from '@forgerock/sdk-logger';
1111
import { createStorage } from '@forgerock/storage';
12+
import { isGenericError } from '@forgerock/sdk-utilities';
1213

1314
import { createClientStore, handleUpdateValidateError, RootState } from './client.store.utils.js';
1415
import { nodeSlice } from './node.slice.js';
@@ -126,7 +127,17 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
126127

127128
if (serverSlice && serverSlice.status === 'continue') {
128129
return async () => {
129-
await serverInfo.set(serverSlice);
130+
const setResult = await serverInfo.set(serverSlice);
131+
if (isGenericError(setResult)) {
132+
log.error(setResult.message ?? setResult.error);
133+
return {
134+
error: {
135+
message: setResult.message ?? 'Failed to store server info for external IDP flow',
136+
type: 'internal_error',
137+
},
138+
} as InternalErrorResponse;
139+
}
140+
return;
130141
};
131142
}
132143
return async () => {
@@ -196,21 +207,53 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
196207
continueToken: string;
197208
}): Promise<InternalErrorResponse | NodeStates> => {
198209
try {
199-
const storedServerInfo = (await serverInfo.get()) as ContinueNode['server'];
210+
const storedServerInfo = await serverInfo.get();
211+
212+
if (storedServerInfo === null) {
213+
log.error('No server info found in storage for resume operation');
214+
return {
215+
error: {
216+
message:
217+
'No server info found in storage. Social login needs server info which is saved in local storage. You may have cleared your browser data.',
218+
type: 'state_error',
219+
},
220+
type: 'internal_error',
221+
};
222+
}
223+
224+
if (isGenericError(storedServerInfo)) {
225+
log.error(storedServerInfo.message ?? storedServerInfo.error);
226+
return {
227+
error: {
228+
message:
229+
storedServerInfo.message ??
230+
'Failed to retrieve server info from storage for resume operation',
231+
type: 'internal_error',
232+
},
233+
type: 'internal_error',
234+
};
235+
}
236+
200237
await store.dispatch(
201238
davinciApi.endpoints.resume.initiate({ continueToken, serverInfo: storedServerInfo }),
202239
);
203-
await serverInfo.remove();
240+
241+
const removeResult = await serverInfo.remove();
242+
if (isGenericError(removeResult)) {
243+
log.warn(
244+
removeResult.message ?? 'Failed to remove server info from storage after resume',
245+
);
246+
}
204247

205248
const node = nodeSlice.selectSlice(store.getState());
206249

207250
return node;
208-
} catch {
209-
// logger.error('No url found in collector, social login needs a url in the collector');
251+
} catch (err) {
252+
const error = err as Error;
253+
log.error(error.message);
210254
return {
211255
error: {
212-
message:
213-
'No url found in storage, social login needs a continue url which is saved in local storage. You may have cleared your browser data',
256+
message: error.message ?? 'An unexpected error occurred during resume operation',
214257
type: 'internal_error',
215258
},
216259
type: 'internal_error',

‎packages/davinci-client/tsconfig.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
{
1414
"path": "../sdk-effects/storage"
1515
},
16+
{
17+
"path": "../sdk-utilities"
18+
},
1619
{
1720
"path": "../sdk-types"
1821
},

‎packages/davinci-client/tsconfig.lib.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@
3434
{
3535
"path": "../sdk-effects/storage/tsconfig.lib.json"
3636
},
37+
{
38+
"path": "../sdk-utilities/tsconfig.lib.json"
39+
},
3740
{
3841
"path": "../sdk-types/tsconfig.lib.json"
3942
},

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,13 @@ export function createStorage<Value>(config: StorageConfig): StorageClient<Value
6868
return {
6969
get: async function storageGet(): Promise<Value | GenericError | null> {
7070
if (storeType === 'custom') {
71-
const value = await config.custom.get(key);
71+
let value: Awaited<ReturnType<typeof config.custom.get>>;
72+
try {
73+
value = await config.custom.get(key);
74+
} catch {
75+
return createStorageError(storeType, 'Retrieving');
76+
}
77+
7278
if (value === null || (typeof value === 'object' && 'error' in value)) {
7379
return value;
7480
}
@@ -101,8 +107,12 @@ export function createStorage<Value>(config: StorageConfig): StorageClient<Value
101107
set: async function storageSet(value: Value): Promise<GenericError | null> {
102108
const valueToStore = JSON.stringify(value);
103109
if (storeType === 'custom') {
104-
const value = await config.custom.set(key, valueToStore);
105-
return Promise.resolve(value ?? null);
110+
try {
111+
const result = await config.custom.set(key, valueToStore);
112+
return result ?? null;
113+
} catch {
114+
return createStorageError(storeType, 'Storing');
115+
}
106116
}
107117

108118
try {
@@ -114,8 +124,12 @@ export function createStorage<Value>(config: StorageConfig): StorageClient<Value
114124
},
115125
remove: async function storageRemove(): Promise<GenericError | null> {
116126
if (storeType === 'custom') {
117-
const value = await config.custom.remove(key);
118-
return Promise.resolve(value ?? null);
127+
try {
128+
const result = await config.custom.remove(key);
129+
return result ?? null;
130+
} catch {
131+
return createStorageError(storeType, 'Removing');
132+
}
119133
}
120134

121135
try {

‎packages/sdk-utilities/package.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@
3636
"test": "pnpm nx nxTest",
3737
"test:watch": "pnpm nx nxTest --watch"
3838
},
39+
"dependencies": {
40+
"@forgerock/sdk-types": "workspace:*"
41+
},
3942
"nx": {
4043
"tags": ["scope:sdk-utilities"]
4144
}

‎packages/sdk-utilities/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*
88
*/
99

10+
export * from './lib/error/index.js';
1011
export * from './lib/oidc/index.js';
1112
export * from './lib/strings/index.js';
1213
export * from './lib/url/index.js';

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

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
/*
2+
* Copyright (c) 2025 Ping Identity Corporation. All rights reserved.
3+
*
4+
* This software may be modified and distributed under the terms
5+
* of the MIT license. See the LICENSE file for details.
6+
*/
7+
8+
import { describe, it, expect } from 'vitest';
9+
import { isGenericError } from './error.utils.js';
10+
import type { GenericError } from '@forgerock/sdk-types';
11+
12+
describe('isGenericError', () => {
13+
describe('success cases', () => {
14+
it('isGenericError_ValidGenericErrorWithRequiredFields_ReturnsTrue', () => {
15+
// Arrange
16+
const error: GenericError = {
17+
error: 'storage_error',
18+
type: 'unknown_error',
19+
};
20+
21+
// Act
22+
const result = isGenericError(error);
23+
24+
// Assert
25+
expect(result).toBe(true);
26+
});
27+
28+
it('isGenericError_ValidGenericErrorWithAllFields_ReturnsTrue', () => {
29+
// Arrange
30+
const error: GenericError = {
31+
code: 500,
32+
error: 'storage_error',
33+
message: 'Failed to store value',
34+
status: 500,
35+
type: 'unknown_error',
36+
};
37+
38+
// Act
39+
const result = isGenericError(error);
40+
41+
// Assert
42+
expect(result).toBe(true);
43+
});
44+
45+
it('isGenericError_ValidGenericErrorWithParseErrorType_ReturnsTrue', () => {
46+
// Arrange
47+
const error: GenericError = {
48+
error: 'Parsing_error',
49+
message: 'Error parsing value from session storage',
50+
type: 'parse_error',
51+
};
52+
53+
// Act
54+
const result = isGenericError(error);
55+
56+
// Assert
57+
expect(result).toBe(true);
58+
});
59+
});
60+
61+
describe('failure cases', () => {
62+
it('isGenericError_NullValue_ReturnsFalse', () => {
63+
// Arrange
64+
const value = null;
65+
66+
// Act
67+
const result = isGenericError(value);
68+
69+
// Assert
70+
expect(result).toBe(false);
71+
});
72+
73+
it('isGenericError_UndefinedValue_ReturnsFalse', () => {
74+
// Arrange
75+
const value = undefined;
76+
77+
// Act
78+
const result = isGenericError(value);
79+
80+
// Assert
81+
expect(result).toBe(false);
82+
});
83+
84+
it('isGenericError_PrimitiveString_ReturnsFalse', () => {
85+
// Arrange
86+
const value = 'error string';
87+
88+
// Act
89+
const result = isGenericError(value);
90+
91+
// Assert
92+
expect(result).toBe(false);
93+
});
94+
95+
it('isGenericError_PrimitiveNumber_ReturnsFalse', () => {
96+
// Arrange
97+
const value = 500;
98+
99+
// Act
100+
const result = isGenericError(value);
101+
102+
// Assert
103+
expect(result).toBe(false);
104+
});
105+
106+
it('isGenericError_EmptyObject_ReturnsFalse', () => {
107+
// Arrange
108+
const value = {};
109+
110+
// Act
111+
const result = isGenericError(value);
112+
113+
// Assert
114+
expect(result).toBe(false);
115+
});
116+
117+
it('isGenericError_ObjectMissingErrorProperty_ReturnsFalse', () => {
118+
// Arrange
119+
const value = {
120+
type: 'unknown_error',
121+
message: 'Some error message',
122+
};
123+
124+
// Act
125+
const result = isGenericError(value);
126+
127+
// Assert
128+
expect(result).toBe(false);
129+
});
130+
131+
it('isGenericError_ObjectMissingTypeProperty_ReturnsFalse', () => {
132+
// Arrange
133+
const value = {
134+
error: 'storage_error',
135+
message: 'Some error message',
136+
};
137+
138+
// Act
139+
const result = isGenericError(value);
140+
141+
// Assert
142+
expect(result).toBe(false);
143+
});
144+
145+
it('isGenericError_ObjectWithNonStringError_ReturnsFalse', () => {
146+
// Arrange
147+
const value = {
148+
error: 123,
149+
type: 'unknown_error',
150+
};
151+
152+
// Act
153+
const result = isGenericError(value);
154+
155+
// Assert
156+
expect(result).toBe(false);
157+
});
158+
159+
it('isGenericError_ObjectWithNonStringType_ReturnsFalse', () => {
160+
// Arrange
161+
const value = {
162+
error: 'storage_error',
163+
type: 123,
164+
};
165+
166+
// Act
167+
const result = isGenericError(value);
168+
169+
// Assert
170+
expect(result).toBe(false);
171+
});
172+
173+
it('isGenericError_ArrayValue_ReturnsFalse', () => {
174+
// Arrange
175+
const value = ['error', 'type'];
176+
177+
// Act
178+
const result = isGenericError(value);
179+
180+
// Assert
181+
expect(result).toBe(false);
182+
});
183+
184+
it('isGenericError_ValidDataObject_ReturnsFalse', () => {
185+
// Arrange
186+
const value = {
187+
id: '123',
188+
name: 'test',
189+
data: { nested: 'value' },
190+
};
191+
192+
// Act
193+
const result = isGenericError(value);
194+
195+
// Assert
196+
expect(result).toBe(false);
197+
});
198+
});
199+
});

0 commit comments

Comments
 (0)