-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PSG-3862: improve error handling #15
Merged
+164
−82
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
fef8ad9
refactor: removes transaction method parameter wrapper types for the …
ctran88 e856528
fix: now passes the error info from the api response through to the P…
ctran88 ee103c7
Change files
ctran88 f88ec0f
refactor: renames PassageError fields to be more aligned with the cli…
ctran88 aceb8fe
chore: wraps generated error code exports and sets them as a string l…
ctran88 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
change/@passageidentity-passage-flex-node-5cfb61ba-2e79-4e6c-a10a-9cac13fe9ab4.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "patch", | ||
"comment": "fix: now passes the error info from the api response through to the PassageError object", | ||
"packageName": "@passageidentity/passage-flex-node", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,52 @@ | ||
import { ResponseError } from '../generated'; | ||
|
||
type APIResponseError = { statusCode: number; errorCode: string; message: string }; | ||
|
||
/** | ||
* PassageError Class used to handle errors from PassageFlex | ||
*/ | ||
export class PassageError extends Error { | ||
readonly statusCode: number | undefined; | ||
readonly error: string | undefined; | ||
readonly message: string; | ||
public readonly statusCode: number | undefined; | ||
public readonly statusText: string | undefined; | ||
|
||
/** | ||
* Initialize a new PassageError instance. | ||
* @param {string} message friendly message, | ||
* @param {Error} err error from node-fetch request | ||
* @param {string} message friendly message | ||
* @param {APIResponseError} response error information from PassageFlex API | ||
*/ | ||
constructor(message: string, err?: ResponseError) { | ||
super(); | ||
private constructor(message: string, response?: APIResponseError) { | ||
super(message); | ||
|
||
if (err) { | ||
this.message = `${message}: ${err.message}`; | ||
this.statusCode = 500; | ||
this.error = err.message; | ||
} else { | ||
this.message = message; | ||
if (!response) { | ||
return; | ||
} | ||
|
||
this.message = `${message}: ${response.message}`; | ||
this.statusCode = response.statusCode; | ||
this.statusText = response.errorCode; | ||
} | ||
|
||
/** | ||
* Initialize a new PassageError instance. | ||
* @param {string} message friendly message | ||
* @return {PassageError} | ||
*/ | ||
public static fromMessage(message: string): PassageError { | ||
return new PassageError(message); | ||
} | ||
|
||
/** | ||
* Initialize a new PassageError instance. | ||
* @param {string} message friendly message | ||
* @param {ResponseError} err error from node-fetch request | ||
* @return {Promise<PassageError>} | ||
*/ | ||
public static async fromResponseError(message: string, err: ResponseError): Promise<PassageError> { | ||
const body: { code: string; error: string } = await err.response.json(); | ||
return new PassageError(message, { | ||
statusCode: err.response.status, | ||
errorCode: body.code, | ||
message: body.error, | ||
}); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,45 @@ | ||
import { ResponseError } from '../src/generated'; | ||
import { PassageError } from '../src/classes/PassageError'; | ||
import { faker } from '@faker-js/faker'; | ||
|
||
describe('PassageError', () => { | ||
test('message only', async () => { | ||
const msg = 'Could not find valid cookie for authentication. You must catch this error.'; | ||
const err = new PassageError(msg); | ||
describe('fromMessage', () => { | ||
it('should set PassageError.message to message', async () => { | ||
const expected = faker.string.sample(); | ||
const actual = PassageError.fromMessage(expected); | ||
|
||
expect(err.message).toEqual(msg); | ||
expect(err.error).toBeUndefined; | ||
expect(actual.message).toEqual(expected); | ||
expect(actual.stack).toBeDefined(); | ||
expect(actual.statusText).toBeUndefined(); | ||
expect(actual.statusCode).toBeUndefined(); | ||
}); | ||
}); | ||
|
||
test('with Response Error', async () => { | ||
const responseError = { | ||
message: 'some error message', | ||
name: 'ResponseError', | ||
} as ResponseError; | ||
describe('fromResponseError', () => { | ||
it('should set PassageError.code and PassageError.status from ResponseError', async () => { | ||
const expectedMessage = 'friendly message'; | ||
const expectedResponseCode = faker.string.sample(); | ||
const expectedResponseError = 'error body message'; | ||
|
||
const msg = 'Could not find valid cookie for authentication. You must catch this error'; | ||
const err = new PassageError(msg, responseError); | ||
const responseError = { | ||
message: faker.string.sample(), | ||
response: { | ||
status: faker.internet.httpStatusCode(), | ||
json: async () => { | ||
return { | ||
code: expectedResponseCode, | ||
error: expectedResponseError, | ||
}; | ||
}, | ||
} as Response, | ||
} as ResponseError; | ||
|
||
expect(err.message).toEqual(`${msg}: ${responseError.message}`); | ||
expect(err.statusCode).toBe(500); | ||
const actual = await PassageError.fromResponseError(expectedMessage, responseError); | ||
|
||
expect(err.error).toBe('some error message'); | ||
expect(actual.message).toEqual(`${expectedMessage}: ${expectedResponseError}`); | ||
expect(actual.statusText).toEqual(expectedResponseCode); | ||
expect(actual.statusCode).toEqual(responseError.response.status); | ||
expect(actual.stack).toBeDefined(); | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware that these enums are coming from generated code but it would be ideal if they had more user-friendly error enum names that map to the type of error. Might not be particularly easy to do with the codegen core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could alias them and export those. I was also going to set the
statusText
to a union of those codesWhich would alleviate the need to export them since it would be a string literal union, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting I expected those were an enum of the numbers for the error code not the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm I see we're mapping response.errorCode to
this.statusText
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah i'm assuming it's because this is how it's laid out in the openapi spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see the mixup with how the client code defines "ErrorCode". I can rename that to avoid the overloaded term between the sdks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so I pulled up these types and played around with some typescript. I think what you want to do is this - create a merged enum of all these ModelXXXErrorCodeEnum types like so:
Then set the statusText property to be this enum union:
This will allow customers to do this kind of error handling:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in aceb8fe