From 640625bc9e98dd4060a1e61c717ddc98f8b3808b Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 27 Aug 2012 16:04:16 -0400 Subject: [PATCH] - [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] --- CHANGES | 6 ++ lib/sqlalchemy/ext/declarative/__init__.py | 119 ++++++++++++++++++++- lib/sqlalchemy/ext/declarative/api.py | 5 +- lib/sqlalchemy/ext/declarative/base.py | 4 +- lib/sqlalchemy/orm/attributes.py | 4 +- lib/sqlalchemy/orm/interfaces.py | 7 +- lib/sqlalchemy/orm/mapper.py | 5 +- test/ext/declarative/test_basic.py | 36 +++---- test/ext/declarative/test_inheritance.py | 60 +++++++++++ test/ext/declarative/test_mixin.py | 57 +++++++++- 10 files changed, 271 insertions(+), 32 deletions(-) diff --git a/CHANGES b/CHANGES index 1430ad0b7b..1f488b013a 100644 --- a/CHANGES +++ b/CHANGES @@ -236,6 +236,12 @@ underneath "0.7.xx". mappers*. See the next note for this ticket. [ticket:2526] + - [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] + - [feature] *Very limited* support for inheriting mappers to be GC'ed when the class itself is deferenced. The mapper diff --git a/lib/sqlalchemy/ext/declarative/__init__.py b/lib/sqlalchemy/ext/declarative/__init__.py index e6d6e388b5..8bf03748e2 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 + 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 80934c194f..143468c136 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 0348da744d..e42ec26452 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 9b0b35e284..eec72b5f33 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 12c38b595b..272d4edd57 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 bcc1a24548..45124127a3 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 diff --git a/test/ext/declarative/test_basic.py b/test/ext/declarative/test_basic.py index 2d6534942e..5af2b88dc5 100644 --- a/test/ext/declarative/test_basic.py +++ b/test/ext/declarative/test_basic.py @@ -897,26 +897,24 @@ class DeclarativeTest(DeclarativeTestBase): eq_(sess.query(User).all(), [User(name='u1', address_count=2, addresses=[Address(email='one'), Address(email='two')])]) - def test_useless_declared_attr_warns_on_subclass(self): - def go(): - class MyBase(Base): - __tablename__ = 'foo' - id = Column(Integer, primary_key=True) - @declared_attr - def somecol(cls): - return Column(Integer) + def test_declared_on_base_class(self): + class MyBase(Base): + __tablename__ = 'foo' + id = Column(Integer, primary_key=True) + @declared_attr + def somecol(cls): + return Column(Integer) - class MyClass(MyBase): - __tablename__ = 'bar' - assert_raises_message( - sa.exc.SAWarning, - r"Regular \(i.e. not __special__\) attribute 'MyBase.somecol' " - "uses @declared_attr, but owning class " - " is " - "mapped - not applying to subclass .", - go - ) + class MyClass(MyBase): + __tablename__ = 'bar' + id = Column(Integer, ForeignKey('foo.id'), primary_key=True) + + # previously, the 'somecol' declared_attr would be ignored + # by the mapping and would remain unused. now we take + # it as part of MyBase. + + assert 'somecol' in MyBase.__table__.c + assert 'somecol' not in MyClass.__table__.c def test_column(self): diff --git a/test/ext/declarative/test_inheritance.py b/test/ext/declarative/test_inheritance.py index 3bcd17bb71..7a2cb2deff 100644 --- a/test/ext/declarative/test_inheritance.py +++ b/test/ext/declarative/test_inheritance.py @@ -478,6 +478,66 @@ class DeclarativeInheritanceTest(DeclarativeTestBase): eq_(sess.query(Engineer).filter_by(primary_language='cobol' ).one(), Engineer(name='vlad', primary_language='cobol')) + def test_columns_single_inheritance_conflict_resolution(self): + """Test that a declared_attr can return the existing column and it will + be ignored. this allows conditional columns to be added. + + See [ticket:2472]. + + """ + class Person(Base): + __tablename__ = 'person' + id = Column(Integer, primary_key=True) + + class Engineer(Person): + """single table inheritance""" + + @declared_attr + def target_id(cls): + return cls.__table__.c.get('target_id', + Column(Integer, ForeignKey('other.id')) + ) + @declared_attr + def target(cls): + return relationship("Other") + + class Manager(Person): + """single table inheritance""" + + @declared_attr + def target_id(cls): + return cls.__table__.c.get('target_id', + Column(Integer, ForeignKey('other.id')) + ) + @declared_attr + def target(cls): + return relationship("Other") + + class Other(Base): + __tablename__ = 'other' + id = Column(Integer, primary_key=True) + + is_( + Engineer.target_id.property.columns[0], + Person.__table__.c.target_id + ) + is_( + Manager.target_id.property.columns[0], + Person.__table__.c.target_id + ) + # do a brief round trip on this + Base.metadata.create_all() + session = Session() + o1, o2 = Other(), Other() + session.add_all([ + Engineer(target=o1), + Manager(target=o2), + Manager(target=o1) + ]) + session.commit() + eq_(session.query(Engineer).first().target, o1) + + def test_joined_from_single(self): class Company(Base, fixtures.ComparableEntity): diff --git a/test/ext/declarative/test_mixin.py b/test/ext/declarative/test_mixin.py index 0876ebe63a..a77d6be819 100644 --- a/test/ext/declarative/test_mixin.py +++ b/test/ext/declarative/test_mixin.py @@ -1,5 +1,5 @@ from test.lib.testing import eq_, assert_raises, \ - assert_raises_message + assert_raises_message, is_ from sqlalchemy.ext import declarative as decl import sqlalchemy as sa from test.lib import testing @@ -13,6 +13,8 @@ from sqlalchemy.util import classproperty from sqlalchemy.ext.declarative import declared_attr from test.lib import fixtures +Base = None + class DeclarativeTestBase(fixtures.TestBase, testing.AssertsExecutionResults): def setup(self): global Base @@ -287,6 +289,59 @@ class DeclarativeMixinTest(DeclarativeTestBase): assert len(General.bar.prop.columns) == 1 assert Specific.bar.prop is General.bar.prop + def test_columns_single_inheritance_conflict_resolution(self): + """Test that a declared_attr can return the existing column and it will + be ignored. this allows conditional columns to be added. + + See [ticket:2472]. + + """ + class Person(Base): + __tablename__ = 'person' + id = Column(Integer, primary_key=True) + + class Mixin(object): + @declared_attr + def target_id(cls): + return cls.__table__.c.get('target_id', + Column(Integer, ForeignKey('other.id')) + ) + + @declared_attr + def target(cls): + return relationship("Other") + + class Engineer(Mixin, Person): + """single table inheritance""" + + class Manager(Mixin, Person): + """single table inheritance""" + + class Other(Base): + __tablename__ = 'other' + id = Column(Integer, primary_key=True) + + is_( + Engineer.target_id.property.columns[0], + Person.__table__.c.target_id + ) + is_( + Manager.target_id.property.columns[0], + Person.__table__.c.target_id + ) + # do a brief round trip on this + Base.metadata.create_all() + session = Session() + o1, o2 = Other(), Other() + session.add_all([ + Engineer(target=o1), + Manager(target=o2), + Manager(target=o1) + ]) + session.commit() + eq_(session.query(Engineer).first().target, o1) + + 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. -- 2.47.3