Remove try/except block for JSON decoding

Hi,

I use requests a lot to interact with some applications made by other developers who use a programming language where a JSON encoding implementation doesn't exist.
Sometimes, their JSON content is wrong, I must notify them.

For me, it's important to catch the JSON decoding problem in the same way as the others errors, with an exception.

With this patch, the user of requests can handle the exception himself.

Thanks for your feedback.


This pull request [passes](http://travis-ci.org/kennethreitz/requests/builds/1673216) (merged 6f13ea71 into 64da351e).


In this case, I think you should be decoding the json yourself.

Thanks for the contribution though. Sorry I can't include it :)


Ok, it's already the case for me ;-)

My comment was on the error handling. To my knowledge, the good practice it's always use exceptions, instead of an if block.

If you prefer keep this behaviour, I won't use .json(), that's it, the alternative is very easy to implement :-)


Well, the intention is to simply decode JSON if it's there. If the content of the response isn't JSON, there's no need to raise an exception, there simply isn't any JSON, so the value is None. 

This behaves much like Flask/Werkzeug's handling of request objects.


Ok, now, I understand the goal of .json method. My use case is specific for me to handle JSON encoding errors, forget this pull request.


@kennethreitz I got bitten by this today. To me the behavior of `response.json` is very unintuitive. If I use `response.json` that's because I expect the response to actually be json-formatted. And I expect this call to fail in case it's not proper json. Say an API I use is mis-behaving and returns weird errors, if an exception occurs here I know something is wrong. Otherwise I just get a `None` object, things start to fail elsewhere in my system and I need to figure out why an object is `None` instead of a proper api response and where this comes from.

This is also inconsistent with python's `json.loads`.

At the very least this property should be renamed to `try_json` or `json_or_none` / `json_maybe` ;). But the current implementation is weak. When you say:

> there simply isn't any JSON, so the value is None

If I translate this to reading a file, this could also mean `open(filename, 'r')` returning `None` if the file doesn't exist. What you're currently saying is basically "if you expect json data. don't use `response.json`", which makes me wonder why there is such an API in the first place.

Sorry to be that guy, I'm rather strongly opposed to silent errors because they tend to catch you by surprise at the wrong moment. Hope you'll reconsider this change.

Thanks!


@brutasse If you keep the request object then you can test if `r.json` is None. If so, then check the response object for the content of the response (e.g., `r.content`) and that will tell you what was returned by the API. I'm handling things that way myself.


@sigmavirus24 I know I can do it this way but it doesn't address the fact that the API is misleading: I expect some decoding happen and if the content is malformed then this decoding should fail loudly. I'd rather use json.loads on the response content directly but again, why is there a helper for json decoding if you have to hack around it to even notice there was a decoding error…

To quote pep20, "Errors should never pass silently unless explicitly silenced." I know this silencing is documented but I wouldn't qualify it as explicit.


I'm not exactly sure how writing a function that does something like:

``` python
def get_json_or_error(request):
    if request.json is None:
        # Check content for encoding error and throw error
    return request.json
```

And then wrapping that in your own try/except block is any different 
from requests removing it from that for you. For one thing, if the API 
returned a 404 (not found) error in which no json was sent, then it 
makes perfect sense that request.json would be None. How would 
request.json throwing an error in that case be in the least bit 
informative?

As per being explicit, is requests supposed to throw an error if someone 
tries to access json when there is no json (say on an HTML document), 
e.g., 

``` python
r = requests.get('http://google.com')
r.json
```

Would make any sense. The person may have been expecting to see json for 
whatever reason but there isn't so requests doesn't return any. Maybe 
the json property could be more intelligent and check the content_type 
in the headers, but then the user still has to check the original 
request object. Say it was written like so:

``` python
@property
def json(self):
    if 'json' in self.headers.get('content-encoding'):
        return json.loads(self.text or self.content)
    return None
```

Then you might have an exception thrown or you might get json or you 
might get None. If you're expecting json and you don't check afterwards 
if the object is None (assuming no exception was thrown) then you're 
application is still poorly coded. As it is now, I'm not certain how 
much more explicit you want it to be. Returning None is fairly obvious 
to me at least. And beyond that, I'm not certain that headers will be 
set by that point (it should be) but it's also entirely possible that 
you (or someone else) accidentally change them and then regardless of 
the response, you get None. Sure that may be easy to find and fix but 
then requests gets blamed for that as they are getting blamed now for 
not throwing an exception.


This isn't really up for discussion. The json property is to be considered a filter. If the content contains no json, the value is None.

Just like in Flask.


### What if the response body is `null` which decodes as `None`?

There is an ambiguity: does `response.json==None` means it is not valid json body OR the reponse body is the json value `null`?

For your information, this is actually the case on several facebook graph api methods.
i.e.: Facebook subscription APIs for real time updates return `null` in case of success.

### Why use `Response.json` then?

Because of this ambiguity, why should we use `Response.json` then?

From the documentation:

> There’s also a builtin JSON decoder, in case you’re dealing with JSON data

Any proper JSON decoder fails decoding malformated data. 

I think lots of folks would expect same behaviour when using the `response.json` properly.


Indeed, Pyramid, WebOb, WebTest and others don’t silence exceptions if you access request.json_body / response.json / etc and it’s malformed.  Likewise, if you access request.POST and the request body is not proper multipart/form-data, you get an exception.  I find this behavior is intuitive and helpful, and wish I could use requests’ json attribute in the same way.


I'm agree with @brutasse and @merwok 


Again, this isn't really up for discussion.

Thank for your feedback though, it's well noted and will be considered in future releases.


Rather, I fully understand why you'd want it that way. No need for further elaboration.


I see this is not up for discussion, but, for the record, one more evidence the current behavior is less useful that is could be: https://github.com/jgorset/facepy/issues/54


> Thank for your feedback though, it's well noted and will be considered in future releases.

It means I can reopen my pull request, or you'll implement an other thing in a future release ?


+1.

I don't mind using a control flow that doesn't involve catching an exception to facilitate for an unexpected response (though I, too, think it would be more pythonic to raise an exception in this case), but the ambiguity of `None` makes this a dubious feature in an otherwise excellent library.


This is implemented now, btw :)


And this is why we love you.  Happy holidays season!