Move setup functionality into _register_attribute

Options like uselist and backref can be determined from
within _register_attribute based on parent_property
given; move this logic inside so that individual strategies
have less responsibility.  Also don't require that
_register_attribute consider the "strategy" itself
at all; it would be better if we could no longer require
that Joined/Subquery/etc loaders call upon the "lazy" strategy
in order to initialize attribute instrumentation and
this could be done more generically.

Fixes long-standing bug where the "noload" relationship loading
strategy would cause backrefs and/or back_populates options to be
ignored.   There is concern that some application that uses
"noload" might be surprised at a back-populating attribute
appearing suddenly, which may have side effects.  However,
"noload" itself must be extremely seldom used since as a
strategy, it already disables loading, population of attributes
is the only behavior that is even supported, so that this issue has existed
for at least through 0.7 four years ago without ever being
reported indicates extremely low use of this option.

Change-Id: Icffb9c83ac5782b76ce882ed1df4361a1efbfba3
Fixes: #3845

diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst
index 05c4529bb71c9e61c9ab027348171434e4fc691d..afa4389a8862a034da47053712736235b3c97b2c 100644 (file)
--- a/doc/build/changelog/changelog_11.rst
+++ b/doc/build/changelog/changelog_11.rst
@@ -21,6 +21,14 @@
 .. changelog::
     :version: 1.1.4
 
+    .. change::
+        :tags: bug, orm
+        :tickets: 3845
+
+        Fixed long-standing bug where the "noload" relationship loading
+        strategy would cause backrefs and/or back_populates options to be
+        ignored.
+
     .. change::
         :tags: bug, mysql
         :tickets: 3841

diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py
index 026ebc317eb80afa0113158ba04cae09d98f96bd..867c6ee15fe96fd1d905a51ac545fcfb7fde49fb 100644 (file)
--- a/lib/sqlalchemy/orm/dynamic.py
+++ b/lib/sqlalchemy/orm/dynamic.py
@@ -32,15 +32,13 @@ class DynaLoader(strategies.AbstractRelationshipLoader):
                 "many-to-one/one-to-one relationships and/or "
                 "uselist=False." % self.parent_property)
         strategies._register_attribute(
-            self,
+            self.parent_property,
             mapper,
             useobject=True,
-            uselist=True,
             impl_class=DynamicAttributeImpl,
             target_mapper=self.parent_property.mapper,
             order_by=self.parent_property.order_by,
             query_class=self.parent_property.query_class,
-            backref=self.parent_property.back_populates,
         )
 
 

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 5f5ab10690c7ceb1db530e251d61b2db9a55161d..33feab0dc37a556e4ee93539502d4992bd0ef907 100644 (file)
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -28,10 +28,9 @@ import itertools
 
 
 def _register_attribute(
-    strategy, mapper, useobject,
+    prop, mapper, useobject,
     compare_function=None,
     typecallable=None,
-    uselist=False,
     callable_=None,
     proxy_property=None,
     active_history=False,
@@ -39,12 +38,12 @@ def _register_attribute(
     **kw
 ):
 
-    prop = strategy.parent_property
-
     attribute_ext = list(util.to_list(prop.extension, default=[]))
 
     listen_hooks = []
 
+    uselist = useobject and prop.uselist
+
     if useobject and prop.single_parent:
         listen_hooks.append(single_parent_validator)
 
@@ -61,15 +60,16 @@ def _register_attribute(
 
     # need to assemble backref listeners
     # after the singleparentvalidator, mapper validator
-    backref = kw.pop('backref', None)
-    if backref:
-        listen_hooks.append(
-            lambda desc, prop: attributes.backref_listeners(
-                desc,
-                backref,
-                uselist
+    if useobject:
+        backref = prop.back_populates
+        if backref:
+            listen_hooks.append(
+                lambda desc, prop: attributes.backref_listeners(
+                    desc,
+                    backref,
+                    uselist
+                )
             )
-        )
 
     # a single MapperProperty is shared down a class inheritance
     # hierarchy, so we set up attribute instrumentation and backref event
@@ -173,7 +173,7 @@ class ColumnLoader(LoaderStrategy):
             mapper.version_id_col in set(self.columns)
 
         _register_attribute(
-            self, mapper, useobject=False,
+            self.parent_property, mapper, useobject=False,
             compare_function=coltype.compare_values,
             active_history=active_history
         )
@@ -228,7 +228,7 @@ class DeferredColumnLoader(LoaderStrategy):
         self.is_class_level = True
 
         _register_attribute(
-            self, mapper, useobject=False,
+            self.parent_property, mapper, useobject=False,
             compare_function=self.columns[0].type.compare_values,
             callable_=self._load_for_state,
             expire_missing=False
@@ -350,9 +350,8 @@ class NoLoader(AbstractRelationshipLoader):
         self.is_class_level = True
 
         _register_attribute(
-            self, mapper,
+            self.parent_property, mapper,
             useobject=True,
-            uselist=self.parent_property.uselist,
             typecallable=self.parent_property.collection_class,
         )
 
@@ -434,12 +433,10 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots):
         # in that case.  otherwise we don't need the
         # "old" value during backref operations.
         _register_attribute(
-            self,
+            self.parent_property,
             mapper,
             useobject=True,
             callable_=self._load_for_state,
-            uselist=self.parent_property.uselist,
-            backref=self.parent_property.back_populates,
             typecallable=self.parent_property.collection_class,
             active_history=active_history
         )

diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py
index 429e0f3083e5619116d0642514dcceb34374a8c9..93db643a6a852113b62ea97dbf9664c26bfecfac 100644 (file)
--- a/test/orm/test_relationships.py
+++ b/test/orm/test_relationships.py
@@ -11,7 +11,7 @@ from sqlalchemy.orm import mapper, relationship, relation, \
     Session, composite, column_property, foreign,\
     remote, synonym, joinedload, subqueryload
 from sqlalchemy.orm.interfaces import ONETOMANY, MANYTOONE
-from sqlalchemy.testing import eq_, startswith_, AssertsCompiledSQL, is_
+from sqlalchemy.testing import eq_, startswith_, AssertsCompiledSQL, is_, in_
 from sqlalchemy.testing import fixtures
 from test.orm import _fixtures
 from sqlalchemy import exc
@@ -1621,6 +1621,54 @@ class ManualBackrefTest(_fixtures.FixtureTest):
                               configure_mappers)
 
 
+
+class NoLoadBackPopulates(_fixtures.FixtureTest):
+
+    """test the noload stratgegy which unlike others doesn't use
+    lazyloader to set up instrumentation"""
+
+    def test_o2m(self):
+        users, Address, addresses, User = (self.tables.users,
+                                           self.classes.Address,
+                                           self.tables.addresses,
+                                           self.classes.User)
+
+        mapper(User, users, properties={
+            'addresses': relationship(
+                Address, back_populates='user', lazy="noload")
+        })
+
+        mapper(Address, addresses, properties={
+            'user': relationship(User)
+        })
+
+        u1 = User()
+        a1 = Address()
+        u1.addresses.append(a1)
+        is_(a1.user, u1)
+
+    def test_m2o(self):
+        users, Address, addresses, User = (self.tables.users,
+                                           self.classes.Address,
+                                           self.tables.addresses,
+                                           self.classes.User)
+
+        mapper(User, users, properties={
+            'addresses': relationship(
+                Address)
+        })
+
+        mapper(Address, addresses, properties={
+            'user': relationship(
+                User, back_populates='addresses', lazy="noload")
+        })
+
+        u1 = User()
+        a1 = Address()
+        a1.user = u1
+        in_(a1, u1.addresses)
+
+
 class JoinConditionErrorTest(fixtures.TestBase):
 
     def test_clauseelement_pj(self):