From: Koichiro Den Date: Tue, 1 Dec 2020 02:53:33 +0000 (-0500) Subject: Raise an exception if any loop or cycle found in a revision graph constructed. X-Git-Tag: rel_1_5_0~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3e178bd6d728fc2b0924fb25427e8e83c3431096;p=thirdparty%2Fsqlalchemy%2Falembic.git Raise an exception if any loop or cycle found in a revision graph constructed. The revision tree is now checked for cycles and loops between revision files when the revision environment is loaded up. Scenarios such as a revision pointing to itself, or a revision that can reach itself via a loop, are handled and will raise the :class:`.CycleDetected` exception when the environment is loaded (expressed from the Alembic commandline as a failure message and nonzero return code). Previously, these situations were silently ignored up front, and the behavior of revision traversal would either be silently incorrect, or would produce errors such as :class:`.RangeNotAncestorError`. Pull request courtesy Koichiro Den. Fixes: #757 Closes: #758 Pull-request: https://github.com/sqlalchemy/alembic/pull/758 Pull-request-sha: a5c5a72c407b74e8b2ca9a1289a85617e154fcb9 Change-Id: I2199caf237870df943e5a8816e6f0beb80a5950a --- diff --git a/alembic/script/revision.py b/alembic/script/revision.py index 22481a08..683d3227 100644 --- a/alembic/script/revision.py +++ b/alembic/script/revision.py @@ -40,6 +40,38 @@ class ResolutionError(RevisionError): self.argument = argument +class CycleDetected(RevisionError): + kind = "Cycle" + + def __init__(self, revisions): + self.revisions = revisions + super(CycleDetected, self).__init__( + "%s is detected in revisions (%s)" + % (self.kind, ", ".join(revisions)) + ) + + +class DependencyCycleDetected(CycleDetected): + kind = "Dependency cycle" + + def __init__(self, revisions): + super(DependencyCycleDetected, self).__init__(revisions) + + +class LoopDetected(CycleDetected): + kind = "Self-loop" + + def __init__(self, revision): + super(LoopDetected, self).__init__([revision]) + + +class DependencyLoopDetected(DependencyCycleDetected, LoopDetected): + kind = "Dependency self-loop" + + def __init__(self, revision): + super(DependencyLoopDetected, self).__init__(revision) + + class RevisionMap(object): """Maintains a map of :class:`.Revision` objects. @@ -115,8 +147,8 @@ class RevisionMap(object): heads = sqlautil.OrderedSet() _real_heads = sqlautil.OrderedSet() - self.bases = () - self._real_bases = () + bases = () + _real_bases = () has_branch_labels = set() has_depends_on = set() @@ -131,15 +163,16 @@ class RevisionMap(object): has_branch_labels.add(revision) if revision.dependencies: has_depends_on.add(revision) - heads.add(revision.revision) - _real_heads.add(revision.revision) + heads.add(revision) + _real_heads.add(revision) if revision.is_base: - self.bases += (revision.revision,) + bases += (revision,) if revision._is_real_base: - self._real_bases += (revision.revision,) + _real_bases += (revision,) # add the branch_labels to the map_. We'll need these # to resolve the dependencies. + rev_map = map_.copy() for revision in has_branch_labels: self._map_branch_labels(revision, map_) @@ -156,12 +189,49 @@ class RevisionMap(object): down_revision = map_[downrev] down_revision.add_nextrev(rev) if downrev in rev._versioned_down_revisions: - heads.discard(downrev) - _real_heads.discard(downrev) + heads.discard(down_revision) + _real_heads.discard(down_revision) + + if rev_map: + if not heads or not bases: + raise CycleDetected(rev_map.keys()) + total_space = { + rev.revision + for rev in self._iterate_related_revisions( + lambda r: r._versioned_down_revisions, heads, map_=rev_map + ) + }.intersection( + rev.revision + for rev in self._iterate_related_revisions( + lambda r: r.nextrev, bases, map_=rev_map + ) + ) + deleted_revs = set(rev_map.keys()) - total_space + if deleted_revs: + raise CycleDetected(sorted(deleted_revs)) + + if not _real_heads or not _real_bases: + raise DependencyCycleDetected(rev_map.keys()) + total_space = { + rev.revision + for rev in self._iterate_related_revisions( + lambda r: r._all_down_revisions, _real_heads, map_=rev_map + ) + }.intersection( + rev.revision + for rev in self._iterate_related_revisions( + lambda r: r._all_nextrev, _real_bases, map_=rev_map + ) + ) + deleted_revs = set(rev_map.keys()) - total_space + if deleted_revs: + raise DependencyCycleDetected(sorted(deleted_revs)) map_[None] = map_[()] = None - self.heads = tuple(heads) - self._real_heads = tuple(_real_heads) + self.heads = tuple(rev.revision for rev in heads) + self._real_heads = tuple(rev.revision for rev in _real_heads) + self.bases = tuple(rev.revision for rev in bases) + self._real_bases = tuple(rev.revision for rev in _real_bases) for revision in has_branch_labels: self._add_branches(revision, map_, map_branch_labels=False) @@ -964,6 +1034,11 @@ class Revision(object): def __init__( self, revision, down_revision, dependencies=None, branch_labels=None ): + if down_revision and revision in down_revision: + raise LoopDetected(revision) + elif dependencies is not None and revision in dependencies: + raise DependencyLoopDetected(revision) + self.verify_rev_id(revision) self.revision = revision self.down_revision = tuple_rev_as_scalar(down_revision) diff --git a/docs/build/unreleased/757.rst b/docs/build/unreleased/757.rst new file mode 100644 index 00000000..1b6d5300 --- /dev/null +++ b/docs/build/unreleased/757.rst @@ -0,0 +1,14 @@ +.. change:: + :tags: feature, versioning + :tickets: 757 + + The revision tree is now checked for cycles and loops between revision + files when the revision environment is loaded up. Scenarios such as a + revision pointing to itself, or a revision that can reach itself via a + loop, are handled and will raise the :class:`.CycleDetected` exception when + the environment is loaded (expressed from the Alembic commandline as a + failure message and nonzero return code). Previously, these situations were + silently ignored up front, and the behavior of revision traversal would + either be silently incorrect, or would produce errors such as + :class:`.RangeNotAncestorError`. Pull request courtesy Koichiro Den. + diff --git a/tests/test_revision.py b/tests/test_revision.py index bf433f51..767baa6e 100644 --- a/tests/test_revision.py +++ b/tests/test_revision.py @@ -1,3 +1,7 @@ +from alembic.script.revision import CycleDetected +from alembic.script.revision import DependencyCycleDetected +from alembic.script.revision import DependencyLoopDetected +from alembic.script.revision import LoopDetected from alembic.script.revision import MultipleHeads from alembic.script.revision import Revision from alembic.script.revision import RevisionError @@ -125,6 +129,39 @@ class APITest(TestBase): "heads", ) + def test_get_revisions_head_multiple(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", ("a",)), + Revision("c1", ("b",)), + Revision("c2", ("b",)), + ] + ) + assert_raises_message( + MultipleHeads, + "Multiple heads are present", + map_.get_revisions, + "head", + ) + + def test_get_revisions_heads_multiple(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", ("a",)), + Revision("c1", ("b",)), + Revision("c2", ("b",)), + ] + ) + eq_( + map_.get_revisions("heads"), + ( + map_._revision_map["c1"], + map_._revision_map["c2"], + ), + ) + def test_get_revision_base_multiple(self): map_ = RevisionMap( lambda: [ @@ -1213,3 +1250,239 @@ class DepResolutionFailedTest(DownIterateTest): assert_raises_message( RevisionError, "Dependency resolution failed;", list, iter_ ) + + +class InvalidRevisionMapTest(TestBase): + def _assert_raises_revision_map(self, map_, except_cls, msg): + assert_raises_message(except_cls, msg, lambda: map_._revision_map) + + def _assert_raises_revision_map_loop(self, map_, revision): + self._assert_raises_revision_map( + map_, + LoopDetected, + r"^Self-loop is detected in revisions \(%s\)$" % revision, + ) + + def _assert_raises_revision_map_dep_loop(self, map_, revision): + self._assert_raises_revision_map( + map_, + DependencyLoopDetected, + r"^Dependency self-loop is detected in revisions \(%s\)$" + % revision, + ) + + def _assert_raises_revision_map_cycle(self, map_, revisions): + self._assert_raises_revision_map( + map_, + CycleDetected, + r"^Cycle is detected in revisions \(\(%s\)\(, \)?\)+$" + % "|".join(revisions), + ) + + def _assert_raises_revision_map_dep_cycle(self, map_, revisions): + self._assert_raises_revision_map( + map_, + DependencyCycleDetected, + r"^Dependency cycle is detected in revisions \(\(%s\)\(, \)?\)+$" + % "|".join(revisions), + ) + + +class GraphWithLoopTest(InvalidRevisionMapTest): + def test_revision_map_solitary_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", "a"), + ] + ) + self._assert_raises_revision_map_loop(map_, "a") + + def test_revision_map_base_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", "a"), + Revision("b", "a"), + Revision("c", "b"), + ] + ) + self._assert_raises_revision_map_loop(map_, "a") + + def test_revision_map_head_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", "a"), + Revision("c", ("b", "c")), + ] + ) + self._assert_raises_revision_map_loop(map_, "c") + + def test_revision_map_branch_point_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", ("a", "b")), + Revision("c1", "b"), + Revision("c2", "b"), + ] + ) + self._assert_raises_revision_map_loop(map_, "b") + + def test_revision_map_merge_point_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b1", "a"), + Revision("b2", "a"), + Revision("c", ("b1", "b2", "c")), + ] + ) + self._assert_raises_revision_map_loop(map_, "c") + + def test_revision_map_solitary_dependency_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", (), dependencies="a"), + ] + ) + self._assert_raises_revision_map_dep_loop(map_, "a") + + def test_revision_map_base_dependency_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", (), dependencies="a"), + Revision("b", "a"), + Revision("c", "b"), + ] + ) + self._assert_raises_revision_map_dep_loop(map_, "a") + + def test_revision_map_head_dep_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", "a"), + Revision("c", "b", dependencies="c"), + ] + ) + self._assert_raises_revision_map_dep_loop(map_, "c") + + def test_revision_map_branch_point_dep_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", "a", dependencies="b"), + Revision("c1", "b"), + Revision("c2", "b"), + ] + ) + self._assert_raises_revision_map_dep_loop(map_, "b") + + def test_revision_map_merge_point_dep_loop(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b1", "a"), + Revision("b2", "a"), + Revision("c", ("b1", "b2"), dependencies="c"), + ] + ) + self._assert_raises_revision_map_dep_loop(map_, "c") + + +class GraphWithCycleTest(InvalidRevisionMapTest): + def test_revision_map_simple_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", "c"), + Revision("b", "a"), + Revision("c", "b"), + ] + ) + self._assert_raises_revision_map_cycle(map_, ["a", "b", "c"]) + + def test_revision_map_extra_simple_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", "c"), + Revision("b", "a"), + Revision("c", "b"), + Revision("d", ()), + Revision("e", "d"), + ] + ) + self._assert_raises_revision_map_cycle(map_, ["a", "b", "c"]) + + def test_revision_map_lower_simple_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", "c"), + Revision("b", "a"), + Revision("c", "b"), + Revision("d", "c"), + Revision("e", "d"), + ] + ) + self._assert_raises_revision_map_cycle(map_, ["a", "b", "c", "d", "e"]) + + def test_revision_map_upper_simple_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", "a"), + Revision("c", ("b", "e")), + Revision("d", "c"), + Revision("e", "d"), + ] + ) + self._assert_raises_revision_map_cycle(map_, ["a", "b", "c", "d", "e"]) + + def test_revision_map_simple_dep_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", (), dependencies="c"), + Revision("b", "a"), + Revision("c", "b"), + ] + ) + self._assert_raises_revision_map_dep_cycle(map_, ["a", "b", "c"]) + + def test_revision_map_extra_simple_dep_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", (), dependencies="c"), + Revision("b", "a"), + Revision("c", "b"), + Revision("d", ()), + Revision("e", "d"), + ] + ) + self._assert_raises_revision_map_dep_cycle(map_, ["a", "b", "c"]) + + def test_revision_map_lower_simple_dep_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", (), dependencies="c"), + Revision("b", "a"), + Revision("c", "b"), + Revision("d", "c"), + Revision("e", "d"), + ] + ) + self._assert_raises_revision_map_dep_cycle( + map_, ["a", "b", "c", "d", "e"] + ) + + def test_revision_map_upper_simple_dep_cycle(self): + map_ = RevisionMap( + lambda: [ + Revision("a", ()), + Revision("b", "a"), + Revision("c", "b", dependencies="e"), + Revision("d", "c"), + Revision("e", "d"), + ] + ) + self._assert_raises_revision_map_dep_cycle( + map_, ["a", "b", "c", "d", "e"] + )