david-caro / python-foreman

Small low level python wrapper around Foreman API
GNU General Public License v2.0
57 stars 37 forks source link

Fix JSONDecodeError reference #65

Closed pief closed 8 years ago

pief commented 8 years ago

The "requests" module does not know about JSONDecodeError because it actually comes from the underlying simplejson module. Reuse requests' internal reference so we notice if requests should ever change from simplejson to a different module.

david-caro commented 8 years ago

Why should we ever want to care what's the specific implementation inside requests?

pief commented 8 years ago

Because we need to be prepared to catch the exception and need to do so in a proper way.

Background: in PR66 I copied existing code ("except requests.JSONDecodeError"). This failed because requests does not define that error:

2016-03-01 11:21:47 GlobalExceptionHandl():630 CRITICAL:   File "/usr/lib/python2.7/site-packages/foreman/client.py", line 911, in do_get
2016-03-01 11:21:47 GlobalExceptionHandl():630 CRITICAL:     return self._process_request_result(res)
2016-03-01 11:21:47 GlobalExceptionHandl():630 CRITICAL:   File "/usr/lib/python2.7/site-packages/foreman/client.py", line 887, in _process_request_result
2016-03-01 11:21:47 GlobalExceptionHandl():630 CRITICAL:     except requests.JSONDecodeError:
2016-03-01 11:21:47 GlobalExceptionHandl():630 CRITICAL: AttributeError: 'module' object has no attribute 'JSONDecodeError'
david-caro commented 8 years ago

Then we should catch the proper exception defined by requests instead of going deep into requests implementation internal details no?

pief commented 8 years ago

Correct, but at least in my requests version (2.9.1) it doesn't seem to define an exception of its own:

    def json(self, **kwargs):
        """Returns the json-encoded content of a response, if any.

        :param \*\*kwargs: Optional arguments that ``json.loads`` takes.
        """

        if not self.encoding and len(self.content) > 3:
            # No encoding set. JSON RFC 4627 section 3 states we should expect
            # UTF-8, -16 or -32. Detect which one to use; If the detection or
            # decoding fails, fall back to `self.text` (using chardet to make
            # a best guess).
            encoding = guess_json_utf(self.content)
            if encoding is not None:
                try:
                    return complexjson.loads(
                        self.content.decode(encoding), **kwargs
                    )
                except UnicodeDecodeError:
                    # Wrong UTF codec detected; usually because it's not UTF-8
                    # but some other 8-bit codec.  This is an RFC violation,
                    # and the server didn't bother to tell us what codec *was*
                    # used.
                    pass
        return complexjson.loads(self.text, **kwargs)
david-caro commented 8 years ago

The docs say that it will rise a value error: http://docs.python-requests.org/en/master/user/quickstart/#json-response-content

Is that wrong?

pief commented 8 years ago

Ping.