diff --git a/CHANGES.md b/CHANGES.md index d7f362a..c28082a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -14,6 +14,15 @@ no catalog-lag skew vs. live workflow state. Closes [#8](https://github.com/bluedynamics/plone-pgthumbor/issues/8). +- Fix: `@thumbor-auth` REST service now prefers the ZODB storage + connection (already held for the request) over the psycopg pool, so + per-image auth verification doesn't contend on `pool.getconn()`. + The SQL query is unchanged — this is strictly a connection-acquisition + change, matching the pattern plone-pgcatalog uses in + `_get_pg_read_connection`. Falls back to the pool when no ZODB + storage is in scope (tests, scripts). + Related to [#8](https://github.com/bluedynamics/plone-pgthumbor/issues/8). + ## 0.6.3 (2026-04-13) - Move `@@images` put of overrides, it is on a layer. diff --git a/src/plone/pgthumbor/restapi.py b/src/plone/pgthumbor/restapi.py index 1469018..91ee863 100644 --- a/src/plone/pgthumbor/restapi.py +++ b/src/plone/pgthumbor/restapi.py @@ -23,6 +23,7 @@ from AccessControl import getSecurityManager from plone.pgcatalog.pool import get_pool from plone.pgcatalog.pool import get_request_connection +from plone.pgcatalog.pool import get_storage_connection from plone.rest.service import Service from Products.CMFCore.utils import getToolByName @@ -66,9 +67,14 @@ def render(self): # Single PG query: does the object's allowed_roles overlap with # user principals? plone-pgcatalog stores allowedRolesAndUsers in # a dedicated TEXT[] column (with GIN index), not inside idx JSONB. + # + # Prefer the ZODB storage connection (already held for this request) + # so we don't contend on the psycopg pool — under cold-cache load, + # the pool becomes the bottleneck (see #8). try: - pool = get_pool(self.context) - conn = get_request_connection(pool) + conn = get_storage_connection(self.context) + if conn is None: + conn = get_request_connection(get_pool(self.context)) row = conn.execute( "SELECT (allowed_roles && %s::text[]) AS allowed " "FROM object_state WHERE zoid = %s", diff --git a/tests/test_auth_service.py b/tests/test_auth_service.py index 7c1e0f0..f11337f 100644 --- a/tests/test_auth_service.py +++ b/tests/test_auth_service.py @@ -33,7 +33,14 @@ def _mock_catalog(self, principals=None): catalog._listAllowedRolesAndUsers.return_value = principals return catalog - def _patch_dependencies(self, service, catalog, row): + def _patch_dependencies(self, service, catalog, row, storage_conn=True): + """Patch REST service dependencies. + + By default (``storage_conn=True``) the storage connection is used + — matching the production path where ZODB holds a request-scoped + connection. Set ``storage_conn=False`` to simulate the pool + fallback (tests, scripts, non-ZODB contexts). + """ mock_conn = MagicMock() mock_conn.execute.return_value.fetchone.return_value = row mock_pool = MagicMock() @@ -44,6 +51,10 @@ def _patch_dependencies(self, service, catalog, row): patch( "plone.pgthumbor.restapi.get_request_connection", return_value=mock_conn ), + patch( + "plone.pgthumbor.restapi.get_storage_connection", + return_value=mock_conn if storage_conn else None, + ), ] return patches @@ -53,7 +64,7 @@ def test_allowed_user_returns_200(self): catalog = self._mock_catalog(["user:john", "Authenticated", "Anonymous"]) patches = self._patch_dependencies(service, catalog, {"allowed": True}) - with patches[0], patches[1] as mock_sm, patches[2], patches[3]: + with patches[0], patches[1] as mock_sm, patches[2], patches[3], patches[4]: mock_sm.return_value.getUser.return_value = MagicMock() result = service.render() @@ -66,7 +77,7 @@ def test_denied_user_returns_401(self): catalog = self._mock_catalog(["user:john", "Authenticated"]) patches = self._patch_dependencies(service, catalog, {"allowed": False}) - with patches[0], patches[1] as mock_sm, patches[2], patches[3]: + with patches[0], patches[1] as mock_sm, patches[2], patches[3], patches[4]: mock_sm.return_value.getUser.return_value = MagicMock() result = service.render() @@ -95,7 +106,7 @@ def test_zoid_not_in_catalog_returns_404(self): catalog = self._mock_catalog(["user:john", "Authenticated"]) patches = self._patch_dependencies(service, catalog, None) - with patches[0], patches[1] as mock_sm, patches[2], patches[3]: + with patches[0], patches[1] as mock_sm, patches[2], patches[3], patches[4]: mock_sm.return_value.getUser.return_value = MagicMock() result = service.render() @@ -110,10 +121,68 @@ def test_db_error_returns_503(self): with ( patch("plone.pgthumbor.restapi.getToolByName", return_value=catalog), patch("plone.pgthumbor.restapi.getSecurityManager") as mock_sm, - patch("plone.pgthumbor.restapi.get_pool", side_effect=Exception("DB down")), + patch( + "plone.pgthumbor.restapi.get_storage_connection", + side_effect=Exception("DB down"), + ), ): mock_sm.return_value.getUser.return_value = MagicMock() result = service.render() assert service.request.response.status == 503 assert "error" in json.loads(result) + + def test_prefers_storage_connection_over_pool(self): + """Storage conn is used when available; pool is not touched (regression for #8).""" + service = self._make_service("000000000000001a") + catalog = self._mock_catalog(["user:john", "Authenticated", "Anonymous"]) + + storage_conn = MagicMock() + storage_conn.execute.return_value.fetchone.return_value = {"allowed": True} + + with ( + patch("plone.pgthumbor.restapi.getToolByName", return_value=catalog), + patch("plone.pgthumbor.restapi.getSecurityManager") as mock_sm, + patch( + "plone.pgthumbor.restapi.get_storage_connection", + return_value=storage_conn, + ) as mock_storage, + patch("plone.pgthumbor.restapi.get_pool") as mock_pool, + patch("plone.pgthumbor.restapi.get_request_connection") as mock_req_conn, + ): + mock_sm.return_value.getUser.return_value = MagicMock() + service.render() + + mock_storage.assert_called_once_with(service.context) + # Pool fallback must not be entered when storage conn is available. + mock_pool.assert_not_called() + mock_req_conn.assert_not_called() + storage_conn.execute.assert_called_once() + + def test_falls_back_to_pool_when_no_storage_connection(self): + """When storage conn is unavailable (tests, scripts), fall back to pool.""" + service = self._make_service("000000000000001a") + catalog = self._mock_catalog(["user:john", "Authenticated", "Anonymous"]) + + pool_conn = MagicMock() + pool_conn.execute.return_value.fetchone.return_value = {"allowed": True} + + with ( + patch("plone.pgthumbor.restapi.getToolByName", return_value=catalog), + patch("plone.pgthumbor.restapi.getSecurityManager") as mock_sm, + patch( + "plone.pgthumbor.restapi.get_storage_connection", + return_value=None, + ), + patch("plone.pgthumbor.restapi.get_pool") as mock_pool, + patch( + "plone.pgthumbor.restapi.get_request_connection", + return_value=pool_conn, + ) as mock_req_conn, + ): + mock_sm.return_value.getUser.return_value = MagicMock() + service.render() + + mock_pool.assert_called_once_with(service.context) + mock_req_conn.assert_called_once() + pool_conn.execute.assert_called_once()