Merge pull request #218 from untitaker/specialchars-in-collection

Fix special chars once again

tests/storage/__init__.py

@@ -6,7 +6,7 @@ import pytest
 
 import vdirsyncer.exceptions as exceptions
 from vdirsyncer.storage.base import Item
-from vdirsyncer.utils.compat import iteritems, text_type
+from vdirsyncer.utils.compat import PY2, iteritems, text_type
 
 from .. import EVENT_TEMPLATE, TASK_TEMPLATE, VCARD_TEMPLATE, \
     assert_item_equals
@@ -215,3 +215,15 @@ class StorageTests(object):
         items = list(href for href, etag in s.list())
         assert len(items) == 2
         assert len(set(items)) == 2
+
+    @pytest.mark.parametrize('collname', [
+        'test@foo',
+        'testätfoo',
+    ])
+    def test_specialchar_collection(self, requires_collections,
+                                    get_storage_args, get_item, collname):
+        if getattr(self, 'dav_server', '') == 'radicale' and PY2:
+            pytest.skip('Radicale is broken on Python 2.')
+        s = self.storage_class(**get_storage_args(collection=collname))
+        href, etag = s.upload(get_item())
+        s.get(href)

vdirsyncer/storage/dav.py

@@ -22,23 +22,23 @@ CALDAV_DT_FORMAT = '%Y%m%dT%H%M%SZ'
 def _normalize_href(base, href):
     '''Normalize the href to be a path only relative to hostname and
     schema.'''
+    orig_href = href
     base = to_native(base, 'utf-8')
     href = to_native(href, 'utf-8')
     if not href:
         raise ValueError(href)
+
     x = utils.compat.urlparse.urljoin(base, href)
     x = utils.compat.urlparse.urlsplit(x).path
+
+    x = utils.compat.urlunquote(x)
+    x = utils.compat.urlquote(x, '/@%')
+
+    dav_logger.debug('Normalized URL from {!r} to {!r}'.format(orig_href, x))
+
     return x
 
 
-def _encode_url(x):
-    return utils.compat.urlquote(x, '/@')
-
-
-def _decode_url(x):
-    return utils.compat.urlunquote(x)
-
-
 class InvalidXMLResponse(exceptions.InvalidResponse):
     pass
 
@@ -80,7 +80,7 @@ def _fuzzy_matches_mimetype(strict, weak):
 
 def _get_collection_from_url(url):
     _, collection = url.rstrip('/').rsplit('/', 1)
-    return collection
+    return utils.compat.urlunquote(collection)
 
 
 class Discover(object):
@@ -204,7 +204,10 @@ class Discover(object):
                 return c
 
         home = self.find_home()
-        url = '{}/{}'.format(home.rstrip('/'), collection)
+        url = utils.compat.urlparse.urljoin(
+            home,
+            utils.compat.urlquote(collection, '/@')
+        )
 
         try:
             url = self._create_collection_impl(url)
@@ -382,7 +385,7 @@ class DavStorage(Storage):
         for href in hrefs:
             if href != self._normalize_href(href):
                 raise exceptions.NotFoundError(href)
-            href_xml.append('<D:href>{}</D:href>'.format(_encode_url(href)))
+            href_xml.append('<D:href>{}</D:href>'.format(href))
         if not href_xml:
             return ()
 
@@ -435,12 +438,12 @@ class DavStorage(Storage):
 
         response = self.session.request(
             'PUT',
-            _encode_url(href),
+            href,
             data=item.raw.encode('utf-8'),
             headers=headers
         )
         etag = response.headers.get('etag', None)
-        href = _decode_url(self._normalize_href(response.url))
+        href = self._normalize_href(response.url)
         if not etag:
             dav_logger.warning('Race condition detected with server {}, '
                                'consider using an alternative.'
@@ -468,11 +471,11 @@ class DavStorage(Storage):
 
         self.session.request(
             'DELETE',
-            _encode_url(href),
+            href,
             headers=headers
         )
 
-    def _parse_prop_responses(self, root, decoding_rounds=1):
+    def _parse_prop_responses(self, root):
         hrefs = set()
         for response in root.iter('{DAV:}response'):
             href = response.find('{DAV:}href')
@@ -481,8 +484,6 @@ class DavStorage(Storage):
                 continue
 
             href = self._normalize_href(href.text)
-            for i in range(decoding_rounds):
-                href = _decode_url(href)
 
             if href in hrefs:
                 # Servers that send duplicate hrefs:
@@ -544,9 +545,9 @@ class DavStorage(Storage):
 
         # Decode twice because ownCloud encodes twice.
         # See https://github.com/owncloud/contacts/issues/581
-        rv = self._parse_prop_responses(root, decoding_rounds=2)
+        rv = self._parse_prop_responses(root)
         for href, etag, prop in rv:
-            yield href, etag
+            yield utils.compat.urlunquote(href), etag
 
 
 class CaldavStorage(DavStorage):

vdirsyncer/storage/filesystem.py

@@ -10,7 +10,7 @@ from .base import Item, Storage
 from .. import exceptions, log
 from ..utils import checkdir, expand_path, generate_href, get_etag_from_file, \
     get_etag_from_fileobject
-from ..utils.compat import text_type
+from ..utils.compat import text_type, to_native
 
 logger = log.get(__name__)
 
@@ -30,7 +30,7 @@ class FilesystemStorage(Storage):
         href, so if you change the file extension after a sync, this will
         trigger a re-download of everything (but *should* not cause data-loss
         of any kind).
-    :param encoding: File encoding for items.
+    :param encoding: File encoding for items, both content and filename.
     :param post_hook: A command to call for each item creation and
         modification. The command will be called with the path of the
         new/updated file.
@@ -42,7 +42,7 @@ class FilesystemStorage(Storage):
     def __init__(self, path, fileext, encoding='utf-8', post_hook=None,
                  **kwargs):
         super(FilesystemStorage, self).__init__(**kwargs)
-        path = expand_path(path)
+        path = expand_path(to_native(path, encoding))
         checkdir(path, create=False)
         self.path = path
         self.encoding = encoding
@@ -70,15 +70,21 @@ class FilesystemStorage(Storage):
     @classmethod
     def create_collection(cls, collection, **kwargs):
         kwargs = dict(kwargs)
+        encoding = kwargs.get('encoding', 'utf-8')
+        path = to_native(kwargs['path'], encoding)
 
         if collection is not None:
-            kwargs['path'] = os.path.join(kwargs['path'], collection)
-        checkdir(expand_path(kwargs['path']), create=True)
+            collection = to_native(collection, encoding)
+            path = os.path.join(path, collection)
+
+        checkdir(expand_path(path), create=True)
+
+        kwargs['path'] = path
         kwargs['collection'] = collection
         return kwargs
 
     def _get_filepath(self, href):
-        return os.path.join(self.path, href)
+        return os.path.join(self.path, to_native(href, self.encoding))
 
     def _get_href(self, ident):
         # XXX: POSIX only defines / and \0 as invalid chars, but we should make