- some deeper error checking when compiling relations, to detect an ambiguous "prima...

 in the case that both sides of the relationship have foreign key references in the primary
 join condition

diff --git a/CHANGES b/CHANGES
index 0dc9411c840514386cdb9ad99a2e2482b96fe347..d8cff55dec3c8967acf3b3eae8986f592b365766 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -46,6 +46,9 @@
   - poked the first hole in the can of worms: saying query.select_by(somerelationname=someinstance)
   will create the join of the primary key columns represented by "somerelationname"'s mapper to the
   actual primary key in "someinstance".
+  - some deeper error checking when compiling relations, to detect an ambiguous "primaryjoin"
+  in the case that both sides of the relationship have foreign key references in the primary
+  join condition
   - added a mutex to the mapper compilation step.  ive been reluctant to add any kind
   of threading anything to SA but this is one spot that its its really needed since mappers
   are typically "global", and while their state does not change during normal operation, the 

diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index fc945fc7236850c6c1715acfce51aa9bf28b49b1..ef7c8236000fc64ad485a7eb8fd64b3e079f6958 100644 (file)
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -274,12 +274,17 @@ class PropertyLoader(StrategizedProperty):
                         return sync.ONETOMANY
                 else:
                     return sync.MANYTOONE
-        elif len([c for c in self.foreignkey if self.mapper.unjoined_table.corresponding_column(c, False) is not None]):
-            return sync.ONETOMANY
-        elif len([c for c in self.foreignkey if self.parent.unjoined_table.corresponding_column(c, False) is not None]):
-            return sync.MANYTOONE
         else:
-            raise exceptions.ArgumentError("Cant determine relation direction for '%s' in mapper '%s' with primary join\n '%s'" %(self.key, str(self.mapper), str(self.primaryjoin)))
+            onetomany = len([c for c in self.foreignkey if self.mapper.unjoined_table.corresponding_column(c, False) is not None])
+            manytoone = len([c for c in self.foreignkey if self.parent.unjoined_table.corresponding_column(c, False) is not None])
+            if not onetomany and not manytoone:
+                raise exceptions.ArgumentError("Cant determine relation direction for '%s' on mapper '%s' with primary join '%s' - foreign key columns are not present in neither the parent nor the child's mapped tables" %(self.key, str(self.parent), str(self.primaryjoin)))
+            elif onetomany and manytoone:
+                raise exceptions.ArgumentError("Cant determine relation direction for '%s' on mapper '%s' with primary join '%s' - foreign key columns are present in both the parent and the child's mapped tables.  Specify 'foreignkey' argument." %(self.key, str(self.parent), str(self.primaryjoin)))
+            elif onetomany:
+                return sync.ONETOMANY
+            elif manytoone:
+                return sync.MANYTOONE
             
     def _find_dependent(self):
         """searches through the primary join condition to determine which side
@@ -298,7 +303,7 @@ class PropertyLoader(StrategizedProperty):
         visitor = mapperutil.BinaryVisitor(foo)
         self.primaryjoin.accept_visitor(visitor)
         if len(foreignkeys) == 0:
-            raise exceptions.ArgumentError("On relation '%s', can't figure out which side is the foreign key for join condition '%s'.  Specify the 'foreignkey' argument to the relation." % (self.key, str(self.primaryjoin)))
+            raise exceptions.ArgumentError("Cant determine relation direction for '%s' on mapper '%s' with primary join '%s' - no foreign key relationship is expressed within the join condition.  Specify 'foreignkey' argument." %(self.key, str(self.parent), str(self.primaryjoin)))
         self.foreignkey = foreignkeys
         
     def get_join(self):

diff --git a/test/orm/relationships.py b/test/orm/relationships.py
index e0bb5fecd829f8f240c677384bc4d83bff295713..ef8ee9e02b5cc83110767775b74f18ae9cc9a226 100644 (file)
--- a/test/orm/relationships.py
+++ b/test/orm/relationships.py
@@ -351,6 +351,74 @@ class RelationTest3(testbase.PersistTest):
         s.delete(j)
         s.flush()
         
+class CyclicalRelationInheritTest(testbase.PersistTest):
+    """testing a mapper with an inheriting mapper, where the base mapper also has a relation to the inheriting mapper,
+    as well as using unexpected columns to create the join conditions which do not indicate the usual "cyclical" relationship
+    this represents.  test that proper error messages are raised guiding the user to a reasonably working setup, and that 
+    the ultimate setup works."""
+    def setUpAll(self):
+        global people, managers, metadata
+        metadata = BoundMetaData(testbase.db)
+
+        people = Table('people', metadata, 
+           Column('person_id', Integer, Sequence('person_id_seq', optional=True), primary_key=True),
+           Column('manager_id', Integer, ForeignKey('managers.person_id', use_alter=True, name="mpid_fq")),
+           Column('name', String(50)),
+           Column('type', String(30)))
+
+        managers = Table('managers', metadata, 
+           Column('person_id', Integer, ForeignKey('people.person_id'), primary_key=True),
+           Column('status', String(30)),
+           Column('manager_name', String(50))
+           )
+
+        metadata.create_all()
+
+    def tearDownAll(self):
+        metadata.drop_all()
+
+    def tearDown(self):
+        clear_mappers()
+        people.update().execute(manager_id=None)
+        for t in metadata.table_iterator(reverse=True):
+            t.delete().execute()
+
+    def testbasic(self):
+        class Person(object):
+            pass
+        class Manager(Person):
+            pass
+
+        mapper(Person, people, properties={
+            'manager':relation(Manager, primaryjoin=people.c.manager_id==managers.c.person_id, uselist=False)
+        })
+        mapper(Manager, managers, inherits=Person, inherit_condition=people.c.person_id==managers.c.person_id)
+        try:
+            compile_mappers()
+        except exceptions.ArgumentError, ar:
+            assert str(ar) == "Cant determine relation direction for 'manager' on mapper 'Mapper|Person|people' with primary join 'people.manager_id = managers.person_id' - foreign key columns are present in both the parent and the child's mapped tables.  Specify 'foreignkey' argument."
+
+        clear_mappers()
+
+        mapper(Person, people, properties={
+            'manager':relation(Manager, primaryjoin=people.c.manager_id==managers.c.person_id, foreignkey=people.c.manager_id, uselist=False, post_update=True)
+        })
+        mapper(Manager, managers, inherits=Person, inherit_condition=people.c.person_id==managers.c.person_id)
+
+        session = create_session()
+        p = Person()
+        p.name = 'some person'
+        m = Manager()
+        m.name = 'some manager'
+        p.manager = m
+        session.save(p)
+        session.flush()
+        session.clear()
+
+        p = session.query(Person).get(p.person_id)
+        m = session.query(Manager).get(m.person_id)
+        print p, m, p.manager
+        assert p.manager is m