ArrayWrapper types not unpacked on __init__
Closed this issue · 9 comments
ArrayWrapper's __init__() method checks whether it is initialized with an ArrayWrapper instance, however it does so after checking for instances of collections.Sequence. Since isinstance(x, collections.Sequence) is True if x is an instance of ArrayWrapper this renders the check for ArrayWrapper instances useless:
>>> import python_jsonschema_objects.wrapper_types as pjo_wt
>>> x=pjo_wt.ArrayWrapper([])
>>> import collections
>>> isinstance(x, collections.Sequence)
True
>>> y=pjo_wt.ArrayWrapper(x)
>>> type(y.data)
<class 'python_jsonschema_objects.wrapper_types.ArrayWrapper'>To prevent this issue, the __init__() method should simply check for instances of ArrayWrapper first before checking for collection.Sequence.
@steinymity I fixed this by just removing that check entirely. I think it's actually better to just re-use the ArrayWrapper init logic than to try and shortcircuit, given that it implements collections.Sequence anyway.
Also, the way you're initializing ArrayWrapper objects in that example is incorrect. They need to be created using ArrayWrapper.create to create a subclass that does the object specific logic.
What made you file this bug? While strictly speaking incorrect, there's no way this ever caused a problem, and the example is contrived (not a way you should ever use ArrayWrapper). It's the type of bug that makes me think it was generated from a linter not from use.
Well, actually I extended the library a bit (using monkey patching) to support detecting changes of data structures. I.e., functions can register callbacks that are called when a new value is assigned to an attribute or added to an array.
However, I noticed that sometimes I got more callbacks than expected and tracked that down to the fact that an ArrayWrapper contained another ArrayWrapper, so the insert into the outer and into the inner caused a trigger each. I then looked at the code and noticed the wrong order of the checks causing the nesting.
In case you are interested in such a trigger system to observe the data structure for changes, I'd be happy to share my code — I was just unsure whether it would be of general use/need beyond my application.
Btw, this nesting was caused by a call to validate() which is a bit unfortunate since I'd expect validate() not to change the data structure.
Removing the check entirely is a bit unfortunate since receiving extraneous triggers causes unnecessary extra load in my application (which I'd like to avoid, obviously).
Oh interesting. Then I can see how my fix doesn’t actually help because it still nests. A more correct fix would check for ArrayWrapper then unroll it’s values instead of nesting.
I think the validate call is unavoidable. Otherwise it would be possible to assign a list of a different type just because it was an ArrayWrapper.
I’d be interested in seeing the hook stuff if you’re game to put up an MR with it and it’s not too complicated.
Yeah, I agree, the validate() call should stay inside. Right — a simple check for ArrayWrapper and unroll would really help here, like before c44198f.
Sure — I'm happy to prepare an MR on the trigger stuff (might take a day or two).
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Don't be stale. Gotta turn that guy off
Stale issue message
closed again :/