Handle collections correctly

Fix #132

Passing the collections parameter used to mean that the storage should
append its value to the URL or path. This was a leaky abstraction for
the reasons explained in #132.

The new behavior removes this meaning from this parameter. Vdirsyncer
now maintains a cache of discovered collections.

Also, section names are now validated more strictly, in order to avoid
problems with the status files' naming.

docs/config.rst

@@ -51,6 +51,8 @@ Pair Section
     b = ...
     #conflict_resolution = ...
 
+- Pair names can consist of any alphanumeric characters and the underscore.
+
 - ``a`` and ``b`` reference the storages to sync by their names.
 
 - ``collections``: Optional, a comma-separated list of collections to
@@ -84,6 +86,8 @@ Storage Section
     [storage storage_name]
     type = ...
 
+- Storage names can consist of any alphanumeric characters and the underscore.
+
 - ``type`` defines which kind of storage is defined. See :ref:`storages`.
 
 - ``read_only`` defines whether the storage should be regarded as a read-only

tests/storage/__init__.py

@@ -148,22 +148,30 @@ class SupportsCollections(object):
 
     def test_discover(self, get_storage_args, get_item):
         expected = set()
+        items = {}
 
         for i in range(1, 5):
             # Create collections, but use the "collection" attribute because
             # Radicale requires file extensions in their names.
-            expected.add(
-                self.storage_class(
-                    **get_storage_args(collection='test{}'.format(i))
-                ).collection
-            )
+            collection = 'test{}'.format(i)
+            s = self.storage_class(**get_storage_args(collection=collection))
+            items[s.collection] = [s.upload(get_item())]
+            expected.add(s.collection)
 
-        d = self.storage_class.discover(
-            **get_storage_args(collection=None))
+        d = list(self.storage_class.discover(
+            **get_storage_args(collection=None)))
 
-        actual = set(s.collection for s in d)
+        actual = set(args['collection'] for args in d)
         assert actual >= expected
 
+        for storage_args in d:
+            collection = storage_args['collection']
+            if collection not in expected:
+                continue
+            s = self.storage_class(**storage_args)
+            rv = list(s.list())
+            assert rv == items[collection]
+
     def test_discover_collection_arg(self, get_storage_args):
         args = get_storage_args(collection='test2')
         with pytest.raises(TypeError) as excinfo:

tests/storage/dav/servers/radicale/__init__.py

@@ -110,6 +110,7 @@ def inner(collection='test'):
             url = 'http://127.0.0.1/bob/'
             if collection is not None:
                 collection += self.storage_class.fileext
+                url = url.rstrip('/') + '/' + collection
 
             rv = {'url': url, 'username': 'bob', 'password': 'bob',
                   'collection': collection, 'unsafe_href_chars': ''}

tests/storage/test_filesystem.py

@@ -31,22 +31,22 @@ def inner(collection=None):
             path = self.tmpdir
             if collection is not None:
                 os.makedirs(os.path.join(path, collection))
+                path = os.path.join(path, collection)
             return {'path': path, 'fileext': '.txt', 'collection': collection}
         return inner
 
     def test_create_is_false(self, tmpdir):
         with pytest.raises(IOError):
-            self.storage_class(str(tmpdir), '.txt', collection='lol',
-                               create=False)
+            self.storage_class(str(tmpdir) + '/lol', '.txt', create=False)
 
     def test_is_not_directory(self, tmpdir):
         with pytest.raises(IOError):
             f = tmpdir.join('hue')
             f.write('stub')
-            self.storage_class(str(tmpdir), '.txt', collection='hue')
+            self.storage_class(str(tmpdir) + '/hue', '.txt')
 
     def test_create_is_true(self, tmpdir):
-        self.storage_class(str(tmpdir), '.txt', collection='asd')
+        self.storage_class(str(tmpdir) + '/asd', '.txt')
         assert tmpdir.listdir() == [tmpdir.join('asd')]
 
     def test_broken_data(self, tmpdir):

tests/test_cli.py

@@ -6,49 +6,48 @@
     :copyright: (c) 2014 Markus Unterwaditzer & contributors
     :license: MIT, see LICENSE for more details.
 '''
+import io
 from textwrap import dedent
 
 from click.testing import CliRunner
 
+import pytest
+
 import vdirsyncer.cli as cli
 
 
-def test_load_config(tmpdir, monkeypatch):
-    f = tmpdir.join('test.cfg')
-    status_path = '{}/status/'.format(str(tmpdir))
-    contacts_path = '{}/contacts/'.format(str(tmpdir))
-    f.write(dedent('''
-        [general]
-        status_path = {status}
+def test_load_config(monkeypatch):
+    f = io.StringIO(dedent(u'''
+    [general]
+    status_path = /tmp/status/
 
-        [pair bob]
-        a = bob_a
-        b = bob_b
-        foo = bar
-        bam = true
+    [pair bob]
+    a = bob_a
+    b = bob_b
+    foo = bar
+    bam = true
 
-        [storage bob_a]
-        type = filesystem
-        path = {contacts}
-        fileext = .vcf
-        yesno = off
-        number = 42
+    [storage bob_a]
+    type = filesystem
+    path = /tmp/contacts/
+    fileext = .vcf
+    yesno = off
+    number = 42
 
-        [storage bob_b]
-        type = carddav
+    [storage bob_b]
+    type = carddav
 
-        [bogus]
-        lol = true
-        ''').strip().format(status=status_path, contacts=contacts_path))
+    [bogus]
+    lol = true
+    '''))
 
-    fname = str(tmpdir) + '/test.cfg'
     errors = []
     monkeypatch.setattr('vdirsyncer.cli.cli_logger.error', errors.append)
-    general, pairs, storages = cli.load_config(fname, pair_options=('bam',))
-    assert general == {'status_path': status_path}
+    general, pairs, storages = cli.load_config(f, pair_options=('bam',))
+    assert general == {'status_path': '/tmp/status/'}
     assert pairs == {'bob': ('bob_a', 'bob_b', {'bam': True}, {'foo': 'bar'})}
     assert storages == {
-        'bob_a': {'type': 'filesystem', 'path': contacts_path, 'fileext':
+        'bob_a': {'type': 'filesystem', 'path': '/tmp/contacts/', 'fileext':
                   '.vcf', 'yesno': False, 'number': 42,
                   'instance_name': 'bob_a'},
         'bob_b': {'type': 'carddav', 'instance_name': 'bob_b'}
@@ -80,11 +79,9 @@ def test_parse_pairs_args():
     assert list(
         cli.parse_pairs_args(['foo/foocoll', 'one', 'eins'], pairs)
     ) == [
-        ('foo', 'foocoll'),
-        ('one', 'a'),
-        ('one', 'b'),
-        ('one', 'c'),
-        ('eins', None)
+        ('foo', ['foocoll']),
+        ('one', ['a', 'b', 'c']),
+        ('eins', [None])
     ]
 
 
@@ -161,8 +158,8 @@ def test_empty_storage(tmpdir):
     assert result.output.splitlines() == [
         'Syncing my_pair',
         'error: {status_name}: Storage "{name}" was completely emptied. Use '
-        '"--force-delete {status_name}" to synchronize that emptyness to '
-        'the other side, or delete the status by yourself to restore the '
+        '"--force-delete {status_name}" to synchronize that emptyness to the '
+        'other side, or delete the `.items`-file yourself to restore the '
         'items from the non-empty side.'.format(status_name='my_pair',
                                                 name='my_b')
     ]
@@ -234,3 +231,17 @@ def test_verbosity(tmpdir):
     )
     assert result.exception
     assert 'invalid verbosity value' in result.output.lower()
+
+
+def test_invalid_storage_name():
+    f = io.StringIO(dedent(u'''
+    [general]
+    status_path = /tmp/status/
+
+    [storage foo.bar]
+    '''))
+
+    with pytest.raises(cli.CliError) as excinfo:
+        cli.load_config(f)
+
+    assert 'invalid characters' in str(excinfo.value).lower()

tests/utils/test_main.py

@@ -246,7 +246,8 @@ def test_get_class_init_args_on_storage():
     from vdirsyncer.storage.memory import MemoryStorage
 
     all, required = utils.get_class_init_args(MemoryStorage)
-    assert all == set(['fileext', 'collection', 'read_only', 'instance_name'])
+    assert all == set(['fileext', 'collection', 'read_only', 'instance_name',
+                       'collection_human'])
     assert not required