fix: download telegram photo/sticker/document/video to local temp path - #9467
fix: download telegram photo/sticker/document/video to local temp path#9467wcqqq1214 wants to merge 2 commits into
Conversation
Fixes AstrBotDevs#9448: these components stored Telegram's file URL directly, which generic message components can't resolve as a local file.
33a14e9 to
2e14309
Compare
AmirF194
left a comment
There was a problem hiding this comment.
_download_to_temp() passes Telegram's raw getFile file_path straight into download_file()'s url argument (aiohttp session.get(url)), for the photo, sticker, document and video branches.
Per the Bot API docs, file_path from getFile is a path relative to the file storage root (e.g. photos/file_10.jpg), not a full URL. python-telegram-bot's own File.download_to_drive() docstring confirms this: it only treats file_path as directly usable "when a Bot API Server is running in local mode". This adapter is not in local mode: tg_adapter.py:63-71 sets file_base_url to https://api.telegram.org/file/bot by default and wires it into the ApplicationBuilder via .base_file_url(...), so the standard cloud API is the deployment this hits.
Verified in a clean container (python-telegram-bot 22.6):
>>> from telegram import File
>>> File.de_json({"file_id": "abc", "file_unique_id": "u1", "file_path": "photos/file_10.jpg"}, bot=None).file_path
'photos/file_10.jpg'
and feeding that string to the same aiohttp call download_file() makes:
EXCEPTION: InvalidUrlClientError photos/file_10.jpg
So on a standard (non self-hosted) deployment, every photo/sticker/document/video message would raise in _download_to_temp instead of downloading, which is the opposite of the #9448 fix. The existing voice branch just above (untouched by this PR) has the same download_file(file.file_path, ...) shape, so it isn't new here, but this PR is what extends it to four more message types.
The added tests patch download_file with an AsyncMock, so this path never actually runs through aiohttp and the tests pass either way.
file.download_to_drive(custom_path=temp_path) (PTB's own method, which knows how to combine file_base_url and file_path correctly) looks like it would sidestep this, if that fits the intended shape.
|
Thanks for the detailed writeup — the deployment analysis is spot on, this adapter does run against the cloud API by default ( On the cloud path, though, I don't think the failure triggers. The code goes through # telegram/_bot.py (22.x)
file_path = result.get("file_path")
if file_path and not is_local_file(file_path):
result["file_path"] = f"{self._base_file_url}/{file_path}"
return File.de_json(result, self)So by the time That said, your pointer to |
|
@AmirF194 Sorry, forgot to mention you above — the previous comment was a reply to your review. |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tests patch
convert_message.__func__.__globals__and_download_to_temp.__func__.__globals__, which is quite brittle; consider refactoring to injectget_astrbot_temp_path/download_filevia constructor or parameters so they can be overridden in tests without touching function globals. - _download_to_temp currently treats any existing path as a local file (via
os.path.isfile); if Telegram ever returns non-file paths (e.g., directories or other local schemes), you may want to tighten this check or add logging to make misclassified paths more visible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests patch `convert_message.__func__.__globals__` and `_download_to_temp.__func__.__globals__`, which is quite brittle; consider refactoring to inject `get_astrbot_temp_path`/`download_file` via constructor or parameters so they can be overridden in tests without touching function globals.
- _download_to_temp currently treats any existing path as a local file (via `os.path.isfile`); if Telegram ever returns non-file paths (e.g., directories or other local schemes), you may want to tighten this check or add logging to make misclassified paths more visible.
## Individual Comments
### Comment 1
<location path="tests/test_telegram_adapter.py" line_range="159" />
<code_context>
@pytest.mark.asyncio
-async def test_telegram_document_caption_populates_message_text_and_plain():
+async def test_telegram_document_caption_populates_message_text_and_plain(tmp_path):
TelegramPlatformAdapter = _load_telegram_adapter()
adapter = TelegramPlatformAdapter(
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid reaching into `__func__.__globals__` in tests; patch the module symbols instead for clarity and robustness.
Using `convert_message.__func__.__globals__` with `patch.dict` to override `get_astrbot_temp_path`/`download_file` is brittle and hard to follow. Prefer patching these at the module level (e.g. `patch("astrbot.core.platform.sources.telegram.tg_adapter.get_astrbot_temp_path", ...)`) or via pytest’s `monkeypatch` fixture so the tests remain stable if `convert_message` is refactored and are easier to understand.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_telegram_document_caption_populates_message_text_and_plain(tmp_path):
TelegramPlatformAdapter = _load_telegram_adapter()
adapter = TelegramPlatformAdapter(
make_platform_config("telegram"),
caption="@alice 请总结这份文档",
caption_entities=[mention],
)
# Patch the module symbols directly instead of reaching into convert_message.__func__.__globals__
with patch(
"astrbot.core.platform.sources.telegram.tg_adapter.get_astrbot_temp_path",
return_value=tmp_path,
), patch(
"astrbot.core.platform.sources.telegram.tg_adapter.download_file"
```
The `with patch(... download_file` block should be completed to mirror whatever behavior the original `patch.dict(... {"download_file": ...})` provided. For example, you likely need to:
1. Provide an appropriate `side_effect` or `return_value` for `download_file` matching the expectations of `convert_message` in this test (e.g., creating a temporary file in `tmp_path` and returning its path, or returning a bytes-like object).
2. Remove the now-unused mapping body that followed the original `patch.dict(...)` call (the `{ ... }`), since the two `patch` calls fully replace the globals for `get_astrbot_temp_path` and `download_file`.
Adjust the `patch` arguments if the actual module import path for the Telegram adapter differs from `astrbot.core.platform.sources.telegram.tg_adapter`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_telegram_document_caption_populates_message_text_and_plain(): | ||
| async def test_telegram_document_caption_populates_message_text_and_plain(tmp_path): |
There was a problem hiding this comment.
suggestion (testing): Avoid reaching into __func__.__globals__ in tests; patch the module symbols instead for clarity and robustness.
Using convert_message.__func__.__globals__ with patch.dict to override get_astrbot_temp_path/download_file is brittle and hard to follow. Prefer patching these at the module level (e.g. patch("astrbot.core.platform.sources.telegram.tg_adapter.get_astrbot_temp_path", ...)) or via pytest’s monkeypatch fixture so the tests remain stable if convert_message is refactored and are easier to understand.
Suggested implementation:
@pytest.mark.asyncio
async def test_telegram_document_caption_populates_message_text_and_plain(tmp_path):
TelegramPlatformAdapter = _load_telegram_adapter()
adapter = TelegramPlatformAdapter(
make_platform_config("telegram"),
caption="@alice 请总结这份文档",
caption_entities=[mention],
)
# Patch the module symbols directly instead of reaching into convert_message.__func__.__globals__
with patch(
"astrbot.core.platform.sources.telegram.tg_adapter.get_astrbot_temp_path",
return_value=tmp_path,
), patch(
"astrbot.core.platform.sources.telegram.tg_adapter.download_file"The with patch(... download_file block should be completed to mirror whatever behavior the original patch.dict(... {"download_file": ...}) provided. For example, you likely need to:
- Provide an appropriate
side_effectorreturn_valuefordownload_filematching the expectations ofconvert_messagein this test (e.g., creating a temporary file intmp_pathand returning its path, or returning a bytes-like object). - Remove the now-unused mapping body that followed the original
patch.dict(...)call (the{ ... }), since the twopatchcalls fully replace the globals forget_astrbot_temp_pathanddownload_file.
Adjust thepatcharguments if the actual module import path for the Telegram adapter differs fromastrbot.core.platform.sources.telegram.tg_adapter.
Fixes #9448
Telegram photo/sticker/document/video messages stored Telegram's
file_pathdirectly on the message component. The genericFile/Image/Videocomponents only resolve local paths or http(s) URLs from their internalfile_/pathfield, so downstream consumers (LLM file read tool, file extract stage) could not reliably read the content.Modifications / 改动点
Added
TelegramPlatformAdapter._download_to_temp(), downloading via the existingdownload_file()utility intoget_astrbot_temp_path(), mirroring the pattern already used by thevoicebranch. When the Bot API server runs in local mode,get_file()returns an absolute local path instead of an http(s) URL, so the helper returns that path as-is rather than feeding a local path intodownload_file()(an aiohttp GET, which cannot resolve it).Updated the
photo,sticker,document, andvideobranches inconvert_message()to download to a local temp file first, then construct the message component from that local path instead of the raw Telegram URL.Added regression tests asserting the resulting component holds a local temp path (not the original Telegram URL) for all four media types, plus tests covering
_download_to_tempreturning a local path as-is without downloading (local-mode) and still downloading remote URLs; updated existing document/video tests to mock the download call.This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Also verified end-to-end against a real local HTTP server standing in for Telegram's file host (real network GET + real disk write, no mocks), confirming downloaded byte content matches the source and the resulting file is readable as text. Temp files/server were cleaned up after the check.
Checklist / 检查清单
requirements.txtandpyproject.toml.Summary by Sourcery
Ensure Telegram media messages are converted to components that reference locally stored temp files instead of raw Telegram file URLs, improving downstream file readability and compatibility.
Bug Fixes:
Enhancements:
Tests: