Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26643 closed Bug (fixed)

AlterModelManagers migration is generated for all Models with custom Managers pointing to the default Django Manager

Reported by: Anthony King Owned by: Matthew Schinckel
Component: Migrations Version: 1.10
Severity: Release blocker Keywords:
Cc: Loic Bistuer 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

Working on a project with Models written with migrations generated by Django 1.8 (upgraded to 1.9), when running makemigrations, some unexpected migrations were generated.

...
migrations.AlterModelManagers(
    name='some_model',
    managers=[
        ('objects', django.db.models.manager.Manager()),
    ],
),

We've never run AlterModelManagers in the past. Instead of the Manager we define being added, the default Manager is being set.

I can add code examples + instructions later

Change History (16)

comment:1 by Anthony King, 8 years ago

Further investigation shows that if you have a manager without use_in_migrations set, it will explicitly set the default manager in AlterModelManagers instead of the previous behaviour of not creating a migration at all (ie, the migration treats the model as if no manager has been set at all).

As such, I'm going to remove the bug label, however it would be good to mention this in the release notes to avoid questions down the road.

comment:2 by Anthony King, 8 years ago

Type: BugUncategorized

comment:3 by Tim Graham, 8 years ago

Cc: Loic Bistuer added
Component: MigrationsDocumentation
Keywords: 1.10 added
Type: UncategorizedCleanup/optimization

comment:4 by Loic Bistuer, 8 years ago

Hi cybojenix, could you still provide code examples? I'd like to ensure it's the ideal behavior.

comment:5 by Markus Holtermann, 8 years ago

Component: DocumentationMigrations
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationBug

I briefly talked to cybojenix on IRC, this sounds like a bug to me.

Seen behavior:

class CustomManager(models.Manager):
    pass

class Model(models.Model):
    objects = CustomManager()

This creates a migration with a default manager though no manager is marked with use_in_migrations = True.

Expected behavior:

Unless there's >=1 manager with use_in_migrations = True don't actually add the manager to the state but only consider it was added. This prevents unnecessary migrations that don't add anything useful. Essentially, a ModelState with an empty managers dict is the same as defining a model without setting an explicit manager (neither default nor a custom one).


Marked as release blocker due to unexpected creation of migrations. I'd have wanted this fixed when I'd have noticed it while reviewing https://github.com/django/django/pull/6175

comment:7 by Markus Holtermann, 8 years ago

Patch needs improvement: set

comment:8 by Matthew Schinckel, 8 years ago

in reply to:  8 comment:9 by Matthew Schinckel, 8 years ago

Replying to schinckel:

I believe reinstating the code at https://github.com/django/django/commit/ed0ff913c648b16c4471fc9a9441d1ee48cb5420#diff-c8a3e1d248f8f5b10af7ac5b1d8479f4L516 is sufficient to fix this bug.

Ah, not quite. This causes a deprecation warning in the manager inheritance stuff.

Perhaps the check could be included in the autodetector, instead of the ModelState.

comment:10 by Matthew Schinckel, 8 years ago

Needs tests: set
Owner: changed from nobody to Matthew Schinckel
Status: newassigned

Okay, I have a patch, but no tests: https://github.com/schinckel/django/commit/38a0cb2a9bfb4752b661905a2de6278fed9771dd

Should I be creating a PR against 1.10.x, or master?

Version 0, edited 8 years ago by Matthew Schinckel (next)

comment:11 by Tim Graham, 8 years ago

Keywords: 1.10 removed
Version: master1.10

comment:12 by Tim Graham, 8 years ago

Needs tests: unset
Patch needs improvement: unset

Test is added to the PR.

comment:13 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Loic Bistuer, 8 years ago

Latest PR.

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

Resolution: fixed
Status: assignedclosed

In 2eb7cb2f:

Fixed #26643 -- Prevented unnecessary AlterModelManagers operations caused by the manager inheritance refactor.

This also makes migrations respect the base_manager_name and
default_manager_name model options.

Thanks Anthony King and Matthew Schinckel for the initial patches.

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

In 0f23bceb:

[1.10.x] Fixed #26643 -- Prevented unnecessary AlterModelManagers operations caused by the manager inheritance refactor.

This also makes migrations respect the base_manager_name and
default_manager_name model options.

Thanks Anthony King and Matthew Schinckel for the initial patches.

Backport of 2eb7cb2fffcc382979d0731370de26b051d04659 from master

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