From: Mike Bayer Date: Tue, 16 Mar 2021 21:59:44 +0000 (-0400) Subject: turn off eager configure_mappers() outside of Query, Load X-Git-Tag: rel_1_4_1~9^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=547ac69d9dab78af9a7ccd71ee55102f344065f1;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git turn off eager configure_mappers() outside of Query, Load Fixed regression where producing a Core expression construct such as :func:`_sql.select` using ORM entities would eagerly configure the mappers, in an effort to maintain compatibility with the :class:`_orm.Query` object which necessarily does this to support many backref-related legacy cases. However, core :func:`_sql.select` constructs are also used in mapper configurations and such, and to that degree this eager configuration is more of an inconvenience, so eager configure has been disabled for the :func:`_sql.select` and other Core constructs in the absence of ORM loading types of functions such as :class:`_orm.Load`. The change maintains the behavior of :class:`_orm.Query` so that backwards compatibility is maintained. However, when using a :func:`_sql.select` in conjunction with ORM entities, a "backref" that isn't explicitly placed on one of the classes until mapper configure time won't be available unless :func:`_orm.configure_mappers` or the newer :func:`_orm.registry.configure` has been called elsewhere. Prefer using :paramref:`_orm.relationship.back_populates` for more explicit relationship configuration which does not have the eager configure requirement. Fixes: #6066 Change-Id: I7a953ddcc189471fbac63c97c51ab8956f64012e --- diff --git a/doc/build/changelog/unreleased_14/6066.rst b/doc/build/changelog/unreleased_14/6066.rst new file mode 100644 index 0000000000..e2eba5a132 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6066.rst @@ -0,0 +1,23 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 6066 + + Fixed regression where producing a Core expression construct such as + :func:`_sql.select` using ORM entities would eagerly configure the mappers, + in an effort to maintain compatibility with the :class:`_orm.Query` object + which necessarily does this to support many backref-related legacy cases. + However, core :func:`_sql.select` constructs are also used in mapper + configurations and such, and to that degree this eager configuration is + more of an inconvenience, so eager configure has been disabled for the + :func:`_sql.select` and other Core constructs in the absence of ORM loading + types of functions such as :class:`_orm.Load`. + + The change maintains the behavior of :class:`_orm.Query` so that backwards + compatibility is maintained. However, when using a :func:`_sql.select` in + conjunction with ORM entities, a "backref" that isn't explicitly placed on + one of the classes until mapper configure time won't be available unless + :func:`_orm.configure_mappers` or the newer :func:`_orm.registry.configure` + has been called elsewhere. Prefer using + :paramref:`_orm.relationship.back_populates` for more explicit relationship + configuration which does not have the eager configure requirement. + diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index 4e2b4cdebb..bbad6b05a2 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -179,7 +179,10 @@ class Query( def _set_entities(self, entities): self._raw_columns = [ coercions.expect( - roles.ColumnsClauseRole, ent, apply_propagate_attrs=self + roles.ColumnsClauseRole, + ent, + apply_propagate_attrs=self, + post_inspect=True, ) for ent in util.to_list(entities) ] @@ -1415,7 +1418,10 @@ class Query( self._raw_columns.extend( coercions.expect( - roles.ColumnsClauseRole, c, apply_propagate_attrs=self + roles.ColumnsClauseRole, + c, + apply_propagate_attrs=self, + post_inspect=True, ) for c in column ) @@ -3217,7 +3223,10 @@ class FromStatement(GroupedElement, SelectBase, Executable): def __init__(self, entities, element): self._raw_columns = [ coercions.expect( - roles.ColumnsClauseRole, ent, apply_propagate_attrs=self + roles.ColumnsClauseRole, + ent, + apply_propagate_attrs=self, + post_inspect=True, ) for ent in util.to_list(entities) ] diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py index f24f42a6b3..932c4a37d0 100644 --- a/lib/sqlalchemy/orm/strategy_options.py +++ b/lib/sqlalchemy/orm/strategy_options.py @@ -89,6 +89,8 @@ class Load(Generative, LoaderOption): def __init__(self, entity): insp = inspect(entity) + insp._post_inspect + self.path = insp._path_registry # note that this .context is shared among all descendant # Load objects diff --git a/lib/sqlalchemy/sql/coercions.py b/lib/sqlalchemy/sql/coercions.py index 8c7963205f..76ba7e2146 100644 --- a/lib/sqlalchemy/sql/coercions.py +++ b/lib/sqlalchemy/sql/coercions.py @@ -109,7 +109,14 @@ def _expression_collection_was_a_list(attrname, fnname, args): return args -def expect(role, element, apply_propagate_attrs=None, argname=None, **kw): +def expect( + role, + element, + apply_propagate_attrs=None, + argname=None, + post_inspect=False, + **kw +): if ( role.allows_lambda # note callable() will not invoke a __getattr__() method, whereas @@ -157,7 +164,8 @@ def expect(role, element, apply_propagate_attrs=None, argname=None, **kw): if impl._use_inspection: insp = inspection.inspect(element, raiseerr=False) if insp is not None: - insp._post_inspect + if post_inspect: + insp._post_inspect try: resolved = insp.__clause_element__() except AttributeError: diff --git a/test/orm/test_events.py b/test/orm/test_events.py index 6ba94096d1..cd8adf8e35 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -999,8 +999,7 @@ class MapperEventsTest(_RemoveListeners, _fixtures.FixtureTest): event.listen(mapper, "before_configured", m1) event.listen(mapper, "after_configured", m2) - s = fixture_session() - s.query(User) + inspect(User)._post_inspect eq_(m1.mock_calls, [call()]) eq_(m2.mock_calls, [call()]) @@ -1132,8 +1131,7 @@ class MapperEventsTest(_RemoveListeners, _fixtures.FixtureTest): # fails by default because Mammal needs to be configured, and cannot # be: def probe(): - s = fixture_session() - s.query(User) + inspect(User)._post_inspect if create_dependency: assert_raises(sa.exc.InvalidRequestError, probe) diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py index ba4cf26373..e4c89c7e8a 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -21,6 +21,8 @@ from sqlalchemy.orm import composite from sqlalchemy.orm import configure_mappers from sqlalchemy.orm import deferred from sqlalchemy.orm import dynamic_loader +from sqlalchemy.orm import Load +from sqlalchemy.orm import load_only from sqlalchemy.orm import mapper from sqlalchemy.orm import reconstructor from sqlalchemy.orm import registry @@ -2998,3 +3000,136 @@ class RegistryConfigDisposeTest(fixtures.TestBase): "pass cascade=True to clear these also", ): reg3.dispose() + + +class ConfigureOrNotConfigureTest(_fixtures.FixtureTest, AssertsCompiledSQL): + __dialect__ = "default" + + @testing.combinations((True,), (False,)) + def test_no_mapper_configure_w_selects_etc(self, use_legacy_query): + Address, addresses, users, User = ( + self.classes.Address, + self.tables.addresses, + self.tables.users, + self.classes.User, + ) + + am = self.mapper(Address, addresses) + + um = self.mapper( + User, + users, + properties={ + "address_count": column_property( + select(Address) + .where(Address.id == users.c.id) + .correlate_except(Address) + .scalar_subquery() + ) + }, + ) + + is_false(am.configured) + is_false(um.configured) + + if use_legacy_query: + stmt = Session().query(User).filter(User.name == "ed") + self.assert_compile( + stmt, + "SELECT (SELECT addresses.id, addresses.user_id, " + "addresses.email_address FROM addresses " + "WHERE addresses.id = users.id) AS anon_1, " + "users.id AS users_id, users.name AS users_name " + "FROM users WHERE users.name = :name_1", + ) + else: + stmt = select(User).where(User.name == "ed") + + self.assert_compile( + stmt, + "SELECT (SELECT addresses.id, addresses.user_id, " + "addresses.email_address FROM addresses " + "WHERE addresses.id = users.id) AS anon_1, " + "users.id, users.name " + "FROM users WHERE users.name = :name_1", + ) + + is_true(am.configured) + is_true(um.configured) + + @testing.combinations((True,), (False,)) + def test_load_options(self, use_bound): + User = self.classes.User + + users = self.tables.users + + um = mapper(User, users) + + if use_bound: + stmt = select(User).options( + Load(User).load_only("name"), + ) + + is_true(um.configured) + else: + stmt = select(User).options( + load_only("name"), + ) + is_false(um.configured) + + self.assert_compile( + stmt, + "SELECT users.id, " "users.name " "FROM users", + ) + is_true(um.configured) + + @testing.combinations((True,), (False,)) + def test_backrefs(self, use_legacy_query): + User, Address = self.classes("User", "Address") + users, addresses = self.tables("users", "addresses") + + mapper( + User, + users, + properties={"addresses": relationship(Address, backref="user")}, + ) + am = mapper(Address, addresses) + + if use_legacy_query: + s = Session() + + # legacy, Query still forces configure + stmt = s.query(Address).join(Address.user) + + is_true(am.configured) + + self.assert_compile( + stmt, + "SELECT addresses.id AS addresses_id, " + "addresses.user_id AS addresses_user_id, " + "addresses.email_address AS addresses_email_address " + "FROM addresses JOIN users ON users.id = addresses.user_id", + ) + else: + # new queries, they can't, because they are used in mapper + # config also. backrefs that aren't explicit on the class + # are the only thing we can't do. we would need __getattr__ + # to intercept this error. + with expect_raises_message( + AttributeError, "type object 'Address' has no attribute 'user'" + ): + stmt = select(Address).join(Address.user) + + is_false(am.configured) + + configure_mappers() + + is_true(am.configured) + + stmt = select(Address).join(Address.user) + self.assert_compile( + stmt, + "SELECT addresses.id, addresses.user_id, " + "addresses.email_address FROM addresses JOIN users " + "ON users.id = addresses.user_id", + )