Better Exception-Handling for BasePersistence.replace/insert_bot (#2564)

* Catch exceptions raised while copying __dict__/__slots__ in BasePersistence.replace/insert_bot()

Also updated the docstrings to reflect the changes in behavior with unexpected errors

* Tests: added to CustomClass immutable object that would trigger a setattr() exception

* Tests: added new uuid_ property to own CustomClass methods

* Updated AUTHORS.rst

* Revert "Tests: added new uuid_ property to own CustomClass methods"

This reverts commit 9e67463cf7.

* Revert "Tests: added to CustomClass immutable object that would trigger a setattr() exception"

This reverts commit 1c258304

* Removed unneeded Exception cast to string

f-string will perform the string-ification on their own

* Removed another unneeded Exception cast to string

* Added test to parse unparsable objects in __dict__ or __slots__

* Applied black and pylint style suggestions

All lint tests passed

* Fix typo

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

AUTHORS.rst

@@ -106,6 +106,7 @@ The following wonderful people contributed directly or indirectly to this projec
 - `Vorobjev Simon <https://github.com/simonvorobjev>`_
 - `Wagner Macedo <https://github.com/wagnerluis1982>`_
 - `wjt <https://github.com/wjt>`_
+- `zeroone2numeral2 <https://github.com/zeroone2numeral2>`_
 - `zeshuaro <https://github.com/zeshuaro>`_
 
 Please add yourself here alphabetically when you submit your first pull request.

telegram/ext/basepersistence.py

@@ -212,7 +212,8 @@ class BasePersistence(Generic[UD, CD, BD], ABC):
         :attr:`REPLACED_BOT`. Currently, this handles objects of type ``list``, ``tuple``, ``set``,
         ``frozenset``, ``dict``, ``defaultdict`` and objects that have a ``__dict__`` or
         ``__slots__`` attribute, excluding classes and objects that can't be copied with
-        ``copy.copy``.
+        ``copy.copy``. If the parsing of an object fails, the object will be returned unchanged and
+         the error will be logged.
 
         Args:
             obj (:obj:`object`): The object
@@ -278,20 +279,31 @@ class BasePersistence(Generic[UD, CD, BD], ABC):
             return new_obj
         # if '__dict__' in obj.__slots__, we already cover this here, that's why the
         # __dict__ case comes below
-        if hasattr(obj, '__slots__'):
-            for attr_name in new_obj.__slots__:
-                setattr(
-                    new_obj,
-                    attr_name,
-                    cls._replace_bot(cls._replace_bot(getattr(new_obj, attr_name), memo), memo),
-                )
-            memo[obj_id] = new_obj
-            return new_obj
-        if hasattr(obj, '__dict__'):
-            for attr_name, attr in new_obj.__dict__.items():
-                setattr(new_obj, attr_name, cls._replace_bot(attr, memo))
-            memo[obj_id] = new_obj
-            return new_obj
+        try:
+            if hasattr(obj, '__slots__'):
+                for attr_name in new_obj.__slots__:
+                    setattr(
+                        new_obj,
+                        attr_name,
+                        cls._replace_bot(
+                            cls._replace_bot(getattr(new_obj, attr_name), memo), memo
+                        ),
+                    )
+                memo[obj_id] = new_obj
+                return new_obj
+            if hasattr(obj, '__dict__'):
+                for attr_name, attr in new_obj.__dict__.items():
+                    setattr(new_obj, attr_name, cls._replace_bot(attr, memo))
+                memo[obj_id] = new_obj
+                return new_obj
+        except Exception as exception:
+            warnings.warn(
+                f'Parsing of an object failed with the following exception: {exception}. '
+                f'See the docs of BasePersistence.replace_bot for more information.',
+                RuntimeWarning,
+            )
+            memo[obj_id] = obj
+            return obj
 
         return obj
 
@@ -301,7 +313,8 @@ class BasePersistence(Generic[UD, CD, BD], ABC):
         :attr:`bot`. Currently, this handles objects of type ``list``, ``tuple``, ``set``,
         ``frozenset``, ``dict``, ``defaultdict`` and objects that have a ``__dict__`` or
         ``__slots__`` attribute, excluding classes and objects that can't be copied with
-        ``copy.copy``.
+        ``copy.copy``. If the parsing of an object fails, the object will be returned unchanged and
+        the error will be logged.
 
         Args:
             obj (:obj:`object`): The object
@@ -368,20 +381,31 @@ class BasePersistence(Generic[UD, CD, BD], ABC):
             return new_obj
         # if '__dict__' in obj.__slots__, we already cover this here, that's why the
         # __dict__ case comes below
-        if hasattr(obj, '__slots__'):
-            for attr_name in obj.__slots__:
-                setattr(
-                    new_obj,
-                    attr_name,
-                    self._insert_bot(self._insert_bot(getattr(new_obj, attr_name), memo), memo),
-                )
-            memo[obj_id] = new_obj
-            return new_obj
-        if hasattr(obj, '__dict__'):
-            for attr_name, attr in new_obj.__dict__.items():
-                setattr(new_obj, attr_name, self._insert_bot(attr, memo))
-            memo[obj_id] = new_obj
-            return new_obj
+        try:
+            if hasattr(obj, '__slots__'):
+                for attr_name in obj.__slots__:
+                    setattr(
+                        new_obj,
+                        attr_name,
+                        self._insert_bot(
+                            self._insert_bot(getattr(new_obj, attr_name), memo), memo
+                        ),
+                    )
+                memo[obj_id] = new_obj
+                return new_obj
+            if hasattr(obj, '__dict__'):
+                for attr_name, attr in new_obj.__dict__.items():
+                    setattr(new_obj, attr_name, self._insert_bot(attr, memo))
+                memo[obj_id] = new_obj
+                return new_obj
+        except Exception as exception:
+            warnings.warn(
+                f'Parsing of an object failed with the following exception: {exception}. '
+                f'See the docs of BasePersistence.insert_bot for more information.',
+                RuntimeWarning,
+            )
+            memo[obj_id] = obj
+            return obj
 
         return obj
 

tests/test_persistence.py

@@ -18,6 +18,7 @@
 # along with this program.  If not, see [http://www.gnu.org/licenses/].
 import gzip
 import signal
+import uuid
 from threading import Lock
 
 from telegram.ext.callbackdatacache import CallbackDataCache
@@ -689,6 +690,36 @@ class TestBasePersistence:
             "BasePersistence.insert_bot does not handle objects that can not be copied."
         )
 
