Opened 6 years ago

Closed 5 years ago

Last modified 5 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 Changed 6 years ago by Simon Charette

Has patch: set

Created a PR for this ticket.

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

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

comment:2 Changed 6 years ago by Markus Holtermann

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 6 years ago by Markus Holtermann (previous) (diff)

comment:3 Changed 6 years ago by Markus Holtermann

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

comment:5 Changed 6 years ago by frnhr

Cc: frnhr added

comment:6 Changed 6 years ago by Torsten Bronger

Cc: bronger@… added

comment:7 Changed 6 years ago by Markus Holtermann

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

comment:8 Changed 6 years ago by Tim Graham

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

comment:9 Changed 6 years ago by Amos Onn

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 Changed 6 years ago by Simon Charette

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 Changed 6 years ago by Simon Charette

Needs tests: unset

PR.

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

comment:12 Changed 6 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:13 Changed 5 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:14 Changed 5 years ago by Tim Graham

This may also fix #26588.

comment:15 Changed 5 years ago by Simon Charette

Patch needs improvement: unset

comment:16 Changed 5 years ago by Tim Graham

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

comment:17 Changed 5 years ago by Simon Charette <charette.s@…>

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 Changed 5 years ago by Simon Charette <charette.s@…>

In 05a9f3a0:

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

Thanks Tim for the report.

comment:19 Changed 5 years ago by Simon Charette <charette.s@…>

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 Changed 5 years ago by Simon Charette <charette.s@…>

In d5c4ea52:

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

Refs #24100.

Thanks Tim for the review.

comment:21 Changed 5 years ago by Simon Charette <charette.s@…>

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 Changed 5 years ago by Simon Charette <charette.s@…>

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