Keep track of attribute namespaces #33

ctrlcctrlv · 2022-01-25T07:58:37Z

Closes #13.

ctrlcctrlv · 2022-01-25T08:03:24Z

src/lib.rs

+trait ToAttributeMaps {
+    fn to_attribute_maps(self) -> (AttributeMap<String, String>, AttributeMap<String, Namespace>);
+}
+
+impl ToAttributeMaps for Vec<xml::attribute::OwnedAttribute> {
+    fn to_attribute_maps(self) -> (AttributeMap<String, String>, AttributeMap<String, Namespace>) {
+        let mut attr_map = AttributeMap::with_capacity(self.len());
+        let mut attr_map_ns = AttributeMap::new();
+
+        for attr in self {
+            if let Some(ns) = attr.name.prefix {
+                let xns = Namespace::from_iter([(ns, attr.name.namespace.unwrap_or(xml::namespace::NS_EMPTY_URI.to_string()))]);
+                attr_map_ns.insert(attr.name.local_name.clone(), xns);
+            }
+            attr_map.insert(attr.name.local_name, attr.value);
+        }
+
+        (attr_map, attr_map_ns)
+    }
+}
+


This is the important bit, most of the rest is tests/boilerplate to try to keep as much compatibility with the old non-newtype xmltree::Namespace as Rust will let us have.

Suggested by clippy: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports

ctrlcctrlv · 2022-01-26T20:03:59Z

The changes just pushed (9216575 – 8972930) improve code quality a tad, but don't change function or test results.

[fred@🍇葡萄🍇xmltree-rs]$ cargo test
   Compiling xmltree v0.11.0 (/home/fred/Workspace/xmltree-rs)
    Finished test [unoptimized + debuginfo] target(s) in 0.71s
     Running unittests (target/debug/deps/xmltree-c3a1a640866251c3)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/namespaces.rs (target/debug/deps/namespaces-7ccaa7bc5ab4ba2e)

running 5 tests
test test_default_ns ... ok
test test_empty_ns - should panic ... ok
test test_issue13 ... ok
test test_ns ... ok
test test_ns_rw ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/suite.rs (target/debug/deps/suite-66ee467e004ba98b)

running 17 tests
test test_decl ... ok
test test_mal_02 ... ok
test test_new ... ok
test test_mal_01 ... ok
test test_nodecl ... ok
test test_mut ... ok
test test_mal_03 ... ok
test test_no_root_node ... ok
test test_04 ... ok
test test_03 ... ok
test test_text ... ok
test test_take ... ok
test test_rw ... ok
test test_parse_all ... ok
test test_02 ... ok
test test_write_with_config ... ok
test test_01 ... ok

test result: ok. 17 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests xmltree

running 1 test
test src/lib.rs - (line 7) - compile ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

sdroege · 2022-10-12T15:25:29Z

src/lib.rs

+    /// ```xml
+    /// <mdui:DisplayName xml:lang="en">TestShib Test IdP</mdui:DisplayName>
+    /// ```
+    pub attribute_namespaces: AttributeMap<String, Namespace>,


I think a better API would be to add a new Attribute struct similar to the Element struct

pub struct Attribute { pub name: String, pub namespace: Option<String>, pub prefix Option<String>, }

and have that be stored in Element::attributes.

IIRC attributes can also be provided multiple times, so maybe Element::attributes should just be a Vec of such structs, and the struct should also include the attribute value.

And one could also add a getter function that works on predicates and finds the corresponding attribute.

I think the underlying xml library will never give us the same attribute/ns combo with the same value twice.

Hello, I hope you don't mind me chiming in on this PR but I also have need of this feature. Unfortunately the PR as it currently stands doesn't work for my use case, which is where I have two attributes with the same name but different namespaces, e.g.

<tag attribute="foo" extended:attribute="bar"/>

So Sebastian's suggestion seems like a good one, although it sounds like it would break backward compatibility if it changed the current Element::attributes member.

Uh, does that happen even in the case of:

<tag a:b="A" b:b="B"/>

?

Okay I see what's wrong. You're right @tombailo that @sdroege made a good suggestion. My not acting on it wasn't a judgment on the quality of the suggestion, my lack of action was just due to the fact the main struggle in this PR is keeping backwards compatibility, which I didn't even manage fully.

I'm going to think some more on the problem and see if I can't find some happy medium between a total API break and my broken implementation. Thanks for testing my PR!

You're welcome!

Regarding API compatibility, I can't really see how to see how to fix this without a breaking change. It's an unarguable fact that attribute names can be namespace-qualified and that names in different namespaces need to be discriminated. One option could be to use the namespace-qualified name in attributes, but that would be inconsistent with the rest of the Element API where names and namespaces are broken out and handled separately. So to me it looks like an API change is inevitable, but I have no idea how big a deal that would be.

Of course ultimately it's up to the maintainers, and I'm pretty new to Rust development, but that's my 2 cents for what it's worth.

I'm considering storing both by default with a compile-time flag to disable the duplication. Thoughts?

That would keep current users of the library happy, but would have a couple of downsides. One is some increased complexity in the code and tests, which would have to be maintained going forward. The other is that the current API and implementation could be causing bugs for current users of the library, which they just haven't found yet. If I'm parsing XML which looks like <tag ns1:attr="foo" ns2:attr="bar"/> and I only care about ns2:attr, my application will work fine until it receives XML with the attributes reversed, i.e. <tag ns2:attr="bar" ns1:attr="foo"/>.

So to me that's an argument for making a breaking change and forcing current users to make the (relatively simple) changes update their code.

But, like I say, it's not my project, and I'm pretty new to the world of Rust.

Perhaps a pragmatic way to proceed would be to put the change up for review without any compile-time backwards compatibility feature flag, and if the maintainers request such a feature flag, you can always add it.

Hope this helps and thanks for trying to fix this!

Thanks for the thoughts. I agree with you and will make a semver breaking change. It's up to @eminence whether to pull it or not. :-)

ctrlcctrlv · 2023-05-02T04:20:53Z

@eminence Don't merge until reading discussion above about two attributes w/same name from different namespaces. I didn't consider this case and more radical change is needed to handle it but since you were occupied with other matters didn't try to architect something as it's gonna have to be semver breaking.

eminence · 2023-05-02T04:25:05Z

Thanks for the heads up

Keep track of attribute namespaces

03bc802

Closes eminence#13.

ctrlcctrlv mentioned this pull request Jan 25, 2022

RFC: new YaDeserialize interface media-io/yaserde#84

Open

ctrlcctrlv commented Jan 25, 2022

View reviewed changes

ctrlcctrlv mentioned this pull request Jan 26, 2022

Attribute namespaces are not preserved #13

Open

ctrlcctrlv added 4 commits January 26, 2022 14:58

Simplify FromIterator impl

9216575

Don't import xml needlessly as we import all needed members

adb315a

Suggested by clippy: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports

Avoid allocating empty string for namespace unless necessary

ca97078

Fix reference to reference

8972930

ctrlcctrlv mentioned this pull request Jan 26, 2022

Code quality: consider splitting test suite and library up more #34

Open

sdroege reviewed Oct 12, 2022

View reviewed changes

tombailo mentioned this pull request Mar 6, 2023

Discriminate between attributes in different namespaces #38

Open

eminence self-assigned this May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep track of attribute namespaces #33

Keep track of attribute namespaces #33

ctrlcctrlv commented Jan 25, 2022

ctrlcctrlv Jan 25, 2022

ctrlcctrlv commented Jan 26, 2022

sdroege Oct 12, 2022

sdroege Oct 12, 2022

ctrlcctrlv Oct 12, 2022

tombailo Jan 1, 2023

ctrlcctrlv Jan 1, 2023

ctrlcctrlv Jan 2, 2023

tombailo Jan 2, 2023

ctrlcctrlv Jan 2, 2023

tombailo Jan 2, 2023

ctrlcctrlv Jan 2, 2023

ctrlcctrlv commented May 2, 2023 •

edited

Loading

eminence commented May 2, 2023

Keep track of attribute namespaces #33

Are you sure you want to change the base?

Keep track of attribute namespaces #33

Conversation

ctrlcctrlv commented Jan 25, 2022

Choose a reason for hiding this comment

ctrlcctrlv commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrlcctrlv commented May 2, 2023 • edited Loading

eminence commented May 2, 2023

ctrlcctrlv commented May 2, 2023 •

edited

Loading