Repository: python-telegram-bot/python-telegram-bot
Pull request: Fix unpickling of classes with new/removed attributes
Closes #3379 

- Attributes which are removed will automatically go to the object's `api_kwargs`.
- Attributes which are new to the class won't be available in the unpickled version

### Checklist for PRs

- [ ] ~Added `.. versionadded:: version`, `.. versionchanged:: version` or `.. deprecated:: version` to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)~
- [x] Created new or adapted existing unit tests
- [x] Documented code changes according to the [CSI standard](https://standards.mousepawmedia.com/en/stable/csi.html)
- [ ] ~Added myself alphabetically to `AUTHORS.rst` (optional)~
- [ ] ~Added new classes & modules to the docs and all suitable `__all__` s~

Problem statement:
[BUG] PicklePersistence: AttributeError: 'Chat' object has no attribute 'all_members_are_administrators'
### Steps to Reproduce

1. Run a bot with some picklepersistence files from previous version (v20a4)



### Expected behaviour

1. Run a bot with some picklepersistence files from previous version (v20a4)
2. Bot will start.

### Actual behaviour

1. Run a bot with some picklepersistence files from previous version (v20a4)
2. Bot won't start.

### Operating System

Debian GNU/Linux 11 (bullseye) aarch64 

### Version of Python, python-telegram-bot & dependencies

```shell
python-telegram-bot 20.0a5
Bot API 6.3
Python 3.10.2 (main, Feb 12 2022, 01:26:02) [GCC 10.2.1 20210110]
```


### Relevant log output

```python
Traceback (most recent call last):
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_picklepersistence.py", line 246, in _load_singlefile
    data = _BotUnpickler(self.bot, file).load()
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_picklepersistence.py", line 56, in _reconstruct_to
    obj.__setstate__(kwargs)
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/_telegramobject.py", line 200, in __setstate__
    setattr(self, key, val)
AttributeError: 'Chat' object has no attribute 'all_members_are_administrators'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/pi/Desktop/emily/main.py", line 711, in <module>
    main()
  File "/home/pi/Desktop/emily/main.py", line 341, in main
    app.run_polling(drop_pending_updates=True, allowed_updates=Update.ALL_TYPES)
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_application.py", line 667, in run_polling
    return self.__run(
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_application.py", line 855, in __run
    raise exc
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_application.py", line 844, in __run
    loop.run_until_complete(self.initialize())
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/asyncio/base_events.py", line 641, in run_until_complete
    return future.result()
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_application.py", line 365, in initialize
    await self._initialize_persistence()
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_application.py", line 445, in _initialize_persistence
    self._user_data.update(await self.persistence.get_user_data())
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_picklepersistence.py", line 307, in get_user_data
    self._load_singlefile()
  File "/home/pi/.pyenv/versions/3.10.2/lib/python3.10/site-packages/telegram/ext/_picklepersistence.py", line 264, in _load_singlefile
    raise TypeError(f"Something went wrong unpickling {self.filepath.name}") from exc
TypeError: Something went wrong unpickling picklepersistence
```


### Additional Context

_No response_

Hints from issue discussion:

Commit message: use protocol version 4 pickle file

Patch to review:
diff --git a/telegram/_telegramobject.py b/telegram/_telegramobject.py
index a9a31cd3c86..a45812ee1fe 100644
--- a/telegram/_telegramobject.py
+++ b/telegram/_telegramobject.py
@@ -197,7 +197,14 @@ def __setstate__(self, state: dict) -> None:
         setattr(self, "_bot", None)
 
         for key, val in state.items():
-            setattr(self, key, val)
+            try:
+                setattr(self, key, val)
+            except AttributeError:  # catch cases when old attributes are removed from new versions
+                setattr(self, "api_kwargs", state.get("api_kwargs", {}))  # assign api_kwargs first
+                if self.api_kwargs is None:  # should never happen, but just in case
+                    setattr(self, "api_kwargs", {})
+                self.api_kwargs[key] = val  # add it to api_kwargs as fallback
+
         self._apply_api_kwargs()
 
     def __deepcopy__(self: Tele_co, memodict: dict) -> Tele_co:
diff --git a/tests/conftest.py b/tests/conftest.py
index 9d56aab7619..e05442dad98 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -248,7 +248,7 @@ async def updater(bot_info):
 TEST_DATA_PATH = Path(__file__).parent.resolve() / "data"
 
 
-def data_file(filename: str):
+def data_file(filename: str) -> Path:
     return TEST_DATA_PATH / filename
 
 
diff --git a/tests/data/old_chat.pickle b/tests/data/old_chat.pickle
new file mode 100644
index 00000000000..3756d1e7e5c
Binary files /dev/null and b/tests/data/old_chat.pickle differ
diff --git a/tests/test_telegramobject.py b/tests/test_telegramobject.py
index 5eb24820055..feb29878666 100644
--- a/tests/test_telegramobject.py
+++ b/tests/test_telegramobject.py
@@ -25,6 +25,8 @@
 import pytest
 
 from telegram import Bot, BotCommand, Chat, Message, PhotoSize, TelegramObject, User
+from telegram.ext import PicklePersistence
+from tests.conftest import data_file
 
 
 def all_subclasses(cls):
@@ -243,7 +245,7 @@ def test_pickle(self, bot):
         assert unpickled.date == date, f"{unpickled.date} != {date}"
         assert unpickled.photo[0] == photo
 
-    def test_pickle_apply_api_kwargs(self, bot):
+    def test_pickle_apply_api_kwargs(self):
         """Makes sure that when a class gets new attributes, the api_kwargs are moved to the
         new attributes on unpickling."""
         obj = self.ChangingTO(api_kwargs={"foo": "bar"})
@@ -255,6 +257,17 @@ def test_pickle_apply_api_kwargs(self, bot):
         assert obj.foo == "bar"
         assert obj.api_kwargs == {}
 
+    def test_pickle_removed_attribute(self):
+        """Test when newer versions of the library remove attributes from classes (which the old
+        pickled versions still have).
+        """
+        # load old Chat object, which doesn't have api_kwargs attribute, and has now removed attr
+        # `all_members_are_administrators`.
+        pp = PicklePersistence(data_file("old_chat.pickle"))
+        chat = pp._load_file(data_file("old_chat.pickle"))
+        assert chat.api_kwargs == {"all_members_are_administrators": None}
+        assert getattr(chat, "all_members_are_administrators", False) is False
+
     def test_deepcopy_telegram_obj(self, bot):
         chat = Chat(2, Chat.PRIVATE)
         user = User(3, "first_name", False)


Code review comments:
diff_hunk: @@ -255,6 +257,17 @@ def test_pickle_apply_api_kwargs(self, bot):
         assert obj.foo == "bar"
         assert obj.api_kwargs == {}
 
+    def test_pickle_removed_attribute(self):
+        """Test when newer versions of the library remove attributes from classes (which the old
+        pickled versions still have).
+        """
+        # load old Chat object, which doesn't have api_kwargs attribute, and has now removed attr
+        # `all_members_are_administrators`.
+        pp = PicklePersistence(data_file("old_chat.pickle"))
+        chat = pp._load_file(data_file("old_chat.pickle"))
path: tests/test_telegramobject.py
text: @user1:
I would prefer it if we didn't have to rely on protected methods for the tests … maybe you can create the pickle file via `PicklePersistence` itself? It would also be nice to have a few CSI-style comments about how the file was created such that one can reproduce it if necessary :)

diff_hunk: @@ -197,7 +197,14 @@ def __setstate__(self, state: dict) -> None:
         setattr(self, "_bot", None)
 
         for key, val in state.items():
-            setattr(self, key, val)
+            try:
+                setattr(self, key, val)
+            except AttributeError:  # catch cases when old attributes are removed from new versions
+                setattr(self, "api_kwargs", state.get("api_kwargs", {}))  # assign api_kwargs first
+                if self.api_kwargs is None:  # should never happen, but just in case
+                    setattr(self, "api_kwargs", {})
+                self.api_kwargs[key] = val  # add it to api_kwargs as fallback
+
path: telegram/_telegramobject.py
text: @user1:
If the removed attribute is handled before `api_kwargs` is, the `setattr` in line 201 will override the `api_kwargs` again. Could you

1) set the api_kwargs manually before the for-loop & pop it from the `state`?
2) instead of a v20.0a4-Chat for testing use a modified version of the v20.0a5-Chat that has both `all_members_are_admins` *and* non-empty api_kwargs?

lines 204-205 can be removed then IMO