From: Mike Bayer Date: Thu, 1 Nov 2018 17:15:14 +0000 (-0400) Subject: Implement __delete__ X-Git-Tag: rel_1_3_0b1~28^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2e2af8d918a568ca3036f88e457caaf59f6af644;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Implement __delete__ A long-standing oversight in the ORM, the ``__delete__`` method for a many- to-one relationship was non-functional, e.g. for an operation such as ``del a.b``. This is now implemented and is equivalent to setting the attribute to ``None``. Fixes: #4354 Change-Id: I60131a84c007b0bf6f20c5cc5f21a3b96e954046 --- diff --git a/doc/build/changelog/migration_13.rst b/doc/build/changelog/migration_13.rst index 585a9c3a5a..fee5ad5e94 100644 --- a/doc/build/changelog/migration_13.rst +++ b/doc/build/changelog/migration_13.rst @@ -84,6 +84,24 @@ users should report a bug, however the change also incldues a flag :ticket:`4340` +.. _change_4354: + +"del" implemented for ORM attributes +------------------------------------ + +The Python ``del`` operation was not really usable for mapped attributes, either +scalar columns or object references. Support has been added for this to work correctly, +where the ``del`` operation is roughly equivalent to setting the attribute to the +``None`` value:: + + + some_object = session.query(SomeObject).get(5) + + del some_object.some_attribute # from a SQL perspective, works like "= None" + +:ticket:`4354` + + .. _change_4257: info dictionary added to InstanceState diff --git a/doc/build/changelog/unreleased_13/4354.rst b/doc/build/changelog/unreleased_13/4354.rst new file mode 100644 index 0000000000..ba25a2231e --- /dev/null +++ b/doc/build/changelog/unreleased_13/4354.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, orm + :tickets: 4354 + + A long-standing oversight in the ORM, the ``__delete__`` method for a many- + to-one relationship was non-functional, e.g. for an operation such as ``del + a.b``. This is now implemented and is equivalent to setting the attribute + to ``None``. + + .. seealso:: + + :ref:`change_4354` \ No newline at end of file diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 745032e447..3eaa41e8fe 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -674,7 +674,6 @@ class ScalarAttributeImpl(AttributeImpl): self._remove_token = Event(self, OP_REMOVE) def delete(self, state, dict_): - if self.dispatch._active_history: old = self.get(state, dict_, PASSIVE_RETURN_NEVER_SET) else: @@ -683,10 +682,12 @@ class ScalarAttributeImpl(AttributeImpl): if self.dispatch.remove: self.fire_remove_event(state, dict_, old, self._remove_token) state._modified_event(dict_, self, old) - try: - del dict_[self.key] - except KeyError: - raise AttributeError("%s object does not have a value" % self) + + existing = dict_.pop(self.key, NO_VALUE) + if existing is NO_VALUE and old is NO_VALUE and \ + not state.expired and \ + self.key not in state.expired_attributes: + raise AttributeError("%s object does not have a value" % self) def get_history(self, state, dict_, passive=PASSIVE_OFF): if self.key in dict_: @@ -744,11 +745,25 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): __slots__ = () def delete(self, state, dict_): - old = self.get(state, dict_) + if self.dispatch._active_history: + old = self.get( + state, dict_, + passive=PASSIVE_ONLY_PERSISTENT | + NO_AUTOFLUSH | LOAD_AGAINST_COMMITTED) + else: + old = self.get( + state, dict_, passive=PASSIVE_NO_FETCH ^ INIT_OK | + LOAD_AGAINST_COMMITTED) + self.fire_remove_event(state, dict_, old, self._remove_token) - try: - del dict_[self.key] - except: + + existing = dict_.pop(self.key, NO_VALUE) + + # if the attribute is expired, we currently have no way to tell + # that an object-attribute was expired vs. not loaded. So + # for this test, we look to see if the object has a DB identity. + if existing is NO_VALUE and old is not PASSIVE_NO_RESULT and \ + state.key is None: raise AttributeError("%s object does not have a value" % self) def get_history(self, state, dict_, passive=PASSIVE_OFF): @@ -1382,6 +1397,10 @@ class History(History): # key situations if id(original) in _NO_STATE_SYMBOLS: deleted = () + # indicate a "del" operation occured when we don't have + # the previous value as: ([None], (), ()) + if id(current) in _NO_STATE_SYMBOLS: + current = None else: deleted = [original] if current is NEVER_SET: @@ -1398,7 +1417,7 @@ class History(History): return cls((), (), ()) else: return cls((), [current], ()) - elif current is original: + elif current is original and current is not NEVER_SET: return cls((), [current], ()) else: # current convention on related objects is to not @@ -1408,6 +1427,10 @@ class History(History): # ignore the None in any case. if id(original) in _NO_STATE_SYMBOLS or original is None: deleted = () + # indicate a "del" operation occured when we don't have + # the previous value as: ([None], (), ()) + if id(current) in _NO_STATE_SYMBOLS: + current = None else: deleted = [original] if current is NO_VALUE or current is NEVER_SET: diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index 1a68ea9c7a..960b9e5d59 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -752,6 +752,9 @@ class ManyToOneDP(DependencyProcessor): for child in history.added: self._synchronize(state, child, None, False, uowcommit, "add") + elif history.deleted: + self._synchronize( + state, None, None, True, uowcommit, "delete") if self.post_update: self._post_update(state, uowcommit, history.sum()) diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py index ae5cee8d2e..a83a99d78d 100644 --- a/lib/sqlalchemy/orm/unitofwork.py +++ b/lib/sqlalchemy/orm/unitofwork.py @@ -61,19 +61,22 @@ def track_cascade_events(descriptor, prop): if prop.uselist else "related attribute delete") - # expunge pending orphans - item_state = attributes.instance_state(item) + if item is not None and \ + item is not attributes.NEVER_SET and \ + item is not attributes.PASSIVE_NO_RESULT and \ + prop._cascade.delete_orphan: + # expunge pending orphans + item_state = attributes.instance_state(item) - if prop._cascade.delete_orphan and \ - prop.mapper._is_orphan(item_state): - if sess and item_state in sess._new: - sess.expunge(item) - else: - # the related item may or may not itself be in a - # Session, however the parent for which we are catching - # the event is not in a session, so memoize this on the - # item - item_state._orphaned_outside_of_session = True + if prop.mapper._is_orphan(item_state): + if sess and item_state in sess._new: + sess.expunge(item) + else: + # the related item may or may not itself be in a + # Session, however the parent for which we are catching + # the event is not in a session, so memoize this on the + # item + item_state._orphaned_outside_of_session = True def set_(state, newvalue, oldvalue, initiator): # process "save_update" cascade rules for when an instance diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index d56d81565c..6d2610fd67 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -319,6 +319,8 @@ class AttributesTest(fixtures.ORMTest): del f1.b is_(f1.b, None) + f1 = Foo() + def go(): del f1.b @@ -1651,7 +1653,7 @@ class HistoryTest(fixtures.TestBase): **kw) return Foo - def _two_obj_fixture(self, uselist): + def _two_obj_fixture(self, uselist, active_history=False): class Foo(fixtures.BasicEntity): pass @@ -1662,7 +1664,8 @@ class HistoryTest(fixtures.TestBase): instrumentation.register_class(Foo) instrumentation.register_class(Bar) attributes.register_attribute(Foo, 'someattr', uselist=uselist, - useobject=True) + useobject=True, + active_history=active_history) return Foo, Bar def _someattr_history(self, f, **kw): @@ -1732,6 +1735,69 @@ class HistoryTest(fixtures.TestBase): f = Foo() eq_(self._someattr_history(f), ((), (), ())) + def test_object_replace(self): + Foo, Bar = self._two_obj_fixture(uselist=False) + f = Foo() + b1, b2 = Bar(), Bar() + f.someattr = b1 + self._commit_someattr(f) + + f.someattr = b2 + eq_(self._someattr_history(f), ([b2], (), [b1])) + + def test_object_set_none(self): + Foo, Bar = self._two_obj_fixture(uselist=False) + f = Foo() + b1 = Bar() + f.someattr = b1 + self._commit_someattr(f) + + f.someattr = None + eq_(self._someattr_history(f), ([None], (), [b1])) + + def test_object_set_none_expired(self): + Foo, Bar = self._two_obj_fixture(uselist=False) + f = Foo() + b1 = Bar() + f.someattr = b1 + self._commit_someattr(f) + + attributes.instance_state(f).dict.pop('someattr', None) + attributes.instance_state(f).expired_attributes.add('someattr') + + f.someattr = None + eq_(self._someattr_history(f), ([None], (), ())) + + def test_object_del(self): + Foo, Bar = self._two_obj_fixture(uselist=False) + f = Foo() + b1 = Bar() + f.someattr = b1 + + self._commit_someattr(f) + + del f.someattr + eq_(self._someattr_history(f), ((), (), [b1])) + + def test_object_del_expired(self): + Foo, Bar = self._two_obj_fixture(uselist=False) + f = Foo() + b1 = Bar() + f.someattr = b1 + self._commit_someattr(f) + + # the "delete" handler checks if the object + # is db-loaded when testing if an empty "del" is valid, + # because there's nothing else to look at for a related + # object, there's no "expired" status + attributes.instance_state(f).key = ('foo', ) + attributes.instance_state(f)._expire_attributes( + attributes.instance_dict(f), + ['someattr']) + + del f.someattr + eq_(self._someattr_history(f), ([None], (), ())) + def test_scalar_no_init_side_effect(self): Foo = self._fixture(uselist=False, useobject=False, active_history=False) @@ -1760,6 +1826,40 @@ class HistoryTest(fixtures.TestBase): f.someattr = None eq_(self._someattr_history(f), ([None], (), ())) + def test_scalar_del(self): + # note - compare: + # test_scalar_set_None, + # test_scalar_get_first_set_None, + # test_use_object_set_None, + # test_use_object_get_first_set_None + Foo = self._fixture(uselist=False, useobject=False, + active_history=False) + f = Foo() + f.someattr = 5 + attributes.instance_state(f).key = ('foo', ) + self._commit_someattr(f) + + del f.someattr + eq_(self._someattr_history(f), ((), (), [5])) + + def test_scalar_del_expired(self): + # note - compare: + # test_scalar_set_None, + # test_scalar_get_first_set_None, + # test_use_object_set_None, + # test_use_object_get_first_set_None + Foo = self._fixture(uselist=False, useobject=False, + active_history=False) + f = Foo() + f.someattr = 5 + self._commit_someattr(f) + + attributes.instance_state(f)._expire_attributes( + attributes.instance_dict(f), + ['someattr']) + del f.someattr + eq_(self._someattr_history(f), ([None], (), ())) + def test_scalar_get_first_set_None(self): # note - compare: # test_scalar_set_None, diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py index 9c1a26e4bb..0d19bee75e 100644 --- a/test/orm/test_unitofworkv2.py +++ b/test/orm/test_unitofworkv2.py @@ -455,6 +455,74 @@ class RudimentaryFlushTest(UOWTest): ), ) + def test_many_to_one_del_attr(self): + users, Address, addresses, User = (self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User) + + mapper(User, users) + mapper(Address, addresses, properties={ + 'user': relationship(User) + }) + sess = create_session() + + u1 = User(name='u1') + a1, a2 = Address(email_address='a1', user=u1), \ + Address(email_address='a2', user=u1) + sess.add_all([a1, a2]) + sess.flush() + + del a1.user + self.assert_sql_execution( + testing.db, + sess.flush, + CompiledSQL( + "UPDATE addresses SET user_id=:user_id WHERE " + "addresses.id = :addresses_id", + lambda ctx: [ + {'addresses_id': a1.id, 'user_id': None}, + ] + ) + ) + + def test_many_to_one_del_attr_unloaded(self): + users, Address, addresses, User = (self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User) + + mapper(User, users) + mapper(Address, addresses, properties={ + 'user': relationship(User) + }) + sess = create_session() + + u1 = User(name='u1') + a1, a2 = Address(email_address='a1', user=u1), \ + Address(email_address='a2', user=u1) + sess.add_all([a1, a2]) + sess.flush() + + # trying to guarantee that the history only includes + # PASSIVE_NO_RESULT for "deleted" and nothing else + sess.expunge(u1) + sess.expire(a1, ['user']) + del a1.user + + sess.add(a1) + self.assert_sql_execution( + testing.db, + sess.flush, + CompiledSQL( + "UPDATE addresses SET user_id=:user_id WHERE " + "addresses.id = :addresses_id", + lambda ctx: [ + {'addresses_id': a1.id, 'user_id': None}, + ] + ) + ) + def test_natural_ordering(self): """test that unconnected items take relationship() into account regardless."""