]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
- Improved the error message emitted when a "backref loop" is detected,
authorMike Bayer <mike_mp@zzzcomputing.com>
Sun, 3 Mar 2013 18:59:12 +0000 (13:59 -0500)
committerMike Bayer <mike_mp@zzzcomputing.com>
Sun, 3 Mar 2013 18:59:12 +0000 (13:59 -0500)
that is when an attribute event triggers a bidirectional
assignment between two other attributes with no end.
This condition can occur not just when an object of the wrong
type is assigned, but also when an attribute is mis-configured
to backref into an existing backref pair.
- A warning is emitted when a MapperProperty is assigned to a mapper
that replaces an existing property, if the properties in question
aren't plain column-based properties.   Replacement of relationship
properties is rarely (ever?) what is intended and usually refers to a
mapper mis-configuration.   This will also warn if a backref configures
itself on top of an existing one in an inheritance relationship
(which is an error in 0.8).
backport for [ticket:2674]

doc/build/changelog/changelog_07.rst
lib/sqlalchemy/orm/attributes.py
lib/sqlalchemy/orm/mapper.py
test/orm/test_attributes.py
test/orm/test_compile.py
test/orm/test_mapper.py

index f07c9eccd0da36db21f1afa49fca40ad303016fb..aeb7251db50a27c563b9cc484505c3bba0acf83b 100644 (file)
       Fixed an import of "logging" in test_execute which was not
       working on some linux platforms.
 
+    .. change::
+        :tags: bug, orm
+        :tickets: 2674
+
+      Improved the error message emitted when a "backref loop" is detected,
+      that is when an attribute event triggers a bidirectional
+      assignment between two other attributes with no end.
+      This condition can occur not just when an object of the wrong
+      type is assigned, but also when an attribute is mis-configured
+      to backref into an existing backref pair.
+
+    .. change::
+      :tags: bug, orm
+      :tickets: 2674
+
+      A warning is emitted when a MapperProperty is assigned to a mapper
+      that replaces an existing property, if the properties in question
+      aren't plain column-based properties.   Replacement of relationship
+      properties is rarely (ever?) what is intended and usually refers to a
+      mapper mis-configuration.   This will also warn if a backref configures
+      itself on top of an existing one in an inheritance relationship
+      (which is an error in 0.8).
+
 .. changelog::
     :version: 0.7.10
     :released: Thu Feb 7 2013
index b40abd57bce549233b6dfe5c39fa8fa7dfafbede..59cc461ff9c6503605cae781ea6e488caffa6484 100644 (file)
@@ -319,6 +319,9 @@ class AttributeImpl(object):
 
         self.expire_missing = expire_missing
 
+    def __str__(self):
+        return "%s.%s" % (self.class_.__name__, self.key)
+
     def _get_active_history(self):
         """Backwards compat for impl.active_history"""
 
@@ -975,11 +978,18 @@ def backref_listeners(attribute, key, uselist):
 
     parent_token = attribute.impl.parent_token
 
-    def _acceptable_key_err(child_state, initiator):
+    def _acceptable_key_err(child_state, initiator, child_impl):
         raise ValueError(
-            "Object %s not associated with attribute of "
-            "type %s" % (mapperutil.state_str(child_state),
-                    manager_of_class(initiator.class_)[initiator.key]))
+            "Bidirectional attribute conflict detected: "
+            'Passing object %s to attribute "%s" '
+            'triggers a modify event on attribute "%s" '
+            'via the backref "%s".' % (
+                mapperutil.state_str(child_state),
+                initiator.parent_token,
+                child_impl.parent_token,
+                attribute.impl.parent_token
+            )
+        )
 
     def emit_backref_from_scalar_set_event(state, child, oldchild, initiator):
         if oldchild is child:
@@ -1002,7 +1012,7 @@ def backref_listeners(attribute, key, uselist):
             child_impl = child_state.manager[key].impl
             if initiator.parent_token is not parent_token and \
                 initiator.parent_token is not child_impl.parent_token:
-                _acceptable_key_err(state, initiator)
+                _acceptable_key_err(state, initiator, child_impl)
             child_impl.append(
                                 child_state,
                                 child_dict,
@@ -1015,9 +1025,11 @@ def backref_listeners(attribute, key, uselist):
         child_state, child_dict = instance_state(child), \
                                     instance_dict(child)
         child_impl = child_state.manager[key].impl
+
+        print initiator.parent_token, parent_token, child_impl.parent_token
         if initiator.parent_token is not parent_token and \
             initiator.parent_token is not child_impl.parent_token:
-            _acceptable_key_err(state, initiator)
+            _acceptable_key_err(state, initiator, child_impl)
         child_impl.append(
                                 child_state,
                                 child_dict,
index de4d351b0ccd387fa9d902a691ac822a22a38592..36771de234076d1f72707b8a5b4bf2fd55a68168 100644 (file)
@@ -1101,7 +1101,7 @@ class Mapper(object):
                 # initialized; check for 'readonly'
                 if hasattr(self, '_readonly_props') and \
                     (not hasattr(col, 'table') or
-                    col.table not in self._cols_by_table):
+                        col.table not in self._cols_by_table):
                         self._readonly_props.add(prop)
 
             else:
@@ -1139,6 +1139,16 @@ class Mapper(object):
                         "%r for column %r" % (syn, key, key, syn)
                     )
 
+        if key in self._props and \
+                not isinstance(prop, properties.ColumnProperty) and \
+                not isinstance(self._props[key], properties.ColumnProperty):
+            util.warn("Property %s on %s being replaced with new "
+                            "property %s; the old property will be discarded" % (
+                            self._props[key],
+                            self,
+                            prop,
+                        ))
+
         self._props[key] = prop
 
         if not self.non_primary:
