From becab22dcbe9d68b0671a9246e022c9810f7e319 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 21 Mar 2020 17:26:24 -0400 Subject: [PATCH] Test instance for matching class hierarchy on get_from_identity Fixed issue where a lazyload that uses session-local "get" against a target many-to-one relationship where an object with the correct primary key is present, however it's an instance of a sibling class, does not correctly return None as is the case when the lazy loader actually emits a load for that row. Fixes: #5210 Change-Id: I89f9946cfeba61d89a272435f76a5a082b1da30c --- doc/build/changelog/unreleased_13/5210.rst | 9 +++ lib/sqlalchemy/orm/attributes.py | 1 + lib/sqlalchemy/orm/base.py | 7 +++ lib/sqlalchemy/orm/loading.py | 5 +- lib/sqlalchemy/orm/query.py | 2 + lib/sqlalchemy/orm/session.py | 2 +- lib/sqlalchemy/orm/strategies.py | 5 +- test/orm/inheritance/test_relationship.py | 73 ++++++++++++++++++++++ 8 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/5210.rst diff --git a/doc/build/changelog/unreleased_13/5210.rst b/doc/build/changelog/unreleased_13/5210.rst new file mode 100644 index 0000000000..0a50ba04e9 --- /dev/null +++ b/doc/build/changelog/unreleased_13/5210.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 5210 + + Fixed issue where a lazyload that uses session-local "get" against a target + many-to-one relationship where an object with the correct primary key is + present, however it's an instance of a sibling class, does not correctly + return None as is the case when the lazy loader actually emits a load for + that row. diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index a959b0a400..5ca2858e90 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -34,6 +34,7 @@ from .base import NO_CHANGE # noqa from .base import NO_RAISE from .base import NO_VALUE from .base import NON_PERSISTENT_OK # noqa +from .base import PASSIVE_CLASS_MISMATCH # noqa from .base import PASSIVE_NO_FETCH from .base import PASSIVE_NO_FETCH_RELATED # noqa from .base import PASSIVE_NO_INITIALIZE diff --git a/lib/sqlalchemy/orm/base.py b/lib/sqlalchemy/orm/base.py index a31745aec2..6df8acf1ee 100644 --- a/lib/sqlalchemy/orm/base.py +++ b/lib/sqlalchemy/orm/base.py @@ -25,6 +25,13 @@ PASSIVE_NO_RESULT = util.symbol( """, ) +PASSIVE_CLASS_MISMATCH = util.symbol( + "PASSIVE_CLASS_MISMATCH", + """Symbol indicating that an object is locally present for a given + primary key identity but it is not of the requested class. The + return value is therefore None and no SQL should be emitted.""", +) + ATTR_WAS_SET = util.symbol( "ATTR_WAS_SET", """Symbol returned by a loader callable to indicate the diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index c3d4773cd3..49c71e5b2c 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -161,7 +161,7 @@ def merge_result(query, iterator, load=True): session.autoflush = autoflush -def get_from_identity(session, key, passive): +def get_from_identity(session, mapper, key, passive): """Look up the given key in the given session's identity map, check the object for expired state if found. @@ -171,6 +171,9 @@ def get_from_identity(session, key, passive): state = attributes.instance_state(instance) + if mapper.inherits and not state.mapper.isa(mapper): + return attributes.PASSIVE_CLASS_MISMATCH + # expired - ensure it still exists if state.expired: if not passive & attributes.SQL_OK: diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index fe1ea9bfcd..0da7d08a4a 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -1048,6 +1048,8 @@ class Query(Generative): if not issubclass(instance.__class__, mapper.class_): return None return instance + elif instance is attributes.PASSIVE_CLASS_MISMATCH: + return None return db_load_fn(self, primary_key_identity) diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index f172649ba2..fefdd4ef1a 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1617,7 +1617,7 @@ class Session(_SessionClassMethods): key = mapper.identity_key_from_primary_key( primary_key_identity, identity_token=identity_token ) - return loading.get_from_identity(self, key, passive) + return loading.get_from_identity(self, mapper, key, passive) @property @util.contextmanager diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 7d05a37f5c..0946370821 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -774,7 +774,10 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots): ) if instance is not None: - return instance + if instance is attributes.PASSIVE_CLASS_MISMATCH: + return None + else: + return instance elif ( not passive & attributes.SQL_OK or not passive & attributes.RELATED_OBJECT_OK diff --git a/test/orm/inheritance/test_relationship.py b/test/orm/inheritance/test_relationship.py index 9bbee1e2c8..b7d49829e0 100644 --- a/test/orm/inheritance/test_relationship.py +++ b/test/orm/inheritance/test_relationship.py @@ -19,6 +19,7 @@ from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ +from sqlalchemy.testing.entities import ComparableEntity from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table @@ -2682,3 +2683,75 @@ class BetweenSubclassJoinWExtraJoinedLoad( "seen AS seen_1 ON people.id = seen_1.id LEFT OUTER JOIN " "seen AS seen_2 ON people_1.id = seen_2.id", ) + + +class M2ODontLoadSiblingTest(fixtures.DeclarativeMappedTest): + """test for #5210""" + + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class Parent(Base, ComparableEntity): + __tablename__ = "parents" + + id = Column(Integer, primary_key=True) + child_type = Column(String(50), nullable=False) + + __mapper_args__ = { + "polymorphic_on": child_type, + } + + class Child1(Parent): + __tablename__ = "children_1" + + id = Column(Integer, ForeignKey(Parent.id), primary_key=True) + + __mapper_args__ = { + "polymorphic_identity": "child1", + } + + class Child2(Parent): + __tablename__ = "children_2" + + id = Column(Integer, ForeignKey(Parent.id), primary_key=True) + + __mapper_args__ = { + "polymorphic_identity": "child2", + } + + class Other(Base): + __tablename__ = "others" + + id = Column(Integer, primary_key=True) + parent_id = Column(Integer, ForeignKey(Parent.id)) + + parent = relationship(Parent) + child2 = relationship(Child2, viewonly=True) + + @classmethod + def insert_data(cls): + Other, Child1 = cls.classes("Other", "Child1") + s = Session() + obj = Other(parent=Child1()) + s.add(obj) + s.commit() + + def test_load_m2o_emit_query(self): + Other, Child1 = self.classes("Other", "Child1") + s = Session() + + obj = s.query(Other).first() + + is_(obj.child2, None) + eq_(obj.parent, Child1()) + + def test_load_m2o_use_get(self): + Other, Child1 = self.classes("Other", "Child1") + s = Session() + + obj = s.query(Other).first() + c1 = s.query(Child1).first() + + is_(obj.child2, None) + is_(obj.parent, c1) -- 2.47.3