Skip to content

collect_namespaces: Different result since 1.5.6 #885

Closed
Closed
collect_namespaces: Different result since 1.5.6#885

Description

@wolfgangw
wolfgangw
opened on Apr 9, 2013

Nokogiri::XML::Document#collect_namespaces will return different results with version 1.5.5 and versions since 1.5.6 respectively.

By the way: the sample below, while abstract and reduced, reflects real-world instance documents, not merely an academic exercise.

Consider this document with multiple default namespace scopes:

<?xml version="1.0"?>
<foo xmlns="foo_space">
  <bar>
    <barchild xmlns="bar_space">
      barchild says: I'm not always here.
      When I am I don't want to clutter
      root node's namespace definitions.
      Also I don't want to be cluttered
      myself -- no prefix for me please.
    </barchild>
  </bar>
</foo>

1.5.5:

irb> [Nokogiri::VERSION, Nokogiri::LIBXML_VERSION]
=> ["1.5.5", "2.7.8"]
irb> Nokogiri::XML(open 'multi-default-ns.xml').collect_namespaces
=> {"xmlns"=>"foo_space"}

1.5.6 and ever since:

irb> [Nokogiri::VERSION, Nokogiri::LIBXML_VERSION]
=> ["1.5.6", "2.7.8"]
irb> Nokogiri::XML(open 'multi-default-ns.xml').collect_namespaces
=> {"xmlns"=>"bar_space"}

This on ruby 2.0.0-p0. Reproducible with ruby 1.9.3-p194 / libxml 2.8.0.

$ diff nokogiri-1.5.5/lib/nokogiri/xml/document.rb nokogiri-1.5.6/lib/nokogiri/xml/document.rb
152,154c152,155
<       # Note this is a very expensive operation in current implementation, as it
<       # traverses the entire graph, and also has to bring each node across the
<       # libxml bridge into a ruby object.

---
>       # Note that this method does an xpath lookup for nodes with
>       # namespaces, and as a result the order may be dependent on the
>       # implementation of the underlying XML library.
>       #
156,158c157,160
<         ns = {}
<         traverse { |j| ns.merge!(j.namespaces) }
<         ns

---
>         xpath("//namespace::*").inject({}) do |hash, ns|
>           hash[["xmlns",ns.prefix].compact.join(":")] = ns.href if ns.prefix != "xml"
>           hash
>         end

Apart from the changed behaviour: Facing a document like the sample above (perfectly valid XML, I believe) a namespaces hash with attributes as keys won't fly, right? A list would. And break a million systems along the way. Essentially I'm looking for the proper mechanism to process these documents. Any hint appreciated.

By the way, the comment Non-prefixed default namespaces (as in “xmlns=”) are not included in the hash. on Nokogiri's::XML::Document#collect_namespaces needs updating.

Activity

  1. wolfgangw commented on Apr 23, 2013

    @wolfgangw
    wolfgangw
    on Apr 23, 2013
    ContributorAuthor

    See this real-world example with multiple default namespace declarations (Signer and Signature nodes).

  2. flavorjones commented on Apr 23, 2013

    @flavorjones
    flavorjones
    on Apr 23, 2013
    Member

    Thanks for pointing this out. As mentioned in the relevant commit (c13f9a5) Nokogiri is relying on the underlying XML library to provide the namespaces.

    In this case, it's unclear to me what the "right" behavior is, since the return value is (as you point out) a hash and both namespaces cannot be returned.

    Rather than classifying this as a bug, I think the answer is that #collect_namespaces should return an array of tuples; but I'm not going to make that kind of a breaking change without bumping the major version, so maybe an alternative method? I'm open to discussion here.

  3. wolfgangw commented on Apr 24, 2013

    @wolfgangw
    wolfgangw
    on Apr 24, 2013
    ContributorAuthor

    Yes, I don't see how you could touch #collect_namespaces without breaking a lot of dishes. Only that what's been served on those dishes is not correct for all cases. My code naively depended on 1.5.5 behaviour and fell flat on its nose after c13f9a5. Lesson learned. Interesting, though, that the change didn't raise much, if any, concern.

    Can I suggest #collect_all_namespaces (returns [[node, prefix, href], [...]]) without being flagellated to pieces?

  4. wolfgangw commented on May 8, 2013

    @wolfgangw
    wolfgangw
    on May 8, 2013
    ContributorAuthor

    Using these in my code (dcp_inspect) now: Nokogiri::XML::Document#collect_all_namespaces_href_keys and Nokogiri::XML::Document#collect_all_namespaces_prefix_keys.

  5. leejarvis commented on May 15, 2013

    @leejarvis
    leejarvis
    on May 15, 2013
    Member

    @wolfgangw #collect_all_namespaces sounds fine to me. Could you draft this up?

  6. wolfgangw commented on May 15, 2013

    @wolfgangw
    wolfgangw
    on May 15, 2013
    ContributorAuthor

    Sure, define draft up? Where and how?

  7. leejarvis commented on May 15, 2013

    @leejarvis
    leejarvis
    on May 15, 2013
    Member

    @wolfgangw If you could make the changes you propose and submit a pull request, we can continue discussion over implementation there. This issue can be closed in favour.

  8. wolfgangw
    added a commit that references this issue on Nov 13, 2013

    Add Nokogiri::XML::Document#collect_namespaces_[href_keys|prefix_keys]

    2613e70

  9. flavorjones commented on Feb 17, 2016

    @flavorjones
    flavorjones
    on Feb 17, 2016
    Member

    I'm going to close this, as it's not clear how we can fix this without changing the return value of collect_namespaces, which is sadly misdesigned given this edge case. I've captured this problem in the ROADMAP.md for future consideration.

  10. flavorjones
    closed this as completedon Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions