From 64492e067bd8911915a19736d473cf735dcbd867 Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Tue, 5 Dec 2023 16:45:19 -0800 Subject: [PATCH 1/4] Fix downgrade when normalized down revisions have overlap via depends_on Fixes #1373 --- alembic/runtime/migration.py | 12 +++++++++++- tests/test_version_traversal.py | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/alembic/runtime/migration.py b/alembic/runtime/migration.py index 24e3d644..2cbc42a2 100644 --- a/alembic/runtime/migration.py +++ b/alembic/runtime/migration.py @@ -1168,7 +1168,17 @@ def _unmerge_to_revisions(self, heads: Set[str]) -> Tuple[str, ...]: } return tuple(set(self.to_revisions).difference(ancestors)) else: - return self.to_revisions + # For each revision we plan to return, compute its ancestors (excluding self), + # and remove those from the final output since they are already accounted for. + ancestors = { + r.revision + for to_revision in self.to_revisions + for r in self.revision_map._get_ancestor_nodes( + self.revision_map.get_revisions(to_revision), check=False + ) + if r.revision != to_revision + } + return tuple(set(self.to_revisions).difference(ancestors)) def unmerge_branch_idents( self, heads: Set[str] diff --git a/tests/test_version_traversal.py b/tests/test_version_traversal.py index 0628f3fd..6d3921a9 100644 --- a/tests/test_version_traversal.py +++ b/tests/test_version_traversal.py @@ -1176,6 +1176,40 @@ def test_dependencies_are_normalized(self): ) +class DependsOnBranchTestFive(MigrationTest): + @classmethod + def setup_class(cls): + """ + issue #1373 + + Structure:: + + -> a1 ------+ + ^ | + | +-> bmerge + | | + +-- b1 --+ + """ + cls.env = env = staging_env() + cls.a1 = env.generate_revision("a1", "->a1") + cls.b1 = env.generate_revision("b1", "->b1", head="base", depends_on="a1") + cls.bmerge = env.generate_revision("bmerge"," bmerge", head=[cls.a1.revision, cls.b1.revision]) + + @classmethod + def teardown_class(cls): + clear_staging_env() + + def test_downgrade_to_depends_on(self): + # Upgrade from a1 to b1 just has heads={"b1"}. + self._assert_upgrade(self.b1.revision, self.a1.revision, expected=[self.up_(self.b1)], expected_heads={self.b1.revision}) + + # Upgrade from b1 to bmerge just has {"bmerge"} + self._assert_upgrade(self.bmerge.revision, self.b1.revision, expected=[self.up_(self.bmerge)], expected_heads={self.bmerge.revision}) + + # Downgrading from bmerge to a1 should return back to heads={"b1"}. + self._assert_downgrade(self.a1.revision, self.bmerge.revision, expected=[self.down_(self.bmerge)], expected_heads={self.b1.revision}) + + class DependsOnBranchLabelTest(MigrationTest): @classmethod def setup_class(cls): From 39f5ad05e08021c0c67fb7334b7bf5c25788c8c6 Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Wed, 6 Dec 2023 16:35:36 -0800 Subject: [PATCH 2/4] fix lint --- alembic/runtime/migration.py | 5 +++-- tests/test_version_traversal.py | 29 ++++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/alembic/runtime/migration.py b/alembic/runtime/migration.py index 2cbc42a2..1c79ba99 100644 --- a/alembic/runtime/migration.py +++ b/alembic/runtime/migration.py @@ -1168,8 +1168,9 @@ def _unmerge_to_revisions(self, heads: Set[str]) -> Tuple[str, ...]: } return tuple(set(self.to_revisions).difference(ancestors)) else: - # For each revision we plan to return, compute its ancestors (excluding self), - # and remove those from the final output since they are already accounted for. + # for each revision we plan to return, compute its ancestors + # (excluding self), and remove those from the final output since + # they are already accounted for. ancestors = { r.revision for to_revision in self.to_revisions diff --git a/tests/test_version_traversal.py b/tests/test_version_traversal.py index 6d3921a9..32ce9511 100644 --- a/tests/test_version_traversal.py +++ b/tests/test_version_traversal.py @@ -1192,8 +1192,12 @@ def setup_class(cls): """ cls.env = env = staging_env() cls.a1 = env.generate_revision("a1", "->a1") - cls.b1 = env.generate_revision("b1", "->b1", head="base", depends_on="a1") - cls.bmerge = env.generate_revision("bmerge"," bmerge", head=[cls.a1.revision, cls.b1.revision]) + cls.b1 = env.generate_revision( + "b1", "->b1", head="base", depends_on="a1" + ) + cls.bmerge = env.generate_revision( + "bmerge", "bmerge", head=[cls.a1.revision, cls.b1.revision] + ) @classmethod def teardown_class(cls): @@ -1201,13 +1205,28 @@ def teardown_class(cls): def test_downgrade_to_depends_on(self): # Upgrade from a1 to b1 just has heads={"b1"}. - self._assert_upgrade(self.b1.revision, self.a1.revision, expected=[self.up_(self.b1)], expected_heads={self.b1.revision}) + self._assert_upgrade( + self.b1.revision, + self.a1.revision, + [self.up_(self.b1)], + {self.b1.revision}, + ) # Upgrade from b1 to bmerge just has {"bmerge"} - self._assert_upgrade(self.bmerge.revision, self.b1.revision, expected=[self.up_(self.bmerge)], expected_heads={self.bmerge.revision}) + self._assert_upgrade( + self.bmerge.revision, + self.b1.revision, + [self.up_(self.bmerge)], + {self.bmerge.revision}, + ) # Downgrading from bmerge to a1 should return back to heads={"b1"}. - self._assert_downgrade(self.a1.revision, self.bmerge.revision, expected=[self.down_(self.bmerge)], expected_heads={self.b1.revision}) + self._assert_downgrade( + self.a1.revision, + self.bmerge.revision, + [self.down_(self.bmerge)], + {self.b1.revision}, + ) class DependsOnBranchLabelTest(MigrationTest): From da30dbfe748ec33a1203164d587981106d1adf7d Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Mon, 11 Dec 2023 18:47:13 -0800 Subject: [PATCH 3/4] address concerns --- tests/test_version_traversal.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/test_version_traversal.py b/tests/test_version_traversal.py index 32ce9511..09816dff 100644 --- a/tests/test_version_traversal.py +++ b/tests/test_version_traversal.py @@ -1212,7 +1212,7 @@ def test_downgrade_to_depends_on(self): {self.b1.revision}, ) - # Upgrade from b1 to bmerge just has {"bmerge"} + # Upgrade from b1 to bmerge just has {"bmerge"}. self._assert_upgrade( self.bmerge.revision, self.b1.revision, @@ -1228,6 +1228,14 @@ def test_downgrade_to_depends_on(self): {self.b1.revision}, ) + # Downgrading from bmerge to b1 also returns back to heads={"b1"}. + self._assert_downgrade( + self.b1.revision, + self.bmerge.revision, + [self.down_(self.bmerge)], + {self.b1.revision}, + ) + class DependsOnBranchLabelTest(MigrationTest): @classmethod From dc8c7f8f7f8bc8e753aac8b8a1d6d70d79b12573 Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Tue, 12 Dec 2023 14:05:25 -0800 Subject: [PATCH 4/4] add docs --- docs/build/unreleased/1373.rst | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 docs/build/unreleased/1373.rst diff --git a/docs/build/unreleased/1373.rst b/docs/build/unreleased/1373.rst new file mode 100644 index 00000000..556b580d --- /dev/null +++ b/docs/build/unreleased/1373.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: bug, versioning + :tickets: 1373 + + Fixed bug in versioning model where a downgrade across a revision with two + down revisions with one down revision depending on the other, would produce + an erroneous state in the alembic_version table, making upgrades impossible + without manually repairing the table.