Repository: python-telegram-bot/python-telegram-bot
Pull request: Ignore classes on replace/insert_bot
Closes #2513

### Checklist for PRs

- [x] `.. versionadded:: version` not necessary, this is more a bug fix than a change …
- [x] Created new or adapted existing unit tests


Problem statement:
[BUG] Bot.insert/replace_bot with types
Yes, here we go again 😬 
If your data contains a class, you get `AttributeError: attribute '__dict__' of 'type' objects is not writable`.
I guess we can just skip types in the insert/replace logic. If a class holds a reference to a Bot it must be a class variable and that should be reconstructed at runtime. But this logic mostly comes from my understanding of how pickle persists classes, so this needs some double checking, if there are implications for other cases.

Hints from issue discussion:
Thinking about it some more, when you want to serialize a class you either store a reference to where that class is defined or you store _how_ it is defined. The latter is probably relevant only for dynamically generated classes, which is an edge use case already, IG. In the former case, we don't have to worry about not handling stuff, as explained above.
So I guess "ignore & warn" should be enough here …

Commit message: Ignore classes on replace/insert_bot

Patch to review:
diff --git a/telegram/ext/basepersistence.py b/telegram/ext/basepersistence.py
index c7968f4bdff..a3af871a79b 100644
--- a/telegram/ext/basepersistence.py
+++ b/telegram/ext/basepersistence.py
@@ -137,7 +137,8 @@ def replace_bot(cls, obj: object) -> object:
         Replaces all instances of :class:`telegram.Bot` that occur within the passed object with
         :attr:`REPLACED_BOT`. Currently, this handles objects of type ``list``, ``tuple``, ``set``,
         ``frozenset``, ``dict``, ``defaultdict`` and objects that have a ``__dict__`` or
-        ``__slot__`` attribute, excluding objects that can't be copied with `copy.copy`.
+        ``__slot__`` attribute, excluding objects that can't be copied with `copy.copy` and
+        classes.
 
         Args:
             obj (:obj:`object`): The object
@@ -168,6 +169,14 @@ def _replace_bot(cls, obj: object, memo: Dict[int, object]) -> object:  # pylint
             new_immutable = obj.__class__(cls._replace_bot(item, memo) for item in obj)
             memo[obj_id] = new_immutable
             return new_immutable
+        if isinstance(obj, type):
+            # classes usually do have a __dict__, but it's not writable
+            warnings.warn(
+                'BasePersistence.replace_bot does not handle classes. See '
+                'the docs of BasePersistence.replace_bot for more information.',
+                RuntimeWarning,
+            )
+            return obj
 
         try:
             new_obj = copy(obj)
@@ -215,7 +224,8 @@ def insert_bot(self, obj: object) -> object:
         Replaces all instances of :attr:`REPLACED_BOT` that occur within the passed object with
         :attr:`bot`. Currently, this handles objects of type ``list``, ``tuple``, ``set``,
         ``frozenset``, ``dict``, ``defaultdict`` and objects that have a ``__dict__`` or
-        ``__slot__`` attribute, excluding objects that can't be copied with `copy.copy`.
+        ``__slot__`` attribute, excluding objects that can't be copied with `copy.copy` and
+        classes.
 
         Args:
             obj (:obj:`object`): The object
@@ -248,6 +258,14 @@ def _insert_bot(self, obj: object, memo: Dict[int, object]) -> object:  # pylint
             new_immutable = obj.__class__(self._insert_bot(item, memo) for item in obj)
             memo[obj_id] = new_immutable
             return new_immutable
+        if isinstance(obj, type):
+            # classes usually do have a __dict__, but it's not writable
+            warnings.warn(
+                'BasePersistence.insert_bot does not handle classes. See '
+                'the docs of BasePersistence.replace_bot for more information.',
+                RuntimeWarning,
+            )
+            return obj
 
         try:
             new_obj = copy(obj)
diff --git a/tests/test_persistence.py b/tests/test_persistence.py
index 6b00046dfe5..47422508282 100644
--- a/tests/test_persistence.py
+++ b/tests/test_persistence.py
@@ -590,6 +590,33 @@ def __copy__(self):
             "BasePersistence.insert_bot does not handle objects that can not be copied."
         )
 
+    def test_bot_replace_insert_bot_classes(self, bot, bot_persistence, recwarn):
+        """Here check that classes are just returned verbatim."""
+        persistence = bot_persistence
+        persistence.set_bot(bot)
+
+        class CustomClass:
+            pass
+
+        persistence.update_bot_data({1: CustomClass})
+        assert persistence.bot_data[1] is CustomClass
+        persistence.update_chat_data(123, {1: CustomClass})
+        assert persistence.chat_data[123][1] is CustomClass
+        persistence.update_user_data(123, {1: CustomClass})
+        assert persistence.user_data[123][1] is CustomClass
+
+        assert persistence.get_bot_data()[1] is CustomClass
+        assert persistence.get_chat_data()[123][1] is CustomClass
+        assert persistence.get_user_data()[123][1] is CustomClass
+
+        assert len(recwarn) == 2
+        assert str(recwarn[0].message).startswith(
+            "BasePersistence.replace_bot does not handle classes."
+        )
+        assert str(recwarn[1].message).startswith(
+            "BasePersistence.insert_bot does not handle classes."
+        )
+
     def test_bot_replace_insert_bot_objects_with_faulty_equality(self, bot, bot_persistence):
         """Here check that trying to compare obj == self.REPLACED_BOT doesn't lead to problems."""
         persistence = bot_persistence


Code review comments:
diff_hunk: @@ -137,7 +137,8 @@ def replace_bot(cls, obj: object) -> object:
         Replaces all instances of :class:`telegram.Bot` that occur within the passed object with
         :attr:`REPLACED_BOT`. Currently, this handles objects of type ``list``, ``tuple``, ``set``,
         ``frozenset``, ``dict``, ``defaultdict`` and objects that have a ``__dict__`` or
-        ``__slot__`` attribute, excluding objects that can't be copied with `copy.copy`.
+        ``__slot__`` attribute, excluding objects that can't be copied with `copy.copy` and
+        classes.
path: telegram/ext/basepersistence.py
text: @user1:
I would put classes first, i.e. `excluding classes and objects that can't ...`