Skip to content

Optimize reindexObjectSecurity: bypass processQueue mid-request#155

Merged
jensens merged 4 commits into
masterfrom
fix-processqueue-reindexsecurity
Feb 19, 2026
Merged

Optimize reindexObjectSecurity: bypass processQueue mid-request#155
jensens merged 4 commits into
masterfrom
fix-processqueue-reindexsecurity

Conversation

@jensens
Copy link
Copy Markdown
Member

@jensens jensens commented Feb 17, 2026

Summary

Builds on #154 (revert of tree-walk approach).

  • Adds _unrestrictedSearchResults to CatalogTool — calls ZCatalog.searchResults directly, bypassing processQueue()
  • Updates reindexObjectSecurity to use the new method with getattr fallback for custom catalog tools

Follows the existing _indexObject/_reindexObject/_unindexObject convention for queue-bypassing counterparts.

Objects still in the indexing queue don't need to be found: their pending INDEX operations will include the current security state when processed at transaction commit time. see #152 (comment)

Benchmark

With a clean queue (no pending operations), performance is identical (~0.5ms).
With a dirty queue (pending reindex operations from mid-request modifications), bypassing processQueue() avoids a costly flush:

Config Objects Old (flush) New (bypass) Saved
Wide & shallow 421 967ms 73ms 894ms
Balanced 400 990ms 88ms 902ms
Deep & narrow 364 939ms 105ms 834ms
Wide flat 101 216ms 9ms 207ms

Fixes #152

@jensens jensens force-pushed the fix-processqueue-reindexsecurity branch from b6fdfaf to 2c96c0f Compare February 17, 2026 14:07
@jensens jensens marked this pull request as ready for review February 17, 2026 14:17
@jensens jensens changed the title Fix reindexObjectSecurity: bypass processQueue mid-request Optimize reindexObjectSecurity: bypass processQueue mid-request Feb 17, 2026
Add CatalogTool._unrestrictedSearchResults that calls
ZCatalog.searchResults directly without processQueue().
Use it in reindexObjectSecurity with a getattr fallback
for custom catalog tools.

Refs #152
Copy link
Copy Markdown
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine.

@jensens
Copy link
Copy Markdown
Member Author

jensens commented Feb 18, 2026

@davisagli At this point (no offense) I would like to have a second opinion before merge

@davisagli
Copy link
Copy Markdown
Member

@jensens I would like to review this but it might take me a day or two to get to it.

Copy link
Copy Markdown
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be okay (aside from the possible warnings re deleted objects that I mentioned in #152 (comment))

@jensens jensens enabled auto-merge (squash) February 19, 2026 15:13
@jensens jensens merged commit 8742a68 into master Feb 19, 2026
14 checks passed
@jensens jensens deleted the fix-processqueue-reindexsecurity branch February 19, 2026 15:14
@mauritsvanrees
Copy link
Copy Markdown
Member

I have released this in Products.CMFCore 3.9 and have updated the Plone 6.2 coredev buildout to use it. But since then, Jenkins fails and some GitHub Actions fail.

They pass again when I use a checkout of Products.CMFCore and undo the changes of this PR. Most easily: in CatalogAware.py make this change:

- search = getattr(catalog, '_unrestrictedSearchResults',
+ search = getattr(catalog, '_XXX',
                   catalog.unrestrictedSearchResults)

It could be that the broken tests just need to be fixed, but there are 6 of them, divided over 4 packages, so maybe the change really is not sufficient.

Sample failures:

$ bin/test -s Products.CMFPlone -m testNavigationView
...
Failure in test testCreateTopLevelTabs (Products.CMFPlone.tests.testNavigationView.TestCatalogPortalTabs.testCreateTopLevelTabs)
Traceback (most recent call last):
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 669, in run
    self._callTestMethod(testMethod)
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 615, in _callTestMethod
    result = method()
  File "/Users/maurits/community/plone-coredev/6.2/src/Products.CMFPlone/src/Products/CMFPlone/tests/testNavigationView.py", line 165, in testCreateTopLevelTabs
    self.assertEqual(len(tabs), 7)
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 925, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 918, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 5 != 7

and:

$ bin/test -s plone.api -m test_content
...
Failure in test test_create_raises_unicodedecodeerror (plone.api.tests.test_content.TestPloneApiContent.test_create_raises_unicodedecodeerror)
Traceback (most recent call last):
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 669, in run
    self._callTestMethod(testMethod)
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 615, in _callTestMethod
    result = method()
  File "/Users/maurits/community/plone-coredev/6.2/src/plone.api/src/plone/api/tests/test_content.py", line 354, in test_create_raises_unicodedecodeerror
    with self.assertRaises(UnicodeDecodeError) as ude:
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 272, in __exit__
    self._raiseFailure("{} not raised".format(exc_name))
  File "/Users/maurits/.pyenv/versions/3.14.2/lib/python3.14/unittest/case.py", line 209, in _raiseFailure
    raise self.test_case.failureException(msg)
AssertionError: UnicodeDecodeError not raised

@jensens Can you have a look please?

Meanwhile I have reverted the CMFCore pin in Plone 6.2.

@mauritsvanrees
Copy link
Copy Markdown
Member

I quickly checked. This log line that got changed from warning to debug level, does not get triggered in the failing tests: I put a breakpoint in there, but it never happened. So we have no indication of a problem there.

jensens added a commit that referenced this pull request Mar 23, 2026
When the indexing queue is processed outside a request context
(e.g. in the before_commit transaction hook), getSite() returns
None. IndexableObjectWrapper then falls back to the global site
manager, which lacks locally registered indexer adapters. This
causes attributes like exclude_from_nav to silently not be indexed.

Previously masked because processQueue() was called mid-request
(via reindexObjectSecurity -> unrestrictedSearchResults) when the
site was always set. Since #155 bypasses processQueue in
reindexObjectSecurity, more operations are deferred to
before_commit where the site may not be set.

Fix: PortalCatalogProcessor._ensure_site() walks the object's
acquisition chain to find and activate a site before each catalog
operation.

Refs: plone/buildout.coredev#1080

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jensens added a commit that referenced this pull request Mar 23, 2026
When the indexing queue is processed outside a request context
(e.g. in the before_commit transaction hook), getSite() returns
None. IndexableObjectWrapper then falls back to the global site
manager, which lacks locally registered indexer adapters. This
causes attributes like exclude_from_nav to silently not be indexed.

Previously masked because processQueue() was called mid-request
(via reindexObjectSecurity -> unrestrictedSearchResults) when the
site was always set. Since #155 bypasses processQueue in
reindexObjectSecurity, more operations are deferred to
before_commit where the site may not be set.

Fix: PortalCatalogProcessor._ensure_site() walks the object's
acquisition chain to find and activate a site before each catalog
operation.

Refs: plone/buildout.coredev#1080
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reindexObjectSecurity: avoid transaction.commit in the middle of a request.

3 participants