Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#27914 closed New feature (fixed)

makemigrations produces incorrect path for inner classes

Reported by: Serge van den Boom Owned by: Hasan Ramezani
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: anand21nanda@…, Hasan Ramezani Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When you define a subclass from django.db.models.Field as an inner class of some other class, and use this field inside a django.db.models.Model class, then when you run manage.py makemigrations, a migrations file is created which refers to the inner class as if it were a top-level class of the module it is in.

To reproduce, create the following as your model:

class Outer(object):
    class Inner(models.CharField):
        pass

class A(models.Model):
    field = Outer.Inner(max_length=20)

After running manage.py makemigrations, the generated migrations file contains the following:

migrations.CreateModel(
    name='A',
    fields=[
        ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
        ('field', test1.models.Inner(max_length=20)),
    ],
),

Note the test1.models.Inner, which should have been test1.models.Outer.Inner.

The real life case involved an EnumField from django-enumfields, defined as an inner class of a Django Model class, similar to this:

import enum
from enumfields import Enum, EnumField

class Thing(models.Model):
    @enum.unique
    class State(Enum):
        on = 'on'
        off = 'off'

    state = EnumField(enum=State)

This results in the following migrations code:

migrations.CreateModel(
    name='Thing',
    fields=[
        ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
        ('state', enumfields.fields.EnumField(enum=test1.models.State, max_length=10)),
    ],
),

This refers to test1.models.State, instead of to test1.models.Thing.State.

Change History (26)

comment:1 by Serge van den Boom, 8 years ago

Type: UncategorizedBug

comment:2 by Simon Charette, 8 years ago

Component: Database layer (models, ORM)Migrations
Triage Stage: UnreviewedAccepted
Type: BugNew feature
Version: 1.10master

This should be possible to do by relying on __qualname__ (instead of __name__) now that master is Python 3 only.

comment:3 by ChillarAnand, 8 years ago

Owner: changed from nobody to ChillarAnand
Status: newassigned

comment:4 by ChillarAnand, 8 years ago

Cc: anand21nanda@… added
Has patch: set

comment:5 by Simon Charette, 8 years ago

Patch needs improvement: set
Summary: makemigrations produces incorrect path for subclasses of Field which are inner classesmakemigrations produces incorrect path for inner classes

I think we should focus on using __qualname__ during migration serialization as well instead of simply solving the field subclasses case.

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

Resolution: fixed
Status: assignedclosed

In fb0f987:

Fixed #27914 -- Added support for nested classes in Field.deconstruct()/repr().

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

In 451b585:

Refs #27914 -- Used qualname in model operations' deconstruct().

comment:8 by Jeff Cohen, 5 years ago

I am still encountering this issue when running makemigrations on models that include a django-enumfields EnumField. From tracing through the code, I believe the Enum is getting serialized using the django.db.migrations.serializer.TypeSerializer, which still uses the __name__ rather than __qualname__. As a result, the Enum's path gets resolved to app_name.models.enum_name and the generated migration file throws an error "app_name.models has no 'enum_name' member". The correct path for the inner class should be app_name.models.model_name.enum_name.

https://github.com/django/django/blob/master/django/db/migrations/serializer.py#L266

comment:9 by ChillarAnand, 5 years ago

Resolution: fixed
Status: closednew

Reopening it. Will recheck with nested enum field.

comment:10 by Mariusz Felisiak, 5 years ago

Cc: Hasan Ramezani added

comment:11 by Hasan Ramezani, 5 years ago

Owner: changed from ChillarAnand to Hasan Ramezani
Status: newassigned

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In d3030dea:

Refs #27914 -- Moved test enum.Enum subclasses outside of WriterTests.test_serialize_enums().

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 6452112:

Refs #27914 -- Fixed serialization of nested enum.Enum classes in migrations.

comment:15 by Mariusz Felisiak, 5 years ago

Resolution: fixed
Status: assignedclosed

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 1a4db2c:

[3.0.x] Refs #27914 -- Moved test enum.Enum subclasses outside of WriterTests.test_serialize_enums().

Backport of d3030deaaa50b7814e34ef1e71f2afaf97c6bec6 from master

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 30271a47:

[3.0.x] Refs #27914 -- Fixed serialization of nested enum.Enum classes in migrations.

Backport of 6452112640081ac8838147a8ba192c45879203d8 from master

comment:18 by John Bowen, 5 years ago

Resolution: fixed
Status: closednew

commit 6452112640081ac8838147a8ba192c45879203d8 does not resolve this ticket.

The commit patched the EnumSerializer with __qualname__, which works for Enum members.
However, the serializer_factory is returning TypeSerializer for the Enum subclass, which is still using __name__

With v3.0.x introducing models.Choices, models.IntegerChoices, using nested enums will become a common pattern; serializing them properly
with __qualname__ seems prudent.

Here's a patch for the 3.0rc1 build
https://github.com/django/django/files/3879265/django_db_migrations_serializer_TypeSerializer.patch.txt

comment:19 by Mariusz Felisiak, 5 years ago

Has patch: unset
Patch needs improvement: unset

Agreed, we should fix this.

comment:20 by Hasan Ramezani, 5 years ago

I will create a patch a soon as possible.

comment:22 by John Bowen, 5 years ago

Has patch: set

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: newclosed

In 29d8198:

Fixed #27914 -- Fixed serialization of nested classes in migrations.

comment:24 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In b6cbc88f:

[3.0.x] Fixed #27914 -- Fixed serialization of nested classes in migrations.

Backport of 29d8198841ea39af44f3bc835d646e642d498475 from master

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