Merge "Imply `sync_backref` flag in a viewonly relationship"

4 files changed, 112 insertions, 82 deletions

diff --git a/doc/build/changelog/migration_14.rst b/doc/build/changelog/migration_14.rst
index 93fde1e8b..ff4d58da7 100644
--- a/doc/build/changelog/migration_14.rst
+++ b/doc/build/changelog/migration_14.rst
@@ -1083,6 +1083,56 @@ these operations were already operating in an on-demand fashion.
 
 :ticket:`5074`
 
+.. _change_5237_14:
+
+Viewonly relationships don't synchronize backrefs
+-------------------------------------------------
+
+In :ticket:`5149` in 1.3.14, SQLAlchemy began emitting a warning when the
+:paramref:`_orm.relationship.backref` or :paramref:`_orm.relationship.back_populates`
+keywords would be used at the same time as the :paramref:`_orm.relationship.viewonly`
+flag on the target relationship.  This was because a "viewonly" relationship does
+not actually persist changes made to it, which could cause some misleading
+behaviors to occur.  However, in :ticket:`5237`, we sought to refine this
+behavior as there are legitimate use cases to have backrefs set up on
+viewonly relationships, including that back populates attributes are used
+in some cases by the relationship lazy loaders to determine that an additional
+eager load in the other direction is not necessary, as well as that back
+populates can be used for mapper introspection and that :func:`_orm.backref`
+can be a convenient way to set up bi-directional relationships.
+
+The solution then was to make the "mutation" that occurs from a backref
+an optional thing, using the :paramref:`_orm.relationship.sync_backref`
+flag.  In 1.4 the value of :paramref:`_orm.relationship.sync_backref` defaults
+to False for a relationship target that also sets :paramref:`_orm.relationship.viewonly`.
+This indicates that any changes made to a relationship with
+viewonly will not impact the state of the other side or of the :class:`_orm.Session`
+in any way::
+
+
+    class User(Base):
+        # ...
+
+        addresses = relationship(Address, backref=backref("user", viewonly=True))
+
+    class Address(Base):
+        # ...
+
+
+    u1 = session.query(User).filter_by(name="x").first()
+
+    a1 = Address()
+    a1.user = u1
+
+Above, the ``a1`` object will **not** be added to the ``u1.addresses``
+collection, nor will the ``a1`` object be added to the session.  Previously,
+both of these things would be true.   The warning that
+:paramref:`.relationship.sync_backref` should be set to ``False`` when
+:paramref:`.relationship.viewonly` is ``False`` is no longer emitted as this is
+now the default behavior.
+
+:ticket:`5237`
+
 .. _change_1763:
 
 Eager loaders emit during unexpire operations
diff --git a/doc/build/changelog/unreleased_14/5237.rst b/doc/build/changelog/unreleased_14/5237.rst
new file mode 100644
index 000000000..2a1e5a377
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/5237.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: orm, usecase
+    :tickets: 5237
+
+    Update :paramref:`_orm.relationship.sync_backref` flag in a relationship
+    to make it implicitly ``False`` in ``viewonly=True`` relationships,
+    preventing synchronization events.
+
+
+    .. seealso::
+
+        :ref:`change_5237_14`
\ No newline at end of file
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 0be15260e..d4c5b4665 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -920,17 +920,14 @@ class RelationshipProperty(StrategizedProperty):
           collection from resulting in persistence operations.
 
           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.
+          conjunction with backrefs, the originating relationship for a
+          particular state change will not produce state changes within the
+          viewonly relationship.   This is the behavior implied by
+          :paramref:`_orm.relationship.sync_backref` being set to False.
+
+          .. versionchanged:: 1.3.17 - the
+             :paramref:`_orm.relationship.sync_backref` flag is set to False
+                 when using viewonly in conjunction with backrefs.
 
           .. seealso::
 
@@ -945,12 +942,15 @@ class RelationshipProperty(StrategizedProperty):
           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``.
+          default, changes in state will be back-populated only if neither
+          sides of a relationship is viewonly.
 
           .. versionadded:: 1.3.17
 
+          .. versionchanged:: 1.4 - A relationship that specifies
+             :paramref:`_orm.relationship.viewonly` automatically implies
+             that :paramref:`_orm.relationship.sync_backref` is ``False``.
+
           .. seealso::
 
             :paramref:`_orm.relationship.viewonly`
@@ -1998,7 +1998,10 @@ class RelationshipProperty(StrategizedProperty):
 
     @property
     def _effective_sync_backref(self):
-        return self.sync_backref is not False
+        if self.viewonly:
+            return False
+        else:
+            return self.sync_backref is not False
 
     @staticmethod
     def _check_sync_backref(rel_a, rel_b):
@@ -2007,13 +2010,12 @@ class RelationshipProperty(StrategizedProperty):
                 "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 should include "
-                "sync_backref=False set on the %s relationship. ",
-                (rel_b, rel_a, rel_b),
-            )
+        if (
+            rel_a.viewonly
+            and not rel_b.viewonly
+            and rel_b.sync_backref is not False
+        ):
+            rel_b.sync_backref = False
 
     def _add_reverse_property(self, key):
         other = self.mapper.get_property(key, _configure_mappers=False)
diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py
index 22b028c47..75d047b72 100644
--- a/test/orm/test_relationships.py
+++ b/test/orm/test_relationships.py
@@ -2838,25 +2838,20 @@ 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 should include sync_backref=False "
-            "set on the B.a relationship."
-        ):
-            configure_mappers()
+        configure_mappers()
 
         a1 = A()
         b1 = B()
         a1.bs.append(b1)
-        assert b1.a is a1
+        assert b1.a is None
         assert not inspect(a1).attrs.bs.history.has_changes()
-        assert inspect(b1).attrs.a.history.has_changes()
+        assert not inspect(b1).attrs.a.history.has_changes()
 
-        sess = self._assert_fk(a1, b1, True)
+        sess = self._assert_fk(a1, b1, False)
 
         a1.bs.remove(b1)
         assert a1 not in sess.dirty
-        assert b1 in sess.dirty
+        assert b1 not in sess.dirty
 
     def test_m2o_viewonly_oneside(self):
         class A(fixtures.ComparableEntity):
@@ -2876,24 +2871,19 @@ 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 should include sync_backref=False "
-            "set on the A.bs relationship."
-        ):
-            configure_mappers()
+        configure_mappers()
 
         a1 = A()
         b1 = B()
         b1.a = a1
-        assert b1 in a1.bs
-        assert inspect(a1).attrs.bs.history.has_changes()
+        assert b1 not in a1.bs
+        assert not inspect(a1).attrs.bs.history.has_changes()
         assert not inspect(b1).attrs.a.history.has_changes()
 
-        sess = self._assert_fk(a1, b1, True)
+        sess = self._assert_fk(a1, b1, False)
 
-        a1.bs.remove(b1)
-        assert a1 in sess.dirty
+        b1.a = None
+        assert a1 not in sess.dirty
         assert b1 not in sess.dirty
 
     def test_o2m_viewonly_only(self):
@@ -2983,12 +2973,7 @@ 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.as_ should include sync_backref=False "
-            "set on the A.bs relationship."
-        ):
-            configure_mappers()
+        configure_mappers()
 
         sess = create_session()
         a1 = A()
@@ -2998,8 +2983,8 @@ class ViewOnlyM2MBackrefTest(fixtures.MappedTest):
 
         sess.add(a1)
         sess.flush()
-        eq_(sess.query(A).first(), A(bs=[B(id=b1.id)]))
-        eq_(sess.query(B).first(), B(as_=[A(id=a1.id)]))
+        eq_(sess.query(A).first(), A(bs=[]))
+        eq_(sess.query(B).first(), None)
 
 
 class ViewOnlyOverlappingNames(fixtures.MappedTest):
@@ -3121,14 +3106,12 @@ class ViewOnlySyncBackref(fixtures.MappedTest):
             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
 
@@ -3154,24 +3137,24 @@ class ViewOnlySyncBackref(fixtures.MappedTest):
         (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, 0): Case(),
         (0, None, 1, 1): Case(Abs_err=1),
-        (1, None, 0, 0): Case(Ba_evt=1),
+        (1, None, 0, 0): Case(),
         (1, None, 0, 1): Case(map_err="AB"),
-        (1, None, 1, 0): Case(ctor_warn="BA", Ba_evt=1),
+        (1, None, 1, 0): Case(),
         (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, 0, 1, None): Case(),
         (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, 0, 0, None): Case(),
+        (1, 0, 1, None): Case(),
         (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),
+        (0, None, 1, None): Case(),
+        (1, None, 0, None): Case(),
+        (1, None, 1, None): Case(),
     }
 
     @testing.combinations(True, False, None, argnames="A_bs_sync")
@@ -3238,28 +3221,13 @@ class ViewOnlySyncBackref(fixtures.MappedTest):
             )
             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()
+        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(a1).attrs.bs.history.has_changes() == case.B_a_event
         assert inspect(b1).attrs.a.history.has_changes() == (not B_a_view)
 
         a2 = A()
@@ -3267,9 +3235,7 @@ class ViewOnlySyncBackref(fixtures.MappedTest):
         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
-        )
+        assert inspect(b2).attrs.a.history.has_changes() == case.A_bs_event
 
 
 class ViewOnlyUniqueNames(fixtures.MappedTest):