From 1031fc6f781a3a6088467791e837f23a4b338aef Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 9 Mar 2021 10:48:59 -0500 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_14/6020.rst | 11 ++++ lib/sqlalchemy/ext/mutable.py | 23 ++++---- test/ext/test_mutable.py | 63 +++++++++++++++++++++- 3 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/6020.rst diff --git a/doc/build/changelog/unreleased_14/6020.rst b/doc/build/changelog/unreleased_14/6020.rst new file mode 100644 index 0000000000..0db2fa9904 --- /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 4bb9e795bb..fef8865011 100644 --- 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 49f54c4a5c..e89107c6d3 100644 --- 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() -- 2.47.2