[Django] #24067: Renaming Models asks for double confirmation (and doesn't preserve relationships?)
Django
relationships?)
-------------------------------+--------------------
Reporter: doepunk | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
When renaming a model (from OldModel to NewModel) I noticed that
makemigrations asks for confirmation if I had renamed NewModel to
OldModel. Which is great that that was detected and the confirmation helps
to get a correct migration file, which makes this step reproducible. The
Model in question did not involve any foreign keys to or from it. However
when I ran the migrate, I got this message
{{{
The following content types are stale and need to be deleted:
<app-name> | <old-name>
Any objects related to these content types by a foreign key will also
be deleted. Are you sure you want to delete these content types?
If you're unsure, answer 'no'.
}}}
I'm unsure what's going on here. But shouldn't renaming involve the
automatic deletion of the old name and replacing the foreign keys with the
new database table keys? All without a need for confirmation. Or at least
if there are no foreign keys as in my case, the confirmation seems
unnecessary and it only makes it harder to run the migration unsupervised
and it introduces a potential error source. Assuming that all data from
the old table were copied to the new table and all foreign key
relationships copied and updated as well, I don't see a use case for
keeping the old table and "related objects" around. And as for the case
that there were any changes in relationships reflected in the Models
(other than the name change) this could be detected and confirmed in the
makemigrations stage. (For example if there were still references to
OldName in the code when OldName no longer exists as a class, this would
result in a syntax error, or if a Foreign Key attribute to OldModel was
deleted and not replaced with NewModel Foreign Key it's just akin to a
field deletion). It just seems more reproducible to keep all confirmations
in the makemigration stage.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
relationships?)
Reporter: doepunk | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
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 doepunk):
* needs_better_patch: => 0
* component: Uncategorized => Migrations
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:1>
Django
relationships?)
Reporter: doepunk | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by doepunk:
Old description:
New description:
When renaming a model (from OldModel to NewModel) I noticed that
makemigrations asks for confirmation if I had renamed NewModel to
OldModel. Which is great that that was detected and the confirmation helps
to get a correct migration file, which makes this step reproducible. The
Model in question did not involve any foreign keys to or from it. However
when I ran the migrate, I got this message
{{{
The following content types are stale and need to be deleted:
<app-name> | <old-name>
Any objects related to these content types by a foreign key will also
be deleted. Are you sure you want to delete these content types?
If you're unsure, answer 'no'.
}}}
I'm unsure what's going on here. But shouldn't renaming involve the
automatic deletion of the old name and replacing the foreign keys with the
new database table keys? All without a need for confirmation. Or at least
if there are no foreign keys as in my case, the confirmation seems
unnecessary and it only makes it harder to run the migration unsupervised
and it introduces a potential error source. Assuming that all data from
the old table were copied to the new table and all foreign key
relationships copied and updated as well, I don't see a use case for
keeping the old table and "related objects" around. And as for the case
that there were any changes in relationships reflected in the Models
(other than the name change) this could be detected and confirmed in the
makemigrations stage. (For example if there were still references to
OldName in the code when OldName no longer exists as a class, this would
result in an exception, or if a Foreign Key attribute to OldModel was
deleted and not replaced with NewModel Foreign Key it's just akin to a
field deletion). It just seems more reproducible to keep all confirmations
in the makemigration stage.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:2>
Django
-----------------------------+------------------------------------
Reporter: doepunk | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
I'm not sure if automating the data migration is a good idea, but we
should likely at least document these considerations if not.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:3>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: charettes (added)
Comment:
IMO the content types should be automatically updated upon `RenameModel`
operations.
Unfortunately there's no way for the
`django.contrib.contenttypes.management.update_contenttypes` receiver to
differentiate between model renaming, addition and deletion hence the
current prompt.
Would it make sense to dispatch the plan via `post_migrate` to allow
receivers to deal with applied migrations? In the case of the `flush`
command plan would be `None`.
Given this plan, `update_contenttypes` could automatically update the
`ContentType.model` by following the `RenameModel` chain.
By the way I find it a bit odd that we document that `post_migrate`
receivers should always perform database operation when
`update_contenttypes` is.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:4>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
I agree with charettes. I wrote data migrations to handle this in some
projects.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:5>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
+1
Having the plan in `post_migrate` would also allow us to easily fix
#24075.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:6>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
Created #24100 to track the addition of the `plan` kwarg.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:7>
Django
Reporter: doepunk | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
* version: 1.7 => master
* type: New feature => Cleanup/optimization
Comment:
[https://github.com/django/django/pull/5835 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:8>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Doesn't seem to work quite right in my testing.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:9>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 1 => 0
Comment:
The case issue should be fixed.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:10>
Django
Reporter: doepunk | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | 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/24067#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Good to see that this ticket is moving forwards, as I just hit this issue
again.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* owner: nobody => charettes
* needs_better_patch: 0 => 1
* status: new => assigned
* stage: Ready for checkin => Accepted
Comment:
I should be able to deal with what's left next week.
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: doepunk | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/6612 updated PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: doepunk | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | 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/24067#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: doepunk | Owner: charettes
Cleanup/optimization |
Component: Migrations | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f179113e6cbc8ba0a8d4e87e1d4410fb61d63e75" f179113e]:
{{{
#!CommitTicketReference repository=""
revision="f179113e6cbc8ba0a8d4e87e1d4410fb61d63e75"
Fixed #24067 -- Renamed content types upon model renaming.
Thanks to Tim for the extensive review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: doepunk | Owner: charettes
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"722344ee59fb89ea2cd5b906d61b35f76579de4e" 722344ee]:
{{{
#!CommitTicketReference repository=""
revision="722344ee59fb89ea2cd5b906d61b35f76579de4e"
Refs #24067 -- Fixed contenttypes rename tests failures on Oracle.
Broke the initial migration in two to work around #25530 and added
'django.contrib.auth' to the available_apps to make sure its tables are
also
flushed as Oracle doesn't implement cascade deletion in sql_flush().
Thanks Tim for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: doepunk | Owner: charettes
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"826ec5ee57ba798f4706d4fd6f9ffa8b1065508d" 826ec5ee]:
{{{
#!CommitTicketReference repository=""
revision="826ec5ee57ba798f4706d4fd6f9ffa8b1065508d"
[1.10.x] Refs #24067 -- Fixed contenttypes rename tests failures on
Oracle.
Broke the initial migration in two to work around #25530 and added
'django.contrib.auth' to the available_apps to make sure its tables are
also
flushed as Oracle doesn't implement cascade deletion in sql_flush().
Thanks Tim for the report.
Backport of 722344ee59fb89ea2cd5b906d61b35f76579de4e from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24067#comment:18>