- 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]

- 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

4 files changed, 130 insertions, 90 deletions

diff --git a/lib/sqlalchemy/ext/declarative.py b/lib/sqlalchemy/ext/declarative.py
index 2310f01ce..e40ba3ec4 100755
--- a/lib/sqlalchemy/ext/declarative.py
+++ b/lib/sqlalchemy/ext/declarative.py
@@ -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))
diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py
index 2004dccd1..c74fabacd 100644
--- a/lib/sqlalchemy/orm/__init__.py
+++ b/lib/sqlalchemy/orm/__init__.py
@@ -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
diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index cbfba91f3..5788c30f9 100644
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -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)
diff --git a/lib/sqlalchemy/test/testing.py b/lib/sqlalchemy/test/testing.py
index 70ddc7ba2..78cd74d22 100644
--- a/lib/sqlalchemy/test/testing.py
+++ b/lib/sqlalchemy/test/testing.py
@@ -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