Link to state, not object, for mutable extension

The ``sqlalchemy.ext.mutable`` extension now tracks the "parents"
collection using the :class:`.InstanceState` associated with objects,
rather than the object itself. The latter approach required that the object
be hashable so that it can be inside of a ``WeakKeyDictionary``, which goes
against the behavioral contract of the ORM overall which is that ORM mapped
objects do not need to provide any particular kind of ``__hash__()`` method
and that unhashable objects are supported.

Fixes: #6020
Change-Id: I414c8861bc5691f5f320dac3eb696f4b8c37d347

diff --git a/doc/build/changelog/unreleased_14/6020.rst b/doc/build/changelog/unreleased_14/6020.rst
new file mode 100644 (file)
index 0000000..0db2fa9
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/6020.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, ext
+    :tickets: 6020
+
+    The ``sqlalchemy.ext.mutable`` extension now tracks the "parents"
+    collection using the :class:`.InstanceState` associated with objects,
+    rather than the object itself. The latter approach required that the object
+    be hashable so that it can be inside of a ``WeakKeyDictionary``, which goes
+    against the behavioral contract of the ORM overall which is that ORM mapped
+    objects do not need to provide any particular kind of ``__hash__()`` method
+    and that unhashable objects are supported.

diff --git a/lib/sqlalchemy/ext/mutable.py b/lib/sqlalchemy/ext/mutable.py
index 4bb9e795bb312a3da84ad50ac43b489263f02bb7..fef8865011bf44f4c9d08bff67ccae02b3ee8282 100644 (file)
--- a/lib/sqlalchemy/ext/mutable.py
+++ b/lib/sqlalchemy/ext/mutable.py
@@ -357,10 +357,10 @@ pickling process of the parent's object-relational state so that the
 import weakref
 
 from .. import event
+from .. import inspect
 from .. import types
 from ..orm import Mapper
 from ..orm import mapper
-from ..orm import object_mapper
 from ..orm.attributes import flag_modified
 from ..sql.base import SchemaEventTarget
 from ..util import memoized_property
@@ -374,12 +374,17 @@ class MutableBase(object):
 
     @memoized_property
     def _parents(self):
-        """Dictionary of parent object->attribute name on the parent.
+        """Dictionary of parent object's :class:`.InstanceState`->attribute
+        name on the parent.
 
         This attribute is a so-called "memoized" property.  It initializes
         itself with a new ``weakref.WeakKeyDictionary`` the first time
         it is accessed, returning the same object upon subsequent access.
 
