Handle 4xx as errors?

[robertrossmann]

I was off the grid since Friday so could not join the discussion around #293 on #304 but here are my suggestions that might improve on that. Comments are very welcome!

• Also handle client (4xx) HTTP errors (just in case, you never know...)
• Return actual instance or Error when an HTTP error occurs

PS. Not sure if we should also have a test case for a successful HTTP response...?

[ryanseys]

So discussion around why 5xx are not returned as error in request library is due to the fact that the it's not related to a socket error. 5xx are returned when the server weirds out trying to handle the request, therefore otherwise something we cannot control (I would think).

4xx are returned when there is an authorization issue, not found or otherwise the mistake is a programmer's error (our fault) so I'm not sure about returning it as an error... the server did everything it could to handle. I don't know how 401 and 403 are currently handled by people using this library... would this change affect these developers?

[robertrossmann]

Hmm, it probably would, indeed. According to OAuth 2.0 RFC's Error Response spec, a server is expected to respond with HTTP 400 response during an erroneous OAuth flow request (be it refreshing access token or exchanging authorisation code for the tokens, or something else). Since these responses were previously hidden in the response body, I would assume that error handling for situations where i.e. the user revokes access to an authorised application has been done by reading the response body and extracting the code directly from it.

So I think we can safely assume that catching 4xx responses is indeed a breaking change, even though the previous behaviour can be clearly described as broken.

What do we do? Personally, I would be probably daring enough to release this as 1.1.0 on the basis that it introduces backwards-incompatible (although quite important, now that I think of it in the broader scope) bugfix...

[ryanseys]

Hmm, I'd like to hold off on this for now. Not sure if I have authority to introduce breaking changes. Would like to summon @rakyll on this to get her thoughts. What is the general consensus for handling 4xx's in Google API client libraries? Is it an error or no?

[robertrossmann]

I just pushed an update to this PR that should be backwards-compatible. The trick is to provide Error object with proper code and message but preserve the body as is. This way existing code will continue to work and developers may start utilising err to handle 4xx errors.

PS. I also updated the original commit you submitted to return an Error object instead of a generic object, but if there was a reason for this let me know, I can revert this.

PS2. I wonder... They way we started handling 5xx errors feels like the same thing with backwards-compatibility as we are trying to do with 5xx errors - devs probably have to look at the body to get more info on the 5xx errors, but we started to use the err object and clear the body. And, surprisingly, so far I have not registered any complaint.:) Anyway, maybe we should patch it to make it backwards-compatible again (by preserving body)...?

Please review and let me know what your thoughts are on this. Thanks!

[robertrossmann]

Bump...? Any thoughts, especially on the last update...?

[ryanseys]

Sorry for the delay. Someone else will have to chime in about this PR before it gets merged. I don't know what standard protocol is for handling these errors.

[ryanseys]

I'm going to close this because I think 400's shouldn't be handled as errors and likely won't happen anyway if they are using the library as expected. To keep this library consistent with the other libraries, I'm going to close this.

[ryanseys]

Thanks for the contribution as always though @Alaneor !