Remove passing exception as args to super in APIError

[mike-flowers-airbnb]

First, thanks for this tool!

We have a pipeline where exceptions from packages such as gspread are pickled and stored for further processing. Unfortunately, the APIError exception is not able to be unpickled as the args it passed to the Exception super are different from what is passed into the exception itself.

The sample test extension exposes this error as seen below:

    def __init__(self, response: Response):
        super().__init__(self._extract_error(response))
        self.response: Response = response
>       self.error: Mapping[str, Any] = response.json()["error"]
E       AttributeError: 'dict' object has no attribute 'json'

gspread/exceptions.py:45: AttributeError

This PR ultimately only replaces the extracted error with the raw response passed to the kwargs. Given that __str__ and __repr__ are overloaded and all other usage of this exception use the other members, there is no visible impact from this change.

[alifeee]

The lint is failing for unrelated reasons. Please bear with us while #1478 is solved :)

[lavigne958]

Hi @mike-flowers-airbnb thank you for this improvement!

I need to run a couple of tests first.

As you remove the use of the function extract_error and we already extract it in the response object just below could you please remove the method extract_error please ?

[mike-flowers-airbnb]

Hi @mike-flowers-airbnb thank you for this improvement!

I need to run a couple of tests first.

As you remove the use of the function extract_error and we already extract it in the response object just below could you please remove the method extract_error please ?

Sure thing, removed the obsolete function

[lavigne958]

@mike-flowers-airbnb problem should be solved now, please rebase your branch on laster commit on master branch in order to pass the CI.

[mike-flowers-airbnb]

✅ Done , thanks!

[lavigne958]

Hi I ran som regression tests. I found an issue in the current design:

• current a user is allowed to do something like this:

try:
    sheet = file.del_worksheet_by_id(112341462553536987)
except gspread.exceptions.APIError as e:
    pprint(e.args[0]["status"])

In our current code base, this will return:

• the first argument passed to the Exception parent class
• this argument is a dict, so it will look for the key code in this dict.
• This returns the error status returned by the API.

With your current implementation we face the following error with the above code:

Traceback (most recent call last):
  File "/..../exception_test.py", line 10, in <module>
    print(e.args[0]["code"])
TypeError: 'Response' object is not subscriptable

This is because the first argument passed to the Exception parent class is no longer a dict but a Response object.

I tried the below code that would allow us to make the changes and keep the old behavior and prevent regression.

class APIError(GSpreadException):
    """Errors coming from the API itself,
    such as when we attempt to retrieve things that don't exist."""

    def __init__(self, response: Response):
        super().__init__(response)
        self.response: Response = response
        self.error: Mapping[str, Any] = response.json()["error"]
        self.code: int = self.error["code"]

    @property
    def args(self):
        return [dict(self.error)]

the above @property on args will allow us to catch anyone trying to access the args attribute and return the error object as a dict.

@alifeee what do you think of this workaround ?

apart from that, which could introduce a breaking change, I am fine with the changes.

[mike-flowers-airbnb]

Appreciate the callout here @lavigne958 . Definitely don't want to be breaking exception catch flows.

I did some searching and found that there is actually a proper form to expose the ability for pickling to work when args don't match. I had incorrectly assumed that args were supposed to match the underlying Exception args, but it looks like there is a built-in capability to support this scenario by using reduce. If there's no objections, I'd propose that we use this instead as it will achieve the desired effect while not tampering with other lower-level functionality. Let me know what you think.

[lavigne958]

Appreciate the callout here @lavigne958 . Definitely don't want to be breaking exception catch flows.

I did some searching and found that there is actually a proper form to expose the ability for pickling to work when args don't match. I had incorrectly assumed that args were supposed to match the underlying Exception args, but it looks like there is a built-in capability to support this scenario by using reduce. If there's no objections, I'd propose that we use this instead as it will achieve the desired effect while not tampering with other lower-level functionality. Let me know what you think.

that's a great news, if I understand correctly your point, you don't need these changes anymore from gspread right ?
If yes, then I'll be keen on making the changes your propose, but in the next major release so we can introduce the breaking change with a proper warning and users can adjust themselves.

let me know if that's ok with you and I'll put your issue in the next major release milestone.

[mike-flowers-airbnb]

Sorry I mean that I can change this PR so that the exception implements __reduce__ and no compatibility will be lost while also introducing new pickling support. Let me add this in real quick to show.

[mike-flowers-airbnb]

I just pushed a new commit to implement it. You'll notice that the args property is now completely backwards compatible and the exception is able to be properly pickled/unpickled. I also extended the existing test to include your regression test. Let me know your thoughts, thanks.

[lavigne958]

I just pushed a new commit to implement it. You'll notice that the args property is now completely backwards compatible and the exception is able to be properly pickled/unpickled. I also extended the existing test to include your regression test. Let me know your thoughts, thanks.

thank you for the prompt reply, I'll have a look and test it. looks qiet good to me !

[mike-flowers-airbnb]

@lavigne958 , hey there, just checking in to see if anything else is needed for this PR. Thanks.

[lavigne958]

@lavigne958 , hey there, just checking in to see if anything else is needed for this PR. Thanks.

Hi so far what is mostly needed is "time", I didn't have any time recently to take a look.

With the coming days I will have more time to take a look at it.

Sorry for the delay.

[mike-flowers-airbnb]

@lavigne958 , hey there, just checking in to see if anything else is needed for this PR. Thanks.

Hi so far what is mostly needed is "time", I didn't have any time recently to take a look.

With the coming days I will have more time to take a look at it.

Sorry for the delay.

No worries, definitely understand, thanks for the update!

[lavigne958]

looks good to me, with regression test as extra ! thank you.
Only a small detail about the indentation just in case then: looks good to merge

[lavigne958]

LGTM