Possible bug when using Mutable and detached instances

[anler]

Describe the bug

Hello!

I have run into this issue.

1. I have a model with a Mutable field
2. I cache that model, and then read it back. The cache is done using Pickle
3. Since the cached model is read in a detached state, I move it into a persistent state using session.merge(instance, load=False)
4. Now, if I change the Mutable field in the persistent instance returned by session.merge(), those changes are not detected by the session

Here's the snippet of code and the link to the full example

    account = session.query(Account).get(1)
    cached_detached_account = pickle.loads(pickle.dumps(account))
    cached_persistent_account = session.merge(cached_detached_account, load=False)

    cached_persistent_account.settings["email_notifications"] = True

    # Both of these assertions fail
    assert sa.inspect(cached_persistent_account).modified, "instance not modified"
    assert (
        cached_persistent_account in cached_detached_account.settings._parents
    ), "instance not being tracked"

To Reproduce

I have created this example project: https://github.com/anler/sqlalchemy-mutable-example with branches for SA 1.3 and 1.4

import pickle

import sqlalchemy as sa
from sqlalchemy.orm import sessionmaker
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.ext.mutable import Mutable, MutableDict
from sqlalchemy.types import JSON

Base = declarative_base()
Session = sessionmaker()


class Account(Base):
    __tablename__ = "account"

    id = sa.Column(sa.Integer, primary_key=True)
    settings = sa.Column(MutableDict.as_mutable(JSON))


def main():
    engine = sa.create_engine("sqlite:///:memory:", echo=True)
    Base.metadata.create_all(engine)
    Session.configure(bind=engine)

    session = Session()

    session.add(Account(id=1, settings={"email_notifications": False}))
    session.commit()

    account = session.query(Account).get(1)
    cached_detached_account = pickle.loads(pickle.dumps(account))
    cached_persistent_account = session.merge(cached_detached_account, load=False)

    cached_persistent_account.settings["email_notifications"] = True

    # Both of these assertions fail
    assert sa.inspect(cached_persistent_account).modified, "instance not modified"
    assert (
        cached_persistent_account in cached_detached_account.settings._parents
    ), "instance not being tracked"

Error

There is no error

Versions

• OS: MacOS Monterrey 12.5.1, reproduced in both M1 and Intel chips
• Python: 2.7 and 3.10
• SQLAlchemy: 1.3.24 and 1.4.40
• Database: MySQL 5.5 and SQLite
• DBAPI (eg: psycopg, cx_oracle, mysqlclient): mysqlclient, sqlite3

Additional context

No response

Thank you! 🙏🏻

[zzzeek]

here's a few things:

1. the assertion at the end is wrong. those keys are instance states, so this is the better assertion:

    assert (
        sa.inspect(cached_persistent_account) in cached_detached_account.settings._parents
    ), "instance not being tracked"

2. the thing that is making something go wrong is the combination of "load=False" combined with the fact that "account" is already present in the session. no pickle needed

    some_other_session = Session()

    # dont use pickle.  just have an Account that we can merge from outside
    # that is independent of this Session.
    account = some_other_session.query(Account).get(1)

    # establish the Account as already present in the session.
    # combined with load=False below this causes a problem
    # comment this out, test case works
    account_already_loaded = session.query(Account).get(1)


    cached_detached_account = account

    cached_persistent_account = session.merge(
        cached_detached_account,

        # or, comment this out, test case works
        load=False
    )

we can make a test case that shows the problem more directly as:

import sqlalchemy as sa
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.ext.mutable import MutableDict
from sqlalchemy.orm import sessionmaker
from sqlalchemy.types import JSON

Base = declarative_base()
Session = sessionmaker()


class Account(Base):
    __tablename__ = "account"

    id = sa.Column(sa.Integer, primary_key=True)
    settings = sa.Column(MutableDict.as_mutable(JSON))


def main():
    engine = sa.create_engine("sqlite:///:memory:", echo=True)
    Base.metadata.create_all(engine)
    Session.configure(bind=engine)

    session = Session()

    session.add(Account(id=1, settings={"email_notifications": False}))
    session.commit()

    some_other_session = Session()

    # dont use pickle.  just have an Account that we can merge from outside
    # that is independent of this Session.
    account = some_other_session.query(Account).get(1)

    # establish the Account as already present in the session.
    # combined with load=False below this causes a problem
    # comment this out, test case works
    account_already_loaded = session.query(Account).get(1)


    cached_detached_account = account

    cached_persistent_account = session.merge(
        cached_detached_account,
        load=False
    )

    assert cached_persistent_account is account_already_loaded
    cached_persistent_account.settings["email_notifications"] = True

    assert sa.inspect(
        cached_persistent_account
    ).modified, "instance not modified"


main()

the fix needs to be something like the following, as there's no event hook for "merged existing object with load=False", yet the merge between the persistent and "detached" object still happens, overwriting the dictionary

diff --git a/lib/sqlalchemy/ext/mutable.py b/lib/sqlalchemy/ext/mutable.py
index cbec06a31f..0ef4b69e39 100644
--- a/lib/sqlalchemy/ext/mutable.py
+++ b/lib/sqlalchemy/ext/mutable.py
@@ -511,6 +511,8 @@ class MutableBase(object):
                     for val in state_dict["ext.mutable.values"][key]:
                         val._parents[state] = key
 
+        event.listen(parent_cls, "_sa_event_merge_wo_load", load, raw=True, propagate=True)
+
         event.listen(parent_cls, "load", load, raw=True, propagate=True)
         event.listen(
             parent_cls, "refresh", load_attrs, raw=True, propagate=True
diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py
index 39659c7232..27bc0bf470 100644
--- a/lib/sqlalchemy/orm/events.py
+++ b/lib/sqlalchemy/orm/events.py
@@ -325,6 +325,9 @@ class InstanceEvents(event.Events):
 
         """
 
+    def _sa_event_merge_wo_load(self, target, context):
+        """internal use only"""
+
     def load(self, target, context):
         """Receive an object instance after it has been created via
         ``__new__``, and after initial attribute population has
diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index d32af17464..a025a774db 100644
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -311,6 +311,7 @@ class ColumnProperty(StrategizedProperty):
 
             if not load:
                 dest_dict[self.key] = value
+
             else:
                 impl = dest_state.get_impl(self.key)
                 impl.set(dest_state, dest_dict, value, None)
diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index c6a91693e3..8757632b66 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -3151,6 +3151,7 @@ class Session(_SessionClassMethods):
         if not load:
             # remove any history
             merged_state._commit_all(merged_dict, self.identity_map)
+            merged_state.manager.dispatch._sa_event_merge_wo_load(merged_state, None)
 
         if new_instance:
             merged_state.manager.dispatch.load(merged_state, None)

[anler]

Thanks @zzzeek I tried out your changes and they work, only that I had to remove the leading score in the event name since the project I'm working in uses SQLAlchemy v1.3. Do you plan to apply these changes in that version too?

[zzzeek]

this would be in 1.4.41. releases for 1.3 have been over for about 18 months and is expected to be EOL sometime next year.

[anler]

Understood, thanks again!

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the main branch:

implement event for merge/load=False for mutable state setup https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4056

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the rel_1_4 branch:

implement event for merge/load=False for mutable state setup https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4057