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

[blueyed]

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.

[blueyed]

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?!

[blueyed]

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]

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?!)

[apollo13]

Not sure either, lets see what @andrewgodwin says

[andrewgodwin]

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?

[blueyed]

@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)).

[andrewgodwin]

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.

[blueyed]

@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

[andrewgodwin]

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

[apollo13]

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

[blueyed]

Yes, this was fixed in #207.