From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 20 Nov 2019 17:15:57 +0000 (-0500)
Subject: Warn for settings that don't work with viewonly=True
X-Git-Tag: rel_1_3_12~11
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e4927a881a721f42afb1a25dd8dc9806a7a1a3d0;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Warn for settings that don't work with viewonly=True

Setting persistence-related flags on :func:`.relationship` while also
setting viewonly=True will now emit a regular warning, as these flags do
not make sense for a viewonly=True relationship.   In particular, the
"cascade" settings have their own warning that is generated based on the
individual values, such as "delete, delete-orphan", that should not apply
to a viewonly relationship.   Note however that in the case of "cascade",
these settings are still erroneously taking effect even though the
relationship is set up as "viewonly".   In 1.4, all persistence-related
cascade settings will be disallowed on a viewonly=True relationship in
order to resolve this issue.

Fixes: #4993

Change-Id: I4b607a96a7de2ffa15303a27fd93c162a681556d
(cherry picked from commit 22d1f0706bd6a6742ad13f0bec75b04e705ff46b)
---

diff --git a/doc/build/changelog/unreleased_13/4993.rst b/doc/build/changelog/unreleased_13/4993.rst
new file mode 100644
index 0000000000..abeaa31c63
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4993.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4993
+
+    Setting persistence-related flags on :func:`.relationship` while also
+    setting viewonly=True will now emit a regular warning, as these flags do
+    not make sense for a viewonly=True relationship.   In particular, the
+    "cascade" settings have their own warning that is generated based on the
+    individual values, such as "delete, delete-orphan", that should not apply
+    to a viewonly relationship.   Note however that in the case of "cascade",
+    these settings are still erroneously taking effect even though the
+    relationship is set up as "viewonly".   In 1.4, all persistence-related
+    cascade settings will be disallowed on a viewonly=True relationship in
+    order to resolve this issue.
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 3b5508ff37..47d93e829a 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -103,6 +103,14 @@ class RelationshipProperty(StrategizedProperty):
 
     strategy_wildcard_key = "relationship"
 
+    _persistence_only = dict(
+        passive_deletes=False,
+        passive_updates=True,
+        enable_typechecks=True,
+        active_history=False,
+        cascade_backrefs=True,
+    )
+
     _dependency_processor = None
 
     @util.deprecated_params(
@@ -131,18 +139,18 @@ class RelationshipProperty(StrategizedProperty):
         viewonly=False,
         lazy="select",
         collection_class=None,
-        passive_deletes=False,
-        passive_updates=True,
+        passive_deletes=_persistence_only["passive_deletes"],
+        passive_updates=_persistence_only["passive_updates"],
         remote_side=None,
-        enable_typechecks=True,
+        enable_typechecks=_persistence_only["enable_typechecks"],
         join_depth=None,
         comparator_factory=None,
         single_parent=False,
         innerjoin=False,
         distinct_target_key=None,
         doc=None,
-        active_history=False,
-        cascade_backrefs=True,
+        active_history=_persistence_only["active_history"],
+        cascade_backrefs=_persistence_only["cascade_backrefs"],
         load_on_pending=False,
         bake_queries=True,
         _local_remote_pairs=None,
@@ -855,6 +863,14 @@ class RelationshipProperty(StrategizedProperty):
         self.post_update = post_update
         self.direction = None
         self.viewonly = viewonly
+        if viewonly:
+            self._warn_for_persistence_only_flags(
+                passive_deletes=passive_deletes,
+                passive_updates=passive_updates,
+                enable_typechecks=enable_typechecks,
+                active_history=active_history,
+                cascade_backrefs=cascade_backrefs,
+            )
         self.lazy = lazy
         self.single_parent = single_parent
         self._user_defined_foreign_keys = foreign_keys
@@ -897,9 +913,10 @@ class RelationshipProperty(StrategizedProperty):
 
         self._reverse_property = set()
 
-        self.cascade = (
-            cascade if cascade is not False else "save-update, merge"
-        )
+        if cascade is not False:
+            self.cascade = cascade
+        else:
+            self._set_cascade("save-update, merge", warn=False)
 
         self.order_by = order_by
 
@@ -915,6 +932,24 @@ class RelationshipProperty(StrategizedProperty):
         else:
             self.backref = backref
 
+    def _warn_for_persistence_only_flags(self, **kw):
+        for k, v in kw.items():
+            if v != self._persistence_only[k]:
+                # we are warning here rather than warn deprecated as this is a
+                # configuration mistake, and Python shows regular warnings more
+                # aggressively than deprecation warnings by default. Unlike the
+                # case of setting viewonly with cascade, the settings being
+                # warned about here are not actively doing the wrong thing
+                # against viewonly=True, so it is not as urgent to have these
+                # raise an error.
+                util.warn(
+                    "Setting %s on relationship() while also "
+                    "setting viewonly=True does not make sense, as a "
+                    "viewonly=True relationship does not perform persistence "
+                    "operations. This configuration may raise an error "
+                    "in a future release." % (k,)
+                )
+
     def instrument_class(self, mapper):
         attributes.register_descriptor(
             mapper.class_,
@@ -1988,14 +2023,41 @@ class RelationshipProperty(StrategizedProperty):
                 )
             )
 
-    def _get_cascade(self):
+    @property
+    def cascade(self):
         """Return the current cascade setting for this
         :class:`.RelationshipProperty`.
         """
         return self._cascade
 
-    def _set_cascade(self, cascade):
+    @cascade.setter
+    def cascade(self, cascade):
+        self._set_cascade(cascade)
+
+    def _set_cascade(self, cascade, warn=True):
         cascade = CascadeOptions(cascade)
+
+        if warn and self.viewonly:
+            non_viewonly = set(cascade).difference(
+                CascadeOptions._viewonly_cascades
+            )
+            if non_viewonly:
+                # we are warning here rather than warn deprecated as this
+                # setting actively does the wrong thing and Python shows
+                # regular warnings more aggressively than deprecation warnings
+                # by default. There's no other guard against setting active
+                # persistence cascades under viewonly=True so this will raise
+                # in 1.4.
+                util.warn(
+                    'Cascade settings "%s" should not be combined with a '
+                    "viewonly=True relationship.   This configuration will "
+                    "raise an error in version 1.4.  Note that in versions "
+                    "prior to 1.4, "
+                    "these cascade settings may still produce a mutating "
+                    "effect even though this relationship is marked as "
+                    "viewonly=True." % (", ".join(sorted(non_viewonly)))
+                )
+
         if "mapper" in self.__dict__:
             self._check_cascade_settings(cascade)
         self._cascade = cascade
@@ -2003,8 +2065,6 @@ class RelationshipProperty(StrategizedProperty):
         if self._dependency_processor:
             self._dependency_processor.cascade = cascade
 
-    cascade = property(_get_cascade, _set_cascade)
-
     def _check_cascade_settings(self, cascade):
         if (
             cascade.delete_orphan
diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index a8af25c3fc..18d693279d 100644
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -56,6 +56,8 @@ class CascadeOptions(frozenset):
     )
     _allowed_cascades = all_cascades
 
+    _viewonly_cascades = ["expunge", "all", "none", "refresh-expire"]
+
     __slots__ = (
         "save_update",
         "delete",
diff --git a/test/orm/test_deprecations.py b/test/orm/test_deprecations.py
index b5609deb06..c806ec6442 100644
--- a/test/orm/test_deprecations.py
+++ b/test/orm/test_deprecations.py
@@ -20,6 +20,7 @@ from sqlalchemy.orm import create_session
 from sqlalchemy.orm import defer
 from sqlalchemy.orm import deferred
 from sqlalchemy.orm import EXT_CONTINUE
+from sqlalchemy.orm import foreign
 from sqlalchemy.orm import identity
 from sqlalchemy.orm import instrumentation
 from sqlalchemy.orm import joinedload
@@ -41,7 +42,9 @@ from sqlalchemy.testing import AssertsCompiledSQL
 from sqlalchemy.testing import engines
 from sqlalchemy.testing import eq_
 from sqlalchemy.testing import fixtures
+from sqlalchemy.testing import in_
 from sqlalchemy.testing import is_
+from sqlalchemy.testing import not_in_
 from sqlalchemy.testing.schema import Column
 from sqlalchemy.testing.schema import Table
 from sqlalchemy.testing.util import gc_collect
@@ -2501,6 +2504,291 @@ class NonPrimaryRelationshipLoaderTest(_fixtures.FixtureTest):
         )
 
 
