Add sync_backref flag in a relationship

Introduce :paramref:`_orm.relationship.sync_backref` flag in a relationship
to control if the synchronization events that mutate the in-Python
attributes are added.
This flag is implied in a ``viewonly=True`` relationship.
This supersedes the previous change #5149, that warned that ``viewonly=True``
relationship target of a back_populates or backref configuration would be
disallowed.

Fixes: #5237
Change-Id: I22c5ba28dcea22fc78a83e68e667140edffc515c
(cherry picked from commit 04c990a011)

.gitignore

@@ -4,6 +4,7 @@
 /build/
 /dist/
 /doc/build/output/
+/doc/build/_build/
 /dogpile_data/
 *.orig
 *,cover

doc/build/changelog/unreleased_13/5237.rst

@@ -0,0 +1,10 @@
+.. change::
+    :tags: orm, usecase
+    :tickets: 5237
+
+    Introduce :paramref:`_orm.relationship.sync_backref` flag in a relationship
+    to control if the synchronization events that mutate the in-Python
+    attributes are added. This flag is implied in a ``viewonly=True``
+    relationship. This supersedes the previous change :ticket:`5149`, which
+    warned that ``viewonly=True`` relationship target of a back_populates or
+    backref configuration would be disallowed.

lib/sqlalchemy/orm/relationships.py

@@ -157,6 +157,7 @@ class RelationshipProperty(StrategizedProperty):
         query_class=None,
         info=None,
         omit_join=None,
+        sync_backref=None,
     ):
         """Provide a relationship between two mapped classes.
 
@@ -900,32 +901,55 @@ class RelationshipProperty(StrategizedProperty):
               :paramref:`_orm.relationship.uselist` flag is needed.
 
         :param viewonly=False:
-          When set to True, the relationship is used only for loading objects,
-          and not for any persistence operation.  A :func:`_orm.relationship`
-          which specifies :paramref:`_orm.relationship.viewonly` can work
+          When set to ``True``, the relationship is used only for loading
+          objects, and not for any persistence operation.  A
+          :func:`_orm.relationship` which specifies
+          :paramref:`_orm.relationship.viewonly` can work
           with a wider range of SQL operations within the
           :paramref:`_orm.relationship.primaryjoin` condition, including
           operations that feature the use of a variety of comparison operators
           as well as SQL functions such as :func:`_expression.cast`.  The
           :paramref:`_orm.relationship.viewonly`
-          flag is also of general use when
-          defining any kind of :func:`_orm.relationship`
-          that doesn't represent
+          flag is also of general use when defining any kind of
+          :func:`_orm.relationship` that doesn't represent
           the full set of related objects, to prevent modifications of the
           collection from resulting in persistence operations.
 
-          .. warning:: The viewonly=True relationship should not be mutated
-             in Python; that means, elements should not be added or removed
-             from collections nor should a many-to-one or one-to-one attribute
-             be altered in Python.  The viewonly=True relationship should only
-             be accessed via read.   Towards this behavior, it is also not
-             appropriate for the viewonly=True relationship to have any kind
-             of persistence cascade settings, nor should it be the target of
-             either :paramref:`_orm.relationship.backref` or
-             :paramref:`_orm.relationship.back_populates`, as backrefs imply
-             in-Python mutation of the attribute.  SQLAlchemy may emit
-             warnings for some or all of these conditions as of the 1.3 and
-             1.4 series of SQLAlchemy and will eventually be disallowed.
+          When using the :paramref:`_orm.relationship.viewonly` flag in
+          conjunction with backrefs, the
+          :paramref:`_orm.relationship.sync_backref` should be set to False;
+          this indicates that the backref should not actually populate this
+          relationship with data when changes occur on the other side; as this
+          is a viewonly relationship, it cannot accommodate changes in state
+          correctly as these will not be persisted.
+
+          .. versionadded:: 1.3.17 - the
+             :paramref:`_orm.relationship.sync_backref`
+             flag set to False is required when using viewonly in conjunction
+             with backrefs. A warning is emitted when this flag is not set.
+
+          .. seealso::
+
+            :paramref:`_orm.relationship.sync_backref`
+
+        :param sync_backref:
+          A boolean that enables the events used to synchronize the in-Python
+          attributes when this relationship is target of either
+          :paramref:`_orm.relationship.backref` or
+          :paramref:`_orm.relationship.back_populates`.
+
+          Defaults to ``None``, which indicates that an automatic value should
+          be selected based on the value of the
+          :paramref:`_orm.relationship.viewonly` flag.  When left at its
+          default, changes in state for writable relationships will be
+          back-populated normally.  For viewonly relationships, a warning is
+          emitted unless the flag is set to ``False``.
+
+          .. versionadded:: 1.3.17
+
+          .. seealso::
+
+            :paramref:`_orm.relationship.viewonly`
 
         :param omit_join:
           Allows manual control over the "selectin" automatic join
@@ -963,6 +987,11 @@ class RelationshipProperty(StrategizedProperty):
                 active_history=active_history,
                 cascade_backrefs=cascade_backrefs,
             )
+        if viewonly and sync_backref:
+            raise sa_exc.ArgumentError(
+                "sync_backref and viewonly cannot both be True"
+            )
+        self.sync_backref = sync_backref
         self.lazy = lazy
         self.single_parent = single_parent
         self._user_defined_foreign_keys = foreign_keys
@@ -1949,16 +1978,37 @@ class RelationshipProperty(StrategizedProperty):
 
             yield c, instance_mapper, instance_state, instance_dict
 
-    def _add_reverse_property(self, key):
-        other = self.mapper.get_property(key, _configure_mappers=False)
-        if other.viewonly:
+    @property
+    def _effective_sync_backref(self):
+        return self.sync_backref is not False
+
+    @staticmethod
+    def _check_sync_backref(rel_a, rel_b):
+        if rel_a.viewonly and rel_b.sync_backref:
+            raise sa_exc.InvalidRequestError(
+                "Relationship %s cannot specify sync_backref=True since %s "
+                "includes viewonly=True." % (rel_b, rel_a)
+            )
+        if rel_a.viewonly and rel_b.sync_backref is not False:
             util.warn_limited(
                 "Setting backref / back_populates on relationship %s to refer "
-                "to viewonly relationship %s will be deprecated in SQLAlchemy "
-                "1.4, and will be disallowed in a future release.  "
-                "viewonly relationships should not be mutated",
-                (self, other),
+                "to viewonly relationship %s should include "
+                "sync_backref=False set on the %s relationship. ",
+                (rel_b, rel_a, rel_b),
             )
+
+    def _add_reverse_property(self, key):
+        other = self.mapper.get_property(key, _configure_mappers=False)
+        # viewonly and sync_backref cases
+        # 1. self.viewonly==True and other.sync_backref==True -> error
+        # 2. self.viewonly==True and other.viewonly==False and
+        #    other.sync_backref==None -> warn sync_backref=False, set to False
+        self._check_sync_backref(self, other)
+        # 3. other.viewonly==True and self.sync_backref==True -> error
+        # 4. other.viewonly==True and self.viewonly==False and
+        #    self.sync_backref==None -> warn sync_backref=False, set to False
+        self._check_sync_backref(other, self)
+
         self._reverse_property.add(other)
         other._reverse_property.add(self)
 
@@ -2299,6 +2349,7 @@ class RelationshipProperty(StrategizedProperty):
             kwargs.setdefault("viewonly", self.viewonly)
             kwargs.setdefault("post_update", self.post_update)
             kwargs.setdefault("passive_updates", self.passive_updates)
+            kwargs.setdefault("sync_backref", self.sync_backref)
             self.back_populates = backref_key
             relationship = RelationshipProperty(
                 parent,

lib/sqlalchemy/orm/strategies.py

@@ -75,7 +75,7 @@ def _register_attribute(
     # after the singleparentvalidator, mapper validator
     if useobject:
         backref = prop.back_populates
-        if backref:
+        if backref and prop._effective_sync_backref:
             listen_hooks.append(
                 lambda desc, prop: attributes.backref_listeners(
                     desc, backref, uselist

test/orm/test_relationships.py

@@ -2705,8 +2705,9 @@ class ViewOnlyHistoryTest(fixtures.MappedTest):
         mapper(B, self.tables.t2)
 
         with testing.expect_warnings(
-            "Setting backref / back_populates on "
-            "relationship B.a to refer to viewonly relationship A.bs"
+            "Setting backref / back_populates on relationship B.a to refer "
+            "to viewonly relationship A.bs should include sync_backref=False "
+            "set on the B.a relationship."
         ):
             configure_mappers()
 
@@ -2742,8 +2743,9 @@ class ViewOnlyHistoryTest(fixtures.MappedTest):
         mapper(B, self.tables.t2)
 
         with testing.expect_warnings(
-            "Setting backref / back_populates on "
-            "relationship A.bs to refer to viewonly relationship B.a"
+            "Setting backref / back_populates on relationship A.bs to refer "
+            "to viewonly relationship B.a should include sync_backref=False "
+            "set on the A.bs relationship."
         ):
             configure_mappers()
 
@@ -2848,8 +2850,9 @@ class ViewOnlyM2MBackrefTest(fixtures.MappedTest):
         mapper(B, t2)
 
         with testing.expect_warnings(
-            "Setting backref / back_populates on "
-            "relationship A.bs to refer to viewonly relationship B.a"
+            "Setting backref / back_populates on relationship A.bs to refer "
+            "to viewonly relationship B.as_ should include sync_backref=False "
+            "set on the A.bs relationship."
         ):
             configure_mappers()
 
@@ -2957,6 +2960,184 @@ class ViewOnlyOverlappingNames(fixtures.MappedTest):
         assert set([x.id for x in c1.t2_view]) == set([c2b.id])
 
 
+class ViewOnlySyncBackref(fixtures.MappedTest):
+    @classmethod
+    def define_tables(cls, metadata):
+        Table(
+            "t1",
+            metadata,
+            Column(
+                "id", Integer, primary_key=True, test_needs_autoincrement=True
+            ),
+            Column("data", String(40)),
+        )
+        Table(
+            "t2",
+            metadata,
+            Column(
+                "id", Integer, primary_key=True, test_needs_autoincrement=True
+            ),
+            Column("data", String(40)),
+            Column("t1id", Integer, ForeignKey("t1.id")),
+        )
+
+    class Case:
+        def __init__(
+            self,
+            Ba_err=False,
+            Abs_err=False,
+            map_err=False,
+            ctor_warn=False,
+            Ba_evt=False,
+            Abs_evt=False,
+        ):
+            self.B_a_init_error = Ba_err
+            self.A_bs_init_error = Abs_err
+            self.map_error = map_err
+            self.ctor_warn = ctor_warn
+            self.B_a_event = Ba_evt
+            self.A_bs_event = Abs_evt
+
+        def __repr__(self):
+            return str(self.__dict__)
+
+    cases = {
+        (0, 0, 0, 0): Case(),
+        (0, 0, 0, 1): Case(Abs_evt=1),
+        (0, 0, 1, 0): Case(),
+        (0, 0, 1, 1): Case(Abs_err=1),
+        (0, 1, 0, 0): Case(Ba_evt=1),
+        (0, 1, 0, 1): Case(Ba_evt=1, Abs_evt=1),
+        (0, 1, 1, 0): Case(map_err="BA"),
+        (0, 1, 1, 1): Case(Abs_err=1),
+        (1, 0, 0, 0): Case(),
+        (1, 0, 0, 1): Case(map_err="AB"),
+        (1, 0, 1, 0): Case(),
+        (1, 0, 1, 1): Case(Abs_err=1),
+        (1, 1, 0, 0): Case(Ba_err=1),
+        (1, 1, 0, 1): Case(Ba_err=1),
+        (1, 1, 1, 0): Case(Ba_err=1),
+        (1, 1, 1, 1): Case(Abs_err=1),
+        (0, None, 0, 0): Case(Ba_evt=1),
+        (0, None, 0, 1): Case(Ba_evt=1, Abs_evt=1),
+        (0, None, 1, 0): Case(ctor_warn="BA", Ba_evt=1),
+        (0, None, 1, 1): Case(Abs_err=1),
+        (1, None, 0, 0): Case(Ba_evt=1),
+        (1, None, 0, 1): Case(map_err="AB"),
+        (1, None, 1, 0): Case(ctor_warn="BA", Ba_evt=1),
+        (1, None, 1, 1): Case(Abs_err=1),
+        (0, 0, 0, None): Case(Abs_evt=1),
+        (0, 0, 1, None): Case(Abs_evt=1),
+        (0, 1, 0, None): Case(Ba_evt=1, Abs_evt=1),
+        (0, 1, 1, None): Case(map_err="BA"),
+        (1, 0, 0, None): Case(ctor_warn="AB", Abs_evt=1),
+        (1, 0, 1, None): Case(ctor_warn="AB", Abs_evt=1),
+        (1, 1, 0, None): Case(Ba_err=1),
+        (1, 1, 1, None): Case(Ba_err=1),
+        (0, None, 0, None): Case(Ba_evt=1, Abs_evt=1),
+        (0, None, 1, None): Case(ctor_warn="BA", Abs_evt=1, Ba_evt=1),
+        (1, None, 0, None): Case(ctor_warn="AB", Abs_evt=1, Ba_evt=1),
+        (1, None, 1, None): Case(ctor_warn="*", Abs_evt=1, Ba_evt=1),
+    }
+
+    @testing.combinations(True, False, None, argnames="A_bs_sync")
+    @testing.combinations(True, False, argnames="A_bs_view")
+    @testing.combinations(True, False, None, argnames="B_a_sync")
+    @testing.combinations(True, False, argnames="B_a_view")
+    def test_case(self, B_a_view, B_a_sync, A_bs_view, A_bs_sync):
+        class A(fixtures.ComparableEntity):
+            pass
+
+        class B(fixtures.ComparableEntity):
+            pass
+
+        case = self.cases[(B_a_view, B_a_sync, A_bs_view, A_bs_sync)]
+        print(
+            {
+                "B_a_view": B_a_view,
+                "B_a_sync": B_a_sync,
+                "A_bs_view": A_bs_view,
+                "A_bs_sync": A_bs_sync,
+            },
+            case,
+        )
+
+        def rel():
+            return relationship(
+                B,
+                viewonly=A_bs_view,
+                sync_backref=A_bs_sync,
+                backref=backref("a", viewonly=B_a_view, sync_backref=B_a_sync),
+            )
+
+        if case.A_bs_init_error:
+            assert_raises_message(
+                exc.ArgumentError,
+                "sync_backref and viewonly cannot both be True",
+                rel,
+            )
+            return
+
+        mapper(
+            A, self.tables.t1, properties={"bs": rel()},
+        )
+        mapper(B, self.tables.t2)
+
+        if case.B_a_init_error:
+            assert_raises_message(
+                exc.ArgumentError,
+                "sync_backref and viewonly cannot both be True",
+                configure_mappers,
+            )
+            return
+
+        if case.map_error:
+            if case.map_error == "AB":
+                args = ("A.bs", "B.a")
+            else:
+                args = ("B.a", "A.bs")
+            assert_raises_message(
+                exc.InvalidRequestError,
+                "Relationship %s cannot specify sync_backref=True since %s "
+                % args,
+                configure_mappers,
+            )
+            return
+
+        if case.ctor_warn:
+            warns = []
+            msg = (
+                "Setting backref / back_populates on relationship %s "
+                "to refer to viewonly relationship %s"
+            )
+            if case.ctor_warn in ("AB", "*"):
+                warns.append(msg % ("A.bs", "B.a"))
+            if case.ctor_warn in ("BA", "*"):
+                warns.append(msg % ("B.a", "A.bs"))
+            with testing.expect_warnings(*warns):
+                configure_mappers()
+        else:
+            configure_mappers()
+
+        a1 = A()
+        b1 = B()
+        b1.a = a1
+        assert (b1 in a1.bs) == case.B_a_event
+        assert inspect(a1).attrs.bs.history.has_changes() == (
+            case.B_a_event and not A_bs_view
+        )
+        assert inspect(b1).attrs.a.history.has_changes() == (not B_a_view)
+
+        a2 = A()
+        b2 = B()
+        a2.bs.append(b2)
+        assert (b2.a == a2) == case.A_bs_event
+        assert inspect(a2).attrs.bs.history.has_changes() == (not A_bs_view)
+        assert inspect(b2).attrs.a.history.has_changes() == (
+            case.A_bs_event and not B_a_view
+        )
+
+
 class ViewOnlyUniqueNames(fixtures.MappedTest):
 
     """'viewonly' mappings with unique PK column names."""