Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26475 closed Bug (fixed)

Using functools.partial in model field options causes creation of unnecessary migration on every 'makemigrations' call

Reported by: un.def Owned by: nobody
Component: Migrations Version: 1.9
Severity: Normal Keywords:
Cc: Markus Holtermann Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider this simple model:

from django.db import models

from functools import partial


def concat(*args):
    return ''.join(args)


class TestModel(models.Model):

    test_field = models.CharField(
        max_length=128,
        default=partial(concat, 'foo', 'bar')
    )

Every time you run ./manage.py makemigration it leads to creation of migration:

$ ./manage.py makemigrations
Migrations for 'test_app':
  0008_auto_....py:
    - Alter field test_field on testmodel

$ ./manage.py makemigrations
Migrations for 'test_app':
  0009_auto_....py:
    - Alter field test_field on testmodel

$ ./manage.py makemigrations
Migrations for 'test_app':
  0010_auto_....py:
    - Alter field test_field on testmodel

All created migrations are equal.

Cause: when project state is rendered from previous migration file, functools.partial instance is created (migrations.AlterField(... default=functools.partial(djtest_app.models.concat, *('foo', 'bar'), **{}), ...)). After that each model field is deconstructed to dict and previous and current field states (i.e. dicts) are compared https://github.com/django/django/blob/2cd2d188516475ddf256e6267cd82c495fb5c430/django/db/migrations/autodetector.py#L862
But different instances of functools.partial with same arguments are not equal — I guess that partial doesn't implement own __eq__/__ne__ methods, so comparison is made by object id() (memory address), and false field changes is detected.

Possible solution (workaround): we can use own partial subclass in migration files. Our subclass implements custom __eq__/__ne__ magic methods:

class Partial(functools.partial):
    
    def __eq__(self, other):
        return (self.func == other.func and
                self.args == other.args and
                self.keywords == other.keywords)
                
    def __ne__(self, other):
        return not self.__eq__(other)

Change History (8)

comment:1 by Dylan Herman, 8 years ago

partial is not a python class to subclass; it's a function: see [implementation] :https://github.com/python/cpython/blob/master/Lib/functools.py#236

Maybe:

class Partial(object):

     def __init__(self,partialFct):
          self.func = partialFct
     def __call__(self,*args,**kwargs):
         return self.func(*args,**kwargs)

     def __eq__(self,other):
         return(  self.func.func == other.func.func and
                       self.func.args == other.func.args and
                       self.func.keywords == other.func.keywords)
     def __ne__(self,other):
          return not self.__eq__(other)

Usage:
 def concat(*args):
       return ''.join(args)

class TestModel(models.Model):
      test_field = models.CharField(max_length =128, default = Partial(partial(concat, 'foo', 'bar')))


Just an idea. Tested with python, but not tested with Django.

Last edited 8 years ago by Dylan Herman (previous) (diff)

comment:2 by Tim Graham, 8 years ago

Cc: Markus Holtermann added

I guess it was a mistake to add serialization support for functools.partial in #25185 when the rest of the migration process doesn't totally work. Perhaps we should revert that in favor of a wrapper class similar to what's suggested above and that also has a deconstruct() method?

comment:3 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Markus Holtermann, 8 years ago

Has patch: set
Needs documentation: set
Patch needs improvement: set

Tim, I think we should backport this to 1.9, given the half-implemented partial support in #25185.

comment:5 by Matthew Schinckel, 8 years ago

I've created a PR for this issue: https://github.com/django/django/pull/6470

comment:6 by Tim Graham, 8 years ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:7 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 5402f3ab:

Fixed #26475 -- Added functools.partial() support to migrations autodetector.

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

In 799e81e:

[1.9.x] Fixed #26475 -- Added functools.partial() support to migrations autodetector.

Backport of 5402f3ab09413a571fd9d3aa27f6c76ec42ff891 from master

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