Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#23822 closed New feature (fixed)

Serialize model managers in migrations

Reported by: Markus Holtermann Owned by: Markus Holtermann
Component: Migrations Version: dev
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 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

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

comment:2 by Shai Berger, 10 years ago

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 by Markus Holtermann, 10 years ago

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 by Carl Meyer, 10 years ago

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 by Shai Berger, 10 years ago

Just FTR, the use_in_migration flag indeed removes my objections.

comment:6 by Simon Charette, 10 years ago

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 by Markus Holtermann, 10 years ago

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 by Markus Holtermann, 10 years ago

Needs tests: unset

comment:9 by Carl Meyer, 10 years ago

Triage Stage: AcceptedReady for checkin

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

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 by Tim Graham <timograham@…>, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

In 3ef50a772b6419c0c41635f80995df25fbc5116e:

Fixed reverse test execution for migration manager tests

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

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

In 1f03d2d924ba90e83dc9d8c93438bf5ee2f17ccd:

Refs #23822 -- Made MigrationOptimizer aware of model managers

comment:14 by Simon Charette <charette.s@…>, 9 years ago

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 by Simon Charette <charette.s@…>, 9 years ago

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 by Simon Charette <charette.s@…>, 9 years ago

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