[Django] #26643: AlterModelManagers migration is generated for all Models with custom Managers pointing to the default Django Manager
Django
Managers pointing to the default Django Manager
----------------------------+--------------------
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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.
{{{#!python
...
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26643>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cybojenix):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:1>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cybojenix):
* type: Bug => Uncategorized
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:2>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: 1.10 | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: loic (added)
* keywords: => 1.10
* component: Migrations => Documentation
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:3>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: master
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
Hi cybojenix, could you still provide code examples? I'd like to ensure
it's the ideal behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:4>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* type: Cleanup/optimization => Bug
* component: Documentation => Migrations
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
I briefly talked to cybojenix on IRC, this sounds like a bug to me.
'''Seen behavior:'''
{{{#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:5>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: 1.10 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cybojenix):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/6635
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:6>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:7>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by schinckel):
I believe reinstating the code at
https://github.com/django/django/commit/ed0ff913c648b16c4471fc9a9441d1ee48cb5420
#diff-c8a3e1d248f8f5b10af7ac5b1d8479f4L516 is sufficient to fix this bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:8>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by schinckel):
Replying to [comment:8 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:9>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by schinckel):
* owner: nobody => schinckel
* status: new => assigned
* needs_tests: 0 => 1
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:10>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by timgraham):
* keywords: 1.10 =>
* version: master => 1.10
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:11>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Type: Bug | Status: assigned
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
Test is added to the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:12>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Type: Bug | Status: assigned
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:13>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Type: Bug | Status: assigned
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
Latest [https://github.com/django/django/pull/6799 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:14>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2eb7cb2fffcc382979d0731370de26b051d04659" 2eb7cb2f]:
{{{
#!CommitTicketReference repository=""
revision="2eb7cb2fffcc382979d0731370de26b051d04659"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:15>
Django
Managers pointing to the default Django Manager
Reporter: cybojenix | Owner: schinckel
Type: Bug | Status: closed
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"0f23bcebdfe556711a0463c8775981575c272b79" 0f23bceb]:
{{{
#!CommitTicketReference repository=""
revision="0f23bcebdfe556711a0463c8775981575c272b79"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26643#comment:16>