diff --git a/tests/storage/__init__.py b/tests/storage/__init__.py index a430e8f..61585fb 100644 --- a/tests/storage/__init__.py +++ b/tests/storage/__init__.py @@ -39,7 +39,8 @@ class BaseStorageTests(object): return storage() def _create_bogus_item(self, item_template=None): - r = random.random() + # assert that special chars are handled correctly. + r = '{}@vdirsyncer'.format(random.random()) item_template = item_template or self.item_template return Item(item_template.format(r=r)) @@ -84,8 +85,6 @@ class BaseStorageTests(object): def test_update_nonexisting(self, s): item = self._create_bogus_item() - with pytest.raises(exceptions.PreconditionFailed): - s.update(s._get_href(item), item, '"123"') with pytest.raises(exceptions.PreconditionFailed): s.update('huehue', item, '"123"') @@ -108,8 +107,8 @@ class BaseStorageTests(object): def test_list(self, s): assert not list(s.list()) - s.upload(self._create_bogus_item()) - assert list(s.list()) + href, etag = s.upload(self._create_bogus_item()) + assert list(s.list()) == [(href, etag)] def test_has(self, s): assert not s.has('asd') diff --git a/tests/storage/dav/servers/radicale/__init__.py b/tests/storage/dav/servers/radicale/__init__.py index 9d2c029..3ba1dd8 100644 --- a/tests/storage/dav/servers/radicale/__init__.py +++ b/tests/storage/dav/servers/radicale/__init__.py @@ -105,4 +105,4 @@ class ServerMixin(object): collection += self.storage_class.fileext return {'url': url, 'username': 'bob', 'password': 'bob', - 'collection': collection} + 'collection': collection, 'unsafe_href_chars': ''} diff --git a/tests/storage/dav/test_main.py b/tests/storage/dav/test_main.py index f9e67e5..2048ddc 100644 --- a/tests/storage/dav/test_main.py +++ b/tests/storage/dav/test_main.py @@ -20,7 +20,8 @@ from tests import EVENT_TEMPLATE, TASK_TEMPLATE, VCARD_TEMPLATE import vdirsyncer.exceptions as exceptions from vdirsyncer.storage.base import Item -from vdirsyncer.storage.dav import CaldavStorage, CarddavStorage +from vdirsyncer.storage.dav import CaldavStorage, CarddavStorage, \ + _normalize_href from .. import StorageTests @@ -215,3 +216,26 @@ class TestCaldavStorage(DavStorageTests): class TestCarddavStorage(DavStorageTests): storage_class = CarddavStorage item_template = VCARD_TEMPLATE + + +@pytest.mark.parametrize('base,path', [ + ('http://example.com/', ''), + ('http://example.com/L%C3%98/', '/L%C3%98'), + ('http://example.com/LØ/', '/L%C3%98'), +]) +def test_normalize_href(base, path): + assert _normalize_href(base, 'asdf') == path + '/asdf' + + assert _normalize_href(base, 'hahah') == path + '/hahah' + + assert _normalize_href(base, 'whoops@vdirsyncer.vcf') == \ + path + '/whoops@vdirsyncer.vcf' + + assert _normalize_href(base, 'whoops%40vdirsyncer.vcf') == \ + path + '/whoops@vdirsyncer.vcf' + + assert _normalize_href(base, 'wh%C3%98ops@vdirsyncer.vcf') == \ + path + '/wh%C3%98ops@vdirsyncer.vcf' + + assert _normalize_href(base, 'whØops@vdirsyncer.vcf') == \ + path + '/wh%C3%98ops@vdirsyncer.vcf' diff --git a/tests/test_sync.py b/tests/test_sync.py index 8aab195..aed544d 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -23,7 +23,7 @@ def empty_storage(x): def test_irrelevant_status(): a = MemoryStorage() b = MemoryStorage() - status = {'1': ('1.txt', 1234, '1.ics', 2345)} + status = {'1': ('1', 1234, '1.ics', 2345)} sync(a, b, status) assert not status assert empty_storage(a) @@ -55,8 +55,8 @@ def test_missing_status_and_different_items(): sync(a, b, status) assert not status sync(a, b, status, conflict_resolution='a wins') - assert_item_equals(item1, b.get('1.txt')[0]) - assert_item_equals(item1, a.get('1.txt')[0]) + assert_item_equals(item1, b.get('1')[0]) + assert_item_equals(item1, a.get('1')[0]) def test_upload_and_update(): @@ -67,22 +67,22 @@ def test_upload_and_update(): item = Item(u'UID:1') # new item 1 in a a.upload(item) sync(a, b, status) - assert_item_equals(b.get('1.txt')[0], item) + assert_item_equals(b.get('1')[0], item) item = Item(u'UID:1\nASDF:YES') # update of item 1 in b - b.update('1.txt', item, b.get('1.txt')[1]) + b.update('1', item, b.get('1')[1]) sync(a, b, status) - assert_item_equals(a.get('1.txt')[0], item) + assert_item_equals(a.get('1')[0], item) item2 = Item(u'UID:2') # new item 2 in b b.upload(item2) sync(a, b, status) - assert_item_equals(a.get('2.txt')[0], item2) + assert_item_equals(a.get('2')[0], item2) item2 = Item(u'UID:2\nASDF:YES') # update of item 2 in a - a.update('2.txt', item2, a.get('2.txt')[1]) + a.update('2', item2, a.get('2')[1]) sync(a, b, status) - assert_item_equals(b.get('2.txt')[0], item2) + assert_item_equals(b.get('2')[0], item2) def test_deletion(): @@ -94,16 +94,16 @@ def test_deletion(): a.upload(item) a.upload(Item(u'UID:2')) sync(a, b, status) - b.delete('1.txt', b.get('1.txt')[1]) + b.delete('1', b.get('1')[1]) sync(a, b, status) - assert not a.has('1.txt') and not b.has('1.txt') + assert not a.has('1') and not b.has('1') a.upload(item) sync(a, b, status) - assert a.has('1.txt') and b.has('1.txt') - a.delete('1.txt', a.get('1.txt')[1]) + assert a.has('1') and b.has('1') + a.delete('1', a.get('1')[1]) sync(a, b, status) - assert not a.has('1.txt') and not b.has('1.txt') + assert not a.has('1') and not b.has('1') def test_already_synced(): @@ -113,7 +113,7 @@ def test_already_synced(): a.upload(item) b.upload(item) status = { - '1': ('1.txt', a.get('1.txt')[1], '1.txt', b.get('1.txt')[1]) + '1': ('1', a.get('1')[1], '1', b.get('1')[1]) } old_status = dict(status) a.update = b.update = a.upload = b.upload = \ @@ -122,7 +122,7 @@ def test_already_synced(): for i in (1, 2): sync(a, b, status) assert status == old_status - assert a.has('1.txt') and b.has('1.txt') + assert a.has('1') and b.has('1') @pytest.mark.parametrize('winning_storage', 'ab') diff --git a/vdirsyncer/storage/base.py b/vdirsyncer/storage/base.py index 1a96ece..d2a0e5a 100644 --- a/vdirsyncer/storage/base.py +++ b/vdirsyncer/storage/base.py @@ -65,9 +65,6 @@ class Storage(object): ''' raise NotImplementedError() - def _get_href(self, item): - return item.ident + self.fileext - def __repr__(self): return '<{}(**{})>'.format( self.__class__.__name__, diff --git a/vdirsyncer/storage/dav.py b/vdirsyncer/storage/dav.py index a5f1a16..c47c5a5 100644 --- a/vdirsyncer/storage/dav.py +++ b/vdirsyncer/storage/dav.py @@ -22,6 +22,21 @@ dav_logger = log.get(__name__) CALDAV_DT_FORMAT = '%Y%m%dT%H%M%SZ' +def _normalize_href(base, href, decoding_rounds=1): + '''Normalize the href to be a path only relative to hostname and + schema.''' + if not href: + raise ValueError(href) + x = utils.urlparse.urljoin(base, href) + x = utils.urlparse.urlsplit(x).path + + for i in range(decoding_rounds): + x = utils.compat.urlunquote(x) + + x = utils.compat.urlquote(x, '/@') + return x + + class Discover(object): xml_home = None @@ -240,6 +255,8 @@ class DavStorage(Storage): ``guess``. If you know yours, consider setting it explicitly for performance. :param useragent: Default ``vdirsyncer``. + :param unsafe_href_chars: Replace the given characters when generating + hrefs. Defaults to ``'@'``. ''' # the file extension of items. Useful for testing against radicale. @@ -259,7 +276,8 @@ class DavStorage(Storage): _repr_attributes = ('username', 'url') def __init__(self, url, username='', password='', collection=None, - verify=True, auth=None, useragent=USERAGENT, **kwargs): + verify=True, auth=None, useragent=USERAGENT, + unsafe_href_chars='@', **kwargs): super(DavStorage, self).__init__(**kwargs) url = url.rstrip('/') + '/' @@ -268,6 +286,7 @@ class DavStorage(Storage): self.session = DavSession(url, username, password, verify, auth, useragent, dav_header=self.dav_header) self.collection = collection + self.unsafe_href_chars = unsafe_href_chars # defined for _repr_attributes self.username = username @@ -288,17 +307,14 @@ class DavStorage(Storage): s.displayname = c['displayname'] yield s - def _normalize_href(self, href): - '''Normalize the href to be a path only relative to hostname and - schema.''' - if not href: - raise ValueError(href) - x = utils.urlparse.urljoin(self.session.url, href) - return utils.compat.urlunquote_plus(utils.urlparse.urlsplit(x).path) + def _normalize_href(self, *args, **kwargs): + return _normalize_href(self.session.url, *args, **kwargs) def _get_href(self, item): - href = utils.compat.urlunquote_plus(item.ident) + self.fileext - return self._normalize_href(href) + href = item.ident + for char in self.unsafe_href_chars: + href = item.ident.replace(char, '_') + return self._normalize_href(href + self.fileext) def get(self, href): ((actual_href, item, etag),) = self.get_multi([href]) @@ -613,7 +629,10 @@ class CarddavStorage(DavStorage): if 'vcard' not in content_type.text.lower(): continue - href = self._normalize_href(element.find('{DAV:}href').text) + # Decode twice because ownCloud encodes twice. + # See https://github.com/owncloud/contacts/issues/581 + href = self._normalize_href(element.find('{DAV:}href').text, + decoding_rounds=2) etag = prop.find('{DAV:}getetag').text if href in hrefs: @@ -623,4 +642,4 @@ class CarddavStorage(DavStorage): continue hrefs.add(href) - yield self._normalize_href(href), etag + yield href, etag diff --git a/vdirsyncer/storage/filesystem.py b/vdirsyncer/storage/filesystem.py index 84db39f..e47b7bc 100644 --- a/vdirsyncer/storage/filesystem.py +++ b/vdirsyncer/storage/filesystem.py @@ -61,6 +61,9 @@ class FilesystemStorage(Storage): def _get_filepath(self, href): return os.path.join(self.path, href) + def _get_href(self, item): + return item.ident + self.fileext + def list(self): for fname in os.listdir(self.path): fpath = os.path.join(self.path, fname) diff --git a/vdirsyncer/storage/http.py b/vdirsyncer/storage/http.py index 30d5e75..ad918e6 100644 --- a/vdirsyncer/storage/http.py +++ b/vdirsyncer/storage/http.py @@ -110,9 +110,8 @@ class HttpStorage(Storage): for item in split_collection(r.text): item = Item(item) - href = self._get_href(item) etag = item.hash - self._items[href] = item, etag + self._items[item.ident] = item, etag return ((href, etag) for href, (item, etag) in iteritems(self._items)) diff --git a/vdirsyncer/storage/memory.py b/vdirsyncer/storage/memory.py index 8c9c1e9..ae2400c 100644 --- a/vdirsyncer/storage/memory.py +++ b/vdirsyncer/storage/memory.py @@ -41,7 +41,7 @@ class MemoryStorage(Storage): return href in self.items def upload(self, item): - href = self._get_href(item) + href = item.ident if href in self.items: raise exceptions.AlreadyExistingError(item) etag = _get_etag() diff --git a/vdirsyncer/storage/singlefile.py b/vdirsyncer/storage/singlefile.py index aaf893b..adae229 100644 --- a/vdirsyncer/storage/singlefile.py +++ b/vdirsyncer/storage/singlefile.py @@ -96,9 +96,8 @@ class SingleFileStorage(Storage): for item in split_collection(text): item = Item(item) - href = self._get_href(item) etag = item.hash - self._items[href] = item, etag + self._items[item.ident] = item, etag return ((href, etag) for href, (item, etag) in iteritems(self._items)) @@ -112,7 +111,7 @@ class SingleFileStorage(Storage): raise exceptions.NotFoundError(href) def upload(self, item): - href = self._get_href(item) + href = item.ident self.list() if href in self._items: raise exceptions.AlreadyExistingError(href) diff --git a/vdirsyncer/utils/compat.py b/vdirsyncer/utils/compat.py index e088e2d..20bfd0b 100644 --- a/vdirsyncer/utils/compat.py +++ b/vdirsyncer/utils/compat.py @@ -15,15 +15,15 @@ PY2 = sys.version_info[0] == 2 if PY2: # pragma: no cover import urlparse from urllib import \ - quote_plus as urlquote_plus, \ - unquote_plus as urlunquote_plus + quote as urlquote, \ + unquote as urlunquote text_type = unicode # flake8: noqa iteritems = lambda x: x.iteritems() itervalues = lambda x: x.itervalues() else: # pragma: no cover import urllib.parse as urlparse - urlquote_plus = urlparse.quote_plus - urlunquote_plus = urlparse.unquote_plus + urlquote = urlparse.quote + urlunquote = urlparse.unquote text_type = str iteritems = lambda x: x.items() itervalues = lambda x: x.values()