Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#23822 closed New feature (fixed)

Serialize model managers in migrations

Reported by: Markus Holtermann Owned by: Markus Holtermann
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: 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

Right now it's impossible to use e.g. the UserManager in a RunPython operation, thus User.objects.create_superuser() isn't available. We should serialize the model managers into migrations too. I suggest to do it the same way as with callable argument values on fields like upload_to: the manager need to stay around as long as there is a migration referencing it.

Change History (16)

comment:1 Changed 6 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

My understanding is that this has been discussed offline with Andrew and accepted.

comment:2 Changed 6 years ago by Shai Berger

I don't understand how this is supposed to work. As a trivial example -- suppose your (custom) user model changes over time, and grows a new mandatory field. This requires you to change your UserManager.create_superuser() accordingly. For the migration to still work, you need to serialize the actual manager code -- but you clearly do not intend this to be the case, and rightly so.

No, I think that if you need to call some code in a migration, then this code needs to be made available to the migration, but requiring the manager to be compatible with all past migrations still around makes very little sense to me.

comment:3 Changed 6 years ago by Markus Holtermann

Hi Shai,

the managers will be serialized in the same way as e.g. functions for field arguments are serialized: imported and referenced. A migration for a custom user model will thus look like

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import models, migrations
import myapp.models


class Migration(migrations.Migration):

    operations = [
        migrations.CreateModel(
            name='MyUser',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
                ('password', models.CharField(max_length=128, verbose_name='password')),
                ('email', models.EmailField(max_length=255, verbose_name='email address', blank=True, unique=True)),
            ],
            options={
                'verbose_name': 'user',
                'verbose_name_plural': 'users',
            },
            managers=[
                ('objects', myapp.models.MyUserManager()),
            ],
        ),
    ]

The managers argument keeps track of all managers of a model unless only the default manager (objects = models.Manager()) is available. The argument will be empty / not given in those situations.

A current work-in-progress implementation is available in my migration-managers branch.

comment:4 Changed 6 years ago by Carl Meyer

I had the same concern as Shai with this ticket. Anything that is imported in migration files adopts some new, rather stringent, backwards-compatibility requirements. It can never move, go away, or change in backwards-incompatible ways, as long as the migration(s) referencing it are still around. And getting rid of old migrations is not easy, especially for reusable third-party projects (see the lengthy discussion over on #23861).

But I can also see some cases where a simple custom manager is fundamental to a model's behavior, and less likely to change. (For one example, I think of custom managers I've implemented in the past for soft-delete). I can certainly see the attractiveness of automatically having this manager in migrations as well.

One option would be to make this behavior available, but opt-in via something like a use_in_migrations = True attribute on the manager, roughly parallel to the existing use_for_related_fields. That way we wouldn't be forcing anyone into these new back-compat restrictions on custom managers, but for those who understand the tradeoff (the downsides should be well documented) and want to make it, they have that choice.

I like this idea, although I still wonder whether even having that option is a bad temptation to have lying around -- something that'll be awfully tempting in the moment, but you may regret it in the morning :-)

comment:5 Changed 6 years ago by Shai Berger

Just FTR, the use_in_migration flag indeed removes my objections.

comment:6 Changed 6 years ago by Simon Charette

Hi Markus,

Since we discussed this topic a bit in Amsterdam I also wanted to chime in order to let you know the use_in_migration flag also removes the objections I raised.

comment:7 Changed 6 years ago by Markus Holtermann

Has patch: set
Needs tests: set

I finally had some time to write some docs and tests. Here's the pull request: https://github.com/django/django/pull/3687

comment:8 Changed 6 years ago by Markus Holtermann

Needs tests: unset

comment:9 Changed 6 years ago by Carl Meyer

Triage Stage: AcceptedReady for checkin

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

In e37ab311fc52ff5c9ea0951ce3a8d8b38f285900:

Changed internal storing of abstract and concrete managers to be in a single list.

This commit prepares the internal manager layout to be serialized by
migrations; refs #23822.

comment:11 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In aa5ef0d4fc67a95ac2a5103810d0c87d8c547bac:

Fixed #23822 -- Added support for serializing model managers in migration

Thanks to Shai Berger, Loïc Bistuer, Simon Charette, Andrew Godwin,
Tim Graham, Carl Meyer, and others for their review and input.

comment:12 Changed 6 years ago by Tim Graham <timograham@…>

In 3ef50a772b6419c0c41635f80995df25fbc5116e:

Fixed reverse test execution for migration manager tests

Thanks to Tim Graham for reporting the issue; refs #23822

comment:13 Changed 6 years ago by Tim Graham <timograham@…>

In 1f03d2d924ba90e83dc9d8c93438bf5ee2f17ccd:

Refs #23822 -- Made MigrationOptimizer aware of model managers

comment:14 Changed 5 years ago by Simon Charette <charette.s@…>

In 3938b3cc:

Fixed #26286 -- Prevented content type managers from sharing their cache.

This should prevent managers methods from returning content type instances
registered to foreign apps now that these managers are also attached to models
created during migration phases.

Thanks Tim for the review.

Refs #23822.

comment:15 Changed 5 years ago by Simon Charette <charette.s@…>

In ba6f83ec:

[1.9.x] Fixed #26286 -- Prevented content type managers from sharing their cache.

This should prevent managers methods from returning content type instances
registered to foreign apps now that these managers are also attached to models
created during migration phases.

Thanks Tim for the review.

Refs #23822.

Backport of 3938b3ccaa85f1c366909a4839696007726a09da from master

comment:16 Changed 5 years ago by Simon Charette <charette.s@…>

In 4701c81d:

[1.8.x] Fixed #26286 -- Prevented content type managers from sharing their cache.

This should prevent managers methods from returning content type instances
registered to foreign apps now that these managers are also attached to models
created during migration phases.

Thanks Tim for the review.

Refs #23822.

Backport of 3938b3ccaa85f1c366909a4839696007726a09da from master

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