From ccc3dee28b124d6c7c11ece9dc6e96fadc24c9e1 Mon Sep 17 00:00:00 2001
From: Markus Unterwaditzer <markus@unterwaditzer.net>
Date: Sat, 3 Jan 2015 16:26:56 +0100
Subject: [PATCH] Handle UID conflicts during sync

---
 CHANGELOG.rst      |  2 ++
 tests/test_sync.py | 20 +++++++++++++++-
 vdirsyncer/sync.py | 60 ++++++++++++++++++++++++++++++++++++----------
 3 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 1b1ad17..81066aa 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -18,6 +18,8 @@ Version 0.4.1
   it should try to create collections.
 - The old config values ``True``, ``False``, ``on``, ``off`` and ``None`` are
   now invalid.
+- UID conflicts are now properly handled instead of ignoring one item. Card-
+  and CalDAV servers are already supposed to take care of those though.
 
 Version 0.4.0
 =============
diff --git a/tests/test_sync.py b/tests/test_sync.py
index d5779a9..493b6b0 100644
--- a/tests/test_sync.py
+++ b/tests/test_sync.py
@@ -12,7 +12,8 @@ import pytest
 import vdirsyncer.exceptions as exceptions
 from vdirsyncer.storage.base import Item
 from vdirsyncer.storage.memory import MemoryStorage
-from vdirsyncer.sync import BothReadOnly, StorageEmpty, SyncConflict, sync
+from vdirsyncer.sync import BothReadOnly, IdentConflict, StorageEmpty, \
+    SyncConflict, sync
 
 from . import assert_item_equals, normalize_item
 
@@ -264,3 +265,20 @@ def test_readonly():
     assert len(status) == 2 and a.has(href_a) and not b.has(href_a)
     sync(a, b, status)
     assert len(status) == 1 and not a.has(href_a) and not b.has(href_a)
+
+
+@pytest.mark.parametrize('sync_inbetween', (True, False))
+def test_ident_conflict(sync_inbetween):
+    a = MemoryStorage()
+    b = MemoryStorage()
+    status = {}
+    href_a, etag_a = a.upload(Item(u'UID:aaa'))
+    href_b, etag_b = a.upload(Item(u'UID:bbb'))
+    if sync_inbetween:
+        sync(a, b, status)
+
+    a.update(href_a, Item(u'UID:xxx'), etag_a)
+    a.update(href_b, Item(u'UID:xxx'), etag_b)
+
+    with pytest.raises(IdentConflict):
+        sync(a, b, status)
diff --git a/vdirsyncer/sync.py b/vdirsyncer/sync.py
index 9c9772d..117f390 100644
--- a/vdirsyncer/sync.py
+++ b/vdirsyncer/sync.py
@@ -42,6 +42,27 @@ class SyncConflict(SyncError):
     href_b = None
 
 
+class IdentConflict(SyncError):
+    '''
+    Multiple items on the same storage have the same UID.
+
+    :param storage: The affected storage.
+    :param hrefs: List of affected hrefs on `storage`.
+    '''
+    storage = None
+    _hrefs = None
+
+    @property
+    def hrefs(self):
+        return self._hrefs
+
+    @hrefs.setter
+    def hrefs(self, val):
+        val = set(val)
+        assert len(val) > 1
+        self._hrefs = val
+
+
 class StorageEmpty(SyncError):
     '''
     One storage unexpectedly got completely empty between two synchronizations.
@@ -61,7 +82,23 @@ class BothReadOnly(SyncError):
     '''
 
 
-def _prepare_idents(storage, other_storage, href_to_status):
+def _prefetch(storage, rv, hrefs):
+    if rv is None:
+        rv = {}
+    if not hrefs:
+        return rv
+
+    for href, item, etag in storage.get_multi(hrefs):
+        props = rv[href]
+        props['item'] = item
+        props['ident'] = item.ident
+        if props['etag'] != etag:
+            raise SyncError('Etag changed during sync.')
+
+    return rv
+
+
+def _prepare_hrefs(storage, other_storage, href_to_status):
     hrefs = {}
     download = []
     for href, etag in storage.list():
@@ -75,21 +112,18 @@ def _prepare_idents(storage, other_storage, href_to_status):
             download.append(href)
 
     _prefetch(storage, hrefs, download)
-    return dict((x['ident'], x) for href, x in iteritems(hrefs))
+    return hrefs
 
 
-def _prefetch(storage, rv, hrefs):
-    if rv is None:
-        rv = {}
-    if not hrefs:
-        return rv
+def _prepare_idents(storage, other_storage, href_to_status):
+    hrefs = _prepare_hrefs(storage, other_storage, href_to_status)
 
-    for href, item, etag in storage.get_multi(hrefs):
-        props = rv[href]
-        props['item'] = item
-        props['ident'] = item.ident
-        if props['etag'] != etag:
-            raise SyncError('Etag changed during sync.')
+    rv = {}
+    for href, props in iteritems(hrefs):
+        other_props = rv.setdefault(props['ident'], props)
+        if other_props != props:
+            raise IdentConflict(storage=storage,
+                                hrefs=[props['href'], other_props['href']])
 
     return rv