feat: добавлен метод download_file для скачивания файлов#96
feat: добавлен метод download_file для скачивания файлов#96love-apples merged 10 commits intolove-apples:mainfrom
Conversation
- Bot.ensure_session() — гарантированное получение HTTP-сессии (DRY) - BaseConnection.download_file() — потоковое скачивание с retry/backoff - Bot.download_file() — высокоуровневый метод для пользователей - Константа DOWNLOAD_CHUNK_SIZE = 65536 вместо магического числа - Защита от path traversal через Path(filename).name - Тесты: успешное скачивание, path traversal, HTTP-ошибка, retry closes love-apples#64 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Olegt0rr
left a comment
There was a problem hiding this comment.
Отличная работа! Осталось ещё немного причесать дублирование
| for attempt in range(max_retries + 1): | ||
| try: | ||
| response = await session.get(url) | ||
| except ClientConnectionError as e: |
There was a problem hiding this comment.
Логика backoff теперь дублируется в нескольких местах. Опять нарушаем dry. В идеале вынести в отдельный декоратор и навешивать его на нужные функции
Заменяет contextlib.suppress(Exception) на явный except (MaxApiError, MaxConnection) с logger.warning(), чтобы ошибки оставляли след в логах вместо молчаливого подавления.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
PR добавляет поддержку скачивания пользовательских вложений (документы/аудио/изображения/стикеры) по URL в SDK MAX, а также выносит часть логики работы с HTTP-сессией в отдельный метод бота.
Changes:
- Добавлен низкоуровневый
BaseConnection.download_file()со стриминговым чтением и retry/backoff. - Добавлен высокоуровневый
Bot.download_file()иBot.ensure_session()для гарантированного полученияaiohttp-сессии. - Добавлены unit-тесты для
download_fileиensure_session; обновлён экспорт исключений.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
maxapi/connection/base.py |
Retry-helper _retry_request, рефакторинг request(), добавлен download_file() для потокового скачивания. |
maxapi/bot.py |
Добавлен ensure_session() и публичный метод download_file(). |
maxapi/exceptions/download_file.py |
Добавлено новое исключение DownloadFileError. |
maxapi/exceptions/__init__.py |
Экспортирован DownloadFileError через __all__. |
tests/test_download_file.py |
Новые тесты на скачивание/ретраи/path traversal и ensure_session. |
maxapi/utils/updates.py |
Добавлен логгер и обработка ошибок при get_chat_member() в enrichment. |
pyproject.toml |
Ruff-исключение по сложности (C90) для maxapi/utils/updates.py. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not response.ok: | ||
| raise DownloadFileError( | ||
| f"Ошибка при скачивании файла: HTTP {response.status}" | ||
| ) | ||
|
|
||
| cd = response.content_disposition | ||
| if cd and cd.filename: | ||
| filename = Path(cd.filename).name | ||
| else: | ||
| ext = mimetypes.guess_extension(response.content_type or "") or "" | ||
| filename = f"file{ext}" | ||
|
|
||
| path = Path(destination) / filename | ||
|
|
||
| async with aiofiles.open(path, "wb") as f: | ||
| async for chunk in response.content.iter_chunked(chunk_size): | ||
| await f.write(chunk) | ||
|
|
||
| return path |
There was a problem hiding this comment.
В download_file() ответ aiohttp не закрывается/не освобождается. При успешной загрузке после записи на диск стоит гарантированно освободить соединение (например, через try/finally с response.release()/response.close() или использовать async with session.get(...) as response). Сейчас при исключении в середине чтения (или при раннем выходе) соединение может остаться занятым и со временем привести к утечке соединений в пуле.
| if not response.ok: | |
| raise DownloadFileError( | |
| f"Ошибка при скачивании файла: HTTP {response.status}" | |
| ) | |
| cd = response.content_disposition | |
| if cd and cd.filename: | |
| filename = Path(cd.filename).name | |
| else: | |
| ext = mimetypes.guess_extension(response.content_type or "") or "" | |
| filename = f"file{ext}" | |
| path = Path(destination) / filename | |
| async with aiofiles.open(path, "wb") as f: | |
| async for chunk in response.content.iter_chunked(chunk_size): | |
| await f.write(chunk) | |
| return path | |
| try: | |
| if not response.ok: | |
| raise DownloadFileError( | |
| f"Ошибка при скачивании файла: HTTP {response.status}" | |
| ) | |
| cd = response.content_disposition | |
| if cd and cd.filename: | |
| filename = Path(cd.filename).name | |
| else: | |
| ext = mimetypes.guess_extension(response.content_type or "") or "" | |
| filename = f"file{ext}" | |
| path = Path(destination) / filename | |
| async with aiofiles.open(path, "wb") as f: | |
| async for chunk in response.content.iter_chunked(chunk_size): | |
| await f.write(chunk) | |
| return path | |
| finally: | |
| response.release() |
| if not response.ok: | ||
| raise DownloadFileError( | ||
| f"Ошибка при скачивании файла: HTTP {response.status}" | ||
| ) | ||
|
|
||
| cd = response.content_disposition | ||
| if cd and cd.filename: | ||
| filename = Path(cd.filename).name | ||
| else: | ||
| ext = mimetypes.guess_extension(response.content_type or "") or "" | ||
| filename = f"file{ext}" | ||
|
|
||
| path = Path(destination) / filename | ||
|
|
||
| async with aiofiles.open(path, "wb") as f: | ||
| async for chunk in response.content.iter_chunked(chunk_size): | ||
| await f.write(chunk) | ||
|
|
||
| return path |
There was a problem hiding this comment.
При not response.ok выбрасывается DownloadFileError без чтения/освобождения тела ответа. Чтобы не оставлять соединение в состоянии connection acquired, нужно перед raise вызвать await response.read() или response.release() (и аналогично в других ветках раннего выхода).
| if not response.ok: | |
| raise DownloadFileError( | |
| f"Ошибка при скачивании файла: HTTP {response.status}" | |
| ) | |
| cd = response.content_disposition | |
| if cd and cd.filename: | |
| filename = Path(cd.filename).name | |
| else: | |
| ext = mimetypes.guess_extension(response.content_type or "") or "" | |
| filename = f"file{ext}" | |
| path = Path(destination) / filename | |
| async with aiofiles.open(path, "wb") as f: | |
| async for chunk in response.content.iter_chunked(chunk_size): | |
| await f.write(chunk) | |
| return path | |
| try: | |
| if not response.ok: | |
| await response.read() | |
| raise DownloadFileError( | |
| f"Ошибка при скачивании файла: HTTP {response.status}" | |
| ) | |
| cd = response.content_disposition | |
| if cd and cd.filename: | |
| filename = Path(cd.filename).name | |
| else: | |
| ext = mimetypes.guess_extension(response.content_type or "") or "" | |
| filename = f"file{ext}" | |
| path = Path(destination) / filename | |
| async with aiofiles.open(path, "wb") as f: | |
| async for chunk in response.content.iter_chunked(chunk_size): | |
| await f.write(chunk) | |
| return path | |
| finally: | |
| response.release() |
| path = Path(destination) / filename | ||
|
|
||
| async with aiofiles.open(path, "wb") as f: | ||
| async for chunk in response.content.iter_chunked(chunk_size): | ||
| await f.write(chunk) | ||
|
|
There was a problem hiding this comment.
download_file() пишет в Path(destination) / filename, но не гарантирует, что директория destination существует. Сейчас при передаче несуществующего пути метод упадёт с FileNotFoundError (мимо DownloadFileError). Стоит заранее создать директорию (mkdir(parents=True, exist_ok=True)) и/или обернуть ошибку записи в DownloadFileError, чтобы поведение соответствовало docstring.
| path = Path(destination) / filename | |
| async with aiofiles.open(path, "wb") as f: | |
| async for chunk in response.content.iter_chunked(chunk_size): | |
| await f.write(chunk) | |
| destination_path = Path(destination) | |
| try: | |
| destination_path.mkdir(parents=True, exist_ok=True) | |
| path = destination_path / filename | |
| async with aiofiles.open(path, "wb") as f: | |
| async for chunk in response.content.iter_chunked(chunk_size): | |
| await f.write(chunk) | |
| except OSError as e: | |
| raise DownloadFileError( | |
| f"Ошибка при сохранении скачанного файла: {e}" | |
| ) from e |
| bot = self._ensure_bot() | ||
|
|
||
| if not bot.session: | ||
| bot.session = ClientSession( | ||
| base_url=bot.api_url, | ||
| timeout=bot.default_connection.timeout, | ||
| headers=bot.headers, | ||
| **bot.default_connection.kwargs, | ||
| ) | ||
|
|
||
| conn = bot.default_connection | ||
| max_retries = conn.max_retries | ||
| retry_statuses = conn.retry_on_statuses | ||
| backoff_factor = conn.retry_backoff_factor | ||
|
|
||
| url = path.value if isinstance(path, ApiPath) else path | ||
|
|
||
| for attempt in range(max_retries + 1): | ||
| try: | ||
| r = await bot.session.request( | ||
| method=method.value, | ||
| url=url, | ||
| **kwargs, | ||
| ) | ||
| except ClientConnectionError as e: | ||
| if attempt < max_retries: | ||
| delay = backoff_factor * (2**attempt) | ||
| logger_bot.warning( | ||
| f"Ошибка соединения ({e}), " | ||
| f"попытка {attempt + 1}/{max_retries + 1}, " | ||
| f"жду {delay:.1f}с" | ||
| try: | ||
| r = await _retry_request( | ||
| bot.session, | ||
| method.value, | ||
| url, | ||
| max_retries=max_retries, | ||
| retry_statuses=retry_statuses, | ||
| backoff_factor=backoff_factor, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
В request() по-прежнему вручную создаётся ClientSession через if not bot.session, при этом не учитывается кейс bot.session.closed == True и не используется новый Bot.ensure_session() (что заявлено в описании PR). Лучше заменить на session = await bot.ensure_session() и передавать session в _retry_request, чтобы избежать вызовов на закрытой сессии и убрать дублирование логики.
| async def download_file( | ||
| self, | ||
| url: str, | ||
| destination: str | Path, | ||
| *, | ||
| chunk_size: int = 65536, | ||
| ) -> Path: | ||
| """ |
There was a problem hiding this comment.
В Bot.download_file() дефолт chunk_size=65536 дублирует значение константы DOWNLOAD_CHUNK_SIZE из maxapi/connection/base.py. Чтобы не расходиться при будущих изменениях, лучше использовать общую константу (импортировать её или прокинуть дефолт через BaseConnection.download_file).
- request(): replaced manual ClientSession creation with ensure_session() (DRY) - download_file(): added dest.mkdir(parents=True, exist_ok=True) before write - download_file(): wrapped file write in try/finally with response.release() - bot.py: imported DOWNLOAD_CHUNK_SIZE constant, removed magic 65536 default - tests: added session.closed = False to mock sessions in test_retry.py so ensure_session() reuses them instead of creating real connections Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… and _retry_request fallback - test_enrich_event.py: 4 new tests covering MaxApiError/MaxConnection exceptions being swallowed in _resolve_from_user for MessageRemoved and UserRemoved events - test_retry.py: 1 new test covering the post-loop fallback return line in _retry_request via empty range(max_retries=-1) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ASYNC240: dest.mkdir() — блокирующий вызов в async функции. Заменено на aiofiles.os.makedirs(), который выполняется асинхронно через пул потоков и не блокирует event loop.
Убрана дублирующаяся функция _retry_request и ручные retry-циклы. Вместо них — единый декоратор @backoff.on_exception для request() и download_file(), как рекомендовал мейнтейнер. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…m_user После мерджа main в ветку лог-сообщение в get_chat_member потеряло текст исключения, из-за чего падал тест test_user_removed_connection_error_logs_and_keeps_from_user_none. Восстанавливаем %s для exc и чиним CI.
Подтянуты PR из upstream: love-apples#93 (FSM), love-apples#96 (download_file), love-apples#101 (fetch user/chat), love-apples#105 (ClipboardButton), love-apples#109 (share payload), love-apples#110 (webhook secret warning). Конфликт в tests/test_types.py: принят upstream-стиль (явный update_type, разнесённые assert). Сохранены доп. тесты test_get_ids_ignores_inviter_id / test_get_ids_ignores_admin_id — их purpose именно цель PR love-apples#94 (не путать inviter_id/admin_id с user.user_id).
Описание
Добавлен метод
download_fileдля скачивания файлов (документы, аудио, изображения, стикеры) с сервера MAX.closes #64
Что добавлено
Bot.ensure_session()— гарантированное получение HTTP-сессииВынесена логика создания/проверки сессии в отдельный метод. Ранее эта конструкция дублировалась в
request(),upload_file(),upload_file_buffer(). Метод проверяет, что сессия существует и не закрыта, при необходимости создаёт новую.BaseConnection.download_file(url, destination)— низкоуровневое скачиваниеiter_chunkedс настраиваемым размером чанка (константаDOWNLOAD_CHUNK_SIZE = 65536)request()Path(filename).nameотсекает../../Content-Dispositionили MIME-typeaiofilesдля асинхронной записи на дискBot.download_file(url, destination)— высокоуровневый методПример использования:
URL для скачивания доступен в
attachment.payload.urlдля типов: image, audio, file, sticker.Учтённые замечания из PR #90
65536вынесено в константуDOWNLOAD_CHUNK_SIZEBot.ensure_session()(DRY)request()request()Тестирование