Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#24067 closed Cleanup/optimization (fixed)

Renaming models prompts for content type deletions

Reported by: doepunk Owned by: Simon Charette
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Simon Charette 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 (last modified by doepunk)

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.

Change History (18)

comment:1 by doepunk, 9 years ago

Component: UncategorizedMigrations

comment:2 by doepunk, 9 years ago

Description: modified (diff)

comment:3 by Tim Graham, 9 years ago

Summary: Renaming Models asks for double confirmation (and doesn't preserve relationships?)Renaming models prompts for content type deletions
Triage Stage: UnreviewedAccepted
Type: BugNew feature

I'm not sure if automating the data migration is a good idea, but we should likely at least document these considerations if not.

comment:4 by Simon Charette, 9 years ago

Cc: Simon Charette added

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 never perform database operation when update_contenttypes is.

Last edited 9 years ago by Simon Charette (previous) (diff)

comment:5 by Aymeric Augustin, 9 years ago

I agree with charettes. I wrote data migrations to handle this in some projects.

comment:6 by Claude Paroz, 9 years ago

+1

Having the plan in post_migrate would also allow us to easily fix #24075.

comment:7 by Simon Charette, 9 years ago

Created #24100 to track the addition of the plan kwarg.

comment:8 by Simon Charette, 8 years ago

Has patch: set
Type: New featureCleanup/optimization
Version: 1.7master

comment:9 by Tim Graham, 8 years ago

Patch needs improvement: set

Doesn't seem to work quite right in my testing.

comment:10 by Simon Charette, 8 years ago

Patch needs improvement: unset

The case issue should be fixed.

comment:11 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Aymeric Augustin, 8 years ago

Good to see that this ticket is moving forwards, as I just hit this issue again.

comment:13 by Simon Charette, 8 years ago

Owner: changed from nobody to Simon Charette
Patch needs improvement: set
Status: newassigned
Triage Stage: Ready for checkinAccepted

I should be able to deal with what's left next week.

comment:14 by Simon Charette, 8 years ago

Patch needs improvement: unset

comment:15 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by Simon Charette <charette.s@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In f179113e:

Fixed #24067 -- Renamed content types upon model renaming.

Thanks to Tim for the extensive review.

comment:17 by Simon Charette <charette.s@…>, 8 years ago

In 722344ee:

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.

comment:18 by Simon Charette <charette.s@…>, 8 years ago

In 826ec5ee:

[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

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