diff --git a/README.md b/README.md index f213c8b..8b898c9 100644 --- a/README.md +++ b/README.md @@ -21,16 +21,17 @@ with an optimized version. On a true object move (old parent ≠ new parent, i.e. cut-paste): 1. **`IObjectWillBeMovedEvent`** — instead of calling `unindexObject()`, the - object's current physical path is saved in the transaction-local registry - keyed by its ZODB `_p_oid`. The catalog entry is left untouched. + indexing queue is flushed (queued objects still report their old path) and + the object's current physical path is saved in the transaction-local + registry keyed by its ZODB `_p_oid`. The catalog entry is left untouched. 2. **`IObjectMovedEvent`** — the saved old path is retrieved, and `CatalogTool.moveObject()` (injected by this add-on) is called. It remaps `old_path → same RID → new_path` in the catalog's internal BTree structures, - then calls `reindexObject()` with **only the context-aware indexes**. + updates the modification date, then calls `reindexObject()` with **all + indexes except the contextless ones** (see below). -The net result: the RID is preserved, only the path-dependent and -security-dependent indexes are recomputed, and the full reindex of expensive -text/metadata indexes is skipped entirely. +The net result: the RID is preserved, and the contextless indexes +(`SearchableText` unless configured otherwise) are not recomputed. For **renames** (same parent, new id) the same path is followed — the object stays in the same container, only its path and id change. @@ -48,41 +49,45 @@ for large subtrees, objects can be evicted from the ZODB cache between the reindex. Transaction-attached data lives outside the ZODB object graph and is discarded automatically on commit or abort. -### Context-aware indexes +### Contextless indexes -Only indexes whose values change when an object moves need to be reindexed. -This add-on ships with two built-in providers: +The catalog entry is remapped on move (RID preserved) instead of being +unindexed and indexed again. Every index is then reindexed, except the +*contextless* ones: indexes whose value does not depend on the object's +location or security context. They are listed in the optional +`contextless_indexes` lines property of `portal_catalog`. -| Provider name | Indexes | -|---|---| -| `cmf.location` | `path`, `getId`, `id` | -| `cmf.security` | `allowedRolesAndUsers` | - -Third-party packages can contribute additional indexes by registering a named -utility providing `IContextAwareIndexProvider`: +**Without the property, `SearchableText` is treated as contextless**, where +most of the cost is. This differs from upstream CMFCore#161, which reindexes +every index when the property is missing. The property can be set from a +GenericSetup `catalog.xml`: ```xml - - + + + + + ``` -```python -# my.package/providers.py -from zope.interface import implementer -from experimental.catalogmoveopt.interfaces import IContextAwareIndexProvider +Set the property to an empty list to opt out of the default and reindex every +index on move. Add other indexes to the list only if their value cannot change +on move. -@implementer(IContextAwareIndexProvider) -class MyIndexProvider: - def getIndexNames(self): - return ("my_custom_index",) -``` +An index added later (e.g. through the ZMI) is reindexed on move unless it is +listed. Do not list an index if a subscriber changes its value on move. + +### Catalogs without `moveObject` -If no providers are registered the optimization is disabled and the stock -full-reindex path is used as a safe fallback. +`moveObject(object, old_path)` is optional for `ICatalogTool` implementations. +If the registered catalog does not provide it (e.g. `plone-pgcatalog`), the +stock unindex + index flow is used. `CatalogTool.moveObject` also updates the +modification date of the moved object, and returns `False` if nothing is +cataloged under `old_path` (the object is then indexed like a regular add). + +Note for `IIndexQueueProcessor` implementations: a move now queues a +`reindex` at the new path instead of an `unindex` of the old path followed by +an `index`. ## Installation @@ -96,9 +101,19 @@ dependencies = [ ] ``` -No further configuration is required. The add-on uses -`z3c.autoinclude.plugin` so its ZCML is loaded automatically when installed in -a Plone site. +The add-on uses `z3c.autoinclude.plugin` so its ZCML is loaded automatically +and the optimization is active as soon as the package is installed. + +Note that this changes behavior without any further step: `SearchableText` is +no longer reindexed on move. To keep reindexing it, set `contextless_indexes` +to an empty list (see above) or install the `uninstall` profile below. + +No profile is required: when `portal_catalog` has no `contextless_indexes` +property, `SearchableText` is skipped on move. Installing the +`experimental.catalogmoveopt:default` GenericSetup profile makes this explicit +by setting the property on `portal_catalog`, so it can be edited or exported. +The `experimental.catalogmoveopt:uninstall` profile sets it to an empty list, +so all indexes are reindexed on move. ## Compatibility @@ -135,8 +150,8 @@ way into the Plone/CMFCore ecosystem proper. Key references: - **[zopefoundation/Products.CMFCore#161](https://github.com/zopefoundation/Products.CMFCore/pull/161)** — the upstream CMFCore pull request (by the author of this package) that - proposes adding `CatalogTool.moveObject()` and the `IContextAwareIndexProvider` - interface directly to CMFCore. Once merged, this add-on will become + proposes adding `CatalogTool.moveObject()` and the `contextless_indexes` + catalog property directly to CMFCore. Once merged, this add-on will become unnecessary. ## Contribute diff --git a/news/+contextless.breaking b/news/+contextless.breaking new file mode 100644 index 0000000..1b2c638 --- /dev/null +++ b/news/+contextless.breaking @@ -0,0 +1,5 @@ +Align with the updated upstream Products.CMFCore#161: every index is now reindexed on move, except the ones listed in the `contextless_indexes` property of `portal_catalog`. +`IContextAwareIndexProvider` and the built-in providers are removed. +`CatalogTool.moveObject(object, old_path)` no longer takes `idxs`, updates the modification date and returns `True`/`False`. +The indexing queue is flushed before a move, and a leftover catalog entry at the new path is dropped. +Catalogs without `moveObject` keep the stock unindex + index behavior. diff --git a/news/+profile.feature b/news/+profile.feature new file mode 100644 index 0000000..9ec48b4 --- /dev/null +++ b/news/+profile.feature @@ -0,0 +1,2 @@ +Skip `SearchableText` on move by default, even without installing a profile: it applies when `portal_catalog` has no `contextless_indexes` property. An empty property opts out. +Add the `experimental.catalogmoveopt:default` GenericSetup profile, which sets the property explicitly, and an `uninstall` profile that sets it empty. diff --git a/src/experimental/catalogmoveopt/configure.zcml b/src/experimental/catalogmoveopt/configure.zcml index 3875821..b979895 100644 --- a/src/experimental/catalogmoveopt/configure.zcml +++ b/src/experimental/catalogmoveopt/configure.zcml @@ -1,27 +1,25 @@ - - - - - + + diff --git a/src/experimental/catalogmoveopt/providers.py b/src/experimental/catalogmoveopt/providers.py deleted file mode 100644 index 696da0d..0000000 --- a/src/experimental/catalogmoveopt/providers.py +++ /dev/null @@ -1,60 +0,0 @@ -from .interfaces import IContextAwareIndexProvider -from zope.component import getUtilitiesFor -from zope.interface import implementer - - -@implementer(IContextAwareIndexProvider) -class _BuiltinLocationIndexProvider: - """Default provider for location-based context-aware indexes. - - These indexes change when an object moves to a different container - (its path and id change). - """ - - def getIndexNames(self): - return ("path", "getId", "id") - - -@implementer(IContextAwareIndexProvider) -class _BuiltinSecurityIndexProvider: - """Default provider for security context-aware indexes. - - allowedRolesAndUsers must be recomputed whenever the object moves - into a differently-protected part of the tree. - """ - - def getIndexNames(self): - return ("allowedRolesAndUsers",) - - -@implementer(IContextAwareIndexProvider) -class _BuiltinTemporalIndexProvider: - """Default provider for modification-date context-aware indexes. - - Moving an object changes its context (path, security tree), which is a - meaningful content change from the perspective of caches and change-tracking - systems. Updating ``modified`` / ``Date`` on move ensures that cache keys - built on modification dates are correctly invalidated. - """ - - def getIndexNames(self): - return ("modified", "Date") - - -#: Module-level singletons registered as named utilities in configure.zcml. -builtin_location_provider = _BuiltinLocationIndexProvider() -builtin_security_provider = _BuiltinSecurityIndexProvider() -builtin_temporal_provider = _BuiltinTemporalIndexProvider() - - -def get_context_aware_indexes(): - """Return frozenset of all context-aware catalog index names. - - Aggregates all named IContextAwareIndexProvider utilities registered in - the global site manager. Returns an empty frozenset if no providers are - registered (which disables the optimization). - """ - indexes = set() - for _name, provider in getUtilitiesFor(IContextAwareIndexProvider): - indexes.update(provider.getIndexNames()) - return frozenset(indexes) diff --git a/src/experimental/catalogmoveopt/testing.py b/src/experimental/catalogmoveopt/testing.py index f568d3d..9a540d2 100644 --- a/src/experimental/catalogmoveopt/testing.py +++ b/src/experimental/catalogmoveopt/testing.py @@ -1,4 +1,5 @@ from plone.app.contenttypes.testing import PLONE_APP_CONTENTTYPES_FIXTURE +from plone.app.testing import applyProfile from plone.app.testing import IntegrationTesting from plone.app.testing import PloneSandboxLayer @@ -17,6 +18,9 @@ def setUpZope(self, app, configurationContext): apply_patches() + def setUpPloneSite(self, portal): + applyProfile(portal, "experimental.catalogmoveopt:default") + FIXTURE = Layer() diff --git a/tests/setup/test_patches_applied.py b/tests/setup/test_patches_applied.py index 12f9b13..c6b8e0a 100644 --- a/tests/setup/test_patches_applied.py +++ b/tests/setup/test_patches_applied.py @@ -32,3 +32,18 @@ def test_catalog_tool_has_move_object(self, integration): assert hasattr(CatalogTool, "moveObject") assert CatalogTool.moveObject is _catalog_tool_move_object + + +class TestProfile: + def test_contextless_indexes_installed(self, portal): + catalog = portal.portal_catalog + assert catalog.getProperty("contextless_indexes") == ("SearchableText",) + + def test_uninstall_empties_property(self, portal): + from Products.CMFCore.utils import getToolByName + + setup = getToolByName(portal, "portal_setup") + setup.runAllImportStepsFromProfile( + "profile-experimental.catalogmoveopt:uninstall" + ) + assert portal.portal_catalog.getProperty("contextless_indexes") == () diff --git a/tests/test_move_optimization.py b/tests/test_move_optimization.py index 3fff551..e44b079 100644 --- a/tests/test_move_optimization.py +++ b/tests/test_move_optimization.py @@ -147,57 +147,168 @@ def test_new_path_findable_after_move( assert catalog._catalog.uids.get(new_path) is not None -class TestOnlyContextAwareIndexesReindexed: - def test_rename_reindexes_only_declared_indexes(self, portal, doc, integration): - """reindexObject is called with only the context-aware index set.""" - from Acquisition import aq_base +def _capture_reindex(catalog, obj): + """Patch context manager helper: record ``idxs`` of reindexes of *obj*.""" + from Acquisition import aq_base + from unittest.mock import patch + + obj_base = aq_base(obj) + calls = [] + original = catalog.reindexObject + + # ``CMFCatalogAware.reindexObject`` forwards a ``uid`` keyword to the + # catalog tool, so the wrapper must accept it. Only calls for the moved + # object are recorded (renaming a child also reindexes the container). + def capturing_reindex(o, idxs=None, update_metadata=0, uid=None): + if aq_base(o) is obj_base: + calls.append(frozenset(idxs or ())) + return original(o, idxs=idxs, update_metadata=update_metadata, uid=uid) + + return calls, patch.object(catalog, "reindexObject", capturing_reindex) + + +class TestContextlessIndexes: + def test_rename_skips_contextless_indexes(self, portal, doc, integration): + """With the profile installed, SearchableText is not reindexed.""" from Products.CMFCore.utils import getToolByName - from unittest.mock import patch import plone.api catalog = getToolByName(portal, "portal_catalog") - doc_path_base = aq_base(doc) - reindex_calls = [] + calls, patcher = _capture_reindex(catalog, doc) - original = catalog.reindexObject + with patcher, plone.api.env.adopt_roles(["Manager"]): + plone.api.content.rename(obj=doc, new_id="test-doc-renamed") - # ``CMFCatalogAware.reindexObject`` forwards a ``uid`` keyword to the - # catalog tool, so the wrapper must accept it. We only record calls for - # the moved object itself (renaming a child also reindexes the container - # folder via a ContainerModifiedEvent — standard Plone behaviour that is - # unrelated to the move optimization). - def capturing_reindex(obj, idxs=None, update_metadata=0, uid=None): - if aq_base(obj) is doc_path_base: - reindex_calls.append(frozenset(idxs or ())) - return original(obj, idxs=idxs, update_metadata=update_metadata, uid=uid) + moved = [c for c in calls if "SearchableText" not in c and "path" in c] + assert moved, f"move must reindex all but contextless indexes; got {calls}" + assert frozenset(catalog.indexes()) - moved[0] == {"SearchableText"} + # Never a full reindex (empty idxs == all indexes). + assert all(calls), f"move must not full-reindex the object; got {calls}" - with ( - patch.object(catalog, "reindexObject", capturing_reindex), - plone.api.env.adopt_roles(["Manager"]), - ): + def test_rename_skips_default_without_property(self, portal, doc, integration): + """Without the property (no profile) SearchableText is still skipped.""" + from Products.CMFCore.utils import getToolByName + + import plone.api + + catalog = getToolByName(portal, "portal_catalog") + catalog.manage_delProperties(["contextless_indexes"]) + calls, patcher = _capture_reindex(catalog, doc) + + with patcher, plone.api.env.adopt_roles(["Manager"]): plone.api.content.rename(obj=doc, new_id="test-doc-renamed") - # The optimized move handler must reindex exactly the context-aware - # index set and nothing more. - expected = frozenset(( - "path", - "getId", - "id", - "allowedRolesAndUsers", - "modified", - "Date", - )) - assert expected in reindex_calls, ( - f"move handler must reindex the context-aware set; got {reindex_calls}" - ) - # Crucially it must never trigger a full reindex of the object (empty - # idxs == all indexes) — that is precisely the expensive operation the - # optimization exists to avoid. Any other reindex (e.g. the ordering - # support's single getObjPositionInParent reindex) is targeted, not full. - assert all(reindex_calls), ( - f"move must not full-reindex the object; got {reindex_calls}" - ) + assert frozenset(catalog.indexes()) - {"SearchableText"} in calls + + def test_rename_reindexes_all_with_empty_property(self, portal, doc, integration): + """An empty ``contextless_indexes`` opts out: every index is reindexed.""" + from Products.CMFCore.utils import getToolByName + + import plone.api + + catalog = getToolByName(portal, "portal_catalog") + catalog.manage_changeProperties(contextless_indexes=[]) + calls, patcher = _capture_reindex(catalog, doc) + + with patcher, plone.api.env.adopt_roles(["Manager"]): + plone.api.content.rename(obj=doc, new_id="test-doc-renamed") + + assert frozenset(catalog.indexes()) in calls + + def test_skipped_index_keeps_value_on_move(self, portal, doc, integration): + from Products.CMFCore.utils import getToolByName + + import plone.api + + from Products.CMFCore.indexing import processQueue + + catalog = getToolByName(portal, "portal_catalog") + processQueue() + rid = _rid(catalog, doc) + index = catalog._catalog.getIndex("SearchableText") + # Make the stored value differ from the object's, then move. + before = index.getEntryForObject(rid) + doc.title = "Changed without reindex" + + with plone.api.env.adopt_roles(["Manager"]): + plone.api.content.rename(obj=doc, new_id="test-doc-renamed") + + assert before + assert index.getEntryForObject(rid) == before + + def test_move_updates_modification_date(self, portal, doc, integration): + import plone.api + + before = doc.modified() + + with plone.api.env.adopt_roles(["Manager"]): + plone.api.content.rename(obj=doc, new_id="test-doc-renamed") + + assert doc.modified() > before + + +class TestMoveObject: + def test_returns_false_when_not_cataloged(self, portal, doc, integration): + from Products.CMFCore.utils import getToolByName + + catalog = getToolByName(portal, "portal_catalog") + assert catalog.moveObject(doc, "/plone/nowhere") is False + + def test_drops_stale_entry_at_new_path(self, portal, doc, integration): + from Products.CMFCore.utils import getToolByName + + catalog = getToolByName(portal, "portal_catalog") + new_path = "/".join(doc.getPhysicalPath()) + old_path = "/plone/old-doc" + rid = _rid(catalog, doc) + # Pretend doc was cataloged at old_path, with a leftover at new_path. + catalog._catalog.uids[old_path] = rid + catalog._catalog.uids.pop(new_path) + catalog.catalog_object(doc, new_path) + stale_rid = catalog._catalog.uids[new_path] + assert stale_rid != rid + + assert catalog.moveObject(doc, old_path) is True + + assert catalog._catalog.uids[new_path] == rid + assert old_path not in catalog._catalog.uids + assert stale_rid not in catalog._catalog.paths + + +class TestFallbackWithoutMoveObject: + def test_catalog_without_moveObject_unindexes(self, portal, integration): + """Catalogs lacking ``moveObject`` keep the stock unindex + index.""" + from experimental.catalogmoveopt.patches import handleContentishEvent + from OFS.interfaces import IObjectWillBeMovedEvent + from Products.CMFCore.interfaces import ICatalogTool + from unittest.mock import MagicMock + from zope.component import getSiteManager + from zope.interface import implementer + + @implementer(ICatalogTool) + class NoMoveCatalog: + pass + + sm = getSiteManager() + original = sm.getUtility(ICatalogTool) + sm.registerUtility(NoMoveCatalog(), ICatalogTool) + try: + ob = MagicMock() + ob._p_oid = b"\x00" * 8 + + @implementer(IObjectWillBeMovedEvent) + class Event: + oldParent = object() + newParent = object() + + event = Event() + + handleContentishEvent(ob, event) + finally: + sm.registerUtility(original, ICatalogTool) + + ob.unindexObject.assert_called_once() class TestFallbackOnNoOid: @@ -207,7 +318,6 @@ def test_object_without_oid_falls_back_to_full_reindex(self, portal, integration from OFS.interfaces import IObjectWillBeMovedEvent from unittest.mock import MagicMock from unittest.mock import patch - from zope.lifecycleevent.interfaces import IObjectMovedEvent ob = MagicMock() ob._p_oid = None # no OID @@ -216,8 +326,6 @@ def test_object_without_oid_falls_back_to_full_reindex(self, portal, integration will_be_moved = MagicMock(spec=IObjectWillBeMovedEvent) will_be_moved.oldParent = MagicMock() will_be_moved.newParent = MagicMock() - IObjectWillBeMovedEvent.providedBy = lambda e: e is will_be_moved - IObjectMovedEvent.providedBy = lambda e: False with ( patch( @@ -247,3 +355,27 @@ def test_object_without_oid_falls_back_to_full_reindex(self, portal, integration handleContentishEvent(ob, will_be_moved) ob.unindexObject.assert_called_once() + + +class TestPendingQueue: + def test_rename_with_pending_reindex_leaves_single_entry( + self, portal, doc, integration + ): + """A reindex queued before the rename must not create a second entry.""" + from Products.CMFCore.indexing import processQueue + from Products.CMFCore.utils import getToolByName + + import plone.api + + catalog = getToolByName(portal, "portal_catalog") + processQueue() + rid = _rid(catalog, doc) + doc.reindexObject() # queued, not yet processed + + with plone.api.env.adopt_roles(["Manager"]): + plone.api.content.rename(obj=doc, new_id="test-doc-renamed") + processQueue() + + cat = catalog._catalog + assert len(cat.uids) == len(cat.paths) + assert _rid(catalog, doc) == rid diff --git a/tests/test_providers.py b/tests/test_providers.py deleted file mode 100644 index 61f14b8..0000000 --- a/tests/test_providers.py +++ /dev/null @@ -1,80 +0,0 @@ -"""Unit tests for get_context_aware_indexes() and IContextAwareIndexProvider.""" - -from experimental.catalogmoveopt.interfaces import IContextAwareIndexProvider -from experimental.catalogmoveopt.providers import _BuiltinLocationIndexProvider -from experimental.catalogmoveopt.providers import _BuiltinSecurityIndexProvider -from experimental.catalogmoveopt.providers import get_context_aware_indexes -from zope.component import getGlobalSiteManager -from zope.interface import implementer - -import pytest - - -@implementer(IContextAwareIndexProvider) -class _CustomProvider: - def getIndexNames(self): - return ("my_custom_index",) - - -@pytest.fixture(autouse=True) -def clean_gsm(): - """Ensure no stray test utilities leak between tests.""" - yield - gsm = getGlobalSiteManager() - for name in ("_test.location", "_test.security", "_test.custom"): - gsm.unregisterUtility(provided=IContextAwareIndexProvider, name=name) - - -class TestGetContextAwareIndexes: - def test_empty_when_no_providers(self): - """With no utilities registered the result is an empty frozenset.""" - assert get_context_aware_indexes() == frozenset() - - def test_location_provider_indexes(self): - gsm = getGlobalSiteManager() - provider = _BuiltinLocationIndexProvider() - gsm.registerUtility(provider, IContextAwareIndexProvider, "_test.location") - result = get_context_aware_indexes() - assert "path" in result - assert "getId" in result - assert "id" in result - - def test_security_provider_indexes(self): - gsm = getGlobalSiteManager() - provider = _BuiltinSecurityIndexProvider() - gsm.registerUtility(provider, IContextAwareIndexProvider, "_test.security") - result = get_context_aware_indexes() - assert "allowedRolesAndUsers" in result - - def test_providers_merged(self): - """Indexes from multiple providers are merged into one frozenset.""" - gsm = getGlobalSiteManager() - gsm.registerUtility( - _BuiltinLocationIndexProvider(), - IContextAwareIndexProvider, - "_test.location", - ) - gsm.registerUtility( - _BuiltinSecurityIndexProvider(), - IContextAwareIndexProvider, - "_test.security", - ) - result = get_context_aware_indexes() - assert result >= frozenset(("path", "getId", "id", "allowedRolesAndUsers")) - - def test_custom_provider_included(self): - gsm = getGlobalSiteManager() - gsm.registerUtility( - _CustomProvider(), IContextAwareIndexProvider, "_test.custom" - ) - result = get_context_aware_indexes() - assert "my_custom_index" in result - - def test_result_is_frozenset(self): - gsm = getGlobalSiteManager() - gsm.registerUtility( - _BuiltinLocationIndexProvider(), - IContextAwareIndexProvider, - "_test.location", - ) - assert isinstance(get_context_aware_indexes(), frozenset)