From 0fc7f787a83c00bd2bfde9c7ad58731c35fa91b0 Mon Sep 17 00:00:00 2001 From: Unrud Date: Wed, 3 Aug 2016 14:34:36 +0200 Subject: [PATCH 1/5] Remove test_folder configuration It's not used. --- radicale/tests/custom/storage.py | 2 -- radicale/tests/test_base.py | 1 - 2 files changed, 3 deletions(-) diff --git a/radicale/tests/custom/storage.py b/radicale/tests/custom/storage.py index ece1c17..9829673 100644 --- a/radicale/tests/custom/storage.py +++ b/radicale/tests/custom/storage.py @@ -29,5 +29,3 @@ class Collection(storage.Collection): """Collection stored in a folder.""" def __init__(self, path, principal=False): super().__init__(path, principal) - self._filesystem_path = storage.path_to_filesystem( - self.configuration.get("storage", "test_folder"), self.path) diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 84bad8f..5d0f7d8 100644 --- a/radicale/tests/test_base.py +++ b/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): From de510148a0c8ff57de05c7875d14863f4c8b3069 Mon Sep 17 00:00:00 2001 From: Unrud Date: Wed, 3 Aug 2016 14:35:50 +0200 Subject: [PATCH 2/5] *args and **kwargs for test collection --- radicale/tests/custom/storage.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/radicale/tests/custom/storage.py b/radicale/tests/custom/storage.py index 9829673..63a1713 100644 --- a/radicale/tests/custom/storage.py +++ b/radicale/tests/custom/storage.py @@ -27,5 +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): - super().__init__(path, principal) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) From bd7641699e9502d2a663d1703c6b0d46e5b75a36 Mon Sep 17 00:00:00 2001 From: Unrud Date: Wed, 3 Aug 2016 14:45:52 +0200 Subject: [PATCH 3/5] Atomic PROPPATCH --- radicale/__init__.py | 8 +++----- radicale/storage.py | 25 +++++++++++-------------- radicale/xmlutils.py | 12 +++++++----- 3 files changed, 21 insertions(+), 24 deletions(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index 8cee0de..dde8890 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -484,8 +484,7 @@ class Application: # timezone = props.get("C:calendar-timezone") collection = self.Collection.create_collection( environ["PATH_INFO"], tag="VCALENDAR") - for key, value in props.items(): - collection.set_meta(key, value) + collection.set_meta(props) return client.CREATED, {}, None def do_MKCOL(self, environ, read_collections, write_collections, content, @@ -498,8 +497,7 @@ class Application: props = xmlutils.props_from_request(content) collection = self.Collection.create_collection(environ["PATH_INFO"]) - for key, value in props.items(): - collection.set_meta(key, value) + collection.set_meta(props) 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) diff --git a/radicale/storage.py b/radicale/storage.py index 10cffc1..feee9e5 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -298,8 +298,8 @@ class BaseCollection: """Get metadata value for collection.""" raise NotImplementedError - def set_meta(self, key, value): - """Set metadata value for collection.""" + def set_meta(self, props): + """Set metadata values for collection.""" raise NotImplementedError @property @@ -419,7 +419,7 @@ class Collection(BaseCollection): tag = collection[0].name if tag == "VCALENDAR": - self.set_meta("tag", "VCALENDAR") + self.set_meta({"tag": "VCALENDAR"}) if collection: collection, = collection items = [] @@ -440,7 +440,7 @@ class Collection(BaseCollection): self._find_available_file_name(), new_collection) elif tag == "VCARD": - self.set_meta("tag", "VADDRESSBOOK") + self.set_meta({"tag": "VADDRESSBOOK"}) if collection: for card in collection: self.upload(self._find_available_file_name(), card) @@ -542,19 +542,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): - properties = {} + def set_meta(self, props): if os.path.exists(self._props_path): with open(self._props_path, encoding=self.storage_encoding) as prop: - properties.update(json.load(prop)) - - if value: - properties[key] = value - else: - properties.pop(key, None) - + old_props = json.load(prop) + old_props.update(props) + props = old_props + # filter empty entries + props = {k:v for k,v in props.items() if v} with self._atomic_write(self._props_path, "w+") as prop: - json.dump(properties, prop) + json.dump(props, prop) @property def last_modified(self): diff --git a/radicale/xmlutils.py b/radicale/xmlutils.py index 9134102..0682a96 100644 --- a/radicale/xmlutils.py +++ b/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(): - collection.set_meta(short_name, value) - _add_propstat_to(response, short_name, 200) - + # Merge props_to_set and props_to_remove 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) From e34d1c46cda93a254bbc8d78c2ca548ca619c516 Mon Sep 17 00:00:00 2001 From: Unrud Date: Wed, 3 Aug 2016 14:51:34 +0200 Subject: [PATCH 4/5] Move collections into collection-root folder This is required for atomic creation and deletion of the "/" collection. --- radicale/storage.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/radicale/storage.py b/radicale/storage.py index feee9e5..64bf085 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -327,8 +327,7 @@ class Collection(BaseCollection): """Collection stored in several files per calendar.""" def __init__(self, path, principal=False): - folder = os.path.expanduser( - 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 +342,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 +376,11 @@ class Collection(BaseCollection): attributes.pop() # Try to guess if the path leads to a collection or an item - folder = os.path.expanduser( - cls.configuration.get("storage", "filesystem_folder")) + folder = cls._get_collection_root_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, @@ -406,8 +415,7 @@ class Collection(BaseCollection): @classmethod def create_collection(cls, href, collection=None, tag=None): - folder = os.path.expanduser( - cls.configuration.get("storage", "filesystem_folder")) + folder = cls._get_collection_root_folder() path = path_to_filesystem(folder, href) self = cls(href) From ae89082c24dcef90b9500b536efb5a7a61074072 Mon Sep 17 00:00:00 2001 From: Unrud Date: Wed, 3 Aug 2016 15:04:55 +0200 Subject: [PATCH 5/5] Atomic creation of collections --- radicale/__init__.py | 8 +-- radicale/storage.py | 113 ++++++++++++++++++++++++++++--------------- 2 files changed, 79 insertions(+), 42 deletions(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index dde8890..1c8bb92 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -480,11 +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") - collection.set_meta(props) + environ["PATH_INFO"], props=props) return client.CREATED, {}, None def do_MKCOL(self, environ, read_collections, write_collections, content, @@ -496,8 +496,8 @@ class Application: collection = write_collections[0] props = xmlutils.props_from_request(content) - collection = self.Collection.create_collection(environ["PATH_INFO"]) - collection.set_meta(props) + collection = self.Collection.create_collection( + environ["PATH_INFO"], props=props) return client.CREATED, {}, None def do_MOVE(self, environ, read_collections, write_collections, content, diff --git a/radicale/storage.py b/radicale/storage.py index 64bf085..3e932d7 100644 --- a/radicale/storage.py +++ b/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 - ``tag`` is not given, it is guessed from the collection. + ``props`` are metadata values for 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 @@ -326,8 +346,9 @@ class BaseCollection: class Collection(BaseCollection): """Collection stored in several files per calendar.""" - def __init__(self, path, principal=False): - folder = self._get_collection_root_folder() + def __init__(self, path, principal=False, folder=None): + if not 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") @@ -414,46 +435,62 @@ 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 = cls._get_collection_root_folder() - path = path_to_filesystem(folder, href) - self = cls(href) - if os.path.exists(path): - return self - else: - os.makedirs(path) - if not tag and collection: - tag = collection[0].name + # path should already be sanitized + sane_path = sanitize_path(href).strip("/") + attributes = sane_path.split("/") + if not attributes[0]: + attributes.pop() + principal = len(attributes) == 1 + filesystem_path = path_to_filesystem(folder, sane_path) - if tag == "VCALENDAR": - self.set_meta({"tag": "VCALENDAR"}) - if collection: - collection, = collection - items = [] - for content in ("vevent", "vtodo", "vjournal"): - items.extend(getattr(collection, "%s_list" % content, [])) + if not props: + 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) - def get_uid(item): - return hasattr(item, "uid") and item.uid.value + 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_by_uid = groupby( - sorted(items, key=get_uid), get_uid) + def get_uid(item): + return hasattr(item, "uid") and item.uid.value - for uid, items in items_by_uid: - new_collection = vobject.iCalendar() - for item in items: - new_collection.add(item) - self.upload( - self._find_available_file_name(), new_collection) + items_by_uid = groupby( + sorted(items, key=get_uid), get_uid) - elif tag == "VCARD": - self.set_meta({"tag": "VADDRESSBOOK"}) - if collection: - for card in collection: - self.upload(self._find_available_file_name(), card) - - return self + for uid, items in items_by_uid: + new_collection = vobject.iCalendar() + for item in items: + new_collection.add(item) + self.upload( + self._find_available_file_name(), new_collection) + elif props.get("tag") == "VCARD": + if collection: + for card in collection: + self.upload(self._find_available_file_name(), card) + os.rename(tmp_filesystem_path, filesystem_path) + sync_directory(parent_dir) + return cls(sane_path, principal=principal) def list(self): try: