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

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

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`

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:

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())
 

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

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,

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."""