Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22351 closed Cleanup/optimization (fixed)

Remove suggestion of using lambdas in model field arguments (incompatible with migrations)

Reported by: dimyur27@… Owned by: nobody
Component: Documentation Version: dev
Severity: Normal Keywords: migrations lambdas
Cc: Stefan Kögl, niwi@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In Django's model fields there are a lot of places where lambdas are the most obvious way to get things done.
Many keyword arguments can accept callable objects which do not supposed to do complex logic, but some simple things.
E.g. ForeignKey.limit_choices_to, FileField.upload_to...

Example from Django Docs:
limit_choices_to = lambda: {'pub_datelte': datetime.date.utcnow()}

But Django's migrations cannot cope with lambdas passed as keyword arguments, so that fact either should be mentioned in the docs (let examples use named functions rather than lambdas) or community has to put extra attention on migrations+lambdas problem.

Thanks for the magnificent framework! My best regards.

Attachments (1)

22351.diff (1.6 KB ) - added by Tim Graham 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Agreed this should at least be mentioned in docs.

comment:2 by chris cauley, 10 years ago

This is a similar problem to tickets #22373 and #22436 and loosely related to a few others. In #22436 we discovered that if we do limit_choices_to=module.some_function, migrations will break in the event that module.some_function is deleted, changed, renamed, or moved. This makes migrations not future safe. I'm pretty new to django migrations, but I know with south the overall goal was to have previous migrations not rely on the contemporary state of models.py for this very reason.

Is the limit_choices_to function even used in migrations? If we make the Field.deconstruct() method return a function that raises a NotImplementedError when called then it wouldn't mind the lambda. It would also be future safe in the event that the function provided is renamed or changed in a way that no longer reflects the code. I'll make a patch to reflect this change.

comment:3 by chris cauley, 10 years ago

I tried to reproduce this specific issue and I don't get any error when running python manage.py makemigrations APP_NAME with a lambda in limit_choices_to. That kwarg is not returned in ForeignKey.deconstruct(), so the migration inspector doesn't care if it is a lambda. Can you show me code to illustrate what you mean?

comment:4 by dimyur27@…, 10 years ago

When instead of get_user_picture_path there was a lambda, it refused to do "makemigrations".

class MyModel(models.Model):

    picture = models.ImageField(
        upload_to=get_user_picture_path,
        blank=True,
    )

comment:5 by chris cauley, 10 years ago

I thought you specifically were having a problem with limit_choices_to. That makes more sense. upload_to is addressed in #22436 where SomeClass.some_method breaks in the same way. The solution there will fix lambdas as well.

comment:6 by dimyur27@…, 10 years ago

I just wanted the docs to mention about these migration nuances when describing such kwargs of the fields. To prevent the newbies (as I am) getting into trouble.

comment:7 by Stefan Kögl, 10 years ago

Cc: Stefan Kögl added

comment:8 by Andrei Antoukh, 10 years ago

Cc: niwi@… added

It also happens with simple charfield and "default" keyword argument like this:

token = models.CharField(max_length=255, blank=True, unique=True, db_index=True,
                         default=lambda: str(uuid.uuid1()))

This is a traceback:

Traceback (most recent call last):
  File "manage.py", line 8, in <module>
    execute_from_command_line(sys.argv)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/core/management/commands/makemigrations.py", line 115, in handle
    self.write_migration_files(changes)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/core/management/commands/makemigrations.py", line 143, in write_migration_files
    migration_string = writer.as_string()
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/db/migrations/writer.py", line 124, in as_string
    operation_string, operation_imports = OperationWriter(operation).serialize()
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/db/migrations/writer.py", line 76, in serialize
    arg_string, arg_imports = MigrationWriter.serialize(item)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/db/migrations/writer.py", line 229, in serialize
    item_string, item_imports = cls.serialize(item)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/db/migrations/writer.py", line 290, in serialize
    return cls.serialize_deconstructed(path, args, kwargs)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/db/migrations/writer.py", line 205, in serialize_deconstructed
    arg_string, arg_imports = cls.serialize(arg)
  File "/home/niwi/.virtualenvs/table/lib/python3.4/site-packages/Django-1.7b4-py3.4.egg/django/db/migrations/writer.py", line 303, in serialize
    raise ValueError("Cannot serialize function: lambda")
ValueError: Cannot serialize function: lambda

Django version: 0cabf3a (2014-06-21) revision from git repository.

Without default keyword with lambda, everything works ok.

comment:9 by Tim Graham, 10 years ago

Component: MigrationsDocumentation
Easy pickings: set
Summary: New django migrations and places where lambdas are supposedRemove suggestion of using lambdas in model field arguments (incompatible with migrations)

The fact that Django cannot serialize lambdas was added in [6bbb8200].

There are a couple places where we document the use of a lambda which should be cleaned up:

ref/models/fields.txt:a dictionary as the default, use a ``lambda`` as follows::
ref/models/fields.txt:    contact_info = JSONField("ContactInfo", default=lambda:{"email": "to1@example.com"})
ref/models/fields.txt:        limit_choices_to = lambda: {'pub_date__lte': datetime.date.utcnow()}

comment:10 by Tim Graham, 10 years ago

Has patch: set

comment:11 by Claude Paroz, 10 years ago

You have a backtick left after "function". That aside, I think that adding a quick note about lambdas being problematic with migrations wouldn't hurt (with a link to migration-serializing), at least in the default section.

by Tim Graham, 10 years ago

Attachment: 22351.diff added

comment:12 by Claude Paroz, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 5ebf03b7dd2d1c215f5c0b725083d36379a7ac5b:

Fixed #22351 -- Removed usage of lambdas in model field options.

Thanks claudep for review.

comment:14 by Tim Graham <timograham@…>, 10 years ago

In 9673013abe4cd64183bf55d89adfa04927e3b947:

[1.7.x] Fixed #22351 -- Removed usage of lambdas in model field options.

Thanks claudep for review.

Backport of 5ebf03b7dd from master

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