Move hook into storage.Collection

The hook is only valid for filesystem storage, it's meaningless for other backends like databases.

radicale/__init__.py

@@ -32,11 +32,9 @@ import itertools
 import os
 import posixpath
 import pprint
-import shlex
 import socket
 import socketserver
 import ssl
-import subprocess
 import threading
 import wsgiref.simple_server
 import zlib
@@ -285,12 +283,12 @@ class Application:
         if user and is_authenticated:
             principal_path = "/%s/" % user
             if self.authorized(user, principal_path, "w"):
-                with self._lock_collection("r", user):
+                with self.Collection.acquire_lock("r", user):
                     principal = next(
                         self.Collection.discover(principal_path, depth="1"),
                         None)
                 if not principal:
-                    with self._lock_collection("w", user):
+                    with self.Collection.acquire_lock("w", user):
                         self.Collection.create_collection(principal_path)
 
         # Verify content length
@@ -363,20 +361,6 @@ class Application:
             allowed |= self.authorized(user, parent_path, permission)
         return allowed
 
-    @contextmanager
-    def _lock_collection(self, lock_mode, user):
-        """Lock the collection with ``permission`` and execute hook."""
-        with self.Collection.acquire_lock(lock_mode) as value:
-            yield value
-            hook = self.configuration.get("storage", "hook")
-            if lock_mode == "w" and hook:
-                self.logger.debug("Running hook")
-                folder = os.path.expanduser(self.configuration.get(
-                    "storage", "filesystem_folder"))
-                subprocess.check_call(
-                    hook % {"user": shlex.quote(user or "Anonymous")},
-                    shell=True, cwd=folder)
-
     def _read_content(self, environ):
         content_length = int(environ.get("CONTENT_LENGTH") or 0)
         if content_length > 0:
@@ -391,7 +375,7 @@ class Application:
         """Manage DELETE request."""
         if not self._access(user, path, "w"):
             return NOT_ALLOWED
-        with self._lock_collection("w", user):
+        with self.Collection.acquire_lock("w", user):
             item = next(self.Collection.discover(path), None)
             if not self._access(user, path, "w", item):
                 return NOT_ALLOWED
@@ -416,7 +400,7 @@ class Application:
             return client.OK, headers, answer
         if not self._access(user, path, "r"):
             return NOT_ALLOWED
-        with self._lock_collection("r", user):
+        with self.Collection.acquire_lock("r", user):
             item = next(self.Collection.discover(path), None)
             if not self._access(user, path, "r", item):
                 return NOT_ALLOWED
@@ -445,7 +429,7 @@ class Application:
         if not self.authorized(user, path, "w"):
             return NOT_ALLOWED
         content = self._read_content(environ)
-        with self._lock_collection("w", user):
+        with self.Collection.acquire_lock("w", user):
             item = next(self.Collection.discover(path), None)
             if item:
                 return client.CONFLICT, {}, None
@@ -461,7 +445,7 @@ class Application:
         if not self.authorized(user, path, "w"):
             return NOT_ALLOWED
         content = self._read_content(environ)
-        with self._lock_collection("w", user):
+        with self.Collection.acquire_lock("w", user):
             item = next(self.Collection.discover(path), None)
             if item:
                 return client.CONFLICT, {}, None
@@ -481,7 +465,7 @@ class Application:
         if not self._access(user, to_path, "w"):
             return NOT_ALLOWED
 
-        with self._lock_collection("w", user):
+        with self.Collection.acquire_lock("w", user):
             item = next(self.Collection.discover(path), None)
             if not self._access(user, path, "w", item):
                 return NOT_ALLOWED
@@ -519,7 +503,7 @@ class Application:
         if not self._access(user, path, "r"):
             return NOT_ALLOWED
         content = self._read_content(environ)
-        with self._lock_collection("r", user):
+        with self.Collection.acquire_lock("r", user):
             items = self.Collection.discover(
                 path, environ.get("HTTP_DEPTH", "0"))
             # take root item for rights checking
@@ -544,7 +528,7 @@ class Application:
         if not self.authorized(user, path, "w"):
             return NOT_ALLOWED
         content = self._read_content(environ)
-        with self._lock_collection("w", user):
+        with self.Collection.acquire_lock("w", user):
             item = next(self.Collection.discover(path), None)
             if not isinstance(item, self.Collection):
                 return client.CONFLICT, {}, None
@@ -557,7 +541,7 @@ class Application:
         if not self._access(user, path, "w"):
             return NOT_ALLOWED
         content = self._read_content(environ)
-        with self._lock_collection("w", user):
+        with self.Collection.acquire_lock("w", user):
             parent_path = storage.sanitize_path(
                 "/%s/" % posixpath.dirname(path.strip("/")))
             item = next(self.Collection.discover(path), None)
@@ -612,7 +596,7 @@ class Application:
         if not self._access(user, path, "w"):
             return NOT_ALLOWED
         content = self._read_content(environ)
-        with self._lock_collection("r", user):
+        with self.Collection.acquire_lock("r", user):
             item = next(self.Collection.discover(path), None)
             if not self._access(user, path, "w", item):
                 return NOT_ALLOWED

radicale/storage.py

@@ -29,7 +29,9 @@ import errno
 import json
 import os
 import posixpath
+import shlex
 import stat
+import subprocess
 import threading
 import time
 from contextlib import contextmanager
@@ -371,12 +373,14 @@ class BaseCollection:
 
     @classmethod
     @contextmanager
-    def acquire_lock(cls, mode):
+    def acquire_lock(cls, mode, user=None):
         """Set a context manager to lock the whole storage.
 
         ``mode`` must either be "r" for shared access or "w" for exclusive
         access.
 
+        ``user`` is the name of the logged in user or empty.
+
         """
         raise NotImplementedError
 
@@ -743,13 +747,15 @@ class Collection(BaseCollection):
 
     @classmethod
     @contextmanager
-    def acquire_lock(cls, mode):
+    def acquire_lock(cls, mode, user=None):
         def condition():
             if mode == "r":
                 return not cls._writer
             else:
                 return not cls._writer and cls._readers == 0
 
+        folder = os.path.expanduser(cls.configuration.get(
+            "storage", "filesystem_folder"))
         # Use a primitive lock which only works within one process as a
         # precondition for inter-process file-based locking
         with cls._lock:
@@ -770,8 +776,6 @@ class Collection(BaseCollection):
             else:
                 cls._writer = True
             if not cls._lock_file:
-                folder = os.path.expanduser(
-                    cls.configuration.get("storage", "filesystem_folder"))
                 cls._makedirs_synced(folder)
                 lock_path = os.path.join(folder, ".Radicale.lock")
                 cls._lock_file = open(lock_path, "w+")
@@ -797,6 +801,13 @@ class Collection(BaseCollection):
                 cls._lock_file_locked = True
         try:
             yield
+            # execute hook
+            hook = cls.configuration.get("storage", "hook")
+            if mode == "w" and hook:
+                cls.logger.debug("Running hook")
+                subprocess.check_call(
+                    hook % {"user": shlex.quote(user or "Anonymous")},
+                    shell=True, cwd=folder)
         finally:
             with cls._lock:
                 if mode == "r":