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] When copying between documents nokogiri strips attribute namespace if it differs from tag's #2228

Closed
fiddlededee opened this issue May 5, 2021 · 6 comments · Fixed by #2230

Comments

@fiddlededee
Copy link
Contributor

When copying between documents nokogiri strips attribute namespace if it differs from tag's

Here is the test that checks current (in my opionion erroneous behaviour). assert_nil should fail but it passes.

#! /usr/bin/env ruby

require 'nokogiri'

class TestNokogiriCornerCase
  describe "Nokogiri" do
    before do
      @xml_text_src = <<-TESTXML
        <p1:a xmlns:p1="ns1" xmlns:p2="ns2">
          <p1:b p2:c="d">
        </p1:a>
        TESTXML
      @xml_text_dest = <<-TESTXML
        <p1:a xmlns:p1="ns1" xmlns:p2="ns2">
        </p1:a>
        TESTXML
      @xml_doc_src = Nokogiri::XML @xml_text_src
      @xml_doc_dest = Nokogiri::XML @xml_text_dest
    end
    it "should not (probably) but loses attribute namespace" do
      ns = {'p1': "ns1", 'p2': "ns2"}
      node_dest = @xml_doc_dest.xpath("//p1:a", ns)[0]
      node_to_insert = @xml_doc_src.xpath("//p1:b", ns)[0]
      node_dest.add_child node_to_insert
      assert_nil @xml_doc_dest.xpath("/p1:a/p1:b", 
                                       ns)[0].attribute_with_ns('c', 'ns2'),
        "check attribute with ns"
      node_dest.xpath("//*/@c").each do |node|
        node.parent["p2:c"] = node.to_s
        node.remove
      end
      assert_equal @xml_doc_dest.xpath("/p1:a/p1:b", 
                                       ns)[0].attribute_with_ns('c', 'ns2').to_s, 
        "d", "check namespace restored"
    end
  end
end
@fiddlededee fiddlededee added the state/needs-triage Inbox for non-installation-related bug reports or help requests label May 5, 2021
@flavorjones
Copy link
Member

@fiddlededee Hi, thanks for opening this isue, and I'm sorry you're having problems.

I unfortunately can't reproduce what you're seeing -- the above test passes on my system, and unfortunately since you didn't answer the questions in the issue template.

Here's my setup:

$ nokogiri -v
# Nokogiri (1.11.3)
    ---
    warnings: []
    nokogiri:
      version: 1.11.3
      cppflags:
      - "-I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.3-x86_64-linux/ext/nokogiri"
      - "-I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.3-x86_64-linux/ext/nokogiri/include"
      - "-I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.3-x86_64-linux/ext/nokogiri/include/libxml2"
      ldflags: []
    ruby:
      version: 2.7.2
      platform: x86_64-linux
      gem_platform: x86_64-linux
      description: ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux]
      engine: ruby
    libxml:
      source: packaged
      precompiled: true
      patches:
      - 0001-Revert-Do-not-URI-escape-in-server-side-includes.patch
      - 0002-Remove-script-macro-support.patch
      - 0003-Update-entities-to-remove-handling-of-ssi.patch
      - 0004-libxml2.la-is-in-top_builddir.patch
      - 0005-Fix-infinite-loop-in-xmlStringLenDecodeEntities.patch
      - 0006-htmlParseComment-treat-as-if-it-closed-the-comment.patch
      - 0007-use-new-htmlParseLookupCommentEnd-to-find-comment-en.patch
      - '0008-use-glibc-strlen.patch'
      - '0009-avoid-isnan-isinf.patch'
      - 0010-parser.c-shrink-the-input-buffer-when-appropriate.patch
      - 0011-update-automake-files-for-arm64.patch
      libxml2_path: "/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.3-x86_64-linux/ext/nokogiri"
      iconv_enabled: true
      compiled: 2.9.10
      loaded: 2.9.10
    libxslt:
      source: packaged
      precompiled: true
      patches:
      - 0001-update-automake-files-for-arm64.patch
      compiled: 1.1.34
      loaded: 1.1.34
    other_libraries:
      zlib: 1.2.11

Can you please post the output from nokogiri -v on your system?

@flavorjones
Copy link
Member

Um, ok, I'm multitasking and didn't catch that this isn't a failing test.

Can you please rewrite this to be a failing test that describes the behavior you think should be exhibited?

