From 61bf23588de4f3a06fe94de8b091fe05c0592a76 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Mon, 8 Dec 2014 16:32:23 +0100 Subject: [PATCH] Refactor! --- tests/storage/dav/test_main.py | 8 +- vdirsyncer/storage/dav.py | 158 +++++++++++++++------------------ 2 files changed, 75 insertions(+), 91 deletions(-) diff --git a/tests/storage/dav/test_main.py b/tests/storage/dav/test_main.py index 49ae533..701512c 100644 --- a/tests/storage/dav/test_main.py +++ b/tests/storage/dav/test_main.py @@ -82,14 +82,14 @@ class TestCaldavStorage(DavStorageTests): def test_item_types_performance(self, get_storage_args, item_types, calls_num, monkeypatch, get_item): s = self.storage_class(item_types=item_types, **get_storage_args()) - old_dav_query = s._dav_query + old_parse = s._parse_prop_responses calls = [] - def _dav_query(*a, **kw): + def new_parse(*a, **kw): calls.append(None) - return old_dav_query(*a, **kw) + return old_parse(*a, **kw) - monkeypatch.setattr(s, '_dav_query', _dav_query) + monkeypatch.setattr(s, '_parse_prop_responses', new_parse) list(s.list()) assert len(calls) == calls_num diff --git a/vdirsyncer/storage/dav.py b/vdirsyncer/storage/dav.py index dc47564..2f18362 100644 --- a/vdirsyncer/storage/dav.py +++ b/vdirsyncer/storage/dav.py @@ -307,18 +307,11 @@ class DavStorage(Storage): root = _parse_xml(response.content) # etree only can handle bytes rv = [] hrefs_left = set(hrefs) - for element in root.iter('{DAV:}response'): - href = self._normalize_href( - element.find('{DAV:}href').text) - + for href, etag, prop in self._parse_prop_responses(root): try: - prop = element.find('{DAV:}propstat').find('{DAV:}prop') raw = prop.find(self.get_multi_data_query).text - etag = prop.find('{DAV:}getetag').text - if not raw or not etag: - raise AttributeError() except AttributeError: - dav_logger.warning('Skipping {}, properties are missing.' + dav_logger.warning('Skipping {}, the item content is missing.' .format(href)) continue @@ -326,6 +319,7 @@ class DavStorage(Storage): raw = raw.decode(response.encoding) if isinstance(etag, bytes): etag = etag.decode(response.encoding) + try: hrefs_left.remove(href) except KeyError: @@ -385,48 +379,55 @@ class DavStorage(Storage): headers=headers ) - def _dav_query(self, xml): - headers = self.session.get_default_headers() + def _parse_prop_responses(self, root, decoding_rounds=1): + hrefs = set() + for response in root.iter('{DAV:}response'): + try: + href = response.find('{DAV:}href').text + except AttributeError: + dav_logger.error('Skipping response, href is missing.') + continue - # CardDAV: The request MUST include a Depth header. The scope of the - # query is determined by the value of the Depth header. For example, - # to query all address object resources in an address book collection, - # the REPORT would use the address book collection as the Request- URI - # and specify a Depth of 1 or infinity. - # http://tools.ietf.org/html/rfc6352#section-8.6 - # - # CalDAV: - # The request MAY include a Depth header. If no Depth header is - # included, Depth:0 is assumed. - # http://tools.ietf.org/search/rfc4791#section-7.8 - headers['Depth'] = 'infinity' - response = self.session.request( - 'REPORT', - '', - data=xml, - headers=headers - ) - root = _parse_xml(response.content) - for element in root.iter('{DAV:}response'): - propstat = element.find('{DAV:}propstat') - prop = propstat.find('{DAV:}prop') - - contenttype = prop.find('{DAV:}getcontenttype').text - - href = self._normalize_href(element.find('{DAV:}href').text) - etag = prop.find('{DAV:}getetag').text - if not etag: - raise ValueError('Server did not return an etag for item {}. ' - 'Vdirsyncer is unable to function without ' - 'one.'.format(href)) - - if not contenttype or self.item_mimetype in contenttype: - yield href, etag + href = self._normalize_href(href, decoding_rounds=decoding_rounds) + if href in hrefs: + # Servers that send duplicate hrefs: + # - Zimbra + # https://github.com/untitaker/vdirsyncer/issues/88 + # - Davmail + # https://github.com/untitaker/vdirsyncer/issues/144 + dav_logger.warning('Skipping identical href: {!r}' + .format(href)) + continue else: - dav_logger.debug( - 'Skipping item with href {!r} ' - 'because content type {!r} != {!r}.' - .format(href, contenttype, self.item_mimetype)) + hrefs.add(href) + + try: + prop = response.find('{DAV:}propstat/{DAV:}prop') + contenttype = prop.find('{DAV:}getcontenttype') + etag = prop.find('{DAV:}getetag').text + except AttributeError: + dav_logger.warning('Skipping {!r}, properties are missing.' + .format(href)) + continue + + is_collection = prop.find('{DAV:}resourcetype/{DAV:}collection') + if is_collection is not None: + dav_logger.debug('Skipping {!r}, is collection.'.format(href)) + continue + + # different servers give different getcontenttypes: + # "text/vcard", "text/x-vcard", "text/x-vcard; charset=utf-8", + # "text/directory;profile=vCard", "text/directory", + # "text/vcard; charset=utf-8" + if self.item_mimetype is not None: + mediatype, subtype = self.item_mimetype.split('/') + if contenttype is not None and subtype not in contenttype.text: + dav_logger.debug('Skipping {!r}, {!r} != {!r}.' + .format(href, contenttype, + self.item_mimetype)) + continue + + yield href, etag, prop class CaldavStorage(DavStorage): @@ -550,22 +551,29 @@ class CaldavStorage(DavStorage): ''' - hrefs = set() + # CardDAV: The request MUST include a Depth header. The scope of the + # query is determined by the value of the Depth header. For example, + # to query all address object resources in an address book collection, + # the REPORT would use the address book collection as the Request- URI + # and specify a Depth of 1 or infinity. + # http://tools.ietf.org/html/rfc6352#section-8.6 + # + # CalDAV: The request MAY include a Depth header. If no Depth header + # is included, Depth:0 is assumed. + # http://tools.ietf.org/search/rfc4791#section-7.8 + headers = self.session.get_default_headers() + headers['Depth'] = 'infinity' caldavfilters = self._get_list_filters(self.item_types, self.start_date, self.end_date) for caldavfilter in caldavfilters: xml = data.format(caldavfilter=caldavfilter) - for href, etag in self._dav_query(xml): - if href in hrefs: - # Can't do stronger assertion here, see - # https://github.com/untitaker/vdirsyncer/issues/88 - dav_logger.warning('Skipping identical href: {}' - .format(href)) - continue - - hrefs.add(href) + response = self.session.request('REPORT', '', data=xml, + headers=headers) + root = _parse_xml(response.content) + rv = self._parse_prop_responses(root) + for href, etag, prop in rv: yield href, etag @@ -612,33 +620,9 @@ class CarddavStorage(DavStorage): headers=headers) root = _parse_xml(response.content) - hrefs = set() - for element in root.iter('{DAV:}response'): - prop = element.find('{DAV:}propstat').find('{DAV:}prop') - resource_type = prop.find('{DAV:}resourcetype') - if resource_type.find('{DAV:}collection') is not None: - continue - - content_type = prop.find('{DAV:}getcontenttype') - # different servers give different getcontenttypes: - # "text/vcard", "text/x-vcard", "text/x-vcard; charset=utf-8", - # "text/directory;profile=vCard", "text/directory", - # "text/vcard; charset=utf-8" - if 'vcard' not in content_type.text.lower(): - continue - - # Decode twice because ownCloud encodes twice. - # See https://github.com/owncloud/contacts/issues/581 - href = self._normalize_href(element.find('{DAV:}href').text, - decoding_rounds=2) - etag = prop.find('{DAV:}getetag').text - - if href in hrefs: - # Can't do stronger assertion here, see - # https://github.com/untitaker/vdirsyncer/issues/88 - dav_logger.warning('Skipping identical href: {}'.format(href)) - continue - - hrefs.add(href) + # Decode twice because ownCloud encodes twice. + # See https://github.com/owncloud/contacts/issues/581 + rv = self._parse_prop_responses(root, decoding_rounds=2) + for href, etag, prop in rv: yield href, etag