[Django] #26475: Using functools.partial in model field options causes creation of unnecessary migration on every 'makemigrations' call
Django
unnecessary migration on every 'makemigrations' call
----------------------------+--------------------
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26475>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by oneTimePad):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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. Not tested with Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:1>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: MarkusH (added)
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:2>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:3>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
Tim, I think we should backport this to 1.9, given the half-implemented
partial support in #25185.
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:4>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by schinckel):
I've created a PR for this issue:
https://github.com/django/django/pull/6470
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:5>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:6>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5402f3ab09413a571fd9d3aa27f6c76ec42ff891" 5402f3ab]:
{{{
#!CommitTicketReference repository=""
revision="5402f3ab09413a571fd9d3aa27f6c76ec42ff891"
Fixed #26475 -- Added functools.partial() support to migrations
autodetector.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:7>
Django
unnecessary migration on every 'makemigrations' call
Reporter: un-def | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"799e81ef6bc8cbb944dd3594a2646a594a04b44d" 799e81e]:
{{{
#!CommitTicketReference repository=""
revision="799e81ef6bc8cbb944dd3594a2646a594a04b44d"
[1.9.x] Fixed #26475 -- Added functools.partial() support to migrations
autodetector.
Backport of 5402f3ab09413a571fd9d3aa27f6c76ec42ff891 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26475#comment:8>