gintas / django-picklefield

A pickled object field for Django
MIT License
180 stars 47 forks source link

mutable default value causes issues #34

Closed tisdall closed 5 years ago

tisdall commented 6 years ago

There seems to be issues if a mutable default value is used. I'm not sure the best way to handle this, though.

Example:

class MyModel(models.Model):
    pickle = PickledObjectField(default={})
x = MyModel()
x.pickle['a'] = 3
x.pickle  #  {'a': 3}

y = MyModel()
y.pickle   # {'a': 3}
tisdall commented 6 years ago

to clarify.. I know that the fix for the above code is to have default=dict, but I'm not sure how this library should handle the general case of providing a mutable value as a default. Maybe the default should be stored into a pickle and then get_default returns an unpickled version (thus no longer providing a reference to the original and then making sure it's not modified).

charettes commented 6 years ago

Hey @tisdall, thanks for the report!

What I suggest we do is add a picklefield.0001 system check like it was done for django.contrib.postgres.JSONField and ArrayField:

https://github.com/django/django/commit/f6e1789654e82bac08cead5a2d2a9132f6403f52

tisdall commented 6 years ago

I had also thought about requiring callables but then I thought of something like this:

x = {}

class MyModel(models.Model):
    pickle = PickledObjectField(default=lambda: x)

In that example the default is still a mutable that's shared between calls. I guess the only way to protect against that is wrapping everything in a method that makes a deep copy (ex unpickle(pickle())) but that would be detrimental when it's not needed.

The JSONField solution seems like a good compromise (and non-breaking change). Maybe a note in the docs too about avoiding mutable defaults?