Opened 3 years ago

Closed 2 years ago

Last modified 2 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: master
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 2 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 2 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Agreed this should at least be mentioned in docs.

comment:2 Changed 2 years ago by chris cauley

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 Changed 2 years ago by chris cauley

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 Changed 2 years ago by dimyur27@…

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 Changed 2 years ago by chris cauley

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 Changed 2 years ago by dimyur27@…

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 Changed 2 years ago by Stefan Kögl

Cc: Stefan Kögl added

comment:8 Changed 2 years ago by Andrei Antoukh

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 Changed 2 years ago by Tim Graham

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 Changed 2 years ago by Tim Graham

Has patch: set

comment:11 Changed 2 years ago by Claude Paroz

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.

Changed 2 years ago by Tim Graham

Attachment: 22351.diff added

comment:12 Changed 2 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:13 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 5ebf03b7dd2d1c215f5c0b725083d36379a7ac5b:

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

Thanks claudep for review.

comment:14 Changed 2 years ago by Tim Graham <timograham@…>

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