- add additional logic that duplicates mapper's prop.copy(); prop.columns.append...

logic when columns are present in a joined subclass with an attribute name different
than the column name itself [ticket:1931]
- add coverage to verify that we need to check (obj.name or name) when
deciding if a Column from a mixin should be added to the mapped table

diff --git a/CHANGES b/CHANGES
index c6de24280a28b9398675008c2aba01a4bf838389..a46b74b939ca9105daa73f6f4f71dc7528b9bb36 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -149,10 +149,10 @@ CHANGES
      not just an attribute technique.  [ticket:1915]
 
    - Fixed bug whereby columns on a mixin wouldn't propagate
-     correctly to a single-table inheritance scheme where
-     the attribute name is different than that of the column.
-     [ticket:1930].   Note [ticket:1931] which is the same
-     issue for joined inh, not yet resolved.
+     correctly to a single-table, or joined-table,
+     inheritance scheme where the attribute name is
+     different than that of the column. [ticket:1930],
+     [ticket:1931].
           
 - engine
    

diff --git a/lib/sqlalchemy/ext/declarative.py b/lib/sqlalchemy/ext/declarative.py
index 211b6724a70aba3d50de50cdf52f240210aa6880..fabd9aaf903c34ce5039de889892505a1a3a484b 100755 (executable)
--- a/lib/sqlalchemy/ext/declarative.py
+++ b/lib/sqlalchemy/ext/declarative.py
@@ -855,7 +855,7 @@ from sqlalchemy.orm.interfaces import MapperProperty
 from sqlalchemy.orm.properties import RelationshipProperty, ColumnProperty
 from sqlalchemy.orm.util import _is_mapped_class
 from sqlalchemy import util, exceptions
-from sqlalchemy.sql import util as sql_util
+from sqlalchemy.sql import util as sql_util, expression
 
 
 __all__ = 'declarative_base', 'synonym_for', \
@@ -938,9 +938,6 @@ def _as_declarative(cls, classname, dict_):
                         "on declarative mixin classes. ")
                     if name not in dict_ and not (
                             '__table__' in dict_ and 
-                            # TODO: coverage here when obj.name is not 
-                            # None and obj.name != name and obj.name in 
-                            # table.c
                             (obj.name or name) in dict_['__table__'].c
                             ):
                         potential_columns[name] = \
@@ -1119,7 +1116,25 @@ def _as_declarative(cls, classname, dict_):
                 set([c.key for c in inherited_table.c
                      if c not in inherited_mapper._columntoproperty])
             exclude_properties.difference_update([c.key for c in cols])
-    
+        
+        # look through columns in the current mapper that 
+        # are keyed to a propname different than the colname
+        # (if names were the same, we'd have popped it out above,
+        # in which case the mapper makes this combination).
+        # See if the superclass has a similar column property.   
+        # If so, join them together.   
+        for k, col in our_stuff.items():
+            if not isinstance(col, expression.ColumnElement):
+                continue
+            if k in inherited_mapper._props:
+                p = inherited_mapper._props[k]
+                if isinstance(p, ColumnProperty):
+                    # note here we place the superclass column
+                    # first.  this corresponds to the 
+                    # append() in mapper._configure_property().
+                    # change this ordering when we do [ticket:1892]
+                    our_stuff[k] = p.columns + [col]
+                
     cls.__mapper__ = mapper_cls(cls, 
                                 table, 
                                 properties=our_stuff, 

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index d27b9960183ee7c66bd207489d2dc52b841869e0..378570723b3e6c0ff026f706e82d0cff5840b2f9 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -639,7 +639,9 @@ class Mapper(object):
                             "or more attributes for these same-named columns "
                             "explicitly."
                              % (prop.columns[-1], column, key))
-
+                    
+                # this hypothetically changes to 
+                # prop.columns.insert(0, column) when we do [ticket:1892]
                 prop.columns.append(column)
                 self._log("appending to existing ColumnProperty %s" % (key))
                              

diff --git a/test/ext/test_declarative.py b/test/ext/test_declarative.py
index 1b3e72fc7f13546be28608c3583627c45a938e90..0202aa69fc3768863e15ed0208ea26bdd0628760 100644 (file)
--- a/test/ext/test_declarative.py
+++ b/test/ext/test_declarative.py
@@ -2305,6 +2305,13 @@ class DeclarativeMixinTest(DeclarativeTestBase):
         eq_(General.__table__.kwargs, {'mysql_engine': 'InnoDB'})
 
     def test_columns_single_table_inheritance(self):
+        """Test a column on a mixin with an alternate attribute name,
+        mapped to a superclass and single-table inheritance subclass.
+        The superclass table gets the column, the subclass shares
+        the MapperProperty.
+        
+        """
+        
         class MyMixin(object):
             foo = Column('foo', Integer)
             bar = Column('bar_newname', Integer)
@@ -2318,8 +2325,18 @@ class DeclarativeMixinTest(DeclarativeTestBase):
         class Specific(General):
             __mapper_args__ = {'polymorphic_identity': 'specific'}
 
-    @testing.fails_if(lambda: True, "Unhandled declarative use case")
+        assert General.bar.prop.columns[0] is General.__table__.c.bar_newname
+        assert len(General.bar.prop.columns) == 1
+        assert Specific.bar.prop is General.bar.prop
+        
     def test_columns_joined_table_inheritance(self):
+        """Test a column on a mixin with an alternate attribute name,
+        mapped to a superclass and joined-table inheritance subclass.
+        Both tables get the column, in the case of the subclass the two
+        columns are joined under one MapperProperty.
+        
+        """
+
         class MyMixin(object):
             foo = Column('foo', Integer)
             bar = Column('bar_newname', Integer)
@@ -2334,6 +2351,52 @@ class DeclarativeMixinTest(DeclarativeTestBase):
             __tablename__ = 'sub'
             id = Column(Integer, ForeignKey('test.id'), primary_key=True)
             __mapper_args__ = {'polymorphic_identity': 'specific'}
+
+        assert General.bar.prop.columns[0] is General.__table__.c.bar_newname
+        assert len(General.bar.prop.columns) == 1
+        assert Specific.bar.prop is not General.bar.prop
+        assert len(Specific.bar.prop.columns) == 2
+        assert Specific.bar.prop.columns[0] is General.__table__.c.bar_newname
+        assert Specific.bar.prop.columns[1] is Specific.__table__.c.bar_newname
+    
+    def test_column_join_checks_superclass_type(self):
+        """Test that the logic which joins subclass props to those
+        of the superclass checks that the superclass property is a column.
+
+        """
+        class General(Base):
+            __tablename__ = 'test'
+            id = Column(Integer, primary_key=True)
+            general_id = Column(Integer, ForeignKey('test.id'))
+            type_ = relationship("General")
+
+        class Specific(General):
+            __tablename__ = 'sub'
+            id = Column(Integer, ForeignKey('test.id'), primary_key=True)
+            type_ = Column('foob', String(50))
+        
+        assert isinstance(General.type_.property, sa.orm.RelationshipProperty)
+        assert Specific.type_.property.columns[0] is Specific.__table__.c.foob
+
+    def test_column_join_checks_subclass_type(self):
+        """Test that the logic which joins subclass props to those
+        of the superclass checks that the subclass property is a column.
+
+        """
+        def go():
+            class General(Base):
+                __tablename__ = 'test'
+                id = Column(Integer, primary_key=True)
+                type_ = Column('foob', Integer)
+
+            class Specific(General):
+                __tablename__ = 'sub'
+                id = Column(Integer, ForeignKey('test.id'), primary_key=True)
+                specific_id = Column(Integer, ForeignKey('sub.id'))
+                type_ = relationship("Specific")
+        assert_raises_message(
+            sa.exc.ArgumentError, "column 'foob' conflicts with property", go
+        )
         
     def test_table_args_overridden(self):
 
@@ -2777,7 +2840,6 @@ class DeclarativeMixinTest(DeclarativeTestBase):
         class ColumnMixin:
             tada = Column(Integer)
             
-            
         def go():
 
             class Model(Base, ColumnMixin):
@@ -2791,6 +2853,29 @@ class DeclarativeMixinTest(DeclarativeTestBase):
                               "Can't add additional column 'tada' when "
                               "specifying __table__", go)
 
+    def test_table_in_model_and_different_named_alt_key_column_in_mixin(self):
+        
+        # here, the __table__ has a column 'tada'.  We disallow
+        # the add of the 'foobar' column, even though it's
+        # keyed to 'tada'.
+        
+        class ColumnMixin:
+            tada = Column('foobar', Integer)
+            
+        def go():
+
+            class Model(Base, ColumnMixin):
+
+                __table__ = Table('foo', Base.metadata, 
+                                Column('data',Integer), 
+                                Column('tada', Integer),
+                                Column('id', Integer,primary_key=True))
+                foo = relationship("Dest")
+                
+        assert_raises_message(sa.exc.ArgumentError,
+                              "Can't add additional column 'foobar' when "
+                              "specifying __table__", go)
+
     def test_table_in_model_overrides_different_typed_column_in_mixin(self):
 
         class ColumnMixin: