Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13328 closed (fixed)

Cannot pickle a queryset with filter on field with callable default datetime.datetime.now

Reported by: bkonkle Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2-beta
Severity: Keywords: pickle, queryset, datetime__lte
Cc: brandonkonkle@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by kmtracey)

We're caching with memcached, and using django.core.cache to directly set cache values. When we try to cache a queryset of blog entries with a datetime__lte filter, we get a TypeError telling us that a string or Unicode object was expected, but a NoneType was received instead. I'm attaching an abbreviated example of the model and manager we're using, and the manage.py shell steps taken to reproduce the error along with the traceback it provides.

The error is related to line 6 of the attached models.py file - if I remove the release_date__lte=datetime.datetime.now() filter, the cache.set works without an issue.

Attachments (7)

interacitve_output.txt (1.7 KB) - added by bkonkle 5 years ago.
iPython interactive interpreter output, including traceback
models.py (843 bytes) - added by bkonkle 5 years ago.
An abbreviated models.py which, when used, reproduces the error.
13328_test.diff (1.5 KB) - added by kmtracey 5 years ago.
patch_13328.diff (610 bytes) - added by vbabiy 5 years ago.
patch
patch_v2_13328.diff (1.1 KB) - added by vbabiy 5 years ago.
Second version of my patch
patch_v3_13328_with_tests.diff (3.0 KB) - added by vbabiy 5 years ago.
Thrid version of my patch
patch_v3_13328_with_tests.2.diff (3.0 KB) - added by vbabiy 5 years ago.
Thrid version of my patch

Download all attachments as: .zip

Change History (26)

Changed 5 years ago by bkonkle

iPython interactive interpreter output, including traceback

Changed 5 years ago by bkonkle

An abbreviated models.py which, when used, reproduces the error.

comment:1 Changed 5 years ago by kmtracey

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Fixed formatting. Please use preview.

comment:2 Changed 5 years ago by bkonkle

  • Cc brandonkonkle@… added

comment:3 Changed 5 years ago by kmtracey

  • Component changed from Cache system to Database layer (models, ORM)
  • Summary changed from Pickling a queryset with a datetime__lte filter causes a TypeError to Cannot pickle a queryset with filter on field with callable default datetime.datetime.now
  • Triage Stage changed from Unreviewed to Accepted

The root of the problem is that the field being filtered has a default value of datetime.datetime.now. Change in behavior was introduced with mutlidb (r11952). I'll attach a testcase that passes prior to r11952 and fails after.

Changed 5 years ago by kmtracey

comment:4 follow-up: Changed 5 years ago by kmtracey

Note the TypeError: expected string or Unicode object, NoneType found message in the attached interactive output is the cPickle (used by the cache code if available) version of plain pickle error below:

No fixtures found.
test_datetime_callable_default_all (regressiontests.queryset_pickle.tests.PickleabilityTestCase) ... ok
test_datetime_callable_default_filter (regressiontests.queryset_pickle.tests.PickleabilityTestCase) ... ERROR
test_related_field (regressiontests.queryset_pickle.tests.PickleabilityTestCase) ... ok

======================================================================
ERROR: test_datetime_callable_default_filter (regressiontests.queryset_pickle.tests.PickleabilityTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kmt/django/trunk/tests/regressiontests/queryset_pickle/tests.py", line 20, in test_datetime_callable_default_filter
    self.assert_pickles(Happening.objects.filter(when=datetime.datetime.now()))
  File "/home/kmt/django/trunk/tests/regressiontests/queryset_pickle/tests.py", line 10, in assert_pickles
    self.assertEqual(list(pickle.loads(pickle.dumps(qs))), list(qs))
  File "/usr/lib/python2.5/pickle.py", line 1366, in dumps
    Pickler(file, protocol).dump(obj)
  File "/usr/lib/python2.5/pickle.py", line 224, in dump
    self.save(obj)
  File "/usr/lib/python2.5/pickle.py", line 331, in save
    self.save_reduce(obj=obj, *rv)
  File "/usr/lib/python2.5/pickle.py", line 419, in save_reduce
    save(state)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 649, in save_dict
    self._batch_setitems(obj.iteritems())
  File "/usr/lib/python2.5/pickle.py", line 663, in _batch_setitems
    save(v)
  File "/usr/lib/python2.5/pickle.py", line 331, in save
    self.save_reduce(obj=obj, *rv)
  File "/usr/lib/python2.5/pickle.py", line 419, in save_reduce
    save(state)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 649, in save_dict
    self._batch_setitems(obj.iteritems())
  File "/usr/lib/python2.5/pickle.py", line 663, in _batch_setitems
    save(v)
  File "/usr/lib/python2.5/pickle.py", line 331, in save
    self.save_reduce(obj=obj, *rv)
  File "/usr/lib/python2.5/pickle.py", line 419, in save_reduce
    save(state)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 649, in save_dict
    self._batch_setitems(obj.iteritems())
  File "/usr/lib/python2.5/pickle.py", line 663, in _batch_setitems
    save(v)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 600, in save_list
    self._batch_appends(iter(obj))
  File "/usr/lib/python2.5/pickle.py", line 615, in _batch_appends
    save(x)
  File "/usr/lib/python2.5/pickle.py", line 331, in save
    self.save_reduce(obj=obj, *rv)
  File "/usr/lib/python2.5/pickle.py", line 419, in save_reduce
    save(state)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 649, in save_dict
    self._batch_setitems(obj.iteritems())
  File "/usr/lib/python2.5/pickle.py", line 663, in _batch_setitems
    save(v)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 600, in save_list
    self._batch_appends(iter(obj))
  File "/usr/lib/python2.5/pickle.py", line 615, in _batch_appends
    save(x)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 562, in save_tuple
    save(element)
  File "/usr/lib/python2.5/pickle.py", line 331, in save
    self.save_reduce(obj=obj, *rv)
  File "/usr/lib/python2.5/pickle.py", line 419, in save_reduce
    save(state)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 649, in save_dict
    self._batch_setitems(obj.iteritems())
  File "/usr/lib/python2.5/pickle.py", line 663, in _batch_setitems
    save(v)
  File "/usr/lib/python2.5/pickle.py", line 331, in save
    self.save_reduce(obj=obj, *rv)
  File "/usr/lib/python2.5/pickle.py", line 419, in save_reduce
    save(state)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 649, in save_dict
    self._batch_setitems(obj.iteritems())
  File "/usr/lib/python2.5/pickle.py", line 663, in _batch_setitems
    save(v)
  File "/usr/lib/python2.5/pickle.py", line 286, in save
    f(self, obj) # Call unbound method with explicit self
  File "/usr/lib/python2.5/pickle.py", line 748, in save_global
    (obj, module, name))
PicklingError: Can't pickle <built-in method now of type object at 0xb7da4500>: it's not found as __main__.now

----------------------------------------------------------------------
Ran 3 tests in 0.117s

FAILED (errors=1)

comment:5 in reply to: ↑ 4 Changed 5 years ago by bkonkle

That makes MUCH more sense now - the Unicode/string error had me thoroughly confused.

comment:6 Changed 5 years ago by vbabiy

Here is my first go at a patch for Django.

My thinking was that if you are serializing the queryset we can just remove the default off the datetime field to allow pickling the queryset.

Changed 5 years ago by vbabiy

patch

comment:7 Changed 5 years ago by vbabiy

  • Has patch set

Changed 5 years ago by vbabiy

Second version of my patch

Changed 5 years ago by vbabiy

Thrid version of my patch

Changed 5 years ago by vbabiy

Thrid version of my patch

comment:8 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [12977]) Fixed #13328 -- Added a getstate/setstate pair to fields so that callable default values aren't pickled. Thanks to bkonkle for the report, and Vitaly Babiy for the patch.

