diff options
| -rw-r--r-- | doc/build/changelog/migration_14.rst | 108 | ||||
| -rw-r--r-- | doc/build/changelog/unreleased_14/5122.rst | 19 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/loading.py | 40 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/query.py | 8 | ||||
| -rw-r--r-- | test/orm/inheritance/test_basic.py | 90 | ||||
| -rw-r--r-- | test/orm/inheritance/test_single.py | 111 |
6 files changed, 340 insertions, 36 deletions
diff --git a/doc/build/changelog/migration_14.rst b/doc/build/changelog/migration_14.rst index 63b59841f..c7819a5ae 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 + (<class '__main__.Employee'>, (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 + (<class '__main__.Employee'>, (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 000000000..8a76e597c --- /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 b823de4ab..218449cdd 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 55b569bcd..15319e049 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 ebb485156..831781330 100644 --- a/test/orm/inheritance/test_basic.py +++ b/test/orm/inheritance/test_basic.py @@ -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 ed9c781f4..25349157c 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") |