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

On JRuby, removing a namespaced attribute doesn't work #1299

Open
alex opened this issue Jun 8, 2015 · 11 comments
Open

On JRuby, removing a namespaced attribute doesn't work #1299

alex opened this issue Jun 8, 2015 · 11 comments

Comments

@alex
Copy link

alex commented Jun 8, 2015

This code reproduces the issue:

#!/usr/bin/env jruby

require 'nokogiri'

puts RUBY_ENGINE
doc = Nokogiri::XML(<<-EOF)
<?xml version="1.0" encoding="UTF-8"?>
<node xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/">
    <otherNode soapenv:mustUnderstand="1"></otherNode>
</node>
EOF

node = doc.at_xpath("//otherNode")
puts node.attributes
puts node.remove_attribute("mustUnderstand")
puts doc.to_s

When run under CRuby, remove_attribute will return the value "1", and doc.to_s will show the XML without the attribute. On JRuby remove_attribute returns nil, and doc.to_s shows that mustUnderstand is still present.

@alex
Copy link
Author

alex commented Jun 9, 2015

Digging in a bit more, the problem appears to be with has_attribute?, if I use node.attributes["mustUnderstand"].remove, the code works fine.

@alex
Copy link
Author

alex commented Jun 9, 2015

Reading between the lines on http://www.w3.org/2003/01/dom2-javadoc/org/w3c/dom/Element.html#hasAttribute_java.lang.String_, it looks like Element.hasAttribute doesn't support namespaced elements, and a caller must use Element.hasAttributeNS.

@yokolet
Copy link
Member

yokolet commented Jun 10, 2015

Hi, thanks for reporting!
Since this is not a complicated bug, I fixed the bug in the commit, 1be8831 . As far as I tested, the result was the same as CRuby version.

@yokolet yokolet closed this as completed Jun 10, 2015
@yokolet
Copy link
Member

yokolet commented Jun 10, 2015

Aw, actually, the commit, e1b5e34 .

@alex
Copy link
Author

alex commented Jun 10, 2015

Awesome! Thank you!

@flavorjones
Copy link
Member

Sorry, I need to revert this, as it's not apparent what the meaning of this code would be given the current implementations in both JRuby and C:

        doc = Nokogiri::XML <<-EOXML
          <?xml version="1.0" encoding="UTF-8"?>
          <root xmlns:foo="http://foo.xmlsoap.org/"
                xmlns:bar="http://bar.xmlsoap.org/">
            <node foo:myAttr="foo" bar:myAttr="bar"></node>
          </root>
        EOXML

        doc.at_xpath("//node").remove_attribute("myAttr")

And I think the right thing to do is to add a new method, analogous to Node#attribute_with_ns, which would remove an attribute by specifying the namespace: Node#remove_attribute_with_ns.

Further, I think we could reasonably allow an argument to #attribute and #remove_attribute that would allow specifying the namespace (and thereby deprecate the _with_ns flavors in a future release).

@flavorjones flavorjones reopened this Jun 15, 2015
@flavorjones
Copy link
Member

@alex as a workaround in the meantime, you can find the appropriate member of Node#attribute_nodes and call #remove on it.

@alex
Copy link
Author

alex commented Jun 15, 2015

@flavorjones Yup, that's what I'm doing node.attributes["mustUnderstand"].remove works fine

@flavorjones
Copy link
Member

@alex well, again, that happens to work because you don't have any name collisions. In the example I give above, you would perhaps not get the results you wanted. I'd recommend using Node#attribute_with_ns if there's any possible question.

@alex
Copy link
Author

alex commented Jun 16, 2015

Thanks.

alex added a commit to department-of-veterans-affairs/connect_vbms that referenced this issue Aug 3, 2015
* Work around a bug with Nokogiri on JRuby.
    * See sparklemotion/nokogiri#1299 for details.
* Add a basic unit test for remove_mustUnderstand

Close #24
@flavorjones flavorjones added this to the v1.17.0 milestone Jul 2, 2024
@flavorjones
Copy link
Member

The action item here is: write a method Node#remove_attribute_with_ns

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

3 participants