From: Tom Manderson <me@trm.io>
Date: Tue, 30 Oct 2018 17:05:43 +0000 (-0400)
Subject: Move pk on single-inh subclass check below conflict resolution check
X-Git-Tag: rel_1_3_0b1~33^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3ed79a5c18c14d842280805d1dae8a9c99ec8f18;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Move pk on single-inh subclass check below conflict resolution check

The column conflict resolution technique discussed at
:ref:`declarative_column_conflicts` is now functional for a :class:`.Column`
that is also a primary key column.  Previously, a check for primary key
columns declared on a single-inheritance subclass would occur before the
column copy were allowed to pass.

Fixes: #4352
Change-Id: Id4c025da53c28e58db6b549fe398f25f8a90d355
Pull-request: https://github.com/zzzeek/sqlalchemy/pull/483
---

diff --git a/doc/build/changelog/unreleased_12/4352.rst b/doc/build/changelog/unreleased_12/4352.rst
new file mode 100644
index 0000000000..7db01f6f64
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4352.rst
@@ -0,0 +1,10 @@
+.. change::
+   :tags: bug, orm, declarative
+   :tickets: 4352
+
+   The column conflict resolution technique discussed at
+   :ref:`declarative_column_conflicts` is now functional for a :class:`.Column`
+   that is also a primary key column.  Previously, a check for primary key
+   columns declared on a single-inheritance subclass would occur before the
+   column copy were allowed to pass.
+
diff --git a/doc/build/orm/extensions/declarative/inheritance.rst b/doc/build/orm/extensions/declarative/inheritance.rst
index 25ce986b7d..6ab3e10e71 100644
--- a/doc/build/orm/extensions/declarative/inheritance.rst
+++ b/doc/build/orm/extensions/declarative/inheritance.rst
@@ -88,6 +88,8 @@ 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__``.
 
+.. _declarative_column_conflicts:
+
 Resolving Column Conflicts
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index a6642364dc..e5bc670b3b 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -534,11 +534,6 @@ class _MapperConfig(object):
                     )
                 # add any columns declared here to the inherited table.
                 for c in declared_columns:
-                    if c.primary_key:
-                        raise exc.ArgumentError(
-                            "Can't place primary key columns on an inherited "
-                            "class with no table."
-                        )
                     if c.name in inherited_table.c:
                         if inherited_table.c[c.name] is c:
                             continue
@@ -547,6 +542,11 @@ class _MapperConfig(object):
                             "existing column '%s'" %
                             (c, cls, inherited_table.c[c.name])
                         )
+                    if c.primary_key:
+                        raise exc.ArgumentError(
+                            "Can't place primary key columns on an inherited "
+                            "class with no table."
+                        )
                     inherited_table.append_column(c)
                     if inherited_mapped_table is not None and \
                             inherited_mapped_table is not inherited_table:
diff --git a/test/ext/declarative/test_inheritance.py b/test/ext/declarative/test_inheritance.py
index c310b725f4..a453bc165c 100644
--- a/test/ext/declarative/test_inheritance.py
+++ b/test/ext/declarative/test_inheritance.py
@@ -762,6 +762,75 @@ class DeclarativeInheritanceTest(DeclarativeTestBase):
         session.commit()
         eq_(session.query(Engineer).first().target, o1)
 
+    def test_columns_single_inheritance_conflict_resolution_pk(self):
+        """Test #2472 in terms of a primary key column.  This is
+        #4352.
+
+        """
+        class Person(Base):
+            __tablename__ = 'person'
+            id = Column(Integer, primary_key=True)
+
+            target_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, primary_key=True))
+
+        class Manager(Person):
+
+            """single table inheritance"""
+
+            @declared_attr
+            def target_id(cls):
+                return cls.__table__.c.get(
+                    'target_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
+        )
+
+    def test_columns_single_inheritance_cascading_resolution_pk(self):
+        """An additional test for #4352 in terms of the requested use case.
+
+        """
+        class TestBase(Base):
+            __abstract__ = True
+
+            @declared_attr.cascading
+            def id(cls):
+                col_val = None
+                if TestBase not in cls.__bases__:
+                    col_val = cls.__table__.c.get('id')
+                if col_val is None:
+                    col_val = Column(Integer, primary_key=True)
+                return col_val
+
+        class Person(TestBase):
+            """single table base class"""
+            __tablename__ = 'person'
+
+        class Engineer(Person):
+            """ single table inheritance, no extra cols """
+
+        class Manager(Person):
+            """ single table inheritance, no extra cols """
+
+        is_(Engineer.id.property.columns[0], Person.__table__.c.id)
+        is_(Manager.id.property.columns[0], Person.__table__.c.id)
+
     def test_joined_from_single(self):
 
         class Company(Base, fixtures.ComparableEntity):