Skip to content
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
merged 5 commits into from
Apr 23, 2024
Merged

Conversation

ctran88
Copy link
Contributor

@ctran88 ctran88 commented Apr 16, 2024

Fixes:

  • PassageError now passes through the specific status code and error information from the API responses
    • Error messages will no longer be prefixed by "Response returned an error code: "
    • Error status will now reflect the actual response code instead of always being "500"
    • Rethrown errors are now checked if they are the right type before wrapping it in PassageError

Misc:

  • PassageError.error has been renamed to code to better reflect its value as an error code statusText to be more aligned with client SDKs (i.e. invalid_nonce)
  • Creating a PassageError object can now only be done with the static builder methods
    • Since the error information required is behind an async call, it cannot be done in the constructor
    • Even though throwing a PassageError without a response error is not async and can therefore be created via the constructor, putting it behind the same static builder method pattern for consistency
  • Exports the error code enums so they can be used to handle specific error types

@ctran88 ctran88 requested a review from flanagankp April 16, 2024 00:45
@ctran88 ctran88 marked this pull request as ready for review April 16, 2024 00:45
@ctran88 ctran88 requested a review from a team as a code owner April 16, 2024 00:45
readonly statusCode: number | undefined;
readonly error: string | undefined;
readonly message: string;
public readonly status: number | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me these feel backwards - I'd expect a code to be a number and the status to be a string. This is similar to how we define PassageError in our JS. Does it matter that we diverge on this between Node + client JS, and is this more standard for node error handling?

    public statusCode: number;
    public statusText: string;
    public message: string;
    public name: string;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did read the PR description and disambiguating statusCode from HTTP status codes does make sense, just to me code reads as a numeric error code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to conform to the client packages and swap it so that it'd be something like this

PassageError {
  statusCode: 409,
  statusText: 'user_has_no_passkeys',
  message: 'Could not create authenticate transaction: User has no passkeys',
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I like keeping things uniform where we can, but if you or Kevin have strong feelings this format no longer makes sense (PassageError and those fields were created over 2 years ago) we could change it. Downside of changing the JS SDK is it'll introduce a potentially breaking change to passageJS consumers so we'd need to handle it accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in f88ec0f

src/index.ts Outdated
export {
CreateTransactionAuthenticateRequest,
CreateTransactionRegisterRequest,
Model400ErrorCodeEnum,
Copy link
Contributor

@flanagankp flanagankp Apr 16, 2024

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

Copy link
Contributor Author

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 codes

public readonly statusText:
        | Model400ErrorCodeEnum
        | Model401ErrorCodeEnum
        | Model403ErrorCodeEnum
        | Model404ErrorCodeEnum
        | Model409ErrorCodeEnum
        | Model500ErrorCodeEnum
        | undefined;

Which would alleviate the need to export them since it would be a string literal union, right?
image

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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
image

Copy link
Contributor Author

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

Ah I see the mixup with how the client code defines "ErrorCode". I can rename that to avoid the overloaded term between the sdks

Copy link
Contributor

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:

export const ErrorStatusText = { ...Model400ErrorCodeEnum, ...Model401ErrorCodeEnum, //and all the rest};

Then set the statusText property to be this enum union:

public readonly statusText: ErrorStatusText | undefined;

This will allow customers to do this kind of error handling:

import {ErrorStatusText} from '@passageidentity/passage-flex-node'
try {
// function
} catch(error as PassageError) {
 switch(error.statusText){
    case ErrorStatusText.UserNotFound:
     // do whatever
    break;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in aceb8fe

@ctran88 ctran88 force-pushed the PSG-3862-improve-error-handling branch from b4d5abb to aceb8fe Compare April 16, 2024 17:37
@ctran88 ctran88 merged commit 9854556 into main Apr 23, 2024
1 check passed
@ctran88 ctran88 deleted the PSG-3862-improve-error-handling branch April 23, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants