From: Mike Bayer Date: Mon, 29 Aug 2022 14:43:36 +0000 (-0400) Subject: refine ruleset to determine when poly adaption should be used X-Git-Tag: rel_2_0_0b1~86^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b7f1fb5879606934abb54a8b75e4613339bc8f29;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git refine ruleset to determine when poly adaption should be used Fixed regression appearing in the 1.4 series where a joined-inheritance query placed as a subquery within an enclosing query for that same entity would fail to render the JOIN correctly for the inner query. The issue manifested in two different ways prior and subsequent to version 1.4.18 (related issue #6595), in one case rendering JOIN twice, in the other losing the JOIN entirely. To resolve, the conditions under which "polymorphic loading" are applied have been scaled back to not be invoked for simple joined inheritance queries. Fixes: #8456 Change-Id: Ie4332fadb1dfc670cd31d098a6586a9f6976bcf7 --- diff --git a/doc/build/changelog/unreleased_14/8456.rst b/doc/build/changelog/unreleased_14/8456.rst new file mode 100644 index 0000000000..ca769fd342 --- /dev/null +++ b/doc/build/changelog/unreleased_14/8456.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 8456 + + Fixed regression appearing in the 1.4 series where a joined-inheritance + query placed as a subquery within an enclosing query for that same entity + would fail to render the JOIN correctly for the inner query. The issue + manifested in two different ways prior and subsequent to version 1.4.18 + (related issue #6595), in one case rendering JOIN twice, in the other + losing the JOIN entirely. To resolve, the conditions under which + "polymorphic loading" are applied have been scaled back to not be invoked + for simple joined inheritance queries. diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index b762908c55..ff0cdd6807 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -490,6 +490,10 @@ class ORMCompileState(AbstractORMCompileState): ] def _create_with_polymorphic_adapter(self, ext_info, selectable): + """given MapperEntity or ORMColumnEntity, setup polymorphic loading + if appropriate + + """ if ( not ext_info.is_aliased_class and ext_info.mapper.persist_selectable @@ -2423,14 +2427,7 @@ class _MapperEntity(_QueryEntity): self._with_polymorphic_mappers = ext_info.with_polymorphic_mappers self._polymorphic_discriminator = ext_info.polymorphic_on - if ( - mapper.with_polymorphic - # controversy - only if inheriting mapper is also - # polymorphic? - # or (mapper.inherits and mapper.inherits.with_polymorphic) - or mapper.inherits - or mapper._requires_row_aliasing - ): + if mapper._should_select_with_poly_adapter: compile_state._create_with_polymorphic_adapter( ext_info, self.selectable ) @@ -2923,11 +2920,7 @@ class _ORMColumnEntity(_ColumnEntity): self._extra_entities = (self.expr, self.column) - if ( - mapper.with_polymorphic - or mapper.inherits - or mapper._requires_row_aliasing - ): + if mapper._should_select_with_poly_adapter: compile_state._create_with_polymorphic_adapter( ezero, ezero.selectable ) diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index b4aee545ee..c8df51b068 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -2296,6 +2296,41 @@ class Mapper( else: return None + @HasMemoized.memoized_attribute + def _should_select_with_poly_adapter(self): + """determine if _MapperEntity or _ORMColumnEntity will need to use + polymorphic adaption when setting up a SELECT as well as fetching + rows for mapped classes and subclasses against this Mapper. + + moved here from context.py for #8456 to generalize the ruleset + for this condition. + + """ + + # this has been simplified as of #8456. + # rule is: if we have a with_polymorphic or a concrete-style + # polymorphic selectable, *or* if the base mapper has either of those, + # we turn on the adaption thing. if not, we do *no* adaption. + # + # this splits the behavior among the "regular" joined inheritance + # and single inheritance mappers, vs. the "weird / difficult" + # concrete and joined inh mappings that use a with_polymorphic of + # some kind or polymorphic_union. + # + # note we have some tests in test_polymorphic_rel that query against + # a subclass, then refer to the superclass that has a with_polymorphic + # on it (such as test_join_from_polymorphic_explicit_aliased_three). + # these tests actually adapt the polymorphic selectable (like, the + # UNION or the SELECT subquery with JOIN in it) to be just the simple + # subclass table. Hence even if we are a "plain" inheriting mapper + # but our base has a wpoly on it, we turn on adaption. + return ( + self.with_polymorphic + or self._requires_row_aliasing + or self.base_mapper.with_polymorphic + or self.base_mapper._requires_row_aliasing + ) + @HasMemoized.memoized_attribute def _with_polymorphic_mappers(self) -> Sequence[Mapper[Any]]: self._check_configure() diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 599a12761b..41e598c38e 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -264,6 +264,7 @@ class ColumnLoader(LoaderStrategy): ): # look through list of columns represented here # to see which, if any, is present in the row. + for col in self.columns: if adapter: col = adapter.columns[col] diff --git a/test/orm/inheritance/test_concrete.py b/test/orm/inheritance/test_concrete.py index 1c5540115e..05d1fb2b20 100644 --- a/test/orm/inheritance/test_concrete.py +++ b/test/orm/inheritance/test_concrete.py @@ -198,6 +198,7 @@ class ConcreteTest(AssertsCompiledSQL, fixtures.MappedTest): "Manager Sally knows how to manage things", ] ) + assert set([repr(x) for x in session.query(Manager)]) == set( ["Manager Sally knows how to manage things"] ) @@ -1683,6 +1684,11 @@ class AdaptOnNamesTest(_RemoveListeners, fixtures.DeclarativeMappedTest): "metadata.some_data AS some_data FROM b " "JOIN metadata ON metadata.id = b.metadata_id " "WHERE metadata.id < :id_3) AS anon_1 ORDER BY anon_1.id", + # tip: whether or not there is "id_2" and "id_3" here, + # or just "id_2", is based on whether or not the two + # queries had polymorphic adaption proceed, so that the + # two filter criterias are different vs. the same object. see + # mapper._should_select_with_poly_adapter added in #8456. [{"param_1": "a", "id_2": 3, "param_2": "b", "id_3": 3}], ) ) diff --git a/test/orm/inheritance/test_polymorphic_rel.py b/test/orm/inheritance/test_polymorphic_rel.py index 5b1af266d5..64b4bd9a88 100644 --- a/test/orm/inheritance/test_polymorphic_rel.py +++ b/test/orm/inheritance/test_polymorphic_rel.py @@ -503,6 +503,7 @@ class _PolymorphicTestBase: def test_join_from_polymorphic_explicit_aliased_three(self): sess = fixture_session() pa = aliased(Paperwork) + eq_( sess.query(Engineer) .order_by(Person.person_id) @@ -2063,6 +2064,41 @@ class _PolymorphicTestBase: class PolymorphicTest(_PolymorphicTestBase, _Polymorphic): + def test_joined_aliasing_unrelated_subuqery(self): + """test #8456""" + + inner = select(Engineer).where(Engineer.name == "vlad").subquery() + + crit = select(inner.c.person_id) + + outer = select(Engineer).where(Engineer.person_id.in_(crit)) + + # this query will not work at all for any "polymorphic" case + # as it will adapt the inner query as well. for those cases, + # aliased() has to be used for the inner entity to disambiguate it. + self.assert_compile( + outer, + "SELECT engineers.person_id, people.person_id AS person_id_1, " + "people.company_id, people.name, people.type, engineers.status, " + "engineers.engineer_name, engineers.primary_language " + "FROM people JOIN engineers " + "ON people.person_id = engineers.person_id " + "WHERE engineers.person_id IN " + "(SELECT anon_1.person_id FROM " + "(SELECT engineers.person_id AS person_id, " + "people.person_id AS person_id_1, " + "people.company_id AS company_id, people.name AS name, " + "people.type AS type, engineers.status AS status, " + "engineers.engineer_name AS engineer_name, " + "engineers.primary_language AS primary_language FROM people " + "JOIN engineers ON people.person_id = engineers.person_id " + "WHERE people.name = :name_1) " + "AS anon_1)", + ) + + sess = fixture_session() + eq_(sess.scalars(outer).all(), [Engineer(name="vlad")]) + def test_primary_eager_aliasing_three_dont_reset_selectable(self): """test now related to #7262