1. Michael Bayer
  2. sqlalchemy

Issues

Issue #3042 resolved

ensure warning for implicitly combined columns occurs for inherited classes as well

Michael Bayer
repo owner created an issue

test:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class A(Base):
    __tablename__ = 'a'

    id = Column(Integer, primary_key=True)

class B(A):
    __tablename__ = 'b'

    id = Column(Integer, primary_key=True)
    a_id = Column(Integer, ForeignKey('a.id'))

note that we don't get the "same named columns" warning for A.id and B.id because our test for this is too simplistic, assuming that in an inheritance scenario, a subtable with the same column should implicitly combine with that of a supertable. I'm not sure if I intended this to be this way for all implicitly combined columns that were in an inheritance situation or somehow thought these would always be tables in the sync condition, but these are separate columns and the "same named" error should still apply. We will make it a warning in 0.9 and an error in 1.0 as follows:

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 3e93840..1377945 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1604,13 +1604,17 @@ class Mapper(_InspectionAttr):
         prop = self._props.get(key, None)

         if isinstance(prop, properties.ColumnProperty):
-            if prop.parent is self:
-                raise sa_exc.InvalidRequestError(
-                        "Implicitly combining column %s with column "
-                        "%s under attribute '%s'.  Please configure one "
-                        "or more attributes for these same-named columns "
-                        "explicitly."
-                         % (prop.columns[-1], column, key))
+            if (prop.columns[0], column) not in self._inherits_equated_pairs and \
+                    not prop.columns[0].shares_lineage(column):
+                warn_only = prop.parent is not self
+                msg = ("Implicitly combining column %s with column "
+                      "%s under attribute '%s'.  Please configure one "
+                      "or more attributes for these same-named columns "
+                      "explicitly." % (prop.columns[-1], column, key))
+                if warn_only:
+                    util.warn(msg)
+                else:
+                    raise sa_exc.InvalidRequestError(msg)

             # existing properties.ColumnProperty from an inheriting
             # mapper. make a copy and append our column to it

this is extracted from #3041.

needs tests for this specific case.

patch is attached which modifies quite a lot of tests which suffer from this warning.

Comments (5)

  1. Michael Bayer reporter

    make sure the docs make the approach clear:

    class A(Base):
        __tablename__ = 'a'
    
        id = Column(Integer, primary_key=True)
    
    class B(A):
        __tablename__ = 'b'
    
        id = column_property(Column(Integer, primary_key=True), A.id)
        a_id = Column(Integer, ForeignKey('a.id'))
    
  2. Michael Bayer reporter
    • Additional checks have been added for the case where an inheriting mapper is implicitly combining one of its column-based attributes with that of the parent, where those columns normally don't necessarily share the same value. This is an extension of an existing check that was added via ticket1892; however this new check emits only a warning, instead of an exception, to allow for applications that may be relying upon the existing behavior. fixes #3042

    → <<cset a1bbf3a00567>>

  3. Michael Bayer reporter
    • Additional checks have been added for the case where an inheriting mapper is implicitly combining one of its column-based attributes with that of the parent, where those columns normally don't necessarily share the same value. This is an extension of an existing check that was added via ticket1892; however this new check emits only a warning, instead of an exception, to allow for applications that may be relying upon the existing behavior. fixes #3042

    → <<cset cc3d6e9a4efa>>

  4. Log in to comment