Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26605 closed Bug (fixed)

Abstract model inheriting concrete model crashes migrations

Reported by: Sébastien Diemer Owned by: Ingo Klöcker
Component: Migrations Version: 1.8
Severity: Normal Keywords:
Cc: diemersebastien@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Sébastien Diemer)

When using Model inheritance with an abstract model in between, it can happen that the _default_manager of the actual child class is None. In this case the ModelState.from_model function fails with the following error:

django/django/db/migrations/state.py", line 439, in from_model
    default_manager_name = force_text(model._default_manager.name)
AttributeError: 'NoneType' object has no attribute 'name'

The following modified test reproduces the error:

    @override_settings(TEST_SWAPPABLE_MODEL='migrations.SomeFakeModel')
    def test_create_swappable(self):
        """
        Tests making a ProjectState from an Apps with a swappable model
        """
        new_apps = Apps(['migrations'])

        class Base(models.Model):
            pass

        class AbstractAuthor(Base):
            class Meta:
                abstract = True

        class Author(AbstractAuthor):
            name = models.CharField(max_length=255)
            bio = models.TextField()
            age = models.IntegerField(blank=True, null=True)

            class Meta(AbstractAuthor.Meta):
                app_label = 'migrations'
                apps = new_apps
                swappable = 'TEST_SWAPPABLE_MODEL'

        author_state = ModelState.from_model(Author)
        self.assertEqual(author_state.app_label, 'migrations')
        self.assertEqual(author_state.name, 'Author')
        self.assertEqual([x for x, y in author_state.fields], ['id', 'name', 'bio', 'age'])
        self.assertEqual(author_state.fields[1][1].max_length, 255)
        self.assertEqual(author_state.fields[2][1].null, False)
        self.assertEqual(author_state.fields[3][1].null, True)
        self.assertEqual(author_state.options, {'abstract': False, 'swappable': 'TEST_SWAPPABLE_MODEL'})
        self.assertEqual(author_state.bases, (models.Model, ))
        self.assertEqual(author_state.managers, [])

Changing the following line in ModelState.from_model

        if hasattr(model, "_default_manager"):

to

       if getattr(model, "_default_manager", None):

is probably sufficient to fix the bug.

See also PR https://github.com/django/django/pull/6847

Change History (12)

comment:1 Changed 4 years ago by Sébastien Diemer

Type: UncategorizedBug

comment:2 Changed 4 years ago by Sébastien Diemer

Cc: diemersebastien@… added

comment:3 Changed 4 years ago by Tim Graham

Needs tests: set
Summary: Model _default_manager can be NoneAbstract model inheriting concrete model crashes migrations
Triage Stage: UnreviewedAccepted

There's an abstract model inheriting a concrete one in tests/model_inheritance_regress/models.py so this seems to be a supported use case. A regression test might go in tests/migrations/test_state.py.

comment:4 Changed 4 years ago by Akshesh Doshi

Owner: changed from nobody to Akshesh Doshi
Status: newassigned
Version: 1.8master

comment:5 Changed 4 years ago by Simon Charette

Version: master1.8

As noted on the PR this bug that has been fixed on the master branch (and 1.10.x) by the manager state refactor.

Based on our policy I don't think this warrant a backport to 1.9.x and 1.8.x but adding the test to the suite wouldn't hurt.

comment:6 Changed 4 years ago by Sébastien Diemer

Description: modified (diff)

comment:7 Changed 4 years ago by Akshesh Doshi

Owner: Akshesh Doshi deleted
Status: assignednew

comment:8 Changed 4 years ago by Ingo Klöcker

Owner: set to Ingo Klöcker
Status: newassigned

comment:9 Changed 4 years ago by Ingo Klöcker

Added PR with the test from the original PR.

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

In 67b2b1f1:

Refs #26605 -- Added migrations state test for a swappable model inheriting an abstract model inheriting concrete model.

Thanks Sébastien Diemer for the report and test.

comment:11 Changed 4 years ago by Tim Graham

Resolution: fixed
Status: assignedclosed

comment:12 Changed 4 years ago by GitHub <noreply@…>

In eb9a3bd:

Refs #26605 -- Isolated a migrations state test.

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