Opened 3 years ago

Last modified 3 years ago

#24470 new New feature

Serialization of base classes is not customizable for migrations

Reported by: Rocky Meza Owned by: nobody
Component: Migrations Version: 1.7
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have a base class that is created by a class factory for one of my models. You can see an example here, but basically it works like this:

def CreateBaseClass(arg):
    class cls(object):
        def get_arg(self):
            return arg

    return cls


class MyModel(CreateBaseClass('foo'), models.Model):
    # ...

When I run makemigrations, it serializes the base classes to something like this:

        migrations.CreateModel(
            name='MyModel',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
            ],
            options={},
            bases=(myapp.models.cls, models.Model),
        ),

But this errors because myapp.models.cls doesn't exist.

I think that Django should allow the base classes to customize their serialization so that the migration would end up something like this:

            bases=(myapp.models.CreateBaseClass('foo'), models.Model),

This could be done through a classmethod called deconstruct_class, and it would be used like this:

def CreateBaseClass(arg):
    class cls(object):
        @classmethod
        def deconstruct_class(cls):
            return 'myapp.models.CreateBaseClass', (arg,), {}

        # ...

    return cls

Here are some related tickets:
#23950, #22951

Thanks,

Change History (4)

comment:1 Changed 3 years ago by Simon Charette

Can't you simply define your factored mixin at the module level?

from django.db import models


def model_mixin_factory(name):
    class cls(object):
        pass
    cls.__name__ = name
    return cls

MyMixin = model_mixin_factory('MyMixin')


class MyModel(MyMixin, models.Model):
    pass

comment:2 Changed 3 years ago by Gavin Wahl

There are several problems with doing that.

  • The __module__ of MyMixin will be the __module__ of model_mixin_factory, not the module were MyMixin was created.
  • model_mixin_factory needs to remain backwards compatible. Changing the signature would break existing code, and there is existing code that uses it without assigning it to a module-level variable
  • It's ugly to specify the name twice

comment:3 Changed 3 years ago by Carl Meyer

Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

On the one hand, I think there is definitely a point where we just have to say "auto-generated migrations are on a best-effort basis, if you're doing complex things then you will probably have to edit the auto-generated migrations by hand."

On the other hand, this specific feature request looks relatively elegant and generic, and I think would probably find use cases beyond this specific situation , so provided the implementation isn't too complex in practice I think it's reasonable.

I've marked accepted, but if anyone with more migrations experience thinks there are serious problems here, feel free to reverse that!

comment:4 Changed 3 years ago by Markus Holtermann

It would be nice if we can stick to deconstruct() and not introduce deconstruct_class().

For serializing model managers we follow the pattern class MyManager(MyBaseManager.from_queryset(CustomQuerySet)): https://docs.djangoproject.com/en/1.8/topics/migrations/#model-managers which will also work here I think.

I have to think about it a bit more, though, but the general feature seems useful for more complex projects.

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