Skip to content

Stop using QDomDocument::toString to avoid QHash random reordering #46

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

Merged
merged 1 commit into from
Oct 26, 2019

Conversation

glpuga
Copy link
Contributor

@glpuga glpuga commented Oct 26, 2019

This PR fixes a random reordering of keys in the xml output file that happens each time the file is saved from a new Groot process, even if no changes were made to the tree.

The issue can be replicated by creating a tree description xml file that has nodes with multiple input or output ports (the test_data/test_xml_key_reordering_issue.xml included in this PR, for instance). Open Groot, load the file and save a copy of it. Close and start Groot again, open the file again a save a second copy. Then compare the original with both copies using diff; there should be differences in the ordering of xml attributes.

This becomes an issue when the xml file is stored in a version control system such as git.

The random reordering happens because newer versions of the QDomDocument uses QHash, which in newer versions of the Qt library uses randomness to prevent certain types of DoS attacks.

There are provisions to prevent the random reordering from within the Qt library, but they are not the same in all versions of Qt.

This PR is based on an alternative solution posted here, which generates the xml contents recursively generating elements for each node.

This patch also adds a test for this fix.

@facontidavide
Copy link
Contributor

when I saw that you included the unit test... I wanted to cry of happiness!

Thanks for this nice PR! i am testing it on my machine and merge it ASAP.

@glpuga glpuga marked this pull request as ready for review October 26, 2019 18:22
@facontidavide facontidavide merged commit 416bd6c into BehaviorTree:master Oct 26, 2019
@glpuga glpuga deleted the pr/orderedxmlkeys branch October 26, 2019 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants