very intricate expiration sequence can lead to a detached object buried in a bound parameter

[sqlalchemy-bot]

Migrated issue, originally created by Michael Bayer (@zzzeek)

I've not managed to reduce this further yet, simple attempts are still failing to identify the full issue

from sqlalchemy import Column, ForeignKey, Integer, MetaData, create_engine
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import backref, relationship, Session

Base = declarative_base()


class Person(Base):
    __tablename__ = "persons"

    id = Column(Integer, primary_key=True)


class Car(Base):
    __tablename__ = "cars"

    id = Column(Integer, primary_key=True)
    owner_id = Column(Integer, ForeignKey("persons.id"))
    owner = relationship("Person")


class House(Base):
    __tablename__ = "houses"

    id = Column(Integer, primary_key=True)
    owner_id = Column(Integer, ForeignKey("persons.id"))
    owner = relationship("Person")


class PersonPersonRelationship(Base):
    __tablename__ = "person_person_relationships"

    from_person_id = Column(Integer, ForeignKey("persons.id"), primary_key=True)
    from_person = relationship(
        "Person",
        foreign_keys=[from_person_id],
        backref=backref("relationships"),
    )
    to_person_id = Column(Integer, ForeignKey("persons.id"), primary_key=True)
    to_person = relationship("Person", foreign_keys=[to_person_id])


engine = create_engine("sqlite://")

Base.metadata.create_all(engine)

# Set up our data
session = Session(bind=engine)
person1 = Person(id=1)
session.add(person1)
person2 = Person(id=2)
session.add(person2)
PersonPersonRelationship(from_person=person2, to_person=person1)
car = Car(owner=person2)
session.add(car)
session.commit()
session.close()

print("------------------------------------------------------------")
# Start with a fresh session to reproduce the issue
new_session = Session(engine)


def make_foo_query(sess):
    owner = sess.query(Person).get(2)
    return House.owner == owner

# TODO: need to figure out how to reduce this mapping
# Pull the car, its owner, and the owner's related people into memory
car = new_session.query(Car).one()
relationships = car.owner.relationships

crit = make_foo_query(new_session)

for s in sorted(
    new_session.identity_map.all_states(),
    key=lambda x: id(x.obj()),
    reverse=True
):
    print("object: %s" % s.obj())
    new_session.expire(s.obj())


print(crit.left.callable())

------------------------------------------------------------
object: <__main__.Person object at 0x7fd62fd87d30>
object: <__main__.Car object at 0x7fd62fd87898>
object: <__main__.PersonPersonRelationship object at 0x7fd62fd214e0>
Traceback (most recent call last):
  File "test.py", line 84, in <module>
    print(crit.left.callable())
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/relationships.py", line 1429, in _go
    value = fn(*arg, **kw)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/mapper.py", line 2625, in _get_state_attr_by_column
    return state.manager[prop.key].impl.get(state, dict_, passive=passive)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/attributes.py", line 597, in get
    value = state._load_expired(state, passive)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/state.py", line 617, in _load_expired
    self.manager.deferred_scalar_loader(self, toload)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 813, in load_scalar_attributes
    (state_str(state)))
sqlalchemy.orm.exc.DetachedInstanceError: Instance <Person at 0x87b640> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/bhk3)

for some reason if I just do a simple A/B test and have "B" expired first, the issue does not reproduce. so not sure what's going on yet.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

OK here's a much more succinct case

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'

    id = Column(Integer, primary_key=True)
    bs = relationship("B", backref="a")


class B(Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)
    a_id = Column(ForeignKey("a.id"))

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)

s.add(A(bs=[B()]))
s.commit()


def foo(s):
    a = s.query(A).first()
    return B.a == a

b1 = s.query(B).first()
b1.a
crit = foo(s)

s.expire(b1.a)
s.expire(b1)

print(crit.left.callable())

I think the deferred nature of the comparison needs to be liberalized such that it memoizes the immediate comparison state and uses that if the object is expired.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

this is going to be great. the whole tradition of these loader callables being deferred is almost never needed and the purpose of it is to suit using unflushed objects. so we will just get the value immediately, as is almost always what people want, and only if that is None do we try again. it's a big change but will make things work a lot more simply.

diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 818f1c0ae2..8ab64cd66e 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -1425,7 +1425,16 @@ class RelationshipProperty(StrategizedProperty):
         return criterion
 
     def _get_attr_w_warn_on_none(self, column, fn, *arg, **kw):
+        # in the vast majority of cases, this is the value
+        # we want to use.   The edge case served by having a deferred
+        # callable is that the object has been added to a session but
+        # is not flushed yet, in which case we re-evaluate the callable
+        # a second time.
+        pre_exec_value = fn(*arg, **kw)
+
         def _go():
+            if pre_exec_value is not None:
+                return pre_exec_value
             value = fn(*arg, **kw)
             if value is None:
                 util.warn(
diff --git a/test/orm/test_dynamic.py b/test/orm/test_dynamic.py
index 5c514ff1a5..3cbf651716 100644
--- a/test/orm/test_dynamic.py
+++ b/test/orm/test_dynamic.py
@@ -504,13 +504,12 @@ class UOWTest(
 
         u1.addresses.append(Address(email_address='a2'))
 
+        # this was not true before the change
+        assert 'id' in u1.__dict__
+
         self.assert_sql_execution(
             testing.db,
             sess.flush,
-            CompiledSQL(
-                "SELECT users.id AS users_id, users.name AS users_name "
-                "FROM users WHERE users.id = :param_1",
-                lambda ctx: [{"param_1": u1_id}]),
             CompiledSQL(
                 "INSERT INTO addresses (user_id, email_address) "
                 "VALUES (:user_id, :email_address)",
@@ -536,6 +535,9 @@ class UOWTest(
 
         u1.addresses.remove(a2)
 
+        # this was not true before the change
+        assert 'id' in u1.__dict__
+
         self.assert_sql_execution(
             testing.db,
             sess.flush,
@@ -549,11 +551,7 @@ class UOWTest(
                 "UPDATE addresses SET user_id=:user_id WHERE addresses.id = "
                 ":addresses_id",
                 lambda ctx: [{'addresses_id': a2_id, 'user_id': None}]
-            ),
-            CompiledSQL(
-                "SELECT users.id AS users_id, users.name AS users_name "
-                "FROM users WHERE users.id = :param_1",
-                lambda ctx: [{"param_1": u1_id}]),
+            )
         )
 
     def test_rollback(self):

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• added labels: high priority

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed milestone from "1.2.x" to "1.3"

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

https://gerrit.sqlalchemy.org/#/c/zzzeek/sqlalchemy/+/918

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

Modernize deferred callable for many-to-one comparison

Improved the behavior of a relationship-bound many-to-one object expression
such that the retrieval of column values on the related object are now
resilient against the object being detached from its parent
:class:.Session, even if the attribute has been expired. New features
within the :class:.InstanceState are used to memoize the last known value
of a particular column attribute before its expired, so that the expression
can still evaluate when the object is detached and expired at the same
time. Error conditions are also improved using modern attribute state
features to produce more specific messages as needed.

To support the value being mutated while also being resilient towards
expiration, a new feature to InstanceState is added ._last_known_values
which holds onto the expired value when an individual key is expired.
Only takes effect specific to keys and InstanceState objects that
received a special instruction so this does not add to overall
memory/latency.

Fixes: #4359
Change-Id: Iff272e667bf741074549db550bf65348553ca8e7

→ d5c2db4

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed status to closed