Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#23420 closed Bug (fixed)

Custom lookup transformers can't create DateTimeFields when USE_TZ=True

Reported by: Andy Chosak Owned by: nobody
Component: Database layer (models, ORM) Version: 1.7
Severity: Normal Keywords: lookup, transform, datetimefield
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you define a custom lookup transformer that converts to a DateTimeField, and USE_TZ=True, then making a query with the transform fails with a cryptic error.

Consider the case where you have a model that stores Unix timestamps (seconds since 1/1/1970) as positive integers. (Granted, Django supports DateTimeFields, but there may be other reasons why you want to store the raw timestamp.) It might be useful to define a custom lookup transform that makes use of MySQL's FROM_UNIXTIME to convert to DateTime representation when queried.

Take for example this model:

from django.db import models

class UnixTimestamp(models.Model):
    ts = models.PositiveIntegerField()

It'd be nice to be able to do something like:

from datetime import datetime, timedelta

one_week_ago = datetime.utcnow() - timedelta(days=7)
recent_ts = UnixTimestamp.objects.filter(ts__as_datetime__gt=one_week_ago)

You should be able to do this:

class PositiveIntegerDateTimeTransform(models.Transform):                           
    lookup_name = 'as_datetime'
    
    @property                                                                       
    def output_field(self):                                                         
        return models.DateTimeField()                                               
        
    def as_sql(self, qn, connection):                                               
        lhs, params = qn.compile(self.lhs)                                          
        return 'from_unixtime({})'.format(lhs), params
                         
models.PositiveIntegerField.register_lookup(PositiveIntegerDateTimeTransform)

But in practice the above produces output like this (see attached unit test):

Traceback (most recent call last):
  File "/dev/django/tests/custom_lookups/tests.py", line 253, in test_datetime_output_field
    UnixTimestamp.objects.filter(ts__as_datetime__gt=year_one),
  File "/dev/django/django/db/models/manager.py", line 80, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/dev/django/django/db/models/query.py", line 702, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "/dev/django/django/db/models/query.py", line 720, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "/dev/django/django/db/models/sql/query.py", line 1319, in add_q
    clause, require_inner = self._add_q(where_part, self.used_aliases)
  File "/dev/django/django/db/models/sql/query.py", line 1346, in _add_q
    current_negated=current_negated, connector=connector)
  File "/dev/django/django/db/models/sql/query.py", line 1218, in build_filter
    condition = self.build_lookup(lookups, col, value)
  File "/dev/django/django/db/models/sql/query.py", line 1123, in build_lookup
    return final_lookup(lhs, rhs)
  File "/dev/django/django/db/models/lookups.py", line 82, in __init__
    self.rhs = self.get_prep_lookup()
  File "/dev/django/django/db/models/lookups.py", line 85, in get_prep_lookup
    return self.lhs.output_field.get_prep_lookup(self.lookup_name, self.rhs)
  File "/dev/django/django/db/models/fields/__init__.py", line 1249, in get_prep_lookup
    return super(DateField, self).get_prep_lookup(lookup_type, value)
  File "/dev/django/django/db/models/fields/__init__.py", line 651, in get_prep_lookup
    return self.get_prep_value(value)
  File "/dev/django/django/db/models/fields/__init__.py", line 1405, in get_prep_value
    (self.model.__name__, self.name, value),    
AttributeError: 'DateTimeField' object has no attribute 'model'

This error is caused by a DateTimeField naive timezone warning (code here) assuming that the field instance is bound to a model, which it is not in this case.

3 attachments to this ticket:

  1. output_field_docs.patch : clarifies documentation of output_field in django.db.models.Transform, which is currently wrong
  2. datetime_unbound_warning.patch : modifies DateTimeField to warn about naive datetimes with unbound Field instances.
  3. datetime_custom_lookup_test.patch : test that demonstrates the above-described scenario (MySQL only)

Attachments (3)

output_field_docs.patch (1.3 KB) - added by Andy Chosak 6 years ago.
datetime_unbound_warning.patch (1.1 KB) - added by Andy Chosak 6 years ago.
datetime_custom_lookup_test.patch (2.7 KB) - added by Andy Chosak 6 years ago.

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by Andy Chosak

Attachment: output_field_docs.patch added

Changed 6 years ago by Andy Chosak

Changed 6 years ago by Andy Chosak

comment:1 Changed 6 years ago by Aymeric Augustin

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

The issue is valid and patches look very good.

I would like to avoid the \ in patch 2 and make the code more robust with this pattern:

   try:
       name = '%s.%s' % (self.model.__name__, self.name)
   except AttributeError:
       name = '(unbound)'

In patch 3, I would also suggest to:

  • rename UnixTimestamp to MySQLUnixTimestamp to make it clear that it's database-specific;
  • avoid overriding save(); creating an instance with UnixTimestamp.objects.create(ts=time.time()) in the tests will be easier to understand for future readers.

Would you like to submit a pull request on GitHub? (Add yourself in AUTHORS if you aren't there already.)

Otherwise I can take care of committing the changes.

comment:2 Changed 6 years ago by Andy Chosak

Thanks for the review and comments @aaugustin, I've applied your suggestions. All tests pass for both SQLite and MySQL.

I've created a PR at https://github.com/django/django/pull/3326.

comment:3 Changed 6 years ago by Collin Anderson

Patch needs improvement: unset

comment:4 Changed 6 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

In ceb1ffcc8da92a82338582bc6801de4bc8f05e32:

Fixed #23420 - broken warning for unbound naive datetime objects

Fixed issue with warning message displayed for unbound naive datetime
objects when USE_TZ is True. Adds unit test that demonstrates the issue
(discoverable when using a custom lookup in MySQL).

comment:5 Changed 6 years ago by Anssi Kääriäinen <akaariai@…>

In 12e5b87b89074cd0380ddcdadd24a6c621178c07:

[1.7.x] Fixed #23420 - broken warning for unbound naive datetime objects

Fixed issue with warning message displayed for unbound naive datetime
objects when USE_TZ is True. Adds unit test that demonstrates the issue
(discoverable when using a custom lookup in MySQL).

Backport of ceb1ffcc8d from master.

Conflicts:

tests/custom_lookups/tests.py

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