almost through all the fine tuning

diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 76e219efee17078e0cb967d84fe512f420bb397a..adf3b0cde9c2a75c3fe87a62eed189cc09a30ba6 100644 (file)
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -58,7 +58,7 @@ class JoinCondition(object):
                     self_referential=False,
                     prop=None,
                     support_sync=True,
-                    can_be_synced_fn=lambda c: True
+                    can_be_synced_fn=lambda *c: True
                     ):
         self.parent_selectable = parent_selectable
         self.parent_local_selectable = parent_local_selectable
@@ -531,7 +531,8 @@ class JoinCondition(object):
                     % (self.prop, ))
 
     def _check_foreign_cols(self, join_condition, primary):
-        """Check the foreign key columns collected and emit error messages."""
+        """Check the foreign key columns collected and emit error 
+        messages."""
 
         can_sync = False
 
@@ -554,17 +555,19 @@ class JoinCondition(object):
         # (not just ==), perhaps they need to turn on "viewonly=True".
         if self.support_sync and has_foreign and not can_sync:
             err = "Could not locate any simple equality expressions "\
-                    "involving foreign key columns for %s join condition "\
+                    "involving locally mapped foreign key columns for "\
+                    "%s join condition "\
                     "'%s' on relationship %s." % (
                         primary and 'primary' or 'secondary', 
                         join_condition, 
                         self.prop
                     )
-            err += "  Ensure that referencing columns are associated with a "\
-                    "ForeignKey or ForeignKeyConstraint, or are annotated "\
-                    "in the join condition with the foreign() annotation. "\
-                    "To allow comparison operators other than '==', "\
-                    "the relationship can be marked as viewonly=True."
+            err += \
+                "  Ensure that referencing columns are associated "\
+                "with a ForeignKey or ForeignKeyConstraint, or are "\
+                "annotated in the join condition with the foreign() "\
+                "annotation. To allow comparison operators other than "\
+                "'==', the relationship can be marked as viewonly=True."
 
             raise sa_exc.ArgumentError(err)
         else:
@@ -574,9 +577,11 @@ class JoinCondition(object):
                         join_condition, 
                         self.prop
                     )
-            err += "  Ensure that referencing columns are associated with a "\
-                    "ForeignKey or ForeignKeyConstraint, or are annotated "\
-                    "in the join condition with the foreign() annotation."
+            err += \
+                '  Ensure that referencing columns are associated '\
+                'with a ForeignKey or ForeignKeyConstraint, or are '\
+                'annotated in the join condition with the foreign() '\
+                'annotation.'
             raise sa_exc.ArgumentError(err)
 
     def _determine_direction(self):
@@ -617,11 +622,15 @@ class JoinCondition(object):
                     self.direction = MANYTOONE
                 else:
                     raise sa_exc.ArgumentError(
-                            "Can't determine relationship"
-                            " direction for relationship '%s' - foreign "
-                            "key columns are present in both the parent "
-                            "and the child's mapped tables.  Specify "
-                            "'foreign_keys' argument." % self.prop)
+                        "Can't determine relationship"
+                        " direction for relationship '%s' - foreign "
+                        "key columns within the join condition are present "
+                        "in both the parent and the child's mapped tables.  "
+                        "Ensure that only those columns referring "
+                        "to a parent column are marked as foreign, "
+                        "either via the foreign() annotation or "
+                        "via the foreign_keys argument."
+                         % self.prop)
             elif onetomany_fk:
                 self.direction = ONETOMANY
             elif manytoone_fk:
@@ -647,7 +656,8 @@ class JoinCondition(object):
                     "remote" not in right._annotations and \
                     self.can_be_synced_fn(right):
                     lrp.add((right, left))
-                if binary.operator is operators.eq:
+                if binary.operator is operators.eq and \
+                    self.can_be_synced_fn(left, right):
                     if "foreign" in right._annotations:
                         collection.append((left, right))
                     elif "foreign" in left._annotations:

diff --git a/test/orm/test_rel_fn.py b/test/orm/test_rel_fn.py
index 9fe6ce170630a4c6e36753545a8df14ecd0cc416..e35dd925a610b239e26e4d5a3c198d761dc250cd 100644 (file)
--- a/test/orm/test_rel_fn.py
+++ b/test/orm/test_rel_fn.py
@@ -86,6 +86,40 @@ class _JoinFixtures(object):
             Column('base_id', Integer, ForeignKey('base.id'))
         )
 
