Merge pull request #2867 from sparklemotion/2147-deprecate-nonnamespa…

…ced-xpath-functions

deprecate nonnamespaced xpath functions

CHANGELOG.md

@@ -56,6 +56,7 @@ You can read more about this in the decision record at `adr/2023-04-libxml-memor
 * Passing a `Nokogiri::XML::Node` as the first parameter to `CDATA.new` is deprecated and will generate a warning. This parameter should be a kind of `Nokogiri::XML::Document`. This will become an error in a future version of Nokogiri.
 * Passing a `Nokogiri::XML::Node` as the first parameter to `Schema.from_document` is deprecated and will generate a warning. This parameter should be a kind of `Nokogiri::XML::Document`. This will become an error in a future version of Nokogiri.
 * Passing a `Nokogiri::XML::Node` as the second parameter to `Text.new` is deprecated and will generate a warning. This parameter should be a kind of `Nokogiri::XML::Document`. This will become an error in a future version of Nokogiri.
+* [CRuby] Calling a custom XPath function without the `nokogiri` namespace is deprecated and will generate a warning. Support for non-namespaced functions will be removed in a future version of Nokogiri. (Note that JRuby has never supported non-namespaced custom XPath functions.)
 
 
 ### Performance

ext/nokogiri/nokogiri.h

@@ -218,9 +218,9 @@ xmlParserCtxtPtr noko_xml_sax_parser_context_unwrap(VALUE rb_context);
 #define DISCARD_CONST_QUAL_XMLCHAR(v) DISCARD_CONST_QUAL(xmlChar *, v)
 
 #if HAVE_RB_CATEGORY_WARNING
-#  define NOKO_WARN_DEPRECATION(message) rb_category_warning(RB_WARN_CATEGORY_DEPRECATED, message)
+#  define NOKO_WARN_DEPRECATION(message...) rb_category_warning(RB_WARN_CATEGORY_DEPRECATED, message)
 #else
-#  define NOKO_WARN_DEPRECATION(message) rb_warning(message)
+#  define NOKO_WARN_DEPRECATION(message...) rb_warning(message)
 #endif
 
 void Nokogiri_structured_error_func_save(libxmlStructuredErrorHandlerState *handler_state);

ext/nokogiri/xml_xpath_context.c

@@ -320,6 +320,13 @@ handler_lookup(void *data, const xmlChar *c_name, const xmlChar *c_ns_uri)
 {
   VALUE rb_handler = (VALUE)data;
   if (rb_respond_to(rb_handler, rb_intern((const char *)c_name))) {
+    if (c_ns_uri == NULL) {
+      NOKO_WARN_DEPRECATION(
+        "A custom XPath or CSS handler function named '%s' is being invoked without a namespace."
+        " Please update your query to reference this function as 'nokogiri:%s'."
+        " Invoking custom handler functions without a namespace is deprecated and support will be removed in a future release of Nokogiri.",
+        c_name, c_name);
+    }
     return method_caller;
   }
 

lib/nokogiri/css/xpath_visitor.rb

@@ -133,7 +133,7 @@ def visit_function(node)
           args += node.value[1..-1].map do |n|
             n.is_a?(Nokogiri::CSS::Node) ? n.accept(self) : n
           end
-          "#{node.value.first}#{args.join(",")})"
+          "nokogiri:#{node.value.first}#{args.join(",")})"
         end
       end
 
@@ -207,7 +207,7 @@ def visit_pseudo_class(node)
           when "parent" then "node()"
           when "root" then "not(parent::*)"
           else
-            node.value.first + "(.)"
+            "nokogiri:#{node.value.first}(.)"
           end
         end
       end

lib/nokogiri/xml/searchable.rb

@@ -36,16 +36,19 @@ module Searchable
       #   node.search('.//address[@domestic=$value]', nil, {:value => 'Yes'})
       #
       # 💡 Custom XPath functions and CSS pseudo-selectors may also be defined. To define custom
-      # functions create a class and implement the function you want to define. The first argument
-      # to the method will be the current matching NodeSet. Any other arguments are ones that you
-      # pass in. Note that this class may appear anywhere in the argument list. For example:
+      # functions create a class and implement the function you want to define, which will be in the
+      # `nokogiri` namespace in XPath queries.
+      #
+      # The first argument to the method will be the current matching NodeSet. Any other arguments
+      # are ones that you pass in. Note that this class may appear anywhere in the argument
+      # list. For example:
       #
       #   handler = Class.new {
       #     def regex node_set, regex
       #       node_set.find_all { |node| node['some_attribute'] =~ /#{regex}/ }
       #     end
       #   }.new
-      #   node.search('.//title[regex(., "\w+")]', 'div.employee:regex("[0-9]+")', handler)
+      #   node.search('.//title[nokogiri:regex(., "\w+")]', 'div.employee:regex("[0-9]+")', handler)
       #
       # See Searchable#xpath and Searchable#css for further usage help.
       def search(*args)
@@ -160,16 +163,18 @@ def at_css(*args)
       #   node.xpath('.//address[@domestic=$value]', nil, {:value => 'Yes'})
       #
       # 💡 Custom XPath functions may also be defined. To define custom functions create a class and
-      # implement the function you want to define. The first argument to the method will be the
-      # current matching NodeSet. Any other arguments are ones that you pass in. Note that this
-      # class may appear anywhere in the argument list. For example:
+      # implement the function you want to define, which will be in the `nokogiri` namespace.
+      #
+      # The first argument to the method will be the current matching NodeSet. Any other arguments
+      # are ones that you pass in. Note that this class may appear anywhere in the argument
+      # list. For example:
       #
       #   handler = Class.new {
       #     def regex(node_set, regex)
       #       node_set.find_all { |node| node['some_attribute'] =~ /#{regex}/ }
       #     end
       #   }.new
-      #   node.xpath('.//title[regex(., "\w+")]', handler)
+      #   node.xpath('.//title[nokogiri:regex(., "\w+")]', handler)
       #
       def xpath(*args)
         paths, handler, ns, binds = extract_params(args)

test/css/test_xpath_visitor.rb

@@ -343,27 +343,27 @@ def assert_xpath(expecteds, asts)
       end
 
       it "miscellaneous pseudo-classes are converted into xpath function calls" do
-        assert_xpath("//a[aaron(.)]", parser.parse("a:aaron"))
-        assert_xpath("//a[aaron(.)]", parser.parse("a:aaron()"))
-        assert_xpath("//a[aaron(.,12)]", parser.parse("a:aaron(12)"))
-        assert_xpath("//a[aaron(.,12,1)]", parser.parse("a:aaron(12, 1)"))
+        assert_xpath("//a[nokogiri:aaron(.)]", parser.parse("a:aaron"))
+        assert_xpath("//a[nokogiri:aaron(.)]", parser.parse("a:aaron()"))
+        assert_xpath("//a[nokogiri:aaron(.,12)]", parser.parse("a:aaron(12)"))
+        assert_xpath("//a[nokogiri:aaron(.,12,1)]", parser.parse("a:aaron(12, 1)"))
 
-        assert_xpath("//a[link(.)]", parser.parse("a:link"))
-        assert_xpath("//a[visited(.)]", parser.parse("a:visited"))
-        assert_xpath("//a[hover(.)]", parser.parse("a:hover"))
-        assert_xpath("//a[active(.)]", parser.parse("a:active"))
+        assert_xpath("//a[nokogiri:link(.)]", parser.parse("a:link"))
+        assert_xpath("//a[nokogiri:visited(.)]", parser.parse("a:visited"))
+        assert_xpath("//a[nokogiri:hover(.)]", parser.parse("a:hover"))
+        assert_xpath("//a[nokogiri:active(.)]", parser.parse("a:active"))
 
-        assert_xpath("//a[foo(.,@href)]", parser.parse("a:foo(@href)"))
-        assert_xpath("//a[foo(.,@href,@id)]", parser.parse("a:foo(@href, @id)"))
-        assert_xpath("//a[foo(.,@a,b)]", parser.parse("a:foo(@a, b)"))
-        assert_xpath("//a[foo(.,a,@b)]", parser.parse("a:foo(a, @b)"))
-        assert_xpath("//a[foo(.,a,10)]", parser.parse("a:foo(a, 10)"))
-        assert_xpath("//a[foo(.,42)]", parser.parse("a:foo(42)"))
-        assert_xpath("//a[foo(.,'bar')]", parser.parse("a:foo('bar')"))
+        assert_xpath("//a[nokogiri:foo(.,@href)]", parser.parse("a:foo(@href)"))
+        assert_xpath("//a[nokogiri:foo(.,@href,@id)]", parser.parse("a:foo(@href, @id)"))
+        assert_xpath("//a[nokogiri:foo(.,@a,b)]", parser.parse("a:foo(@a, b)"))
+        assert_xpath("//a[nokogiri:foo(.,a,@b)]", parser.parse("a:foo(a, @b)"))
+        assert_xpath("//a[nokogiri:foo(.,a,10)]", parser.parse("a:foo(a, 10)"))
+        assert_xpath("//a[nokogiri:foo(.,42)]", parser.parse("a:foo(42)"))
+        assert_xpath("//a[nokogiri:foo(.,'bar')]", parser.parse("a:foo('bar')"))
       end
 
       it "bare pseudo-class matches any ident" do
-        assert_xpath("//*[link(.)]", parser.parse(":link"))
+        assert_xpath("//*[nokogiri:link(.)]", parser.parse(":link"))
         assert_xpath("//*[not(@id='foo')]", parser.parse(":not(#foo)"))
         assert_xpath("//*[count(preceding-sibling::*)=0]", parser.parse(":first-child"))
       end
@@ -483,18 +483,18 @@ def visit_pseudo_class_aaron(node)
 
     it "handles pseudo-class with class selector" do
       assert_xpath(
-        "//a[active(.) and contains(concat(' ',normalize-space(@class),' '),' foo ')]",
+        "//a[nokogiri:active(.) and contains(concat(' ',normalize-space(@class),' '),' foo ')]",
         parser.parse("a:active.foo"),
       )
       assert_xpath(
-        "//a[contains(concat(' ',normalize-space(@class),' '),' foo ') and active(.)]",
+        "//a[contains(concat(' ',normalize-space(@class),' '),' foo ') and nokogiri:active(.)]",
         parser.parse("a.foo:active"),
       )
     end
 
     it "handles pseudo-class with an id selector" do
-      assert_xpath("//a[@id='foo' and active(.)]", parser.parse("a#foo:active"))
-      assert_xpath("//a[active(.) and @id='foo']", parser.parse("a:active#foo"))
+      assert_xpath("//a[@id='foo' and nokogiri:active(.)]", parser.parse("a#foo:active"))
+      assert_xpath("//a[nokogiri:active(.) and @id='foo']", parser.parse("a:active#foo"))
     end
 
     it "handles function with pseudo-class" do

test/xml/test_node_set.rb

@@ -173,8 +173,8 @@ class TestNodeSet < Nokogiri::TestCase
             set = xml.xpath("//staff")
 
             [
-              [:xpath,  "//*[awesome(.)]"],
-              [:search, "//*[awesome(.)]"],
+              [:xpath,  "//*[nokogiri:awesome(.)]"],
+              [:search, "//*[nokogiri:awesome(.)]"],
               [:css,    "*:awesome"],
               [:search, "*:awesome"],
             ].each do |method, query|

test/xml/test_xpath.rb

@@ -121,6 +121,24 @@ def test_css_search_with_ambiguous_integer_or_string_attributes
         refute_nil(doc.at_css("img[width=200]"))
       end
 
+      def test_xpath_with_nonnamespaced_custom_function_is_deprecated_but_works
+        skip_unless_libxml2("only deprecated in CRuby")
+
+        result = nil
+        assert_output("", /Invoking custom handler functions without a namespace is deprecated/) do
+          result = @xml.xpath("anint()", @handler)
+        end
+        assert_equal(1230456, result)
+      end
+
+      def test_xpath_with_namespaced_custom_function_is_not_deprecated
+        result = nil
+        assert_silent do
+          result = @xml.xpath("nokogiri:anint()", @handler)
+        end
+        assert_equal(1230456, result)
+      end
+
       def test_css_search_uses_custom_selectors_with_arguments
         set = @xml.css('employee > address:my_filter("domestic", "Yes")', @handler)
         refute_empty(set)
@@ -145,33 +163,15 @@ def test_search_with_css_query_uses_custom_selectors_with_arguments
       end
 
       def test_search_with_xpath_query_uses_custom_selectors_with_arguments
-        set = if Nokogiri.uses_libxml?
-          @xml.search('//employee/address[my_filter(., "domestic", "Yes")]', @handler)
-        else
-          @xml.search('//employee/address[nokogiri:my_filter(., "domestic", "Yes")]', @handler)
-        end
-        refute_empty(set)
-        set.each do |node|
-          assert_equal("Yes", node["domestic"])
-        end
-      end
-
-      def test_search_with_xpath_query_using_namespaced_custom_function
-        # Note that invocation with namespaces was not historically supported in CRuby.
-        # see https://github.com/sparklemotion/nokogiri/issues/2147 for more context.
-        set = @xml.xpath('//employee/address[nokogiri:my_filter(., "domestic", "Yes")]', @handler)
+        set = @xml.search('//employee/address[nokogiri:my_filter(., "domestic", "Yes")]', @handler)
         refute_empty(set)
         set.each do |node|
           assert_equal("Yes", node["domestic"])
         end
       end
 
       def test_pass_self_to_function
-        set = if Nokogiri.uses_libxml?
-          @xml.xpath('//employee/address[my_filter(., "domestic", "Yes")]', @handler)
-        else
-          @xml.xpath('//employee/address[nokogiri:my_filter(., "domestic", "Yes")]', @handler)
-        end
+        set = @xml.xpath('//employee/address[nokogiri:my_filter(., "domestic", "Yes")]', @handler)
         refute_empty(set)
         set.each do |node|
           assert_equal("Yes", node["domestic"])
@@ -180,11 +180,7 @@ def test_pass_self_to_function
 
       def test_custom_xpath_function_gets_strings
         set = @xml.xpath("//employee")
-        if Nokogiri.uses_libxml?
-          @xml.xpath('//employee[thing("asdf")]', @handler)
-        else
-          @xml.xpath('//employee[nokogiri:thing("asdf")]', @handler)
-        end
+        @xml.xpath('//employee[nokogiri:thing("asdf")]', @handler)
         assert_equal(set.length, @handler.things.length)
         assert_equal(["asdf"] * set.length, @handler.things)
       end
@@ -246,65 +242,41 @@ def test_xpath_results_cache_should_get_cleared_on_attr_update
       end
 
       def test_custom_xpath_function_returns_string
-        result = if Nokogiri.uses_libxml?
-          @xml.xpath('thing("asdf")', @handler)
-        else
-          @xml.xpath('nokogiri:thing("asdf")', @handler)
-        end
+        result = @xml.xpath('nokogiri:thing("asdf")', @handler)
         assert_equal("asdf", result)
       end
 
       def test_custom_xpath_gets_true_booleans
         set = @xml.xpath("//employee")
-        if Nokogiri.uses_libxml?
-          @xml.xpath("//employee[thing(true())]", @handler)
-        else
-          @xml.xpath("//employee[nokogiri:thing(true())]", @handler)
-        end
+        @xml.xpath("//employee[nokogiri:thing(true())]", @handler)
         assert_equal(set.length, @handler.things.length)
         assert_equal([true] * set.length, @handler.things)
       end
 
       def test_custom_xpath_gets_false_booleans
         set = @xml.xpath("//employee")
-        if Nokogiri.uses_libxml?
-          @xml.xpath("//employee[thing(false())]", @handler)
-        else
-          @xml.xpath("//employee[nokogiri:thing(false())]", @handler)
-        end
+        @xml.xpath("//employee[nokogiri:thing(false())]", @handler)
         assert_equal(set.length, @handler.things.length)
         assert_equal([false] * set.length, @handler.things)
       end
 
       def test_custom_xpath_gets_numbers
         set = @xml.xpath("//employee")
-        if Nokogiri.uses_libxml?
-          @xml.xpath("//employee[thing(10)]", @handler)
-        else
-          @xml.xpath("//employee[nokogiri:thing(10)]", @handler)
-        end
+        @xml.xpath("//employee[nokogiri:thing(10)]", @handler)
         assert_equal(set.length, @handler.things.length)
         assert_equal([10] * set.length, @handler.things)
       end
 
       def test_custom_xpath_gets_node_sets
         set = @xml.xpath("//employee/name")
-        if Nokogiri.uses_libxml?
-          @xml.xpath("//employee[thing(name)]", @handler)
-        else
-          @xml.xpath("//employee[nokogiri:thing(name)]", @handler)
-        end
+        @xml.xpath("//employee[nokogiri:thing(name)]", @handler)
         assert_equal(set.length, @handler.things.length)
         assert_equal(set.to_a, @handler.things.flatten)
       end
 
       def test_custom_xpath_gets_node_sets_and_returns_array
         set = @xml.xpath("//employee/name")
-        if Nokogiri.uses_libxml?
-          @xml.xpath("//employee[returns_array(name)]", @handler)
-        else
-          @xml.xpath("//employee[nokogiri:returns_array(name)]", @handler)
-        end
+        @xml.xpath("//employee[nokogiri:returns_array(name)]", @handler)
         assert_equal(set.length, @handler.things.length)
         assert_equal(set.to_a, @handler.things.flatten)
       end
@@ -317,7 +289,7 @@ def awesome!; end
 
         assert(@xml.xpath("//employee/name"))
 
-        @xml.xpath("//employee[saves_node_set(name)]", @handler)
+        @xml.xpath("//employee[nokogiri:saves_node_set(name)]", @handler)
         assert_equal(@xml, @handler.things.document)
         assert_respond_to(@handler.things, :awesome!)
       end
@@ -352,7 +324,7 @@ def name_equals(nodeset, name, *args)
 
           # long list of long arguments, to apply GC pressure during
           # ruby_funcall argument marshalling
-          xpath = ["//tool[name_equals(.,'hammer'"]
+          xpath = ["//tool[nokogiri:name_equals(.,'hammer'"]
           500.times { xpath << "'unused argument #{"x" * 1000}'" }
           xpath << "'unused argument')]"
           xpath = xpath.join(",")
@@ -362,28 +334,20 @@ def name_equals(nodeset, name, *args)
       end
 
       def test_custom_xpath_without_arguments
-        value = if Nokogiri.uses_libxml?
-          @xml.xpath("value()", @handler)
-        else
-          @xml.xpath("nokogiri:value()", @handler)
-        end
+        value = @xml.xpath("nokogiri:value()", @handler)
         assert_in_delta(123.456, value)
       end
 
       def test_custom_xpath_without_arguments_returning_int
-        value = if Nokogiri.uses_libxml?
-          @xml.xpath("anint()", @handler)
-        else
-          @xml.xpath("nokogiri:anint()", @handler)
-        end
+        value = @xml.xpath("nokogiri:anint()", @handler)
         assert_equal(1230456, value)
       end
 
       def test_custom_xpath_with_bullshit_arguments
         xml = "<foo> </foo>"
         doc = Nokogiri::XML.parse(xml)
         foo = doc.xpath(
-          "//foo[bool_function(bar/baz)]",
+          "//foo[nokogiri:bool_function(bar/baz)]",
           Class.new do
             def bool_function(value)
               true
@@ -593,21 +557,21 @@ def collision(nodes)
 
         it "handles multiple handler function calls" do
           # test that jruby handles this case identically to C
-          result = @xml.xpath("//employee[thing(.)]/employeeId[another_thing(.)]", @handler)
+          result = @xml.xpath("//employee[nokogiri:thing(.)]/employeeId[nokogiri:another_thing(.)]", @handler)
           assert_equal(5, result.length)
           assert_equal(10, @handler.things.length)
         end
 
         it "doesn't get confused by an XPath function, flavor 1" do
           # test that it doesn't get confused by an XPath function
-          result = @xml.xpath("//employee[thing(.)]/employeeId[last()]", @handler)
+          result = @xml.xpath("//employee[nokogiri:thing(.)]/employeeId[last()]", @handler)
           assert_equal(5, result.length)
           assert_equal(5, @handler.things.length)
         end
 
         it "doesn't get confused by an XPath function, flavor 2" do
           # test that it doesn't get confused by an XPath function
-          result = @xml.xpath("//employee[last()]/employeeId[thing(.)]", @handler)
+          result = @xml.xpath("//employee[last()]/employeeId[nokogiri:thing(.)]", @handler)
           assert_equal(1, result.length)
           assert_equal(1, @handler.things.length)
         end