swev-id: django__django-15563 by casey-brooks · Pull Request #514 · agyn-sandbox/django

casey-brooks · 2025-12-24T17:07:40Z

Summary

add regression coverage for MTI queryset.update with multiple concrete parents
ensure UpdateQuery stores per-parent related ids
select parent-link values in SQLUpdateCompiler to constrain ancestor updates

Testing

PYTHONPATH=$PWD:$PWD/tests:$HOME/.nix-profile/lib/python3.11/site-packages DJANGO_SETTINGS_MODULE=tests.test_sqlite python3 tests/runtests.py model_inheritance.test_mti_update --parallel=1
PYTHONPATH=$PWD:$PWD/tests:$HOME/.nix-profile/lib/python3.11/site-packages python3 -m compileall django/db/models/sql/compiler.py django/db/models/sql/subqueries.py tests/model_inheritance/test_mti_update.py

Pre-fix failure

FAIL: test_update_other_parent_targets_related_rows (model_inheritance.test_mti_update.MultiParentUpdateTests.test_update_other_parent_targets_related_rows)
AssertionError: 77 != 303

Resolves #504

casey-brooks · 2025-12-24T17:07:51Z

Test & Lint Summary

PYTHONPATH=$PWD:$PWD/tests:$HOME/.nix-profile/lib/python3.11/site-packages DJANGO_SETTINGS_MODULE=tests.test_sqlite python3 tests/runtests.py model_inheritance.test_mti_update --parallel=1
PYTHONPATH=$PWD:$PWD/tests:$HOME/.nix-profile/lib/python3.11/site-packages python3 -m compileall django/db/models/sql/compiler.py django/db/models/sql/subqueries.py tests/model_inheritance/test_mti_update.py

Result: 4 tests passed; linting via compileall succeeded with no errors.

rowan-stein · 2025-12-24T17:08:27Z

Requesting review for this fix. References Issue #504.

noa-lucent

Thanks for tackling the MTI update bug. I found two places where we silently skip work when the update invariants break: (1) in the compiler we when returns None, and (2) in we fall back to an empty list when a model key is missing. Both cases mask real errors and leave parent tables stale. Please make these code paths fail loudly instead of papering over them.

noa-lucent · 2025-12-24T17:16:30Z

django/db/models/sql/compiler.py

+                for model in self.query.related_updates:
+                    parent_link = self.query.get_meta().get_ancestor_link(model)
+                    if parent_link is None:
+                        continue


[major] returning would mean we somehow enqueued a model in that isn’t actually an ancestor of the base model. That’s an invariant violation, and silently -ing leaves the parent table stale without any signal. In this internal code we should fail loudly instead of hiding the problem. Please turn this into an assertion (or otherwise raise) so that we surface the bug rather than silently skipping the update.

noa-lucent · 2025-12-24T17:16:40Z

django/db/models/sql/subqueries.py

            query.values = values
-            if self.related_ids is not None:
-                query.add_filter("pk__in", self.related_ids)
+            ids = self.related_ids.get(model, [])


[major] now populates per ancestor, so by the time we get here every in should have an entry. Falling back to masks cases where we failed to collect IDs and silently turns the parent update into a no-op. Please require the key (e.g. ) or assert that it exists so we fail loudly when invariants break.

noa-lucent

The assertions you added address the invariants I was worried about and prevent silent skips in the parent updates. All previous concerns are resolved from my side.

noa-lucent

Thanks for the fix and coverage. I left one blocking note on the new SQL assertion test: as written it still passes against the buggy implementation because the condition accepts the child PKs. Please tighten the assertions so the test fails if we regress.

noa-lucent · 2026-01-11T20:32:34Z

tests/model_inheritance/test_mti_update.py

+                continue
+            self.assertIn(" WHERE ", statement)
+            self.assertIn(" IN ", statement)
+            self.assertTrue(


[major] This regression test still passes against the broken implementation. When the UPDATE on incorrectly uses the child PKs (the original bug), the SQL still contains and , so both succeed. To prove we really constrain by the parent-link IDs, the assertions need to check each table separately: e.g. require for the secondary-parent UPDATE and for the primary-parent UPDATE (and ideally assert the wrong IDs are absent). That way the test will fail if we regress to selecting the wrong key. Can you tighten the assertions accordingly?

noa-lucent · 2026-01-11T20:32:46Z

tests/model_inheritance/test_mti_update.py

+                continue
+            self.assertIn(" WHERE ", statement)
+            self.assertIn(" IN ", statement)
+            self.assertTrue(


[major] The regression still passes against the buggy implementation. When the SecondaryParent UPDATE mistakenly uses the child PKs (the original bug), the SQL still contains self.child_one.pk/self.child_two.pk, so both assertTrue(... or ...) succeed. To prove we really constrain by each parent-link ID, the test should branch per table — e.g. require child_*_secondary_id for the secondary-parent UPDATE and child_*.pk for the primary-parent UPDATE (and ideally assert that the wrong IDs are absent). Without that tightening, this test won’t fail on a regression.

casey-brooks · 2026-01-11T20:47:18Z

Tests & Lint Summary

PYTHONPATH=$PWD:$PWD/tests:$HOME/.local/lib/python3.11/site-packages:$HOME/.nix-profile/lib/python3.11/site-packages
DJANGO_SETTINGS_MODULE=tests.test_sqlite
python3 tests/runtests.py model_inheritance.test_mti_update --parallel=1 → 4 passed
PATH=$HOME/.local/bin:$PATH flake8 tests/model_inheritance/test_mti_update.py → no lint issues

noa-lucent

The tightened SQL assertions now force the secondary-parent UPDATE to use the parent-link IDs and fail against the original bug. My previous concern is resolved.

noa-lucent

Thanks for digging into the MTI update regression. The fix currently adds a second query per ancestor and broadens the UpdateQuery API. To keep the patch focused, please revert the related_ids dict changes (and the new MTI test module) and instead extend the existing preselect so it returns the ancestor pk columns in the same round-trip. Once those columns are available we can rebuild the per-parent id lists without cloning new queries or touching the UpdateQuery surface. Happy to take another look once that’s in place.

noa-lucent · 2026-01-13T12:46:04Z

django/db/models/sql/compiler.py

+                        parent_link is not None
+                    ), "Related update requested for non-ancestor model"
+                    parent_query = query.clone()
+                    parent_query.select = []


[major] This loop is doing a second pass over the update candidates for every ancestor. That gives us 1+n round trips per update and forces the rest of the compiler to grow the dict plumbing. The regression we need to cover is only that the existing preselect wasn’t including the ancestor PK columns. We can fix that by having the first select the child PK plus any parent-link attnames that aren’t already on the primary-key chain, then build the per-parent id lists from that single result set. Could you revert the extra dict machinery and fold the ancestor IDs into the existing preselect instead?}

Updated pre_sql_setup() to fold ancestor IDs into the primary preselect and raise if we ever miss a parent link. Ancestor queries now reuse that single result set without any extra round-trips.

noa-lucent · 2026-01-13T12:46:14Z

django/db/models/sql/subqueries.py

        """
        self.values = []
-        self.related_ids = None
+        self.related_ids = {}


[major] Let’s keep UpdateQuery.related_ids as the original list of primary keys. Once pre_sql_setup() returns the ancestor IDs from its single preselect we can fan them out per ancestor without changing the query object’s public surface. The dict cloning/copying here is extra churn that we don’t need once the compiler collects all columns up front. Could you drop this type change and keep the old related_ids storage?

Restored UpdateQuery.related_ids to lists populated by the compiler pass. We now error out if the compiler fails to supply IDs instead of materialising empty dict entries.

noa-lucent · 2026-01-13T12:46:18Z

django/db/models/sql/subqueries.py

-            if self.related_ids is not None:
-                query.add_filter("pk__in", self.related_ids)
+            try:
+                ids = self.related_ids[model]


[major] Once the compiler hands us all the ancestor PKs via a single preselect, we can populate the per-model id lists without any KeyError plumbing or extra assertions here. Please revert this catch/raise block and keep the old pk__in call that reused the preselected ids.

Dropped the fallback plumbing—get_related_updates() now expects the compiler to hand back the ancestor PK lists and will raise immediately if any model is missing.

noa-lucent · 2026-01-13T12:46:36Z

tests/model_inheritance/test_mti_update.py

@@ -0,0 +1,259 @@
+import re


[major] The regression coverage shouldn’t need its own 250-line test module with dynamically constructed apps. We can exercise this bug inside the existing suite by extending the current multi-table inheritance cases. Please drop this new file and keep the coverage focused in the existing test module.

Moved the regression coverage into tests/model_inheritance/tests.py so it extends the existing MTI suite instead of living in a standalone module. The tightened SQL assertions now distinguish parent-link IDs from child PKs.

casey-brooks · 2026-01-13T13:05:45Z

Summary

fold ancestor ID collection into the existing preselect and assert on invalid invariants
restore UpdateQuery.related_ids list semantics and move regression coverage into tests/model_inheritance/tests.py

Test & Lint

source /workspace/.venv/bin/activate && black django/db/models/sql/compiler.py django/db/models/sql/subqueries.py tests/model_inheritance/tests.py
source /workspace/.venv/bin/activate && isort django/db/models/sql/compiler.py django/db/models/sql/subqueries.py tests/model_inheritance/tests.py
source /workspace/.venv/bin/activate && flake8 django/db/models/sql/compiler.py django/db/models/sql/subqueries.py tests/model_inheritance/tests.py
source /workspace/.venv/bin/activate && PYTHONPATH=/workspace/django python tests/runtests.py model_inheritance --parallel=1 (57 passed)
source /workspace/.venv/bin/activate && PYTHONPATH=/workspace/django python tests/runtests.py update --parallel=1 (17 passed, 4 skipped)
source /workspace/.venv/bin/activate && PYTHONPATH=/workspace/django python tests/runtests.py model_inheritance_regress --parallel=1 (29 passed, 1 expected failure)

noa-lucent

The compiler now folds ancestor parent-link columns into the single preselect and reuses those values to build per-parent pk lists, so the related updates no longer drift onto unrelated rows. The regression coverage exercises mixed parent updates and verifies the emitted SQL constrains each ancestor with the right ids. Looks good to me.

fix(orm): correct mti related updates

8c603c1

casey-brooks requested a review from a team December 24, 2025 17:07

noa-lucent requested changes Dec 24, 2025

View reviewed changes

fix(orm): assert ancestor update invariants

a6489b9

noa-lucent approved these changes Dec 24, 2025

View reviewed changes

noa-lucent requested changes Jan 11, 2026

View reviewed changes

test(model_inheritance): tighten MTI update assertions

45c2380

noa-lucent approved these changes Jan 11, 2026

View reviewed changes

rowan-stein changed the base branch from django__django-15563 to main January 11, 2026 20:51

rowan-stein changed the base branch from main to django__django-15563 January 12, 2026 20:14

noa-lucent requested changes Jan 13, 2026

View reviewed changes

fix(sql): preselect ancestor update ids once

e3e6944

noa-lucent approved these changes Jan 13, 2026

View reviewed changes

Comments

Conversation

casey-brooks commented Dec 24, 2025

Summary

Testing

Pre-fix failure

Uh oh!

casey-brooks commented Dec 24, 2025

Test & Lint Summary

Uh oh!

rowan-stein commented Dec 24, 2025

Uh oh!

noa-lucent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noa-lucent left a comment

Choose a reason for hiding this comment

Uh oh!

noa-lucent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casey-brooks commented Jan 11, 2026

Uh oh!

noa-lucent left a comment

Choose a reason for hiding this comment

Uh oh!

noa-lucent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casey-brooks commented Jan 13, 2026

Summary

Test & Lint

Uh oh!

noa-lucent left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants