A change to throw actual exceptions #157

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@alertedsnake

When we're doing OAuth and the response code isn't 200OK, why shouldn't we return a proper exception to the client? Without doing this, we have to parse client response, and it may not be something reasonable.

I'd make a test case, but that would require a working web server which returns bad responses.

alertedsnake and others added some commits Apr 25, 2014

Reworked how the decoder argument works - it's actually required on a…
… few other

methods but wasn't there, specifically what happens when you need a decoder but
you call service.get_auth_session() and it defaults to not passing a decoder to
session.get_access_token()?  It breaks.

Yes, you can replace the call to get_auth_session() with get_access_token() and
get_session(), but why have the shortcut method if it doesn't work in all
cases?

So what I've done here is added a parameter to the Session object itself, so
you can set it once and forget it for good.
Revert "Reworked how the decoder argument works - it's actually requi…
…red on a few other"

This reverts commit 1ce3b3f.
Moved it to a branch.
@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 3, 2014

Hmm sorry about the commits listing too many things, the actual change is minimal.

Hmm sorry about the commits listing too many things, the actual change is minimal.

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman May 3, 2014

Contributor

Couldn't the caller raise these manually themselves?

Contributor

maxcountryman commented May 3, 2014

Couldn't the caller raise these manually themselves?

@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 3, 2014

Where? There's nothing that tells the caller what the response is - get_request_token() and get_access_token() never check for an OK response, they just assume it's OK and start trying to parse the response, which often isn't formatted right, and you get KeyError raised from process_token_request.

Where? There's nothing that tells the caller what the response is - get_request_token() and get_access_token() never check for an OK response, they just assume it's OK and start trying to parse the response, which often isn't formatted right, and you get KeyError raised from process_token_request.

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman May 3, 2014

Contributor

KeyError is generally not an invalid response, that's related to an improper decoder. (The error message should make this pretty clear.)

The caller has direct access to the responses, however; e.g. request_token_response.

Contributor

maxcountryman commented May 3, 2014

KeyError is generally not an invalid response, that's related to an improper decoder. (The error message should make this pretty clear.)

The caller has direct access to the responses, however; e.g. request_token_response.

@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 3, 2014

When you call get_request_token() you don't get the response returned, you get the token and secret. What you're suggesting is that the caller should not use this but use the get_raw_* methods and handle the response manually?

As for decoding, it would be nice if all services would return error responses (like a 500) that are parseable with the default decoder, but that doesn't seem to be the case. The error message is quite clear, but it says there was an error decoding the response, not that the authentication failed for whatever reason (e.g. HTTP 400 or 401)

I'm looking at the OAuth1 docs here and they do say the service provider should return 400 and 401 errors, but there's nothing about a required format, so it seems silly to assume the decoder can handle that when you can easily check if it's an error response and handle that rather than attempt to decode a message in an unknown format.

When you call get_request_token() you don't get the response returned, you get the token and secret. What you're suggesting is that the caller should not use this but use the get_raw_* methods and handle the response manually?

As for decoding, it would be nice if all services would return error responses (like a 500) that are parseable with the default decoder, but that doesn't seem to be the case. The error message is quite clear, but it says there was an error decoding the response, not that the authentication failed for whatever reason (e.g. HTTP 400 or 401)

I'm looking at the OAuth1 docs here and they do say the service provider should return 400 and 401 errors, but there's nothing about a required format, so it seems silly to assume the decoder can handle that when you can easily check if it's an error response and handle that rather than attempt to decode a message in an unknown format.

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman May 3, 2014

Contributor

The wrappers assume a common encoding format, for example JSON, and then give you, the programmer, the option to set that manually. Failing to decode a response does not mean the service returned a non-2xx response.

The raw methods exist so that in non-standard cases, you can do whatever you need to do. These wrappers are for convenience first, with escape hatches for when our assumptions inevitably breakdown.

Contributor

maxcountryman commented May 3, 2014

The wrappers assume a common encoding format, for example JSON, and then give you, the programmer, the option to set that manually. Failing to decode a response does not mean the service returned a non-2xx response.

The raw methods exist so that in non-standard cases, you can do whatever you need to do. These wrappers are for convenience first, with escape hatches for when our assumptions inevitably breakdown.

@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 4, 2014

Yeah, and in the real world, you can't assume any sort of common format in your responses, but you can guarantee is the HTTP response code. In fact, now that I'm looking, I don't see where these errors are handled at all, parseable or not. Can you point out where this occurs?

Yeah, and in the real world, you can't assume any sort of common format in your responses, but you can guarantee is the HTTP response code. In fact, now that I'm looking, I don't see where these errors are handled at all, parseable or not. Can you point out where this occurs?

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman May 4, 2014

Contributor

If you don't know your format ahead of time or can't pass in a custom decoder or want to do special handling of the response, then you should use the raw methods. That's exactly why they exist.

Contributor

maxcountryman commented May 4, 2014

If you don't know your format ahead of time or can't pass in a custom decoder or want to do special handling of the response, then you should use the raw methods. That's exactly why they exist.

@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 4, 2014

It must be nice to have services that never, ever, throw HTTP error codes.

I guess I'm off to fork myself a library.

