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

fix: update SMS messaging parse error #855

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

mariomui
Copy link

@mariomui mariomui commented Dec 29, 2022

Fixes #715

if the messageBody is malformed then there will be a json parse error since the only check to allow parse is whether the body be a string or not. If we wrap the parse in a try catch, we can code a gentler response.

  • Assumptions
    • 1: Assuming that details is where the error details are located
      • I have populated the details property inside the exception object with the parse error {error: bodyParseError}. For more information, peruse the new unit test described with "should test serialize from improper json string".
      • on malformed json response_body, the exception should contain undefined properties except for message which is overwritten by the call to super.
    • 2: Assuming that the logger is catching the RestException and not another area of code:

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@mariomui
Copy link
Author

@childish-sambino as per request within closed PR #852 , i've submitted DI-2442 (with supporting unit test) to target the 4.0.0rc branch instead.

Copy link

@beebzz beebzz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for submitting this change!

lib/base/RestException.ts Outdated Show resolved Hide resolved
lib/base/RestException.ts Outdated Show resolved Hide resolved
@mariomui mariomui force-pushed the DI-2442 branch 2 times, most recently from 51d1844 to eadbcbd Compare December 31, 2022 00:33
@childish-sambino childish-sambino changed the title chore: fix SMS messaging parse error fix: update SMS messaging parse error Jan 3, 2023
@childish-sambino childish-sambino merged commit 1508e52 into twilio:4.0.0-rc Jan 3, 2023
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.

4 participants