Don't emit warnings on descriptor access

This commit is revising 5162f2bc5fc0ac239f26a76fc9f0c2, which
when I did it felt a little rushed but I couldn't find anything
wrong.  Well here we are :).

Fixed issue where a :class:`.RemovedIn20Warning` would erroneously emit
when the ``.bind`` attribute were accessed internally on objects,
particularly when stringifying a SQL construct.

Alter the deprecated() decorator so that we can use it just to add
docstring warnings but not actually warn when the function is
accessed, adding new argument enable_warnings that can be
set to False.

Added a safety feature to deprecated_20() that will disallow an
":attr:" from proceeding if enable_warnings=False isn't present,
unless there's an extra flag
warn_on_attribute_access, since we want Session.transaction to
emit a deprecation warning.  This is a little hacky but it's essentially
modifying the decorator to require a positive assertion that
a deprecation decorator on a descriptor should actually warn
on access.

Remove the warning filter for session.transaction and get
tests to pass to ensure this is not also being called
internally.

Added tests to ensure that common places .bind can be passed
as a parameter definitely warn as I was not able to find
this otherwise.

Fixes: #5717
Change-Id: Ia586b4f9ee6b212f3a71104b1caf40b5edd399e2

diff --git a/doc/build/changelog/unreleased_14/5717.rst b/doc/build/changelog/unreleased_14/5717.rst
new file mode 100644 (file)
index 0000000..af8c716
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/5717.rst
@@ -0,0 +1,7 @@
+.. change::
+    :tags: bug, sql
+    :tickets: 5717
+
+    Fixed issue where a :class:`.RemovedIn20Warning` would erroneously emit
+    when the ``.bind`` attribute were accessed internally on objects,
+    particularly when stringifying a SQL construct.

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 2fc2ad68c6a00b0322b8e21543e9bbb692c37df2..b89edca446599f9c988e99d40240a9cbb2891055 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1053,6 +1053,7 @@ class Session(_SessionClassMethods):
         ":meth:`_orm.Session.begin`.  To access "
         "the current root transaction, use "
         ":meth:`_orm.Session.get_transaction`.",
+        warn_on_attribute_access=True,
     )
     def transaction(self):
         """The current active or inactive :class:`.SessionTransaction`.
@@ -1065,6 +1066,9 @@ class Session(_SessionClassMethods):
 
 
         """
+        return self._legacy_transaction()
+
+    def _legacy_transaction(self):
         if not self.future:
             self._autobegin()
         return self._transaction

diff --git a/lib/sqlalchemy/sql/base.py b/lib/sqlalchemy/sql/base.py
index 999169f90c7f9f0e8142cfbbf1e0e772c2ee281a..ff44ab27c908519408da9b8a3866fca79f652b6c 100644 (file)
--- a/lib/sqlalchemy/sql/base.py
+++ b/lib/sqlalchemy/sql/base.py
@@ -909,6 +909,7 @@ class Executable(Generative):
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Returns the :class:`_engine.Engine` or :class:`_engine.Connection`

diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py
index 086cef48a905525e9e64b163ae9ab4dff838378c..d60afdbacf771a0789ecc64939f84aefb0b991e5 100644 (file)
--- a/lib/sqlalchemy/sql/selectable.py
+++ b/lib/sqlalchemy/sql/selectable.py
@@ -1238,6 +1238,7 @@ class Join(roles.DMLTableRole, FromClause):
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Return the bound engine associated with either the left or right
@@ -3559,6 +3560,7 @@ class CompoundSelect(HasCompileState, GenerativeSelect):
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Returns the :class:`_engine.Engine` or :class:`_engine.Connection`
@@ -5443,6 +5445,7 @@ class Select(
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Returns the :class:`_engine.Engine` or :class:`_engine.Connection`

diff --git a/lib/sqlalchemy/testing/warnings.py b/lib/sqlalchemy/testing/warnings.py
index 2a0be42c1fdd1f2c748509674526177360523435..b230bad6f09dd4d6861e20bd176ef44e0be45c63 100644 (file)
--- a/lib/sqlalchemy/testing/warnings.py
+++ b/lib/sqlalchemy/testing/warnings.py
@@ -63,7 +63,6 @@ def setup_filters():
         #
         r"The MetaData.bind argument is deprecated",
         r"The ``bind`` argument for schema methods that invoke SQL ",
-        r"The Executable.bind attribute is considered legacy ",
         r"The Function.bind argument",
         r"The select.bind argument",
         #
@@ -131,7 +130,6 @@ def setup_filters():
         r".*object is being merged into a Session along the backref "
         "cascade path",
         r"Passing bind arguments to Session.execute\(\) as keyword arguments",
-        r"The Session.transaction attribute",
         r"The merge_result\(\) method is superseded by the "
         r"merge_frozen_result\(\)",
         r"The Session.begin.subtransactions flag is deprecated",

diff --git a/lib/sqlalchemy/util/deprecations.py b/lib/sqlalchemy/util/deprecations.py
index f4637460120ff8f47a8cc793cdbbb39482c5ad54..a4c9d9d0e9cd3f7b383a47f5e392b58e3fc79a71 100644 (file)
--- a/lib/sqlalchemy/util/deprecations.py
+++ b/lib/sqlalchemy/util/deprecations.py
@@ -112,7 +112,11 @@ def deprecated_20_cls(
 
 
 def deprecated(
-    version, message=None, add_deprecation_to_docstring=True, warning=None
+    version,
+    message=None,
+    add_deprecation_to_docstring=True,
+    warning=None,
+    enable_warnings=True,
 ):
     """Decorates a function and issues a deprecation warning on use.
 
@@ -157,7 +161,12 @@ def deprecated(
 
     def decorate(fn):
         return _decorate_with_warning(
-            fn, warning, message % dict(func=fn.__name__), version, header
+            fn,
+            warning,
+            message % dict(func=fn.__name__),
+            version,
+            header,
+            enable_warnings=enable_warnings,
         )
 
     return decorate
@@ -189,6 +198,16 @@ def deprecated_20(api_name, alternative=None, becomes_legacy=False, **kw):
         )
     )
 
+    if ":attr:" in api_name:
+        attribute_ok = kw.pop("warn_on_attribute_access", False)
+        if not attribute_ok:
+            assert kw.get("enable_warnings") is False, (
+                "attribute %s will emit a warning on read access.  "
+                "If you *really* want this, "
+                "add warn_on_attribute_access=True.  Otherwise please add "
+                "enable_warnings=False." % api_name
+            )
+
     if alternative:
         message += " " + alternative
 
@@ -347,7 +366,7 @@ def _decorate_cls_with_warning(
 
 
 def _decorate_with_warning(
-    func, wtype, message, version, docstring_header=None
+    func, wtype, message, version, docstring_header=None, enable_warnings=True
 ):
     """Wrap a function with a warnings.warn and augmented docstring."""
 
@@ -363,7 +382,9 @@ def _decorate_with_warning(
 
     @decorator
     def warned(fn, *args, **kwargs):
-        skip_warning = kwargs.pop("_sa_skip_warning", False)
+        skip_warning = not enable_warnings or kwargs.pop(
+            "_sa_skip_warning", False
+        )
         if not skip_warning:
             _warn_with_version(message, version, wtype, stacklevel=3)
         return fn(*args, **kwargs)

diff --git a/test/aaa_profiling/test_orm.py b/test/aaa_profiling/test_orm.py
index 1bbfd36586c8f56c74ab280519719e66fa47b71f..f87f61074591ac57f7e92bdc8ac1b676b92e34a3 100644 (file)
--- a/test/aaa_profiling/test_orm.py
+++ b/test/aaa_profiling/test_orm.py
@@ -108,7 +108,7 @@ class MergeTest(NoCache, fixtures.MappedTest):
         # down from 185 on this this is a small slice of a usually
         # bigger operation so using a small variance
 
-        sess2.transaction  # autobegin
+        sess2._legacy_transaction()  # autobegin
 
         @profiling.function_call_count(variance=0.20)
         def go1():
@@ -118,7 +118,7 @@ class MergeTest(NoCache, fixtures.MappedTest):
 
         # third call, merge object already present. almost no calls.
 
-        sess2.transaction  # autobegin
+        sess2._legacy_transaction()  # autobegin
 
         @profiling.function_call_count(variance=0.10, warmup=1)
         def go2():
@@ -138,7 +138,7 @@ class MergeTest(NoCache, fixtures.MappedTest):
         # using sqlite3 the C extension took it back up to approx. 1257
         # (py2.6)
 
-        sess2.transaction  # autobegin
+        sess2._legacy_transaction()  # autobegin
 
         @profiling.function_call_count(variance=0.10)
         def go():

diff --git a/test/orm/test_bind.py b/test/orm/test_bind.py
index fef827d833b6b935b644267a03e404394b277fc0..3a9959857075b2545a1044ac8bbf054c855b8a1b 100644 (file)
--- a/test/orm/test_bind.py
+++ b/test/orm/test_bind.py
@@ -405,7 +405,7 @@ class BindIntegrationTest(_fixtures.FixtureTest):
         c = testing.db.connect()
         sess = create_session(bind=c)
         sess.begin()
-        transaction = sess.transaction
+        transaction = sess._legacy_transaction()
         u = User(name="u1")
         sess.add(u)
         sess.flush()

diff --git a/test/orm/test_deprecations.py b/test/orm/test_deprecations.py
index 47e8041132e7884366f55dd3e74b7704bbbcdf36..4bd921af45bcdab2a50b82ea559894e770fab7d9 100644 (file)
--- a/test/orm/test_deprecations.py
+++ b/test/orm/test_deprecations.py
@@ -1994,6 +1994,19 @@ class SubqRelationsFromSelfTest(fixtures.DeclarativeMappedTest):
 
 
 class SessionTest(fixtures.RemovesEvents, _LocalFixture):
+    def test_transaction_attr(self):
+        s1 = Session(testing.db)
+
+        with testing.expect_deprecated_20(
+            "The Session.transaction attribute is considered legacy as "
+            "of the 1.x series"
+        ):
+            s1.transaction
+
+    def test_session_str(self):
+        s1 = Session(testing.db)
+        str(s1)
+
     def test_subtransactions_deprecated(self):
         s1 = Session(testing.db)
         s1.begin()

diff --git a/test/orm/test_session.py b/test/orm/test_session.py
index 4562df44a60289265e81f3e94f69bb4f02943c14..165008234603ec047e352c4ed0ffe172ce8834db 100644 (file)
--- a/test/orm/test_session.py
+++ b/test/orm/test_session.py
@@ -501,7 +501,7 @@ class SessionStateTest(_fixtures.FixtureTest):
         sess.flush()
         assert u1 not in sess
         assert_raises(sa.exc.InvalidRequestError, sess.add, u1)
-        assert sess.transaction is not None
+        assert sess.in_transaction()
         sess.rollback()
         assert u1 in sess
 

diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py
index 497693de282f0674fc4755a91a6aa45ac6e85d77..e8f6c5c4052daf451abcc82b370c651d59e0aee8 100644 (file)
--- a/test/orm/test_transaction.py
+++ b/test/orm/test_transaction.py
@@ -48,7 +48,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
             mapper(User, users)
             s = create_session(bind=c)
             s.begin()
-            tran = s.transaction
+            tran = s._legacy_transaction()
             s.add(User(name="first"))
             s.flush()
             c.exec_driver_sql("select * from users")
@@ -58,7 +58,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
             u = User(name="third")
             s.add(u)
             s.flush()
-            assert s.transaction is tran
+            assert s._legacy_transaction() is tran
             tran.close()
         finally:
             c.close()
@@ -564,7 +564,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
         sess.add(User(name="u2"))
 
         t2.commit()
-        assert sess.transaction is t1
+        assert sess._legacy_transaction() is t1
 
         sess.close()
 
@@ -1024,9 +1024,9 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
         mapper(User, users)
         session = create_session(autocommit=False)
         session.add(User(name="ed"))
-        session.transaction.commit()
+        session._legacy_transaction().commit()
 
-        is_not(session.transaction, None)
+        is_not(session._legacy_transaction(), None)
 
     def test_no_autocommit_with_explicit_commit_future(self):
         User, users = self.classes.User, self.tables.users
@@ -1034,10 +1034,10 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
         mapper(User, users)
         session = create_session(testing.db, autocommit=False, future=True)
         session.add(User(name="ed"))
-        session.transaction.commit()
+        session._legacy_transaction().commit()
 
         # new in 1.4
-        is_(session.transaction, None)
+        is_(session._legacy_transaction(), None)
 
     @testing.requires.python2
     @testing.requires.savepoints_w_release
@@ -1278,7 +1278,7 @@ class SubtransactionRecipeTest(FixtureTest):
         sess.add(User(name="u2"))
 
         t2.commit()
-        assert sess.transaction is t1
+        assert sess._legacy_transaction() is t1
 
         sess.close()
 
@@ -1317,7 +1317,7 @@ class SubtransactionRecipeTest(FixtureTest):
 
             try:
                 with subtransaction_recipe(sess):
-                    assert sess.transaction
+                    assert sess._legacy_transaction()
                     raise Exception("force rollback")
             except:
                 pass
@@ -1921,7 +1921,7 @@ class SavepointTest(_LocalFixture):
         nested_trans = trans._connections[self.bind][1]
         nested_trans._do_commit()
 
-        is_(s.transaction, trans)
+        is_(s._legacy_transaction(), trans)
 
         with expect_warnings("nested transaction already deassociated"):
             # this previously would raise
@@ -1932,12 +1932,12 @@ class SavepointTest(_LocalFixture):
         assert u1 not in s.new
 
         is_(trans._state, _session.CLOSED)
-        is_not(s.transaction, trans)
-        is_(s.transaction._state, _session.ACTIVE)
+        is_not(s._legacy_transaction(), trans)
+        is_(s._legacy_transaction()._state, _session.ACTIVE)
 
-        is_(s.transaction.nested, False)
+        is_(s._legacy_transaction().nested, False)
 
-        is_(s.transaction._parent, None)
+        is_(s._legacy_transaction()._parent, None)
 
 
 class AccountingFlagsTest(_LocalFixture):
@@ -2118,7 +2118,7 @@ class ContextManagerPlusFutureTest(FixtureTest):
     def test_explicit_begin(self):
         s1 = Session(testing.db)
         with s1.begin() as trans:
-            is_(trans, s1.transaction)
+            is_(trans, s1._legacy_transaction())
             s1.connection()
 
         is_(s1._transaction, None)
@@ -2152,12 +2152,12 @@ class ContextManagerPlusFutureTest(FixtureTest):
 
         # rolls back the whole transaction
         s1.rollback()
-        is_(s1.transaction, None)
+        is_(s1._legacy_transaction(), None)
 
         eq_(s1.connection().scalar(select(func.count()).select_from(users)), 0)
 
         s1.commit()
-        is_(s1.transaction, None)
+        is_(s1._legacy_transaction(), None)
 
     @testing.requires.savepoints
     def test_old_rollback_is_local(self):
@@ -2180,19 +2180,19 @@ class ContextManagerPlusFutureTest(FixtureTest):
         # rolls back only the savepoint
         s1.rollback()
 
-        is_(s1.transaction, t1)
+        is_(s1._legacy_transaction(), t1)
 
         eq_(s1.connection().scalar(select(func.count()).select_from(users)), 1)
 
         s1.commit()
         eq_(s1.connection().scalar(select(func.count()).select_from(users)), 1)
-        is_not(s1.transaction, None)
+        is_not(s1._legacy_transaction(), None)
 
     def test_session_as_ctx_manager_one(self):
         users = self.tables.users
 
         with Session(testing.db) as sess:
-            is_not(sess.transaction, None)
+            is_not(sess._legacy_transaction(), None)
 
             sess.connection().execute(
                 users.insert().values(id=1, name="user1")
@@ -2202,9 +2202,9 @@ class ContextManagerPlusFutureTest(FixtureTest):
                 sess.connection().execute(users.select()).all(), [(1, "user1")]
             )
 
-            is_not(sess.transaction, None)
+            is_not(sess._legacy_transaction(), None)
 
-        is_not(sess.transaction, None)
+        is_not(sess._legacy_transaction(), None)
 
         # did not commit
         eq_(sess.connection().execute(users.select()).all(), [])
@@ -2213,7 +2213,7 @@ class ContextManagerPlusFutureTest(FixtureTest):
         users = self.tables.users
 
         with Session(testing.db, future=True) as sess:
-            is_(sess.transaction, None)
+            is_(sess._legacy_transaction(), None)
 
             sess.connection().execute(
                 users.insert().values(id=1, name="user1")
@@ -2223,9 +2223,9 @@ class ContextManagerPlusFutureTest(FixtureTest):
                 sess.connection().execute(users.select()).all(), [(1, "user1")]
             )
 
-            is_not(sess.transaction, None)
+            is_not(sess._legacy_transaction(), None)
 
-        is_(sess.transaction, None)
+        is_(sess._legacy_transaction(), None)
 
         # did not commit
         eq_(sess.connection().execute(users.select()).all(), [])
@@ -2235,7 +2235,7 @@ class ContextManagerPlusFutureTest(FixtureTest):
 
         try:
             with Session(testing.db) as sess:
-                is_not(sess.transaction, None)
+                is_not(sess._legacy_transaction(), None)
 
                 sess.connection().execute(
                     users.insert().values(id=1, name="user1")
@@ -2244,14 +2244,14 @@ class ContextManagerPlusFutureTest(FixtureTest):
                 raise Exception("force rollback")
         except:
             pass
-        is_not(sess.transaction, None)
+        is_not(sess._legacy_transaction(), None)
 
     def test_session_as_ctx_manager_two_future(self):
         users = self.tables.users
 
         try:
             with Session(testing.db, future=True) as sess:
-                is_(sess.transaction, None)
+                is_(sess._legacy_transaction(), None)
 
                 sess.connection().execute(
                     users.insert().values(id=1, name="user1")
@@ -2260,7 +2260,7 @@ class ContextManagerPlusFutureTest(FixtureTest):
                 raise Exception("force rollback")
         except:
             pass
-        is_(sess.transaction, None)
+        is_(sess._legacy_transaction(), None)
 
     def test_begin_context_manager(self):
         users = self.tables.users

diff --git a/test/orm/test_unitofwork.py b/test/orm/test_unitofwork.py
index dcd9219a4362665d82a0ea8f00c3e7db7229eb13..eb5530ef4229f45d192e5162f465b885492d041d 100644 (file)
--- a/test/orm/test_unitofwork.py
+++ b/test/orm/test_unitofwork.py
@@ -3307,7 +3307,7 @@ class TransactionTest(fixtures.MappedTest):
             assert False
         except Exception:
             # Flush needs to rollback also when commit fails
-            assert session.transaction is None
+            assert session._legacy_transaction() is None
 
         # todo: on 8.3 at least, the failed commit seems to close the cursor?
         # needs investigation.  leaving in the DDL above now to help verify

diff --git a/test/sql/test_deprecations.py b/test/sql/test_deprecations.py
index 5f86b0b89f295b4c1e8f6269fc2ea29a720f5fab..c0d2e87e8640d1d95ecbdf7f4e080e790956ac4c 100644 (file)
--- a/test/sql/test_deprecations.py
+++ b/test/sql/test_deprecations.py
@@ -90,6 +90,48 @@ class BoundMetadataTest(fixtures.TestBase):
 
         assert "t" not in inspect(testing.db).get_table_names()
 
+    def test_bind_arg_text(self):
+        with testing.expect_deprecated_20(
+            "The text.bind argument is deprecated and will be "
+            "removed in SQLAlchemy 2.0."
+        ):
+            t1 = text("ASdf", bind=testing.db)
+
+        # no warnings emitted
+        is_(t1.bind, testing.db)
+        eq_(str(t1), "ASdf")
+
+    def test_bind_arg_function(self):
+        with testing.expect_deprecated_20(
+            "The text.bind argument is deprecated and will be "
+            "removed in SQLAlchemy 2.0."
+        ):
+            f1 = func.foobar(bind=testing.db)
+
+        # no warnings emitted
+        is_(f1.bind, testing.db)
+        eq_(str(f1), "foobar()")
+
+    def test_bind_arg_select(self):
+        with testing.expect_deprecated_20(
+            "The select.bind argument is deprecated and will be "
+            "removed in SQLAlchemy 2.0."
+        ):
+            s1 = select([column("q")], bind=testing.db)
+
+        # no warnings emitted
+        is_(s1.bind, testing.db)
+        eq_(str(s1), "SELECT q")
+
+    def test_bind_attr_join_no_warning(self):
+        t1 = table("t1", column("a"))
+        t2 = table("t2", column("b"))
+        j1 = join(t1, t2, t1.c.a == t2.c.b)
+
+        # no warnings emitted
+        is_(j1.bind, None)
+        eq_(str(j1), "t1 JOIN t2 ON t1.a = t2.b")
+
 
 class DeprecationWarningsTest(fixtures.TestBase, AssertsCompiledSQL):
     __backend__ = True