Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EnvironmentContext.get_revision_arguments() and EnvironmentContext.get_head_revisions() returns incorrect values #482

Closed
sqlalchemy-bot opened this issue Feb 15, 2018 · 7 comments
Labels
bug Something isn't working low priority

Comments

@sqlalchemy-bot
Copy link

Migrated issue, originally created by Dimitry Lukashov

When there are multiple active heads and the alembic_version table has multiple records
Script.as_revision_number() should return a tuple, instead it returns only returns the hex identifier of one of the heads, making EnvironmentContext.get_head_revisions() useless and EnvironmentContext.get_revision_arguments() not very helpful when there are multiple heads.

Here is my proposed solution:

--- a/alembic/runtime/environment.py
+++ b/alembic/runtime/environment.py
@@ -201,8 +201,9 @@ class EnvironmentContext(util.ModuleClsProxy):
         ``upgrade`` or ``downgrade`` command.
 
         If it was specified as ``head``, the actual
-        version number is returned; if specified
-        as ``base``, ``None`` is returned.
+        version number is returned; if specified as
+        ``heads`` a tuple of version numbers is returned;
+        if specified as ``base``, ``None`` is returned.
 
         This function does not require that the :class:`.MigrationContext`
         has been configured.
diff --git a/alembic/script/base.py b/alembic/script/base.py
index 2e6e0fb..419fce8 100644
--- a/alembic/script/base.py
+++ b/alembic/script/base.py
@@ -228,7 +228,7 @@ class ScriptDirectory(object):
             return self.revision_map.get_revision(id_)
 
     def as_revision_number(self, id_):
-        """Convert a symbolic revision, i.e. 'head' or 'base', into
+        """Convert a symbolic revision, i.e. 'head', 'heads' or 'base', into
         an actual revision number."""
 
         with self._catch_revision_errors():
@@ -237,6 +237,9 @@ class ScriptDirectory(object):
         if not rev:
             # convert () to None
             return None
+        elif id_ == 'heads':
+            # expects a tuple
+            return rev
         else:
             return rev[0]


@sqlalchemy-bot
Copy link
Author

Dimitry Lukashov wrote:

PS I tried to submit a pull request and got a repository access denied. error.

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

bitbucket's pull request feature gave you that error? I also don't accept PRs without tests in any case. it appears that get_head_revisions() was added without any tests at that time. there is no get_revision_arguments() method so I don't know what you're referring to (there's a get_revision_argument() method, which I don't see how that is related - test cases tell all, thanks).

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

get_head_revisions() returning a tuple in all cases is going to break an application that expected this to be a straight string earlier, even though it is documented as returning a tuple. Hopefully this will not have a large impact as the method is entirely wrong right now returning a scalar value.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

Repair as_revision_number to return a tuple for "heads"

Fixed bug where the :meth:.Script.as_revision_number method
did not accommodate for the 'heads' identifier, which in turn
caused the :meth:.EnvironmentContext.get_head_revisions
and :meth:.EnvironmentContext.get_revision_argument methods
to be not usable when multiple heads were present.
The :meth:.EnvironmentContext.get_head_revisions method returns
a tuple in all cases as documented.

Change-Id: I085d9b6c3f4ceafd6828d24983768a3d3916ce00
Fixes: #482

3c726b2

@sqlalchemy-bot
Copy link
Author

Changes by Michael Bayer (@zzzeek):

  • changed status to closed

@sqlalchemy-bot
Copy link
Author

Dimitry Lukashov wrote:

@zzzeek thank you very much! Do you normally allow others to PR, if yes, then it must be a problem on my end. Next time I will add test cases!

@sqlalchemy-bot sqlalchemy-bot added low priority bug Something isn't working labels Nov 27, 2018
vvvrrooomm pushed a commit to vvvrrooomm/alembic that referenced this issue Jan 10, 2019
Fixed bug where the :meth:`.Script.as_revision_number` method
did not accommodate for the 'heads' identifier, which in turn
caused the :meth:`.EnvironmentContext.get_head_revisions`
and :meth:`.EnvironmentContext.get_revision_argument` methods
to be not usable when multiple heads were present.
The :meth:.`EnvironmentContext.get_head_revisions` method returns
a tuple in all cases as documented.

Change-Id: I085d9b6c3f4ceafd6828d24983768a3d3916ce00
Fixes: sqlalchemy#482
vvvrrooomm pushed a commit to vvvrrooomm/alembic that referenced this issue Jan 10, 2019
Fixed bug where the :meth:`.Script.as_revision_number` method
did not accommodate for the 'heads' identifier, which in turn
caused the :meth:`.EnvironmentContext.get_head_revisions`
and :meth:`.EnvironmentContext.get_revision_argument` methods
to be not usable when multiple heads were present.
The :meth:.`EnvironmentContext.get_head_revisions` method returns
a tuple in all cases as documented.

Change-Id: I085d9b6c3f4ceafd6828d24983768a3d3916ce00
Fixes: sqlalchemy#482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority
Projects
None yet
Development

No branches or pull requests

1 participant