Skip to content
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

Rethrow exceptions caught by Java SAX handler #1872

Conversation

adjam
Copy link

@adjam adjam commented Feb 10, 2019

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
Copy link

codeclimate bot commented Feb 10, 2019

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
Copy link
Member

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

@jvshahid
Copy link
Member

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

@adjam
Copy link
Author

adjam commented Feb 12, 2019

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.

Copy link
Member

@jvshahid jvshahid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ext/java/nokogiri/internals/NokogiriHandler.java Outdated Show resolved Hide resolved
ext/java/nokogiri/internals/NokogiriHandler.java Outdated Show resolved Hide resolved
@flavorjones
Copy link
Member

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

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

@flavorjones flavorjones merged commit 9ffde4b into sparklemotion:master Feb 13, 2019
@flavorjones flavorjones added this to the v1.11.0 milestone Feb 13, 2019
flavorjones added a commit that referenced this pull request Feb 13, 2019
related to #1872

[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants