From 44c7abfd9a7df8b45a29e63d0c7dd68cb66ce594 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Wed, 26 Feb 2014 20:24:25 +0100 Subject: [PATCH] Improvements in exception handling --- vdirsyncer/exceptions.py | 34 +++++++++++++------- vdirsyncer/storage/base.py | 19 ++++++----- vdirsyncer/storage/caldav.py | 22 ++++++++----- vdirsyncer/storage/filesystem.py | 3 +- vdirsyncer/tests/test_storage.py | 54 +++++++++++++++++--------------- vdirsyncer/tests/test_sync.py | 44 +++++++++++++------------- 6 files changed, 100 insertions(+), 76 deletions(-) diff --git a/vdirsyncer/exceptions.py b/vdirsyncer/exceptions.py index b450649..b9dc878 100644 --- a/vdirsyncer/exceptions.py +++ b/vdirsyncer/exceptions.py @@ -7,23 +7,33 @@ :license: MIT, see LICENSE for more details. ''' + class Error(Exception): '''Baseclass for all errors.''' - pass -class NotFoundError(Error): - '''The item does not exist (anymore).''' - pass -class AlreadyExistingError(Error): - '''The item exists although it shouldn't, possible race condition.''' - pass +class PreconditionFailed(Error): + ''' + - The item doesn't exist although it should + - The item exists although it shouldn't + - The etags don't match. + + Due to CalDAV we can't actually say which error it is. + This error may indicate race conditions. + ''' + + +class NotFoundError(PreconditionFailed): + '''Item not found''' + + +class AlreadyExistingError(PreconditionFailed): + '''Item already exists''' + + +class WrongEtagError(PreconditionFailed): + '''Wrong etag''' -class WrongEtagError(Error): - '''The given etag doesn't match the etag from the storage, possible race - condition.''' - pass class StorageError(Error): '''Internal or initialization errors with storage.''' - pass diff --git a/vdirsyncer/storage/base.py b/vdirsyncer/storage/base.py index 96f217c..2caf0ad 100644 --- a/vdirsyncer/storage/base.py +++ b/vdirsyncer/storage/base.py @@ -31,8 +31,8 @@ class Storage(object): - HREF: Per-storage identifier of item, might be UID. - ETAG: Checksum of item, or something similar that changes when the object does ''' - def __init__(self, fileext='.txt', item_class=Item): - self.fileext = fileext + fileext = '.txt' + def __init__(self, item_class=Item): self.item_class = item_class def _get_href(self, uid): @@ -54,6 +54,8 @@ class Storage(object): def get_multi(self, hrefs): ''' :param hrefs: list of hrefs to fetch + :raises: :exc:`vdirsyncer.exceptions.PreconditionFailed` if one of the + items couldn't be found. :returns: iterable of (href, obj, etag) ''' for href in hrefs: @@ -70,16 +72,16 @@ class Storage(object): def upload(self, obj): ''' Upload a new object, raise - :exc:`vdirsyncer.exceptions.AlreadyExistingError` if it already exists. + :exc:`vdirsyncer.exceptions.PreconditionFailed` if it already exists. :returns: (href, etag) ''' raise NotImplementedError() def update(self, href, obj, etag): ''' - Update the object, raise :exc:`vdirsyncer.exceptions.WrongEtagError` if - the etag on the server doesn't match the given etag, raise - :exc:`vdirsyncer.exceptions.NotFoundError` if the item doesn't exist. + Update the object, raise + :exc:`vdirsyncer.exceptions.PreconditionFailed` if the etag on the + server doesn't match the given etag or if the item doesn't exist. :returns: etag ''' @@ -87,7 +89,8 @@ class Storage(object): def delete(self, href, etag): ''' - Delete the object by href, raise exceptions when etag doesn't match, no - return value + Delete the object by href, raise + :exc:`vdirsyncer.exceptions.PreconditionFailed` when item has a + different etag or doesn't exist. ''' raise NotImplementedError() diff --git a/vdirsyncer/storage/caldav.py b/vdirsyncer/storage/caldav.py index 85edfe9..16a1aa5 100644 --- a/vdirsyncer/storage/caldav.py +++ b/vdirsyncer/storage/caldav.py @@ -20,6 +20,7 @@ CALDAV_DT_FORMAT = '%Y%m%dT%H%M%SZ' class CaldavStorage(Storage): '''hrefs are full URLs to items''' _session = None + fileext = '.ics' def __init__(self, url, username='', password='', start_date=None, end_date=None, verify=True, auth='basic', useragent='vdirsyncer', _request_func=None, **kwargs): @@ -142,9 +143,7 @@ class CaldavStorage(Storage): data=data, headers=self._default_headers() ) - status_code = response.status_code response.raise_for_status() - c = response.x.get_data() root = etree.XML(response.content) rv = [] hrefs_left = set(hrefs) @@ -161,7 +160,7 @@ class CaldavStorage(Storage): rv.append((href, Item(obj), etag)) hrefs_left.remove(href) for href in hrefs_left: - raise exceptions.NotFoundError(href) + raise exceptions.NotFound(href) return rv def get(self, href): @@ -172,7 +171,7 @@ class CaldavStorage(Storage): def has(self, href): try: self.get(href) - except exceptions.NotFoundError: + except exceptions.PreconditionFailed: return False else: return True @@ -190,11 +189,19 @@ class CaldavStorage(Storage): data=obj.raw, headers=headers ) + if response.status_code == 412: + raise exceptions.PreconditionFailed(response.content) if response.status_code != 201: - raise exceptions.StorageError('Unexpected response with content {}'.format(repr(response.content))) + raise exceptions.StorageError( + 'Unexpected response with content {} and status {}'.format( + repr(response.content), + response.status_code + ) + ) response.raise_for_status() - if not response.headers.get('etag', None): + etag = response.headers.get('etag', None) + if not etag: obj2, etag = self.get(href) assert obj2.raw == obj.raw return href, etag @@ -213,7 +220,8 @@ class CaldavStorage(Storage): ) response.raise_for_status() - if not response.headers.get('etag', None): + etag = response.headers.get('etag', None) + if not etag: obj2, etag = self.get(href) assert obj2.raw == obj.raw return href, etag diff --git a/vdirsyncer/storage/filesystem.py b/vdirsyncer/storage/filesystem.py index a9d43c5..2728703 100644 --- a/vdirsyncer/storage/filesystem.py +++ b/vdirsyncer/storage/filesystem.py @@ -15,11 +15,12 @@ class FilesystemStorage(Storage): '''Saves data in vdir collection mtime is etag filename without path is href''' - def __init__(self, path, **kwargs): + def __init__(self, path, fileext, **kwargs): ''' :param path: Absolute path to a *collection* inside a vdir. ''' self.path = path + self.fileext = fileext super(FilesystemStorage, self).__init__(**kwargs) def _get_filepath(self, href): diff --git a/vdirsyncer/tests/test_storage.py b/vdirsyncer/tests/test_storage.py index 8087199..de23730 100644 --- a/vdirsyncer/tests/test_storage.py +++ b/vdirsyncer/tests/test_storage.py @@ -19,61 +19,65 @@ from vdirsyncer.storage.caldav import CaldavStorage import vdirsyncer.exceptions as exceptions class StorageTests(object): + def _create_bogus_item(self, uid): + return Item('''BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//dmfs.org//mimedir.icalendar//EN +BEGIN:VTODO +CREATED:20130721T142233Z +DTSTAMP:20130730T074543Z +LAST-MODIFIED;VALUE=DATE-TIME:20140122T151338Z +SEQUENCE:2 +SUMMARY:Book: Kowlani - Tödlicher Staub +UID:{} +END:VTODO +END:VCALENDAR +'''.format(uid)) def _get_storage(self, **kwargs): raise NotImplementedError() def test_generic(self): - items = [ - 'UID:1', - 'UID:2', - 'UID:3', - 'UID:4', - 'UID:5', - 'UID:6', - 'UID:7', - 'UID:8', - 'UID:9' - ] - fileext = '.lol' - s = self._get_storage(fileext=fileext) + items = map(self._create_bogus_item, range(1, 10)) + s = self._get_storage() for item in items: - s.upload(Item(item)) + s.upload(item) hrefs = (href for href, etag in s.list()) for href in hrefs: assert s.has(href) obj, etag = s.get(href) - assert obj.raw == 'UID:{}'.format(obj.uid) + assert 'UID:{}'.format(obj.uid) in obj.raw def test_upload_already_existing(self): s = self._get_storage() - item = Item('UID:1') + item = self._create_bogus_item(1) s.upload(item) - self.assertRaises(exceptions.AlreadyExistingError, s.upload, item) + self.assertRaises(exceptions.PreconditionFailed, s.upload, item) def test_update_nonexisting(self): s = self._get_storage() - item = Item('UID:1') - self.assertRaises(exceptions.NotFoundError, s.update, 'huehue', item, 123) + item = self._create_bogus_item(1) + self.assertRaises(exceptions.PreconditionFailed, s.update, 'huehue', item, 123) def test_wrong_etag(self): s = self._get_storage() - obj = Item('UID:1') + obj = self._create_bogus_item(1) href, etag = s.upload(obj) - self.assertRaises(exceptions.WrongEtagError, s.update, href, obj, 'lolnope') - self.assertRaises(exceptions.WrongEtagError, s.delete, href, 'lolnope') + self.assertRaises(exceptions.PreconditionFailed, s.update, href, obj, 'lolnope') + self.assertRaises(exceptions.PreconditionFailed, s.delete, href, 'lolnope') def test_delete_nonexisting(self): s = self._get_storage() - self.assertRaises(exceptions.NotFoundError, s.delete, '1', 123) + self.assertRaises(exceptions.PreconditionFailed, s.delete, '1', 123) class FilesystemStorageTests(TestCase, StorageTests): tmpdir = None def _get_storage(self, **kwargs): path = self.tmpdir = tempfile.mkdtemp() - return FilesystemStorage(path=path, **kwargs) + return FilesystemStorage(path=path, fileext='.txt', **kwargs) def tearDown(self): + print("lol") if self.tmpdir is not None: shutil.rmtree(self.tmpdir) self.tmpdir = None @@ -85,7 +89,6 @@ class MemoryStorageTests(TestCase, StorageTests): class CaldavStorageTests(TestCase, StorageTests): tmpdir = None - old_radicale_config_key = None def _get_storage(self, **kwargs): self.tmpdir = tempfile.mkdtemp() @@ -132,4 +135,3 @@ class CaldavStorageTests(TestCase, StorageTests): if self.tmpdir is not None: shutil.rmtree(self.tmpdir) self.tmpdir = None - diff --git a/vdirsyncer/tests/test_sync.py b/vdirsyncer/tests/test_sync.py index b54b06b..732a01c 100644 --- a/vdirsyncer/tests/test_sync.py +++ b/vdirsyncer/tests/test_sync.py @@ -20,15 +20,15 @@ class SyncTests(TestCase): def test_irrelevant_status(self): a = MemoryStorage() b = MemoryStorage() - status = {'1': ('1.asd', 1234, '1.ics', 2345)} + status = {'1': ('1.txt', 1234, '1.ics', 2345)} sync(a, b, status) assert not status assert empty_storage(a) assert empty_storage(b) def test_missing_status(self): - a = MemoryStorage(fileext='.txt') - b = MemoryStorage(fileext='.asd') + a = MemoryStorage() + b = MemoryStorage() status = {} item = Item('UID:1') a.upload(item) @@ -36,12 +36,12 @@ class SyncTests(TestCase): sync(a, b, status) assert len(status) == 1 assert a.has('1.txt') - assert b.has('1.asd') + assert b.has('1.txt') def test_missing_status_and_different_items(self): return # TODO - a = MemoryStorage(fileext='.txt') - b = MemoryStorage(fileext='.asd') + a = MemoryStorage() + b = MemoryStorage() status = {} item1 = Item('UID:1\nhaha') item2 = Item('UID:1\nhoho') @@ -49,20 +49,20 @@ class SyncTests(TestCase): b.upload(item2) sync(a, b, status) assert status - assert a.get('1.txt')[0].raw == b.get('1.asd')[0].raw + assert a.get('1.txt')[0].raw == b.get('1.txt')[0].raw def test_upload_and_update(self): - a = MemoryStorage(fileext='.txt') - b = MemoryStorage(fileext='.asd') + a = MemoryStorage() + b = MemoryStorage() status = {} item = Item('UID:1') # new item 1 in a a.upload(item) sync(a, b, status) - assert b.get('1.asd')[0].raw == item.raw + assert b.get('1.txt')[0].raw == item.raw item = Item('UID:1\nASDF:YES') # update of item 1 in b - b.update('1.asd', item, b.get('1.asd')[1]) + b.update('1.txt', item, b.get('1.txt')[1]) sync(a, b, status) assert a.get('1.txt')[0].raw == item.raw @@ -74,37 +74,37 @@ class SyncTests(TestCase): item2 = Item('UID:2\nASDF:YES') # update of item 2 in a a.update('2.txt', item2, a.get('2.txt')[1]) sync(a, b, status) - assert b.get('2.asd')[0].raw == item2.raw + assert b.get('2.txt')[0].raw == item2.raw def test_deletion(self): - a = MemoryStorage(fileext='.txt') - b = MemoryStorage(fileext='.asd') + a = MemoryStorage() + b = MemoryStorage() status = {} item = Item('UID:1') a.upload(item) sync(a, b, status) - b.delete('1.asd', b.get('1.asd')[1]) + b.delete('1.txt', b.get('1.txt')[1]) sync(a, b, status) - assert not a.has('1.txt') and not b.has('1.asd') + assert not a.has('1.txt') and not b.has('1.txt') a.upload(item) sync(a, b, status) - assert a.has('1.txt') and b.has('1.asd') + assert a.has('1.txt') and b.has('1.txt') a.delete('1.txt', a.get('1.txt')[1]) sync(a, b, status) - assert not a.has('1.txt') and not b.has('1.asd') + assert not a.has('1.txt') and not b.has('1.txt') def test_already_synced(self): - a = MemoryStorage(fileext='.txt') - b = MemoryStorage(fileext='.asd') + a = MemoryStorage() + b = MemoryStorage() item = Item('UID:1') a.upload(item) b.upload(item) - status = {'1': ('1.txt', a.get('1.txt')[1], '1.asd', b.get('1.asd')[1])} + status = {'1': ('1.txt', a.get('1.txt')[1], '1.txt', b.get('1.txt')[1])} old_status = dict(status) a.update = b.update = a.upload = b.upload = \ lambda *a, **kw: self.fail('Method shouldn\'t have been called.') sync(a, b, status) assert status == old_status - assert a.has('1.txt') and b.has('1.asd') + assert a.has('1.txt') and b.has('1.txt')