Handle tuples during set()/merge() instead of crashing - #205
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
merge() and view() reconstruct a copy of the source tree by walking every path with segments.set(), which assumes each intermediate container it touches can be filled in incrementally through item assignment and extend(). A tuple breaks that assumption: walk() still descends into it since it isn't a leaf, but set() has no way to grow a tuple in place, so anything with a tuple value anywhere in the tree raised AttributeError: 'tuple' object has no attribute 'extend'. set() now swaps a tuple sitting at an intermediate segment for a plain list before continuing to fill it in, and the type-hinted creator does the same when asked to build a fresh container for a segment whose source value happened to be a tuple (or another immutable container like frozenset). The reconstructed result ends up with a list wherever the source had a tuple, since there is no way to build a tuple element-by-element after the fact anyway. Fixes dpath-maintainers#189
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #189.
merge()andview()both reconstruct a copy of the source tree bywalking every path and calling
segments.set()on it, andset()assumes every intermediate container it touches can be filled in
incrementally through item assignment and
extend(). A tuple breaksthat assumption:
walk()still descends into it since a tuple isn'ta leaf, but
set()has no way to grow a tuple in place. In practicethat means a source dict with a tuple anywhere in it crashes:
There are actually two separate ways to hit this, both patched here:
set()'s main loop can find that a tuple is already sitting at thesegment it needs to descend into, left there by an earlier pass over
a shallower path in the same walk. It now swaps that tuple for an
equivalent, mutable list before continuing.
_default_creatorcan also be asked to build a brand new containerfrom a type hint that happens to be
tuple(or another immutablecontainer such as
frozenset), which used to instantiate the hintliterally. It now falls back to
dict/listfor anything thatisn't already a
MutableMapping/MutableSequence, since there's noway to fill an immutable container in afterwards regardless of where
the hint came from.
Either way, the reconstructed result ends up with a
listwhereverthe source had a
tuple, since a tuple can't be built element byelement once dpath decides it needs to add a value one segment at a
time.
I noticed while poking at this that
tests/test_segments.pyalreadydraws its property-based fixtures from two separate strategies:
random_node(which includes tuples, used for read-only walks) andrandom_mutable_node(list/dict only, used for anything that callsset()). That split looks like it was already working around thisexact gap, so I left it alone rather than widening it as part of this
fix and just added focused regression tests in
tests/test_merge.pythat reproduce the reported crash directly.
Ran the full suite locally with
nose2(82 tests, all passing) andflake8clean.