comment:9 Changed 5 years ago by patrys

  • Resolution fixed deleted
  • Status changed from closed to reopened

Actually I think [12977] and [13005] should be reverted as the current solution:

  1. violates the layer separation (fields being self-contained and not caring about the surrounding classes)
  2. does the ugly "find an older definition of myself and try to guess what the default was" dance
  3. breaks in horrible ways when fields are used by anything other than django.db.models.Model subclasses
  4. breaks apps such as transmeta (yes, I know transmeta is not exactly loved by the devs, I just mention it here for completness) that rely on being able to copy() a field in the metaclass

Class and instance methods are unpickleable in Python. Be it Django or not. Period.

What happens here is that datetime.datetime.now is a @staticmethod. You can check that datetime.datetime is an object, not a module. Python only knows how to pickle functions it can reference by a module path. Instance methods are not reachable without an instance and static methods don't know their own python path because in reality they all point to a temporary function created inside the @staticmethod decorator (well, that's how decorators work).

A real fix is to either teach Python how to pickle static methods or use a function instead of a method. Functions are pickleable and work fine:

def current_time():
    return datetime.datetime.now()

class Foo(models.Model):
    some_field = models.DateTimeField(default=current_time)

comment:10 Changed 5 years ago by russellm

I'm in complete agreement that the current solution isn't clean. The report from #13392 is another example of where the complication of a field knowing it's model can go badly wrong.

However, your solution isn't really viable. Like it or not, there is *plenty* of code out there that uses "default=datetime.now". Saying that this isn't allowed would be a *massive* backwards incompatibility.

comment:11 Changed 5 years ago by patrys

Russel:

Keep in mind it only breaks if you try to pickle it. I imagine most people don't pickle evaluated querysets.

If we really need a hack to solve it then it would be easier to detect datetime.datetime.now in the Field constructor and replace it with a reference to, say, django.db.utils.current_time that is a regular module-level function. At the same time raise a deprecation warning.

comment:12 Changed 5 years ago by patrys

To be clear:

Where I say django.db.utils.current_time I actually mean to introduce safe replacements for common unpickleable methods (maybe something like django.pickleable.today or safe_today would be better names).

comment:13 Changed 5 years ago by russellm

It isn't just evaluate querysets,though. It's *any* queryset with a callable default -- see the tests added in r12977 for an example.

On top of that, the docs clearly say that default can be a callable object, so we need to be able to handle *any* callable.

Furthermore, this is a behavior that was legal in 1.1 (due to differences in the way queries were constructed), so we need to restore it for 1.2.

comment:14 Changed 5 years ago by patrys

I assume it wouldn't be possible to handle the most common case in code (datetime.*) and document the rest as backwards-incompatible? :)

Then maybe it would be easier to not pickle the whole model at all when pickling the queryset? I'm sure it wasn't possible to pickle datetime.now or, even worse, a lambda at any point in time so maybe it would be possible to make the post-multidb queryset not pickle the model contents (the model is guaranteed to be there when unpickling).

comment:15 Changed 5 years ago by russellm

I suspect you're probably correct that the solution is to look at cleaning up the object in a query that is being pickled. My original hope was that by solving the problem a little higher up, we might be able to avoid other pickling problems that haven't been reported, but it looks like I've just made more work for myself instead /sigh... :-)

comment:16 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [13013]) Fixed #13328 -- Ensured that querysets on models with callable defaults can be pickled. No, really this time. Thanks to Alex for his help brainstorming the solution.

comment:17 Changed 5 years ago by patrys

Russel:

Thanks for fixing the main issue but I'm not sure the tests in that commit are correct or that they actually test anything related to this bug :)

comment:18 Changed 5 years ago by russellm

Patrys - If you're referring to the 'number2=1' duplication, Karen fixed that in [13014].

comment:19 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top