+        .. versionchanged:: 1.4 the :class:`.InstanceState` is now used
+           as the key in the weak dictionary rather than the instance
+           itself.
+
         """
 
         return weakref.WeakKeyDictionary()
@@ -461,7 +466,7 @@ class MutableBase(object):
                 if coerce:
                     val = cls.coerce(key, val)
                     state.dict[key] = val
-                val._parents[state.obj()] = key
+                val._parents[state] = key
 
         def load_attrs(state, ctx, attrs):
             if not attrs or listen_keys.intersection(attrs):
@@ -482,9 +487,9 @@ class MutableBase(object):
             if not isinstance(value, cls):
                 value = cls.coerce(key, value)
             if value is not None:
-                value._parents[target.obj()] = key
+                value._parents[target] = key
             if isinstance(oldvalue, cls):
-                oldvalue._parents.pop(target.obj(), None)
+                oldvalue._parents.pop(inspect(target), None)
             return value
 
         def pickle(state, state_dict):
@@ -497,7 +502,7 @@ class MutableBase(object):
         def unpickle(state, state_dict):
             if "ext.mutable.values" in state_dict:
                 for val in state_dict["ext.mutable.values"]:
-                    val._parents[state.obj()] = key
+                    val._parents[state] = key
 
         event.listen(parent_cls, "load", load, raw=True, propagate=True)
         event.listen(
@@ -527,7 +532,7 @@ class Mutable(MutableBase):
         """Subclasses should call this method whenever change events occur."""
 
         for parent, key in self._parents.items():
-            flag_modified(parent, key)
+            flag_modified(parent.obj(), key)
 
     @classmethod
     def associate_with_attribute(cls, attribute):
@@ -647,11 +652,11 @@ class MutableComposite(MutableBase):
 
         for parent, key in self._parents.items():
 
-            prop = object_mapper(parent).get_property(key)
+            prop = parent.mapper.get_property(key)
             for value, attr_name in zip(
                 self.__composite_values__(), prop._attribute_keys
             ):
-                setattr(parent, attr_name, value)
+                setattr(parent.obj(), attr_name, value)
 
 
 def _setup_composite_listener():

diff --git a/test/ext/test_mutable.py b/test/ext/test_mutable.py
index 49f54c4a5c097c94a32071ef20b970ca12a22d93..e89107c6d3785716170408f9ec6ff73c064852d4 100644 (file)
--- a/test/ext/test_mutable.py
+++ b/test/ext/test_mutable.py
@@ -6,6 +6,7 @@ from sqlalchemy import ForeignKey
 from sqlalchemy import func
 from sqlalchemy import Integer
 from sqlalchemy import String
+from sqlalchemy import testing
 from sqlalchemy import util
 from sqlalchemy.ext.mutable import MutableComposite
 from sqlalchemy.ext.mutable import MutableDict
@@ -52,6 +53,10 @@ class FooWithEq(object):
         return self.id == other.id
 
 
+class FooWNoHash(fixtures.BasicEntity):
+    __hash__ = None
+
+
 class Point(MutableComposite):
     def __init__(self, x, y):
         self.x = x
@@ -848,6 +853,60 @@ class _MutableSetTestBase(_MutableSetTestFixture):
         eq_(f1.data, set([1, 2]))
 
 
+class _MutableNoHashFixture(object):
+    @testing.fixture(autouse=True, scope="class")
+    def set_class(self):
+        global Foo
+
+        _replace_foo = Foo
+        Foo = FooWNoHash
+
+        yield
+        Foo = _replace_foo
+
+    def test_ensure_not_hashable(self):
+        d = {}
+        obj = Foo()
+        with testing.expect_raises(TypeError):
+            d[obj] = True
+
+
+class MutableListNoHashTest(
+    _MutableNoHashFixture, _MutableListTestBase, fixtures.MappedTest
+):
+    @classmethod
+    def define_tables(cls, metadata):
+        MutableList = cls._type_fixture()
+
+        mutable_pickle = MutableList.as_mutable(PickleType)
+        Table(
+            "foo",
+            metadata,
+            Column(
+                "id", Integer, primary_key=True, test_needs_autoincrement=True
+            ),
+            Column("data", mutable_pickle),
+        )
+
+
+class MutableDictNoHashTest(
+    _MutableNoHashFixture, _MutableDictTestBase, fixtures.MappedTest
+):
+    @classmethod
+    def define_tables(cls, metadata):
+        MutableDict = cls._type_fixture()
+
+        mutable_pickle = MutableDict.as_mutable(PickleType)
+        Table(
+            "foo",
+            metadata,
+            Column(
+                "id", Integer, primary_key=True, test_needs_autoincrement=True
+            ),
+            Column("data", mutable_pickle),
+        )
+
+
 class MutableColumnDefaultTest(_MutableDictTestFixture, fixtures.MappedTest):
     @classmethod
     def define_tables(cls, metadata):
@@ -1396,7 +1455,7 @@ class MutableCompositesTest(_CompositeTestBase, fixtures.MappedTest):
         # by the mutable composite, and tracking would be lost
         sess.refresh(f1, ["unrelated_data"])
 
-        is_(list(f1.data._parents.keys())[0], f1)
+        is_(list(f1.data._parents.keys())[0], f1._sa_instance_state)
 
         f1.data.y = 9
 
@@ -1409,7 +1468,7 @@ class MutableCompositesTest(_CompositeTestBase, fixtures.MappedTest):
 
         sess.refresh(f1, ["unrelated_data", "y"])
 
-        is_(list(f1.data._parents.keys())[0], f1)
+        is_(list(f1.data._parents.keys())[0], f1._sa_instance_state)
 
         f1.data.y = 15
         sess.commit()