Make yaml loading error more transparent to the end user

[purbon]

Fix #5

[guyboertje]

There is an understandable but flawed reason why this line was added. It is because YAML (on jruby, == Psych) defines SyntaxError as a subclass of RuntimeError not StandardError, so hence the rescue Exception => e

This is a classic case of the discussion that @jsvd and I raised about plugins handling specific errors in their rescue blocks. See this discussion issue

The work that may raise an Error in line 139 is doing many things, opening and parsing a file then merging the results to a Hash.

There should be two rescue sections.

rescue YAML::SyntaxError => e
  msg = "..." #specific to the syntax problem
  raise Exception.new(msg) #this will be thrown up to the pipeline; v2.0 should rescue, attempt to log it and reraise, this will kill the worker thread
rescue StandardError => e
  msg = "..." #general message
  #log error

[purbon]

I know SyntaxError is a RuntimeError, because in ruby is a ruby syntax error so it happen in runtime, not an error type located for things like YAML, JSON, or XML syntax validation.

The question I ask myself here is, the actual behaviour is if registering is set to true, raise the exception otherwise not. If we aim to keep this, adding YAML::SyntaxError provide us only with a way to detect when this is a syntax error or not. I understand we don't want to react different for SyntaxError and other errors (StandardError). In your example you aim to raise only SyntaxException up, and log everything under StandardError.

I understand the issue as not reporting everything as syntax error even when they are not, in my example user will notice when Psych claims an error, and also when there are other error.

What do you think?

[purbon]

@guyboertje follow up on that?

[guyboertje]

But with rescue => e on new line 140, you are only handling StandardError not SyntaxError or any other non-StandardError that might be thrown. Also this StandardError is being converted to a RuntimeError and re-raised - if the caller, in its rescue blocks, makes choices based on the class then the choices may be wrong.

If your aim is to add extra info to the error message, then consider this alternative:

  rescue Exception => e
    msg = ...
    new_error = e.class.new(msg)
    if registering
      raise new_error
    else
      @logger.warn("#{new_error.message}, continuing ..")
    end
  end

[purbon]

@guyboertje thoughts on this PR? what do you think? good? any concern? thanks a lot for your time.

[purbon]

@guyboertje feedback on this?

[guyboertje]

I seems to me that the original exception may have info about where (line, column) the syntax problem is. And in a big YAML file that info is gold.

Consider, trying to extract that from the original error and logging an error message especially during register.

[guyboertje]

If you do implement my suggestion, consider changing the YAML fixture to be a lengthy (10 lines) one that have a proper syntax error and verify that the error message logged shows it.

[purbon]

this information is already included in the message coming out of the exception.

[guyboertje]

Other than one idea to consider - LGTM.

[elasticsearch-bot]

Pere Urbon merged this into the following branches!

Branch	Commits
master	d0ef4e1, 2b1fc61, 14f7b0e