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

Ensure all Json.parseToJsonElement calls are wrapped in try-catch wit… #399

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

prashanDYDX
Copy link
Contributor

…h appropriately scoped exceptions.

@prashanDYDX prashanDYDX force-pushed the prashan/json-decoding-crashes branch 2 times, most recently from cb7ee80 to ea15eae Compare May 30, 2024 14:31
Copy link
Contributor

@yogurtandjam yogurtandjam left a comment

Choose a reason for hiding this comment

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

feels like we may want to make a jsonSafe helper method and (if possible?) make a linting rule to not allow usage/import of the json.Json library if we want json to always be a safe operation.

that being said - are there times when the app should fail and throw if we can't jsonify a payload?

@prashanDYDX
Copy link
Contributor Author

@yogurtandjam yes good callouts, i shall tackle in a follow up

@prashanDYDX
Copy link
Contributor Author

@yogurtandjam re failing on payloads, likely yes - but i don't have enough context on these existing usages to say which ones should throw, so just made them safe for now.

@prashanDYDX prashanDYDX force-pushed the prashan/json-decoding-crashes branch 2 times, most recently from 85109d2 to 6b9026a Compare June 7, 2024 18:53
@prashanDYDX prashanDYDX force-pushed the prashan/json-decoding-crashes branch from fdd8a3c to cef9816 Compare June 7, 2024 19:09
@prashanDYDX prashanDYDX merged commit bcc50cf into main Jun 7, 2024
4 checks passed
@prashanDYDX prashanDYDX deleted the prashan/json-decoding-crashes branch June 7, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants