From 507d6bc0e3649916b2f416aff3a2e6c35fc03de2 Mon Sep 17 00:00:00 2001 From: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com> Date: Sun, 3 Nov 2024 16:35:16 +0100 Subject: [PATCH] Improve Exception Handling in `File.download_*` (#4542) --- telegram/_files/file.py | 42 +++++++++++++++++++++++++++++++++------ tests/_files/test_file.py | 24 +++++++++------------- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/telegram/_files/file.py b/telegram/_files/file.py index c9b8d22d4..640dfc968 100644 --- a/telegram/_files/file.py +++ b/telegram/_files/file.py @@ -128,9 +128,8 @@ class File(TelegramObject): ) -> Path: """ Download this file. By default, the file is saved in the current working directory with - :attr:`file_path` as file name. If the file has no filename, the file ID will be used as - filename. If :paramref:`custom_path` is supplied as a :obj:`str` or :obj:`pathlib.Path`, - it will be saved to that path. + :attr:`file_path` as file name. If :paramref:`custom_path` is supplied as a :obj:`str` or + :obj:`pathlib.Path`, it will be saved to that path. Note: If :paramref:`custom_path` isn't provided and :attr:`file_path` is the path of a @@ -152,6 +151,11 @@ class File(TelegramObject): * This method was previously called ``download``. It was split into :meth:`download_to_drive` and :meth:`download_to_memory`. + .. versionchanged:: NEXT.VERSION + Raises :exc:`RuntimeError` if :attr:`file_path` is not set. Note that files without + a :attr:`file_path` could never be downloaded, as this attribute is mandatory for that + operation. + Args: custom_path (:class:`pathlib.Path` | :obj:`str` , optional): The path where the file will be saved to. If not specified, will be saved in the current working directory @@ -175,7 +179,13 @@ class File(TelegramObject): Returns: :class:`pathlib.Path`: Returns the Path object the file was downloaded to. + Raises: + RuntimeError: If :attr:`file_path` is not set. + """ + if not self.file_path: + raise RuntimeError("No `file_path` available for this file. Can not download.") + local_file = is_local_file(self.file_path) url = None if local_file else self._get_encoded_url() @@ -198,10 +208,8 @@ class File(TelegramObject): filename = Path(custom_path) elif local_file: return Path(self.file_path) - elif self.file_path: - filename = Path(Path(self.file_path).name) else: - filename = Path.cwd() / self.file_id + filename = Path(Path(self.file_path).name) buf = await self.get_bot().request.retrieve( url, @@ -237,6 +245,11 @@ class File(TelegramObject): .. versionadded:: 20.0 + .. versionchanged:: NEXT.VERSION + Raises :exc:`RuntimeError` if :attr:`file_path` is not set. Note that files without + a :attr:`file_path` could never be downloaded, as this attribute is mandatory for that + operation. + Args: out (:obj:`io.BufferedIOBase`): A file-like object. Must be opened for writing in binary mode. @@ -254,7 +267,13 @@ class File(TelegramObject): pool_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to :paramref:`telegram.request.BaseRequest.post.pool_timeout`. Defaults to :attr:`~telegram.request.BaseRequest.DEFAULT_NONE`. + + Raises: + RuntimeError: If :attr:`file_path` is not set. """ + if not self.file_path: + raise RuntimeError("No `file_path` available for this file. Can not download.") + local_file = is_local_file(self.file_path) url = None if local_file else self._get_encoded_url() path = Path(self.file_path) if local_file else None @@ -283,6 +302,11 @@ class File(TelegramObject): ) -> bytearray: """Download this file and return it as a bytearray. + .. versionchanged:: NEXT.VERSION + Raises :exc:`RuntimeError` if :attr:`file_path` is not set. Note that files without + a :attr:`file_path` could never be downloaded, as this attribute is mandatory for that + operation. + Args: buf (:obj:`bytearray`, optional): Extend the given bytearray with the downloaded data. @@ -312,7 +336,13 @@ class File(TelegramObject): :obj:`bytearray`: The same object as :paramref:`buf` if it was specified. Otherwise a newly allocated :obj:`bytearray`. + Raises: + RuntimeError: If :attr:`file_path` is not set. + """ + if not self.file_path: + raise RuntimeError("No `file_path` available for this file. Can not download.") + if buf is None: buf = bytearray() diff --git a/tests/_files/test_file.py b/tests/_files/test_file.py index 70874d5fe..fbf71dc70 100644 --- a/tests/_files/test_file.py +++ b/tests/_files/test_file.py @@ -17,6 +17,7 @@ # You should have received a copy of the GNU Lesser Public License # along with this program. If not, see [http://www.gnu.org/licenses/]. import os +from io import BytesIO from pathlib import Path from tempfile import TemporaryFile, mkstemp @@ -181,21 +182,6 @@ class TestFileWithoutRequest(FileTestBase): os.close(file_handle) custom_path.unlink(missing_ok=True) - async def test_download_no_filename(self, monkeypatch, file): - async def test(*args, **kwargs): - return self.file_content - - file.file_path = None - - monkeypatch.setattr(file.get_bot().request, "retrieve", test) - out_file = await file.download_to_drive() - - assert str(out_file)[-len(file.file_id) :] == file.file_id - try: - assert out_file.read_bytes() == self.file_content - finally: - out_file.unlink(missing_ok=True) - async def test_download_file_obj(self, monkeypatch, file): async def test(*args, **kwargs): return self.file_content @@ -272,6 +258,14 @@ class TestFileWithoutRequest(FileTestBase): assert buf2[len(buf) :] == buf assert buf2[: len(buf)] == buf + async def test_download_no_file_path(self): + with pytest.raises(RuntimeError, match="No `file_path` available"): + await File(self.file_id, self.file_unique_id).download_to_drive() + with pytest.raises(RuntimeError, match="No `file_path` available"): + await File(self.file_id, self.file_unique_id).download_to_memory(BytesIO()) + with pytest.raises(RuntimeError, match="No `file_path` available"): + await File(self.file_id, self.file_unique_id).download_as_bytearray() + class TestFileWithRequest(FileTestBase): async def test_error_get_empty_file_id(self, bot):