Re: [Django] #33586: Applying RunPython backward results in ValueError when involved models are referenced through multiple foreign keys

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Changes (by Enrico Zini):

* cc: Enrico Zini (added)

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

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by Tanishq):

Hey,

I was able to reproduce this locally. Noticed PR #16147 mentioned, but it
hasn’t been updated in a while and isn’t linked here.
Is this still open for new ideas/PRs? Happy to build on that one or just
put together a fresh patch with a test + perf notes, whichever helps most.
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:31>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by Jacob Walls):

That PR was closed with a link to comment:19. Yes, a new PR is welcome--as
always, be sure to take previous feedback into account.
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:32>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by Tanishq):

Thank you for the go-ahead, Jacob. I'll begin working on the new PR
shortly and make sure to keep the previous feedback in mind while
preparing the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:33>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Changes (by mag123c):

* cc: mag123c (added)

Comment:

Hi @Tanishq1030, are you still working on this? If not, I'd be happy to
pick this up and prepare a patch with the required tests and performance
benchmarks.
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:34>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by mag123c):

Hi Tanishq1030, are you still working on this?

If not, I'd be happy to pick this up and prepare a patch with the required
tests and performance benchmarks.
--

Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:35>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by Jacob Walls):

From the hint on #36161, I was able to solve this in an affected project
like:

{{{#!diff
diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py
index 9e9cc58fae..2eecb640bf 100644
--- a/django/db/migrations/state.py
+++ b/django/db/migrations/state.py
@@ -410,10 +410,7 @@ class ProjectState:
except LookupError:
pass
else:
- if delay:
-
related_models.update(get_related_models_tuples(rel_model))
- else:
-
related_models.update(get_related_models_recursive(rel_model))
+
related_models.update(get_related_models_recursive(rel_model))

# Include the model itself
related_models.add((app_label, model_name))

}}}

Notice the comment just above this says "For all direct related models
recursively get all related models." So, with this proposal, we really
''would'' be getting all recursive models.

Does this look promising?
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:36>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys
-------------------------------+------------------------------------
Reporter: Jeremy Poulin | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by Simon Charette):

One thing that isn't clear to me if we go froward with this change is what
purpose do `_find_reload_model(delay=True)` even has as if we now
systematically get all models recursively later in the function. By that I
ask what's the purpose of this branch

{{{#!diff
diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py

index 9e9cc58fae..3f8f10d601 100644
--- a/django/db/migrations/state.py
+++ b/django/db/migrations/state.py
@@ -383,10 +383,7 @@ class ProjectState:
else:
# Get all relations to and from the old model before
reloading,
# as _meta.apps may change
- if delay:
- related_models = get_related_models_tuples(old_model)
- else:
- related_models = get_related_models_recursive(old_model)
+ related_models = get_related_models_recursive(old_model)

# Get all outgoing references from the model to be rendered
model_state = self.models[(app_label, model_name)]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:37>

[Django]

#33586: Applying RunPython backward results in ValueError when involved models are
referenced through multiple foreign keys

-------------------------------+---------------------------------------
Reporter: Jeremy Poulin | Owner: Sarah Boyce
Type: Bug | Status: assigned

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

-------------------------------+---------------------------------------
Changes (by Jacob Walls):

* owner: (none) => Sarah Boyce
* status: new => assigned

Comment:

I commented that branch out and got failures relating to `AlterField`
operations changing the targets of an FK. From the comment above, "Get all
relations ... as _meta.apps may change." So my understanding is that the
results are not the same in those two branches (not duplicative work).

Tentatively passing over to Djangonaut Space navigators to evaluate if a
good fit for anyone (git archeology, benchmarking, migration system).

Reiterating that #36161 was a dupe with a nice low-level diagnosis of the
symptoms of the problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/33586#comment:38>