Merge pull request #10 from bluedynamics/fix/thumbor-auth-use-storage-connection

fix: @thumbor-auth prefers storage conn over psycopg pool (related to #8)

Author: jensens

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.

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",

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()