+class ViewonlyFlagWarningTest(fixtures.MappedTest):
+    """test for #4993.
+
+    In 1.4, this moves to test/orm/test_cascade, deprecation warnings
+    become errors, will then be for #4994.
+
+    """
+
+    @classmethod
+    def define_tables(cls, metadata):
+        Table(
+            "users",
+            metadata,
+            Column("id", Integer, primary_key=True),
+            Column("name", String(30)),
+        )
+        Table(
+            "orders",
+            metadata,
+            Column("id", Integer, primary_key=True),
+            Column("user_id", Integer),
+            Column("description", String(30)),
+        )
+
+    @classmethod
+    def setup_classes(cls):
+        class User(cls.Comparable):
+            pass
+
+        class Order(cls.Comparable):
+            pass
+
+    @testing.combinations(
+        ("passive_deletes", True),
+        ("passive_updates", False),
+        ("enable_typechecks", False),
+        ("active_history", True),
+        ("cascade_backrefs", False),
+    )
+    def test_viewonly_warning(self, flag, value):
+        Order = self.classes.Order
+
+        with testing.expect_warnings(
+            r"Setting %s on relationship\(\) while also setting "
+            "viewonly=True does not make sense" % flag
+        ):
+            kw = {
+                "viewonly": True,
+                "primaryjoin": self.tables.users.c.id
+                == foreign(self.tables.orders.c.user_id),
+            }
+            kw[flag] = value
+            rel = relationship(Order, **kw)
+
+            if flag == "cascade":
+                eq_(set(rel.cascade), {"delete", "delete-orphan"})
+            else:
+                eq_(getattr(rel, flag), value)
+
+    @testing.combinations(
+        ({"delete"}, {"delete"}),
+        (
+            {"all, delete-orphan"},
+            {"delete", "delete-orphan", "merge", "save-update"},
+        ),
+        ({"save-update, expunge"}, {"save-update"}),
+    )
+    def test_write_cascades(self, setting, settings_that_warn):
+        Order = self.classes.Order
+
+        with testing.expect_warnings(
+            r"Cascade settings \"%s\" should not be combined"
+            % (", ".join(sorted(settings_that_warn)))
+        ):
+            relationship(
+                Order,
+                primaryjoin=(
+                    self.tables.users.c.id
+                    == foreign(self.tables.orders.c.user_id)
+                ),
+                cascade=", ".join(sorted(setting)),
+                viewonly=True,
+            )
+
+    def test_expunge_cascade(self):
+        User, Order, orders, users = (
+            self.classes.User,
+            self.classes.Order,
+            self.tables.orders,
+            self.tables.users,
+        )
+
+        mapper(Order, orders)
+        mapper(
+            User,
+            users,
+            properties={
+                "orders": relationship(
+                    Order,
+                    primaryjoin=(
+                        self.tables.users.c.id
+                        == foreign(self.tables.orders.c.user_id)
+                    ),
+                    cascade="expunge",
+                    viewonly=True,
+                )
+            },
+        )
+
+        sess = Session()
+        u = User(id=1, name="jack")
+        sess.add(u)
+        sess.add_all(
+            [
+                Order(id=1, user_id=1, description="someorder"),
+                Order(id=2, user_id=1, description="someotherorder"),
+            ]
+        )
+        sess.commit()
+
+        u1 = sess.query(User).first()
+        orders = u1.orders
+        eq_(len(orders), 2)
+
+        in_(orders[0], sess)
+        in_(orders[1], sess)
+
+        sess.expunge(u1)
+
+        not_in_(orders[0], sess)
+        not_in_(orders[1], sess)
+
+    def test_default_save_update_cascade(self):
+        User, Order, orders, users = (
+            self.classes.User,
+            self.classes.Order,
+            self.tables.orders,
+            self.tables.users,
+        )
+
+        mapper(Order, orders)
+        mapper(
+            User,
+            users,
+            properties={
+                "orders": relationship(
+                    Order,
+                    primaryjoin=(
+                        self.tables.users.c.id
+                        == foreign(self.tables.orders.c.user_id)
+                    ),
+                    viewonly=True,
+                )
+            },
+        )
+
+        sess = Session()
+        u1 = User(id=1, name="jack")
+        sess.add(u1)
+
+        o1, o2 = (
+            Order(id=1, user_id=1, description="someorder"),
+            Order(id=2, user_id=1, description="someotherorder"),
+        )
+
+        u1.orders.append(o1)
+        u1.orders.append(o2)
+
+        # in 1.4, this becomes "not_in_"
+        in_(o1, sess)
+        in_(o2, sess)
+
+    def test_default_merge_cascade(self):
+        User, Order, orders, users = (
+            self.classes.User,
+            self.classes.Order,
+            self.tables.orders,
+            self.tables.users,
+        )
+
+        mapper(Order, orders)
+        mapper(
+            User,
+            users,
+            properties={
+                "orders": relationship(
+                    Order,
+                    primaryjoin=(
+                        self.tables.users.c.id
+                        == foreign(self.tables.orders.c.user_id)
+                    ),
+                    viewonly=True,
+                )
+            },
+        )
+
+        sess = Session()
+        u1 = User(id=1, name="jack")
+
+        o1, o2 = (
+            Order(id=1, user_id=1, description="someorder"),
+            Order(id=2, user_id=1, description="someotherorder"),
+        )
+
+        u1.orders.append(o1)
+        u1.orders.append(o2)
+
+        u1 = sess.merge(u1)
+
+        # in 1.4, this becomes "assert not u1.orders", merge does not occur
+        o1, o2 = u1.orders
+
+    def test_default_cascade_didnt_change_yet(self):
+        User, Order, orders, users = (
+            self.classes.User,
+            self.classes.Order,
+            self.tables.orders,
+            self.tables.users,
+        )
+
+        mapper(Order, orders)
+        umapper = mapper(
+            User,
+            users,
+            properties={
+                "orders": relationship(
+                    Order,
+                    primaryjoin=(
+                        self.tables.users.c.id
+                        == foreign(self.tables.orders.c.user_id)
+                    ),
+                    viewonly=True,
+                )
+            },
+        )
+
+        # in 1.4 this becomes {}
+        eq_(umapper.attrs["orders"].cascade, {"save-update", "merge"})
+
+    def test_write_cascade_still_works_w_viewonly(self):
+        """should be no longer possible in 1.4"""
+
+        User, Order, orders, users = (
+            self.classes.User,
+            self.classes.Order,
+            self.tables.orders,
+            self.tables.users,
+        )
+
+        mapper(Order, orders)
+        with testing.expect_warnings(r"Cascade settings"):
+            mapper(
+                User,
+                users,
+                properties={
+                    "orders": relationship(
+                        Order,
+                        primaryjoin=(
+                            self.tables.users.c.id
+                            == foreign(self.tables.orders.c.user_id)
+                        ),
+                        cascade="all, delete, delete-orphan",
+                        viewonly=True,
+                    )
+                },
+            )
+
+        sess = Session()
+        u = User(id=1, name="jack")
+        sess.add(u)
+        sess.add_all(
+            [
+                Order(id=1, user_id=1, description="someorder"),
+                Order(id=2, user_id=1, description="someotherorder"),
+            ]
+        )
+        sess.commit()
+        eq_(sess.query(Order).count(), 2)
+
+        sess.delete(u)
+        sess.commit()
+
+        eq_(sess.query(Order).count(), 0)
+
+
 class NonPrimaryMapperTest(_fixtures.FixtureTest, AssertsCompiledSQL):
     __dialect__ = "default"