Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#24513 closed Bug (fixed)

"... has no field named None" with M2MField when migrating

Reported by: Christopher Schäpers Owned by: nobody
Component: Migrations Version: 1.8
Severity: Release blocker Keywords: migrations, bug
Cc: Florian Apolloner 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

In Django 1.8c1 migrations from scratch don't work for me anymore, this effectively prevents testing.

(When you see project I'm talking about an app, btw)

See file traceback.txt for a test-call and a backtrace.

See file 0011_auto_20140914_2032.py for the migration that failed.

And see file user_model.py for the model that should be migrated.

---

I can't provide much code sadly. I tried to simplify my big project, but I can't get it done without disclosing half of my product (or it would take me days to do so …).

Instead I did a git-bisect to get the commit that makes my migrations fail:

(testenv)kondou:django/ (4e9ecfe) $ git bisect good
a1ba4627931591b80afa46e38e261f354151d91a is the first bad commit
commit a1ba4627931591b80afa46e38e261f354151d91a
Author: Markus Holtermann <info@markusholtermann.eu>
Date:   Mon Feb 9 01:11:25 2015 +0100

    [1.8.x] Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state
    
    Instead of naively reloading only directly related models (FK, O2O, M2M
    relationship) the project state needs to reload their relations as well
    as the model changes as well. Furthermore inheriting models (and super
    models) need to be reloaded in order to keep inherited fields in sync.
    
    To prevent endless recursive calls an iterative approach is taken.
    
    Backport of b29f3b51204d53c1c8745966476543d068c173a2 from master

:040000 040000 2faa54b906f309ddb353a73abbef7db950b18d1c 2eb05be062be72e8a8f4d54961fbe3ec96d0edf1 M      django

Doing a git revert a1ba4627931591b80afa46e38e261f354151d91a on 1.8c1 tag indeed fixes this problem for me.

Attachments (5)

traceback.txt (5.9 KB ) - added by Christopher Schäpers 9 years ago.
Traceback
0011_auto_20140914_2032.py (1.4 KB ) - added by Christopher Schäpers 9 years ago.
Migration
user_model.py (4.3 KB ) - added by Christopher Schäpers 9 years ago.
Model that should be migrated
diff.txt (714 bytes ) - added by Christopher Schäpers 9 years ago.
Diff that created migration 0011…
testproject.zip (5.4 KB ) - added by Christopher Schäpers 9 years ago.
Project that reproduces this error. Do ./manage.py test a few times

Download all attachments as: .zip

Change History (32)

by Christopher Schäpers, 9 years ago

Attachment: traceback.txt added

Traceback

by Christopher Schäpers, 9 years ago

Attachment: 0011_auto_20140914_2032.py added

Migration

by Christopher Schäpers, 9 years ago

Attachment: user_model.py added

Model that should be migrated

comment:1 by Tim Graham, 9 years ago

Can you to backtrack the exception and try to figure out why the get_field(None) call is happening?

File "/home/kondou/testenv/lib/python3.4/site-packages/django/db/models/options.py", line 552, in get_field
    return self.fields_map[field_name]
	KeyError: None

comment:2 by Christopher Schäpers, 9 years ago

In django.db.backends.base.schema.py#L268 the call to get_field() is done, where

fields==(None, 'group'),
model._meta.unique_together==((None, 'group'),),
model==project.user__groups

The call to this is done at django.db.backends.sqlite3.schema.py#L137 where temp_model==<class '__fake__.User_groups'>

I also have a Groups model that refers to User. But there is no unique together constraint in the whole file:

class Groups(models.Model):
    owner = models.ForeignKey(settings.AUTH_USER_MODEL, related_name='owner')
    members = models.ManyToManyField(settings.AUTH_USER_MODEL, related_name='members')
    name = models.CharField(max_length=50, unique=True, validators=[RegexValidator(regex=r'^[\w.@+-]+$')])
    displayname = models.CharField(max_length=150, blank=True)
    description = models.CharField(max_length=5000, blank=True)
    default_moderators = models.ManyToManyField(settings.AUTH_USER_MODEL, related_name="group_default_moderators")

Edit: All this info is included in traceback though … Have a look at the part under KeyError: None

Last edited 9 years ago by Christopher Schäpers (previous) (diff)

comment:3 by Karl Hobley, 9 years ago

I've so far been unable to reproduce this issue. It looks like it's failing when running that AlterField operation on the "groups" field.

Could you paste the code for the user model before this migration so we can see what this operation is changing?

by Christopher Schäpers, 9 years ago

Attachment: diff.txt added

Diff that created migration 0011...

comment:4 by Christopher Schäpers, 9 years ago

I attached the diff that created the migration.

Weird thing is, I can't reproduce this either. If I start a fresh app with all my models in I get a different migration than a squashed on from my app that produces the bug. Using this created initial migration as a "squashed" migration even works as a workaround …

I'm doing a git-bisect on my project with django1.8c1 now, to see what exactly introduced the bug in my project.

comment:5 by Christopher Schäpers, 9 years ago

The git-bisect on my project confirmed that the combination on migration 0011... and diff.txt introduced the bug for me.

Another strange sidenote: This bug may be caching related … Sometimes (~80%) I get it, sometimes I don't …

comment:6 by Markus Holtermann, 9 years ago

I think the unique_together constraint is part of the m2m through table which is automatically created by Django (which is <class '__fake__.User_groups'>). The odd thing, the field None should be user (FK from the m2m through table to your user model table). I have no idea yet why or where the name resolution fails / the field cache isn't cleared.

comment:7 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

I guess we can tentatively accept the ticket, but the inability to reproduce it reliably may make solving it somewhat difficult.

comment:8 by Christopher Schäpers, 9 years ago

Today I ran into this error again, while changing the related_name of a M2M to self with symmetrical=False, and could finally create a project that is able to reproduce this.

You can reproduce it by just running ./manage.py test. Be minded though that (about 50% now) sometimes the migration actually works and no exception is thrown (probably caching related).
Edit: I probably get it that often, because my User model has 4 self, symmetrical=False entries.

Last edited 9 years ago by Christopher Schäpers (previous) (diff)

by Christopher Schäpers, 9 years ago

Attachment: testproject.zip added

Project that reproduces this error. Do ./manage.py test a few times

comment:9 by Patryk Zawadzki, 9 years ago

I believe I have tracked it down further:

In django/db/backends/sqlite3/schema.py line 234 (_alter_many_to_many) we have:

override_uniques=(new_field.m2m_field_name(), new_field.m2m_reverse_field_name())

However new_field.m2m_field_name() returns None.

new_field.m2m_field_name() is a partial that evaluates to self._get_m2m_attr(related, 'name') where related is a class the relation is pointing to.

The implementation lives in django/db/models/fields/related.py line 2470 (_get_m2m_attr):

for f in self.rel.through._meta.fields:
    if (f.is_relation and f.rel.to == related.related_model and
            (link_field_name is None or link_field_name == f.name)):

However at this point f.rel.to and related point to two different classes that are not considered equal. This could be caused by the fact that the operation is an AlterField and it creates two states of the affected class—one before and one after the migration—even though all of the changes happen in a separate m2m model.

Last edited 9 years ago by Patryk Zawadzki (previous) (diff)

comment:10 by Patryk Zawadzki, 9 years ago

Not sure whether I'm on the right track but in django/db/migrations/state.py in ProjectState.reload_model the following happens:

try:
    old_model = self.apps.get_model(app_label, model_name)
except LookupError:
    related_models = set()
else:
    # Get all relations to and from the old model before reloading,
    # as _meta.apps may change
    related_models = get_related_models_recursive(old_model)

The resulting set correctly contains all related m2m models. Later however this code is run:

for rel_app_label, rel_model_name in related_models:
    try:
        model_state = self.models[rel_app_label, rel_model_name]
    except KeyError:
        pass
    else:
        states_to_be_rendered.append(model_state)

This lookup fails for all related m2m models which causes them not to appear in the to-be-rerendered state list. This in turn results in the m2m models pointing to the old (previously rendered) version of the altered class.

I can trigger this problem quite reliably by having the migration first touch any non-m2m field of the affected class tree (which would cause all of the related models to be rendered) and then alter an m2m field (which fails m2m name lookups due to the relations pointing to out of date models).

Last edited 9 years ago by Patryk Zawadzki (previous) (diff)

comment:11 by Patryk Zawadzki, 9 years ago

Side note:

Tracing the cause of this became much easier after I've changed _get_m2m_attr() to end with a raise AttributeError(). There does not seem to be a case when it silently returning None in case of a missing attribute match is deliberate and expected.

Last edited 9 years ago by Patryk Zawadzki (previous) (diff)

comment:12 by Florian Apolloner, 9 years ago

Cc: Florian Apolloner added

comment:13 by Patryk Zawadzki, 9 years ago

I have found the reason of the crash.

It turns that if a model has a relation to itself, ProjectState.reload_model will place it twice on the states_to_be_rendered list. This causes all relations to point to the registered version of the model (and later cause problems where they are unable to find their reverse relationship which leads to the field name being set to None).

comment:14 by Markus Holtermann, 9 years ago

Severity: NormalRelease blocker
Version: 1.8rc11.8

comment:16 by Markus Holtermann, 9 years ago

Has patch: set
Needs documentation: set
Needs tests: set

comment:17 by George Karakostas, 9 years ago

Hi, FYI I have been experiencing the same issue. I was wondering if it would be helpful to let you know of more details in here, open a new ticket or test the patch.

comment:18 by Markus Holtermann, 9 years ago

Hey Wtower, if you can try out patrys' patch and leave a comment on the pull request with your results it'd be much appreciated.

comment:19 by Patryk Zawadzki, 9 years ago

Needs tests: unset

comment:20 by George Karakostas, 9 years ago

The patch seems to work fine for me, thanks!

comment:21 by George Karakostas, 9 years ago

I was eager to speak, I'm afraid. For some reason the tests do not seem to fail every time. Seems so strange. I got all details if interested.

comment:22 by Markus Holtermann, 9 years ago

You're saying the patch doesn't fully work? In that case, can you check "Patch needs improvement" and "Needs tests" on this ticket here and share your details, please

comment:23 by Markus Holtermann, 9 years ago

Patch needs improvement: set

comment:24 by George Karakostas, 9 years ago

The second patch seems to consistently work even after performing several different tests.

comment:25 by Markus Holtermann, 9 years ago

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

comment:26 by Markus Holtermann <info@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 0385dad:

Fixed #24513 -- Made sure a model is only rendered once during reloads

This also prevents state modifications from corrupting previous states.
Previously, when a model defining a relation was unregistered first,
clearing the cache would cause its related models' _meta to be cleared
and would result in the old models losing track of their relations.

comment:27 by Markus Holtermann <info@…>, 9 years ago

In 9f632dc:

[1.8.x] Fixed #24513 -- Made sure a model is only rendered once during reloads

This also prevents state modifications from corrupting previous states.
Previously, when a model defining a relation was unregistered first,
clearing the cache would cause its related models' _meta to be cleared
and would result in the old models losing track of their relations.

Backport of 0385dad073270c37f8c4a5f13edce43f2a69ba8a from master

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