Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#27544 closed Bug (fixed)

F() Expressions updating dates in .update() field fails on SQLite

Reported by: Gary Graham Owned by: Andrew Nester
Component: Database layer (models, ORM) Version: 1.10
Severity: Normal Keywords: F()
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

Upon upgrading from 1.8 -> 1.10, I noticed that some code I had written now threw an error. Specifically, attempting to update datetime field's via F expression.

Branch with test showing regression -> https://github.com/tadgh/django/commit/c31133261c68b10525b8e3b34e6895a0c6ece4d0

Bisected to ed83881e648771d22658f21b939f66e75c499864: Fixed #23820 -- Supported per-database time zone.

Stacktrace from failure:

======================================================================
ERROR: test_F_expression_fails (timezones.test_regression.ProveRegression)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tadgh/Projects/django/tests/timezones/test_regression.py", line 22, in test_F_expression_fails
    self.timestamp.refresh_from_db()
  File "/home/tadgh/Projects/django/django/db/models/base.py", line 585, in refresh_from_db
    db_instance = db_instance_qs.get()
  File "/home/tadgh/Projects/django/django/db/models/query.py", line 381, in get
    num = len(clone)
  File "/home/tadgh/Projects/django/django/db/models/query.py", line 240, in __len__
    self._fetch_all()
  File "/home/tadgh/Projects/django/django/db/models/query.py", line 1061, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/tadgh/Projects/django/django/db/models/query.py", line 68, in __iter__
    for row in compiler.results_iter(results):
  File "/home/tadgh/Projects/django/django/db/models/sql/compiler.py", line 806, in results_iter
    row = self.apply_converters(row, converters)
  File "/home/tadgh/Projects/django/django/db/models/sql/compiler.py", line 790, in apply_converters
    value = converter(value, expression, self.connection, self.query.context)
  File "/home/tadgh/Projects/django/django/db/backends/sqlite3/operations.py", line 159, in convert_datetimefield_value
    value = timezone.make_aware(value, self.connection.timezone)
  File "/home/tadgh/Projects/django/django/utils/timezone.py", line 364, in make_aware
    return timezone.localize(value, is_dst=is_dst)
  File "/home/tadgh/.venvs/django_env/lib/python3.4/site-packages/pytz/__init__.py", line 227, in localize
    raise ValueError('Not naive datetime (tzinfo is already set)')
ValueError: Not naive datetime (tzinfo is already set)

Change History (12)

comment:1 Changed 4 years ago by Tim Graham

Component: UncategorizedDatabase layer (models, ORM)
Description: modified (diff)

comment:2 Changed 4 years ago by Tim Graham

Summary: F() Expressions updating dates in .update() field failsF() Expressions updating dates in .update() field fails on SQLite
Triage Stage: UnreviewedAccepted

The regression seems limited to SQLite. If you could provide a patch, we can backport to 1.10. Thanks.

comment:3 in reply to:  2 Changed 4 years ago by Gary Graham

Replying to Tim Graham:

The regression seems limited to SQLite. If you could provide a patch, we can backport to 1.10. Thanks.

OK, but I have no clue where to even start looking. I will start tracing, but if you could give me a reasonable starting point, I would appreciate it. The first time I ever opened the Django repo was to submit this bug.

comment:4 Changed 4 years ago by Tim Graham

In the sqlite3/operations.py file referenced in the traceback, it might be enough to check the datetime using django.utils.timezone.is_aware() and skip the timezone.make_aware() call if appropriate.

comment:5 Changed 4 years ago by Andrew Nester

Owner: changed from nobody to Andrew Nester
Status: newassigned

I added pull request for this issue PR

comment:6 Changed 4 years ago by Claude Paroz

Has patch: set

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In ade52ef7:

Fixed #27544 -- Fixed QuerySet.update(dt=F('dt') + timedelta) crash on SQLite.

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

In 75de55f1:

[1.10.x] Fixed #27544 -- Fixed QuerySet.update(dt=F('dt') + timedelta) crash on SQLite.

Backport of ade52ef71f04e57e217585358cb289098260e3ec from master

comment:9 in reply to:  8 Changed 4 years ago by Florian Klink

I also got hit by this one. I backported the commit to the stable/1.9.x branch, you can find my PR here.

Replying to Tim Graham <timograham@…>:

In 75de55f1:

[1.10.x] Fixed #27544 -- Fixed QuerySet.update(dt=F('dt') + timedelta) crash on SQLite.

Backport of ade52ef71f04e57e217585358cb289098260e3ec from master

comment:10 Changed 4 years ago by Florian Klink

Resolution: fixed
Status: closednew
Version: 1.101.9

comment:11 Changed 4 years ago by Florian Klink

Resolution: wontfix
Status: newclosed

The PR got rejected:

As per our supported versions policy, 1.9 only receives security support now (and will be unsupported in April upon the release of Django 1.11).

So I close this here again.

comment:12 Changed 4 years ago by Simon Charette

Resolution: wontfixfixed
Version: 1.91.10
Note: See TracTickets for help on using tickets.
Back to Top