New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSONDecodeError should be catched #72

Closed
saz opened this Issue Jan 21, 2015 · 9 comments

Comments

4 participants
@saz

saz commented Jan 21, 2015

If a response results in a status code of 200, but the response isn't valid JSON, there is a simplejson.scanner.JSONDecodeError exception thrown.

It would be good, if such an exception is catched and re-raised as an RedmineException (or a new exception type).

https://github.com/maxtepkeev/python-redmine/blob/master/redmine/__init__.py#L124

Any opinions on this? I can create a PR for this

@maxtepkeev maxtepkeev added the bug label Jan 21, 2015

@maxtepkeev maxtepkeev self-assigned this Jan 21, 2015

@maxtepkeev

This comment has been minimized.

Owner

maxtepkeev commented Jan 21, 2015

Hi,

That is kind of a strange situation, because if a response has a 200 status code, it should return a valid json inside, can you provide a use case for this, or describe a situation when you got something instead of json inside a response.

@saz

This comment has been minimized.

saz commented Jan 21, 2015

This might happen, if there is an error in the web server configuration. It is an edge case, for sure, but, as an internal server error is handled, I think this should be handled, too.

@maxtepkeev

This comment has been minimized.

Owner

maxtepkeev commented Jan 21, 2015

Ok, agree. Go for it.

maxtepkeev added a commit that referenced this issue Feb 3, 2015

@maxtepkeev

This comment has been minimized.

Owner

maxtepkeev commented Feb 3, 2015

Fixed in 34cb6e7.

@maxtepkeev maxtepkeev closed this Feb 3, 2015

@doc212

This comment has been minimized.

doc212 commented May 27, 2015

Hi,

I'm not sure this is the right place for this but I ran into this case where the server was requiring an SSL client certificate.
As my Redmine object was not properly configured, the server was actually returning an HTML response saying "this page is restricted to company employees ..."

My point is that to troubleshoot this, I had to modify the code and add a

 print response.text

before the error is thrown:

raise JSONDecodeError

I was wondering if you could log this response somewhere or throw it back up as exception argument or something.

Thanks for the library by the way!

@maxtepkeev

This comment has been minimized.

Owner

maxtepkeev commented Jun 8, 2015

Hi @doc212 ,

Your server is badly configured or it uses badly written scripts, because if it returns something like "this page is restricted to company employees ..." it should also return a 403 http status code and it returns 200 which is not correct. So if you can reconfigure your server this is the right way to go.

If not, then I can add response.text to JSONDecodeError exception as an argument as you suggested.

Let me know what you think.

@doc212

This comment has been minimized.

doc212 commented Jun 8, 2015

Hi @maxtepkeev,

I totally agree that my server was misconfigured and that it did not respect the http protocol. It said ok I'm sending you what you asked for but it didn't so you threw an exception because the content you received was not as expected. The name of the exception clearly indicates that: JSONDecodeError

I'm just saying that from the user point of view, when I got the exception without any details, I was like... ok ... what now? Is it really malformed JSON or is it a bug in the parsing process? So the natural next step is to try and take a look at the content (what else could I do? Give up? 😄)

Basically, by throwing this exception you're saying I got bad json and the user will always want to know why you think it's bad or what you got, so that he can try and fix it. The internal exception would probably be helpful too because it'd probably be something like Unexpected token <: expected { or something like that but really, I think the content is the key to understand this type of error.

On last example and I rest my case 😉 Imagine you get a SocketError. You'll start sniffing around the network or so ... But a SocketError saying Connection could not be made because the remote party actively refused it, you immediately know where to look: check the connection port, or whether the server's running or whether some firewall is in the way...

I guess my point is just that an exception should contain useful information to troubleshoot the error (besides its primary purpose of interrupting the control flow of the application). And so I think maybe in case of an error http status, like 401, 403 etc... you could also throw the response body along with the exception: it will probably contain something to clarify the error.

But again, thanks for the software, it's really helpful! 👍

@maxtepkeev

This comment has been minimized.

Owner

maxtepkeev commented Jun 17, 2015

Hi @doc212

Wow, amazing explanation.

I've created a separate issue #93 for this. But I don't think that other exceptions, e.g. for 401, 403 etc status codes need the response body as well, because their status code says pretty much everything, but for the case of 200 status code - you're right, we should include response body to the exception because that will help to identify cause of the error.

Thanks for the report and great explanation.

@0x55aa

This comment has been minimized.

Contributor

0x55aa commented Jun 23, 2015

good work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment