- It is now an error to specify both columns of a binary primaryjoin

condition in the foreign_keys or remote_side collection.  Whereas
previously it was just nonsensical, but would succeed in a
non-deterministic way.

diff --git a/CHANGES b/CHANGES
index a6bedd4bf6d49aee2b683ccf2be6ea2853e39f73..04e7d2d145c9c257074ee82e35f32e4c17a28561 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -66,6 +66,11 @@ CHANGES
     - Fixed Query.update() and Query.delete() failures with eagerloaded
       relations. [ticket:1378]
 
+    - It is now an error to specify both columns of a binary primaryjoin
+      condition in the foreign_keys or remote_side collection.  Whereas
+      previously it was just nonsensical, but would succeed in a 
+      non-deterministic way.
+      
 - schema
     - Added a quote_schema() method to the IdentifierPreparer class
       so that dialects can override how schemas get handled. This

diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index 398cbe5d989a8a2c9447189e22d012619b1754bb..d0cca2dc1145a0c6d1f60af45787de550beef63f 100644 (file)
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -839,8 +839,8 @@ class RelationProperty(StrategizedProperty):
                     if self._foreign_keys:
                         raise sa_exc.ArgumentError("Could not determine relation direction for "
                             "primaryjoin condition '%s', on relation %s. "
-                            "Are the columns in 'foreign_keys' present within the given "
-                            "join condition ?" % (self.primaryjoin, self))
+                            "Do the columns in 'foreign_keys' represent only the 'foreign' columns "
+                            "in this join condition ?" % (self.primaryjoin, self))
                     else:
                         raise sa_exc.ArgumentError("Could not determine relation direction for "
                             "primaryjoin condition '%s', on relation %s. "

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index 36357faf505c5a171bb0a3896b6fd23737070c36..f1f329b5e27a31b60b52bd022b2ab69bedb903e2 100644 (file)
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -343,14 +343,14 @@ def criterion_as_pairs(expression, consider_as_foreign_keys=None, consider_as_re
             return
 
         if consider_as_foreign_keys:
-            if binary.left in consider_as_foreign_keys:
+            if binary.left in consider_as_foreign_keys and (binary.right is binary.left or binary.right not in consider_as_foreign_keys):
                 pairs.append((binary.right, binary.left))
-            elif binary.right in consider_as_foreign_keys:
+            elif binary.right in consider_as_foreign_keys and (binary.left is binary.right or binary.left not in consider_as_foreign_keys):
                 pairs.append((binary.left, binary.right))
         elif consider_as_referenced_keys:
-            if binary.left in consider_as_referenced_keys:
+            if binary.left in consider_as_referenced_keys and (binary.right is binary.left or binary.right not in consider_as_referenced_keys):
                 pairs.append((binary.left, binary.right))
-            elif binary.right in consider_as_referenced_keys:
+            elif binary.right in consider_as_referenced_keys and (binary.left is binary.right or binary.left not in consider_as_referenced_keys):
                 pairs.append((binary.right, binary.left))
         else:
             if isinstance(binary.left, schema.Column) and isinstance(binary.right, schema.Column):

diff --git a/test/orm/relationships.py b/test/orm/relationships.py
index 88f132eae25d31add9eaacf138e821f4a6b2c01d..a0a8900b2c07a2d25072bb278555eba00e191f16 100644 (file)
--- a/test/orm/relationships.py
+++ b/test/orm/relationships.py
@@ -835,7 +835,7 @@ class JoinConditionErrorTest(testing.TestBase):
         mapper(C2, t3)
         
         self.assertRaises(sa.exc.NoReferencedColumnError, compile_mappers)
-
+    
     def test_join_error_raised(self):
         m = MetaData()
         t1 = Table('t1', m, 
@@ -1640,6 +1640,53 @@ class InvalidRelationEscalationTest(_base.MappedTest):
             "Could not locate any equated, locally mapped column pairs "
             "for primaryjoin condition", sa.orm.compile_mappers)
 
+    @testing.resolve_artifact_names
+    def test_ambiguous_fks(self):
+        mapper(Foo, foos, properties={
+            'bars':relation(Bar,
+                            primaryjoin=foos.c.id==bars.c.fid,
+                            foreign_keys=[foos.c.id, bars.c.fid])})
+        mapper(Bar, bars)
+
+        self.assertRaisesMessage(
+            sa.exc.ArgumentError, 
+                "Do the columns in 'foreign_keys' represent only the "
+                "'foreign' columns in this join condition ?", 
+                sa.orm.compile_mappers)
+
+    @testing.resolve_artifact_names
+    def test_ambiguous_remoteside_o2m(self):
+        mapper(Foo, foos, properties={
+            'bars':relation(Bar,
+                            primaryjoin=foos.c.id==bars.c.fid,
+                            foreign_keys=[bars.c.fid],
+                            remote_side=[foos.c.id, bars.c.fid],
+                            viewonly=True
+                            )})
+        mapper(Bar, bars)
+
+        self.assertRaisesMessage(
+            sa.exc.ArgumentError, 
+                "could not determine any local/remote column pairs",
+                sa.orm.compile_mappers)
+
+    @testing.resolve_artifact_names
+    def test_ambiguous_remoteside_m2o(self):
+        mapper(Foo, foos, properties={
+            'bars':relation(Bar,
+                            primaryjoin=foos.c.id==bars.c.fid,
+                            foreign_keys=[foos.c.id],
+                            remote_side=[foos.c.id, bars.c.fid],
+                            viewonly=True
+                            )})
+        mapper(Bar, bars)
+
+        self.assertRaisesMessage(
+            sa.exc.ArgumentError, 
+                "could not determine any local/remote column pairs",
+                sa.orm.compile_mappers)
+        
+    
     @testing.resolve_artifact_names
     def test_no_equated_self_ref(self):
         mapper(Foo, foos, properties={