repair: More tests, slight refactor

tests/system/cli/test_repair.py

@@ -5,16 +5,20 @@ from textwrap import dedent
 import pytest
 
 
-@pytest.mark.parametrize('collection', [None, "foocoll"])
-def test_full(tmpdir, runner, collection):
+@pytest.fixture
+def storage(tmpdir, runner):
     runner.write_with_general(dedent('''
-        [storage foo]
-        type = "filesystem"
-        path = "{base}/foo/"
-        fileext = ".txt"
-        ''').format(base=str(tmpdir)))
+    [storage foo]
+    type = "filesystem"
+    path = "{base}/foo/"
+    fileext = ".txt"
+    ''').format(base=str(tmpdir)))
 
-    storage = tmpdir.mkdir('foo')
+    return tmpdir.mkdir('foo')
+
+
+@pytest.mark.parametrize('collection', [None, "foocoll"])
+def test_basic(storage, runner, collection):
     if collection is not None:
         storage = storage.mkdir(collection)
         collection_arg = 'foo/{}'.format(collection)
@@ -32,7 +36,38 @@ def test_full(tmpdir, runner, collection):
     result = runner.invoke(argv, input='y')
     assert not result.exception
     assert 'No UID' in result.output
-    assert 'warning: Item toobroken.txt can\'t be parsed, skipping' \
+    assert '\'toobroken.txt\' is malformed beyond repair' \
         in result.output
     new_fname, = [x for x in storage.listdir() if 'toobroken' not in str(x)]
     assert 'UID:' in new_fname.read()
+
+
+@pytest.mark.parametrize('repair_uids', [None, True, False])
+def test_repair_uids(storage, runner, repair_uids):
+    f = storage.join('baduid.txt')
+    orig_f = 'BEGIN:VCARD\nUID:!!!!!\nEND:VCARD'
+    f.write(orig_f)
+
+    if repair_uids is None:
+        opt = []
+    elif repair_uids:
+        opt = ['--repair-unsafe-uid']
+    else:
+        opt = ['--no-repair-unsafe-uid']
+
+    result = runner.invoke(['repair'] + opt + ['foo'], input='y')
+    assert not result.exception
+
+    if repair_uids:
+        assert 'UID or href is unsafe, assigning random UID' in result.output
+        assert not f.exists()
+        new_f, = storage.listdir()
+        s = new_f.read()
+
+        assert s.startswith('BEGIN:VCARD')
+        assert s.endswith('END:VCARD')
+        assert s != orig_f
+    else:
+        assert 'UID may cause problems, add --repair-unsafe-uid to repair.' \
+            in result.output
+        assert f.read() == orig_f

vdirsyncer/repair.py

@@ -8,6 +8,10 @@ from .utils import generate_href, href_safe
 logger = logging.getLogger(__name__)
 
 
+class IrreparableItem(Exception):
+    pass
+
+
 def repair_storage(storage, repair_unsafe_uid):
     seen_uids = set()
     all_hrefs = list(storage.list())
@@ -16,30 +20,12 @@ def repair_storage(storage, repair_unsafe_uid):
         logger.info(u'[{}/{}] Processing {}'
                     .format(i, len(all_hrefs), href))
 
-        new_item = item
-
-        if item.parsed is None:
-            logger.warning('Item {} can\'t be parsed, skipping.'
-                           .format(href))
-            continue
-
-        if not item.uid:
-            logger.warning('No UID, assigning random one.')
-            new_item = item.with_uid(generate_href())
-        elif item.uid in seen_uids:
-            logger.warning('Duplicate UID, assigning random one.')
-            new_item = item.with_uid(generate_href())
-        elif not href_safe(item.uid) or not href_safe(basename(href)):
-            if not repair_unsafe_uid:
-                logger.warning('UID or href may cause problems, add '
-                               '--repair-unsafe-hrefs to repair.')
-            else:
-                logger.warning('UID or href is unsafe, assigning random UID.')
-                new_item = item.with_uid(generate_href(item.uid))
-
-        if not new_item.uid:
+        try:
+            new_item = repair_item(href, item, seen_uids, repair_unsafe_uid)
+        except IrreparableItem:
             logger.error('Item {!r} is malformed beyond repair. '
-                         'This is a serverside bug.'
+                         'The PRODID property may indicate which software '
+                         'created this item.'
                          .format(href))
             logger.error('Item content: {!r}'.format(item.raw))
             continue
@@ -54,3 +40,29 @@ def repair_storage(storage, repair_unsafe_uid):
                     storage.update(href, new_item, etag)
             except Exception:
                 logger.exception('Server rejected new item.')
+
+
+def repair_item(href, item, seen_uids, repair_unsafe_uid):
+    if item.parsed is None:
+        raise IrreparableItem()
+
+    new_item = item
+
+    if not item.uid:
+        logger.warning('No UID, assigning random UID.')
+        new_item = item.with_uid(generate_href())
+    elif item.uid in seen_uids:
+        logger.warning('Duplicate UID, assigning random UID.')
+        new_item = item.with_uid(generate_href())
+    elif not href_safe(item.uid) or not href_safe(basename(href)):
+        if not repair_unsafe_uid:
+            logger.warning('UID may cause problems, add '
+                           '--repair-unsafe-uid to repair.')
+        else:
+            logger.warning('UID or href is unsafe, assigning random UID.')
+            new_item = item.with_uid(generate_href(item.uid))
+
+    if not new_item.uid:
+        raise IrreparableItem()
+
+    return new_item