response.json() raises inconsistent exception type

[rowanseymour]

There are some comments about this on #4842 but I think it warrants its own issue.

If simplejson is present in the environment, the library uses it so .json() returns simplejson.errors.JSONDecodeError rather than json.decoder.JSONDecodeError

If I'm writing a library that uses requests I don't know what other libraries will be present in an environment. I expect that a method returns a consistent exception type that I can handle. As it stands, anyone writing a library (e.g. conda-forge/conda-smithy#1369, Vonage/vonage-python-sdk#197) has to check themselves if simplejson is present to know what exception type will be thrown.

[rowanseymour]

Maybe the simplest solution which wouldn't break existing code is to provide a requests.JSONDecodeError which aliases whatever exception type will be thrown, and calling code should always use that.

[BarryThrill]

Any update :)?

[wwyl1234]

I don't see a easy way to do this without breaking existing code for other developers because in the requests/models.py (around line 881). It looks like the code intentionally raises different exceptions.

def json(self, **kwargs): r"""Returns the json-encoded content of a response, if any. :param \*\*kwargs: Optional arguments that ``json.loads`` takes. :raises simplejson.JSONDecodeError: If the response body does not contain valid json and simplejson is installed. :raises json.JSONDecodeError: If the response body does not contain valid json and simplejson is not installed on Python 3. :raises ValueError: If the response body does not contain valid json and simplejson is not installed on Python 2. """

    Maybe it is easier if you catch these errors: simplejson.JSONDecodeError, json.JSONDecodeError, ValueError

[wwyl1234]

The more I think about it...
Solution 1:
add a new method so that it returns only 1 error
Solution 2:
change the json method so that it raises only 1 type or Error. I could work on this, but I am not sure which solution is better.

[rowanseymour]

I actually think the long term solution would be to stop using simplejson but maybe until then give callers a way to control which exception they're going to get, e.g. response.json(std_json=True)

[wwyl1234]

It is probably easier to add a flag (or argument) to the json method that catches all the errors and raises only 1 error like requests.JSONDecodeError.

If simplejson is intended to be phased out then adding logic such that if std_json is True then raise only json.JSONDecodeError if any error has occured. If std_json is False then old behaviour happens???

[steveberdy]

Hi. I've been working on ways to fix this issue, and so far, I've found two ways.

The most ideal way seems to be replacing usage of simplejson with json. I implemented that in one of my commits, and created a requests.JSONDecodeError to handle errors in the models (I did not test it at all, though). Of course, anyone relying on catching simplejson's JSONDecodeError would have issues if they updated their requests library. But if we wanted to consistently use json, and remove all instances of simplejson, then perhaps that's not such a bad thing.

Maybe the simplest solution which wouldn't break existing code is to provide a requests.JSONDecodeError which aliases whatever exception type will be thrown, and calling code should always use that.

I agree with this as a second option, although, in the long run, it may eventually be good to use option 1. If we did this in exceptions.py...

class JSONDecodeError(json.JSONDecodeError, simplejson.JSONDecodeError):
    """Couldn't decode the text into json"""

...it could get thrown if json.dumps results in an exception. That way, almost all instances of simplejson can be removed, except for the simplejson.JSONDecodeError inherited from in the alias, which users may be catching themselves.

[steveberdy]

@rowanseymour @BarryThrill @wwyl1234 I'm updating you on this since I made branches in a fork for the two best fixes for this issue.

Fix option 1 (not ideal for now) - replace simplejson with json, and throw a requests.JSONDecodeError when there is an issue parsing text into json: steveberdy@14bc512

Fix option 2 (more ideal for now) - replace simpejson functionality with json functionality, but make a requests.JSONDecodeError that inherits from json.JSONDecodeError and simplejson.JSONDecodeError in order for it to still be caught by users who may still be relying on catching the old error types: steveberdy@5ff9fb7

@nateprewitt @sigmavirus24 @sethmlarson I was told to contact you for maintenance requests. Could I have permission to submit a pull request for one of these branches?

P.S. I ran tests and they passed. Thanks

[sigmavirus24]

There is more than 2 options and there is only one truly ideal option - one that preserves backwards compatibility.

The standard library raises ValueError on Python 2, and it's own exception on Python 3. simplejson has its own exception class. As a result, we could create our own exception that inherits from all of the above as necessary. Something like:

try:
    from json import JSONDecodeError as stdlib_JSONDecodeError
except ImportError:
    stdlib_JSONDecodeError = ValueError

try:
    from simplejson import JSONDecodeError as sj_JSONDecodeError
except ImportError:
    sj_JSONDecodeError = Exception

class JSONDecodeError(stdlib_JSONDecodeError, sj_JSONDecodeError):
    pass

This avoids the unnecessary removal of simplejson support for those that still expect it. This also allows folks catching one of:

simplejson.JSONDecodeError
json.JSONDecodeError
ValueError

To still catch this exception too without having to change code

[steveberdy]

@sigmavirus24 I can make that edit in my second branch. I didn't do any try/except for importing the JSONDecodeError from simplejson or json, so I'll do that and then I'll submit a PR.