Optimize reindexObjectSecurity: bypass processQueue mid-request

CHANGES.rst

@@ -4,6 +4,12 @@ Products.CMFCore Changelog
 3.9 (unreleased)
 ----------------
 
+- Fix ``reindexObjectSecurity`` to not trigger ``processQueue()`` mid-request.
+  Uses ``_unrestrictedSearchResults`` that bypasses the indexing queue flush,
+  consistent with existing ``_indexObject``/``_reindexObject``/
+  ``_unindexObject`` convention.
+  (`#152 <https://github.com/zopefoundation/Products.CMFCore/issues/152>`_)
+
 - Move package metadata from setup.py to pyproject.toml.
 
 

src/Products/CMFCore/CMFCatalogAware.py

@@ -98,7 +98,24 @@ def reindexObject(self, idxs=[], update_metadata=1, uid=None):
 
     @security.protected(ModifyPortalContent)
     def reindexObjectSecurity(self, skip_self=False):
-        """ Reindex security-related indexes on the object.
+        """Reindex security-related indexes on the object.
+
+        Uses ``_unrestrictedSearchResults`` instead of
+        ``unrestrictedSearchResults`` to avoid triggering
+        ``processQueue()`` mid-request.  This is safe because:
+
+        1. **Pre-existing objects** are already in the catalog, the
+           search finds them, and we queue a security reindex.
+           ``optimize()`` merges with any other pending REINDEX.
+
+        2. **Objects modified in this transaction** are still in the
+           catalog under their existing path.  The search finds them
+           and ``optimize()`` merges both REINDEXes.
+
+        3. **Objects created in this transaction** have pending INDEX
+           operations that will be processed at commit time and will
+           index *all* attributes — including ``allowedRolesAndUsers``
+           — using the current security state.
         """
         catalog = self._getCatalogTool()
         if catalog is None:
@@ -107,7 +124,9 @@ def reindexObjectSecurity(self, skip_self=False):
 
         # XXX if _getCatalogTool() is overriden we will have to change
         # this method for the sub-objects.
-        for brain in catalog.unrestrictedSearchResults(path=path):
+        search = getattr(catalog, '_unrestrictedSearchResults',
+                         catalog.unrestrictedSearchResults)
+        for brain in search(path=path):
             brain_path = brain.getPath()
             if brain_path == path and skip_self:
                 continue
@@ -118,11 +137,10 @@ def reindexObjectSecurity(self, skip_self=False):
                 # don't fail on catalog inconsistency
                 continue
             if ob is None:
-                # BBB: Ignore old references to deleted objects.
-                # Can happen only when using
-                # catalog-getObject-raises off in Zope 2.8
-                logger.warning('reindexObjectSecurity: Cannot get %s from '
-                               'catalog', brain_path)
+                # Expected when objects were deleted in this transaction
+                # but the queue hasn't been flushed yet.
+                logger.debug('reindexObjectSecurity: Cannot get %s from '
+                             'catalog (pending unindex)', brain_path)
                 continue
             s = getattr(ob, '_p_changed', 0)
             ob.reindexObject(idxs=self._cmf_security_indexes,

src/Products/CMFCore/CatalogTool.py

@@ -272,6 +272,16 @@ def unrestrictedSearchResults(self, REQUEST=None, **kw):
         processQueue()
         return ZCatalog.searchResults(self, REQUEST, **kw)
 
+    @security.private
+    def _unrestrictedSearchResults(self, REQUEST=None, **kw):
+        """Like unrestrictedSearchResults but without processQueue().
+
+        Only finds objects already in the catalog.  Used by
+        reindexObjectSecurity to avoid flushing the indexing queue
+        mid-request.
+        """
+        return ZCatalog.searchResults(self, REQUEST, **kw)
+
     def __url(self, ob):
         return '/'.join(ob.getPhysicalPath())
 

src/Products/CMFCore/tests/test_CMFCatalogAware.py

@@ -13,6 +13,7 @@
 """Unit tests for CMFCatalogAware.
 """
 
+import logging
 import unittest
 
 import transaction
@@ -110,6 +111,9 @@ def unrestrictedSearchResults(self, path):
             res.append(self.brain_class(ob, obpath))
         return res
 
+    def _unrestrictedSearchResults(self, **kw):
+        return self.unrestrictedSearchResults(**kw)
+
 
 class DummyWorkflowTool(SimpleItem):
 
@@ -218,19 +222,27 @@ def test_reindexObjectSecurity_missing_raise(self):
 
     def test_reindexObjectSecurity_missing_noraise(self):
         # Raising disabled
-        self._catch_log_errors(subsystem='CMFCore.CMFCatalogAware')
-        foo = self.site.foo
-        missing = TheClass('missing').__of__(foo)
-        missing.GETOBJECT_RAISES = False
-        cat = self.ctool
-        cat.setObs([foo, missing])
-        foo.reindexObjectSecurity()
-        self.assertEqual(
-            cat.log,
-            ['reindex /site/foo %s 0' % str(CMF_SECURITY_INDEXES)])
-        self.assertFalse(foo.notified)
-        self.assertFalse(missing.notified)
-        self.assertEqual(len(self.logged), 1)  # logging because no raise
+        self._catch_log_errors(ignored_level=logging.DEBUG,
+                               subsystem='CMFCore.CMFCatalogAware')
+        test_logger = logging.getLogger('CMFCore.CMFCatalogAware')
+        old_level = test_logger.level
+        test_logger.setLevel(logging.DEBUG)
+        try:
+            foo = self.site.foo
+            missing = TheClass('missing').__of__(foo)
+            missing.GETOBJECT_RAISES = False
+            cat = self.ctool
+            cat.setObs([foo, missing])
+            foo.reindexObjectSecurity()
+            self.assertEqual(
+                cat.log,
+                ['reindex /site/foo %s 0' % str(CMF_SECURITY_INDEXES)])
+            self.assertFalse(foo.notified)
+            self.assertFalse(missing.notified)
+            # debug log for pending unindex
+            self.assertEqual(len(self.logged), 1)
+        finally:
+            test_logger.setLevel(old_level)
 
     def test_catalog_tool(self):
         foo = self.site.foo