Skip to content

fix: download telegram photo/sticker/document/video to local temp path - #9467

Open
wcqqq1214 wants to merge 2 commits into
AstrBotDevs:masterfrom
wcqqq1214:fix/9448-telegram-file-download
Open

fix: download telegram photo/sticker/document/video to local temp path#9467
wcqqq1214 wants to merge 2 commits into
AstrBotDevs:masterfrom
wcqqq1214:fix/9448-telegram-file-download

Conversation

@wcqqq1214

@wcqqq1214 wcqqq1214 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #9448

Telegram photo/sticker/document/video messages stored Telegram's file_path directly on the message component. The generic File/Image/Video components only resolve local paths or http(s) URLs from their internal file_/path field, 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 existing download_file() utility into get_astrbot_temp_path(), mirroring the pattern already used by the voice branch. 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 into download_file() (an aiohttp GET, which cannot resolve it).

  • Updated the photo, sticker, document, and video branches in convert_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_temp returning 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 / 运行截图或测试结果

$ pytest tests/test_telegram_adapter.py -q
..................
18 passed, 1 warning in 3.72s

$ ruff format --check . && ruff check .
483 files already formatted
All checks passed!

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 / 检查清单

  • 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
  • 👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
  • 🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt and pyproject.toml.
  • 😮 My changes do not introduce malicious code.

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:

  • Correct Telegram photo, sticker, document, and video handling so downstream components receive local file paths rather than Telegram URLs, resolving issues with tools that cannot read remote paths.

Enhancements:

  • Add a helper in the Telegram adapter to download media to a temp directory, reusing existing download utilities and supporting both remote URLs and local Bot API file paths.
  • Log warnings when Telegram returns a null file_path for media, avoiding silent failures when files cannot be saved.

Tests:

  • Extend Telegram adapter tests to cover caption handling when downloads are mocked and to verify that photos, stickers, documents, and videos are saved to local temp paths.
  • Add regression tests for the new temp-download helper to confirm it bypasses actual downloads for local paths and correctly downloads remote URLs.

Fixes AstrBotDevs#9448: these components stored Telegram's file URL directly,
which generic message components can't resolve as a local file.
@wcqqq1214
wcqqq1214 force-pushed the fix/9448-telegram-file-download branch from 33a14e9 to 2e14309 Compare July 30, 2026 12:56

@AmirF194 AmirF194 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

@wcqqq1214

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed writeup — the deployment analysis is spot on, this adapter does run against the cloud API by default (file_base_url = https://api.telegram.org/file/bot), not local mode.

On the cloud path, though, I don't think the failure triggers. The code goes through await photo.get_file(), and PTB's Bot.get_file() prefixes the relative file_path with base_file_url before returning:

# 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 _download_to_temp sees it, file.file_path is already a full https://api.telegram.org/file/bot<token>/photos/... URL, which download_file() / aiohttp handles fine. The File.de_json({...}, bot=None) repro constructs the File directly and skips get_file(), which is exactly the step that does the prefixing — that's why the relative path shows up there. (The pre-existing voice branch has shipped with the same download_file(file.file_path, ...) shape, which lines up with this working on cloud.)

That said, your pointer to download_to_drive caught a real bug on the other side: in local mode, get_file() returns an absolute local path (since is_local_file() is true, PTB does not prefix), and feeding that to an aiohttp GET does fail — which is the behavior download_to_drive avoids by handling both cases. I've pushed a fix taking the same approach with a minimal change: _download_to_temp now returns the path as-is when it's already a local file and only downloads remote URLs, with tests covering both. Thanks for the careful review.

@wcqqq1214

Copy link
Copy Markdown
Contributor Author

@AmirF194 Sorry, forgot to mention you above — the previous comment was a reply to your review.

@wcqqq1214
wcqqq1214 marked this pull request as ready for review July 31, 2026 04:55
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:platform The bug / feature is about IM platform adapter, such as QQ, Lark, Telegram, WebChat and so on. labels Jul 31, 2026
@dosubot

dosubot Bot commented Jul 31, 2026

Copy link
Copy Markdown

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-08-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.


Leave Feedback Ask Dosu about AstrBot Add Dosu to your team

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:platform The bug / feature is about IM platform adapter, such as QQ, Lark, Telegram, WebChat and so on. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]telegram文件接受问题

2 participants