index 7a4ef038e880cdc6a91ce3286a4f836173917a74..fecd23c9337f6487018773e43a6255e71e65e295 100644 (file)
@@ -1217,7 +1217,10 @@ class CyclicBackrefAssertionTest(fixtures.TestBase):
         b1 = B()
         assert_raises_message(
             ValueError,
-            "Object <B at .*> not associated with attribute of type C.a",
+            'Bidirectional attribute conflict detected: '
+            'Passing object <B at .*> to attribute "C.a" '
+            'triggers a modify event on attribute "C.b" '
+            'via the backref "B.c".',
             setattr, c1, 'a', b1
         )
 
@@ -1227,10 +1230,14 @@ class CyclicBackrefAssertionTest(fixtures.TestBase):
         b1 = B()
         assert_raises_message(
             ValueError,
-            "Object <B at .*> not associated with attribute of type C.a",
+            'Bidirectional attribute conflict detected: '
+            'Passing object <B at .*> to attribute "C.a" '
+            'triggers a modify event on attribute "C.b" '
+            'via the backref "B.c".',
             c1.a.append, b1
         )
 
+
     def _scalar_fixture(self):
         class A(object):
             pass
@@ -1272,6 +1279,36 @@ class CyclicBackrefAssertionTest(fixtures.TestBase):
 
         return A, B, C
 
+    def _broken_collection_fixture(self):
+        class A(object):
+            pass
+        class B(object):
+            pass
+        instrumentation.register_class(A)
+        instrumentation.register_class(B)
+
+        attributes.register_attribute(A, 'b', backref='a1', useobject=True)
+        attributes.register_attribute(B, 'a1', backref='b', useobject=True,
+                                                uselist=True)
+
+        attributes.register_attribute(B, 'a2', backref='b', useobject=True,
+                                                uselist=True)
+
+        return A, B
+
+    def test_broken_collection_assertion(self):
+        A, B = self._broken_collection_fixture()
+        b1 = B()
+        a1 = A()
+        assert_raises_message(
+            ValueError,
+            'Bidirectional attribute conflict detected: '
+            'Passing object <A at .*> to attribute "B.a2" '
+            'triggers a modify event on attribute "B.a1" '
+            'via the backref "A.b".',
+            b1.a2.append, a1
+        )
+
 class PendingBackrefTest(fixtures.ORMTest):
     def setup(self):
         global Post, Blog, called, lazy_load
index 1b2714d7001bdb7265f0444ae257b0c5521d8c5f..9e058e372ee5978b49a93ba35088e1dde5f20c4c 100644 (file)
@@ -167,8 +167,10 @@ class CompileTest(fixtures.ORMTest):
         b = Table('b', meta, Column('id', Integer, primary_key=True),
                                 Column('a_id', Integer, ForeignKey('a.id')))
 
-        class A(object):pass
-        class B(object):pass
+        class A(object):
+            pass
+        class B(object):
+            pass
 
         mapper(A, a, properties={
             'b':relationship(B, backref='a')
@@ -183,3 +185,32 @@ class CompileTest(fixtures.ORMTest):
             configure_mappers
         )
 
+    def test_conflicting_backref_subclass(self):
+        meta = MetaData()
+
+        a = Table('a', meta, Column('id', Integer, primary_key=True))
+        b = Table('b', meta, Column('id', Integer, primary_key=True),
+                                Column('a_id', Integer, ForeignKey('a.id')))
+
+        class A(object):
+            pass
+        class B(object):
+            pass
+        class C(B):
+            pass
+
+        mapper(A, a, properties={
+            'b': relationship(B, backref='a'),
+            'c': relationship(C, backref='a')
+        })
+        mapper(B, b)
+        mapper(C, None, inherits=B)
+
+        # see [ticket:2674] - 0.8 has an expanded check for
+        # conflicting backrefs that raises an ArgumentError
+        assert_raises_message(
+            sa_exc.SAWarning,
+            "Property C.a on Mapper|C|b being replaced with new "
+            "property B.a; the old property will be discarded",
+            configure_mappers
+        )
index 5a88d937fd44fb72d456061318bf94d4b8c7507c..39e65335728c80f42a1d8cd4856989f8fdc7830a 100644 (file)
@@ -447,7 +447,7 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL):
         assert hasattr(User, 'addresses')
         assert "addresses" in [p.key for p in m1._polymorphic_properties]
 
-    def test_replace_property(self):
+    def test_replace_col_prop_w_syn(self):
         users, User = self.tables.users, self.classes.User
 
         m = mapper(User, users)
@@ -473,6 +473,24 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL):
         u.name = 'jacko'
         assert m._columntoproperty[users.c.name] is m.get_property('_name')
 
+    def test_replace_rel_prop_with_rel_warns(self):
+        users, User = self.tables.users, self.classes.User
+        addresses, Address = self.tables.addresses, self.classes.Address
+
+        m = mapper(User, users, properties={
+                "addresses": relationship(Address)
+            })
+        mapper(Address, addresses)
+
+        assert_raises_message(
+            sa.exc.SAWarning,
+            "Property User.addresses on Mapper|User|users being replaced "
+            "with new property User.addresses; the old property will "
+            "be discarded",
+            m.add_property,
+            "addresses", relationship(Address)
+        )
+
     def test_add_column_prop_deannotate(self):
         User, users = self.classes.User, self.tables.users
         Address, addresses = self.classes.Address, self.tables.addresses