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

OAuth2: don't parse error as JSON #7467

Merged
merged 1 commit into from Feb 26, 2019

Conversation

Projects
None yet
3 participants
@asterite
Copy link
Member

commented Feb 21, 2019

Fixes #7464

Let's not parse the error as the JSON documented in the RFC. Instead, just treat the whole response body as the exception message. One can parse it from there if needed, but I think it's usually not needed.

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

I'm not sure it's need to go that far, why not cover the use-case from the linked issue and throw the catcheable error containing the response body the user can parse as a fallback solution?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

If there's an OAuth error there's not much you can programmatically do: log the error, check it later, fix the bug, if any, or wait until the server fixes the problem, etc. You won't programmatically fix the error by checking the response body. That's why I think that it's better we don't do anything with the body. Just include it as an exception message.

If you really want to parse it, just do JSON.parse(...) or map it to a model of your own.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

We could consider keeping the #error and #error_description methods but let them lazily parse the JSON on demand.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

@straight-shoota We could, but this way users can parse it if they want it so it's a bit faster too (no need to parse it if you don't want to use it)

@straight-shoota
Copy link
Member

left a comment

I'm not sure if removing this entirely is the best solution. It would be nice to still have a means to optionally retrieve error and error_description properties from a standards-compliant response.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

I'm open to suggestions. Let's think about the API and implementation for this (though I really think that parsing this is unnecessary in most of the cases and if you really want to parse it you can use JSON.parse(ex.message).

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I'm not very familiar with the OAuth2 API.

Actually, it's probably okay to merge this and think about alternatives when such a feature is requested (if ever).

@asterite asterite added this to the 0.28.0 milestone Feb 26, 2019

@asterite asterite merged commit 9ef9d8d into crystal-lang:master Feb 26, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asterite asterite deleted the asterite:bug/oauth2-error branch Mar 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.