From: Mike Bayer Date: Tue, 28 Jan 2020 21:44:53 +0000 (-0500) Subject: Raise for unexpected polymorphic identity X-Git-Tag: rel_1_4_0b1~537^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c741b89bd57eb31b7a1bbd187a4d159bdfea5111;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Raise for unexpected polymorphic identity A query that is against an mapped inheritance subclass which also uses :meth:`.Query.select_entity_from` or a similar technique in order to provide an existing subquery to SELECT from, will now raise an error if the given subquery returns entities that do not correspond to the given subclass, that is, they are sibling or superclasses in the same hierarchy. Previously, these would be returned without error. Additionally, if the inheritance mapping is a single-inheritance mapping, the given subquery must apply the appropriate filtering against the polymorphic discriminator column in order to avoid this error; previously, the :class:`.Query` would add this criteria to the outside query however this interferes with some kinds of query that return other kinds of entities as well. Fixes: #5122 Change-Id: I60cf8c1300d5bb279ad99f0f01fefe7e008a159b --- diff --git a/doc/build/changelog/migration_14.rst b/doc/build/changelog/migration_14.rst index 63b59841f1..c7819a5ae6 100644 --- a/doc/build/changelog/migration_14.rst +++ b/doc/build/changelog/migration_14.rst @@ -756,6 +756,114 @@ the cascade settings for a viewonly relationship. :ticket:`4993` :ticket:`4994` +.. _change_5122: + +Stricter behavior when querying inheritance mappings using custom queries +------------------------------------------------------------------------- + +This change applies to the scenario where a joined- or single- table +inheritance subclass entity is being queried, given a completed SELECT subquery +to select from. If the given subquery returns rows that do not correspond to +the requested polymorphic identity or identities, an error is raised. +Previously, this condition would pass silently under joined table inheritance, +returning an invalid subclass, and under single table inheritance, the +:class:`.Query` would be adding additional criteria against the subquery to +limit the results which could inappropriately interfere with the intent of the +query. + +Given the example mapping of ``Employee``, ``Engineer(Employee)``, ``Manager(Employee)``, +in the 1.3 series if we were to emit the following query against a joined +inheritance mapping:: + + s = Session(e) + + s.add_all([Engineer(), Manager()]) + + s.commit() + + print( + s.query(Manager).select_entity_from(s.query(Employee).subquery()).all() + ) + + +The subquery selects both the ``Engineer`` and the ``Manager`` rows, and +even though the outer query is against ``Manager``, we get a non ``Manager`` +object back:: + + SELECT anon_1.type AS anon_1_type, anon_1.id AS anon_1_id + FROM (SELECT employee.type AS type, employee.id AS id + FROM employee) AS anon_1 + 2020-01-29 18:04:13,524 INFO sqlalchemy.engine.base.Engine () + [<__main__.Engineer object at 0x7f7f5b9a9810>, <__main__.Manager object at 0x7f7f5b9a9750>] + +The new behavior is that this condition raises an error:: + + sqlalchemy.exc.InvalidRequestError: Row with identity key + (, (1,), None) can't be loaded into an object; + the polymorphic discriminator column '%(140205120401296 anon)s.type' + refers to mapped class Engineer->engineer, which is not a sub-mapper of + the requested mapped class Manager->manager + +The above error only raises if the primary key columns of that entity are +non-NULL. If there's no primary key for a given entity in a row, no attempt +to construct an entity is made. + +In the case of single inheritance mapping, the change in behavior is slightly +more involved; if ``Engineer`` and ``Manager`` above are mapped with +single table inheritance, in 1.3 the following query would be emitted and +only a ``Manager`` object is returned:: + + SELECT anon_1.type AS anon_1_type, anon_1.id AS anon_1_id + FROM (SELECT employee.type AS type, employee.id AS id + FROM employee) AS anon_1 + WHERE anon_1.type IN (?) + 2020-01-29 18:08:32,975 INFO sqlalchemy.engine.base.Engine ('manager',) + [<__main__.Manager object at 0x7ff1b0200d50>] + +The :class:`.Query` added the "single table inheritance" criteria to the +subquery, editorializing on the intent that was originally set up by it. +This behavior was added in version 1.0 in :ticket:`3891`, and creates a +behavioral inconsistency between "joined" and "single" table inheritance, +and additionally modifies the intent of the given query, which may intend +to return additional rows where the columns that correspond to the inheriting +entity are NULL, which is a valid use case. The behavior is now equivalent +to that of joined table inheritance, where it is assumed that the subquery +returns the correct rows and an error is raised if an unexpected polymorphic +identity is encountered:: + + SELECT anon_1.type AS anon_1_type, anon_1.id AS anon_1_id + FROM (SELECT employee.type AS type, employee.id AS id + FROM employee) AS anon_1 + 2020-01-29 18:13:10,554 INFO sqlalchemy.engine.base.Engine () + Traceback (most recent call last): + # ... + sqlalchemy.exc.InvalidRequestError: Row with identity key + (, (1,), None) can't be loaded into an object; + the polymorphic discriminator column '%(140700085268432 anon)s.type' + refers to mapped class Engineer->employee, which is not a sub-mapper of + the requested mapped class Manager->employee + +The correct adjustment to the situation as presented above which worked on 1.3 +is to adjust the given subquery to correctly filter the rows based on the +discriminator column:: + + print( + s.query(Manager).select_entity_from( + s.query(Employee).filter(Employee.discriminator == 'manager'). + subquery()).all() + ) + + SELECT anon_1.type AS anon_1_type, anon_1.id AS anon_1_id + FROM (SELECT employee.type AS type, employee.id AS id + FROM employee + WHERE employee.type = ?) AS anon_1 + 2020-01-29 18:14:49,770 INFO sqlalchemy.engine.base.Engine ('manager',) + [<__main__.Manager object at 0x7f70e13fca90>] + + +:ticket:`5122` + + New Features - Core ==================== diff --git a/doc/build/changelog/unreleased_14/5122.rst b/doc/build/changelog/unreleased_14/5122.rst new file mode 100644 index 0000000000..8a76e597c4 --- /dev/null +++ b/doc/build/changelog/unreleased_14/5122.rst @@ -0,0 +1,19 @@ +.. change:: + :tags: bug, orm + :tickets: 5122 + + A query that is against a mapped inheritance subclass which also uses + :meth:`.Query.select_entity_from` or a similar technique in order to + provide an existing subquery to SELECT from, will now raise an error if the + given subquery returns entities that do not correspond to the given + subclass, that is, they are sibling or superclasses in the same hierarchy. + Previously, these would be returned without error. Additionally, if the + inheritance mapping is a single-inheritance mapping, the given subquery + must apply the appropriate filtering against the polymorphic discriminator + column in order to avoid this error; previously, the :class:`.Query` would + add this criteria to the outside query however this interferes with some + kinds of query that return other kinds of entities as well. + + .. seealso:: + + :ref:`change_5122` \ No newline at end of file diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index b823de4ab9..218449cdde 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -643,11 +643,9 @@ def _instance_processor( identity_token, ) if not is_not_primary_key(identitykey[1]): - raise sa_exc.InvalidRequestError( - "Row with identity key %s can't be loaded into an " - "object; the polymorphic discriminator column '%s' is " - "NULL" % (identitykey, polymorphic_discriminator) - ) + return identitykey + else: + return None _instance = _decorate_polymorphic_switch( _instance, @@ -843,6 +841,8 @@ def _decorate_polymorphic_switch( else: if sub_mapper is mapper: return None + elif not sub_mapper.isa(mapper): + return False return _instance_processor( sub_mapper, @@ -863,11 +863,37 @@ def _decorate_polymorphic_switch( _instance = polymorphic_instances[discriminator] if _instance: return _instance(row) + elif _instance is False: + identitykey = ensure_no_pk(row) + + if identitykey: + raise sa_exc.InvalidRequestError( + "Row with identity key %s can't be loaded into an " + "object; the polymorphic discriminator column '%s' " + "refers to %s, which is not a sub-mapper of " + "the requested %s" + % ( + identitykey, + polymorphic_on, + mapper.polymorphic_map[discriminator], + mapper, + ) + ) + else: + return None else: return instance_fn(row) else: - ensure_no_pk(row) - return None + identitykey = ensure_no_pk(row) + + if identitykey: + raise sa_exc.InvalidRequestError( + "Row with identity key %s can't be loaded into an " + "object; the polymorphic discriminator column '%s' is " + "NULL" % (identitykey, polymorphic_on) + ) + else: + return None return polymorphic_instance diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index 55b569bcd5..15319e0497 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -260,6 +260,7 @@ class Query(Generative): self._from_obj_alias = sql_util.ColumnAdapter( self._from_obj[0], equivs ) + self._enable_single_crit = False elif ( set_base_alias and len(self._from_obj) == 1 @@ -267,6 +268,7 @@ class Query(Generative): and info.is_aliased_class ): self._from_obj_alias = info._adapter + self._enable_single_crit = False def _reset_polymorphic_adapter(self, mapper): for m2 in mapper._with_polymorphic_mappers: @@ -1379,7 +1381,6 @@ class Query(Generative): ._anonymous_fromclause() ) q = self._from_selectable(fromclause) - q._enable_single_crit = False q._select_from_entity = self._entity_zero() if entities: q._set_entities(entities) @@ -1407,6 +1408,7 @@ class Query(Generative): ): self.__dict__.pop(attr, None) self._set_select_from([fromclause], True) + self._enable_single_crit = False # this enables clause adaptation for non-ORM # expressions. @@ -1875,9 +1877,7 @@ class Query(Generative): self._having = criterion def _set_op(self, expr_fn, *q): - return self._from_selectable( - expr_fn(*([self] + list(q))).subquery() - )._set_enable_single_crit(False) + return self._from_selectable(expr_fn(*([self] + list(q))).subquery()) def union(self, *q): """Produce a UNION of this Query against one or more queries. diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py index 084563972b..831781330b 100644 --- a/test/orm/inheritance/test_basic.py +++ b/test/orm/inheritance/test_basic.py @@ -3384,7 +3384,7 @@ class DiscriminatorOrPkNoneTest(fixtures.DeclarativeMappedTest): eq_(row, (Parent(id=2), None)) def test_pk_not_null_discriminator_null_from_base(self): - A, = self.classes("A") + (A,) = self.classes("A") sess = Session() q = sess.query(A).filter(A.id == 3) @@ -3397,7 +3397,7 @@ class DiscriminatorOrPkNoneTest(fixtures.DeclarativeMappedTest): ) def test_pk_not_null_discriminator_null_from_sub(self): - B, = self.classes("B") + (B,) = self.classes("B") sess = Session() q = sess.query(B).filter(B.id == 4) @@ -3410,6 +3410,96 @@ class DiscriminatorOrPkNoneTest(fixtures.DeclarativeMappedTest): ) +class UnexpectedPolymorphicIdentityTest(fixtures.DeclarativeMappedTest): + run_setup_mappers = "once" + __dialect__ = "default" + + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class AJoined(fixtures.ComparableEntity, Base): + __tablename__ = "ajoined" + id = Column(Integer, primary_key=True) + type = Column(String(10), nullable=False) + __mapper_args__ = { + "polymorphic_on": type, + "polymorphic_identity": "a", + } + + class AJoinedSubA(AJoined): + __tablename__ = "ajoinedsuba" + id = Column(ForeignKey("ajoined.id"), primary_key=True) + __mapper_args__ = {"polymorphic_identity": "suba"} + + class AJoinedSubB(AJoined): + __tablename__ = "ajoinedsubb" + id = Column(ForeignKey("ajoined.id"), primary_key=True) + __mapper_args__ = {"polymorphic_identity": "subb"} + + class ASingle(fixtures.ComparableEntity, Base): + __tablename__ = "asingle" + id = Column(Integer, primary_key=True) + type = Column(String(10), nullable=False) + __mapper_args__ = { + "polymorphic_on": type, + "polymorphic_identity": "a", + } + + class ASingleSubA(ASingle): + __mapper_args__ = {"polymorphic_identity": "suba"} + + class ASingleSubB(ASingle): + __mapper_args__ = {"polymorphic_identity": "subb"} + + @classmethod + def insert_data(cls): + ASingleSubA, ASingleSubB, AJoinedSubA, AJoinedSubB = cls.classes( + "ASingleSubA", "ASingleSubB", "AJoinedSubA", "AJoinedSubB" + ) + s = Session() + + s.add_all([ASingleSubA(), ASingleSubB(), AJoinedSubA(), AJoinedSubB()]) + s.commit() + + def test_single_invalid_ident(self): + ASingle, ASingleSubA = self.classes("ASingle", "ASingleSubA") + + s = Session() + + q = s.query(ASingleSubA).select_entity_from( + select([ASingle]).subquery() + ) + + assert_raises_message( + sa_exc.InvalidRequestError, + r"Row with identity key \(.*ASingle.*\) can't be loaded into an " + r"object; the polymorphic discriminator column '.*.type' refers " + r"to mapped class ASingleSubB->asingle, which is not a " + r"sub-mapper of the requested mapped class ASingleSubA->asingle", + q.all, + ) + + def test_joined_invalid_ident(self): + AJoined, AJoinedSubA = self.classes("AJoined", "AJoinedSubA") + + s = Session() + + q = s.query(AJoinedSubA).select_entity_from( + select([AJoined]).subquery() + ) + + assert_raises_message( + sa_exc.InvalidRequestError, + r"Row with identity key \(.*AJoined.*\) can't be loaded into an " + r"object; the polymorphic discriminator column '.*.type' refers " + r"to mapped class AJoinedSubB->ajoinedsubb, which is not a " + r"sub-mapper of the requested mapped class " + r"AJoinedSubA->ajoinedsuba", + q.all, + ) + + class NameConflictTest(fixtures.MappedTest): @classmethod def define_tables(cls, metadata): diff --git a/test/orm/inheritance/test_single.py b/test/orm/inheritance/test_single.py index ed9c781f41..25349157c1 100644 --- a/test/orm/inheritance/test_single.py +++ b/test/orm/inheritance/test_single.py @@ -1,3 +1,4 @@ +from sqlalchemy import and_ from sqlalchemy import ForeignKey from sqlalchemy import func from sqlalchemy import inspect @@ -76,9 +77,22 @@ class SingleInheritanceTest(testing.AssertsCompiledSQL, fixtures.MappedTest): class JuniorEngineer(Engineer): pass + class Report(cls.Comparable): + pass + @classmethod def setup_mappers(cls): - Employee, Manager, JuniorEngineer, employees, Engineer = ( + ( + Report, + reports, + Employee, + Manager, + JuniorEngineer, + employees, + Engineer, + ) = ( + cls.classes.Report, + cls.tables.reports, cls.classes.Employee, cls.classes.Manager, cls.classes.JuniorEngineer, @@ -86,7 +100,17 @@ class SingleInheritanceTest(testing.AssertsCompiledSQL, fixtures.MappedTest): cls.classes.Engineer, ) - mapper(Employee, employees, polymorphic_on=employees.c.type) + mapper( + Report, reports, properties={"employee": relationship(Employee)} + ) + mapper( + Employee, + employees, + polymorphic_on=employees.c.type, + properties={ + "reports": relationship(Report, back_populates="employee") + }, + ) mapper(Manager, inherits=Employee, polymorphic_identity="manager") mapper(Engineer, inherits=Employee, polymorphic_identity="engineer") mapper( @@ -394,13 +418,68 @@ class SingleInheritanceTest(testing.AssertsCompiledSQL, fixtures.MappedTest): sess.add_all([m1, m2, e1, e2]) sess.flush() + # note test_basic -> UnexpectedPolymorphicIdentityTest as well eq_( sess.query(Manager) - .select_entity_from(employees.select().limit(10).subquery()) + .select_entity_from( + employees.select() + .where(employees.c.type == "manager") + .order_by(employees.c.employee_id) + .limit(10) + .subquery() + ) .all(), [m1, m2], ) + def test_select_from_subquery_with_composed_union(self): + Report, reports, Manager, JuniorEngineer, employees, Engineer = ( + self.classes.Report, + self.tables.reports, + self.classes.Manager, + self.classes.JuniorEngineer, + self.tables.employees, + self.classes.Engineer, + ) + + sess = create_session() + r1, r2, r3, r4 = ( + Report(name="r1"), + Report(name="r2"), + Report(name="r3"), + Report(name="r4"), + ) + m1 = Manager(name="manager1", manager_data="data1", reports=[r1]) + m2 = Manager(name="manager2", manager_data="data2", reports=[r2]) + e1 = Engineer(name="engineer1", engineer_info="einfo1", reports=[r3]) + e2 = JuniorEngineer( + name="engineer2", engineer_info="einfo2", reports=[r4] + ) + sess.add_all([m1, m2, e1, e2]) + sess.flush() + + stmt = ( + select([reports, employees]) + .select_from( + reports.outerjoin( + employees, + and_( + employees.c.employee_id == reports.c.employee_id, + employees.c.type == "manager", + ), + ) + ) + .apply_labels() + .subquery() + ) + eq_( + sess.query(Report, Manager) + .select_entity_from(stmt) + .order_by(Report.name) + .all(), + [(r1, m1), (r2, m2), (r3, None), (r4, None)], + ) + def test_count(self): Employee = self.classes.Employee JuniorEngineer = self.classes.JuniorEngineer @@ -437,21 +516,12 @@ class SingleInheritanceTest(testing.AssertsCompiledSQL, fixtures.MappedTest): ) def test_type_filtering(self): - Employee, Manager, reports, Engineer = ( - self.classes.Employee, + Report, Manager, Engineer = ( + self.classes.Report, self.classes.Manager, - self.tables.reports, self.classes.Engineer, ) - class Report(fixtures.ComparableEntity): - pass - - mapper( - Report, - reports, - properties={"employee": relationship(Employee, backref="reports")}, - ) sess = create_session() m1 = Manager(name="Tom", manager_data="data1") @@ -468,21 +538,12 @@ class SingleInheritanceTest(testing.AssertsCompiledSQL, fixtures.MappedTest): ) def test_type_joins(self): - Employee, Manager, reports, Engineer = ( - self.classes.Employee, + Report, Manager, Engineer = ( + self.classes.Report, self.classes.Manager, - self.tables.reports, self.classes.Engineer, ) - class Report(fixtures.ComparableEntity): - pass - - mapper( - Report, - reports, - properties={"employee": relationship(Employee, backref="reports")}, - ) sess = create_session() m1 = Manager(name="Tom", manager_data="data1")