From: Mike Bayer Date: Sun, 27 Jan 2019 00:49:44 +0000 (-0500) Subject: Improve support for with_polymorphic in mapper options X-Git-Tag: rel_1_3_0b3~5^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=95c371f1d3007071a32fad1d67329e6f9d56931b;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Improve support for with_polymorphic in mapper options Improved the behavior of :func:`.orm.with_polymorphic` in conjunction with loader options, in particular wildcard operations as well as :func:`.orm.load_only`. The polymorphic object will be more accurately targeted so that column-level options on the entity will correctly take effect.The issue is a continuation of the same kinds of things fixed in :ticket:`4468`. The path logic when using chained mapper options is improved to be more accurate in terms of the entities being linked in the path; when using :func:`.with_polymorphic`, mapper options against this entity need to specify attributes in terms of the with_polymorphic() object and not against the base mappings. New error conditions are raised which were previously more than likely silenty failures. Fixes: #4469 Change-Id: Ie8d802879663b4ff6f6ac1438c885c06d78ae2a0 --- diff --git a/doc/build/changelog/unreleased_13/4469.rst b/doc/build/changelog/unreleased_13/4469.rst new file mode 100644 index 0000000000..0ef016709f --- /dev/null +++ b/doc/build/changelog/unreleased_13/4469.rst @@ -0,0 +1,11 @@ +.. change:: + :tags: bug, orm + :tickets: 4469 + + Improved the behavior of :func:`.orm.with_polymorphic` in conjunction with + loader options, in particular wildcard operations as well as + :func:`.orm.load_only`. The polymorphic object will be more accurately + targeted so that column-level options on the entity will correctly take + effect.The issue is a continuation of the same kinds of things fixed in + :ticket:`4468`. + diff --git a/lib/sqlalchemy/orm/path_registry.py b/lib/sqlalchemy/orm/path_registry.py index 7d93da2962..4803dbecb9 100644 --- a/lib/sqlalchemy/orm/path_registry.py +++ b/lib/sqlalchemy/orm/path_registry.py @@ -62,14 +62,14 @@ class PathRegistry(object): def set(self, attributes, key, value): log.debug("set '%s' on path '%s' to '%s'", key, self, value) - attributes[(key, self.path)] = value + attributes[(key, self.natural_path)] = value def setdefault(self, attributes, key, value): log.debug("setdefault '%s' on path '%s' to '%s'", key, self, value) - attributes.setdefault((key, self.path), value) + attributes.setdefault((key, self.natural_path), value) def get(self, attributes, key, value=None): - key = (key, self.path) + key = (key, self.natural_path) if key in attributes: return attributes[key] else: @@ -160,7 +160,7 @@ class RootRegistry(PathRegistry): """ - path = () + path = natural_path = () has_entity = False is_aliased_class = False is_root = True @@ -177,6 +177,7 @@ class TokenRegistry(PathRegistry): self.token = token self.parent = parent self.path = parent.path + (token,) + self.natural_path = parent.natural_path + (token,) has_entity = False @@ -186,6 +187,13 @@ class TokenRegistry(PathRegistry): if not self.parent.is_aliased_class and not self.parent.is_root: for ent in self.parent.mapper.iterate_to_root(): yield TokenRegistry(self.parent.parent[ent], self.token) + elif ( + self.parent.is_aliased_class + and self.parent.entity._is_with_polymorphic + ): + yield self + for ent in self.parent.entity._with_polymorphic_entities: + yield TokenRegistry(self.parent.parent[ent], self.token) else: yield self @@ -211,6 +219,7 @@ class PropRegistry(PathRegistry): self.prop = prop self.parent = parent self.path = parent.path + (prop,) + self.natural_path = parent.natural_path + (prop,) self._wildcard_path_loader_key = ( "loader", @@ -255,6 +264,24 @@ class EntityRegistry(PathRegistry, dict): self.is_aliased_class = entity.is_aliased_class self.entity = entity self.path = parent.path + (entity,) + + # the "natural path" is the path that we get when Query is traversing + # from the lead entities into the various relationships; it corresponds + # to the structure of mappers and relationships. when we are given a + # path that comes from loader options, as of 1.3 it can have ac-hoc + # with_polymorphic() and other AliasedInsp objects inside of it, which + # are usually not present in mappings. So here we track both the + # "enhanced" path in self.path and the "natural" path that doesn't + # include those objects so these two traversals can be matched up. + if parent.path and self.is_aliased_class: + if entity.mapper.isa(parent.natural_path[-1].entity): + self.natural_path = parent.natural_path + (entity.mapper,) + else: + self.natural_path = parent.natural_path + ( + parent.natural_path[-1].entity, + ) + else: + self.natural_path = self.path self.entity_path = self @property diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py index ee3b83caa0..8a34771c75 100644 --- a/lib/sqlalchemy/orm/strategy_options.py +++ b/lib/sqlalchemy/orm/strategy_options.py @@ -236,7 +236,9 @@ class Load(Generative, MapperOption): path = path[attr] elif _is_mapped_class(attr): - if not attr.common_parent(path.mapper): + if not orm_util._entity_corresponds_to_use_path_impl( + attr.parent, path[-1] + ): if raiseerr: raise sa_exc.ArgumentError( "Attribute '%s' does not " @@ -247,11 +249,13 @@ class Load(Generative, MapperOption): else: prop = found_property = attr.property - if not prop.parent.common_parent(path.mapper): + if not orm_util._entity_corresponds_to_use_path_impl( + attr.parent, path[-1] + ): if raiseerr: raise sa_exc.ArgumentError( - "Attribute '%s' does not " - "link from element '%s'" % (attr, path.entity) + 'Attribute "%s" does not ' + 'link from element "%s"' % (attr, path.entity) ) else: return None @@ -272,36 +276,15 @@ class Load(Generative, MapperOption): _existing_alias=existing, ) ext_info = inspect(ac) - elif not ext_info.with_polymorphic_mappers: - ext_info = orm_util.AliasedInsp( - ext_info.entity, - ext_info.mapper.base_mapper, - ext_info.selectable, - ext_info.name, - ext_info.with_polymorphic_mappers or [ext_info.mapper], - ext_info.polymorphic_on, - ext_info._base_alias, - ext_info._use_mapper_path, - ext_info._adapt_on_names, - ext_info.represents_outer_join, - ) path.entity_path[prop].set( self.context, "path_with_polymorphic", ext_info ) - # the path here will go into the context dictionary and - # needs to match up to how the class graph is traversed. - # so we can't put an AliasedInsp in the path here, needs - # to be the base mapper. - path = path[prop][ext_info.mapper] - - # but, we need to know what the original of_type() - # argument is for cache key purposes. so....store that too. - # it might be better for "path" to really represent, - # "the path", but trying to keep the impact of the cache - # key feature localized for now + path = path[prop][ext_info] + self._of_type = of_type_info + else: path = path[prop] diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 92dd4c4ecd..f9258895d3 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -589,12 +589,32 @@ class AliasedInsp(InspectionAttr): self.persist_selectable ) = self.local_table = selectable self.name = name - self.with_polymorphic_mappers = with_polymorphic_mappers self.polymorphic_on = polymorphic_on self._base_alias = _base_alias or self self._use_mapper_path = _use_mapper_path self.represents_outer_join = represents_outer_join + if with_polymorphic_mappers: + self._is_with_polymorphic = True + self.with_polymorphic_mappers = with_polymorphic_mappers + self._with_polymorphic_entities = [] + for poly in self.with_polymorphic_mappers: + if poly is not mapper: + ent = AliasedClass( + poly.class_, + selectable, + base_alias=self, + adapt_on_names=adapt_on_names, + use_mapper_path=_use_mapper_path, + ) + + setattr(self.entity, poly.class_.__name__, ent) + self._with_polymorphic_entities.append(ent._aliased_insp) + + else: + self._is_with_polymorphic = False + self.with_polymorphic_mappers = [mapper] + self._adapter = sql_util.ColumnAdapter( selectable, equivalents=mapper._equivalent_columns, @@ -605,20 +625,6 @@ class AliasedInsp(InspectionAttr): self._adapt_on_names = adapt_on_names self._target = mapper.class_ - for poly in self.with_polymorphic_mappers: - if poly is not mapper: - setattr( - self.entity, - poly.class_.__name__, - AliasedClass( - poly.class_, - selectable, - base_alias=self, - adapt_on_names=adapt_on_names, - use_mapper_path=_use_mapper_path, - ), - ) - is_aliased_class = True "always returns True" @@ -718,7 +724,17 @@ class AliasedInsp(InspectionAttr): ) def __str__(self): - return "aliased(%s)" % (self._target.__name__,) + if self._is_with_polymorphic: + return "with_polymorphic(%s, [%s])" % ( + self._target.__name__, + ", ".join( + mp.class_.__name__ + for mp in self.with_polymorphic_mappers + if mp is not self.mapper + ), + ) + else: + return "aliased(%s)" % (self._target.__name__,) inspection._inspects(AliasedClass)(lambda target: target._aliased_insp) @@ -1225,6 +1241,42 @@ def _entity_corresponds_to(given, entity): return entity.common_parent(given) +def _entity_corresponds_to_use_path_impl(given, entity): + """determine if 'given' corresponds to 'entity', in terms + of a path of loader options where a mapped attribute is taken to + be a member of a parent entity. + + e.g.:: + + someoption(A).someoption(A.b) # -> fn(A, A) -> True + someoption(A).someoption(C.d) # -> fn(A, C) -> False + + a1 = aliased(A) + someoption(a1).someoption(A.b) # -> fn(a1, A) -> False + someoption(a1).someoption(a1.b) # -> fn(a1, a1) -> True + + wp = with_polymorphic(A, [A1, A2]) + someoption(wp).someoption(A1.foo) # -> fn(wp, A1) -> False + someoption(wp).someoption(wp.A1.foo) # -> fn(wp, wp.A1) -> True + + + """ + if given.is_aliased_class: + return ( + entity.is_aliased_class + and not entity._use_mapper_path + and given is entity + or given in entity._with_polymorphic_entities + ) + elif not entity.is_aliased_class: + return given.common_parent(entity.mapper) + else: + return ( + entity._use_mapper_path + and given in entity.with_polymorphic_mappers + ) + + def _entity_isa(given, mapper): """determine if 'given' "is a" mapper, in terms of the given would load rows of type 'mapper'. diff --git a/test/aaa_profiling/test_memusage.py b/test/aaa_profiling/test_memusage.py index 36f28c063f..8e7bd57bb4 100644 --- a/test/aaa_profiling/test_memusage.py +++ b/test/aaa_profiling/test_memusage.py @@ -738,12 +738,19 @@ class MemUsageWBackendTest(EnsureZeroed): Column("foo", Integer), Column("bar", Integer), ) - m1 = mapper(A, a) + b = Table( + "b", + metadata, + Column("id", Integer, primary_key=True), + Column("a_id", ForeignKey("a.id")), + ) + m1 = mapper(A, a, properties={"bs": relationship(B)}) + m2 = mapper(B, b) @profile_memory() def go(): ma = sa.inspect(aliased(A)) - m1._path_registry[m1.attrs.foo][ma][m1.attrs.bar] + m1._path_registry[m1.attrs.bs][ma][m1.attrs.bar] go() clear_mappers() diff --git a/test/orm/inheritance/test_relationship.py b/test/orm/inheritance/test_relationship.py index 9db2a5163a..11f945fcd6 100644 --- a/test/orm/inheritance/test_relationship.py +++ b/test/orm/inheritance/test_relationship.py @@ -1971,7 +1971,7 @@ class JoinedloadOverWPolyAliased( q = session.query(poly).options( joinedload(poly.Sub1.links) .joinedload(Link.child.of_type(Sub1)) - .joinedload(poly.Sub1.links) + .joinedload(Sub1.links) ) self.assert_compile( q, @@ -1999,7 +1999,7 @@ class JoinedloadOverWPolyAliased( q = session.query(poly).options( joinedload(poly.Sub1.links, innerjoin=True) .joinedload(Link.child.of_type(Sub1), innerjoin=True) - .joinedload(poly.Sub1.links, innerjoin=True) + .joinedload(Sub1.links, innerjoin=True) ) self.assert_compile( q, diff --git a/test/orm/test_deferred.py b/test/orm/test_deferred.py index d9ac6b0ff5..6b9c1c9182 100644 --- a/test/orm/test_deferred.py +++ b/test/orm/test_deferred.py @@ -1554,6 +1554,77 @@ class InheritanceTest(_Polymorphic): "ORDER BY people.person_id", ) + def test_load_only_from_with_polymorphic(self): + s = Session() + + wp = with_polymorphic(Person, [Manager], flat=True) + + assert_raises_message( + sa.exc.ArgumentError, + 'Mapped attribute "Manager.status" does not apply to any of the ' + "root entities in this query, e.g. " + r"with_polymorphic\(Person, \[Manager\]\).", + s.query(wp).options, + load_only(Manager.status), + ) + + q = s.query(wp).options(load_only(wp.Manager.status)) + self.assert_compile( + q, + "SELECT people_1.person_id AS people_1_person_id, " + "people_1.type AS people_1_type, " + "managers_1.person_id AS managers_1_person_id, " + "managers_1.status AS managers_1_status " + "FROM people AS people_1 " + "LEFT OUTER JOIN managers AS managers_1 " + "ON people_1.person_id = managers_1.person_id", + ) + + def test_load_only_of_type_with_polymorphic(self): + s = Session() + + wp = with_polymorphic(Person, [Manager], flat=True) + + # needs to be explicit, we don't currently dig onto all the + # sub-entities in the wp + assert_raises_message( + sa.exc.ArgumentError, + r'Can\'t find property named "status" on ' + r"with_polymorphic\(Person, \[Manager\]\) in this Query.", + s.query(Company).options, + joinedload(Company.employees.of_type(wp)).load_only("status"), + ) + + assert_raises_message( + sa.exc.ArgumentError, + 'Attribute "Manager.status" does not link from element ' + r'"with_polymorphic\(Person, \[Manager\]\)"', + s.query(Company).options, + joinedload(Company.employees.of_type(wp)).load_only( + Manager.status + ), + ) + + self.assert_compile( + s.query(Company).options( + joinedload(Company.employees.of_type(wp)).load_only( + wp.Manager.status + ) + ), + # should at least not have manager_name in it + "SELECT companies.company_id AS companies_company_id, " + "companies.name AS companies_name, " + "people_1.person_id AS people_1_person_id, " + "people_1.type AS people_1_type, " + "managers_1.person_id AS managers_1_person_id, " + "managers_1.status AS managers_1_status " + "FROM companies LEFT OUTER JOIN " + "(people AS people_1 LEFT OUTER JOIN managers AS managers_1 " + "ON people_1.person_id = managers_1.person_id) " + "ON companies.company_id = people_1.company_id " + "ORDER BY people_1.person_id", + ) + class WithExpressionTest(fixtures.DeclarativeMappedTest): @classmethod diff --git a/test/orm/test_options.py b/test/orm/test_options.py index b4953cd3bc..e382a80971 100644 --- a/test/orm/test_options.py +++ b/test/orm/test_options.py @@ -21,10 +21,16 @@ from sqlalchemy.orm import Session from sqlalchemy.orm import strategy_options from sqlalchemy.orm import subqueryload from sqlalchemy.orm import util as orm_util +from sqlalchemy.orm import with_polymorphic from sqlalchemy.testing import fixtures from sqlalchemy.testing.assertions import assert_raises_message from sqlalchemy.testing.assertions import eq_ from test.orm import _fixtures +from .inheritance._poly_fixtures import _Polymorphic +from .inheritance._poly_fixtures import Company +from .inheritance._poly_fixtures import Engineer +from .inheritance._poly_fixtures import Manager +from .inheritance._poly_fixtures import Person class QueryTest(_fixtures.FixtureTest): @@ -1239,6 +1245,80 @@ class OptionsNoPropTest(_fixtures.FixtureTest): ) +class OptionsNoPropTestInh(_Polymorphic): + def test_missing_attr_wpoly_subclasss(self): + s = Session() + + wp = with_polymorphic(Person, [Manager], flat=True) + + assert_raises_message( + sa.exc.ArgumentError, + r'Mapped attribute "Manager.status" does not apply to any of ' + r"the root entities in this query, e.g. " + r"with_polymorphic\(Person, \[Manager\]\).", + s.query(wp).options, + load_only(Manager.status), + ) + + def test_missing_attr_of_type_subclass(self): + s = Session() + + assert_raises_message( + sa.exc.ArgumentError, + r'Attribute "Manager.manager_name" does not link from element ' + r'"with_polymorphic\(Person, \[Engineer\]\)"', + s.query(Company).options, + joinedload(Company.employees.of_type(Engineer)).load_only( + Manager.manager_name + ), + ) + + def test_missing_attr_of_type_subclass_name_matches(self): + s = Session() + + # the name "status" is present on Engineer also, make sure + # that doesn't get mixed up here + assert_raises_message( + sa.exc.ArgumentError, + r'Attribute "Manager.status" does not link from element ' + r'"with_polymorphic\(Person, \[Engineer\]\)"', + s.query(Company).options, + joinedload(Company.employees.of_type(Engineer)).load_only( + Manager.status + ), + ) + + def test_missing_str_attr_of_type_subclass(self): + s = Session() + + wp = with_polymorphic(Person, [Manager], flat=True) + + assert_raises_message( + sa.exc.ArgumentError, + r'Can\'t find property named "manager_name" on ' + "mapped class Engineer->engineers in this Query.", + s.query(Company).options, + joinedload(Company.employees.of_type(Engineer)).load_only( + "manager_name" + ), + ) + + def test_missing_attr_of_type_wpoly_subclass(self): + s = Session() + + wp = with_polymorphic(Person, [Manager], flat=True) + + assert_raises_message( + sa.exc.ArgumentError, + r'Attribute "Manager.manager_name" does not link from ' + r'element "with_polymorphic\(Person, \[Manager\]\)"', + s.query(Company).options, + joinedload(Company.employees.of_type(wp)).load_only( + Manager.manager_name + ), + ) + + class PickleTest(PathTest, QueryTest): def _option_fixture(self, *arg): return strategy_options._UnboundLoad._from_keys(