- The include_properties and exclude_properties arguments

to mapper() now accept Column objects as members in
addition to strings.  This so that same-named Column
objects, such as those within a join(), can be
disambiguated.

- A warning is now emitted if a mapper is created against a
join or other single selectable that includes multiple
columns with the same name in its .c. collection,
and those columns aren't explictly named as part of
the same or separate attributes (or excluded).
In 0.7 this warning will be an exception.   Note that
this warning is not emitted when the combination occurs
as a result of inheritance, so that attributes
still allow being overridden naturally.
[ticket:1896].  In 0.7 this will be improved further.

- The primary_key argument to mapper() can now specify
a series of columns that are only a subset of
the calculated "primary key" columns of the mapped
selectable, without an error being raised.  This
helps for situations where a selectable's effective
primary key is simpler than the number of columns
in the selectable that are actually marked as
"primary_key", such as a join against two
tables on their primary key columns [ticket:1896].

8 files changed, 197 insertions, 53 deletions

diff --git a/CHANGES b/CHANGES
index a4e5c2283..d78cd6655 100644
--- a/CHANGES
+++ b/CHANGES
@@ -21,6 +21,33 @@ CHANGES
     where a one-step Session constructor is desired. Most
     users should stick with sessionmaker() for general use,
     however.
+
+  - The include_properties and exclude_properties arguments
+    to mapper() now accept Column objects as members in 
+    addition to strings.  This so that same-named Column
+    objects, such as those within a join(), can be
+    disambiguated.
+
+  - A warning is now emitted if a mapper is created against a
+    join or other single selectable that includes multiple
+    columns with the same name in its .c. collection,
+    and those columns aren't explictly named as part of
+    the same or separate attributes (or excluded).
+    In 0.7 this warning will be an exception.   Note that
+    this warning is not emitted when the combination occurs
+    as a result of inheritance, so that attributes
+    still allow being overridden naturally.  
+    [ticket:1896].  In 0.7 this will be improved further.
+    
+  - The primary_key argument to mapper() can now specify
+    a series of columns that are only a subset of 
+    the calculated "primary key" columns of the mapped 
+    selectable, without an error being raised.  This
+    helps for situations where a selectable's effective
+    primary key is simpler than the number of columns
+    in the selectable that are actually marked as 
+    "primary_key", such as a join against two 
+    tables on their primary key columns [ticket:1896].
     
   - An object that's been deleted now gets a flag
     'deleted', which prohibits the object from
diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py
index 55daeb3db..17d967db4 100644
--- a/lib/sqlalchemy/orm/__init__.py
+++ b/lib/sqlalchemy/orm/__init__.py
@@ -671,22 +671,27 @@ def mapper(class_, local_table=None, *args, **params):
         :param concrete: If True, indicates this mapper should use concrete
            table inheritance with its parent mapper.
 
-        :param exclude_properties: A list of properties not to map.  Columns
-           present in the mapped table and present in this list will not be
-           automatically converted into properties. Note that neither this
-           option nor include_properties will allow an end-run around Python
-           inheritance. If mapped class ``B`` inherits from mapped class
-           ``A``, no combination of includes or excludes will allow ``B`` to
-           have fewer properties than its superclass, ``A``.
+        :param exclude_properties: A list or set of string column names to 
+          be excluded from mapping. As of SQLAlchemy 0.6.4, this collection
+          may also include :class:`.Column` objects. Columns named or present
+          in this list will not be automatically mapped. Note that neither
+          this option nor include_properties will allow one to circumvent plan
+          Python inheritance - if mapped class ``B`` inherits from mapped
+          class ``A``, no combination of includes or excludes will allow ``B``
+          to have fewer properties than its superclass, ``A``.
 
         :param extension: A :class:`.MapperExtension` instance or
            list of :class:`~sqlalchemy.orm.interfaces.MapperExtension`
            instances which will be applied to all operations by this
            :class:`~sqlalchemy.orm.mapper.Mapper`.
 
-        :param include_properties: An inclusive list of properties to map. 
-           Columns present in the mapped table but not present in this list
-           will not be automatically converted into properties.
+        :param include_properties: An inclusive list or set of string column
+          names to map. As of SQLAlchemy 0.6.4, this collection may also
+          include :class:`.Column` objects in order to disambiguate between
+          same-named columns in a selectable (such as a
+          :func:`~.expression.join()`). If this list is not ``None``, columns
+          present in the mapped table but not named or present in this list
+          will not be automatically mapped. See also "exclude_properties".
 
         :param inherits: Another :class:`~sqlalchemy.orm.Mapper` for which 
             this :class:`~sqlalchemy.orm.Mapper` will have an inheritance
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index c3e4b042e..1f2216cec 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -192,8 +192,14 @@ class Mapper(object):
         else:
             self.polymorphic_map = _polymorphic_map
 
