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

Better log the error causing the JSON parsing to fail on deserialization #1158

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

remi-stripe
Copy link
Contributor

We're now logging the error message associated with the JsonSyntaxException exception raised on parsing, if any. We also record that exception as the cause of the ApiException we throw.

r? @richardm-stripe
cc @stripe/api-libraries

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

LGTM

@remi-stripe
Copy link
Contributor Author

Pushed a brand new version based on @ob-stripe's work just adding an extra details in the default log line so that we can debug this more easily. He also helped write an explicit test for this

@ob-stripe
Copy link
Contributor

Note that StripeObjectTest is a terrible place for this test as it has nothing to do with StripeObject -- I just used it as a quick and dirty launch ramp. This test should be in a new LiveStripeResponseGetterTest class.

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I took the liberty of moving the test per @ob's comment in the latest commit.

@remi-stripe remi-stripe merged commit 9af3d4f into master Nov 24, 2020
@remi-stripe remi-stripe deleted the remi-improve-error-logging branch November 24, 2020 16:15
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.

None yet

3 participants