+        cls.three_tab_a = Table('three_tab_a', m, 
+            Column('id', Integer, primary_key=True),
+        )
+        cls.three_tab_b = Table('three_tab_b', m, 
+            Column('id', Integer, primary_key=True),
+            Column('aid', Integer, ForeignKey('three_tab_a.id'))
+        )
+        cls.three_tab_c = Table('three_tab_c', m, 
+            Column('id', Integer, primary_key=True),
+            Column('aid', Integer, ForeignKey('three_tab_a.id')),
+            Column('bid', Integer, ForeignKey('three_tab_b.id'))
+        )
+
+    def _join_fixture_overlapping_three_tables(self, **kw):
+        def _can_sync(*cols):
+            for c in cols:
+                if self.three_tab_c.c.contains_column(c):
+                    return False
+            else:
+                return True
+        return relationships.JoinCondition(
+            self.three_tab_a, 
+            self.three_tab_b,
+            self.three_tab_a, 
+            self.three_tab_b,
+            support_sync=False,
+            can_be_synced_fn=_can_sync,
+            primaryjoin=and_(
+                self.three_tab_a.c.id==self.three_tab_b.c.aid,
+                self.three_tab_c.c.bid==self.three_tab_b.c.id,
+                self.three_tab_c.c.aid==self.three_tab_a.c.id
+            )
+        )
+
     def _join_fixture_m2m(self, **kw):
         return relationships.JoinCondition(
                     self.m2mleft, 
@@ -338,7 +372,7 @@ class _JoinFixtures(object):
         assert_raises_message(
             sa.exc.ArgumentError, 
             "Could not locate any simple equality expressions "
-            "involving foreign key columns for %s join "
+            "involving locally mapped foreign key columns for %s join "
             "condition '%s' on relationship %s.  "
             "Ensure that referencing columns are associated with a "
             "ForeignKey or ForeignKeyConstraint, or are annotated in "
@@ -626,6 +660,16 @@ class ColumnCollectionsTest(_JoinFixtures, fixtures.TestBase, AssertsCompiledSQL
             set([self.selfref.c.id])
         )
 
+    def test_determine_local_remote_cols_three_tab_viewonly(self):
+        joincond = self._join_fixture_overlapping_three_tables()
+        eq_(
+            joincond.local_remote_pairs,
+            [(self.three_tab_a.c.id, self.three_tab_b.c.aid)]
+        )
+        eq_(
+            joincond.remote_columns,
+            set([self.three_tab_b.c.id, self.three_tab_b.c.aid])
+        )
 
 class DirectionTest(_JoinFixtures, fixtures.TestBase, AssertsCompiledSQL):
     def test_determine_direction_compound_2(self):

diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py
index 859aa5b4e02ab846f3bb40b9683dd07c82379237..5c273a6431566823d9be15261efc3fe2305b5f95 100644 (file)
--- a/test/orm/test_relationships.py
+++ b/test/orm/test_relationships.py
@@ -15,6 +15,103 @@ from test.lib import fixtures
 from test.orm import _fixtures
 from sqlalchemy import exc
 
+class _RelationshipErrors(object):
+    def _assert_raises_no_relevant_fks(self, fn, expr, relname, 
+        primary, *arg, **kw):
+        assert_raises_message(
+            sa.exc.ArgumentError, 
+            "Could not locate any relevant foreign key columns "
+            "for %s join condition '%s' on relationship %s.  "
+            "Ensure that referencing columns are associated with "
+            "a ForeignKey or ForeignKeyConstraint, or are annotated "
+            r"in the join condition with the foreign\(\) annotation."
+            % (
+                primary, expr, relname
+            ),
+            fn, *arg, **kw
+        )
+
+    def _assert_raises_no_equality(self, fn, expr, relname, 
+        primary, *arg, **kw):
+        assert_raises_message(
+            sa.exc.ArgumentError, 
+            "Could not locate any simple equality expressions "
+            "involving locally mapped foreign key columns for %s join "
+            "condition '%s' on relationship %s.  "
+            "Ensure that referencing columns are associated with a "
+            "ForeignKey or ForeignKeyConstraint, or are annotated in "
+            r"the join condition with the foreign\(\) annotation. "
+            "To allow comparison operators other than '==', "
+            "the relationship can be marked as viewonly=True." % (
+                primary, expr, relname
+            ),
+            fn, *arg, **kw
+        )
+
+    def _assert_raises_ambig_join(self, fn, relname, secondary_arg,
+        *arg, **kw):
+        if secondary_arg is not None:
+            assert_raises_message(
+                exc.ArgumentError,
+                "Could not determine join condition between "
+                "parent/child tables on relationship %s - "
+                "there are multiple foreign key paths linking the "
+                "tables via secondary table '%s'.  "
+                "Specify the 'foreign_keys' argument, providing a list "
+                "of those columns which should be counted as "
+                "containing a foreign key reference from the "
+                "secondary table to each of the parent and child tables."
+                % (relname, secondary_arg),
+                fn, *arg, **kw)
+        else:
+            assert_raises_message(
+                exc.ArgumentError,
+                "Could not determine join condition between "
+                "parent/child tables on relationship %s - "
+                "there are no foreign keys linking these tables.  " 
+                % (relname,),
+                fn, *arg, **kw)
+
+    def _assert_raises_no_join(self, fn, relname, secondary_arg,
+        *arg, **kw):
+        if secondary_arg is not None:
+            assert_raises_message(
+                exc.NoForeignKeysError,
+                "Could not determine join condition between "
+                "parent/child tables on relationship %s - "
+                "there are no foreign keys linking these tables "
+                "via secondary table '%s'.  "
+                "Ensure that referencing columns are associated with a ForeignKey "
+                "or ForeignKeyConstraint, or specify 'primaryjoin' and "
+                "'secondaryjoin' expressions"
+                % (relname, secondary_arg),
+                fn, *arg, **kw)
+        else:
+            assert_raises_message(
+                exc.NoForeignKeysError,
+                "Could not determine join condition between "
+                "parent/child tables on relationship %s - "
+                "there are no foreign keys linking these tables. "
+                "Ensure that referencing columns are associated with a ForeignKey "
+                "or ForeignKeyConstraint, or specify a 'primaryjoin' "
+                "expression."
+                % (relname,),
+                fn, *arg, **kw)
+
+    def _assert_raises_ambiguous_direction(self, fn, relname, *arg, **kw):
+        assert_raises_message(
+            sa.exc.ArgumentError,
+            "Can't determine relationship"
+            " direction for relationship '%s' - foreign "
+            "key columns within the join condition are present "
+            "in both the parent and the child's mapped tables.  "
+            "Ensure that only those columns referring to a parent column "
+            r"are marked as foreign, either via the foreign\(\) annotation or "
+            "via the foreign_keys argument."
+            % relname,
+            fn, *arg, **kw
+        )
+
 class DependencyTwoParentTest(fixtures.MappedTest):
     """Test flush() when a mapper is dependent on multiple relationships"""
 
@@ -2043,7 +2140,7 @@ class InvalidRemoteSideTest(fixtures.MappedTest):
             "mean to set remote_side on the many-to-one side ?", 
             configure_mappers)
 
-class AmbiguousFKResolutionTest(fixtures.MappedTest):
+class AmbiguousFKResolutionTest(_RelationshipErrors, fixtures.MappedTest):
     @classmethod
     def define_tables(cls, metadata):
         Table("a", metadata,
@@ -2146,14 +2243,9 @@ class AmbiguousFKResolutionTest(fixtures.MappedTest):
             'bs':relationship(B, secondary=a_to_b)
         })
         mapper(B, b)
-        assert_raises_message(
-            sa.exc.NoForeignKeysError,
-            "Could not determine join condition between "
-            "parent/child tables on relationship A.bs - "
-            "there are no foreign keys linking these "
-            "tables via secondary table 'atob'.  "
-            "Specify 'primaryjoin' and 'secondaryjoin' expressions.",
-            sa.orm.configure_mappers
+        self._assert_raises_no_join(
+            sa.orm.configure_mappers,
+            "A.bs", a_to_b,
         )
 
     def test_ambiguous_fks_m2m(self):
@@ -2186,7 +2278,8 @@ class AmbiguousFKResolutionTest(fixtures.MappedTest):
         mapper(B, b)
         sa.orm.configure_mappers()
 
-class InvalidRelationshipEscalationTest(fixtures.MappedTest):
+
+class InvalidRelationshipEscalationTest(_RelationshipErrors, fixtures.MappedTest):
 
     @classmethod
     def define_tables(cls, metadata):
@@ -2211,87 +2304,6 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
         class Bar(cls.Basic):
             pass
 
-    def _assert_raises_no_relevant_fks(self, fn, expr, relname, 
-        primary, *arg, **kw):
-        assert_raises_message(
-            sa.exc.ArgumentError, 
-            "Could not locate any relevant foreign key columns "
-            "for %s join condition '%s' on relationship %s.  "
-            "Ensure that referencing columns are associated with "
-            "a ForeignKey or ForeignKeyConstraint, or are annotated "
-            r"in the join condition with the foreign\(\) annotation."
-            % (
-                primary, expr, relname
-            ),
-            fn, *arg, **kw
-        )
-
-    def _assert_raises_no_equality(self, fn, expr, relname, 
-        primary, *arg, **kw):
-        assert_raises_message(
-            sa.exc.ArgumentError, 
-            "Could not locate any simple equality expressions "
-            "involving foreign key columns for %s join "
-            "condition '%s' on relationship %s.  "
-            "Ensure that referencing columns are associated with a "
-            "ForeignKey or ForeignKeyConstraint, or are annotated in "
-            r"the join condition with the foreign\(\) annotation. "
-            "To allow comparison operators other than '==', "
-            "the relationship can be marked as viewonly=True." % (
-                primary, expr, relname
-            ),
-            fn, *arg, **kw
-        )
-
-    def _assert_raises_ambig_join(self, fn, relname, secondary_arg,
-        *arg, **kw):
-        if secondary_arg is not None:
-            assert_raises_message(
-                exc.ArgumentError,
-                "Could not determine join condition between "
-                "parent/child tables on relationship %s - "
-                "there are multiple foreign key paths linking the "
-                "tables via secondary table '%s'.  "
-                "Specify the 'foreign_keys' argument, providing a list "
-                "of those columns which should be counted as "
-                "containing a foreign key reference from the "
-                "secondary table to each of the parent and child tables."
-                % (relname, secondary_arg),
-                fn, *arg, **kw)
-        else:
-            assert_raises_message(
-                exc.ArgumentError,
-                "Could not determine join condition between "
-                "parent/child tables on relationship %s - "
-                "there are no foreign keys linking these tables.  " 
-                % (relname,),
-                fn, *arg, **kw)
-
-    def _assert_raises_no_join(self, fn, relname, secondary_arg,
-        *arg, **kw):
-        if secondary_arg is not None:
-            assert_raises_message(
-                exc.NoForeignKeysError,
-                "Could not determine join condition between "
-                "parent/child tables on relationship %s - "
-                "there are no foreign keys linking these tables "
-                "via secondary table '%s'.  "
-                "Ensure that referencing columns are associated with a ForeignKey "
-                "or ForeignKeyConstraint, or specify 'primaryjoin' and "
-                "'secondaryjoin' expressions"
-                % (relname, secondary_arg),
-                fn, *arg, **kw)
-        else:
-            assert_raises_message(
-                exc.NoForeignKeysError,
-                "Could not determine join condition between "
-                "parent/child tables on relationship %s - "
-                "there are no foreign keys linking these tables. "
-                "Ensure that referencing columns are associated with a ForeignKey "
-                "or ForeignKeyConstraint, or specify a 'primaryjoin' "
-                "expression."
-                % (relname,),
-                fn, *arg, **kw)
 
     def test_no_join(self):
         bars, Foo, Bar, foos = (self.tables.bars,
@@ -2333,11 +2345,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             primaryjoin=foos.c.id>bars.c.fid)})
         mapper(Bar, bars)
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not determine relationship direction "
-            "for primaryjoin condition",
-            configure_mappers)
+        self._assert_raises_no_relevant_fks(
+            configure_mappers,
+            "foos.id > bars.fid", "Foo.bars", "primary"
+        )
 
     def test_no_equated_fks(self):
         bars, Foo, Bar, foos = (self.tables.bars,
@@ -2350,15 +2361,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             primaryjoin=foos.c.id>bars.c.fid,
                             foreign_keys=bars.c.fid)})
         mapper(Bar, bars)
