From 0e7e92a91be76c365c475043cdffaf6672d7c01b Mon Sep 17 00:00:00 2001
From: Daniil Fajnberg <mail@daniil.fajnberg.de>
Date: Sat, 26 Mar 2022 10:29:34 +0100
Subject: [PATCH] better error handling when converting arguments

---
 src/asyncio_taskpool/control/parser.py | 21 +++++++++++++++++++--
 tests/test_control/test_parser.py      | 26 +++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/src/asyncio_taskpool/control/parser.py b/src/asyncio_taskpool/control/parser.py
index ccf33de..8190e45 100644
--- a/src/asyncio_taskpool/control/parser.py
+++ b/src/asyncio_taskpool/control/parser.py
@@ -20,7 +20,8 @@ Definition of the :class:`ControlParser` used in a
 """
 
 
-from argparse import Action, ArgumentParser, ArgumentDefaultsHelpFormatter, HelpFormatter, SUPPRESS
+import logging
+from argparse import Action, ArgumentParser, ArgumentDefaultsHelpFormatter, HelpFormatter, ArgumentTypeError, SUPPRESS
 from ast import literal_eval
 from asyncio.streams import StreamWriter
 from inspect import Parameter, getmembers, isfunction, signature
@@ -36,6 +37,9 @@ from ..internals.types import ArgsT, CancelCB, CoroutineFunc, EndCB, KwArgsT
 __all__ = ['ControlParser']
 
 
+log = logging.getLogger(__name__)
+
+
 FmtCls = TypeVar('FmtCls', bound=Type[HelpFormatter])
 ParsersDict = Dict[str, 'ControlParser']
 
@@ -300,8 +304,21 @@ def _get_arg_type_wrapper(cls: Type) -> Callable[[Any], Any]:
     Returns a wrapper for the constructor of `cls` to avoid a ValueError being raised on suppressed arguments.
 
     See: https://bugs.python.org/issue36078
+
+    In addition, the type conversion wrapper catches exceptions not handled properly by the parser, logs them, and
+    turns them into `ArgumentTypeError` exceptions the parser can propagate to the client.
     """
-    def wrapper(arg: Any) -> Any: return arg if arg is SUPPRESS else cls(arg)
+    def wrapper(arg: Any) -> Any:
+        if arg is SUPPRESS:
+            return arg
+        try:
+            return cls(arg)
+        except (ArgumentTypeError, TypeError, ValueError):
+            raise  # handled properly by the parser and propagated to the client anyway
+        except Exception as e:
+            text = f"{e.__class__.__name__} occurred in parser trying to convert type: {cls.__name__}({repr(arg)})"
+            log.exception(text)
+            raise ArgumentTypeError(text)  # propagate to the client
     # Copy the name of the class to maintain useful help messages when incorrect arguments are passed.
     wrapper.__name__ = cls.__name__
     return wrapper
diff --git a/tests/test_control/test_parser.py b/tests/test_control/test_parser.py
index 0e12d81..b18f6dd 100644
--- a/tests/test_control/test_parser.py
+++ b/tests/test_control/test_parser.py
@@ -35,7 +35,7 @@ from asyncio_taskpool.internals.types import ArgsT, CancelCB, CoroutineFunc, End
 FOO, BAR = 'foo', 'bar'
 
 
-class ControlServerTestCase(TestCase):
+class ControlParserTestCase(TestCase):
 
     def setUp(self) -> None:
         self.help_formatter_factory_patcher = patch.object(parser.ControlParser, 'help_formatter_factory')
@@ -265,12 +265,36 @@ class ControlServerTestCase(TestCase):
 
 
 class RestTestCase(TestCase):
+    log_lvl: int
+
+    @classmethod
+    def setUpClass(cls) -> None:
+        cls.log_lvl = parser.log.level
+        parser.log.setLevel(999)
+
+    @classmethod
+    def tearDownClass(cls) -> None:
+        parser.log.setLevel(cls.log_lvl)
+
     def test__get_arg_type_wrapper(self):
         type_wrap = parser._get_arg_type_wrapper(int)
         self.assertEqual('int', type_wrap.__name__)
         self.assertEqual(SUPPRESS, type_wrap(SUPPRESS))
         self.assertEqual(13, type_wrap('13'))
 
+        name = 'abcdef'
+        mock_type = MagicMock(side_effect=[parser.ArgumentTypeError, TypeError, ValueError, Exception], __name__=name)
+        type_wrap = parser._get_arg_type_wrapper(mock_type)
+        self.assertEqual(name, type_wrap.__name__)
+        with self.assertRaises(parser.ArgumentTypeError):
+            type_wrap(FOO)
+        with self.assertRaises(TypeError):
+            type_wrap(FOO)
+        with self.assertRaises(ValueError):
+            type_wrap(FOO)
+        with self.assertRaises(parser.ArgumentTypeError):
+            type_wrap(FOO)
+
     @patch.object(parser, '_get_arg_type_wrapper')
     def test__get_type_from_annotation(self, mock__get_arg_type_wrapper: MagicMock):
         mock__get_arg_type_wrapper.return_value = expected_output = FOO + BAR