Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#24100 closed New feature (fixed)

Make `post_migrate` dispatch the migration plan.

Reported by: Simon Charette Owned by: Markus Holtermann
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: frnhr, bronger@… 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 Tim Graham)

Accepting based on +1's received from Aymeric and Claude in order to solve #24067, #24075.

Change History (22)

comment:1 by Simon Charette, 10 years ago

Has patch: set

Created a PR for this ticket.

I'd like to give a try at fixing #24067 before getting this merge to maked sure the provided plan is sufficient to issue ContentType renames.

Version 1, edited 10 years ago by Simon Charette (previous) (next) (diff)

comment:2 by Markus Holtermann, 10 years ago

Cc: info+coding@… added
Needs documentation: set
Needs tests: set
Patch needs improvement: set

As already mentioned on the PR, I'm not sure the current API is stable enough (in terms of "Django doesn't crash", not in terms of "We change a function signature") for public usage. I'd like to have the alpha and beta for tests. Tim noted on IRC that changing the API after beta isn't really a thing we should do which I do understand.

A workaround we could also simply backport to 1.7 (and add plan and state in 1.9), is a check for the respective first migrations being applied:

In django.contrib.contenttypes.management:

def update_contenttypes(...):
    from django.db import connection
    from django.db.migrations.loader import MigrationLoader
    loader = MigrationLoader(connection)
    loader.load_disk()
    if not ('contenttypes', '0001_initial') in loader.applied_migrations:
        return
Last edited 10 years ago by Markus Holtermann (previous) (diff)

comment:3 by Markus Holtermann, 10 years ago

Owner: changed from Simon Charette to Markus Holtermann
Status: newassigned

comment:5 by frnhr, 9 years ago

Cc: frnhr added

comment:6 by Torsten Bronger, 9 years ago

Cc: bronger@… added

comment:7 by Markus Holtermann, 9 years ago

Cc: info+coding@… removed
Needs documentation: unset
Patch needs improvement: unset

comment:8 by Tim Graham, 9 years ago

See #25931 for a related issue with ContentType creation and backwards migrations.

comment:9 by Amos Onn, 9 years ago

For issue #25931, it is sufficient to use the project_state after the migration, and no need to review the plan. This might make it an easier pick for testing that part of the interface.

As for the ContentType renames, I think that perhaps it would be cleaner to add signals for specific migration operations (such as RenameModel), which get passed the two states of the migration and perform their additional action (here, renaming the relevant ContentType entry), and have contrib.contenttypes connect to that - rather than having it sift through the whole plan itself. This way the change can also be done mid-migration, and enable the rest of the migration to rely on a consistent ContentType table.

comment:10 by Simon Charette, 9 years ago

As for the ContentType renames, I think that perhaps it would be cleaner to add signals for specific migration operations (such as RenameModel), which get passed the two states of the migration and perform their additional action (here, renaming the relevant ContentType entry), and have contrib.contenttypes connect to that - rather than having it sift through the whole plan itself. This way the change can also be done mid-migration, and enable the rest of the migration to rely on a consistent ContentType table.

The down side of basing the detection on RenameModel only is the fact we don't handle any third party operation or usage of SeparateDatabaseAndState that could also alter the model state.

I agree with you that contrib.contenttypes shouldn't have to figure out which model were renamed from the plan by itself. Defining a utility function in the migrations module and using it the signal receiver would make more sense.

comment:11 by Simon Charette, 9 years ago

Needs tests: unset

PR.

@amosonn, I ended up going with an approach similar to what you proposed.

comment:12 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Tim Graham, 9 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:14 by Tim Graham, 9 years ago

This may also fix #26588.

comment:15 by Simon Charette, 9 years ago

Patch needs improvement: unset

comment:16 by Tim Graham, 9 years ago

Description: modified (diff)
Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In f937c9e:

Fixed #24100 -- Made the migration signals dispatch its plan and apps.

Thanks Markus for your contribution and Tim for your review.

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

In 05a9f3a0:

Refs #24100 -- Fixed a test failure on MySQL related to non-transactional DDL.

Thanks Tim for the report.

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

In d1757d8d:

Fixed #27044 -- Included already applied migration changes in the post-migrate state when the execution plan is empty.

Refs #24100.

Thanks tkhyn for the report and Tim for the review.

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

In d5c4ea52:

Fixed #27100 -- Included already applied migration changes in the pre-migrate state.

Refs #24100.

Thanks Tim for the review.

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

In 1c60765d:

[1.10.x] Fixed #27044 -- Included already applied migration changes in the post-migrate state when the execution plan is empty.

Refs #24100.

Thanks tkhyn for the report and Tim for the review.

Backport of d1757d8df486b689172d2584ded52fad916bcc33 from master

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

In 0b45480:

[1.10.x] Fixed #27100 -- Included already applied migration changes in the pre-migrate state.

Refs #24100.

Thanks Tim for the review.

Backport of d5c4ea524679a787fe11c927448e44e95646096b from master

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