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

This comment has been minimized.

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.

Adam Constabaris
@flavorjones flavorjones requested a review from jvshahid Feb 10, 2019
@flavorjones

This comment has been minimized.

Copy link
Member

flavorjones commented Feb 10, 2019

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

@jvshahid

This comment has been minimized.

Copy link
Member

jvshahid commented Feb 11, 2019

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

@adjam

This comment has been minimized.

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 left a comment

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
Adam Constabaris
@flavorjones

This comment has been minimized.

Copy link
Member

flavorjones commented Feb 13, 2019

@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
4 checks passed
4 checks passed
codeclimate 2 fixed issues
Details
codeclimate/diff-coverage 100% (80% threshold)
Details
codeclimate/total-coverage 93%
Details
concourse-ci/status Concourse CI build success
Details
@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
Projects
None yet
3 participants
You can’t perform that action at this time.