diff --git a/CHANGES.md b/CHANGES.md index 3f9ac64..4995443 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,24 @@ # Changelog +## 1.0.0b63 + +### Changed + +- ``PGCatalogBrain`` no longer stores a reference to the catalog tool + and no longer consults ``self._catalog.REQUEST`` for URL rendering. + ``getURL`` uses ``zope.globalrequest.getRequest()``; ``getObject`` + and ``_unrestrictedGetObject`` resolve a traversal root lazily via + a new ``_traversal_root()`` helper (``getSite().getPhysicalRoot()`` + first, ``getRequest().PARENTS[-1]`` as fallback). This keeps brains + catalog-independent so callers can cache / pickle / re-queue them + without dragging the Acquisition chain along. The catalog + reference that the ZODB-prefetch batch needs has moved onto the + transient ``CatalogSearchResults`` container, where it belongs. + + The ``catalog=`` keyword on ``PGCatalogBrain.__init__`` is accepted + but ignored — kept until the next major version for signature + compatibility with direct callers. + ## 1.0.0b62 ### Fixed diff --git a/src/plone/pgcatalog/brain.py b/src/plone/pgcatalog/brain.py index 339c1b5..abb7b5d 100644 --- a/src/plone/pgcatalog/brain.py +++ b/src/plone/pgcatalog/brain.py @@ -12,6 +12,8 @@ from plone.pgcatalog.extraction import decode_meta from Products.ZCatalog.interfaces import ICatalogBrain from ZODB.utils import p64 +from zope.component.hooks import getSite +from zope.globalrequest import getRequest from zope.interface import implementer from zope.interface.common.sequence import IFiniteSequence from ZTUtils.Lazy import Lazy @@ -25,12 +27,40 @@ _PREFETCH_BATCH = int(os.environ.get("PGCATALOG_PREFETCH_BATCH", "100")) +def _traversal_root(): + """Return a traversable Zope root (Application), or None. + + Brains avoid holding a reference to the catalog tool so they can + be cached / pickled / re-queued without dragging the Acquisition + chain along. The root is resolved at call time via + ``zope.component.hooks.getSite`` first (works in request and + thread contexts that have a local-site hook), then falls back to + the Zope traversal root from ``zope.globalrequest.getRequest``. + + Returns ``None`` when neither is available — the caller (``getObject`` + / ``_unrestrictedGetObject``) then reports "object not found" + rather than raising, matching the long-standing ZCatalog contract. + """ + site = getSite() + if site is not None: + return site.getPhysicalRoot() + request = getRequest() + if request is not None: + parents = getattr(request, "PARENTS", None) + if parents: + return parents[-1] + return None + + @implementer(ICatalogBrain) class PGCatalogBrain: """Lightweight catalog brain backed by a PostgreSQL row. Implements the essential ICatalogBrain interface without requiring - Zope Acquisition or Record infrastructure. + Zope Acquisition or Record infrastructure. Deliberately does NOT + store a reference to the catalog tool — traversal resolves the + portal root lazily via ``_traversal_root()``, so a brain remains + safe to cache / pickle across request boundaries. Supports two modes: - **Eager** (default): row contains ``idx`` dict, metadata access is direct. @@ -40,14 +70,18 @@ class PGCatalogBrain: Args: row: dict with keys zoid, path (and optionally idx) - catalog: reference to the catalog tool (for getObject traversal) + catalog: accepted for backward compatibility with callers still + passing the catalog tool — ignored. Kept until the next + major version for signature compatibility. """ - __slots__ = ("_catalog", "_result_set", "_row") + __slots__ = ("_result_set", "_row") def __init__(self, row, catalog=None): + # ``catalog`` is accepted for backward compat with callers + # still passing the tool — intentionally unused. + del catalog self._row = row - self._catalog = catalog self._result_set = None # -- ICatalogBrain methods ----------------------------------------------- @@ -59,13 +93,15 @@ def getPath(self): def getURL(self, relative=0): """Generate a URL for this record. - In standalone mode (no request), returns the path. - When integrated with Zope (Phase 6), uses request.physicalPathToURL. + Uses ``zope.globalrequest.getRequest()`` — no reference to the + catalog tool. Keeping brains catalog-independent lets callers + cache / pickle / re-queue them without dragging the acquisition + chain along. Returns the plain path in standalone / script + mode when no request is active. """ - if self._catalog is not None: - request = getattr(self._catalog, "REQUEST", None) - if request is not None: - return request.physicalPathToURL(self.getPath(), relative) + request = getRequest() + if request is not None: + return request.physicalPathToURL(self.getPath(), relative) return self.getPath() def _maybe_prefetch(self): @@ -83,10 +119,7 @@ def _maybe_prefetch(self): result_set._maybe_prefetch_objects(self) def getObject(self): - """Return the object for this record. - - Requires a catalog with traversal support (Phase 6 integration). - Returns None if the object cannot be found. + """Return the object for this record, or None if not found. Mirrors upstream ``Products.ZCatalog.CatalogBrains.AbstractCatalogBrain`` semantics: the catalog filter already authorized access to the @@ -96,30 +129,31 @@ def getObject(self): e.g. an internal calendar container publishing public events) raise ``Unauthorized`` on the parent even though the user is allowed to see the target. + + Resolves the traversal root lazily via ``_traversal_root()`` so + brains stay catalog-independent (cache-friendly). """ - if self._catalog is None: + root = _traversal_root() + if root is None: return None self._maybe_prefetch() path = self.getPath().split("/") if not path: return None try: - parent = ( - self._catalog.unrestrictedTraverse(path[:-1]) - if len(path) > 1 - else self._catalog - ) + parent = root.unrestrictedTraverse(path[:-1]) if len(path) > 1 else root return parent.restrictedTraverse(path[-1]) except (KeyError, AttributeError): return None def _unrestrictedGetObject(self): - """Return the object without security checks.""" - if self._catalog is None: + """Return the object without security checks, or None if not found.""" + root = _traversal_root() + if root is None: return None self._maybe_prefetch() try: - return self._catalog.unrestrictedTraverse(self.getPath()) + return root.unrestrictedTraverse(self.getPath()) except (KeyError, AttributeError): return None @@ -274,7 +308,7 @@ class CatalogSearchResults(Lazy): using the same connection (and thus the same REPEATABLE READ snapshot). """ - def __init__(self, brains, actual_result_count=None, conn=None): + def __init__(self, brains, actual_result_count=None, conn=None, catalog=None): self._brains = list(brains) self.actual_result_count = ( actual_result_count @@ -282,6 +316,7 @@ def __init__(self, brains, actual_result_count=None, conn=None): else len(self._brains) ) self._conn = conn + self._catalog = catalog # used only for ZODB prefetch (needs _p_jar) self._idx_loaded = conn is None # eager if no conn self._prefetched_ranges = set() # set of (start, end) tuples # Build index for O(1) brain → position lookup (used by prefetch) @@ -360,11 +395,12 @@ def _maybe_prefetch_objects(self, brain): if not batch: return - # Get storage from the catalog reference on the first brain. - catalog = batch[0]._catalog - if catalog is None: + # Get storage from the result-set's catalog reference (brains + # are catalog-independent — the tool lives only on the + # transient result set). + if self._catalog is None: return - jar = getattr(catalog, "_p_jar", None) + jar = getattr(self._catalog, "_p_jar", None) if jar is None: return storage = getattr(jar, "_storage", None) @@ -390,6 +426,7 @@ def __getitem__(self, index): result, self.actual_result_count, conn=self._conn, + catalog=self._catalog, ) # Re-wire brains to new result set if idx not yet loaded if not self._idx_loaded: diff --git a/src/plone/pgcatalog/search.py b/src/plone/pgcatalog/search.py index 541b3d8..88b1ab6 100644 --- a/src/plone/pgcatalog/search.py +++ b/src/plone/pgcatalog/search.py @@ -104,10 +104,19 @@ def _unrestrictedGetObject(self): def _build_results(rows, actual_count, catalog, lazy_conn): - """Build CatalogSearchResults from raw rows.""" - brains = [PGCatalogBrain(row, catalog=catalog) for row in rows] + """Build CatalogSearchResults from raw rows. + + Brains are catalog-independent (catalog-independent brains cache + cleanly across request boundaries — see #156). The catalog ref + is only kept on the transient result set, where ZODB-prefetch + needs it to reach the storage jar. + """ + brains = [PGCatalogBrain(row) for row in rows] results = CatalogSearchResults( - brains, actual_result_count=actual_count, conn=lazy_conn + brains, + actual_result_count=actual_count, + conn=lazy_conn, + catalog=catalog, ) if lazy_conn is not None: for brain in brains: diff --git a/tests/test_brain.py b/tests/test_brain.py index 4b0d74c..26e7454 100644 --- a/tests/test_brain.py +++ b/tests/test_brain.py @@ -29,24 +29,47 @@ def test_get_path(self): brain = PGCatalogBrain(_make_row(path="/plone/folder/doc")) assert brain.getPath() == "/plone/folder/doc" - def test_get_url_without_catalog(self): + def test_get_url_uses_globalrequest(self): + """getURL resolves the request via ``zope.globalrequest.getRequest()`` + — the catalog tool is intentionally NOT consulted, so brains can + be cached/pickled/re-queued without dragging the acquisition + chain along. + """ + from unittest import mock + + fake_request = mock.Mock() + fake_request.physicalPathToURL.return_value = "http://example.com/plone/doc" brain = PGCatalogBrain(_make_row(path="/plone/doc")) - assert brain.getURL() == "/plone/doc" - def test_get_url_with_request(self): - from unittest import mock + with mock.patch("plone.pgcatalog.brain.getRequest", return_value=fake_request): + assert brain.getURL() == "http://example.com/plone/doc" - catalog = mock.Mock() - catalog.REQUEST.physicalPathToURL.return_value = "http://example.com/plone/doc" - brain = PGCatalogBrain(_make_row(path="/plone/doc"), catalog=catalog) - assert brain.getURL() == "http://example.com/plone/doc" + def test_get_url_without_request_returns_path(self): + """Standalone / script mode: no request available, return the + raw path. + """ + from unittest import mock - def test_get_url_catalog_without_request(self): + brain = PGCatalogBrain(_make_row(path="/plone/doc")) + with mock.patch("plone.pgcatalog.brain.getRequest", return_value=None): + assert brain.getURL() == "/plone/doc" + + def test_get_url_ignores_catalog_request(self): + """Regression guard: even if the catalog carries a REQUEST + attribute, getURL must not consult it — it goes exclusively + through getRequest(). Prevents the acquisition-based lookup + from sneaking back in via copy-paste. + """ from unittest import mock - catalog = mock.Mock(spec=[]) + catalog = mock.Mock() + # Deliberately different URL so the test fails loudly if the + # code ever reads catalog.REQUEST. + catalog.REQUEST.physicalPathToURL.return_value = "http://WRONG-via-catalog/doc" brain = PGCatalogBrain(_make_row(path="/plone/doc"), catalog=catalog) - assert brain.getURL() == "/plone/doc" + with mock.patch("plone.pgcatalog.brain.getRequest", return_value=None): + # No global request → path, NOT the catalog's mocked URL. + assert brain.getURL() == "/plone/doc" def test_get_rid(self): brain = PGCatalogBrain(_make_row(zoid=42)) @@ -56,112 +79,115 @@ def test_data_record_id(self): brain = PGCatalogBrain(_make_row(zoid=42)) assert brain.data_record_id_ == 42 - def test_get_object_without_catalog(self): + def _mock_root(self, root): + """Patch ``_traversal_root`` to return *root*.""" + from unittest import mock + + return mock.patch("plone.pgcatalog.brain._traversal_root", return_value=root) + + def test_get_object_without_traversal_root(self): + """No site hook and no request — getObject returns None.""" brain = PGCatalogBrain(_make_row()) - assert brain.getObject() is None + with self._mock_root(None): + assert brain.getObject() is None - def test_get_object_with_catalog(self): + def test_get_object_with_traversal_root(self): """getObject traverses the parent unrestricted and the leaf restricted. Matches upstream ``AbstractCatalogBrain.getObject``: the catalog filter already authorized access to the target, so intermediate containers are bypassed — only the final object is permission- - checked. + checked. The traversal root is resolved at call time via + ``_traversal_root()`` (``getSite().getPhysicalRoot()`` + + ``getRequest()`` fallback) — brains do NOT hold a reference + to the catalog tool (cache-friendly). """ from unittest import mock obj = mock.Mock() parent = mock.Mock() parent.restrictedTraverse.return_value = obj - catalog = mock.Mock() - catalog.unrestrictedTraverse.return_value = parent - brain = PGCatalogBrain( - _make_row(path="/plone/kalender/event-xyz"), catalog=catalog - ) + root = mock.Mock() + root.unrestrictedTraverse.return_value = parent + brain = PGCatalogBrain(_make_row(path="/plone/kalender/event-xyz")) - assert brain.getObject() is obj - catalog.unrestrictedTraverse.assert_called_once_with(["", "plone", "kalender"]) + with self._mock_root(root): + assert brain.getObject() is obj + root.unrestrictedTraverse.assert_called_once_with(["", "plone", "kalender"]) parent.restrictedTraverse.assert_called_once_with("event-xyz") def test_get_object_restricted_only_on_leaf(self): - """Regression for #141 — parent folder with stricter permissions. - - Production symptom: - AccessControl.unauthorized.Unauthorized: You are not allowed - to access 'kalender' in this context - - A common Plone pattern is to have a restricted parent container - (e.g. ``kalender/``) that publishes individual public items. - The catalog filter already authorized the leaf, so traversal - must not re-check the parent. - """ + """Regression for #141 — parent folder with stricter permissions.""" from AccessControl.unauthorized import Unauthorized from unittest import mock obj = mock.Mock() parent = mock.Mock() parent.restrictedTraverse.return_value = obj - catalog = mock.Mock() + root = mock.Mock() # restrictedTraverse on the parent path would raise Unauthorized # — the buggy implementation called restrictedTraverse on the # full path, which bubbles through ``kalender``. - catalog.restrictedTraverse.side_effect = Unauthorized( + root.restrictedTraverse.side_effect = Unauthorized( "You are not allowed to access 'kalender'" ) - catalog.unrestrictedTraverse.return_value = parent - brain = PGCatalogBrain( - _make_row(path="/plone/kalender/event-xyz"), catalog=catalog - ) + root.unrestrictedTraverse.return_value = parent + brain = PGCatalogBrain(_make_row(path="/plone/kalender/event-xyz")) - # Must succeed — parent traversal is unrestricted. - assert brain.getObject() is obj + with self._mock_root(root): + assert brain.getObject() is obj def test_get_object_traversal_error(self): from unittest import mock - catalog = mock.Mock() - catalog.unrestrictedTraverse.side_effect = KeyError("not found") - brain = PGCatalogBrain(_make_row(path="/plone/doc"), catalog=catalog) - assert brain.getObject() is None + root = mock.Mock() + root.unrestrictedTraverse.side_effect = KeyError("not found") + brain = PGCatalogBrain(_make_row(path="/plone/doc")) + with self._mock_root(root): + assert brain.getObject() is None def test_get_object_site_root_path(self): - """Path ``/plone`` splits to ``['', 'plone']`` — site root is the + """Path ``/plone`` splits to ``['', 'plone']`` — root is the "parent" (traversed unrestricted to ``['']``), leaf is ``plone`` (restricted). Matches upstream ZCatalog semantics. """ from unittest import mock obj = mock.Mock() + root_via = mock.Mock() + root_via.restrictedTraverse.return_value = obj root = mock.Mock() - root.restrictedTraverse.return_value = obj - catalog = mock.Mock() - catalog.unrestrictedTraverse.return_value = root - brain = PGCatalogBrain(_make_row(path="/plone"), catalog=catalog) + root.unrestrictedTraverse.return_value = root_via + brain = PGCatalogBrain(_make_row(path="/plone")) - assert brain.getObject() is obj - catalog.unrestrictedTraverse.assert_called_once_with([""]) - root.restrictedTraverse.assert_called_once_with("plone") + with self._mock_root(root): + assert brain.getObject() is obj + root.unrestrictedTraverse.assert_called_once_with([""]) + root_via.restrictedTraverse.assert_called_once_with("plone") - def test_unrestricted_get_object_without_catalog(self): + def test_unrestricted_get_object_without_traversal_root(self): brain = PGCatalogBrain(_make_row()) - assert brain._unrestrictedGetObject() is None + with self._mock_root(None): + assert brain._unrestrictedGetObject() is None - def test_unrestricted_get_object_with_catalog(self): + def test_unrestricted_get_object_with_traversal_root(self): from unittest import mock obj = mock.Mock() - catalog = mock.Mock() - catalog.unrestrictedTraverse.return_value = obj - brain = PGCatalogBrain(_make_row(path="/plone/doc"), catalog=catalog) - assert brain._unrestrictedGetObject() is obj + root = mock.Mock() + root.unrestrictedTraverse.return_value = obj + brain = PGCatalogBrain(_make_row(path="/plone/doc")) + with self._mock_root(root): + assert brain._unrestrictedGetObject() is obj def test_unrestricted_get_object_traversal_error(self): from unittest import mock - catalog = mock.Mock() - catalog.unrestrictedTraverse.side_effect = AttributeError("nope") - brain = PGCatalogBrain(_make_row(path="/plone/doc"), catalog=catalog) - assert brain._unrestrictedGetObject() is None + root = mock.Mock() + root.unrestrictedTraverse.side_effect = AttributeError("nope") + brain = PGCatalogBrain(_make_row(path="/plone/doc")) + with self._mock_root(root): + assert brain._unrestrictedGetObject() is None class TestBrainAttributeAccess: