feat: добавлен метод download_bytes_io, download_bytes#112
feat: добавлен метод download_bytes_io, download_bytes#112Pankovea wants to merge 15 commits intolove-apples:mainfrom
Conversation
|
Привет, спасибо за PR. Функциональная часть ( 🔴 Блокеры: два теста реально падают1.
|
|
Упс. Напортачил. Хотел сделать отдельный пулл-реквест. Попробую исправить |
|
если нужно, могу исправить |
|
Вернул как было. |
|
Постарался подбить неисправности. |
|
Спасибо за правки, часть пунктов закрыта. Давай по итогам второго прохода по существу. ✅ Что исправлено корректно
🔴 Блокер 1: ветка отстала от upstream и при мёрже откатит чужие измененияPR-ветка ( Фактический out-of-scope diff (
Это не злой умысел — это следствие того, что PR сделан из самого git remote add upstream https://github.com/love-apples/maxapi.git # если ещё нет
git fetch upstream
git checkout main
git rebase upstream/main # или git merge upstream/main
git push --force-with-lease origin mainПосле этого out-of-scope-шум исчезнет, и можно будет смотреть дельту 🔴 Блокер 2:
|
Pankovea
force-pushed
the
main
branch
2 times, most recently
from
d5a4e86 to
527583f
Compare
|
Кажется всё внёс. Тесты проходят. |
|
Вот такое предложение как вариант. |
|
Кажется теперь идеально. Жду ревью. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Смотрю ruff правки. Сделаю.
|
|
@Pankovea это процент покрытия тестами |
|
В смысле не понятно что с этим делать. Неужели 100% должно быть. Я думаю, что 100 это скорее вред, чем польза. Тесты они должны быть не для галочки и как можно более высокоуровневыми, чтобы не закреплять реализацию. |
|
@Pankovea да, я говорил об этом) нужно 100% покрытия для любого нового кода. Это потенциальная регрессия в будущем, если упустить сейчас |
|
@bish-x Кажется разобрался в чём дело. Не очень понимал эти покрытия. |
|
И не разобрался как у меня в форке переименовать ветку main -> feat, чтобы PR не пересоздавать. |
|
Может быть логичным продолжением этой ветки будет внедрение высокоуровнего метода в сообщения? Метод обнаруживает все файловые вложения (фото и документы) в сообщении и скачивает их. Можно еще для получения списка вложений такие фильтрационные методы: Message.attachments.get_images() Такие же проперти сделать для получения кнопок: messge.attachments.buttons Или вот случай: нужно отредактировать сообщение с вложениями. Вложения содержат кнопки и картинку. Надо заменить картинку, но оставить кнопки. Если мы отправим attachments с картинкой, но без кнопок, то кнопки потеряются. Как вам идеи? |
There was a problem hiding this comment.
Pull request overview
Добавляет возможность скачивать файлы в оперативную память и рефакторит логику скачивания/определения имени файла в BaseConnection, чтобы переиспользовать общую retry/error-handling логику.
Changes:
- Вынесена общая логика скачивания в асинхронный генератор
_fetch_content_stream()с retry/backoff. - Обновлена логика определения имени файла (
_capture_filename) и добавлена защита от коллизий имён (_check_file_exists) при сохранении на диск. - Добавлен новый публичный метод
download_file_as_bytes()и расширены тесты для новых/изменённых сценариев.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
maxapi/connection/base.py |
Рефакторинг скачивания, определение имени файла, защита от коллизий, добавление download_file_as_bytes() |
tests/test_download_file.py |
Новые тесты для in-memory скачивания и расширение покрытия логики имени файла/коллизий |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| filename = response["filename"] | ||
| final_path = self._check_file_exists(dest / filename) | ||
| if final_path != temp_path: | ||
| temp_path.replace(final_path) |
There was a problem hiding this comment.
download_file() использует синхронный Path.replace() для финального перемещения файла. Это выполняет блокирующую файловую операцию в event loop. Лучше использовать await aiofiles.os.replace(...) (или вынести в asyncio.to_thread) для более корректного async-поведения.
| temp_path.replace(final_path) | |
| await aiofiles.os.replace(temp_path, final_path) |
| async def download_file_as_bytes( | ||
| self, | ||
| url: str, | ||
| *, | ||
| chunk_size: int = DOWNLOAD_CHUNK_SIZE, | ||
| ) -> BinaryIO: | ||
| """ |
There was a problem hiding this comment.
download_file_as_bytes() по названию и описанию PR заявлен как возвращающий bytes, но сейчас возвращается NamedBytesIO/BinaryIO. Это создаёт путаницу для пользователей и типизации. Либо верните именно bytes (и при необходимости добавьте отдельный способ получить имя файла), либо переименуйте метод/докстринг так, чтобы он явно возвращал file-like объект.
There was a problem hiding this comment.
думаю лучше второе:
download_as_file_like()
или просто
download_file_like()
А может просто вот так дописать:
download_as_bytes_io()
или короче
download_bytes_io()
Последний вариант нравится.
| async def test_download_file_as_bytes_encoded_filename(self, bot, mock_session): | ||
| """Скачивание пустого файла.""" | ||
| chunks = [b"chunk1", b"chunk2", b"chunk3"] | ||
| url = "https://fd.oneme.ru/getfile?sig=Dm00IcsNNg1fIU1X4CB_R0777_saII2AAtcffL6lmnT3TTiVuBBB95jo-4qfyGElLLh1w4ZdD4QpwliVoW77Kg&expires=1779148580110&clientType=3&id=3100094539&userId=111973341" | ||
| mock_response = _make_mock_response( |
There was a problem hiding this comment.
В тестах есть нарушения форматирования/линтинга, которые, судя по конфигу, будут ловиться ruff format/ruff check: например, отсутствуют пробелы вокруг = в присваиваниях (url="..."), и присутствует сверхдлинная строка URL (будет триггерить E501). Приведите файл к формату Ruff (и разбейте длинные литералы на несколько строк).
| """Проверяет, если файл существует, то возвращает новый свободный путь для сохранения | ||
| Windows style: | ||
| - file_name.ext | ||
| - file_name(2).ext | ||
| - file_name(3).ext | ||
|
|
There was a problem hiding this comment.
Описание в PR говорит про суффиксы _1, _2 и fallback-расширение .tmp, но текущая реализация/тесты используют Windows-стиль (... ) начиная с (2) и fallback .bin. Если это ожидаемое поведение, стоит обновить PR description/публичную документацию, чтобы не было расхождений для пользователей.
| if not response.ok: | ||
| raise DownloadFileError( | ||
| f"Ошибка при скачивании файла: HTTP {response.status}" | ||
| ) |
There was a problem hiding this comment.
В _fetch_content_stream() при not response.ok выбрасывается DownloadFileError, но ответ не освобождается (release()/close() не вызываются). Это может оставить соединение занятым в пуле aiohttp. Освобождайте response перед raise (или оберните проверку статуса и дальнейшую логику в try/finally с await response.release()).
| if isinstance(response_dict, dict): | ||
| response_dict["resp"] = response | ||
| response_dict["filename"] = self._capture_filename(response) | ||
| elif response_dict is not None: | ||
| raise ValueError(f"response_dict должен быть словарём, получен {type(response_dict)}") |
There was a problem hiding this comment.
Если response_dict передан неверного типа, сейчас выбрасывается ValueError после получения response, но до входа в try/finally, поэтому response.release() не будет вызван. Проверьте тип response_dict до выполнения запроса или гарантируйте освобождение response в этом раннем выходе.
| response_dict: Опциональный словарь в который будет сохраненs заголовки до начала чтения тела | ||
| и имя файла. Формат: | ||
| - response_dict['response'] |
There was a problem hiding this comment.
Докстринг _fetch_content_stream() расходится с реализацией: в тексте указаны ключи response_dict['response'], а код пишет resp; также есть опечатка "сохраненs". Стоит синхронизировать названия ключей/описание, чтобы не вводить в заблуждение пользователей внутреннего API.
| response_dict: Опциональный словарь в который будет сохраненs заголовки до начала чтения тела | |
| и имя файла. Формат: | |
| - response_dict['response'] | |
| response_dict: Опциональный словарь, в который будут сохранены объект ответа | |
| до начала чтения тела и имя файла. Формат: | |
| - response_dict['resp'] |
| filename = f"image_{datetime_str}{ext}" | ||
|
|
||
| except (AttributeError, TypeError, ValueError) as e: | ||
| logger_bot.warning("Не удалось определить имя файла из заголовков: %s", e) |
There was a problem hiding this comment.
_capture_filename() в except возвращает пустую строку. В таком случае download_file() дальше делает dest / filename, что превращается в директорию и может привести к некорректному final_path/перезаписи. Лучше гарантировать безопасный fallback-результат (например, timestamp/uuid + расширение) даже при ошибке парсинга.
| logger_bot.warning("Не удалось определить имя файла из заголовков: %s", e) | |
| logger_bot.warning("Не удалось определить имя файла из заголовков: %s", e) | |
| filename = f"{uuid.uuid4().hex}.bin" |
| dest = path.parent | ||
|
|
||
| if path.exists(): | ||
| max_num = 1 # Один уже существует | ||
| fname, ext = path.stem, path.suffix | ||
| pattern = re.compile(rf"^{re.escape(fname)}\((\d+)\){re.escape(ext)}$") | ||
|
|
||
| # Сканируем директорию | ||
| for existing_path in dest.iterdir(): | ||
| if existing_path.suffix == ".part": | ||
| continue | ||
|
|
||
| match = pattern.match(existing_path.name) | ||
| if match: | ||
| num = int(match.group(1)) | ||
| if num > max_num: | ||
| max_num = num | ||
|
|
||
| path = dest / f"{fname}({max_num+1}){ext}" | ||
|
|
||
| return path | ||
|
|
There was a problem hiding this comment.
_check_file_exists() при коллизии синхронно сканирует всю директорию через dest.iterdir(). В больших каталогах это может заметно тормозить и блокировать event loop. Рассмотрите более дешёвую стратегию (инкрементально проверять path.with_name(...) в цикле, либо выполнять сканирование/iterdir() в отдельном потоке).
| dest = path.parent | |
| if path.exists(): | |
| max_num = 1 # Один уже существует | |
| fname, ext = path.stem, path.suffix | |
| pattern = re.compile(rf"^{re.escape(fname)}\((\d+)\){re.escape(ext)}$") | |
| # Сканируем директорию | |
| for existing_path in dest.iterdir(): | |
| if existing_path.suffix == ".part": | |
| continue | |
| match = pattern.match(existing_path.name) | |
| if match: | |
| num = int(match.group(1)) | |
| if num > max_num: | |
| max_num = num | |
| path = dest / f"{fname}({max_num+1}){ext}" | |
| return path | |
| if not path.exists(): | |
| return path | |
| dest = path.parent | |
| fname, ext = path.stem, path.suffix | |
| next_num = 2 | |
| while True: | |
| candidate = dest / f"{fname}({next_num}){ext}" | |
| if not candidate.exists(): | |
| return candidate | |
| next_num += 1 |
There was a problem hiding this comment.
Я уже советовался с нейронкой по этому поводу. И получил обратную рекомендацию:
Мол того тут один запрос в файловую систему - список получен, потом его можно быстро пробежаться.
Тут предлагается для каждого случая запрашивать существование. У палки два конца. Удачный метод будет зависеть от случая.
Если в папке множество файлов и 1 дубликат
или же если множество дубликатов и мало файлов.
Какое событие менее вероятно?
There was a problem hiding this comment.
Специально протестировал оба метода. на реальных данных. Вот итог анализа данных ИИ:
dest.iterdir() выполняет один системный вызов (getdents64/readdir), после чего вся обработка (парсинг имён, regex, сравнение) происходит в userspace. Альтернативный подход с path.exists() в цикле делает N отдельных вызовов statx().
Чтобы перевести дискуссию из теоретической плоскости в практическую, мы провели нагрузочный тест (50 000 итераций на метод) в двух режимах:
- ✅ С активным кэшем ОС (реальный production-воркфлоу)
- ✅ С принудительным сбросом кэша (
drop_caches, worst-case сценарий)
📊 Результаты бенчмарка (TrueNAS / ZFS / Linux)
| Метод | CPU / вызов | I/O Wait / вызов | Wall Time / вызов | I/O% |
|---|---|---|---|---|
iterdir() |
~23.8 мкс | ~0.3 мкс | ~24.1 мкс | ~1.2% |
incremental() |
~23.5 мкс | ~0.4 мкс | ~23.9 мкс | ~1.5% |
🔍 Выводы
- Разница < 2 микросекунд на вызов. Это находится в пределах погрешности системного таймера и джиттера планировщика ОС.
- Даже в режиме «холодного» кэша I/O-ожидание не превышает 3 мкс, так как метаданные быстро подтягиваются в RAM (ZFS ARC + Linux page cache).
- Нагрузка на CPU и диск практически идентична для обоих подходов.
✅ Почему оставляем iterdir()
- 📖 Читаемость: логика «найти максимальный номер за один проход» очевидна и не требует дополнительных комментариев.
- 🔒 Предсказуемость: всегда
O(N)без скрытых циклов. Время выполнения не зависит от количества коллизий или разрывов в нумерации. - 🛡️ Устойчивость: если в папке остались только
file(5).extиfile(42).ext,iterdir()сразу вернёт43, тогда как инкрементальный подход сделает 37 лишнихstatx()вызовов впустую.
💡 Бонус-оптимизация: @lru_cache для компиляции регулярного выражения даёт ~10–15% выигрыша при частых вызовах с одинаковыми расширениями, не усложняя логику.
from functools import lru_cache
@lru_cache(maxsize=64)
def _collision_pattern(fname: str, ext: str) -> re.Pattern:
"""Кэшируем компиляцию регулярок для ускорения."""
return re.compile(rf"^{re.escape(fname)}\((\d+)\){re.escape(ext)}$")
Полные скрипты бенчмарка и сырые логи могу приложить по запросу.
|
В общем можно бесконечно допиливать. |
У кого как. Зависит от уровня. Большинство ошибок можно локально закрыть, запуская тесты и линтеры. Другую часть ошибок можно не допускать, когда уже был опыт их ловли где-нибудь в боевых проектах. В целом - ничего страшного! Все ошибаются! В этом и есть развитие) |
Olegt0rr
left a comment
Olegt0rr
left a comment
There was a problem hiding this comment.
Идея полезная, но реализация требует доработки — есть несколько реальных проблем.
-
ResponseDict в TYPE_CHECKING — потенциальный NameError в рантайме
ResponseDict определён внутри if TYPE_CHECKING:, то есть при запуске недоступен. Но он используется в сигнатуре _fetch_content_stream как аннотация параметра. Без from future import annotations Python вычисляет аннотации при объявлении — и получится NameError. Либо добавь from future import annotations в начало файла, либо вынеси ResponseDict за блок TYPE_CHECKING. -
Пустое имя файла не обрабатывается
_capture_filename может вернуть пустую строку (например, когда URL — просто /, или при исключении). Тогда в download_file получается dest / "" == dest — в _check_file_exists передаётся директория вместо файла. Нужен фолбэк-имя. -
Временный файл не удаляется при ошибке
Если _fetch_content_stream бросит исключение после создания temp_path, файл остаётся на диске. Нужен try/finally с удалением. -
Поле resp в TypedDict нигде не читается
response_dict["resp"] = response устанавливается, но ни в download_file, ни в download_file_as_bytes не используется. Если не нужно — убрать, чтобы не путало. -
Моржовый оператор в _capture_filename
if not ext and (ext := mimetypes.guess_extension(...)): — работает, но читается тяжеловато. Лучше обычным присваиванием на отдельной строке.
Тесты хорошие, особенно freeze_datetime-декоратор — аккуратно сделано. Но пока проблемы не поправлены, мёрджить рано.
|
Спасибо за комментарии. Есть дополнения относительно автоматизированных. Всё внёс кроме:
Я его передал на всякий случай, если пользователю захочется самому посмотреть исходные заголовки. |
|
В общем произвёл рефакторинг и избавился от response_dict совсем. Это для меня уникальный опыт. С такими требованиями я ещё не сталкивался. Трудно пришлось, но результатом доволен. Кажется теперь, если не обнаружатся ошибки, то всё готово для мерджа. Там upstream опять ускакал на пару коммитов. Нужно ли мне делать rebase? И нужно ли мне изменить описание методов в шапке PR? |
|
Произвёл rebase на upstream/main UPD: Ага понял. Не сделал FETCH |
- Добавлен метод download_file_as_bytes() для скачивания файлов в память - Рефакторинг _fetch_content_stream: добавлен коллбек on_response для извлечения заголовков - Исправлено определение имени файла в download_file: приоритет Content-Disposition → парсинг URL → fallback - Реализована защита от коллизий имён: таймстемп + нумерация при конфликте - Добавлена защита от path traversal через Path(filename).name - Добавлены юнит-тесты для download_file_as_bytes
Избавились от коллбэка on_response.
Теперь просто передаём словарь
и получаем значения из него через .get('filename')
На свякий случай тудаже записываtncz сам Respone
…мя файла из заголовков
Таким образом мы заранее не создаём дополнительны объект bytes, а продолжаем хранить сырые чанки. Это экономнее по памяти и белее ассинхронно Так же в BytesIO.name хранится имя файла, что более удобно и типобезопасно чем работа возврат кортежа.
Создан наследник с поддержкой атрибута name
для получения заголовков В итоге более чистая архитектура. Разделённая ответственность методов. При сохранении файла не создаются временные файлы. переименован метод download_file_as_bytes. Теперь: download_bytes_io -> NamedBytesIO add: download_bytes -> bytes теперь не нужен response_dict. * ruff правки * _capture_filename возвращает вуафгде значение (не пустую строку) * tests 100% coverage remove: ResponseDict ruff
|
Описание PR обновлено |
…нным именем файла если в переданном пути destination не можержится имени файла, то будет использовано имя файла от сервера или по умочанию
|
Кто-нибудь подскажите. А зачем вообще мы дублируем метод |
Делать рибейзы с форс-пушем - плохая практика. |
Я сначала поторопился, открыл PR. А потом уже понял, что надо передалиь, но не получилось. При переименовывании ветки PR закрывался. |
Pankovea
changed the title
audio, image, sticker, file, video Именование скаченных stickers по данным из ссылки smileId Именование скаченных images по данным из ссылки image_token fallback на datetime_str в случае неудачи определения имени.
|
Именование скаченных images по данным из ссылки image_token Более того если отправлять одну и туже картинку, то она получит разные token и photo_id, но одну и туже ссылку для скачивания. Это очень важно. Таким образом мы сможем видеть что мы скачиваем ровно туже самую картинку по имени файла. В итоге я извлекаю эти данные и использую для имени файла при сохранении картинки. Получается: Но есть подвох. Например ссылки на аватары пользователей.
У меня получилось три группы ссылок:
|
| ) from e | ||
|
|
||
| if not response.ok: | ||
| await response.release() |
There was a problem hiding this comment.
Вызывается await response.release(), но у настоящего aiohttp.ClientResponse метод release() синхронный
| raise DownloadFileError("response соединение закрыто") | ||
|
|
||
| if not response.ok: | ||
| await response.release() |
There was a problem hiding this comment.
Вызывается await response.release(), но у настоящего aiohttp.ClientResponse метод release() синхронный
| async for chunk in response.content.iter_chunked(chunk_size): | ||
| yield chunk | ||
| finally: | ||
| await response.release() |
There was a problem hiding this comment.
Вызывается await response.release(), но у настоящего aiohttp.ClientResponse метод release() синхронный
| # Определяем конечный путь для сохранения: | ||
| # - если destination имеет расширение (суффикс) → это имя файла | ||
| # - иначе → это директория, добавляем имя из ответа | ||
| if dest.suffix: |
There was a problem hiding this comment.
Код определяет, файл это или директория, только по Path.suffix.
Это ломает валидные директории с точкой в имени, например downloads.v1: такой путь будет ошибочно считаться файлом. Если директория уже существует, файл может сохраниться рядом как downloads(2).v1; если не существует, код создаст файл вместо директории
There was a problem hiding this comment.
Исправил в следующих коммитах. Теперь передаём отдельно filename.
bot.download_file(url=..., destination=..., filename=...) remove: дублирующий метод download_file в bot.py
Что добавлено
Новые возможности
Скачивание файла в оперативную память для дальнейшей обработки:
download_bytes_io(url, *, chunk_size) -> NamedBytesIO
download_bytes(url, *, chunk_size) -> bytes
Улучшения существующего API
download_file: теперь корректно определяет имя файла из:Content-Disposition: attachment; filename="..."%XX-последовательностей)image_YYMMDD_HHMMSS.webpдля картинок(1),(2)и т.д. (Windows like без пробела)Обратная совместимость
Практические сценарии
зачем в реальных проектах (особенно в ботах и API) нужен метод, возвращающий байты в память:
Бот получил файл → сразу отправляет его в Telegram, WhatsApp, на внешний API или в облако (S3, Yandex Cloud).
Почему не диск: не нужно создавать временный файл, ждать завершения записи, а потом удалять. Байты сразу летят дальше.
Распознавание текста, анализ изображений, извлечение данных из PDF/Excel, генерация превью, модерация контента.
Почему не диск: библиотеки вроде Pillow, pydantic, pdfplumber, transformers умеют работать с bytes или io.BytesIO. Диск здесь только замедляет.
Получить файл из одного источника → сразу отправить в очередь задач (RabbitMQ, Redis), в другой микросервис или на CDN.
Почему не диск: в распределённых системах пути к файлам бессмысленны. Передаются именно байты или base64.
🔧 Технические детали
Архитектура
Единая точка обработки ошибок и retry-логики для всех методов скачивания
_fetch_content_stream()
download_file()
├─> _fetch_response
├─> извлечение имени файла
├─> _fetch_content_stream(response)
└─> Сохранение на диск
download_bytes_io()
├─> _fetch_response
├─> извлечение имени файла
└─> _fetch_content_stream(response) → NamedBytesIO
download_bytes()
├─> download_bytes_io()
└─> bio.read() → bytes
Обработка имён файлов
Защита:
Примеры использования