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

[bug] explore removing JRuby implementation's namespace cache #2543

Open
flavorjones opened this issue May 11, 2022 · 0 comments
Open

[bug] explore removing JRuby implementation's namespace cache #2543

flavorjones opened this issue May 11, 2022 · 0 comments

Comments

@flavorjones
Copy link
Member

Please describe the bug

It's not clear to me why we're using a namespace cache in the JRuby implementation. It is causing problems in edge cases like #1247 that I think would be better solved by looking up the node's parent chain for an identical namespace definition a) when it is added, and b) when the node is reparented.

This is what we do in the CRuby implementation. It's more complicated (and likely still has some bugs) but it is obviously more correct in these edge cases.

An example taken from #1247 is:

  it "handles multiple instances of a namespace definition" do
    # this describes behavior that is broken in JRuby related to the namespace cache and should be fixed
    doc = Nokogiri::XML::Document.parse("<root>")
    child1 = doc.create_element("a", "xmlns:foo" => "http://nokogiri.org/ns/foo")
    assert_equal(1, child1.namespace_definitions.length)
    child1.namespace_definitions.first.tap do |ns|
      assert_equal("foo", ns.prefix)
      assert_equal("http://nokogiri.org/ns/foo", ns.href)
    end

    child2 = doc.create_element("b", "xmlns:foo" => "http://nokogiri.org/ns/foo")
    pending_if("broken in JRuby related to the namespace cache and should be fixed", Nokogiri.jruby?) do
      assert_equal(1, child2.namespace_definitions.length)
      child2.namespace_definitions.first.tap do |ns|
        assert_equal("foo", ns.prefix)
        assert_equal("http://nokogiri.org/ns/foo", ns.href)
      end
    end
  end

In summary, the second node does not have a namespace definition in the JRuby implementation. This is clearly wrong.

A partial fix would be to reimplement Node#namespace_definitions to look at the attributes for a namespace definition:

  @JRubyMethod
  public RubyArray<?>
  namespace_definitions(ThreadContext context)
  {
    // TODO: I think this implementation would be better but there are edge cases
    RubyArray<?> nsdefs = RubyArray.newArray(context.getRuntime());
    NamedNodeMap attrs = node.getAttributes();
    for (int j = 0 ; j < attrs.getLength() ; j++) {
      Attr attr = (Attr)attrs.item(j);
      if ("http://www.w3.org/2000/xmlns/" == attr.getNamespaceURI()) {
        nsdefs.append(XmlNamespace.createFromAttr(context.getRuntime(), attr));
      }
    }
    return nsdefs;
  }

but we also need to then implement something like xml_node.c:relink_namespace in order to make this match the CRuby behavior.

@flavorjones flavorjones added state/needs-triage Inbox for non-installation-related bug reports or help requests platform/jruby topic/namespaces and removed state/needs-triage Inbox for non-installation-related bug reports or help requests labels May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant