Re: [Django] #36168: Backwards migration to replaced migration when other app has squashed migrations can lead to FieldDoesNotExist error due to incorrect state

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
------------------------------------+------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------------+------------------------------------
Changes (by Sarah Boyce):

* resolution: needsinfo =>
* status: closed => new

--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
------------------------------------+------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------------+------------------------------------

Comment (by Jacob Walls):

Thanks for the ping on github. I agree some investigation is needed about
whether & where to unset `replace_migrations = False` back to True.

Have you looked into pre-walking the graph nodes to find out if any
migrations are missing, so we could reload with `replace_migrations =
False` *before* the loop? I didn't get a chance to do a deeper dive than
that.
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:7>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
------------------------------------+------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------------+------------------------------------

Comment (by Klaas van Schelven):

I'm not sure what you mean exactly... in particular with "pre walking".
Also I'm not sure what you mean with "the loop"; the whole point is that
there are 2 usage locations (and that in certain conditions they need
different values for replace_migrations).

I think the graph is cached (and this caching is overridden for the case I
highlighted). In my mind, as long as that "clobbering" is done after the
function that needs it returns, we're probably good. In my PoC I did that
by storing the unclobbered result, but any other solution would do to, I
think.
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:8>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
------------------------------------+------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------------+------------------------------------

Comment (by Jacob Walls):

Hi Klaas. Sorry if my shorthand was not helpful. What I meant was that I
tested your suggested patch against your test project and found that I
still couldn't migrate backward to 0001_initial, finding instead this
exception:

{{{#!py
File "/Users/.../django/django/db/migrations/executor.py", line 229, in
_migrate_all_backwards
self.unapply_migration(states[migration], migration, fake=fake)
~~~~~~^^^^^^^^^^^
KeyError: <Migration triggerfailingcode.0002_baz_baz>
}}}

Because your patch (and the changes in #24900) take place inside a
[https://github.com/django/django/blob/14b46c1b848d846fad55c8634daeabd8787cf0d6/django/db/migrations/executor.py#L31
loop over migration targets], by "pre-walking" I wondered if we could
avoid rebuilding the graph during the loop and instead just derive
whatever information we need to build the graph correctly only once, but I
wasn't saying I had confidence in this hypothesis, I was just asking if
you had already explored it. From the PR review comments in #24900 it
appears the original version was quite different and perhaps more like
what I'm suggesting now. (It's hard to say since I don't have the branch
anymore.)

Thanks for looking into it, I'd be glad to review a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:9>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state

-------------------------------------+-------------------------------------

Reporter: Klaas van Schelven | Owner:

| houston0222
Type: Bug | Status: assigned

Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by houston0222):

* owner: (none) => houston0222
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:10>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner:
| houston0222
Type: Bug | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by houston0222):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:11>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner:
| houston0222
Type: Bug | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by houston0222):

Hi everyone,

I've submitted a pull request to address this issue:
https://github.com/django/django/pull/19612

It adds a conditional check to avoid reapplying replaced migrations when
migrating backwards, which can lead to duplicate operations.
Feedback welcome:)
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:12>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner:
| houston0222
Type: Bug | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by houston0222):

Just a quick update: I'm preparing a small example project that shows the
issue, explains how it was found and debugged, and helps confirm that the
fix works.

The goal is to make the problem and the review process clearer for anyone
looking at the patch.

I’ll follow up with more details once it’s ready. Thank you for your time.
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:13>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner:
| houston0222
Type: Bug | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by houston0222):

Replying to [comment:13 houston0222]:

> Just a quick update: I'm preparing a small example project that shows
the issue, explains how it was found and debugged, and helps confirm that
the fix works.
>
> The goal is to make the problem and the review process clearer for
anyone looking at the patch.
>
> I’ll follow up with more details once it’s ready. Thank you for your
time.

Hi, all
I’ve written an article explaining how the issue was identified and how
the proposed logic resolves it
https://dev.to/houston_wong_78b96dd3a773/fixing-django-squashed-migration-
in-multi-app-issue-36168-5g39
the new PR: https://github.com/django/django/pull/19623

Thank you!
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:14>

[Django]

#36168: Backwards migration to replaced migration when other app has squashed
migrations can lead to FieldDoesNotExist error due to incorrect state
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner:
| houston0222
Type: Bug | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:15>