From a46848d73dab2149a18f459f8c1d5ccb6cc96885 Mon Sep 17 00:00:00 2001 From: Nicco Kunzmann Date: Thu, 6 Mar 2025 11:08:06 +0100 Subject: [PATCH 1/3] Refactor If/Else statement --- caldav/davclient.py | 80 ++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 44 deletions(-) diff --git a/caldav/davclient.py b/caldav/davclient.py index c18ea44a..0d2b99f8 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -76,53 +76,35 @@ def __init__( ## TODO: this if/else/elif could possibly be refactored, or we should ## consider to do streaming into the xmltree library as originally ## intended. It only makes sense for really huge payloads though. - if self.headers.get("Content-Type", "").startswith( + content_type = self.headers.get("Content-Type", "") + expect_xml = content_type.startswith( "text/xml" - ) or self.headers.get("Content-Type", "").startswith("application/xml"): - try: - content_length = int(self.headers["Content-Length"]) - except: - content_length = -1 - if content_length == 0 or not self._raw: - self._raw = "" - self.tree = None - log.debug("No content delivered") - else: - ## With response.raw we could be streaming the content, but it does not work because - ## the stream often is compressed. We could add uncompression on the fly, but not - ## considered worth the effort as for now. - # self.tree = etree.parse(response.raw, parser=etree.XMLParser(remove_blank_text=True)) - try: - self.tree = etree.XML( - self._raw, - parser=etree.XMLParser( - remove_blank_text=True, huge_tree=self.huge_tree - ), - ) - except: - logging.critical( - "Expected some valid XML from the server, but got this: \n" - + str(self._raw), - exc_info=True, - ) - raise - if log.level <= logging.DEBUG: - log.debug(etree.tostring(self.tree, pretty_print=True)) - elif self.headers.get("Content-Type", "").startswith( + ) or content_type.startswith("application/xml") + ## text/plain is typically for errors, we shouldn't see it on 200/207 responses. + ## TODO: may want to log an error if it's text/plain and 200/207. + ## Logic here was moved when refactoring + expect_error = content_type.startswith( "text/calendar" - ) or self.headers.get("Content-Type", "").startswith("text/plain"): - ## text/plain is typically for errors, we shouldn't see it on 200/207 responses. - ## TODO: may want to log an error if it's text/plain and 200/207. - ## Logic here was moved when refactoring - pass + ) or content_type.startswith("text/plain") + try: + content_length = int(self.headers["Content-Length"]) + except: + content_length = -1 + if content_length == 0 or not self._raw: + self._raw = "" + self.tree = None + log.debug("No content delivered") else: - ## Probably no content type given (iCloud). Some servers - ## give text/html as the default when no content is - ## delivered or on errors (ref - ## https://github.com/python-caldav/caldav/issues/142). - ## TODO: maybe just remove all of the code above in this if/else and let all - ## data be parsed through this code. + ## With response.raw we could be streaming the content, but it does not work because + ## the stream often is compressed. We could add uncompression on the fly, but not + ## considered worth the effort as for now. + # self.tree = etree.parse(response.raw, parser=etree.XMLParser(remove_blank_text=True)) try: + ## Sometimes no content type given (iCloud). Some servers + ## give text/html as the default when no content is + ## delivered or on errors (ref + ## https://github.com/python-caldav/caldav/issues/142). + ## DONE: let all data be parsed through this code. self.tree = etree.XML( self._raw, parser=etree.XMLParser( @@ -130,7 +112,17 @@ def __init__( ), ) except: - pass + if not expect_error: + logging.critical( + "Expected some valid XML from the server, but got this: \n" + + str(self._raw), + exc_info=True, + ) + if expect_xml: + raise + else: + if log.level <= logging.DEBUG: + log.debug(etree.tostring(self.tree, pretty_print=True)) ## this if will always be true as for now, see other comments on streaming. if hasattr(self, "_raw"): From a1dd9af134fc35c2c44f378dd041b19e417f47b1 Mon Sep 17 00:00:00 2001 From: Nicco Kunzmann Date: Thu, 6 Mar 2025 11:24:19 +0100 Subject: [PATCH 2/3] fix styling issue --- caldav/davclient.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/caldav/davclient.py b/caldav/davclient.py index 0d2b99f8..01c77c5b 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -77,9 +77,9 @@ def __init__( ## consider to do streaming into the xmltree library as originally ## intended. It only makes sense for really huge payloads though. content_type = self.headers.get("Content-Type", "") - expect_xml = content_type.startswith( - "text/xml" - ) or content_type.startswith("application/xml") + expect_xml = content_type.startswith("text/xml") or content_type.startswith( + "application/xml" + ) ## text/plain is typically for errors, we shouldn't see it on 200/207 responses. ## TODO: may want to log an error if it's text/plain and 200/207. ## Logic here was moved when refactoring From 4335f8c3c1c22574791ec113c4399f7e921433ae Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sun, 11 May 2025 00:39:06 +0200 Subject: [PATCH 3/3] ref pull request comments --- caldav/davclient.py | 48 ++++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/caldav/davclient.py b/caldav/davclient.py index 01c77c5b..7201ab98 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -73,19 +73,11 @@ def __init__( if davclient: self.huge_tree = davclient.huge_tree - ## TODO: this if/else/elif could possibly be refactored, or we should - ## consider to do streaming into the xmltree library as originally - ## intended. It only makes sense for really huge payloads though. content_type = self.headers.get("Content-Type", "") - expect_xml = content_type.startswith("text/xml") or content_type.startswith( - "application/xml" - ) - ## text/plain is typically for errors, we shouldn't see it on 200/207 responses. - ## TODO: may want to log an error if it's text/plain and 200/207. - ## Logic here was moved when refactoring - expect_error = content_type.startswith( - "text/calendar" - ) or content_type.startswith("text/plain") + xml = ["text/xml", "application/xml"] + no_xml = ["text/plain", "text/calendar"] + expect_xml = any((content_type.startswith(x) for x in xml)) + expect_no_xml = any((content_type.startswith(x) for x in no_xml)) try: content_length = int(self.headers["Content-Length"]) except: @@ -95,16 +87,15 @@ def __init__( self.tree = None log.debug("No content delivered") else: - ## With response.raw we could be streaming the content, but it does not work because - ## the stream often is compressed. We could add uncompression on the fly, but not - ## considered worth the effort as for now. + ## For really huge objects we should pass the object as a stream to the + ## XML parser, like this: # self.tree = etree.parse(response.raw, parser=etree.XMLParser(remove_blank_text=True)) + ## However, we would also need to decompress on the fly. I won't bother now. try: - ## Sometimes no content type given (iCloud). Some servers - ## give text/html as the default when no content is - ## delivered or on errors (ref - ## https://github.com/python-caldav/caldav/issues/142). - ## DONE: let all data be parsed through this code. + ## https://github.com/python-caldav/caldav/issues/142 + ## We cannot trust the content=type (iCloud, OX and others). + ## We'll try to parse the content as XML no matter + ## the content type given. self.tree = etree.XML( self._raw, parser=etree.XMLParser( @@ -112,8 +103,21 @@ def __init__( ), ) except: - if not expect_error: - logging.critical( + ## Content wasn't XML. What does the content-type say? + ## expect_no_xml means text/plain or text/calendar + ## expect_no_xml -> ok, pass on, with debug logging + ## expect_xml means text/xml or application/xml + ## expect_xml -> raise an error + ## anything else (text/plain, text/html, ''), + ## log an error and continue + if not expect_no_xml or log.level <= logging.DEBUG: + if not expect_no_xml: + _log = logging.critical + else: + _log = logging.debug + ## The statement below may not be true. + ## We may be expecting something else + _log( "Expected some valid XML from the server, but got this: \n" + str(self._raw), exc_info=True,