Rethrow exceptions caught by Java SAX handler

[adjam]

Patch for #1847 (JRuby implementation) -- ensures that exceptions raised from #error are not swallowed by nokogiri.internals.NokogiriHandler. Includes unit tests to verify the new behavior (as well as avoiding regressions in the SAXPullParser implementation); tests were run under both MRI 2.5.1 and JRuby 9.2.0.0

[codeclimate]

Code Climate has analyzed commit 0429513 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 93.5%.

View more on Code Climate.

[flavorjones]

Thank you for submitting this PR! I've asked @jvshahid to review this before merging.

[jvshahid]

Sorry, I have been busy lately. I will take a look at the changes and the discussion in #1847 shortly.

[adjam]

One thing I didn't test directly was the observable behavior in a script (i.e. are the rethrown exceptions causing problems elsewhere, e.g. extraneous stack traces to stderr that aren't reflected when running with the test harness?)

So, just in case it helps, I created the following script:

#!/usr/bin/env ruby

require 'stringio'
require 'nokogiri'

jruby_version = defined?(JRUBY_VERSION) ? "(jruby #{JRUBY_VERSION})" : ''
puts "Running with #{RUBY_VERSION}p#{RUBY_PATCHLEVEL} #{jruby_version} and Nokogiri #{Nokogiri::VERSION}"

class ErrDoc < Nokogiri::XML::SAX::Document
  def error(msg)
    raise(StandardError, "CUSTOM: #{msg}")
  end
end

def die(msg, status=1)
  warn(msg)
  exit(status)
end

begin
  p = Nokogiri::XML::SAX::PushParser.new(ErrDoc.new)
  p << "<xml/><xml/>"
  die("Error was swallowed(push)")
rescue StandardError => e
  if e.message.match?(/^CUSTOM:/)
    puts "Got an expected exception (push)"
  else
    warn "Oh no, got a wrong error message: #{e.message}"
  end
end

begin 
  p = Nokogiri::XML::SAX::Parser.new(ErrDoc.new)
  io = StringIO.new("<xml/><xml/>")
  p.parse(io)
  die("Error was swallowed(SAX)")
rescue StandardError => e
  if e.message.match?(/^CUSTOM:/)
    puts "Got an expected exception (SAX)"
  else
    warn "Oh no, got a wrong error message: #{e.message}"
  end
end

In my local working dir where the PR resides. Then I edited lib/nokogiri/version.rb to add -pr1872 to the version string, and then I did the following running under JRuby 9.2.6.0:

$ gem install nokogiri # installs (1.10.1)
$ # using 1.10.1
$ ruby test_push.rb || echo "Failed"

Got an expected exception (push)
Error was swallowed(SAX)
Failed

This is the current behavior under 1.10.1. Then,

$ ruby -I lib test_push.rb || echo "Failed"

Running with 2.5.3p0 (jruby 9.2.6.0) and Nokogiri 1.10.1-pr1872
Got an expected exception (push)
Got an expected exception (SAX)

Showing that the patch behaves as intended, and doesn't have any obvious external unintended consequences.

[jvshahid]

LGTM except for a few minor changes that I requested in the comments.

[flavorjones]

@adjam Thank you for your contribution! I'll acknowledge you in the CHANGELOG. 🎆

@jvshahid Thanks as always for carrying the JRuby torch. 🔥