Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #28188 - fixed model field pickling. #8481

Closed

Conversation

Projects
None yet
3 participants
@adamalton
Copy link
Contributor

commented May 9, 2017

Fixed #28188 - fixed model field pickling.
Fixed issue where model field instances with non-callable defaults could not be pickled.

Also added comments to the related `django.utils.functional.cached_property` decorator to explain magic.
@timgraham
Copy link
Member

left a comment

A release note in docs/releases/1.11.2.txt should also be added. The usual process is to send a PR to master and let the committer backport, but this is okay.

@adamchainz, any comments?

@@ -32,6 +32,8 @@ def __init__(self, func, name=None):
def __get__(self, instance, cls=None):
if instance is None:
return self
# This calls the function and then assigns the returned value in place

This comment has been minimized.

Copy link
@timgraham

timgraham May 10, 2017

Member

I wouldn't put this mostly unrelated comment in the PR.

@@ -76,6 +77,13 @@ def test_field_ordering(self):
self.assertIsNotNone(f1)
self.assertNotIn(f2, (None, 1, ''))

def test_field_instance_is_pickleable(self):

This comment has been minimized.

Copy link
@timgraham

timgraham May 10, 2017

Member

picklable

def test_field_instance_is_pickleable(self):
"""Field instances can be pickled."""
field = models.Field(max_length=100, default="a string")
# Access the default value, this is where sneaky lambdas like to hide

This comment has been minimized.

Copy link
@timgraham

timgraham May 10, 2017

Member

# Field is picklable with this cached property populated (#28188).

This comment has been minimized.

Copy link
@adamchainz

adamchainz May 10, 2017

Member

sneaky indeed!

@@ -76,6 +77,13 @@ def test_field_ordering(self):
self.assertIsNotNone(f1)
self.assertNotIn(f2, (None, 1, ''))

def test_field_instance_is_pickleable(self):
"""Field instances can be pickled."""
field = models.Field(max_length=100, default="a string")

This comment has been minimized.

Copy link
@timgraham

timgraham May 10, 2017

Member

use single quotes

@@ -775,17 +771,21 @@ def get_default(self):
"""
Returns the default value for this field.
"""
return self._get_default()
# As much logic as possible is cached inside the _get_default method,

This comment has been minimized.

Copy link
@timgraham

timgraham May 10, 2017

Member

A tentative tweaking of this comment might be:

# As much logic as possible is cached inside _get_default(), but to
# avoid requiring _get_default() to return unpicklable lambdas, allow
# the return value to be noncallable.
@adamchainz
Copy link
Member

left a comment

What's the use case for pickling a field?

We could fix this another way by simply clearing the cached property when pickling. I presume pickling a model field is rare in the wild since this is the first instance in Django's test suite, so leaving the performance optimization in would be the greatest good

def test_field_instance_is_pickleable(self):
"""Field instances can be pickled."""
field = models.Field(max_length=100, default="a string")
# Access the default value, this is where sneaky lambdas like to hide

This comment has been minimized.

Copy link
@adamchainz

adamchainz May 10, 2017

Member

sneaky indeed!

@timgraham

This comment has been minimized.

Copy link
Member

commented May 11, 2017

I implemented Adam J's suggested approach in #8489.

@timgraham timgraham closed this May 11, 2017

@adamalton

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2017

Thanks for the feedback @timgraham and @adamchainz. Even though it wasn't merged, the comments were still useful.

@adamchainz

This comment has been minimized.

Copy link
Member

commented May 14, 2017

@adamalton what is your use case for pickling fields?

@adamalton

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2017

We're pickling fields in Djangae's migrations implementation. Essentially we're pickling the migration operations in order to run them in background tasks.

I would normally have tried to just work around it, but I saw that there had been other Django tickets fixed in the past for when things weren't pickle-able which led me to believe that making sure things can be pickled is probably the intended approach, hence the ticket & PR.

@adamchainz

This comment has been minimized.

Copy link
Member

commented May 14, 2017

Ah right, interesting approach.

I don't think there's any guarantee that model fields can be picklable and nor was it necessary intended. As I wrote on Tim's follow-up PR, users could easily make a field unpicklable by passing an unpicklable validator or any number of other things, and there are no checks for that. Perhaps worth adding some system checks to Djangae.

@adamalton

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Ah yes, that's a good point. I'll add a check into Djangae. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.