-        self.include_properties = include_properties
-        self.exclude_properties = exclude_properties
+        if include_properties:
+            self.include_properties = util.to_set(include_properties)
+        else:
+            self.include_properties = None
+        if exclude_properties:
+            self.exclude_properties = util.to_set(exclude_properties)
+        else:
+            self.exclude_properties = None
 
         self.compiled = False
         
@@ -471,7 +477,7 @@ class Mapper(object):
             for col in self._columntoproperty
             if not hasattr(col, 'table') or 
             col.table not in self._cols_by_table)
-
+        
         # if explicit PK argument sent, add those columns to the 
         # primary key mappings
         if self.primary_key_argument:
@@ -479,13 +485,14 @@ class Mapper(object):
                 if k.table not in self._pks_by_table:
                     self._pks_by_table[k.table] = util.OrderedSet()
                 self._pks_by_table[k.table].add(k)
-
-        if self.mapped_table not in self._pks_by_table or \
-                len(self._pks_by_table[self.mapped_table]) == 0:
-            raise sa_exc.ArgumentError(
-                    "Mapper %s could not assemble any primary "
-                    "key columns for mapped table '%s'" % 
-                    (self, self.mapped_table.description))
+        
+        # otherwise, see that we got a full PK for the mapped table
+        elif self.mapped_table not in self._pks_by_table or \
+                    len(self._pks_by_table[self.mapped_table]) == 0:
+                raise sa_exc.ArgumentError(
+                        "Mapper %s could not assemble any primary "
+                        "key columns for mapped table '%s'" % 
+                        (self, self.mapped_table.description))
 
         if self.inherits and \
                 not self.concrete and \
@@ -537,7 +544,7 @@ class Mapper(object):
         if self.inherits:
             for key, prop in self.inherits._props.iteritems():
                 if key not in self._props and \
-                    not self._should_exclude(key, key, local=False):
+                    not self._should_exclude(key, key, local=False, column=None):
                     self._adapt_inherited_property(key, prop, False)
 
         # create properties for each column in the mapped table,
@@ -550,7 +557,8 @@ class Mapper(object):
 
             if self._should_exclude(
                             column.key, column_key,
-                             local=self.local_table.c.contains_column(column)
+                             local=self.local_table.c.contains_column(column),
+                             column=column
                             ):
                 continue
 
@@ -583,7 +591,7 @@ class Mapper(object):
                               % col.description)
             else:
                 instrument = True
-            if self._should_exclude(col.key, col.key, local=False):
+            if self._should_exclude(col.key, col.key, local=False, column=col):
                 raise sa_exc.InvalidRequestError(
                     "Cannot exclude or override the discriminator column %r" %
                     col.key)
@@ -625,8 +633,18 @@ class Mapper(object):
                     # existing ColumnProperty from an inheriting mapper.
                     # make a copy and append our column to it
                     prop = prop.copy()
+                else:
+                    util.warn(
+                            "Implicitly combining column %s with column "
+                            "%s under attribute '%s'.  This usage will be "
+                            "prohibited in 0.7.  Please configure one "
+                            "or more attributes for these same-named columns "
+                            "explicitly."
+                             % (prop.columns[-1], column, key))
+
                 prop.columns.append(column)
                 self._log("appending to existing ColumnProperty %s" % (key))
+                             
             elif prop is None or isinstance(prop, ConcreteInheritedProperty):
                 mapped_column = []
                 for c in columns:
@@ -1099,7 +1117,7 @@ class Mapper(object):
                     (MapperProperty, attributes.InstrumentedAttribute)) and \
                     hasattr(obj, '__get__')
 
-    def _should_exclude(self, name, assigned_name, local):
+    def _should_exclude(self, name, assigned_name, local, column):
         """determine whether a particular property should be implicitly
         present on the class.
 
@@ -1121,13 +1139,17 @@ class Mapper(object):
                             getattr(self.class_, assigned_name)):
                 return True
 
-        if (self.include_properties is not None and
-            name not in self.include_properties):
+        if self.include_properties is not None and \
+                name not in self.include_properties and \
+                (column is None or column not in self.include_properties):
             self._log("not including property %s" % (name))
             return True
 
-        if (self.exclude_properties is not None and
-            name in self.exclude_properties):
+        if self.exclude_properties is not None and \
+            (
+                name in self.exclude_properties or \
+                (column is not None and column in self.exclude_properties)
+            ):
             self._log("excluding property %s" % (name))
             return True
 
diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index 7e19d7b16..263b611a5 100644
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -60,7 +60,17 @@ class ColumnProperty(StrategizedProperty):
                                             self.__class__.Comparator)
         self.descriptor = kwargs.pop('descriptor', None)
         self.extension = kwargs.pop('extension', None)
-        self.doc = kwargs.pop('doc', getattr(columns[0], 'doc', None))
+        
+        if 'doc' in kwargs:
+            self.doc = kwargs.pop('doc')
+        else:
+            for col in reversed(self.columns):
+                doc = getattr(col, 'doc', None)
+                if doc is not None:
+                    self.doc = doc
+                    break
+            else:
+                self.doc = None
         
         if kwargs:
             raise TypeError(
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 1c4571aed..4d98e8e62 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -118,17 +118,20 @@ class ColumnLoader(LoaderStrategy):
        )
         
     def create_row_processor(self, selectcontext, path, mapper, row, adapter):
-        key, col = self.key, self.columns[0]
-        if adapter:
-            col = adapter.columns[col]
-            
-        if col is not None and col in row:
-            def new_execute(state, dict_, row):
-                dict_[key] = row[col]
+        key = self.key
+        # look through list of columns represented here
+        # to see which, if any, is present in the row.
+        for col in self.columns:
+            if adapter:
+                col = adapter.columns[col]
+            if col is not None and col in row:
+                def new_execute(state, dict_, row):
+                    dict_[key] = row[col]
+                return new_execute, None
         else:
             def new_execute(state, dict_, row):
                 state.expire_attribute_pre_commit(dict_, key)
-        return new_execute, None
+            return new_execute, None
 
 log.class_logger(ColumnLoader)
 
diff --git a/test/ext/test_declarative.py b/test/ext/test_declarative.py
index 71e31233b..26e1563fe 100644
--- a/test/ext/test_declarative.py
+++ b/test/ext/test_declarative.py
@@ -1316,7 +1316,42 @@ class DeclarativeInheritanceTest(DeclarativeTestBase):
             primary_language = Column('primary_language', String(50))
 
         assert class_mapper(Engineer).inherits is class_mapper(Person)
+    
+    @testing.fails_if(lambda: True, "Not implemented until 0.7")
+    def test_foreign_keys_with_col(self):
+        """Test that foreign keys that reference a literal 'id' subclass 
+        'id' attribute behave intuitively.  
+        
+        See ticket 1892.
+        
+        """
+        class Booking(Base):
+            __tablename__ = 'booking'
+            id = Column(Integer, primary_key=True)
+
+        class PlanBooking(Booking):
+            __tablename__ = 'plan_booking'
+            id = Column(Integer, ForeignKey(Booking.id),
+                            primary_key=True)
+
+        # referencing PlanBooking.id gives us the column
+        # on plan_booking, not booking
+        class FeatureBooking(Booking):
+            __tablename__ = 'feature_booking'
+            id = Column(Integer, ForeignKey(Booking.id),
+                                        primary_key=True)
+            plan_booking_id = Column(Integer,
+                                ForeignKey(PlanBooking.id))
+            
+            plan_booking = relationship(PlanBooking,
+                        backref='feature_bookings')
+        
+        assert FeatureBooking.__table__.c.plan_booking_id.\
+                    references(PlanBooking.__table__.c.id)
 
+        assert FeatureBooking.__table__.c.id.\
+                    references(Booking.__table__.c.id)
+      
     def test_with_undefined_foreignkey(self):
 
         class Parent(Base):
diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py
index b4aaf13ba..c6dec16b7 100644
--- a/test/orm/inheritance/test_basic.py
+++ b/test/orm/inheritance/test_basic.py
@@ -981,7 +981,8 @@ class OverrideColKeyTest(_base.MappedTest):
         # s2 gets a new id, base_id is overwritten by the ultimate
         # PK col
         assert s2.id == s2.base_id != 15
-        
+    
+    @testing.emits_warning(r'Implicit')
     def test_override_implicit(self):
         # this is how the pattern looks intuitively when 
         # using declarative.
@@ -1143,7 +1144,9 @@ class OptimizedLoadTest(_base.MappedTest):
         
         # redefine Sub's "id" to favor the "id" col in the subtable.
         # "id" is also part of the primary join condition
-        mapper(Sub, sub, inherits=Base, polymorphic_identity='sub', properties={'id':sub.c.id})
+        mapper(Sub, sub, inherits=Base, 
+                        polymorphic_identity='sub',
+                        properties={'id':[sub.c.id, base.c.id]})
         sess = sessionmaker()()
         s1 = Sub(data='s1data', sub='s1sub')
         sess.add(s1)
diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py
index e24906a1f..3012f4b43 100644
--- a/test/orm/test_mapper.py
+++ b/test/orm/test_mapper.py
@@ -482,18 +482,19 @@ class MapperTest(_fixtures.FixtureTest):
         class Manager(Employee): pass
         class Hoho(object): pass
         class Lala(object): pass
-
+        class Fub(object):pass
+        class Frob(object):pass
         class HasDef(object):
             def name(self):
                 pass
             
         p_m = mapper(Person, t, polymorphic_on=t.c.type,
                      include_properties=('id', 'type', 'name'))
-        e_m = mapper(Employee, inherits=p_m, polymorphic_identity='employee',
-          properties={
-            'boss': relationship(Manager, backref=backref('peon', ), remote_side=t.c.id)
-          },
-          exclude_properties=('vendor_id',))
+        e_m = mapper(Employee, inherits=p_m,
+                     polymorphic_identity='employee', properties={'boss'
+                     : relationship(Manager, backref=backref('peon'),
+                     remote_side=t.c.id)},
+                     exclude_properties=('vendor_id', ))
 
         m_m = mapper(Manager, inherits=e_m, polymorphic_identity='manager',
                      include_properties=('id', 'type'))
@@ -506,8 +507,12 @@ class MapperTest(_fixtures.FixtureTest):
 
         hd_m = mapper(HasDef, t, column_prefix="h_")
         
+        fb_m = mapper(Fub, t, include_properties=(t.c.id, t.c.type))
+        frb_m = mapper(Frob, t, column_prefix='f_',
+                       exclude_properties=(t.c.boss_id,
+                       'employee_number', t.c.vendor_id))
+        
         p_m.compile()
-        #sa.orm.compile_mappers()
 
         def assert_props(cls, want):
             have = set([n for n in dir(cls) if not n.startswith('_')])
@@ -519,7 +524,8 @@ class MapperTest(_fixtures.FixtureTest):
             want = set(want)
             eq_(have, want)
         
-        assert_props(HasDef, ['h_boss_id', 'h_employee_number', 'h_id', 'name', 'h_name', 'h_vendor_id', 'h_type'])    
+        assert_props(HasDef, ['h_boss_id', 'h_employee_number', 'h_id', 
+                                'name', 'h_name', 'h_vendor_id', 'h_type'])    
         assert_props(Person, ['id', 'name', 'type'])
         assert_instrumented(Person, ['id', 'name', 'type'])
         assert_props(Employee, ['boss', 'boss_id', 'employee_number',
@@ -535,24 +541,57 @@ class MapperTest(_fixtures.FixtureTest):
         assert_props(Vendor, ['vendor_id', 'id', 'name', 'type'])
         assert_props(Hoho, ['id', 'name', 'type'])
         assert_props(Lala, ['p_employee_number', 'p_id', 'p_name', 'p_type'])
-
+        assert_props(Fub, ['id', 'type'])
+        assert_props(Frob, ['f_id', 'f_type', 'f_name', ])
         # excluding the discriminator column is currently not allowed
         class Foo(Person):
             pass
-        assert_raises(sa.exc.InvalidRequestError, mapper, Foo, inherits=Person, polymorphic_identity='foo', exclude_properties=('type',) )
+
+        assert_raises(
+            sa.exc.InvalidRequestError,
+            mapper,
+            Foo, inherits=Person, polymorphic_identity='foo',
+            exclude_properties=('type', ),
+            )
     
     @testing.resolve_artifact_names
-    def test_mapping_to_join(self):
-        """Mapping to a join"""
+    def test_mapping_to_join_raises(self):
+        """Test implicit merging of two cols warns."""
+        
         usersaddresses = sa.join(users, addresses,
                                  users.c.id == addresses.c.user_id)
-        mapper(User, usersaddresses, primary_key=[users.c.id])
+        assert_raises_message(
+            sa.exc.SAWarning,
+            "Implicitly",
+            mapper, User, usersaddresses, primary_key=[users.c.id]
+        )
+        sa.orm.clear_mappers()
+        
+        @testing.emits_warning(r'Implicitly')
+        def go():
+            # but it works despite the warning
+            mapper(User, usersaddresses, primary_key=[users.c.id])
+            l = create_session().query(User).order_by(users.c.id).all()
+            eq_(l, self.static.user_result[:3])
+        go()
+
+    @testing.resolve_artifact_names
+    def test_mapping_to_join(self):
+        """Mapping to a join"""
+
+        usersaddresses = sa.join(users, addresses, users.c.id
+                                 == addresses.c.user_id)
+        mapper(User, usersaddresses, primary_key=[users.c.id],
+               exclude_properties=[addresses.c.id])
         l = create_session().query(User).order_by(users.c.id).all()
         eq_(l, self.static.user_result[:3])
 
     @testing.resolve_artifact_names
     def test_mapping_to_join_no_pk(self):
-        m = mapper(Address, addresses.join(email_bounces))
+        m = mapper(Address, 
+                    addresses.join(email_bounces), 
+                    properties={'id':[addresses.c.id, email_bounces.c.id]}
+                )
         m.compile()
         assert addresses in m._pks_by_table
         assert email_bounces not in m._pks_by_table