It must be nice to have services that never, ever, throw HTTP error codes.

I guess I'm off to fork myself a library.

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman May 4, 2014

Contributor

Why are you unable to use the raw methods? Don't they do exactly what you want? (You can do whatever response handling you want with them.)

I strongly recommend against doing this, but you could also monkey patch the token parser.

Contributor

maxcountryman commented May 4, 2014

Why are you unable to use the raw methods? Don't they do exactly what you want? (You can do whatever response handling you want with them.)

I strongly recommend against doing this, but you could also monkey patch the token parser.

@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 4, 2014

I'm not unable to use the raw methods. I just thought you might want to improve your library. You might update the documentation to say something like "this doesn't handle HTTP errors!" for get_access_token() and get_request_token() so the next guy doesn't have to spend a few hours trying to figure out what I did here.

I'll close the pull request.

I'm not unable to use the raw methods. I just thought you might want to improve your library. You might update the documentation to say something like "this doesn't handle HTTP errors!" for get_access_token() and get_request_token() so the next guy doesn't have to spend a few hours trying to figure out what I did here.

I'll close the pull request.

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman May 4, 2014

Contributor

Would something along these lines help you?

diff --git a/rauth/service.py b/rauth/service.py
index 14f7b74..d846489 100644
--- a/rauth/service.py
+++ b/rauth/service.py
@@ -15,13 +15,26 @@ PROCESS_TOKEN_ERROR = ('Decoder failed to handle {key} with data as returned '
                        'Provider returned: {raw}')


+class TokenParseError(KeyError):
+    '''
+    Raised when a response cannot be automatically handled. This may be because
+    the decoder could not handle the format of the response or a non-2xx
+    response was received. The full response object is available as a property,
+    `response`.
+    '''
+    pass
+
+
 def process_token_request(r, decoder, *args):
     try:
         data = decoder(r.content)
         return tuple(data[key] for key in args)
     except KeyError as e:  # pragma: no cover
         bad_key = e.args[0]
-        raise KeyError(PROCESS_TOKEN_ERROR.format(key=bad_key, raw=r.content))
+        err_msg = PROCESS_TOKEN_ERROR.format(key=bad_key, raw=r.content)
+        err = TokenParseError(err_msg)
+        err.response = r
+        raise err


 class Service(object):
@@ -224,6 +237,8 @@ class OAuth1Service(Service):
         '''
         Return a request token pair.

+        Potentially raises `TokenParseError`.
+
         :param method: A string representation of the HTTP method to be used,
             defaults to `GET`.
         :type method: str
@@ -302,6 +317,8 @@ class OAuth1Service(Service):
         '''
         Returns an access token pair.

+        Potentially raises `TokenParseError`.
+
         :param request_token: The request token as returned by
             :meth:`get_request_token`.
         :type request_token: str
Contributor

maxcountryman commented May 4, 2014

Would something along these lines help you?

diff --git a/rauth/service.py b/rauth/service.py
index 14f7b74..d846489 100644
--- a/rauth/service.py
+++ b/rauth/service.py
@@ -15,13 +15,26 @@ PROCESS_TOKEN_ERROR = ('Decoder failed to handle {key} with data as returned '
                        'Provider returned: {raw}')


+class TokenParseError(KeyError):
+    '''
+    Raised when a response cannot be automatically handled. This may be because
+    the decoder could not handle the format of the response or a non-2xx
+    response was received. The full response object is available as a property,
+    `response`.
+    '''
+    pass
+
+
 def process_token_request(r, decoder, *args):
     try:
         data = decoder(r.content)
         return tuple(data[key] for key in args)
     except KeyError as e:  # pragma: no cover
         bad_key = e.args[0]
-        raise KeyError(PROCESS_TOKEN_ERROR.format(key=bad_key, raw=r.content))
+        err_msg = PROCESS_TOKEN_ERROR.format(key=bad_key, raw=r.content)
+        err = TokenParseError(err_msg)
+        err.response = r
+        raise err


 class Service(object):
@@ -224,6 +237,8 @@ class OAuth1Service(Service):
         '''
         Return a request token pair.

+        Potentially raises `TokenParseError`.
+
         :param method: A string representation of the HTTP method to be used,
             defaults to `GET`.
         :type method: str
@@ -302,6 +317,8 @@ class OAuth1Service(Service):
         '''
         Returns an access token pair.

+        Potentially raises `TokenParseError`.
+
         :param request_token: The request token as returned by
             :meth:`get_request_token`.
         :type request_token: str
@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake May 4, 2014

You're still not actually looking at the HTTP response, so returning a "parsing error" is misleading - it may be a parsing error, but the real HTTP error is now being masked so the client doesn't know what actually happened. Not the way I'd handle it.

Don't worry about it. I just converted 3 lines of clean code in my client into 12 slightly messier lines (two of them imports) and wrapped a function around it, by using the raw request methods.

You're still not actually looking at the HTTP response, so returning a "parsing error" is misleading - it may be a parsing error, but the real HTTP error is now being masked so the client doesn't know what actually happened. Not the way I'd handle it.

Don't worry about it. I just converted 3 lines of clean code in my client into 12 slightly messier lines (two of them imports) and wrapped a function around it, by using the raw request methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment