search

instant-labs / instant-xml

11 stars 3 forks source link

really fix scalar enum namespace handling #61

Closed wez closed 1 month ago

wez commented 2 months ago

My previous commit 05de91af021c9e3574ad0d8880063e13c8a7b6dc wasn't quite right: It is actually important to consider the field name when provided, otherwise we can incorrectly match fields with different names!

I don't have a simple test case for this, but it manifested as fields within https://github.com/wez/wez-sonos/blob/3c077dbd3aadbd0ca440b1af1b5ea00899e51421/src/didl.rs#L179-L199 incorrectly matching the wrong elements and resulted in non-sensical parse errors.

wez commented 2 months ago

https://github.com/wez/wez-sonos/blob/3c077dbd3aadbd0ca440b1af1b5ea00899e51421/src/didl.rs#L321-L389 is my test case downstream; the "DJ Borchy" element was being tried as ObjectClass instead of Creator

djc commented 2 months ago

Nice catch! I think we'll definitely want a regression test for this.

wez commented 1 month ago

OK, test added. I tried to minimize the xml and set of structs, but it does need to be reasonably complex to trigger the problem case, so we'll have to settle for a middle ground.

djc commented 1 month ago

Can you describe the intuition/conceptual issue behind what this is trying to fix? And do we really need all those fields in the test type?

wez commented 1 month ago

I spent a significant amount of time reducing the test case from the downstream scenario to what you see in this PR. It may be possible to reduce further, but I just can't afford to spend the time on that.

The problem that is being fixed is described in https://github.com/instant-labs/instant-xml/pull/61#issuecomment-2131282964. The mechanism for the error is that the proc macro generates a sequence of FromXml::matches calls in the same order as the fields are declared in the struct, but the sequence of Node::Opens matches the input xml document. The faulty matches from my earlier commit could result in passing the node to the wrong element deserializer and produce a runtime error as a result.

djc commented 1 month ago

I tried to reduce the test case down a little (while verifying that it still failed without the fix) to make sure I understood why the fix worked, merged this in #63. Thanks!