Return JSON from facebook error if body has a json. #480

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

wsantos commented Mar 19, 2012

Return JSON Error message from facebook, this is useful to some functional logics, ex,: in some cases like missing some keys in dict like actions, actions need ("name", "link") keys, if you miss facebook return the error message.

Return JSON Error message from facebook, this is useful to some funct…
…ional logics, ex,: in some cases like missing some keys in dict like actions, actions need ("name", "link") keys, if you miss facebook return the error message.
Owner

bdarnell commented Mar 24, 2012

With this change the application will receive a json object with no way of knowing if it was the result of a success or failure, which seems like a problem. (I think it's also best to check e.g. the Content-Type header before trying to parse the json, rather than having the behavior change based on whether the response body happens to be parseable json).

There's a larger issue that when there is an error, we don't communicate anything about the failure back to the application. We should figure out a way to pass the error back to the application in any case rather than passing a json object back to the application only for the subset of errors that return parseable json without an error_code attribute.

Contributor

wsantos commented Apr 2, 2012

I agree with Content-type check, it's okay to thorw a HttpClient exception ? or better approach is to return a list (json, status) ?

Contributor

wsantos commented Apr 7, 2013

@bdarnell do you thin we need a Exception per server ? and a execption with a attibute message and another to contains the json ?

Owner

bdarnell commented Apr 7, 2013

By "per server" do you mean e.g. FacebookException, GoogleException, TwitterException? I don't think it makes sense to split the errors up that way. It might make sense to create special exceptions for some classes of errors that could be handled differently, though (e.g. AuthTokenExpiredError). And of course, now that we have the ability to pass an AuthError back through the returned Future we can add attributes to it to allow for better logging or recovery at the application level.

Contributor

wsantos commented Apr 7, 2013

Yeap, service no server sorry. But for now im thinking about services request errors, i don't wanna inspect the json response and figure out the erro, so you think we must use a single RequestException ? i wan't to work in auth module to "fix" the exceptions things becouse you can now, to pass the exception, i just do "raise Exception" ?

Owner

bdarnell commented Apr 7, 2013

The auth module works differently than most; instead of raising the exception you need to use future.set_exception().

Contributor

wsantos commented Apr 8, 2013

Why FacebookGraphMixin don't have _auth_return_future decorator ?

Owner

bdarnell commented Apr 8, 2013

Hmm, that's a good question. Looks like an oversight. I know I left the old FacebookMixin alone, but it looks like I forgot FacebookGraphMixin too.

Contributor

wsantos commented Apr 8, 2013

Ok @bdarnell i will work on it, we still need FacebookMixin ? it still working ?

Owner

bdarnell commented Apr 9, 2013

I think FacebookMixin is still working, although I haven't tested it myself in a long time. The interface it uses is deprecated but not gone yet.

Owner

bdarnell commented Apr 14, 2013

Closing this pull request now that we have Futures and AuthError in Tornado 3.0.

@bdarnell bdarnell closed this Apr 14, 2013

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