avoid recursion depth error for label with many references

lindsay-stevens commented 2 years ago

Closes #444

The root cause was the call to copy.deepcopy. The child argument is a minidom Node type which has a reference back to it's parentNode. The parent in turn has references to it's childNodes. So a deepcopy would try to traverse this recursive structure and error out. A copy of the parentNode is unnecessary anyway, since the copy is being made in order to append to a new parent.

Why is this the best possible solution? Were any other approaches considered?

The updated call to child.cloneNode seems to be the appropriate minidom API method for making a copy of a Node (docs). It doesn't copy the parentNode, thus avoiding the recursion issue. It's not clear from the pyxform git history why this wasn't used earlier. The method seems to have been available for at least 15 years. Maybe there was a bug in the implementation or usage in prior Python versions.

Another approach could be to use the child's __dict__ or __slots__ to filter out parentNode and copy everything else. But that may not work without making new DOM objects for the copied attributes. Then at that point this strategy essentially becomes the same as what cloneNode does.

What are the regression risks?

Should not be any. In case there is an unknown edge/use case that requires a recursive copy, the cloneNode call requests a deep copy of childNodes. As noted in the new inline comment, there did not seem to be any tests which involved copying childNodes which themselves had childNodes.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Probably not. The original bug report was perhaps an extreme use case.

Before submitting this PR, please make sure you have:

[x] included test cases for core behavior and edge cases in tests_v1
[x] run nosetests and verified all tests pass
[x] run black pyxform to format code
[x] verified that any code or assets from external sources are properly credited in comments

lindsay-stevens commented 2 years ago

The formencode commit eda825d was cherry-picked from #548 so that the CI tests can pass.

lindsay-stevens commented 2 years ago

Hi @yanokwa or @lognaturel would this be OK to merge?

XLSForm / pyxform