Merge pull request #2636 from sparklemotion/2211-node-set-typeerror-o…

…n-jruby

[jruby] NodeSet#[] raises TypeError when passed invalid param type

CHANGELOG.md

@@ -44,6 +44,7 @@ This version of Nokogiri uses [`jar-dependencies`](https://github.com/mkristian/
 * [CRuby] `HTML5::Document#fragment` now always uses `body` as the parsing context. Previously, fragments were parsed in the context of the associated document's root node, which allowed for inconsistent parsing. [[#2553](https://github.com/sparklemotion/nokogiri/issues/2553)]
 * [CRuby] `Nokogiri::HTML5::Document#url` now correctly returns the URL passed to the constructor method. Previously it always returned `nil`. [[#2583](https://github.com/sparklemotion/nokogiri/issues/2583)]
 * [JRuby] Fixed a bug with adding the same namespace to multiple nodes via `#add_namespace_definition`. [[#1247](https://github.com/sparklemotion/nokogiri/issues/1247)]
+* [JRuby] `NodeSet#[]` now raises a TypeError if passed an invalid parameter type. [[#2211](https://github.com/sparklemotion/nokogiri/issues/2211)]
 
 
 ### Improved

Gemfile

@@ -23,7 +23,7 @@ group :development do
   # tests
   gem "minitest", "= 5.16.3"
   gem "minitest-reporters", "= 1.5.0"
-  gem "ruby_memcheck", "= 1.0.3"
+  gem "ruby_memcheck", "= 1.0.3" unless ::RUBY_PLATFORM == "java"
   gem "simplecov", "= 0.21.2"
 
   # rubocop

ext/java/nokogiri/XmlNodeSet.java

@@ -351,12 +351,14 @@ public class XmlNodeSet extends RubyObject implements NodeList
     if (indexOrRange instanceof RubyFixnum) {
       return slice(context, ((RubyFixnum) indexOrRange).getIntValue());
     }
-
-    int[] begLen = new int[2];
-    rangeBeginLength(context, indexOrRange, nodes.length, begLen);
-    int min = begLen[0];
-    int max = begLen[1];
-    return subseq(context, min, max - min);
+    if (indexOrRange instanceof RubyRange) {
+      int[] begLen = new int[2];
+      rangeBeginLength(context, indexOrRange, nodes.length, begLen);
+      int min = begLen[0];
+      int max = begLen[1];
+      return subseq(context, min, max - min);
+    }
+    throw context.runtime.newTypeError("index must be an Integer or a Range");
   }
 
   IRubyObject

rakelib/format.rake

@@ -73,7 +73,7 @@ namespace "format" do
   end
 
   desc "Format Nokogiri's Ruby code"
-  task "ruby" => "rubocop:check:auto_correct"
+  task "ruby" => "rubocop:check:autocorrect"
 
   desc "Regenerate tables of contents in some files"
   task "toc" do

rakelib/test.rake

@@ -1,7 +1,12 @@
 # frozen_string_literal: true
 
 require "rake/testtask"
-require "ruby_memcheck"
+
+begin
+  require "ruby_memcheck"
+rescue LoadError => e
+  warn("WARNING: ruby_memcheck is not available in this environment: #{e}")
+end
 
 #
 #  much of this was ripped out of hoe-debugging
@@ -16,10 +21,12 @@ class ValgrindTestTask < Rake::TestTask
                       "--error-exitcode=#{ERROR_EXITCODE}",
                       "--gen-suppressions=all",]
 
-  RubyMemcheck.config(
-    binary_name: "nokogiri",
-    valgrind_generate_suppressions: true,
-  )
+  if defined?(RubyMemcheck)
+    RubyMemcheck.config(
+      binary_name: "nokogiri",
+      valgrind_generate_suppressions: true,
+    )
+  end
 
   def ruby(*args, **options, &block)
     valgrind_options = check_for_suppression_file(VALGRIND_OPTIONS)
@@ -116,7 +123,9 @@ namespace "test" do
     nokogiri_test_case_configuration(t)
   end
 
-  RubyMemcheck::TestTask.new("memcheck") do |t|
-    nokogiri_test_case_configuration(t)
+  if defined?(RubyMemcheck)
+    RubyMemcheck::TestTask.new("memcheck") do |t|
+      nokogiri_test_case_configuration(t)
+    end
   end
 end

test/xml/test_node_set.rb

@@ -706,6 +706,13 @@ def awesome(ns)
             assert_equal([employees[1], employees[2], employees[3]], employees[1..3].to_a)
             assert_equal([employees[0], employees[1], employees[2], employees[3]], employees[0..3].to_a)
           end
+
+          it "raises a TypeError if param is not an integer or range" do
+            employees = xml.search("//employee")
+            assert_raises(TypeError) do
+              employees["foo"]
+            end
+          end
         end
 
         describe "#& intersection" do