Opened 15 months ago

Closed 12 months ago

Last modified 12 months 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: stefankoegl, 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 timo 12 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 15 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

Agreed this should at least be mentioned in docs.

comment:2 Changed 15 months ago by chriscauley

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 15 months ago by chriscauley

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 15 months 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 15 months ago by chriscauley

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 15 months 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 13 months ago by stefankoegl

  • Cc stefankoegl added

comment:8 Changed 12 months ago by niwi

  • 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 12 months ago by timo

  • Component changed from Migrations to Documentation
  • Easy pickings set
  • Summary changed from New django migrations and places where lambdas are supposed to Remove 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 12 months ago by timo

  • Has patch set

comment:11 Changed 12 months ago by claudep

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 12 months ago by timo

comment:12 Changed 12 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 12 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5ebf03b7dd2d1c215f5c0b725083d36379a7ac5b:

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

Thanks claudep for review.

comment:14 Changed 12 months 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