Fix proposal for parse_xml in case of unique child: #849
Conversation
|
Hi @ScriptToddler, thank you for this PR! Can you add a test case that demonstrates the bug this PR fixes? |
|
Also, this will need a changelog fragment, is this a breaking change? |
It is breaking in that it changes the result but I think it is a bug since I'm not sure who would want the old behavior so can just be marked as a fix. |
|
Small bump on this @ScriptToddler . Just wondering what you think of the above. |
- Modify single node map insert behaviour -> process_node instead of recurse - Add tests for unique child situation
ScriptToddler
force-pushed
the
fix/parse_xml_unique_child_text
branch
from
99a7a2e
to
af7963f
Compare
Hi, @jszwedko On my part, I consider it should be a fix, as you said. @pront As for the tests, I did include two new unit tests in the parse_xml.rs file : if_no_sibling and if_no_sibling2, demonstrating that it seems to now work as intended, not adding the unwanted "text" keys. If that does not suffice, I'm not sure what is expected then. Finally, I must confess I mostly wanted to point out the problem and offer what appeared to me a quick win fix. For my needs, I have found a workaround so I'm not bothered anymore by this behaviour, and I don't intend to spend more time on this. If this PR does not meet your requirements, it can be thrown away, no problem on my part. Regards, |
|
Hi again, Adding to my previous comment, I just realized my branch is now conflicting since commit ef07d02 introduced a refactoring of the function with the logic now in a separate crate of parsing functions. So basically the proposed change should now be applied to that new file https://github.com/vectordotdev/vrl/blob/main/src/parsing/xml.rs#L174 Regards |
|
Hi @ScriptToddler, thanks again for raising this issue and for submitting a fix. If you don't intend to spend more time on this PR, no worries at all. Leaving it open as a draft for someone else to take over. |
|
I applied the fix to the latest main and added a changelog. |
Related to #844