From fa5522547150687c9b3cd41d28df08ab0512b5b2 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 28 Aug 2014 17:57:48 -0400 Subject: [PATCH] - Made a small adjustment to the mechanics of lazy loading, such that it has less chance of interfering with a joinload() in the very rare circumstance that an object points to itself; in this scenario, the object refers to itself while loading its attributes which can cause a mixup between loaders. The use case of "object points to itself" is not fully supported, but the fix also removes some overhead so for now is part of testing. fixes #3145 --- doc/build/changelog/changelog_10.rst | 12 +++++ lib/sqlalchemy/orm/strategies.py | 5 +- lib/sqlalchemy/testing/util.py | 4 +- test/orm/test_lazy_relations.py | 80 +++++++++++++++++++++++++++- 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst index fe17957911..5243b7a4dc 100644 --- a/doc/build/changelog/changelog_10.rst +++ b/doc/build/changelog/changelog_10.rst @@ -22,6 +22,18 @@ on compatibility concerns, see :doc:`/changelog/migration_10`. + .. change:: + :tags: bug, orm + :tickets: 3145 + + Made a small adjustment to the mechanics of lazy loading, + such that it has less chance of interfering with a joinload() in the + very rare circumstance that an object points to itself; in this + scenario, the object refers to itself while loading its attributes + which can cause a mixup between loaders. The use case of + "object points to itself" is not fully supported, but the fix also + removes some overhead so for now is part of testing. + .. change:: :tags: feature, orm :tickets: 3176 diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index c3edbf6e6d..2d8a81f0a5 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -634,7 +634,7 @@ class LazyLoader(AbstractRelationshipLoader): LoadLazyAttribute(key), key) return set_lazy_callable, None, None - else: + elif context.populate_existing or mapper.always_refresh: def reset_for_lazy_callable(state, dict_, row): # we are the primary manager for this attribute on # this class - reset its @@ -647,6 +647,9 @@ class LazyLoader(AbstractRelationshipLoader): state._reset(dict_, key) return reset_for_lazy_callable, None, None + else: + return None, None, None + class LoadLazyAttribute(object): diff --git a/lib/sqlalchemy/testing/util.py b/lib/sqlalchemy/testing/util.py index fc8390a79e..7b3f721a61 100644 --- a/lib/sqlalchemy/testing/util.py +++ b/lib/sqlalchemy/testing/util.py @@ -203,5 +203,7 @@ class adict(dict): except KeyError: return dict.__getattribute__(self, key) - def get_all(self, *keys): + def __call__(self, *keys): return tuple([self[key] for key in keys]) + + get_all = __call__ diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py index 16d14026c2..e99e227259 100644 --- a/test/orm/test_lazy_relations.py +++ b/test/orm/test_lazy_relations.py @@ -2,7 +2,7 @@ from sqlalchemy.testing import assert_raises import datetime -from sqlalchemy.orm import attributes, exc as orm_exc +from sqlalchemy.orm import attributes, exc as orm_exc, configure_mappers import sqlalchemy as sa from sqlalchemy import testing, and_ from sqlalchemy import Integer, String, ForeignKey, SmallInteger, Boolean @@ -892,3 +892,81 @@ class O2MWOSideFixedTest(fixtures.MappedTest): [p.id for p in c2.people], [] ) + + +class RefersToSelfLazyLoadInterferenceTest(fixtures.MappedTest): + """Test [issue:3145]. + + This involves an object that refers to itself, which isn't + entirely a supported use case. Here, we're able to fix it, + but long term it's not clear if future needs will affect this. + The use case is not super-critical. + + """ + + @classmethod + def define_tables(cls, metadata): + Table( + 'a', metadata, + Column('a_id', Integer, primary_key=True), + Column('b_id', ForeignKey('b.b_id')), + ) + + Table( + 'b', metadata, + Column('b_id', Integer, primary_key=True), + Column('parent_id', ForeignKey('b.b_id')), + ) + + Table( + 'c', metadata, + Column('c_id', Integer, primary_key=True), + Column('b_id', ForeignKey('b.b_id')), + ) + + @classmethod + def setup_classes(cls): + class A(cls.Basic): + pass + + class B(cls.Basic): + pass + + class C(cls.Basic): + pass + + @classmethod + def setup_mappers(cls): + mapper(cls.classes.A, cls.tables.a, properties={ + "b": relationship(cls.classes.B) + }) + bm = mapper(cls.classes.B, cls.tables.b, properties={ + "parent": relationship( + cls.classes.B, remote_side=cls.tables.b.c.b_id), + "zc": relationship(cls.classes.C) + }) + mapper(cls.classes.C, cls.tables.c) + + bmp = bm._props + configure_mappers() + # Bug is order-dependent, must sort the "zc" property to the end + bmp.sort() + + def test_lazy_doesnt_interfere(self): + A, B, C = self.classes("A", "B", "C") + + session = Session() + b = B() + session.add(b) + session.flush() + + b.parent_id = b.b_id + + b.zc.append(C()) + b.zc.append(C()) + session.commit() + + # If the bug is here, the next line throws an exception + session.query(B).options( + sa.orm.joinedload('parent').joinedload('zc')).all() + -- 2.47.3