Missing "to" in `deconstruction` / `KeyError: 'to'` on second `migrate` run (Django 1.7, via django/db/migrations/autodetector.py) #206

Closed
blueyed opened this Issue Mar 12, 2014 · 11 comments

Projects

None yet

3 participants

Contributor
blueyed commented Mar 12, 2014

I am getting a KeyError exception, when running manage.py migrate a second time on a Django 1.7 project (fresh DB, but migrated from 1.6).

It seems like Django expects a key "to" in the field's deconstructor, which is not provided by the deconstructor

    def _rel_agnostic_fields_def(fields):
        """
        Return a definition of the fields that ignores field names and
        what related fields actually relate to.
        """
        fields_def = []
        for name, field in fields:
            deconstruction = field.deconstruct()[1:]
            if field.rel and field.rel.to:
                del deconstruction[2]['to']

When inspection deconstruction in case of the exception, it looks like this:

(u'taggit.managers.TaggableManager', [], {u'help_text': <django.utils.functional.proxy object at 0x3816810>, u'verbose_name': <django.utils.functional.proxy object at 0x38167d0>, u'blank': True})

I am using the following to define the tags field on my model:

tags = TaggableManager(
    verbose_name=_("Tags"),
    help_text=_("Keywords / Tags"),
    blank=True,
)

The initial mange.py migrate worked, and migrate -l shows:
taggit
[X] 0001_initial

I am not sure, if this is an issue with Django or django-taggit.

Using Django master, and django-taggit master.

Contributor
blueyed commented Mar 13, 2014

Here is the full backtrace:

% manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: djsupervisor, autocomplete_light, reversion, sessions, admin, grappelli, sites, auth, compressor, contenttypes, django_extensions, template_debug, sekizai, django_pdb, crispy_forms
  Apply all migrations: contenttypes, test_duration, taggit
Synchronizing apps without migrations:
  Creating tables...
  Installing custom SQL...
  Installing indexes...
Running migrations:
  No migrations needed.
Traceback (most recent call last):
  File "/home/user/.virtualenvs/tmm/bin/manage.py", line 25, in <module>
    execute_from_command_line(sys.argv)
  File "…/django-master/django/core/management/__init__.py", line 427, in execute_from_command_line
    utility.execute()
  File "…/django-master/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "…/django-master/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "…/django-master/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File "…/django-master/django/core/management/commands/migrate.py", line 140, in handle
    changes = autodetector.changes(graph=executor.loader.graph)
  File "…/django-master/django/db/migrations/autodetector.py", line 33, in changes
    changes = self._detect_changes()
  File "…/django-master/django/db/migrations/autodetector.py", line 84, in _detect_changes
    model_fields_def = _rel_agnostic_fields_def(model_state.fields)
  File "…/django-master/django/db/migrations/autodetector.py", line 74, in _rel_agnostic_fields_def
    del deconstruction[2]['to']
KeyError: 'to'

It smells like a bug in Django, assuming that if there's rel.to with a field, the deconstruction would have the kwarg to?!

Contributor
blueyed commented Mar 13, 2014

Reported in Django at: https://code.djangoproject.com/ticket/22263.
I will provide a patch there, and think that it's not an issue with django-taggit anymore.

@blueyed blueyed closed this Mar 13, 2014
Contributor
blueyed commented Mar 13, 2014

Hmm, while thinking about it: maybe it's taggit responsibility to return the to kwarg via deconstruct, after it manually sets it?
(Not that it's really relevant in this case, where it's only being deleted, but it's probably relevant in other places then?!)

@blueyed blueyed reopened this Mar 13, 2014
@blueyed blueyed added a commit to blueyed/django that referenced this issue Mar 13, 2014
@blueyed blueyed Fix KeyError in _rel_agnostic_fields_ref for uncommon `rel.to` setup
django-taggit manually sets up `rel.to` and therefore there is no "to"
kwarg.

Fixes #22263
Ref: alex/django-taggit#206
8208b07
Contributor

Not sure either, lets see what @andrewgodwin says

Yes, the autodetector assumes that if you inherit from ForeignKey or ManyToMany you have a "to", as that's a basic way of how those fields work and is needed to work out dependencies and things. If you don't need "to", you're not a relation and shouldn't inherit from them?

Contributor
blueyed commented Mar 13, 2014

@andrewgodwin
"rel.to" is being used, but the TaggableManager sets up rel (https://github.com/alex/django-taggit/blob/develop/taggit/managers.py#L79) and rel.to manually (https://github.com/alex/django-taggit/blob/develop/taggit/managers.py#L134) - not via init's arguments.

It doesn't seem like it needs "to" in the deconstruction to be re-constructed correctly again (assuming that deconstruct gets fixed (see #207)).

Right, but there's a certain amount of assumption for the deconstruction of relations specifically - all other fields you can do whatever you like with, but for relations we need to be able to work out where they're pointing without a full model lying around.

My suggestion would be that deconstruct correctly sets "to" if the manager has been in and done the right stuff, and leaves it out otherwise; and that init accepts a "to" argument but ignores it. That should be enough, as the deconstructed forms are usually from full models' fields.

Contributor
blueyed commented Mar 20, 2014

@andrewgodwin
The correct approach would be probably the following (used in post_through_setup (https://github.com/blueyed/django-taggit/blob/deconstruct-keep-non-defaults/taggit/managers.py#L141)):

kwargs['to'] = self.through._meta.get_field("tag").rel.to

When I've tried it first, it caused some kind of recursion, but I could not reproduce it currently and other circumstances might have been involved.
Does this look reasonable? blueyed@6e9043a

That looks pretty reasonable to me. You should probably try and reproduce the recursion and try to guard against it, though...

Contributor

This got merged via another branch it seems, closing here -- @blueyed please reopen if I missed something.

@apollo13 apollo13 closed this Apr 20, 2014
Contributor
blueyed commented Apr 20, 2014

Yes, this was fixed in #207.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment