Improvements in exception handling

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):
+class PreconditionFailed(Error):
-    '''The item exists although it shouldn't, possible race condition.'''
+    '''
-    pass
+      - 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

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):
+    fileext = '.txt'
-        self.fileext = fileext
+    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
+        Update the object, raise
-        the etag on the server doesn't match the given etag, raise
+        :exc:`vdirsyncer.exceptions.PreconditionFailed` if the etag on the
-        :exc:`vdirsyncer.exceptions.NotFoundError` if the item doesn't exist.
+        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
+        Delete the object by href, raise
-        return value
+        :exc:`vdirsyncer.exceptions.PreconditionFailed` when item has a
+        different etag or doesn't exist.
         '''
         raise NotImplementedError()

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

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):

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 = [
+        items = map(self._create_bogus_item, range(1, 10))
-            'UID:1',
+        s = self._get_storage()
-            'UID:2',
-            'UID:3',
-            'UID:4',
-            'UID:5',
-            'UID:6',
-            'UID:7',
-            'UID:8',
-            'UID:9'
-        ]
-        fileext = '.lol'
-        s = self._get_storage(fileext=fileext)
         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')
+        item = self._create_bogus_item(1)
-        self.assertRaises(exceptions.NotFoundError, s.update, 'huehue', item, 123)
+        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.PreconditionFailed, s.update, href, obj, 'lolnope')
-        self.assertRaises(exceptions.WrongEtagError, s.delete, href, '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
-

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')
+        a = MemoryStorage()
-        b = MemoryStorage(fileext='.asd')
+        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')
+        a = MemoryStorage()
-        b = MemoryStorage(fileext='.asd')
+        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')
+        a = MemoryStorage()
-        b = MemoryStorage(fileext='.asd')
+        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')
+        a = MemoryStorage()
-        b = MemoryStorage(fileext='.asd')
+        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')
+        a = MemoryStorage()
-        b = MemoryStorage(fileext='.asd')
+        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')