From: Mike Bayer Date: Sun, 10 Jul 2016 20:23:21 +0000 (-0400) Subject: Detect downgrades over dependencies distinctly from unmerge X-Git-Tag: rel_0_8_7~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=111b1c13ae5a4330ebf0ed6d21c4a6f6bcd9f40e;p=thirdparty%2Fsqlalchemy%2Falembic.git 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 e811a36c..352737ec 100644 --- 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 f96232cf..0470ab3b 100644 --- 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 b22b177c..a8417ac0 100644 --- 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 + + -> 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]) )