From fdfc2954420ebb83fbbdc7f2ae5aeb3742e2b167 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Wed, 22 Apr 2020 23:25:57 +0200 Subject: [PATCH] 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 04c990a011db5629f1a53a8e5af2080180ac8ec3) --- .gitignore | 1 + doc/build/changelog/unreleased_13/5237.rst | 10 ++ lib/sqlalchemy/orm/relationships.py | 101 ++++++++--- lib/sqlalchemy/orm/strategies.py | 2 +- test/orm/test_relationships.py | 193 ++++++++++++++++++++- 5 files changed, 275 insertions(+), 32 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/5237.rst diff --git a/.gitignore b/.gitignore index 6d6f6788d4..f6d57def8f 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ /build/ /dist/ /doc/build/output/ +/doc/build/_build/ /dogpile_data/ *.orig *,cover diff --git a/doc/build/changelog/unreleased_13/5237.rst b/doc/build/changelog/unreleased_13/5237.rst new file mode 100644 index 0000000000..9c62c8e3ac --- /dev/null +++ b/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. diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index 600f628648..a262d120df 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/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, diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index a75218c2b5..8a97bf6c9f 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/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 diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py index 6cec819ce7..0ac0c172a9 100644 --- a/test/orm/test_relationships.py +++ b/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.""" -- 2.39.5