Opened 3 years ago

Closed 3 years ago

#21954 closed Bug (fixed)

1.7a1 - Migration syntax error

Reported by: caulagi Owned by: Andrew Godwin <andrew@…>
Component: Migrations Version: 1.7-alpha-1
Severity: Release blocker Keywords:
Cc: loic@…, caulagi Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Russell Keith-Magee)

I am using the following (simplified) model that doesn't migrate using the new migrations in Django 1.7a1 -

$
$ pip freeze
Django==1.7a1
argparse==1.2.1
distribute==0.6.34
wsgiref==0.1.2
$
$ cat blog/models.py
from django.db import models
from django.utils import timezone

class Blog(models.Model):

    title = models.CharField(max_length=200)
    created = models.DateTimeField(default=timezone.now())

    def __unicode__(self):
        return self.title

$
$ python manage.py makemigrations
Migrations for 'blog':
  0001_initial.py:
    - Create model Blog
$
$ python manage.py migrate
Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/core/management/__init__.py", line 427, in execute_from_command_line
    utility.execute()
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/core/management/commands/migrate.py", line 62, in handle
    executor = MigrationExecutor(connection, self.migration_progress_callback)
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/db/migrations/executor.py", line 14, in __init__
    self.loader = MigrationLoader(self.connection)
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/db/migrations/loader.py", line 48, in __init__
    self.build_graph()
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/db/migrations/loader.py", line 145, in build_graph
    self.load_disk()
  File "/home/pcaulagi/projects/test/local/lib/python2.7/site-packages/Django-1.7a1-py2.7.egg/django/db/migrations/loader.py", line 103, in load_disk
    migration_module = import_module("%s.%s" % (module_name, migration_name))
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/tmp/bar/blog/migrations/0001_initial.py", line 17
    ('created', models.DateTimeField(default=datetime.datetime(2014, 2, 4, 9, 15, 59, 685251, tzinfo=<UTC>))),

                              ^
SyntaxError: invalid syntax
$
$

We have had an initial discussion on django-users - https://groups.google.com/forum/#!topic/django-users/5ryEZd7I1t8

Change History (11)

comment:1 Changed 3 years ago by loic84

Resolution: invalid
Status: newclosed

Hi @caulagi, the problem is that you are not using a callable for the default value of created but an actual datetime object.

Try created = models.DateTimeField(default=timezone.now) instead; notice the missing parentheses after "timezone.now".

comment:2 Changed 3 years ago by Russell Keith-Magee

Resolution: invalid
Status: closednew

@loic84 - he may well be advised to use datetime.now, but that doesn't change the fact that using datetime.now() is a valid value for a default. It's an unusual value, to be sure, but it is a *valid* value. Off the top of my head, one possible use case would be to set up a membership model where memberships start Jan 1 2014, unless otherwise specified.

Last edited 3 years ago by Russell Keith-Magee (previous) (diff)

comment:3 Changed 3 years ago by Russell Keith-Magee

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:4 Changed 3 years ago by Russell Keith-Magee

Description: modified (diff)

comment:5 Changed 3 years ago by loic84

@russellm, I get your point.

We need to make a decision at how much whitelisting we want to make in MigrationWriter since each supported type is special-cased, we don't for instance support things like frozenset.

The case of "memberships start Jan 1 2014, unless otherwise specified." can already be addressed by providing a callable that returns such date.

comment:6 Changed 3 years ago by loic84

So apparently we already special-case datetime and we document it as supported, so we ought to support timezone as well.

I'm not too sure how we can "reconstruct" the instances of tzinfo provided by pytz, but that's basically what we need.

Maybe we can provide a small utility function that works as a factory?

The failing TestCase for this issue: https://github.com/loic/django/compare/ticket21954.

Last edited 3 years ago by loic84 (previous) (diff)

comment:7 Changed 3 years ago by caulagi

Our usecase is that when creating a Blog entry, most of the time, the default value is what we want. But sometimes, we want to set created to a date in the past. So in these cases, the user will override the default timezone.now that is shown in admin for Blog entry.

Like @russellm mentioned, the stumbling thing is that it is a valid value. Otherwise, the model validation should fail, rather than the migration throwing an error, IMO.

comment:8 Changed 3 years ago by loic84

Cc: loic@… added

@caulagi, migrations have limitations that normal models do not have: everything must be serializable, or more specifically, "reconstructable" (the system relies on "code generation" rather than "serialization" like pickle).

For instance, lambda are valid and pretty common in models, especially for default values, but these can't be supported by migrations; being valid for model validation doesn't automatically make something usable by migrations.

Now for the specific case of datetime, the system already goes out of its way to add support for it, but trips because of timezone support. We currently rely on the repr of datetime objects, but when a tzinfo is present, the repr isn't valid Python. The challenge is now, how to make datetime objects with tzinfo "reconstructable".

comment:9 Changed 3 years ago by caulagi

Cc: caulagi added

comment:10 Changed 3 years ago by Andrew Godwin

There's no way to make datetime objects with timezones reconstructable, unfortunately, as the spec for what can be in tzinfo is pretty sparse. The best I can do here is to either a) ignore the timezone, which seems like a bad option, or b) raise a nice error when this is detected and point out that you can use a callable.

I'm going to implement b).

comment:11 Changed 3 years ago by Andrew Godwin <andrew@…>

Owner: set to Andrew Godwin <andrew@…>
Resolution: fixed
Status: newclosed

In 97a8fd4682b27a3a9f280e661e58db4bae55590a:

Fixed #21954: Raise nice error when serializing datetimes with timezones

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