]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
- Another pass through the series of error messages
authorMike Bayer <mike_mp@zzzcomputing.com>
Sat, 14 Aug 2010 18:52:18 +0000 (14:52 -0400)
committerMike Bayer <mike_mp@zzzcomputing.com>
Sat, 14 Aug 2010 18:52:18 +0000 (14:52 -0400)
emitted when relationship() is configured with
ambiguous arguments.   The "foreign_keys"
setting is no longer mentioned, as it is almost
never needed and it is preferable users set up
correct ForeignKey metadata, which is now the
recommendation.  If 'foreign_keys'
is used and is incorrect, the message suggests
the attribute is probably unnecessary.  Docs
for the attribute are beefed up.  This
because all confused relationship() users on the
ML appear to be attempting to use foreign_keys
due to the message, which only confuses them
further since Table metadata is much clearer.

- If the "secondary" table has no ForeignKey metadata
and no foreign_keys is set, even though the
user is passing screwed up information, it is assumed
that primary/secondaryjoin expressions should
consider only and all cols in "secondary" to be
foreign.  It's not possible with "secondary" for
the foreign keys to be elsewhere in any case.
A warning is now emitted instead of an error,
and the mapping succeeds. [ticket:1877]

- fixed incorrect "Alternate Collection Mappings" reference
in the docs, not sure if someone wants to reference
"Rows that Point to Themselves" function
- "Collection Mapping" is "Advanced Collection Mapping", this
section is troublesome since nobody really needs it but it
is public API

CHANGES
doc/build/mappers.rst
doc/build/ormtutorial.rst
doc/build/reference/orm/collections.rst
lib/sqlalchemy/ext/declarative.py
lib/sqlalchemy/orm/__init__.py
lib/sqlalchemy/orm/properties.py
lib/sqlalchemy/test/testing.py
test/orm/test_relationships.py

diff --git a/CHANGES b/CHANGES
index da9f3dca0ecb2a0581b1abd3d81e730726ff7d23..4f60857ef8c03e920561dda13ed04f4ee6f07849 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -21,6 +21,31 @@ CHANGES
     with_polymorphic selectable, instead of silently
     ignoring it.  Look for this to become an
     exception in 0.7.
+
+  - Another pass through the series of error messages
+    emitted when relationship() is configured with
+    ambiguous arguments.   The "foreign_keys" 
+    setting is no longer mentioned, as it is almost
+    never needed and it is preferable users set up
+    correct ForeignKey metadata, which is now the
+    recommendation.  If 'foreign_keys'
+    is used and is incorrect, the message suggests
+    the attribute is probably unnecessary.  Docs
+    for the attribute are beefed up.  This 
+    because all confused relationship() users on the 
+    ML appear to be attempting to use foreign_keys
+    due to the message, which only confuses them
+    further since Table metadata is much clearer.
+
+  - If the "secondary" table has no ForeignKey metadata
+    and no foreign_keys is set, even though the
+    user is passing screwed up information, it is assumed
+    that primary/secondaryjoin expressions should
+    consider only and all cols in "secondary" to be 
+    foreign.  It's not possible with "secondary" for 
+    the foreign keys to be elsewhere in any case.
+    A warning is now emitted instead of an error, 
+    and the mapping succeeds. [ticket:1877]
     
   - Moving an o2m object from one collection to
     another, or vice versa changing the referenced
index 97677aa080ebd5b7d593627f1f2d06169b6e8aa9..71f71a8cfa8668e651d9f27caf530eb33e7e2c96 100644 (file)
@@ -1490,7 +1490,6 @@ Theres no restriction on how many times you can relate from parent to child.  SQ
                     addresses_table.c.city=='New York')),
     })
 
-.. _alternate_collection_implementations:
 
 Rows that point to themselves / Mutually Dependent Rows
 -------------------------------------------------------
