From: Mike Bayer Date: Sat, 28 Oct 2017 17:28:58 +0000 (-0400) Subject: Filter attributes we don't map during a load_scalar_attributes X-Git-Tag: rel_1_1_15~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e06bf169e5fdac56e074592a4e154cef022bf757;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Filter attributes we don't map during a load_scalar_attributes Fixed bug where a descriptor that is elsewhere a mapped column or relationship within a hierarchy based on :class:`.AbstractConcreteBase` would be referred towards during a refresh operation, causing an error as the attribute is not mapped as a mapper property. A similar issue can arise for other attributes like the "type" column added by :class:`.AbstractConcreteBase` if the class fails to include "concrete=True" in its mapper, however the check here should also prevent that scenario from causing a problem. Change-Id: I407b07a3a3e2c374da19fc86ed44b987d595dcfa Fixes: #4124 (cherry picked from commit fd4289c5829d6498495ac59fe1dccb23b4975281) --- diff --git a/doc/build/changelog/unreleased_11/4124.rst b/doc/build/changelog/unreleased_11/4124.rst new file mode 100644 index 0000000000..27ad67be6e --- /dev/null +++ b/doc/build/changelog/unreleased_11/4124.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, orm, declarative + :tickets: 4124 + :versions: 1.2.0b4 + + Fixed bug where a descriptor that is elsewhere a mapped column + or relationship within a hierarchy based on :class:`.AbstractConcreteBase` + would be referred towards during a refresh operation, causing an error + as the attribute is not mapped as a mapper property. + A similar issue can arise for other attributes like the "type" column + added by :class:`.AbstractConcreteBase` if the class fails to include + "concrete=True" in its mapper, however the check here should also + prevent that scenario from causing a problem. diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index 6f0c860cc5..fe9f9e21a8 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -662,6 +662,15 @@ def load_scalar_attributes(mapper, state, attribute_names): result = False + # in the case of inheritance, particularly concrete and abstract + # concrete inheritance, the class manager might have some keys + # of attributes on the superclass that we didn't actually map. + # These could be mapped as "concrete, dont load" or could be completely + # exluded from the mapping and we know nothing about them. Filter them + # here to prevent them from coming through. + if attribute_names: + attribute_names = attribute_names.intersection(mapper.attrs.keys()) + if mapper.inherits and not mapper.concrete: # because we are using Core to produce a select() that we # pass to the Query, we aren't calling setup() for mapped diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 83f8eca1ce..14debb0c83 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -1098,6 +1098,7 @@ class Mapper(InspectionAttr): self.inherits._inheriting_mappers.append(self) self.passive_updates = self.inherits.passive_updates self._all_tables = self.inherits._all_tables + for key, prop in mapper._props.items(): if key not in self._props and \ not self._should_exclude(key, key, local=False, diff --git a/test/ext/declarative/test_inheritance.py b/test/ext/declarative/test_inheritance.py index e57320cfe4..7ced8ec157 100644 --- a/test/ext/declarative/test_inheritance.py +++ b/test/ext/declarative/test_inheritance.py @@ -1092,6 +1092,10 @@ class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): eq_(sess.query(Manager).all(), [Manager(name='dogbert')]) eq_(sess.query(Boss).all(), [Boss(name='pointy haired')]) + e1 = sess.query(Engineer).order_by(Engineer.name).first() + sess.expire(e1) + eq_(e1.name, 'dilbert') + def test_explicit(self): engineers = Table( 'engineers', Base.metadata, @@ -1213,6 +1217,55 @@ class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): self._roundtrip(Employee, Manager, Engineer, Boss) + def test_abstract_concrete_extension_descriptor_refresh(self): + class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): + @declared_attr + def name(cls): + return Column(String(50)) + + class Manager(Employee): + __tablename__ = 'manager' + employee_id = Column(Integer, primary_key=True, + test_needs_autoincrement=True) + paperwork = Column(String(10)) + __mapper_args__ = { + 'polymorphic_identity': 'manager', 'concrete': True} + + class Engineer(Employee): + __tablename__ = 'engineer' + employee_id = Column(Integer, primary_key=True, + test_needs_autoincrement=True) + + @property + def paperwork(self): + return "p" + + __mapper_args__ = { + 'polymorphic_identity': 'engineer', 'concrete': True} + + Base.metadata.create_all() + sess = Session() + sess.add(Engineer(name='d')) + sess.commit() + + # paperwork is excluded because there's a descritor; so it is + # not in the Engineers mapped properties at all, though is inside the + # class manager. Maybe it shouldn't be in the class manager either. + assert 'paperwork' in Engineer.__mapper__.class_manager + assert 'paperwork' not in Engineer.__mapper__.attrs.keys() + + # type currently does get mapped, as a + # ConcreteInheritedProperty, which means, "ignore this thing inherited + # from the concrete base". if we didn't specify concrete=True, then + # this one gets stuck in the error condition also. + assert 'type' in Engineer.__mapper__.class_manager + assert 'type' in Engineer.__mapper__.attrs.keys() + + e1 = sess.query(Engineer).first() + eq_(e1.name, 'd') + sess.expire(e1) + eq_(e1.name, 'd') + def test_concrete_extension(self): class Employee(ConcreteBase, Base, fixtures.ComparableEntity): __tablename__ = 'employee'