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

parse_xml function : text key added when it should not for single nodes #844

Open
ScriptToddler opened this issue May 18, 2024 · 2 comments
Labels
type: bug A code related bug vrl: stdlib Changes to the standard library

Comments

@ScriptToddler
Copy link

Hello,

Problem

There appears to be an inconsistency with text_key in parse_xml function

Whenever a chain of an "even" number of single child is present, the text key seems to be forcibly added even when it should not.
E.g. :

Investigation (might be mistaken)

I tried to investigate the issue and it seems to me the problem stems from the recurse implementation with single nodes, and precisely this line

Value::Object(recurse(node)),

When recursing the value of the node which would then incorrectly add the text key later on at this step (if I am not mistaken) :
for n in node.children().filter(|n| n.is_element() || n.is_text()) {

The problem seems to be fixed on my end if instead of the recurse, the process_node function is instead called on the node, with no regression seen so far.
I have proposed such a fix on a branch on my forked version of the project there https://github.com/vectordotdev/vrl/compare/main...ScriptToddler:vrl:fix/parse_xml_unique_child_text?expand=1 along with tests highlighting the issue.

I may be mistaken, in which case you may just discard what I said in this last part

Thank you,

@jszwedko jszwedko added type: bug A code related bug vrl: stdlib Changes to the standard library labels May 20, 2024
@jszwedko
Copy link
Member

Thanks for the thorough report @ScriptToddler ! I haven't reviewed the code changes, but the output for your example inputs definitely highlights the issue. We can review the code change if you want to open a PR.

@ScriptToddler
Copy link
Author

Hi @jszwedko,
As suggested I opened a PR (#849) with my proposal, but then again not sure if it is the best way to fix the issue (but it does work for me with no regression seen thus far)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A code related bug vrl: stdlib Changes to the standard library
Projects
None yet
Development

No branches or pull requests

2 participants