Move hack for ownCloud bug into _normalize_href

Also refine the testsuite a bit to catch such problems.

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 PY2, iteritems, text_type
+from vdirsyncer.utils.compat import iteritems, text_type, urlquote, urlunquote
 
 from .. import EVENT_TEMPLATE, TASK_TEMPLATE, VCARD_TEMPLATE, \
     assert_item_equals
@@ -222,17 +222,37 @@ class StorageTests(object):
         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)
+    def test_specialchars(self, monkeypatch, requires_collections,
+                          get_storage_args, get_item):
+        if getattr(self, 'dav_server', '') == 'radicale':
+            pytest.skip('Radicale is fundamentally broken.')
+
+        monkeypatch.setattr('vdirsyncer.utils.generate_href', lambda x: x)
+
+        uid = u'test @ foo ät bar град сатану'
+        collection = 'test @ foo ät bar'
+
+        s = self.storage_class(**get_storage_args(collection=collection))
+        item = get_item(uid=uid)
+
+        href, etag = s.upload(item)
+        item2, etag2 = s.get(href)
+        assert etag2 == etag
+        assert_item_equals(item2, item)
+
+        (href2, etag2), = s.list()
+        assert etag2 == etag
+
+        # https://github.com/owncloud/contacts/issues/581
+        assert href2.replace('%2B', '%20') == href
+
+        item2, etag2 = s.get(href)
+        assert etag2 == etag
+        assert_item_equals(item2, item)
+
+        assert collection in urlunquote(s.collection)
+        if self.storage_class.storage_name.endswith('dav'):
+            assert urlquote(uid, '/@:') in href
 
     def test_metadata(self, requires_metadata, s):
         try:

tests/storage/dav/servers/radicale/__init__.py

@@ -5,6 +5,8 @@ import sys
 
 import pytest
 
+from vdirsyncer.utils.compat import urlquote
+
 import wsgi_intercept
 import wsgi_intercept.requests_intercept
 
@@ -110,7 +112,7 @@ class ServerMixin(object):
             url = 'http://127.0.0.1/bob/'
             if collection is not None:
                 collection += self.storage_class.fileext
-                url = url.rstrip('/') + '/' + collection
+                url = url.rstrip('/') + '/' + urlquote(collection)
 
             rv = {'url': url, 'username': 'bob', 'password': 'bob',
                   'collection': collection}

vdirsyncer/storage/dav.py

@@ -18,6 +18,17 @@ dav_logger = log.get(__name__)
 
 CALDAV_DT_FORMAT = '%Y%m%dT%H%M%SZ'
 
+_path_reserved_chars = frozenset(utils.compat.urlquote(x, '')
+                                 for x in "/?#[]!$&'()*+,;=")
+
+
+def _contains_quoted_reserved_chars(x):
+    for y in _path_reserved_chars:
+        if y in x:
+            dav_logger.debug('Unsafe character: {!r}'.format(y))
+            return True
+    return False
+
 
 def _normalize_href(base, href):
     '''Normalize the href to be a path only relative to hostname and
@@ -31,10 +42,21 @@ def _normalize_href(base, href):
     x = utils.compat.urlparse.urljoin(base, href)
     x = utils.compat.urlparse.urlsplit(x).path
 
-    x = utils.compat.urlunquote(x)
+    # https://github.com/owncloud/contacts/issues/581
+    old_x = None
+    while old_x is None or x != old_x:
+        if _contains_quoted_reserved_chars(x):
+            break
+        old_x = x
+        x = utils.compat.urlunquote(x)
+
     x = utils.compat.urlquote(x, '/@%:')
 
-    dav_logger.debug('Normalized URL from {!r} to {!r}'.format(orig_href, x))
+    if orig_href == x:
+        dav_logger.debug('Already normalized: {!r}'.format(x))
+    else:
+        dav_logger.debug('Normalized URL from {!r} to {!r}'
+                         .format(orig_href, x))
 
     return x
 
@@ -555,11 +577,9 @@ class DavStorage(Storage):
                                         headers=headers)
         root = _parse_xml(response.content)
 
-        # Decode twice because ownCloud encodes twice.
-        # See https://github.com/owncloud/contacts/issues/581
         rv = self._parse_prop_responses(root)
         for href, etag, prop in rv:
-            yield utils.compat.urlunquote(href), etag
+            yield href, etag
 
     def get_meta(self, key):
         try:

vdirsyncer/utils/compat.py

@@ -1,5 +1,6 @@
 # -*- coding: utf-8 -*-
 
+import functools
 import sys
 
 PY2 = sys.version_info[0] == 2
@@ -22,18 +23,23 @@ def to_bytes(x, encoding='ascii'):
     return x
 
 
+def _wrap_native(f, encoding='utf-8'):
+    @functools.wraps(f)
+    def wrapper(x, *a, **kw):
+        to_orig = to_unicode if isinstance(x, text_type) else to_bytes
+        return to_orig(f(to_native(x, encoding), *a, **kw))
+    return wrapper
+
+
 if PY2:  # pragma: no cover
     import urlparse
     import urllib as _urllib
 
     # Horrible hack to make urllib play nice with u'...' urls from requests
-    def urlquote(x, *a, **kw):
-        return _urllib.quote(to_native(x, 'utf-8'), *a, **kw)
+    urlquote = _wrap_native(_urllib.quote)
+    urlunquote = _wrap_native(_urllib.unquote)
 
-    def urlunquote(x, *a, **kw):
-        return _urllib.unquote(to_native(x, 'utf-8'), *a, **kw)
-
-    text_type = unicode  # flake8: noqa
+    text_type = unicode  # noqa
     iteritems = lambda x: x.iteritems()
     itervalues = lambda x: x.itervalues()
     to_native = to_bytes