Repository navigation
fix(migrations): scope leaf/root nodes to same-app edges - #2307
inchang-ing wants to merge 9 commits into
Conversation
Merging this PR will not alter performance1 warning
Comparing |
ef142c6 to
a30b980
Compare
|
CI is now fully green — |
a30b980 to
4115df0
Compare
| return sorted(nodes) | ||
|
|
||
| @staticmethod | ||
| def _has_related_node(related: set[Node], app_label: str | None) -> bool: |
There was a problem hiding this comment.
How about:
@staticmethod
def _has_related_node(related: set[Node], app_label: str | None) -> bool:
# For a single-app request only same-app edges decide whether that app has a root or
# a leaf: an app whose newest migration is depended on from another app, or whose
# first migration depends on another app, still has its own target.
return any(node.key.app_label == app_label for node in related)
def _filter_boundary_keys(
self, attr: Literal["parents", "children"], app_label: str | None
) -> list[MigrationKey]:
nodes = self.node_map.values()
if app_label is None:
return sorted([node.key for node in nodes if not getattr(node, attr)])
keys = [
node.key
for node in nodes
if node.key.app_label == app_label
and not self._has_related_node(node.parents, app_label)
]
return sorted(keys)
def root_nodes(self, app_label: str | None = None) -> list[MigrationKey]:
return self._filter_boundary_keys("parents", app_label)
def leaf_nodes(self, app_label: str | None = None) -> list[MigrationKey]:
return self._filter_boundary_keys("children", app_label)
There was a problem hiding this comment.
Good call — adopted. _filter_boundary_keys now takes the attr and handles the global case once up front, and _has_related_node is a plain same-app check without the app_label is None branch. One correction to the sketch: it hardcoded node.parents, so leaf_nodes() would have kept using parents; it uses getattr(node, attr) now. Rebased onto current develop while at it (the changelog was the only conflict).
9f1331f to
1f9e626
Compare
MigrationGraph.leaf_nodes(app_label) and root_nodes(app_label) filtered the candidate nodes by app label but still required a node to have no children (or no parents) at all, across every app. An app whose newest migration was depended on from another app therefore had no leaf, and an app whose first migration depended on another app had no root: `migrate`/`plan` for `A.__latest__` planned nothing and `B.__first__` rolled nothing back. Only same-app edges decide whether a single-app request has a root or a leaf now; the global call keeps the original any-app behaviour. Signed-off-by: inchang-ing <197932532+inchang-ing@users.noreply.github.com>
1f9e626 to
736def4
Compare
| - Migrations now honor ``db_constraint=False`` on ``ForeignKeyField``/``ManyToManyField`` and emit the column (and through table) without a ``FOREIGN KEY`` constraint, matching the runtime schema generator. (#2226) | ||
| - Fix ``field__not=None`` filter returning NULL rows instead of non-NULL rows. (#2353) | ||
| - ``Case``/``When`` and aggregate ``_filter`` expressions now keep the joins for related fields they reference, so e.g. ``When(tournament__name="a", then=1)`` no longer fails with a missing table. (#2354) | ||
| - ``MigrationGraph.leaf_nodes()``/``root_nodes()`` with an app label no longer treat an edge from another app as making that app leafless or rootless. ``migrate``/``plan`` for ``app.__latest__``/``app.__first__`` planned nothing (or rolled nothing back) when the app's newest migration was depended on from another app, or its first migration depended on another app. (#2305) |
There was a problem hiding this comment.
Use #2307 instead of #2305
| from typing import TYPE_CHECKING | ||
|
|
||
| if TYPE_CHECKING: | ||
| from typing import Literal |
There was a problem hiding this comment.
typing is part of the standard library and is already imported on line 5, so there is no need to put it under TYPE_CHECKING.
…h-app-scoped-leaf-root # Conflicts: # CHANGELOG.rst
…h-app-scoped-leaf-root # Conflicts: # CHANGELOG.rst
…h-app-scoped-leaf-root
…h-app-scoped-leaf-root
…h-app-scoped-leaf-root
596f186 to
1e88310
Compare
Signed-off-by: inchang-ing <197932532+inchang-ing@users.noreply.github.com>
Description
MigrationGraph.leaf_nodes(app_label)androot_nodes(app_label)filtered the candidate nodes by app label but still required a node to have no children (or no parents) at all, including those in other apps. An app whose newest migration is depended on from another app therefore had no leaf, and an app whose first migration depends on another app had no root.With an app label, only same-app edges decide whether that app has a root or a leaf now. The global call (
leaf_nodes()/root_nodes()with no argument, used for the full plan) keeps the original any-app behaviour.Motivation and Context
Fixes #2305.
migrate/planforA.__latest__(and bareA) planned nothing:executor.pylooks upgraph.leaf_nodes(target.app_label)and got an empty list.B.__first__rolled nothing back for the same reason withgraph.root_nodes(target.app_label).loader.pyresolves__first__/__latest__dependencies through the same helpers, so a dependency such as("A", "__latest__")could raise the misleadingDependency on app with no migrationsfor an app that does have migrations.How Has This Been Tested?
tests/migrations/test_migration_graph.pyfor the reported graph (a.0002depended on byb.0001):leaf_nodes("a") == [a.0002_second],root_nodes("b") == [b.0001_initial], plus the unchanged global results.developbefore the change and pass after it.pytest tests/migrations/-> 311 passed, 6 skipped.Checklist:
AI disclosure: prepared with the help of an AI coding assistant. I reviewed the change, reproduced the bug, and ran the tests locally.