From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Sat, 4 Mar 2017 21:42:30 +0000 (-0500)
Subject: Warn on non-Connection present and accommodate for Engine
X-Git-Tag: rel_0_9_2~7
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=adf0a7d1ea881907bdf546778dae72e6ac0a3027;p=thirdparty%2Fsqlalchemy%2Falembic.git

Warn on non-Connection present and accommodate for Engine

A warning is emitted when an object that's not a
:class:`~sqlalchemy.engine.Connection` is passed to
:meth:`.EnvironmentContext.configure`.  For the case of a
:class:`~sqlalchemy.engine.Engine` passed, the check for "in transaction"
introduced in version 0.9.0 has been relaxed to work in the case of an
attribute error, as some users appear to be passing an
:class:`~sqlalchemy.engine.Engine` and not a
:class:`~sqlalchemy.engine.Connection`.

Change-Id: I95ef38955c00511d3055362a03284fb91677595f
Fixes: #419
---

diff --git a/alembic/runtime/migration.py b/alembic/runtime/migration.py
index d13b9996..13e6ebe4 100644
--- a/alembic/runtime/migration.py
+++ b/alembic/runtime/migration.py
@@ -6,6 +6,7 @@ from sqlalchemy import MetaData, Table, Column, String, literal_column,\
     PrimaryKeyConstraint
 from sqlalchemy.engine.strategies import MockEngineStrategy
 from sqlalchemy.engine import url as sqla_url
+from sqlalchemy.engine import Connection
 
 from ..util.compat import callable, EncodedIO
 from .. import ddl, util
@@ -152,6 +153,11 @@ class MigrationContext(object):
             opts = {}
 
         if connection:
+            if not isinstance(connection, Connection):
+                util.warn(
+                    "'connection' argument to configure() is expected "
+                    "to be a sqlalchemy.engine.Connection instance, "
+                    "got %r" % connection)
             dialect = connection.dialect
         elif url:
             url = sqla_url.make_url(url)
@@ -309,7 +315,7 @@ class MigrationContext(object):
         head_maintainer = HeadMaintainer(self, heads)
 
         starting_in_transaction = not self.as_sql and \
-            self.connection.in_transaction()
+            self._in_connection_transaction()
 
         for step in self._migrations_fn(heads, self):
             with self.begin_transaction(_per_migration=True):
@@ -330,8 +336,8 @@ class MigrationContext(object):
                 head_maintainer.update_to_step(step)
 
             if not starting_in_transaction and not self.as_sql and \
-                    not self.impl.transactional_ddl and \
-                    self.connection.in_transaction():
+                not self.impl.transactional_ddl and \
+                    self._in_connection_transaction():
                 raise util.CommandError(
                     "Migration \"%s\" has left an uncommitted "
                     "transaction opened; transactional_ddl is False so "
@@ -341,6 +347,14 @@ class MigrationContext(object):
         if self.as_sql and not head_maintainer.heads:
             self._version.drop(self.connection)
 
+    def _in_connection_transaction(self):
+        try:
+            meth = self.connection.in_transaction
+        except AttributeError:
+            return False
+        else:
+            return meth()
+
     def execute(self, sql, execution_options=None):
         """Execute a SQL construct or string statement.
 
diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst
index 85513606..32a9a2c5 100644
--- a/docs/build/changelog.rst
+++ b/docs/build/changelog.rst
@@ -7,6 +7,19 @@ Changelog
     :version: 0.9.2
     :released:
 
+    .. change:: 419
+      :tags: bug environment
+      :tickets: 419
+
+      A warning is emitted when an object that's not a
+      :class:`~sqlalchemy.engine.Connection` is passed to
+      :meth:`.EnvironmentContext.configure`.  For the case of a
+      :class:`~sqlalchemy.engine.Engine` passed, the check for "in transaction"
+      introduced in version 0.9.0 has been relaxed to work in the case of an
+      attribute error, as some users appear to be passing an
+      :class:`~sqlalchemy.engine.Engine` and not a
+      :class:`~sqlalchemy.engine.Connection`.
+
 .. changelog::
     :version: 0.9.1
     :released: March 1, 2017
diff --git a/tests/test_environment.py b/tests/test_environment.py
index cb8a0a24..42ff328e 100644
--- a/tests/test_environment.py
+++ b/tests/test_environment.py
@@ -4,9 +4,10 @@ from alembic.script import ScriptDirectory
 from alembic.environment import EnvironmentContext
 from alembic.migration import MigrationContext
 from alembic.testing.fixtures import TestBase
-from alembic.testing.mock import Mock, call
+from alembic.testing.mock import Mock, call, MagicMock
 from alembic.testing.env import _no_sql_testing_config, \
-    staging_env, clear_staging_env
+    staging_env, clear_staging_env, write_script, _sqlite_file_db
+from alembic.testing.assertions import expect_warnings
 
 from alembic.testing import eq_, is_
 
@@ -74,3 +75,46 @@ class EnvironmentTest(TestBase):
 
         ctx = MigrationContext(ctx.dialect, None, {})
         is_(ctx.config, None)
+
+    def test_warning_on_passing_engine(self):
+        env = self._fixture()
+
+        engine = _sqlite_file_db()
+
+        a_rev = 'arev'
+        env.script.generate_revision(a_rev, "revision a", refresh=True)
+        write_script(env.script, a_rev, """\
+"Rev A"
+revision = '%s'
+down_revision = None
+
+from alembic import op
+
+
+def upgrade():
+    pass
+
+
+def downgrade():
+    pass
+
+""" % a_rev)
+        migration_fn = MagicMock()
+
+        def upgrade(rev, context):
+            migration_fn(rev, context)
+            return env.script._upgrade_revs(a_rev, rev)
+
+        with expect_warnings(
+                r"'connection' argument to configure\(\) is "
+                r"expected to be a sqlalchemy.engine.Connection "):
+            env.configure(
+                connection=engine, fn=upgrade,
+                transactional_ddl=False)
+
+        env.run_migrations()
+
+        eq_(
+            migration_fn.mock_calls,
+            [call((), env._migration_context)]
+        )