burnash / gspread

Google Sheets Python API
https://docs.gspread.org
MIT License
7.09k stars 948 forks source link

cannot import `JSONDecodeError` since v6.1.3 #1518

Open alifeee opened 1 week ago

alifeee commented 1 week ago

this has been reported somewhere else

I have changed the following line

- from requests.exceptions import JSONDecodeError
+ from requests import JSONDecodeError

...as I think it may be a problem with the structure of the requests library that some (older?) versions of Python cannot import properly

we should probably find out the earliest version of requests that has JSONDecodeError and specify that gspread requires that version or later.

it seems to have been added in https://github.com/psf/requests/pull/5856 as part of requests version 2.27. My local version is 2.25.1 so it is probably not a good idea to pin such a recent version

image

how else can we fix this...? we can use the JSONDecodeError from json module ?

it looks like requests only started throwing JSONDecodeError about 3 years ago (https://github.com/psf/requests/pull/5810) anyway, so maybe we only sometimes try to catch the error ?

something like

try:
  from requests import JSONDecodeError
except ImportError:
  JSONDecodeError = None

...
       if JSONDecodeError is not None:
         try:
              error = response.json()["error"]
          except JSONDecodeError:
            ...
        else:
         try:
              error = response.json()["error"]
          except ValueError:
            ...

...or perhaps we use ValueError for now, as I believe(?) JSONDecodeError inherits from ValueError, so we could have

try:
  from requests import JSONDecodeError
except ImportError:
  JSONDecodeError = ValueError

...

  except JSONDecodeError:
...
alifeee commented 1 week ago

this should become a 6.1.4 release

az22-atl commented 1 week ago

Thanks, I was the one with the problem. Our corporate stack only uses Python 3.8 and I can't update it, I can confirm using 6.1.2 is fine but throws the above error with 6.1.3

lavigne958 commented 1 week ago

Hi thanks for the analysis @alifeee !

I agree with you, version 2.27.0 is the lowest version we need now in order to have the JSON Exception.

I agree with you, we must make a new version v6.1.4 to fix this.

I see you outcomes here:

  1. we add requests==v2.27.0 as dependency in our package build system.
  2. we don't catch JSONDecodeError but rather a generic Exception in case something else happens, like pure Exception and handle that as an error and create the fake -1 error response in that case. that allows us to let anyone use any version.

I have a preference for option 1 it's safer I believe.

alifeee commented 5 days ago

I think option 1 is needlessly restrictive to people who cannot easily change their environments

lavigne958 commented 5 days ago

Agreed, we don't directly depend on request package. We depend on google-auth package which provides us with an authorized session objet. Which makes requests.

I checked and google-auth package has defined a dependency on requests package as it see fits. Which is fine. Currently it's defined as requests>=2.0.0

We should probably go for option 2 which could catch other unexpected errors and prevent us from having stronger dependencies.