From 4183d860ae151188eac421ed8e9492bea473ac17 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 07:32:46 +0100 Subject: [PATCH 01/23] Exit synadm on fatal requests exceptions --- synadm/api.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/synadm/api.py b/synadm/api.py index 9df0663..203322c 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -30,12 +30,23 @@ import requests from http.client import HTTPConnection +from requests.exceptions import InvalidURL, MissingSchema import datetime import json import urllib.parse import re +def log_fatal_exit(error, logger): + """Log a fatal error and exit synadm.""" + logger.fatal( + "%s: %s.\nsynadm exited due to a fatal error.", + type(error).__name__, + error, + ) + raise SystemExit(1) from error + + class ApiRequest: """Basic API request handling and helper utilities @@ -139,6 +150,12 @@ def query(self, method, urlpart, params=None, data=None, token=None, self.log.warning(f"{host_descr} returned status code " f"{resp.status_code}") return resp.json() + except ConnectionError as error: + log_fatal_exit(error, self.log) + except InvalidURL as error: + log_fatal_exit(error, self.log) + except MissingSchema as error: + log_fatal_exit(error, self.log) except Exception as error: self.log.error("%s while querying %s: %s", type(error).__name__, host_descr, error) From ce1e9770bb34b27f0bd2a6691110e76a2539bb93 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 07:37:26 +0100 Subject: [PATCH 02/23] Sort api module imports with isort --- synadm/api.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index 203322c..fef0db8 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -28,13 +28,14 @@ https://matrix.org/docs/spec/#matrix-apis. """ -import requests -from http.client import HTTPConnection -from requests.exceptions import InvalidURL, MissingSchema import datetime import json -import urllib.parse import re +import urllib.parse +from http.client import HTTPConnection + +import requests +from requests.exceptions import InvalidURL, MissingSchema def log_fatal_exit(error, logger): From 18b4991b89edef7aaf6a3aa98e422ac40cceba9c Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 08:29:03 +0100 Subject: [PATCH 03/23] Refactor query args handling more pythonic --- synadm/api.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index fef0db8..c4c2d19 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -117,10 +117,8 @@ def query(self, method, urlpart, params=None, data=None, token=None, JSON strings. On exceptions the error type and description are logged and None is returned. """ - args = list(args) + args = [urllib.parse.quote(arg, safe="") for arg in args] kwargs = dict(kwargs) - for i in range(len(args)): - args[i] = urllib.parse.quote(args[i], safe="") for i in kwargs.keys(): kwargs[i] = urllib.parse.quote(kwargs[i], safe="") urlpart = urlpart.format(*args, **kwargs) From a66f8947aeaae05a70fb532fb8e77420eb881af7 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 08:37:00 +0100 Subject: [PATCH 04/23] Refactor query kwargs handling more pythonic --- synadm/api.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index c4c2d19..f7a74d8 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -118,9 +118,7 @@ def query(self, method, urlpart, params=None, data=None, token=None, logged and None is returned. """ args = [urllib.parse.quote(arg, safe="") for arg in args] - kwargs = dict(kwargs) - for i in kwargs.keys(): - kwargs[i] = urllib.parse.quote(kwargs[i], safe="") + kwargs = {k: urllib.parse.quote(v, safe="") for k, v in kwargs.items()} urlpart = urlpart.format(*args, **kwargs) if base_url_override: From dd2c2ec2319aa20cd536f80173ea0d207a390515 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 08:42:46 +0100 Subject: [PATCH 05/23] Move query args before keyword args to please linter --- synadm/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index f7a74d8..fedfc97 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -85,8 +85,8 @@ def __init__(self, log, user, token, base_url, path, timeout, debug, HTTPConnection.debuglevel = 1 self.verify = verify - def query(self, method, urlpart, params=None, data=None, token=None, - base_url_override=None, verify=None, *args, **kwargs): + def query(self, method, urlpart, *args, params=None, data=None, token=None, + base_url_override=None, verify=None, **kwargs): """Generic wrapper around requests methods. Handles requests methods, logging and exceptions, and URL encoding. From 2a06f91c224c512294f4ebc9d4671c812e0e6eb0 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 08:59:30 +0100 Subject: [PATCH 06/23] Add query function type hints, return only --- synadm/api.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index fedfc97..b369d3c 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -31,6 +31,7 @@ import datetime import json import re +from typing import Optional, Union, Dict, List, Any import urllib.parse from http.client import HTTPConnection @@ -85,8 +86,18 @@ def __init__(self, log, user, token, base_url, path, timeout, debug, HTTPConnection.debuglevel = 1 self.verify = verify - def query(self, method, urlpart, *args, params=None, data=None, token=None, - base_url_override=None, verify=None, **kwargs): + def query( + self, + method, + urlpart, + *args, + params=None, + data=None, + token=None, + base_url_override=None, + verify=None, + **kwargs, + ) -> Optional[Union[Dict[str, Any], List[Dict[str, Any]], None]]: """Generic wrapper around requests methods. Handles requests methods, logging and exceptions, and URL encoding. From 8fc2a70f637417d7b4c8010f345e5e701090ea6c Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 09:09:05 +0100 Subject: [PATCH 07/23] Add query function type hints, arguments only --- synadm/api.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index b369d3c..affd616 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -88,15 +88,15 @@ def __init__(self, log, user, token, base_url, path, timeout, debug, def query( self, - method, - urlpart, - *args, - params=None, - data=None, - token=None, - base_url_override=None, - verify=None, - **kwargs, + method: str, + urlpart: str, + *args: Any, + params: Optional[Dict[str, Any]] = None, + data: Optional[Dict[str, Any]] = None, + token: Optional[str] = None, + base_url_override: Optional[bool] = False, + verify: Optional[bool] = True, + **kwargs: Dict[str, Any], ) -> Optional[Union[Dict[str, Any], List[Dict[str, Any]], None]]: """Generic wrapper around requests methods. From c5093f728b9eaacfa8924ac67077be705236458e Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 24 Jan 2025 09:29:59 +0100 Subject: [PATCH 08/23] Fix query function docstring (arguments only) - Add missing arguments. - Remove (types) since we have type hints now. - Reword here and there. --- synadm/api.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index affd616..b21e067 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -103,19 +103,19 @@ def query( Handles requests methods, logging and exceptions, and URL encoding. Args: - urlpart (string): The path to the API endpoint, excluding - self.base_url and self.path (the part after - proto://fqdn:port/path). It will be passed to Python's - str.format, so the string should not be already formatted - (as f-strings or with str.format) as to sanitize the URL. - params (dict, optional): URL parameters (?param1¶m2). Defaults - to None. - data (dict, optional): Request body used in POST, PUT, DELETE - requests. Defaults to None. - base_url_override (bool): The default setting of self.base_url set + method: The http method to use (get, post, put, ...) + urlpart: The path to the API endpoint, excluding self.base_url and + self.path (the part after proto://fqdn:port/path). It will be + passed to Python's str.format, so the string should not be + already formatted (as f-strings or with str.format) as to + sanitize the URL. + params: URL parameters (?param1¶m2).. + data: Request body used in POST, PUT, DELETE requests. + token: An optional token overriding the configured one. + base_url_override: The default setting of self.base_url set on initialization can be overwritten using this argument. - verify(bool): Mandatory SSL verification is turned on by default - and can be turned off using this method. + verify: Mandatory SSL verification is on by default and can be + turned off using this method. *args: Arguments that will be URL encoded and passed to Python's str.format. **kwargs: Keyword arguments that will be URL encoded (only the From 5524e71f8888182b5fa559a118c5ecec7433bd19 Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 16:32:13 +0100 Subject: [PATCH 09/23] fix ConnectionError not being caught --- synadm/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index b21e067..72278f6 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -36,7 +36,7 @@ from http.client import HTTPConnection import requests -from requests.exceptions import InvalidURL, MissingSchema +from requests.exceptions import InvalidURL, MissingSchema, ConnectionError def log_fatal_exit(error, logger): From 11c1cff787a69b24b11ded53168fbcf772eebd07 Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 16:34:02 +0100 Subject: [PATCH 10/23] add optional messages to log_fatal_exit --- synadm/api.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index 72278f6..6fea2e1 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -39,12 +39,14 @@ from requests.exceptions import InvalidURL, MissingSchema, ConnectionError -def log_fatal_exit(error, logger): +def log_fatal_exit(error, logger, message=None): """Log a fatal error and exit synadm.""" + if message is None: + message = "synadm exited due to a fatal error." logger.fatal( - "%s: %s.\nsynadm exited due to a fatal error.", + "%s: %s.\n%s", type(error).__name__, - error, + error, message ) raise SystemExit(1) from error From 0f9bcd0336161c33435ffe4ed8918f2a2e38f957 Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 16:34:18 +0100 Subject: [PATCH 11/23] more detailed message for connection error --- synadm/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index 6fea2e1..8c350fe 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -165,7 +165,8 @@ def query( except InvalidURL as error: log_fatal_exit(error, self.log) except MissingSchema as error: - log_fatal_exit(error, self.log) + log_fatal_exit(error, self.log, "Connection error. Please check " + "that Synapse can be reached.") except Exception as error: self.log.error("%s while querying %s: %s", type(error).__name__, host_descr, error) From 4f8a9f84fec7ff0e8c7649ea92a4f3d185ac5aab Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 16:34:28 +0100 Subject: [PATCH 12/23] don't catch InvalidURL or MissingSchema exceptions if these happen, it's pretty much a bug in our code, and so having the full stacktrace would help (if the user doesn't provide the command) --- synadm/api.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index 8c350fe..6d149dd 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -36,7 +36,7 @@ from http.client import HTTPConnection import requests -from requests.exceptions import InvalidURL, MissingSchema, ConnectionError +from requests.exceptions import ConnectionError def log_fatal_exit(error, logger, message=None): @@ -161,10 +161,6 @@ def query( f"{resp.status_code}") return resp.json() except ConnectionError as error: - log_fatal_exit(error, self.log) - except InvalidURL as error: - log_fatal_exit(error, self.log) - except MissingSchema as error: log_fatal_exit(error, self.log, "Connection error. Please check " "that Synapse can be reached.") except Exception as error: From e1324c26ef816224562ef0b8cfaf15a922929a04 Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 17:22:13 +0100 Subject: [PATCH 13/23] print traceback upon log_fatal_exit --- synadm/api.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synadm/api.py b/synadm/api.py index 6d149dd..e0776d2 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -34,6 +34,7 @@ from typing import Optional, Union, Dict, List, Any import urllib.parse from http.client import HTTPConnection +import traceback import requests from requests.exceptions import ConnectionError @@ -43,6 +44,10 @@ def log_fatal_exit(error, logger, message=None): """Log a fatal error and exit synadm.""" if message is None: message = "synadm exited due to a fatal error." + + # split by new lines that's already included + logger.info("".join(traceback.format_exception(error))) + logger.fatal( "%s: %s.\n%s", type(error).__name__, From 0f42b30f9b6349913c67c907ca8e21d8bdc9fd00 Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 17:26:39 +0100 Subject: [PATCH 14/23] separate message from error logging make it more clear --- synadm/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index e0776d2..a9e2f22 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -49,7 +49,7 @@ def log_fatal_exit(error, logger, message=None): logger.info("".join(traceback.format_exception(error))) logger.fatal( - "%s: %s.\n%s", + "%s: %s.\n\n%s", type(error).__name__, error, message ) From 67f3b9f752732b6501516776008ab77373dce5c7 Mon Sep 17 00:00:00 2001 From: Jackson Date: Wed, 29 Jan 2025 17:26:56 +0100 Subject: [PATCH 15/23] add args docs --- synadm/api.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index a9e2f22..6ed1c7f 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -41,7 +41,14 @@ def log_fatal_exit(error, logger, message=None): - """Log a fatal error and exit synadm.""" + """Log a fatal error and exit synadm. + + Args: + error: A Python exception. + logger: A Python logger, with info and fatal methods + message: Message to use instead of "synadm exited due to a fatal + error." + """ if message is None: message = "synadm exited due to a fatal error." From 0a6ab354107e89dc648c4b4d7e4bfb67def185e1 Mon Sep 17 00:00:00 2001 From: Jackson Date: Fri, 31 Jan 2025 10:35:58 +0100 Subject: [PATCH 16/23] Apply suggestions from code review Co-authored-by: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> --- synadm/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index 6ed1c7f..5839c65 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -52,7 +52,7 @@ def log_fatal_exit(error, logger, message=None): if message is None: message = "synadm exited due to a fatal error." - # split by new lines that's already included + # format_exception() returns a list of strings. join it into a single string again. logger.info("".join(traceback.format_exception(error))) logger.fatal( From dcade530fb689ee999cfcf679bcbe98aee1392b2 Mon Sep 17 00:00:00 2001 From: Jackson Date: Fri, 31 Jan 2025 10:37:36 +0100 Subject: [PATCH 17/23] Apply another suggestion Co-authored-by: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> --- synadm/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index 5839c65..6527911 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -111,7 +111,7 @@ def query( base_url_override: Optional[bool] = False, verify: Optional[bool] = True, **kwargs: Dict[str, Any], - ) -> Optional[Union[Dict[str, Any], List[Dict[str, Any]], None]]: +Optional[Union[Dict[str, Any], List[Dict[str, Any]]]] """Generic wrapper around requests methods. Handles requests methods, logging and exceptions, and URL encoding. From 7df42c9f90c2db7d49daafc9f7e825f32a8b3b9c Mon Sep 17 00:00:00 2001 From: Jackson Date: Fri, 31 Jan 2025 10:42:30 +0100 Subject: [PATCH 18/23] reword format_exception string join part --- synadm/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index 6527911..c6c5898 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -52,7 +52,8 @@ def log_fatal_exit(error, logger, message=None): if message is None: message = "synadm exited due to a fatal error." - # format_exception() returns a list of strings. join it into a single string again. + # format_exception() returns a list of strings (with new lines). join it + # into a single string again for readability. logger.info("".join(traceback.format_exception(error))) logger.fatal( From 5f76e986205a5531ed598a83685ee24adf7c3997 Mon Sep 17 00:00:00 2001 From: Jackson Date: Fri, 31 Jan 2025 10:42:45 +0100 Subject: [PATCH 19/23] fix typo --- synadm/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index c6c5898..03f05e8 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -112,7 +112,7 @@ def query( base_url_override: Optional[bool] = False, verify: Optional[bool] = True, **kwargs: Dict[str, Any], -Optional[Union[Dict[str, Any], List[Dict[str, Any]]]] + ) -> Optional[Union[Dict[str, Any], List[Dict[str, Any]]]]: """Generic wrapper around requests methods. Handles requests methods, logging and exceptions, and URL encoding. From 252d5996503f90b4f8cac97f847a16c0bfe7408c Mon Sep 17 00:00:00 2001 From: Jackson Date: Fri, 31 Jan 2025 10:59:53 +0100 Subject: [PATCH 20/23] rewrite return section of docs for query api --- synadm/api.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index 03f05e8..70a220b 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -137,11 +137,10 @@ def query( values) and passed to Python's str.format. Returns: - string or None: Usually a JSON string containing - the response of the API; responses that are not 200(OK) (usally - error messages returned by the API) will also be returned as - JSON strings. On exceptions the error type and description are - logged and None is returned. + A dict, list or None. If it's a dict or list, it is a JSON + decoded response. Whether it is a dict or a list depends on what + the server responds with. It returns a None if any exceptions + are encountered. """ args = [urllib.parse.quote(arg, safe="") for arg in args] kwargs = {k: urllib.parse.quote(v, safe="") for k, v in kwargs.items()} From 4bb093d6667fa43583926400d46867591150f14e Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 31 Jan 2025 18:28:55 +0100 Subject: [PATCH 21/23] Explicitely catch requests ConnectionError and don't overwrite built-in ConnectionError with import statement. --- synadm/api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index 70a220b..5557c9a 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -37,7 +37,6 @@ import traceback import requests -from requests.exceptions import ConnectionError def log_fatal_exit(error, logger, message=None): @@ -172,7 +171,7 @@ def query( self.log.warning(f"{host_descr} returned status code " f"{resp.status_code}") return resp.json() - except ConnectionError as error: + except requests.exceptions.ConnectionError as error: log_fatal_exit(error, self.log, "Connection error. Please check " "that Synapse can be reached.") except Exception as error: From 8fa15037d1e1b3a8219aa98b17c54c0bdd2d8383 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 31 Jan 2025 18:34:03 +0100 Subject: [PATCH 22/23] Sort imports again with isort --- synadm/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synadm/api.py b/synadm/api.py index 5557c9a..969f8fa 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -31,10 +31,10 @@ import datetime import json import re -from typing import Optional, Union, Dict, List, Any +import traceback import urllib.parse from http.client import HTTPConnection -import traceback +from typing import Any, Dict, List, Optional, Union import requests From cfa5d15b949622f719fd213d93864dc064946376 Mon Sep 17 00:00:00 2001 From: Jackson Date: Fri, 7 Feb 2025 09:32:43 +0100 Subject: [PATCH 23/23] make format_exception work with python 3.9 --- synadm/api.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synadm/api.py b/synadm/api.py index 969f8fa..20b6c67 100644 --- a/synadm/api.py +++ b/synadm/api.py @@ -53,7 +53,11 @@ def log_fatal_exit(error, logger, message=None): # format_exception() returns a list of strings (with new lines). join it # into a single string again for readability. - logger.info("".join(traceback.format_exception(error))) + # + # error specified multiple times to ensure it works with python 3.9 and + # versions after that which accept things differently + logger.info("".join(traceback.format_exception(error, error, + error.__traceback__))) logger.fatal( "%s: %s.\n\n%s",