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

refactor: Update Error handling for Identity API Client #959

Open
wants to merge 3 commits into
base: refactor/ts-migration-blackout-2024
Choose a base branch
from

Conversation

alexs-mparticle
Copy link
Collaborator

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

  • Updates error handling and reporting for Identity API Client
  • Adds more detailed messaging and better reflects actual Identity API Error Responses.

Testing Plan

  • Was this tested locally? If not, explain why.
  • Run SDK in sample app and disable networking. Manually modify identity requests with bad data and verify that errors are properly reported in developer console.

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@alexs-mparticle alexs-mparticle added wait to merge refactor Changes to the structure of the code labels Dec 9, 2024
Comment on lines 251 to 269
case HTTP_OK:
case HTTP_ACCEPTED:
message = 'Received Identity Response from server: ';
message += JSON.stringify(identityResponse.responseText);
break;

default: {
// 400 has an error message, but 403 doesn't
const errorResponse: IdentityApiErrorResponse = identityResponse.responseText as unknown as IdentityApiErrorResponse;
if (errorResponse?.Errors) {
message = 'Issue with sending Identity Request to mParticle Servers, received HTTP Code of ' + identityResponse.status;

const errorMessage = errorResponse.Errors.map((error) => error.message).join(', ');
message += ' - ' + errorMessage;
}
}
}

verbose(message);
Copy link
Member

Choose a reason for hiding this comment

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

Can you write a test that accounts for the message being passed through, since this is new behavior?

Copy link

sonarcloud bot commented Dec 10, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
46.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud


break;

// 401 and 403 have no bodies and should be rejected outright
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: Add specific comments as to what will cause a 401 or 403

case HTTP_ACCEPTED:
case HTTP_OK:

// 400 error will has a body and will go through the happy path to report the error
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: Add context to what would cause a 400


break;

// 401 and 403 have no bodies and should be rejected outright
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: Rephrase to say 'All other error codes will hit this path'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Changes to the structure of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants