Detect downgrades over dependencies distinctly from unmerge

Previously, a downgrade to a version that is also a dependency
to another branch, where that branch is advanced beyond
the target, would fail to be downgraded, as this would
be detected as an "umerge" even though the target version
to be INSERTed would not be present.
The patch replaces the existing heuristic
that checks for "delete" with a new one that calculates
a potential "unmerge" fully, and returns False only if we in
fact could do an unmerge.

Also change the string display of a version so that we don't
display misleading target versions that might not actually
be getting invoked.

Fixes: #377
Change-Id: I7420dd7adbd9ccf9ca85b56d9a792a85c40f3454

diff --git a/alembic/runtime/migration.py b/alembic/runtime/migration.py
index e811a36ca26f00ea021435cc829e54f983e1581d..352737ecb74bdda2241fac806d766f5539c14a18 100644 (file)
--- a/alembic/runtime/migration.py
+++ b/alembic/runtime/migration.py
@@ -521,16 +521,16 @@ class MigrationStep(object):
     def short_log(self):
         return "%s %s -> %s" % (
             self.name,
-            util.format_as_comma(self.from_revisions),
-            util.format_as_comma(self.to_revisions)
+            util.format_as_comma(self.from_revisions_no_deps),
+            util.format_as_comma(self.to_revisions_no_deps)
         )
 
     def __str__(self):
         if self.doc:
             return "%s %s -> %s, %s" % (
                 self.name,
-                util.format_as_comma(self.from_revisions),
-                util.format_as_comma(self.to_revisions),
+                util.format_as_comma(self.from_revisions_no_deps),
+                util.format_as_comma(self.to_revisions_no_deps),
                 self.doc
             )
         else:
@@ -547,6 +547,11 @@ class RevisionStep(MigrationStep):
         else:
             self.migration_fn = revision.module.downgrade
 
+    def __repr__(self):
+        return "RevisionStep(%r, is_upgrade=%r)" % (
+            self.revision.revision, self.is_upgrade
+        )
+
     def __eq__(self, other):
         return isinstance(other, RevisionStep) and \
             other.revision == self.revision and \
@@ -563,6 +568,13 @@ class RevisionStep(MigrationStep):
         else:
             return (self.revision.revision, )
 
+    @property
+    def from_revisions_no_deps(self):
+        if self.is_upgrade:
+            return self.revision._versioned_down_revisions
+        else:
+            return (self.revision.revision, )
+
     @property
     def to_revisions(self):
         if self.is_upgrade:
@@ -570,11 +582,23 @@ class RevisionStep(MigrationStep):
         else:
             return self.revision._all_down_revisions
 
+    @property
+    def to_revisions_no_deps(self):
+        if self.is_upgrade:
+            return (self.revision.revision, )
+        else:
+            return self.revision._versioned_down_revisions
+
     @property
     def _has_scalar_down_revision(self):
         return len(self.revision._all_down_revisions) == 1
 
     def should_delete_branch(self, heads):
+        """A delete is when we are a. in a downgrade and b.
+        we are going to the "base" or we are going to a version that
+        is implied as a dependency on another version that is remaining.
+
+        """
         if not self.is_downgrade:
             return False
 
@@ -582,45 +606,15 @@ class RevisionStep(MigrationStep):
             return False
 
         downrevs = self.revision._all_down_revisions
+
         if not downrevs:
             # is a base
             return True
-        elif len(downrevs) == 1:
-            downrev = self.revision_map.get_revision(downrevs[0])
-
-            if not downrev._is_real_branch_point:
-                return False
-
-            descendants = set(
-                r.revision for r in self.revision_map._get_descendant_nodes(
-                    self.revision_map.get_revisions(downrev._all_nextrev),
-                    check=False
-                )
-            )
-
-            # the downrev is a branchpoint, and other members or descendants
-            # of the branch are still in heads; so delete this branch.
-            # the reason this occurs is because traversal tries to stay
-            # fully on one branch down to the branchpoint before starting
-            # the other; so if we have a->b->(c1->d1->e1, c2->d2->e2),
-            # on a downgrade from the top we may go e1, d1, c1, now heads
-            # are at c1 and e2, with the current method, we don't know that
-            # "e2" is important unless we get all descendants of c1/c2
-
-            if len(descendants.intersection(heads).difference(
-                    [self.revision.revision])):
-
-            # TODO: this doesn't work; make sure tests are here to ensure
-            # this fails
-            #if len(downrev._all_nextrev.intersection(heads).difference(
-            #        [self.revision.revision])):
-
-                return True
-            else:
-                return False
         else:
-            # is a merge point
-            return False
+            # determine what the ultimate "to_revisions" for an
+            # unmerge would be.  If there are none, then we're a delete.
+            to_revisions = self._unmerge_to_revisions(heads)
+            return not to_revisions
 
     def merge_branch_idents(self, heads):
         other_heads = set(heads).difference(self.from_revisions)
@@ -644,7 +638,7 @@ class RevisionStep(MigrationStep):
             self.to_revisions[0]
         )
 
-    def unmerge_branch_idents(self, heads):
+    def _unmerge_to_revisions(self, heads):
         other_heads = set(heads).difference([self.revision.revision])
         if other_heads:
             ancestors = set(
@@ -654,9 +648,12 @@ class RevisionStep(MigrationStep):
                     check=False
                 )
             )
-            to_revisions = list(set(self.to_revisions).difference(ancestors))
+            return list(set(self.to_revisions).difference(ancestors))
         else:
-            to_revisions = self.to_revisions
+            return self.to_revisions
+
+    def unmerge_branch_idents(self, heads):
+        to_revisions = self._unmerge_to_revisions(heads)
 
         return (
             # update from rev, update to rev, insert revs
@@ -757,6 +754,14 @@ class StampStep(MigrationStep):
     def to_revisions(self):
         return self.to_
 
+    @property
+    def from_revisions_no_deps(self):
+        return self.from_
+
+    @property
+    def to_revisions_no_deps(self):
+        return self.to_
+
     @property
     def delete_version_num(self):
         assert len(self.from_) == 1

diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst
index f96232cf383332ce1ef05fe07b83fd84de5b0849..0470ab3bfc1a1620bd671e7b6e8c79a19ab6c301 100644 (file)
--- a/docs/build/changelog.rst
+++ b/docs/build/changelog.rst
@@ -6,6 +6,15 @@ Changelog
 .. changelog::
     :version: 0.8.7
 
+    .. change::
+      :tags: bug, versioning
+      :tickets: 377
+
+      Fixed bug where a downgrade to a version that is also a dependency
+      to a different branch would fail, as the system attempted to treat
+      this as an "unmerge" of a merge point, when in fact it doesn't have
+      the other side of the merge point available for update.
+
     .. change::
       :tags: bug, versioning
       :tickets: 378

diff --git a/tests/test_version_traversal.py b/tests/test_version_traversal.py
index b22b177cf345003b8e31a261663e0fdf74a074a1..a8417ac02df304ce366569e2dd1e2e9f49cdf35d 100644 (file)
--- a/tests/test_version_traversal.py
+++ b/tests/test_version_traversal.py
@@ -408,7 +408,6 @@ class BranchFrom3WayMergepointTest(MigrationTest):
         )
 
     def test_mergepoint_to_only_one_side_downgrade(self):
-
         self._assert_downgrade(
             self.b1.revision,
             (self.d3.revision, self.d2.revision, self.d1.revision),
@@ -629,29 +628,56 @@ class DependsOnBranchTestTwo(MigrationTest):
 
     @classmethod
     def setup_class(cls):
+        """
+        a1 ---+
+              |
+        a2 ---+--> amerge
+              |
+        a3 ---+
+         ^
+         |
+         +---------------------------+
+                                     |
+        b1 ---+                      |
+              +--> bmerge        overmerge / d1
+        b2 ---+                     |  |
+         ^                          |  |
+         |                          |  |
+         +--------------------------+  |
+                                       |
+         +-----------------------------+
+         |
+         v
+        c1 ---+
+              |
+        c2 ---+--> cmerge
+              |
+        c3 ---+
+
+        """
         cls.env = env = staging_env()
-        cls.a1 = env.generate_revision(util.rev_id(), '->a1', head='base')
-        cls.a2 = env.generate_revision(util.rev_id(), '->a2', head='base')
-        cls.a3 = env.generate_revision(util.rev_id(), '->a3', head='base')
-        cls.amerge = env.generate_revision(util.rev_id(), 'amerge', head=[
+        cls.a1 = env.generate_revision("a1", '->a1', head='base')
+        cls.a2 = env.generate_revision("a2", '->a2', head='base')
+        cls.a3 = env.generate_revision("a3", '->a3', head='base')
+        cls.amerge = env.generate_revision("amerge", 'amerge', head=[
             cls.a1.revision, cls.a2.revision, cls.a3.revision
         ])
 
-        cls.b1 = env.generate_revision(util.rev_id(), '->b1', head='base')
-        cls.b2 = env.generate_revision(util.rev_id(), '->b2', head='base')
-        cls.bmerge = env.generate_revision(util.rev_id(), 'bmerge', head=[
+        cls.b1 = env.generate_revision("b1", '->b1', head='base')
+        cls.b2 = env.generate_revision("b2", '->b2', head='base')
+        cls.bmerge = env.generate_revision("bmerge", 'bmerge', head=[
             cls.b1.revision, cls.b2.revision
         ])
 
-        cls.c1 = env.generate_revision(util.rev_id(), '->c1', head='base')
-        cls.c2 = env.generate_revision(util.rev_id(), '->c2', head='base')
-        cls.c3 = env.generate_revision(util.rev_id(), '->c3', head='base')
-        cls.cmerge = env.generate_revision(util.rev_id(), 'cmerge', head=[
+        cls.c1 = env.generate_revision("c1", '->c1', head='base')
+        cls.c2 = env.generate_revision("c2", '->c2', head='base')
+        cls.c3 = env.generate_revision("c3", '->c3', head='base')
+        cls.cmerge = env.generate_revision("cmerge", 'cmerge', head=[
             cls.c1.revision, cls.c2.revision, cls.c3.revision
         ])
 
         cls.d1 = env.generate_revision(
-            util.rev_id(), 'overmerge',
+            "d1", 'o',
             head="base",
             depends_on=[
                 cls.a3.revision, cls.b2.revision, cls.c1.revision
@@ -672,19 +698,38 @@ class DependsOnBranchTestTwo(MigrationTest):
             self.b2.revision, heads,
             [self.down_(self.bmerge), self.down_(self.d1)],
             set([
-                self.amerge.revision, self.b2.revision,
-                self.b1.revision, self.cmerge.revision])
+                self.amerge.revision,
+                self.b1.revision, self.cmerge.revision, self.b2.revision])
         )
 
+        # start with those heads..
         heads = [
-            self.amerge.revision, self.b2.revision,
+            self.amerge.revision, self.d1.revision,
             self.b1.revision, self.cmerge.revision]
+
+        # downgrade d1...
+        self._assert_downgrade(
+            "d1@base", heads,
+            [self.down_(self.d1)],
+
+            # b2 has to be INSERTed, because it was implied by d1
+            set([
+                self.amerge.revision, self.b1.revision,
+                self.b2.revision, self.cmerge.revision])
+        )
+
+        # start with those heads ...
+        heads = [
+            self.amerge.revision, self.b1.revision,
+            self.b2.revision, self.cmerge.revision
+        ]
+
         self._assert_downgrade(
             "base", heads,
             [
                 self.down_(self.amerge), self.down_(self.a1),
                 self.down_(self.a2), self.down_(self.a3),
-                self.down_(self.b2), self.down_(self.b1),
+                self.down_(self.b1), self.down_(self.b2),
                 self.down_(self.cmerge), self.down_(self.c1),
                 self.down_(self.c2), self.down_(self.c3)
             ],
@@ -692,6 +737,47 @@ class DependsOnBranchTestTwo(MigrationTest):
         )
 
 
+class DependsOnBranchTestThree(MigrationTest):
+
+    @classmethod
+    def setup_class(cls):
+        """
+        issue #377
+
+        <base> -> a1 --+--> a2 -------> a3
+                       |     ^          |
+                       |     |   +------+
+                       |     |   |
+                       |     +---|------+
+                       |         |      |
+                       |         v      |
+                       +-------> b1 --> b2 --> b3
+
+        """
+        cls.env = env = staging_env()
+        cls.a1 = env.generate_revision("a1", '->a1', head='base')
+        cls.a2 = env.generate_revision("a2", '->a2')
+
+        cls.b1 = env.generate_revision("b1", '->b1', head='base')
+        cls.b2 = env.generate_revision("b2", '->b2', depends_on='a2', head='b1')
+        cls.b3 = env.generate_revision("b3", '->b3', head='b2')
+
+        cls.a3 = env.generate_revision("a3", '->a3', head='a2', depends_on='b1')
+
+    def test_downgrade_over_crisscross(self):
+        # this state was not possible prior to
+        # #377.  a3 would be considered half of a merge point
+        # between a3 and b2, and the head would be forced down
+        # to b1.   In this test however, we're not allowed to remove
+        # b2 because a2 is dependent on it, hence we add the ability
+        # to remove half of a merge point.
+        self._assert_downgrade(
+            'b1', ['a3', 'b2'],
+            [self.down_(self.a3), self.down_(self.b2)],
+            set(['a2', 'b1'])
+        )
+
+
 class DependsOnBranchLabelTest(MigrationTest):
     @classmethod
     def setup_class(cls):
@@ -731,7 +817,7 @@ class DependsOnBranchLabelTest(MigrationTest):
                 self.up_(self.b2),
                 self.up_(self.c2),
             ],
-            set([self.c2.revision, ])
+            set([self.c2.revision])
         )