@flavorjones flavorjones added needs/more-info and removed state/needs-triage Inbox for non-installation-related bug reports or help requests labels May 5, 2021
@fiddlededee
Copy link
Contributor Author

fiddlededee commented May 5, 2021

No problem. Nokogiri is great in all respects. The previous code just tested that my workaround works.

Still this workaround looks escessive. The following test in my opinion should pass, but it fails.

class TestAttributeNsCopied
  describe "Nokogiri" do
    before do
      @xml_text_src = <<-TESTXML
        <p1:a xmlns:p1="ns1" xmlns:p2="ns2">
          <p1:b p2:c="d">
        </p1:a>
        TESTXML
      @xml_text_dest = <<-TESTXML
        <p1:a xmlns:p1="ns1" xmlns:p2="ns2">
        </p1:a>
        TESTXML
      @xml_doc_src = Nokogiri::XML @xml_text_src
      @xml_doc_dest = Nokogiri::XML @xml_text_dest
    end
    it "should not lose attribute namespace" do
      ns = {'p1': "ns1", 'p2': "ns2"}
      node_dest = @xml_doc_dest.xpath("//p1:a", ns)[0]
      node_to_insert = @xml_doc_src.xpath("//p1:b", ns)[0]
      node_dest.add_child node_to_insert
      assert_equal @xml_doc_dest.xpath("/p1:a/p1:b", 
                                       ns)[0].attribute_with_ns('c', 'ns2').to_s, 
        "d", "check namespace remained"
    end
  end
end

My setup is

# Nokogiri (1.11.1)
    ---
    warnings: []
    nokogiri:
      version: 1.11.1
      cppflags:
      - "-I/var/lib/gems/2.7.0/gems/nokogiri-1.11.1-x86_64-linux/ext/nokogiri"
      - "-I/var/lib/gems/2.7.0/gems/nokogiri-1.11.1-x86_64-linux/ext/nokogiri/include"
      - "-I/var/lib/gems/2.7.0/gems/nokogiri-1.11.1-x86_64-linux/ext/nokogiri/include/libxml2"
    ruby:
      version: 2.7.0
      platform: x86_64-linux-gnu
      gem_platform: x86_64-linux
      description: ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-linux-gnu]
      engine: ruby
    libxml:
      source: packaged
      precompiled: true
      patches:
      - 0001-Revert-Do-not-URI-escape-in-server-side-includes.patch
      - 0002-Remove-script-macro-support.patch
      - 0003-Update-entities-to-remove-handling-of-ssi.patch
      - 0004-libxml2.la-is-in-top_builddir.patch
      - 0005-Fix-infinite-loop-in-xmlStringLenDecodeEntities.patch
      - 0006-htmlParseComment-treat-as-if-it-closed-the-comment.patch
      - 0007-use-new-htmlParseLookupCommentEnd-to-find-comment-en.patch
      - '0008-use-glibc-strlen.patch'
      - '0009-avoid-isnan-isinf.patch'
      libxml2_path: "/var/lib/gems/2.7.0/gems/nokogiri-1.11.1-x86_64-linux/ext/nokogiri"
      iconv_enabled: true
      compiled: 2.9.10
      loaded: 2.9.10
    libxslt:
      source: packaged
      precompiled: true
      patches: []
      compiled: 1.1.34
      loaded: 1.1.34
    other_libraries:
      zlib: 1.2.11

@flavorjones
Copy link
Member

@fiddlededee Thank you for the failing test -- I agree that the current behavior feels wrong. I'm going to dig into what's happening and why.

@flavorjones
Copy link
Member

@fiddlededee I believe you've found a bug in libxml2. I'm going to file a bug report upstream and suggest a fix; and I will consider including the patch in the next version of Nokogiri. Stay tuned.

flavorjones added a commit that referenced this issue May 11, 2021
Previously, the namespace was dropped from the attribute.

Fixes #2228

Co-authored-by: Nikolaj Potashnikoff <34554890+fiddlededee@users.noreply.github.com>
flavorjones added a commit that referenced this issue May 11, 2021
Previously, the namespace was dropped from the attribute.

Fixes #2228

Co-authored-by: Nikolaj Potashnikoff <34554890+fiddlededee@users.noreply.github.com>
@flavorjones
Copy link
Member

Update: this was indeed a bug in how Nokogiri was duplicating nodes to be copied to another document. See #2230 for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants