- [feature] Conflicts between columns on

    single-inheritance declarative subclasses,
    with or without using a mixin, can be resolved
    using a new @declared_attr usage described
    in the documentation.  [ticket:2472]

6 files changed, 132 insertions, 12 deletions

diff --git a/lib/sqlalchemy/ext/declarative/__init__.py b/lib/sqlalchemy/ext/declarative/__init__.py
index e6d6e388b..8bf03748e 100644
--- a/lib/sqlalchemy/ext/declarative/__init__.py
+++ b/lib/sqlalchemy/ext/declarative/__init__.py
@@ -447,6 +447,8 @@ only the ``engineers.id`` column, give it a different attribute name::
    column over that of the superclass, such as querying above
    for ``Engineer.id``.  Prior to 0.7 this was the reverse.
 
+.. _declarative_single_table:
+
 Single Table Inheritance
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -485,6 +487,115 @@ The attribute exclusion logic is provided by the
 behavior can be disabled by passing an explicit ``exclude_properties``
 collection (empty or otherwise) to the ``__mapper_args__``.
 
+Resolving Column Conflicts
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Note above that the ``primary_language`` and ``golf_swing`` columns
+are "moved up" to be applied to ``Person.__table__``, as a result of their
+declaration on a subclass that has no table of its own.   A tricky case
+comes up when two subclasses want to specify *the same* column, as below::
+
+    class Person(Base):
+        __tablename__ = 'people'
+        id = Column(Integer, primary_key=True)
+        discriminator = Column('type', String(50))
+        __mapper_args__ = {'polymorphic_on': discriminator}
+
+    class Engineer(Person):
+        __mapper_args__ = {'polymorphic_identity': 'engineer'}
+        start_date = Column(DateTime)
+
+    class Manager(Person):
+        __mapper_args__ = {'polymorphic_identity': 'manager'}
+        start_date = Column(DateTime)
+
+Above, the ``start_date`` column declared on both ``Engineer`` and ``Manager``
+will result in an error::
+
+    sqlalchemy.exc.ArgumentError: Column 'start_date' on class
+    <class '__main__.Manager'> conflicts with existing
+    column 'people.start_date'
+
+In a situation like this, Declarative can't be sure
+of the intent, especially if the ``start_date`` columns had, for example,
+different types.   A situation like this can be resolved by using
+:class:`.declared_attr` to define the :class:`.Column` conditionally, taking
+care to return the **existing column** via the parent ``__table__`` if it already
+exists::
+
+    from sqlalchemy.ext.declarative import declared_attr
+
+    class Person(Base):
+        __tablename__ = 'people'
+        id = Column(Integer, primary_key=True)
+        discriminator = Column('type', String(50))
+        __mapper_args__ = {'polymorphic_on': discriminator}
+
+    class Engineer(Person):
+        __mapper_args__ = {'polymorphic_identity': 'engineer'}
+
+        @declared_attr
+        def start_date(cls):
+            "Start date column, if not present already."
+            return Person.__table__.c.get('start_date', Column(DateTime))
+
+    class Manager(Person):
+        __mapper_args__ = {'polymorphic_identity': 'manager'}
+
+        @declared_attr
+        def start_date(cls):
+            "Start date column, if not present already."
+            return Person.__table__.c.get('start_date', Column(DateTime))
+
+Above, when ``Manager`` is mapped, the ``start_date`` column is
+already present on the ``Person`` class.  Declarative lets us return
+that :class:`.Column` as a result in this case, where it knows to skip
+re-assigning the same column. If the mapping is mis-configured such
+that the ``start_date`` column is accidentally re-assigned to a
+different table (such as, if we changed ``Manager`` to be joined
+inheritance without fixing ``start_date``), an error is raised which
+indicates an existing :class:`.Column` is trying to be re-assigned to
+a different owning :class:`.Table`.
+
+.. versionadded:: 0.8 :class:`.declared_attr` can be used on a non-mixin
+   class, and the returned :class:`.Column` or other mapped attribute
+   will be applied to the mapping as any other attribute.  Previously,
+   the resulting attribute would be ignored, and also result in a warning
+   being emitted when a subclass was created.
+
+.. versionadded:: 0.8 :class:`.declared_attr`, when used either with a
+   mixin or non-mixin declarative class, can return an existing
+   :class:`.Column` already assigned to the parent :class:`.Table`,
+   to indicate that the re-assignment of the :class:`.Column` should be
+   skipped, however should still be mapped on the target class,
+   in order to resolve duplicate column conflicts.
+
+The same concept can be used with mixin classes (see
+:ref:`declarative_mixins`)::
+
+    class Person(Base):
+        __tablename__ = 'people'
+        id = Column(Integer, primary_key=True)
+        discriminator = Column('type', String(50))
+        __mapper_args__ = {'polymorphic_on': discriminator}
+
+    class HasStartDate(object):
+        @declared_attr
+        def start_date(cls):
+            return cls.__table__.c.get('start_date', Column(DateTime))
+
+    class Engineer(HasStartDate, Person):
+        __mapper_args__ = {'polymorphic_identity': 'engineer'}
+
+    class Manager(HasStartDate, Person):
+        __mapper_args__ = {'polymorphic_identity': 'manager'}
+
+The above mixin checks the local ``__table__`` attribute for the column.
+Because we're using single table inheritance, we're sure that in this case,
+``cls.__table__`` refers to ``People.__table__``.  If we were mixing joined-
+and single-table inheritance, we might want our mixin to check more carefully
+if ``cls.__table__`` is really the :class:`.Table` we're looking for.
+
 Concrete Table Inheritance
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -708,7 +819,7 @@ keys, as a :class:`.ForeignKey` itself contains references to columns
 which can't be properly recreated at this level.  For columns that
 have foreign keys, as well as for the variety of mapper-level constructs
 that require destination-explicit context, the
-:func:`~.declared_attr` decorator is provided so that
+:class:`~.declared_attr` decorator is provided so that
 patterns common to many classes can be defined as callables::
 
     from sqlalchemy.ext.declarative import declared_attr
@@ -728,9 +839,9 @@ extension can use the resulting :class:`.Column` object as returned by
 the method without the need to copy it.
 
 .. versionchanged:: > 0.6.5
-    Rename 0.6.5 ``sqlalchemy.util.classproperty`` into :func:`~.declared_attr`.
+    Rename 0.6.5 ``sqlalchemy.util.classproperty`` into :class:`~.declared_attr`.
 
-Columns generated by :func:`~.declared_attr` can also be
+Columns generated by :class:`~.declared_attr` can also be
 referenced by ``__mapper_args__`` to a limited degree, currently
 by ``polymorphic_on`` and ``version_id_col``, by specifying the
 classdecorator itself into the dictionary - the declarative extension
@@ -747,6 +858,8 @@ will resolve them at class construction time::
         __tablename__='test'
         id =  Column(Integer, primary_key=True)
 
+
+
 Mixing in Relationships
 ~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py
index 80934c194..143468c13 100644
--- a/lib/sqlalchemy/ext/declarative/api.py
+++ b/lib/sqlalchemy/ext/declarative/api.py
@@ -8,7 +8,8 @@
 
 from ...schema import Table, MetaData
 from ...orm import synonym as _orm_synonym, mapper,\
-                                comparable_property
+                                comparable_property,\
+                                interfaces
 from ...orm.util import polymorphic_union, _mapper_or_none
 from ... import exc
 import weakref
@@ -96,7 +97,7 @@ def comparable_using(comparator_factory):
         return comparable_property(comparator_factory, fn)
     return decorate
 
-class declared_attr(property):
+class declared_attr(interfaces._MappedAttribute, property):
     """Mark a class-level method as representing the definition of
     a mapped property or special declarative member name.
 
diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 0348da744..e42ec2645 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -254,7 +254,6 @@ def _as_declarative(cls, classname, dict_):
                     "Can't place __table_args__ on an inherited class "
                     "with no table."
                     )
-
             # add any columns declared here to the inherited table.
             for c in declared_columns:
                 if c.primary_key:
@@ -263,6 +262,8 @@ def _as_declarative(cls, classname, dict_):
                         "class with no table."
                         )
                 if c.name in inherited_table.c:
+                    if inherited_table.c[c.name] is c:
+                        continue
                     raise exc.ArgumentError(
                         "Column '%s' on class %s conflicts with "
                         "existing column '%s'" %
@@ -354,7 +355,6 @@ class _MapperConfig(object):
                         # note here we place the subclass column
                         # first.  See [ticket:1892] for background.
                         properties[k] = [col] + p.columns
-
         result_mapper_args = mapper_args.copy()
         result_mapper_args['properties'] = properties
         return result_mapper_args
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 9b0b35e28..eec72b5f3 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -117,7 +117,9 @@ PASSIVE_ONLY_PERSISTENT = util.symbol("PASSIVE_ONLY_PERSISTENT",
 )
 
 
-class QueryableAttribute(interfaces._InspectionAttr, interfaces.PropComparator):
+class QueryableAttribute(interfaces._MappedAttribute,
+                            interfaces._InspectionAttr,
+                            interfaces.PropComparator):
     """Base class for class-bound attributes. """
 
     is_attribute = True
diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py
index 12c38b595..272d4edd5 100644
--- a/lib/sqlalchemy/orm/interfaces.py
+++ b/lib/sqlalchemy/orm/interfaces.py
@@ -65,7 +65,12 @@ class _InspectionAttr(object):
     is_attribute = False
     is_clause_element = False
 
-class MapperProperty(_InspectionAttr):
+class _MappedAttribute(object):
+    """Mixin for attributes which should be replaced by mapper-assigned
+    attributes.
+
+    """
+class MapperProperty(_MappedAttribute, _InspectionAttr):
     """Manage the relationship of a ``Mapper`` to a single class
     attribute, as well as that attribute as it appears on individual
     instances of the class, including attribute instrumentation,
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index bcc1a2454..45124127a 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -23,7 +23,7 @@ from .. import sql, util, log, exc as sa_exc, event, schema, inspection
 from ..sql import expression, visitors, operators, util as sql_util
 from . import instrumentation, attributes, \
                         exc as orm_exc, events, loading
-from .interfaces import MapperProperty, _InspectionAttr
+from .interfaces import MapperProperty, _InspectionAttr, _MappedAttribute
 
 from .util import _INSTRUMENTOR, _class_to_mapper, \
      _state_mapper, class_mapper, \
@@ -1629,8 +1629,7 @@ class Mapper(_InspectionAttr):
         return result
 
     def _is_userland_descriptor(self, obj):
-        if isinstance(obj, (MapperProperty,
-                            attributes.QueryableAttribute,
+        if isinstance(obj, (_MappedAttribute,
                             instrumentation.ClassManager,
                             expression.ColumnElement)):
             return False