Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Oj should not raise SyntaxError when parsing invalid JSON #39

Closed
sferik opened this Issue · 9 comments

3 participants

@sferik

According to RubyDoc, SyntaxError is "raised when encountering Ruby code with an invalid syntax."

In my opinion, it should not be raised when attempting to parse JSON. SyntaxError (and other descendants of ScriptError) are typically only raised by the Ruby interpreter. Specifically, SyntaxError doesn't descend from StandardError, making it difficult to catch JSON parsing error while still raising Ruby syntax errors that may occur within the same code block.

I'd encourage you to define a custom Oj::ParseError class that inherits from StandardError. I believe this is what the other JSON libraries do.

@ohler55
Owner

I suppose that is better. I'll try to get a new release out in the next day or so.

@sferik

Cool. No rush.

@ohler55
Owner

I can understand using a custom class that inherits from Exception. Why do you feel it is necessary to group parse exceptions with standard errors?

@sferik

Exception doesn't get caught when you rescue without any arguments, StandardError does, so most people inherit from StandardError when they create custom error classes.

begin
  raise Exception
rescue
  puts "rescued" # this exception won't be caught
end
begin
  raise StandardError
rescue
  puts "rescued" # this will be caught
end
@ohler55
Owner
@funny-falcon

+1 to a ticket. Oj should define its own exception class derived from StandardError (or even better, RuntimeError)

@funny-falcon

actually +4, cause my friends are also angry of this issue

@sferik

I agree that calling rescue without any arguments is a bad practice. I just wanted to show an example.

Perhaps a better example is rescuing from a group of errors by their parent class. For example, in multi_json, I would like to to rescue from all parsing errors, regardless of the parser. If oj raised an Oj::ParseError that inhered from StandardError, I could safely write:

begin
  adapter.load(string, options)
rescue StandardError => exception
  raise MultiJson::DecodeError.new(exception.message, exception.backtrace, string)
end

If I wanted to write this same code today, I'd need to either rescue Exception or manually list all exceptions to rescue, which is difficult to maintain across an ever-changing landscape of JSON parsing libraries. The problem with rescue Exception, is that it would also rescue NoMemoryError, Interrupt, etc. which I explicitly do not want to catch here.

@ohler55
Owner

Release 1.4.0 includes the new Exception classes which are raised instead of core exceptions. Please keep the comments and requests coming.

@ohler55 ohler55 closed this
@sferik sferik referenced this issue in ohler55/ox
Closed

MultiXML spec failing with Ox 1.8.6 #37

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.