-
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not locate any foreign-key-equated, "
-            "locally mapped column pairs for primaryjoin "
-            "condition 'foos.id > bars.fid' on relationship "
-            "Foo.bars.  For more relaxed rules on join "
-            "conditions, the relationship may be marked as viewonly=True.",
-            sa.orm.configure_mappers)
+        self._assert_raises_no_equality(
+            sa.orm.configure_mappers,
+            "foos.id > bars.fid", "Foo.bars", "primary"
+        )
 
     def test_no_equated_wo_fks_works_on_relaxed(self):
         foos_with_fks, Foo, Bar, bars_with_fks, foos = (self.tables.foos_with_fks,
@@ -2382,19 +2388,12 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             )})
         mapper(Bar, bars_with_fks)
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not locate any foreign-key-equated, locally mapped "
-             "column pairs for primaryjoin condition "
-             "'bars_with_fks.fid = foos_with_fks.id AND "
-             "foos_with_fks.id = foos.id' on relationship Foo.bars.  "
-             "Ensure that the referencing Column objects have a "
-             "ForeignKey present, or are otherwise part of a "
-             "ForeignKeyConstraint on their parent Table, or specify "
-             "the foreign_keys parameter to this relationship.  For "
-             "more relaxed rules on join conditions, the relationship "
-             "may be marked as viewonly=True.", 
-            sa.orm.configure_mappers)
+        self._assert_raises_no_equality(
+            sa.orm.configure_mappers,
+            "bars_with_fks.fid = foos_with_fks.id "
+            "AND foos_with_fks.id = foos.id", 
+            "Foo.bars", "primary"
+        )
 
     def test_ambiguous_fks(self):
         bars, Foo, Bar, foos = (self.tables.bars,
@@ -2408,20 +2407,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             foreign_keys=[foos.c.id, bars.c.fid])})
         mapper(Bar, bars)
 
-        assert_raises_message(sa.exc.ArgumentError,
-                              "Could not determine relationship "
-                              "direction for primaryjoin condition "
-                              "'foos.id = bars.fid', on relationship "
-                              "Foo.bars, using manual 'foreign_keys' "
-                              "setting.  Do the columns in "
-                              "'foreign_keys' represent all, and only, "
-                              "the 'foreign' columns in this join "
-                              r"condition\?  Does the mapped Table "
-                              "already have adequate ForeignKey and/or "
-                              "ForeignKeyConstraint objects "
-                              r"established \(in which case "
-                              r"'foreign_keys' is usually unnecessary\)\?"
-                              , sa.orm.configure_mappers)
+        self._assert_raises_ambiguous_direction(
+            sa.orm.configure_mappers,
+            "Foo.bars"
+        )
 
     def test_ambiguous_remoteside_o2m(self):
         bars, Foo, Bar, foos = (self.tables.bars,
@@ -2510,10 +2499,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             viewonly=True)})
         mapper(Bar, bars)
 
-        assert_raises_message(sa.exc.ArgumentError,
-                              'Could not determine relationship '
-                              'direction for primaryjoin condition',
-                              sa.orm.configure_mappers)
+        self._assert_raises_no_relevant_fks(
+            sa.orm.configure_mappers,
+            "foos.id > bars.fid", "Foo.bars", "primary"
+        )
 
         sa.orm.clear_mappers()
         mapper(Foo, foos_with_fks, properties={
@@ -2537,15 +2526,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             viewonly=True)})
         mapper(Bar, bars)
 
-        assert_raises_message(sa.exc.ArgumentError,
-                              "Could not determine relationship "
-                              "direction for primaryjoin condition "
-                              "'foos.id > foos.fid', on relationship "
-                              "Foo.foos. Ensure that the referencing "
-                              "Column objects have a ForeignKey "
-                              "present, or are otherwise part of a "
-                              "ForeignKeyConstraint on their parent "
-                              "Table.", sa.orm.configure_mappers)
+        self._assert_raises_no_relevant_fks(
+            sa.orm.configure_mappers,
+            "foos.id > foos.fid", "Foo.foos", "primary"
+        )
 
         sa.orm.clear_mappers()
         mapper(Foo, foos_with_fks, properties={
@@ -2580,11 +2564,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             primaryjoin=foos.c.id==bars.c.fid)})
         mapper(Bar, bars)
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not determine relationship direction for primaryjoin "
-            "condition",
-            configure_mappers)
+        self._assert_raises_no_relevant_fks(
+            configure_mappers,
+            "foos.id = bars.fid", "Foo.bars", "primary"
+        )
 
         sa.orm.clear_mappers()
         mapper(Foo, foos_with_fks, properties={
@@ -2600,11 +2583,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
             'foos':relationship(Foo,
                             primaryjoin=foos.c.id==foos.c.fid)})
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not determine relationship direction for primaryjoin "
-            "condition",
-            configure_mappers)
+        self._assert_raises_no_relevant_fks(
+            configure_mappers,
+            "foos.id = foos.fid", "Foo.foos", "primary"
+        )
 
 
     def test_equated_self_ref_wrong_fks(self):
@@ -2617,11 +2599,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest):
                             primaryjoin=foos.c.id==foos.c.fid,
                             foreign_keys=[bars.c.id])})
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not determine relationship direction for primaryjoin "
-            "condition",
-            configure_mappers)
+        self._assert_raises_no_relevant_fks(
+            configure_mappers,
+            "foos.id = foos.fid", "Foo.foos", "primary"
+        )
 
 
 class InvalidRelationshipEscalationTestM2M(fixtures.MappedTest):