Conversation
_should_retry_response read data[0]["error"]["errors"]["reason"] for list-wrapped error bodies, but "errors" is a list, so any such 403 raised TypeError out of execute() instead of retrying on rate limits or raising HttpError. Index the first entry, as the dict branch does. Also make LIST_NOT_CONFIGURED_RESPONSE valid JSON so that test_no_retry_403_list_fails actually reaches the list branch.
There was a problem hiding this comment.
Code Review
This pull request resolves a bug where the error reason was incorrectly accessed from a dictionary instead of a list within the JSON response. It also updates the test suite with a valid mock response and adds a new test case for retrying on 403 rate limit errors. The reviewer recommends implementing defensive type and boundary checks when parsing the nested JSON response to prevent potential unhandled exceptions like TypeError or IndexError.
| reason = reason["reason"] | ||
| else: | ||
| reason = data[0]["error"]["errors"]["reason"] | ||
| reason = data[0]["error"]["errors"][0]["reason"] |
There was a problem hiding this comment.
While indexing data[0]["error"]["errors"][0]["reason"] fixes the immediate TypeError when errors is a list, this line is still highly vulnerable to other unhandled exceptions. Since the except block only catches (UnicodeDecodeError, ValueError, KeyError), any of the following scenarios will raise an unhandled TypeError, IndexError, or AttributeError and crash the request execution:
datais an empty list[](raisesIndexError).datais not a list (e.g.,None,int,bool, or a string) (raisesTypeError).data[0]is not a dictionary (raisesTypeError).data[0]["error"]is not a dictionary (raisesTypeErrororAttributeErrorif using.get()).data[0]["error"]["errors"]is an empty list or not a list (raisesIndexErrororTypeError).
To make this completely robust and prevent any unhandled exceptions from escaping _should_retry_response, we should perform defensive type and boundary checks before accessing these nested keys.
| reason = data[0]["error"]["errors"][0]["reason"] | |
| reason = None | |
| if isinstance(data, list) and data and isinstance(data[0], dict): | |
| error = data[0].get("error") | |
| if isinstance(error, dict): | |
| errors = error.get("errors") | |
| if isinstance(errors, list) and errors and isinstance(errors[0], dict): | |
| reason = errors[0].get("reason") |
There was a problem hiding this comment.
Thanks, good catch. These shapes already raised on main (for example, an empty errors list raised TypeError there and IndexError after my first commit), but they still escape as something other than HttpError, which is what this PR is meant to fix. Rather than nesting type checks, I added IndexError and TypeError to the existing except tuple. Those shapes now go down the same path as other invalid content: log a warning, no retry, and the caller gets an HttpError. I also added test_no_retry_403_malformed_body, which covers [], an empty errors list, ["x"] and null.
An empty list, an empty errors list or a non-dict entry in a 403 body still raised IndexError or TypeError out of _should_retry_response. Treat them like other invalid content: log a warning and do not retry, so the caller gets an HttpError.
|
Pushed a follow up commit that makes _should_retry_response treat malformed 403 bodies (empty lists, non-dict entries, null) as invalid content instead of raising, with a test for those cases. |
Root cause
_should_retry_responsehandles two shapes of 403 error bodies. The dict branch reads the reason fromdata["error"]["errors"][0]["reason"], but the list branch (bodies like[{"error": {...}}], the format reported in #225) readsdata[0]["error"]["errors"]["reason"].errorsis a list, so indexing it with"reason"raisesTypeError. That exception is not in the caught tuple(UnicodeDecodeError, ValueError, KeyError), so it escapesHttpRequest.execute(): a list-wrappedrateLimitExceeded403 is never retried, and any other list-wrapped 403 surfaces asTypeError: list indices must be integers or slices, not strinstead ofHttpError.The existing
test_no_retry_403_list_failsdid not catch this because itsLIST_NOT_CONFIGURED_RESPONSEfixture is not valid JSON (the object is missing its braces), sojson.loadsfails before the list branch runs.Fix
Index the first entry of
errors, the same way the dict branch does. One line ingoogleapiclient/http.py. The fixture gets its missing braces so the existing test really exercises the list branch.Test
test_retry_403_list_rate_limit: a list-wrappedrateLimitExceeded403 followed by a 200 should retry once and return{}.test_no_retry_403_list_fails, now with valid JSON: a list-wrappedaccessNotConfigured403 should raiseHttpErrorwithout retrying.Before the fix both fail with
TypeError: list indices must be integers or slices, not str. After the fix both pass, and the rest of the suite gives the same results as on main.Checklist