@@ -1528,7 +1527,7 @@ To enable the UPDATE after INSERT / UPDATE before DELETE behavior on :func:`~sql
 When a structure using the above mapping is flushed, the "widget" row will be INSERTed minus the "favorite_entry_id" value, then all the "entry" rows will be INSERTed referencing the parent "widget" row, and then an UPDATE statement will populate the "favorite_entry_id" column of the "widget" table (it's one row at a time for the time being).
 
 
-.. _advdatamapping_entitycollections:
+.. _alternate_collection_implementations:
 
 Alternate Collection Implementations
 -------------------------------------
index 2d6a3325eacefaec321b6dd0c84521c891ed44db..6f38a35c90b57268c4b706db0a230fb2e2d8b9d8 100644 (file)
@@ -781,7 +781,7 @@ We'll need to create the ``addresses`` table in the database, so we will issue a
 Working with Related Objects
 =============================
 
-Now when we create a ``User``, a blank ``addresses`` collection will be present.  Various collection types, such as sets and dictionaries, are possible here (see :ref:`advdatamapping_entitycollections` for details), but by default, the collection is a Python list.
+Now when we create a ``User``, a blank ``addresses`` collection will be present.  Various collection types, such as sets and dictionaries, are possible here (see :ref:`alternate_collection_implementations` for details), but by default, the collection is a Python list.
 
 .. sourcecode:: python+sql
 
index 350e6312512fba3de38e809f997935dbbf1823c1..818f11498986012a1709a75fe5b365ad9f9ee51e 100644 (file)
@@ -1,5 +1,5 @@
-Collection Mapping
-==================
+Advanced Collection Mapping
+===========================
 
 This is an in-depth discussion of collection mechanics.  For simple examples, see :ref:`alternate_collection_implementations`.
 
index 2310f01ce3a4f12f4feaa55742f0e6b082b38771..e40ba3ec4beea27ed066f886fb50798d74147458 100755 (executable)
@@ -1197,7 +1197,7 @@ def _deferred_relationship(cls, prop):
 
     if isinstance(prop, RelationshipProperty):
         for attr in ('argument', 'order_by', 'primaryjoin', 'secondaryjoin',
-                     'secondary', '_foreign_keys', 'remote_side'):
+                     'secondary', '_user_defined_foreign_keys', 'remote_side'):
             v = getattr(prop, attr)
             if isinstance(v, basestring):
                 setattr(prop, attr, resolve_arg(v))
index 2004dccd1eadb7c82f4db2da15cac4295eda29e6..c74fabacd7de74b6f667e8f30aa966316eb20f8d 100644 (file)
@@ -274,15 +274,24 @@ def relationship(argument, secondary=None, **kwargs):
 
     :param foreign_keys:
       a list of columns which are to be used as "foreign key" columns.
-      this parameter should be used in conjunction with explicit
-      ``primaryjoin`` and ``secondaryjoin`` (if needed) arguments, and
-      the columns within the ``foreign_keys`` list should be present
-      within those join conditions. Normally, ``relationship()`` will
-      inspect the columns within the join conditions to determine
-      which columns are the "foreign key" columns, based on
-      information in the ``Table`` metadata. Use this argument when no
-      ForeignKey's are present in the join condition, or to override
-      the table-defined foreign keys.
+      Normally, :func:`relationship` uses the :class:`.ForeignKey`
+      and :class:`.ForeignKeyConstraint` objects present within the
+      mapped or secondary :class:`.Table` to determine the "foreign" side of 
+      the join condition.  This is used to construct SQL clauses in order
+      to load objects, as well as to "synchronize" values from 
+      primary key columns to referencing foreign key columns.
+      The ``foreign_keys`` parameter overrides the notion of what's
+      "foreign" in the table metadata, allowing the specification
+      of a list of :class:`.Column` objects that should be considered
+      part of the foreign key.
+      
+      There are only two use cases for ``foreign_keys`` - one, when it is not
+      convenient for :class:`.Table` metadata to contain its own foreign key
+      metadata (which should be almost never, unless reflecting a large amount of
+      tables from a MySQL MyISAM schema, or a schema that doesn't actually
+      have foreign keys on it). The other is for extremely
+      rare and exotic composite foreign key setups where some columns
+      should artificially not be considered as foreign.
 
     :param innerjoin=False:
       when ``True``, joined eager loads will use an inner join to join
index cbfba91f32991006abe24eddcfe84072014ab33d..5788c30f948a0b142154205d5276c4ada84f4f23 100644 (file)
@@ -446,7 +446,7 @@ class RelationshipProperty(StrategizedProperty):
         self.viewonly = viewonly
         self.lazy = lazy
         self.single_parent = single_parent
-        self._foreign_keys = foreign_keys
+        self._user_defined_foreign_keys = foreign_keys
         self.collection_class = collection_class
         self.passive_deletes = passive_deletes
         self.passive_updates = passive_updates
@@ -695,7 +695,7 @@ class RelationshipProperty(StrategizedProperty):
             if isinstance(other, (NoneType, expression._Null)):
                 if self.property.direction == MANYTOONE:
                     return sql.or_(*[x != None for x in
-                                   self.property._foreign_keys])
+                                   self.property._calculated_foreign_keys])
                 else:
                     return self._criterion_exists()
             elif self.property.uselist:
@@ -912,7 +912,7 @@ class RelationshipProperty(StrategizedProperty):
             'primaryjoin',
             'secondaryjoin',
             'secondary',
-            '_foreign_keys',
+            '_user_defined_foreign_keys',
             'remote_side',
             ):
             if util.callable(getattr(self, attr)):
@@ -931,9 +931,9 @@ class RelationshipProperty(StrategizedProperty):
         if self.order_by is not False and self.order_by is not None:
             self.order_by = [expression._literal_as_column(x) for x in
                              util.to_list(self.order_by)]
-        self._foreign_keys = \
+        self._user_defined_foreign_keys = \
             util.column_set(expression._literal_as_column(x) for x in
-                            util.to_column_set(self._foreign_keys))
+                            util.to_column_set(self._user_defined_foreign_keys))
         self.remote_side = \
             util.column_set(expression._literal_as_column(x) for x in
                             util.to_column_set(self.remote_side))
@@ -999,8 +999,8 @@ class RelationshipProperty(StrategizedProperty):
             raise sa_exc.ArgumentError("Could not determine join "
                     "condition between parent/child tables on "
                     "relationship %s.  Specify a 'primaryjoin' "
-                    "expression.  If this is a many-to-many "
-                    "relationship, 'secondaryjoin' is needed as well."
+                    "expression.  If 'secondary' is present, "
+                    "'secondaryjoin' is needed as well."
                     % self)
 
     def _col_is_part_of_mappings(self, column):
@@ -1012,91 +1012,121 @@ class RelationshipProperty(StrategizedProperty):
                 self.target.c.contains_column(column) or \
                 self.secondary.c.contains_column(column) is not None
 
+    def _sync_pairs_from_join(self, join_condition, primary):
+        """Given a join condition, figure out what columns are foreign
+        and are part of a binary "equated" condition to their referecned
+        columns, and convert into a list of tuples of (primary col->foreign col).
+        
+        Make several attempts to determine if cols are compared using 
+        "=" or other comparators (in which case suggest viewonly), 
+        columns are present but not part of the expected mappings, columns
+        don't have any :class:`ForeignKey` information on them, or 
+        the ``foreign_keys`` attribute is being used incorrectly.
+        
+        """
+        eq_pairs = criterion_as_pairs(join_condition,
+                consider_as_foreign_keys=self._user_defined_foreign_keys,
+                any_operator=self.viewonly)
+                
+        eq_pairs = [(l, r) for (l, r) in eq_pairs
+                    if self._col_is_part_of_mappings(l)
+                    and self._col_is_part_of_mappings(r)
+                    or self.viewonly and r in self._user_defined_foreign_keys]
+        
+        if not eq_pairs and \
+                self.secondary is not None and \
+                not self._user_defined_foreign_keys:
+            fks = set(self.secondary.c)
+            eq_pairs = criterion_as_pairs(join_condition,
+                    consider_as_foreign_keys=fks,
+                    any_operator=self.viewonly)
+
+            eq_pairs = [(l, r) for (l, r) in eq_pairs
+                        if self._col_is_part_of_mappings(l)
+                        and self._col_is_part_of_mappings(r)
+                        or self.viewonly and r in fks]
+            if eq_pairs:
+                util.warn("No ForeignKey objects were present "
+                            "in secondary table '%s'.  Assumed referenced "
+                            "foreign key columns %s for join condition '%s' "
+                            "on relationship %s" % (
+                                self.secondary.description,
+                                ", ".join(sorted(["'%s'" % col for col in fks])),
+                                join_condition,
+                                self
+                            ))
+                    
+        if not eq_pairs:
+            if not self.viewonly and criterion_as_pairs(join_condition,
+                    consider_as_foreign_keys=self._user_defined_foreign_keys,
+                    any_operator=True):
+                raise sa_exc.ArgumentError("Could not locate any "
+                        "equated, locally mapped column pairs for %s "
+                        "condition '%s' on relationship %s. For more "
+                        "relaxed rules on join conditions, the "
+                        "relationship may be marked as viewonly=True."
+                        % (
+                            primary and 'primaryjoin' or 'secondaryjoin', 
+                            join_condition, 
+                            self
+                        ))
+            else:
+                if self._user_defined_foreign_keys:
+                    raise sa_exc.ArgumentError("Could not determine "
+                            "relationship direction for %s condition "
+                            "'%s', on relationship %s, using manual "
+                            "'foreign_keys' setting.  Do the columns "
+                            "in 'foreign_keys' represent all, and "
+                            "only, the 'foreign' columns in this join "
+                            "condition?  Does the %s Table already "
+                            "have adequate ForeignKey and/or "
+                            "ForeignKeyConstraint objects established "
+                            "(in which case 'foreign_keys' is usually "
+                            "unnecessary)?" 
+                            % (
+                                primary and 'primaryjoin' or 'secondaryjoin',
+                                join_condition, 
+                                self,
+                                primary and 'mapped' or 'secondary'
+                            ))
+                else:
+                    raise sa_exc.ArgumentError("Could not determine "
+                            "relationship direction for %s condition "
+                            "'%s', on relationship %s. Ensure that the "
+                            "referencing Column objects have a "
+                            "ForeignKey present, or are otherwise part "
+                            "of a ForeignKeyConstraint on their parent "
+                            "Table." 
+                            % (
+                                primary and 'primaryjoin' or 'secondaryjoin', 
+                                join_condition, 
+                                self
+                            ))
+        return eq_pairs
+
     def _determine_synchronize_pairs(self):
         if self.local_remote_pairs:
-            if not self._foreign_keys:
+            if not self._user_defined_foreign_keys:
                 raise sa_exc.ArgumentError('foreign_keys argument is '
                         'required with _local_remote_pairs argument')
             self.synchronize_pairs = []
             for l, r in self.local_remote_pairs:
-                if r in self._foreign_keys:
+                if r in self._user_defined_foreign_keys:
                     self.synchronize_pairs.append((l, r))
-                elif l in self._foreign_keys:
+                elif l in self._user_defined_foreign_keys:
                     self.synchronize_pairs.append((r, l))
         else:
-            eq_pairs = criterion_as_pairs(self.primaryjoin,
-                    consider_as_foreign_keys=self._foreign_keys,
-                    any_operator=self.viewonly)
-            eq_pairs = [(l, r) for (l, r) in eq_pairs
-                        if self._col_is_part_of_mappings(l)
-                        and self._col_is_part_of_mappings(r)
-                        or self.viewonly and r in self._foreign_keys]
-            if not eq_pairs:
-                if not self.viewonly \
-                    and criterion_as_pairs(self.primaryjoin,
-                        consider_as_foreign_keys=self._foreign_keys,
-                        any_operator=True):
-                    raise sa_exc.ArgumentError("Could not locate any "
-                            "equated, locally mapped column pairs for "
-                            "primaryjoin condition '%s' on "
-                            "relationship %s. For more relaxed rules "
-                            "on join conditions, the relationship may "
-                            "be marked as viewonly=True."
-                            % (self.primaryjoin, self))
-                else:
-                    if self._foreign_keys:
-                        raise sa_exc.ArgumentError("Could not determine"
-                                " relationship direction for "
-                                "primaryjoin condition '%s', on "
-                                "relationship %s. 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"
-                                " relationship direction for "
-                                "primaryjoin condition '%s', on "
-                                "relationship %s. Specify the "
-                                "'foreign_keys' argument to indicate "
-                                "which columns on the relationship are "
-                                "foreign." % (self.primaryjoin, self))
+            eq_pairs = self._sync_pairs_from_join(self.primaryjoin, True)
             self.synchronize_pairs = eq_pairs
         if self.secondaryjoin is not None:
-            sq_pairs = criterion_as_pairs(self.secondaryjoin,
-                    consider_as_foreign_keys=self._foreign_keys,
-                    any_operator=self.viewonly)
-            sq_pairs = [(l, r) for (l, r) in sq_pairs
-                        if self._col_is_part_of_mappings(l)
-                        and self._col_is_part_of_mappings(r) or r
-                        in self._foreign_keys]
-            if not sq_pairs:
-                if not self.viewonly \
-                    and criterion_as_pairs(self.secondaryjoin,
-                        consider_as_foreign_keys=self._foreign_keys,
-                        any_operator=True):
-                    raise sa_exc.ArgumentError("Could not locate any "
-                            "equated, locally mapped column pairs for "
-                            "secondaryjoin condition '%s' on "
-                            "relationship %s. For more relaxed rules "
-                            "on join conditions, the relationship may "
-                            "be marked as viewonly=True."
-                            % (self.secondaryjoin, self))
-                else:
-                    raise sa_exc.ArgumentError("Could not determine "
-                            "relationship direction for secondaryjoin "
-                            "condition '%s', on relationship %s. "
-                            "Specify the foreign_keys argument to "
-                            "indicate which columns on the "
-                            "relationship are foreign."
-                            % (self.secondaryjoin, self))
+            sq_pairs = self._sync_pairs_from_join(self.secondaryjoin, False)
             self.secondary_synchronize_pairs = sq_pairs
         else:
             self.secondary_synchronize_pairs = None
-        self._foreign_keys = util.column_set(r for (l, r) in
+        self._calculated_foreign_keys = util.column_set(r for (l, r) in
                 self.synchronize_pairs)
         if self.secondary_synchronize_pairs:
-            self._foreign_keys.update(r for (l, r) in
+            self._calculated_foreign_keys.update(r for (l, r) in
                     self.secondary_synchronize_pairs)
 
     def _determine_direction(self):
@@ -1114,7 +1144,7 @@ class RelationshipProperty(StrategizedProperty):
                 remote = self.remote_side
             else:
                 remote = None
-            if not remote or self._foreign_keys.difference(l for (l,
+            if not remote or self._calculated_foreign_keys.difference(l for (l,
                     r) in self.synchronize_pairs).intersection(remote):
                 self.direction = ONETOMANY
             else:
@@ -1192,12 +1222,12 @@ class RelationshipProperty(StrategizedProperty):
                         eq_pairs += self.secondary_synchronize_pairs
                 else:
                     eq_pairs = criterion_as_pairs(self.primaryjoin,
-                            consider_as_foreign_keys=self._foreign_keys,
+                            consider_as_foreign_keys=self._calculated_foreign_keys,
                             any_operator=True)
                     if self.secondaryjoin is not None:
                         eq_pairs += \
                             criterion_as_pairs(self.secondaryjoin,
-                                consider_as_foreign_keys=self._foreign_keys,
+                                consider_as_foreign_keys=self._calculated_foreign_keys,
                                 any_operator=True)
                     eq_pairs = [(l, r) for (l, r) in eq_pairs
                                 if self._col_is_part_of_mappings(l)
@@ -1266,7 +1296,7 @@ class RelationshipProperty(StrategizedProperty):
                         "a non-secondary relationship."
                             )
             foreign_keys = kwargs.pop('foreign_keys',
-                    self._foreign_keys)
+                    self._user_defined_foreign_keys)
             parent = self.parent.primary_mapper()
             kwargs.setdefault('viewonly', self.viewonly)
             kwargs.setdefault('post_update', self.post_update)
index 70ddc7ba207f1834ede2ad7e3f709f088bedf30f..78cd74d22c9df300c74c59eb68cf94ac9f5942b2 100644 (file)
@@ -533,6 +533,7 @@ def assert_raises_message(except_cls, msg, callable_, *args, **kwargs):
         assert False, "Callable did not raise an exception"
     except except_cls, e:
         assert re.search(msg, str(e)), "%r !~ %s" % (msg, e)
+        print str(e)
 
 def fail(msg):
     assert False, msg
index 7f67631a9870e42412de64cd384394476adecbfb..ce315ff35093cf7ba2aaada6bd06715302cafb35 100644 (file)
@@ -1797,6 +1797,13 @@ class InvalidRelationshipEscalationTest(_base.MappedTest):
               Column('id', Integer, primary_key=True),
               Column('fid', Integer))
 
+        Table('foos_with_fks', metadata,
+            Column('id', Integer, primary_key=True),
+            Column('fid', Integer, ForeignKey('foos_with_fks.id')))
+        Table('bars_with_fks', metadata,
+            Column('id', Integer, primary_key=True),
+            Column('fid', Integer, ForeignKey('foos_with_fks.id')))
+
     @classmethod
     def setup_classes(cls):
         class Foo(_base.Entity):
@@ -1859,11 +1866,20 @@ class InvalidRelationshipEscalationTest(_base.MappedTest):
                             foreign_keys=[foos.c.id, bars.c.fid])})
         mapper(Bar, bars)
 
-        assert_raises_message(
-            sa.exc.ArgumentError, 
-                "Do the columns in 'foreign_keys' represent only the "
-                "'foreign' columns in this join condition ?", 
-                sa.orm.compile_mappers)
+        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.compile_mappers)
 
     @testing.resolve_artifact_names
     def test_ambiguous_remoteside_o2m(self):
@@ -1931,11 +1947,19 @@ class InvalidRelationshipEscalationTest(_base.MappedTest):
                             viewonly=True)})
         mapper(Bar, bars)
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not determine relationship direction for primaryjoin condition",
-            sa.orm.compile_mappers)
+        assert_raises_message(sa.exc.ArgumentError,
+                              'Could not determine relationship '
+                              'direction for primaryjoin condition',
+                              sa.orm.compile_mappers)
 
+        sa.orm.clear_mappers()
+        mapper(Foo, foos_with_fks, properties={
+            'bars':relationship(Bar,
+                        primaryjoin=foos_with_fks.c.id>bars_with_fks.c.fid,
+                        viewonly=True)})
+        mapper(Bar, bars_with_fks)
+        sa.orm.compile_mappers()
+        
     @testing.resolve_artifact_names
     def test_no_equated_self_ref_viewonly(self):
         mapper(Foo, foos, properties={
@@ -1944,10 +1968,23 @@ class InvalidRelationshipEscalationTest(_base.MappedTest):
                             viewonly=True)})
         mapper(Bar, bars)
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Specify the 'foreign_keys' argument to indicate which columns "
-            "on the relationship are foreign.", sa.orm.compile_mappers)
+        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.compile_mappers)
+        
+        sa.orm.clear_mappers()
+        mapper(Foo, foos_with_fks, properties={
+          'foos':relationship(Foo,
+                          primaryjoin=foos_with_fks.c.id>foos_with_fks.c.fid,
+                          viewonly=True)})
+        mapper(Bar, bars_with_fks)
+        sa.orm.compile_mappers()
 
     @testing.resolve_artifact_names
     def test_no_equated_self_ref_viewonly_fks(self):
@@ -1972,6 +2009,13 @@ class InvalidRelationshipEscalationTest(_base.MappedTest):
             "Could not determine relationship direction for primaryjoin condition",
             sa.orm.compile_mappers)
 
+        sa.orm.clear_mappers()
+        mapper(Foo, foos_with_fks, properties={
+            'bars':relationship(Bar,
+                            primaryjoin=foos_with_fks.c.id==bars_with_fks.c.fid)})
+        mapper(Bar, bars_with_fks)
+        sa.orm.compile_mappers()
+
     @testing.resolve_artifact_names
     def test_equated_self_ref(self):
         mapper(Foo, foos, properties={
@@ -1982,6 +2026,7 @@ class InvalidRelationshipEscalationTest(_base.MappedTest):
             sa.exc.ArgumentError,
             "Could not determine relationship direction for primaryjoin condition",
             sa.orm.compile_mappers)
+        
 
     @testing.resolve_artifact_names
     def test_equated_self_ref_wrong_fks(self):
@@ -2007,6 +2052,20 @@ class InvalidRelationshipEscalationTestM2M(_base.MappedTest):
         Table('bars', metadata,
               Column('id', Integer, primary_key=True))
 
+        Table('foobars_with_fks', metadata,
+            Column('fid', Integer, ForeignKey('foos.id')), 
+            Column('bid', Integer, ForeignKey('bars.id'))
+        )
+
+        Table('foobars_with_many_columns', metadata,
+              Column('fid', Integer), 
+              Column('bid', Integer),
+              Column('fid1', Integer), 
+              Column('bid1', Integer),
+              Column('fid2', Integer), 
+              Column('bid2', Integer),
+              )
+
     @classmethod
     @testing.resolve_artifact_names
     def setup_classes(cls):
@@ -2040,6 +2099,80 @@ class InvalidRelationshipEscalationTestM2M(_base.MappedTest):
             "on relationship",
             sa.orm.compile_mappers)
 
+    @testing.resolve_artifact_names
+    def test_no_fks_warning_1(self):
+        mapper(Foo, foos, properties={
+            'bars': relationship(Bar, secondary=foobars, 
+                                primaryjoin=foos.c.id==foobars.c.fid,
+                                secondaryjoin=foobars.c.bid==bars.c.id)})
+        mapper(Bar, bars)
+        
+        assert_raises_message(sa.exc.SAWarning,
+                              "No ForeignKey objects were present in "
+                              "secondary table 'foobars'.  Assumed "
+                              "referenced foreign key columns "
+                              "'foobars.bid', 'foobars.fid' for join "
+                              "condition 'foos.id = foobars.fid' on "
+                              "relationship Foo.bars",
+                              sa.orm.compile_mappers)
+        
+        sa.orm.clear_mappers()
+        mapper(Foo, foos, properties={
+                        'bars': relationship(Bar, secondary=foobars_with_many_columns, 
+                              primaryjoin=foos.c.id==foobars_with_many_columns.c.fid,
+                              secondaryjoin=foobars_with_many_columns.c.bid==bars.c.id)})
+        mapper(Bar, bars)
+
+        assert_raises_message(sa.exc.SAWarning,
+                              "No ForeignKey objects were present in "
+                              "secondary table 'foobars_with_many_colum"
+                              "ns'.  Assumed referenced foreign key "
+                              "columns 'foobars_with_many_columns.bid',"
+                              " 'foobars_with_many_columns.bid1', "
+                              "'foobars_with_many_columns.bid2', "
+                              "'foobars_with_many_columns.fid', "
+                              "'foobars_with_many_columns.fid1', "
+                              "'foobars_with_many_columns.fid2' for "
+                              "join condition 'foos.id = "
+                              "foobars_with_many_columns.fid' on "
+                              "relationship Foo.bars",
+                              sa.orm.compile_mappers)
+
+    @testing.emits_warning(r'No ForeignKey objects.*')
+    @testing.resolve_artifact_names
+    def test_no_fks_warning_2(self):
+        mapper(Foo, foos, properties={
+            'bars': relationship(Bar, secondary=foobars, 
+                                primaryjoin=foos.c.id==foobars.c.fid,
+                                secondaryjoin=foobars.c.bid==bars.c.id)})
+        mapper(Bar, bars)
+        sa.orm.compile_mappers()
+        eq_(
+            Foo.bars.property.synchronize_pairs,
+            [(foos.c.id, foobars.c.fid)]
+        )
+        eq_(
+            Foo.bars.property.secondary_synchronize_pairs,
+            [(bars.c.id, foobars.c.bid)]
+        )
+
+        sa.orm.clear_mappers()
+        mapper(Foo, foos, properties={
+                        'bars': relationship(Bar, secondary=foobars_with_many_columns, 
+                              primaryjoin=foos.c.id==foobars_with_many_columns.c.fid,
+                              secondaryjoin=foobars_with_many_columns.c.bid==bars.c.id)})
+        mapper(Bar, bars)
+        sa.orm.compile_mappers()
+        eq_(
+            Foo.bars.property.synchronize_pairs,
+            [(foos.c.id, foobars_with_many_columns.c.fid)]
+        )
+        eq_(
+            Foo.bars.property.secondary_synchronize_pairs,
+            [(bars.c.id, foobars_with_many_columns.c.bid)]
+        )
+        
+        
     @testing.resolve_artifact_names
     def test_bad_primaryjoin(self):
         mapper(Foo, foos, properties={
@@ -2053,7 +2186,29 @@ class InvalidRelationshipEscalationTestM2M(_base.MappedTest):
             sa.exc.ArgumentError,
             "Could not determine relationship direction for primaryjoin condition",
             sa.orm.compile_mappers)
+    
+        sa.orm.clear_mappers()
+        mapper(Foo, foos, properties={
+            'bars': relationship(Bar,
+                             secondary=foobars_with_fks,
+                             primaryjoin=foos.c.id > foobars_with_fks.c.fid,
+                             secondaryjoin=foobars_with_fks.c.bid<=bars.c.id)})
+        mapper(Bar, bars)
+        assert_raises_message(
+            sa.exc.ArgumentError,
+            "Could not locate any equated, locally mapped column pairs for primaryjoin condition ",
+            sa.orm.compile_mappers)
 
+        sa.orm.clear_mappers()
+        mapper(Foo, foos, properties={
+            'bars': relationship(Bar,
+                             secondary=foobars_with_fks,
+                             primaryjoin=foos.c.id > foobars_with_fks.c.fid,
+                             secondaryjoin=foobars_with_fks.c.bid<=bars.c.id,
+                             viewonly=True)})
+        mapper(Bar, bars)
+        sa.orm.compile_mappers()
+        
     @testing.resolve_artifact_names
     def test_bad_secondaryjoin(self):
         mapper(Foo, foos, properties={
@@ -2064,10 +2219,20 @@ class InvalidRelationshipEscalationTestM2M(_base.MappedTest):
                             foreign_keys=[foobars.c.fid])})
         mapper(Bar, bars)
 
-        assert_raises_message(
-            sa.exc.ArgumentError,
-            "Could not determine relationship direction for secondaryjoin "
-            "condition", sa.orm.compile_mappers)
+        assert_raises_message(sa.exc.ArgumentError,
+                              "Could not determine relationship "
+                              "direction for secondaryjoin condition "
+                              r"'foobars.bid \<\= bars.id', 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 "
+                              "secondary Table already have adequate "
+                              "ForeignKey and/or ForeignKeyConstraint "
+                              r"objects established \(in which case "
+                              r"'foreign_keys' is usually unnecessary\)?"
+                              , sa.orm.compile_mappers)
 
     @testing.resolve_artifact_names
     def test_no_equated_secondaryjoin(self):