CybOXProject / mixbox

A library of common code leveraged by python-cybox, python-maec, and python-stix
BSD 3-Clause "New" or "Revised" License
8 stars 15 forks source link

copy.deepcopy() broken on Entity instances #19

Closed bworrell closed 9 years ago

bworrell commented 9 years ago

Calling copy.deepcopy() on an Entity instance doesn't work if it has any TypedField values set.

The deepcopy() call works, but the TypedField value resolution is broken on set fields:

>>> a = Address()
>>> a.address_value = "foobar"
>>> print a.address_value
foobar
>>> # make a deep copy
>>>copied = copy.deepcopy(a)
>>>print copied.address_value
None

This is because we made the Entity._fields dictionary keyed off of TypedField instances. The copy.deepcopy() call creates new keys and breaks the connection between the TypedField descriptor and the _fields entry.

bworrell commented 9 years ago

Fixed in https://github.com/CybOXProject/mixbox/commit/cfd8909a8e8985d7d597e408791b3289f28ac675.

I resolved this by essentially making __copy__() and __deepcopy__() just return self rather than creating a new instance. I think this is fine because TypedField is designed to expect that there will only ever be one instance of itself for a given class.

An alternative would be to control __deepcopy__ at the Entity level, but that got pretty complicated quickly, so I opted for this solution. We can reopen later if this causes problems.

gtback commented 9 years ago

Why define __deepcopy__ to do something other than actually make a copy? If it's causing unexpected behavior (and I would agree losing field values is unexpected :stuck_out_tongue: ), why not just raise an Exception in __deepcopy__. If people are calling copy.deepcopy, I imagine they expect to be able to mutate one or the other without affecting the other, which is certainly unexpected.

gtback commented 9 years ago

Nevermind. I didn't realize you defined it on TypedField, not Entity. Nothing to see here, move along :smile:

bworrell commented 9 years ago

:D