Merge pull request #365 from untitaker/fix-358-again

Refactor metadata None values

tests/__init__.py

@@ -3,6 +3,8 @@
 Test suite for vdirsyncer.
 '''
 
+import hypothesis.strategies as st
+
 from vdirsyncer.utils.vobject import normalize_item
 
 
@@ -66,3 +68,9 @@ UID:{uid}
 X-SOMETHING:{r}
 HAHA:YES
 END:FOO'''
+
+printable_characters_strategy = st.text(
+    st.characters(blacklist_categories=(
+        'Cc', 'Cs'
+    ))
+)

tests/cli/test_repair.py

@@ -12,7 +12,7 @@ from vdirsyncer.utils.vobject import Item
 
 uid_strategy = st.text(st.characters(blacklist_categories=(
     'Zs', 'Zl', 'Zp',
-    'Cc'
+    'Cc', 'Cs'
 )))
 
 

tests/storage/__init__.py

@@ -2,14 +2,17 @@
 
 import random
 
+from hypothesis import given
+import hypothesis.strategies as st
+
 import pytest
 
 import vdirsyncer.exceptions as exceptions
-from vdirsyncer.storage.base import Item
+from vdirsyncer.storage.base import Item, normalize_meta_value
 from vdirsyncer.utils.compat import iteritems, text_type, urlquote, urlunquote
 
 from .. import EVENT_TEMPLATE, TASK_TEMPLATE, VCARD_TEMPLATE, \
-    assert_item_equals
+    assert_item_equals, printable_characters_strategy
 
 
 def get_server_mixin(server_name):
@@ -269,12 +272,12 @@ class StorageTests(object):
             pytest.skip('ownCloud is fundamentally broken.')
 
         if not getattr(self, 'dav_server', ''):
-            assert s.get_meta('color') is None
+            assert not s.get_meta('color')
-            assert s.get_meta('displayname') is None
+            assert not s.get_meta('displayname')
 
         try:
             s.set_meta('color', None)
-            assert s.get_meta('color') is None
+            assert not s.get_meta('color')
             s.set_meta('color', u'#ff0000')
             assert s.get_meta('color') == u'#ff0000'
         except exceptions.UnsupportedMetadataError:
@@ -285,3 +288,17 @@ class StorageTests(object):
             rv = s.get_meta('displayname')
             assert rv == x
             assert isinstance(rv, text_type)
+
+    @given(value=st.one_of(
+        st.none(),
+        printable_characters_strategy.filter(lambda x: x.strip() != x)
+    ))
+    def test_metadata_normalization(self, requires_metadata, s, value):
+        x = s.get_meta('displayname')
+        assert x == normalize_meta_value(x)
+
+        if not getattr(self, 'dav_server', None):
+            # ownCloud replaces "" with "unnamed"
+            # Also https://github.com/owncloud/core/issues/18409
+            s.set_meta('displayname', value)
+            assert s.get_meta('displayname') == normalize_meta_value(value)

tests/test_metasync.py

@@ -1,8 +1,5 @@
 # -*- coding: utf-8 -*-
 
-from hypothesis import given
-import hypothesis.strategies as st
-
 import pytest
 
 from vdirsyncer.metasync import MetaSyncConflict, metasync
@@ -42,7 +39,7 @@ def test_basic(monkeypatch):
 
     b.set_meta('foo', None)
     metasync(a, b, status, keys=['foo'])
-    assert a.get_meta('foo') is b.get_meta('foo') is None
+    assert not a.get_meta('foo') and not b.get_meta('foo')
 
 
 def test_conflict():
@@ -85,17 +82,3 @@ def test_conflict_x_wins(wins):
     assert a.get_meta('foo') == b.get_meta('foo') == status['foo'] == (
         'bar' if wins == 'a' else 'baz'
     )
-
-
-@given(s=st.text().filter(lambda x: x.strip() == x), key=st.text())
-def test_trailing_newline(s, key):
-    a = MemoryStorage()
-    b = MemoryStorage()
-    status = {}
-    a.set_meta(key, s + u'\n')
-    b.set_meta(key, s)
-    a.set_meta = b.set_meta = blow_up
-
-    metasync(a, b, status, keys=[key])
-
-    assert status[key] == s

vdirsyncer/metasync.py

@@ -1,5 +1,5 @@
 from . import exceptions, log
-from .utils.compat import text_type
+from .storage.base import normalize_meta_value
 
 logger = log.get(__name__)
 
@@ -34,9 +34,9 @@ def metasync(storage_a, storage_b, status, keys, conflict_resolution=None):
             _b_to_a()
 
     for key in keys:
-        a = _normalize_value(storage_a.get_meta(key))
+        a = storage_a.get_meta(key)
-        b = _normalize_value(storage_b.get_meta(key))
+        b = storage_b.get_meta(key)
-        s = status.get(key)
+        s = normalize_meta_value(status.get(key))
         logger.debug(u'Key: {}'.format(key))
         logger.debug(u'A: {}'.format(a))
         logger.debug(u'B: {}'.format(b))
@@ -53,11 +53,3 @@ def metasync(storage_a, storage_b, status, keys, conflict_resolution=None):
 
     for key in set(status) - set(keys):
         del status[key]
-
-
-def _normalize_value(value):
-    if value is None:
-        return value
-    else:
-        assert isinstance(value, (bytes, text_type))
-        return value.strip()

vdirsyncer/storage/base.py

@@ -5,7 +5,7 @@ import functools
 
 from .. import exceptions, sync
 from ..utils import uniq
-from ..utils.compat import to_native, with_metaclass
+from ..utils.compat import to_native, to_unicode, with_metaclass
 from ..utils.vobject import Item  # noqa
 
 
@@ -238,3 +238,7 @@ class Storage(with_metaclass(StorageMeta)):
         '''
 
         raise NotImplementedError('This storage does not support metadata.')
+
+
+def normalize_meta_value(value):
+    return to_unicode(value or u'').strip()

vdirsyncer/storage/dav.py

@@ -7,11 +7,11 @@ from lxml import etree
 import requests
 from requests.exceptions import HTTPError
 
-from .base import Item, Storage
+from .base import Item, Storage, normalize_meta_value
 from .http import HTTP_STORAGE_PARAMETERS, USERAGENT, prepare_auth, \
     prepare_client_cert, prepare_verify
 from .. import exceptions, log, utils
-from ..utils.compat import text_type, to_native, to_unicode
+from ..utils.compat import text_type, to_native
 
 
 dav_logger = log.get(__name__)
@@ -597,9 +597,10 @@ class DavStorage(Storage):
         root = _parse_xml(response.content)
 
         for prop in root.findall('.//' + lxml_selector):
-            text = getattr(prop, 'text', None)
+            text = normalize_meta_value(getattr(prop, 'text', None))
             if text:
-                return to_unicode(text)
+                return text
+        return u''
 
     def set_meta(self, key, value):
         try:
@@ -609,7 +610,7 @@ class DavStorage(Storage):
 
         lxml_selector = '{%s}%s' % (namespace, tagname)
         element = etree.Element(lxml_selector)
-        element.text = value
+        element.text = normalize_meta_value(value)
 
         data = '''<?xml version="1.0" encoding="utf-8" ?>
             <D:propertyupdate xmlns:D="DAV:">

vdirsyncer/storage/filesystem.py

@@ -6,7 +6,7 @@ import subprocess
 
 from atomicwrites import atomic_write
 
-from .base import Item, Storage
+from .base import Item, Storage, normalize_meta_value
 from .. import exceptions, log
 from ..utils import checkdir, expand_path, generate_href, get_etag_from_file, \
     get_etag_from_fileobject
@@ -183,16 +183,16 @@ class FilesystemStorage(Storage):
         fpath = os.path.join(self.path, key)
         try:
             with open(fpath, 'rb') as f:
-                return f.read().decode(self.encoding) or None
+                return normalize_meta_value(f.read().decode(self.encoding))
         except IOError as e:
             if e.errno == errno.ENOENT:
-                return None
+                return u''
             else:
                 raise
 
     def set_meta(self, key, value):
-        value = value or u''
+        value = normalize_meta_value(value)
-        assert isinstance(value, text_type)
+
         fpath = os.path.join(self.path, key)
         with atomic_write(fpath, mode='wb', overwrite=True) as f:
             f.write(value.encode(self.encoding))

vdirsyncer/storage/memory.py

@@ -2,8 +2,9 @@
 
 import random
 
-import vdirsyncer.exceptions as exceptions
+from .base import Storage, normalize_meta_value
-from vdirsyncer.storage.base import Storage
+
+from .. import exceptions
 
 
 def _random_string():
@@ -66,7 +67,7 @@ class MemoryStorage(Storage):
         del self.items[href]
 
     def get_meta(self, key):
-        return self.metadata.get(key)
+        return normalize_meta_value(self.metadata.get(key))
 
     def set_meta(self, key, value):
-        self.metadata[key] = value
+        self.metadata[key] = normalize_meta_value(value)

vdirsyncer/storage/remotestorage.py

@@ -19,7 +19,7 @@ We also use a custom ``data``-URI for the redirect in OAuth:
 
 import click
 
-from .base import Item, Storage
+from .base import Item, Storage, normalize_meta_value
 from .http import HTTP_STORAGE_PARAMETERS, prepare_client_cert, \
     prepare_verify
 from .. import exceptions, log, utils
@@ -225,15 +225,15 @@ class RemoteStorage(Storage):
 
     def get_meta(self, key):
         try:
-            return self.session.request('GET', key).text or None
+            return normalize_meta_value(self.session.request('GET', key).text)
         except exceptions.NotFoundError:
-            pass
+            return u''
 
     def set_meta(self, key, value):
         self.session.request(
             'PUT',
             key,
-            data=(value or u'').encode('utf-8'),
+            data=normalize_meta_value(value).encode('utf-8'),
             headers={'Content-Type': 'text/plain; charset=utf-8'}
         )