+    def test_bot_replace_insert_bot_unparsable_objects(self, bot, bot_persistence, recwarn):
+        """Here check that objects in __dict__ or __slots__ that can't
+        be parsed are just returned verbatim."""
+        persistence = bot_persistence
+        persistence.set_bot(bot)
+
+        uuid_obj = uuid.uuid4()
+
+        persistence.update_bot_data({1: uuid_obj})
+        assert persistence.bot_data[1] is uuid_obj
+        persistence.update_chat_data(123, {1: uuid_obj})
+        assert persistence.chat_data[123][1] is uuid_obj
+        persistence.update_user_data(123, {1: uuid_obj})
+        assert persistence.user_data[123][1] is uuid_obj
+        persistence.update_callback_data(([('1', 2, {0: uuid_obj})], {'1': '2'}))
+        assert persistence.callback_data[0][0][2][0] is uuid_obj
+
+        assert persistence.get_bot_data()[1] is uuid_obj
+        assert persistence.get_chat_data()[123][1] is uuid_obj
+        assert persistence.get_user_data()[123][1] is uuid_obj
+        assert persistence.get_callback_data()[0][0][2][0] is uuid_obj
+
+        assert len(recwarn) == 2
+        assert str(recwarn[0].message).startswith(
+            "Parsing of an object failed with the following exception: "
+        )
+        assert str(recwarn[1].message).startswith(
+            "Parsing of an object failed with the following exception: "
+        )
+
     def test_bot_replace_insert_bot_classes(self, bot, bot_persistence, recwarn):
         """Here check that classes are just returned verbatim."""
         persistence = bot_persistence