Merge pull request #457 from Unrud/atomiccreate

Atomic creation of collections and atomic PROPPATCH

radicale/__init__.py

@@ -480,12 +480,11 @@ class Application:
         collection = write_collections[0]
 
         props = xmlutils.props_from_request(content)
+        props["tag"] = "VCALENDAR"
         # TODO: use this?
         # timezone = props.get("C:calendar-timezone")
         collection = self.Collection.create_collection(
-            environ["PATH_INFO"], tag="VCALENDAR")
+            environ["PATH_INFO"], props=props)
-        for key, value in props.items():
-            collection.set_meta(key, value)
         return client.CREATED, {}, None
 
     def do_MKCOL(self, environ, read_collections, write_collections, content,
@@ -497,9 +496,8 @@ class Application:
         collection = write_collections[0]
 
         props = xmlutils.props_from_request(content)
-        collection = self.Collection.create_collection(environ["PATH_INFO"])
+        collection = self.Collection.create_collection(
-        for key, value in props.items():
+            environ["PATH_INFO"], props=props)
-            collection.set_meta(key, value)
         return client.CREATED, {}, None
 
     def do_MOVE(self, environ, read_collections, write_collections, content,
@@ -582,7 +580,7 @@ class Application:
             tags = {value: key for key, value in storage.MIMETYPES.items()}
             tag = tags.get(content_type.split(";")[0])
             if tag:
-                collection.set_meta("tag", tag)
+                collection.set_meta({"tag": tag})
         headers = {}
         item_name = xmlutils.name_from_path(environ["PATH_INFO"], collection)
         item = collection.get(item_name)

radicale/storage.py

@@ -38,6 +38,7 @@ from hashlib import md5
 from importlib import import_module
 from itertools import groupby
 from random import getrandbits
+from tempfile import TemporaryDirectory
 
 from atomicwrites import AtomicWriter
 import vobject
@@ -163,6 +164,23 @@ def path_to_filesystem(root, *paths):
     return safe_path
 
 
+def sync_directory(path):
+    """Sync directory to disk
+
+    This only works on POSIX and does nothing on other systems.
+
+    """
+    if os.name == "posix":
+        fd = os.open(path, 0)
+        try:
+            if hasattr(fcntl, "F_FULLFSYNC"):
+                fcntl.fcntl(fd, fcntl.F_FULLFSYNC)
+            else:
+                os.fsync(fd)
+        finally:
+            os.close(fd)
+
+
 class _EncodedAtomicWriter(AtomicWriter):
     def __init__(self, path, encoding, mode="w", overwrite=True):
         self._encoding = encoding
@@ -225,13 +243,15 @@ class BaseCollection:
         return get_etag(self.serialize())
 
     @classmethod
-    def create_collection(cls, href, collection=None, tag=None):
+    def create_collection(cls, href, collection=None, props=None):
         """Create a collection.
 
         ``collection`` is a list of vobject components.
 
-        ``tag`` is the type of collection (VCALENDAR or VADDRESSBOOK). If
+        ``props`` are metadata values for the collection.
-        ``tag`` is not given, it is guessed from the collection.
+
+        ``props["tag"]`` is the type of collection (VCALENDAR or VADDRESSBOOK). If
+        the key ``tag`` is missing, it is guessed from the collection.
 
         """
         raise NotImplementedError
@@ -298,8 +318,8 @@ class BaseCollection:
         """Get metadata value for collection."""
         raise NotImplementedError
 
-    def set_meta(self, key, value):
+    def set_meta(self, props):
-        """Set metadata value for collection."""
+        """Set metadata values for collection."""
         raise NotImplementedError
 
     @property
@@ -326,9 +346,9 @@ class BaseCollection:
 class Collection(BaseCollection):
     """Collection stored in several files per calendar."""
 
-    def __init__(self, path, principal=False):
+    def __init__(self, path, principal=False, folder=None):
-        folder = os.path.expanduser(
+        if not folder:
-            self.configuration.get("storage", "filesystem_folder"))
+            folder = self._get_collection_root_folder()
         # path should already be sanitized
         self.path = sanitize_path(path).strip("/")
         self.storage_encoding = self.configuration.get("encoding", "stock")
@@ -343,6 +363,13 @@ class Collection(BaseCollection):
             self.owner = None
         self.is_principal = principal
 
+    @classmethod
+    def _get_collection_root_folder(cls):
+        filesystem_folder = os.path.expanduser(
+            cls.configuration.get("storage", "filesystem_folder"))
+        folder = os.path.join(filesystem_folder, "collection-root")
+        return folder
+
     @contextmanager
     def _atomic_write(self, path, mode="w"):
         with _EncodedAtomicWriter(
@@ -370,8 +397,11 @@ class Collection(BaseCollection):
             attributes.pop()
 
         # Try to guess if the path leads to a collection or an item
-        folder = os.path.expanduser(
+        folder = cls._get_collection_root_folder()
-            cls.configuration.get("storage", "filesystem_folder"))
+        # HACK: Detection of principal collections fails if folder doesn't
+        #       exist. This can be removed, when this method stop returning
+        #       collections that don't exist.
+        os.makedirs(folder, exist_ok=True)
         if not os.path.isdir(path_to_filesystem(folder, sane_path)):
             # path is not a collection
             if attributes and os.path.isfile(path_to_filesystem(folder,
@@ -405,26 +435,42 @@ class Collection(BaseCollection):
                     yield cls(posixpath.join(path, sub_path))
 
     @classmethod
-    def create_collection(cls, href, collection=None, tag=None):
+    def create_collection(cls, href, collection=None, props=None):
-        folder = os.path.expanduser(
+        folder = cls._get_collection_root_folder()
-            cls.configuration.get("storage", "filesystem_folder"))
-        path = path_to_filesystem(folder, href)
 
-        self = cls(href)
+        # path should already be sanitized
-        if os.path.exists(path):
+        sane_path = sanitize_path(href).strip("/")
-            return self
+        attributes = sane_path.split("/")
-        else:
+        if not attributes[0]:
-            os.makedirs(path)
+            attributes.pop()
-        if not tag and collection:
+        principal = len(attributes) == 1
-            tag = collection[0].name
+        filesystem_path = path_to_filesystem(folder, sane_path)
 
-        if tag == "VCALENDAR":
+        if not props:
-            self.set_meta("tag", "VCALENDAR")
+            props = {}
+        if not props.get("tag") and collection:
+            props["tag"] = collection[0].name
+        if not props:
+            os.makedirs(filesystem_path, exist_ok=True)
+            return cls(sane_path, principal=principal)
+
+        parent_dir = os.path.dirname(filesystem_path)
+        os.makedirs(parent_dir, exist_ok=True)
+        with TemporaryDirectory(prefix=".Radicale.tmp-",
+                                dir=parent_dir) as tmp_dir:
+            # The temporary directory itself can't be renamed
+            tmp_filesystem_path = os.path.join(tmp_dir, "collection")
+            os.makedirs(tmp_filesystem_path)
+            # path is unsafe
+            self = cls("/", principal=principal, folder=tmp_filesystem_path)
+            self.set_meta(props)
+            if props.get("tag") == "VCALENDAR":
                 if collection:
                     collection, = collection
                     items = []
                     for content in ("vevent", "vtodo", "vjournal"):
-                    items.extend(getattr(collection, "%s_list" % content, []))
+                        items.extend(getattr(collection, "%s_list" % content,
+                                             []))
 
                     def get_uid(item):
                         return hasattr(item, "uid") and item.uid.value
@@ -438,14 +484,13 @@ class Collection(BaseCollection):
                             new_collection.add(item)
                         self.upload(
                             self._find_available_file_name(), new_collection)
-
+            elif props.get("tag") == "VCARD":
-        elif tag == "VCARD":
-            self.set_meta("tag", "VADDRESSBOOK")
                 if collection:
                     for card in collection:
                         self.upload(self._find_available_file_name(), card)
-
+            os.rename(tmp_filesystem_path, filesystem_path)
-        return self
+            sync_directory(parent_dir)
+        return cls(sane_path, principal=principal)
 
     def list(self):
         try:
@@ -542,19 +587,16 @@ class Collection(BaseCollection):
             with open(self._props_path, encoding=self.storage_encoding) as prop:
                 return json.load(prop).get(key)
 
-    def set_meta(self, key, value):
+    def set_meta(self, props):
-        properties = {}
         if os.path.exists(self._props_path):
             with open(self._props_path, encoding=self.storage_encoding) as prop:
-                properties.update(json.load(prop))
+                old_props = json.load(prop)
-
+                old_props.update(props)
-        if value:
+                props = old_props
-            properties[key] = value
+        # filter empty entries
-        else:
+        props = {k:v for k,v in props.items() if v}
-            properties.pop(key, None)
-
         with self._atomic_write(self._props_path, "w+") as prop:
-            json.dump(properties, prop)
+            json.dump(props, prop)
 
     @property
     def last_modified(self):

radicale/tests/custom/storage.py

@@ -27,7 +27,5 @@ from radicale import storage
 # TODO: make something more in this collection (and test it)
 class Collection(storage.Collection):
     """Collection stored in a folder."""
-    def __init__(self, path, principal=False):
+    def __init__(self, *args, **kwargs):
-        super().__init__(path, principal)
+        super().__init__(*args, **kwargs)
-        self._filesystem_path = storage.path_to_filesystem(
-            self.configuration.get("storage", "test_folder"), self.path)

radicale/tests/test_base.py

@@ -654,7 +654,6 @@ class TestCustomStorageSystem(BaseRequests, BaseTest):
         super().setup()
         self.colpath = tempfile.mkdtemp()
         self.configuration.set("storage", "filesystem_folder", self.colpath)
-        self.configuration.set("storage", "test_folder", self.colpath)
         self.application = Application(self.configuration, self.logger)
 
     def teardown(self):

radicale/xmlutils.py

@@ -757,12 +757,14 @@ def proppatch(path, xml_request, collection):
     href.text = _href(collection, path)
     response.append(href)
 
-    for short_name, value in props_to_set.items():
+    # Merge props_to_set and props_to_remove
-        collection.set_meta(short_name, value)
-        _add_propstat_to(response, short_name, 200)
-
     for short_name in props_to_remove:
-        collection.set_meta(short_name, "")
+        props_to_set[short_name] = ""
+
+    # Set/Delete props in one atomic operation
+    collection.set_meta(props_to_set)
+
+    for short_name in props_to_set:
         _add_propstat_to(response, short_name, 200)
 
     return _pretty_xml(multistatus)