Skip to content

Make j throw exceptions on invalid JSON #472

Closed
wants to merge 2 commits into from

4 participants

@marcusramberg
Collaborator

Right now there's no way to get exceptions out of the j commands. Since it's meant primarily for oneliners, I think it makes more sense for it to throw exceptions.

This is a breaking change, and would have to go into the 4.0 release.

Gentlemen, ready your flame throwers.

@jberger
Collaborator
jberger commented Mar 22, 2013

Before hearing other arguments, its a +1 from me. I think its a very rare case that I would want to continue if my data corrupted.

@tempire
Collaborator
@marcusramberg
Collaborator
@tempire
Collaborator
@jberger
Collaborator
jberger commented Mar 22, 2013

@tempire this is exactly what I mean, because I use j in almost everything now, I think its better if it dies on failing. If failing to parse is acceptable, then eval{j()} and you still get your error message.

@kraih
Owner
kraih commented Mar 22, 2013

I'm neutral on changing both ojo::j and Mojo::JSON::j to throw exceptions, but i would really dislike changing only one of them, since it would be inconsistent with ojo::b/Mojo::ByteStream::b and ojo::c/Mojo::Collection::c.

@marcusramberg
Collaborator
@kraih
Owner
kraih commented Mar 22, 2013

You might also want to take a look at Mojo::Message::json and Test::Mojo::json_*, they don't throw exceptions either. Not saying they all should be changed, but the big picture needs to be considered too.

@kraih
Owner
kraih commented Mar 22, 2013

On second thought, i guess i'm with @tempire on this, pretty much all of our JSON comes from untrusted sources, so it's not exactly exceptional that a lot of it will be invalid. Most of the time i just want to know if a chunk of bytes is valid JSON, not the exact reason for why it is not.

@kraih
Owner
kraih commented Mar 22, 2013

Think what i would like to see is some real world use cases that get better after the change, and so far nobody has mentioned the cases that get worse either. Looking through the documentation our use Mojo::JSON 'j'; examples are mostly for WebSockets, which are non-blocking. So those new exceptions can't be caught by the framework (no 500 pages) and will always reach the reactor layer instead. That means additional boilerplate for all WebSocket applications that want to deal with JSON.

@jberger
Collaborator
jberger commented Mar 22, 2013

yeah, ok, that is the kind of case I was worried about. I have never tried to die in a websocket handler. would there be some graceful way of handling that?

For example, while most of the json traffic from the web might be malformed (expectedly), in my apps I use is regularly for websocket traffic, in which I make the JSON on the client side and send it to the server. For me I would like to know when that chain fails and the client sends malformed JSON. Perhaps I should just be doing j($data) or <do something> but I haven't really gotten that far.

@kraih
Owner
kraih commented Mar 22, 2013

@jberger Not really, most event loops would actually take down the whole process in such cases, our final safety net is a unique feature of Mojo::IOLoop (since high availability web servers are kind of a big deal in Mojolicious). It's a measure of last resort, in general you're supposed to prevent or handle exceptions yourself inside the callback.

@jberger
Collaborator
jberger commented Mar 22, 2013

Understood. Ok now I'm distinctly neutral on this.

P.S. I need to be more careful with my phrasing. I should have said "I'm leaning +1" because I had a feeling I was missing something, and indeed I was.

@marcusramberg
Collaborator

The good use case is obviously -Mojo

master

perl -Mojo -E'say j(g("ifconfig.me/al.json"))->{ip_addr}'
Can't use an undefined value as a HASH reference at -e line 1.

vs. branch
/Users/marcus/Source/mojo [git::ojo_fatal_exceptions] [marcus@mrbook] [20:35]

perl -Ilib -Mojo -E'say j(g("ifconfig.me/al.json"))->{ip_addr}'
Malformed JSON: Expected array or object at line 0, offset 0 at -e line 1

But I see the problematic issues with implementing this for Mojo::JSON::j - Are you sure it wouldn't be ok to only implement it for ojo, @kraih ?

@kraih
Owner
kraih commented Mar 22, 2013

@marcusramberg Agree about the oneliner use case, what i dislike is just the inconsistency and ojo source getting less clean again.

@kraih
Owner
kraih commented Mar 27, 2013

Closing this issue since the discussion appears to be over and the patch as-is can't be applied.

@kraih kraih closed this Mar 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.