Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23473 closed Bug (fixed)

Django prompts to create new migrations when none are needed when a class is used in a field declaration

Reported by: Dan Loewenherz Owned by: Markus Holtermann
Component: Migrations Version: 1.7
Severity: Normal Keywords:
Cc: info+coding@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Dan Loewenherz)

Create a model like so:

from django.db import models
from django.utils.deconstruct import deconstructible

@deconstructible
class IgnoreFormatString(object):
    def __init__(self, s):
        self.s = s

    def __str__(self):
        return self.s

    def __mod__(self, k):
        return self

class Product(models.Model):
    name = models.CharField(max_length=32, blank=True, null=True, error_messages={'invalid': IgnoreFormatString('Err, this is invalid.')})

Create a migration, and then run it. If you run "./manage.py migrate" once again, you'll get prompted with

  Your models have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

Running makemigrations will create another migration, yet this message will continue to appear if the migration has already been applied.

Change History (10)

comment:1 by Dan Loewenherz, 10 years ago

Example project available at: https://github.com/dlo/django-migration-bug-23473

Thanks!

comment:2 by Dan Loewenherz, 10 years ago

Description: modified (diff)

comment:3 by Tim Graham, 10 years ago

I believe this is related to #22959 in that IgnoreFormatString must be comparable (add an __eq__ method). If so, we can probably add this to the documentation that should be added for that ticket.

comment:4 by Markus Holtermann, 10 years ago

Triage Stage: UnreviewedAccepted

I can confirm this behavior. I suggest to document the requirement for a __eq__ method.

comment:5 by Markus Holtermann, 10 years ago

Cc: info+coding@… added
Owner: changed from nobody to Markus Holtermann
Status: newassigned

comment:6 by Dan Loewenherz, 10 years ago

Is there any way we could also make the ./manage.py makemigrations command fail unless __eq__ is implemented? Otherwise, users might not know where to look to fix the problem.

in reply to:  6 comment:7 by Markus Holtermann, 10 years ago

Replying to dlo:

Is there any way we could also make the ./manage.py makemigrations command fail unless __eq__ is implemented? Otherwise, users might not know where to look to fix the problem.

This is rather complicated as object itself has a __eq__ method. and would require some crazy comparisons. For what I've tested, this seems to work.

Two solutions that somehow work, but that I'm not really a friend of. The if clause is really error-prone.

  1. Fails even on manage.py shell:
  • django/utils/deconstruct.py

    diff --git a/django/utils/deconstruct.py b/django/utils/deconstruct.py
    index a51a169..b0443ee 100644
    a b def deconstructible(*args, **kwargs):  
    4949        klass.__new__ = staticmethod(__new__)
    5050        klass.deconstruct = deconstruct
    5151
     52        if not hasattr(klass, '__eq__') or klass.__eq__ == object.__eq__:
     53            raise ValueError(
     54                "Cannot deconstruct class %s since no __eq__ method exists or "
     55                "__eq__ is inherited from 'object'. This method is required "
     56                "to prevent infinit migration creation"
     57                % klass.__name__)
     58
    5259        return klass
    5360
    5461    if not args:
  1. Fails only when __eq__ is called:
  • django/utils/deconstruct.py

    diff --git a/django/utils/deconstruct.py b/django/utils/deconstruct.py
    index a51a169..7c3abac 100644
    a b def deconstructible(*args, **kwargs):  
    4949        klass.__new__ = staticmethod(__new__)
    5050        klass.deconstruct = deconstruct
    5151
     52        if not hasattr(klass, '__eq__') or klass.__eq__ == object.__eq__:
     53            def __eq__(obj, other):
     54                raise ValueError(
     55                    "Cannot deconstruct class %s since no __eq__ method "
     56                    "exists or __eq__ is inherited from 'object'. This method "
     57                    "is required to prevent infinit migration creation"
     58                    % obj.__class__.__name__)
     59
     60            klass.__eq__ = __eq__
     61
    5262        return klass
    5363
    5464    if not args:
Last edited 10 years ago by Markus Holtermann (previous) (diff)

comment:8 by Markus Holtermann, 10 years ago

Has patch: set

I added a pull request with documentation updates: ​https://github.com/django/django/pull/3271

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

Resolution: fixed
Status: assignedclosed

In 066e672d79ceb416ac87e51c8fa400221fa8604f:

Fixed #23473 -- Documented that @deconstructible classes need eq.

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

In cf78a0ccc39902f73a1e1540b59c3b6c89aa51f6:

[1.7.x] Fixed #23473 -- Documented that @deconstructible classes need eq.

Backport of 066e672d79 from master

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