Fixed ObjectManager to not swallow exceptions during object deletion (in

debug mode and if the user is not Manager). This allows for better
debugging, while still keeping the possibility for a Manager to delete
buggy objects.

doc/CHANGES.txt

@@ -26,6 +26,11 @@ Zope Changes
 
     Features added
 
+      - Fixed ObjectManager to not swallow exceptions during object
+        deletion (in debug mode and if the user is not Manager). This
+        allows for better debugging, while still keeping the possibility
+        for a Manager to delete buggy objects.
+
       - Added a ZConfig directive 'large-file-threshold' to control
         the request content-size threshold at which a temporary file
         gets created. Use the same value for deciding between reading

lib/python/OFS/ObjectManager.py

@@ -307,7 +307,10 @@ class ObjectManager(
             except:
                 LOG('Zope',ERROR,'manage_beforeDelete() threw',
                     error=sys.exc_info())
-                pass
+                # In debug mode when non-Manager, let exceptions propagate.
+                if getConfiguration().debug_mode:
+                    if not getSecurityManager().getUser().has_role('Manager'):
+                        raise
             if s is None: object._p_deactivate()
 
     def _delObject(self, id, dp=1):
@@ -319,9 +322,12 @@ class ObjectManager(
         except ConflictError:
             raise
         except:
-            LOG('Zope',ERROR,'manage_beforeDelete() threw',
+            LOG('Zope', ERROR, '_delObject() threw',
                 error=sys.exc_info())
-            pass
+            # In debug mode when non-Manager, let exceptions propagate.
+            if getConfiguration().debug_mode:
+                if not getSecurityManager().getUser().has_role('Manager'):
+                    raise
         self._objects=tuple(filter(lambda i,n=id: i['id']!=n, self._objects))
         self._delOb(id)
 

lib/python/OFS/tests/testObjectManager.py

 import unittest
 
+from App.config import getConfiguration
 from Acquisition import Implicit, aq_base, aq_parent
 from AccessControl.SecurityManagement import newSecurityManager
 from AccessControl.SecurityManagement import noSecurityManager
@@ -30,11 +31,34 @@ class FauxUser( Implicit ):
 
         return self._id
 
+class DeleteFailed(Exception):
+    pass
+
+class ItemForDeletion(SimpleItem):
+
+    def __init__(self, fail_on_delete=False):
+        self.id = 'stuff'
+        self.before_delete_called = False
+        self.fail_on_delete = fail_on_delete
+
+    def manage_beforeDelete(self, item, container):
+        self.before_delete_called = True
+        if self.fail_on_delete:
+            raise DeleteFailed
+        return SimpleItem.manage_beforeDelete(self, item, container)
+
+
 class ObjectManagerTests( unittest.TestCase ):
 
-    def tearDown( self ):
+    def setUp(self):
+        self.saved_cfg_debug_mode = getConfiguration().debug_mode
 
+    def tearDown( self ):
         noSecurityManager()
+        getConfiguration().debug_mode = self.saved_cfg_debug_mode
+
+    def setDebugMode(self, mode):
+        getConfiguration().debug_mode = mode
 
     def _getTargetClass( self ):
 
@@ -219,6 +243,66 @@ class ObjectManagerTests( unittest.TestCase ):
 
         self.assertEqual( si.__ac_local_roles__, None )
 
+    def test_delObject_before_delete(self):
+        # Test that manage_beforeDelete is called
+        om = self._makeOne()
+        ob = ItemForDeletion()
+        om._setObject(ob.getId(), ob)
+        self.assertEqual(ob.before_delete_called, False)
+        om._delObject(ob.getId())
+        self.assertEqual(ob.before_delete_called, True)
+
+    def test_delObject_exception_manager(self):
+        # Test exception behavior in manage_beforeDelete
+        # Manager user
+        self.setDebugMode(False)
+        newSecurityManager(None, system) # Manager
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        om._delObject(ob.getId())
+
+    def test_delObject_exception(self):
+        # Test exception behavior in manage_beforeDelete
+        # non-Manager user
+        self.setDebugMode(False)
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        om._delObject(ob.getId())
+
+    def test_delObject_exception_debug_manager(self):
+        # Test exception behavior in manage_beforeDelete in debug mode
+        # Manager user
+        self.setDebugMode(True)
+        newSecurityManager(None, system) # Manager
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        om._delObject(ob.getId())
+
+    def test_delObject_exception_debug(self):
+        # Test exception behavior in manage_beforeDelete in debug mode
+        # non-Manager user
+        # It's the only special case: we let exceptions propagate.
+        self.setDebugMode(True)
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        self.assertRaises(DeleteFailed, om._delObject, ob.getId())
+
+    def test_delObject_exception_debug_deep(self):
+        # Test exception behavior in manage_beforeDelete in debug mode
+        # non-Manager user
+        # Test for deep subobjects.
+        self.setDebugMode(True)
+        om1 = self._makeOne()
+        om2 = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om1._setObject('om2', om2, set_owner=False)
+        om2._setObject(ob.getId(), ob)
+        self.assertRaises(DeleteFailed, om1._delObject, 'om2')
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest( unittest.makeSuite( ObjectManagerTests ) )

lib/python/OFS/tests/testOrderSupport.py

@@ -17,6 +17,8 @@ class DummyObject(CopySource):
         return 1
     def manage_afterAdd(self, item, container):
         return
+    def manage_beforeDelete(self, item, container):
+        return
     def wl_isLocked(self):
         return 0