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") |
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