fix(core): downscale oversized images#7807
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_exceeds_max_size_bytesand_exceeds_max_size_pathhelpers duplicate the same logic; consider consolidating them into a single helper that accepts a file-like object or a more generic input to reduce repetition. - Both helpers swallow all exceptions and return
False, which could silently mask real image parsing errors; it may be safer to either log these failures or narrow the exception types so unexpected issues surface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_exceeds_max_size_bytes` and `_exceeds_max_size_path` helpers duplicate the same logic; consider consolidating them into a single helper that accepts a file-like object or a more generic input to reduce repetition.
- Both helpers swallow all exceptions and return `False`, which could silently mask real image parsing errors; it may be safer to either log these failures or narrow the exception types so unexpected issues surface.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request enhances the image compression logic to account for image dimensions, ensuring that images exceeding a specified size are compressed even if their file size is small. It also introduces unit tests to verify these changes. The review feedback recommends refactoring the two new helper functions into a single, more versatile helper to eliminate code duplication.
| def _exceeds_max_size_bytes(raw: bytes) -> bool: | ||
| try: | ||
| with PILImage.open(io.BytesIO(raw)) as opened_img: | ||
| return max(opened_img.size) > max_size | ||
| except Exception: # noqa: BLE001 | ||
| return False | ||
|
|
||
| def _exceeds_max_size_path(path: Path) -> bool: | ||
| try: | ||
| with PILImage.open(path) as opened_img: | ||
| return max(opened_img.size) > max_size | ||
| except Exception: # noqa: BLE001 | ||
| return False |
There was a problem hiding this comment.
The two helper functions _exceeds_max_size_bytes and _exceeds_max_size_path perform nearly identical logic. Per the general rules, these should be refactored into a single shared helper function to avoid code duplication. Since PILImage.open accepts both file-like objects and path-like objects, a single helper can handle both cases. Additionally, ensure that this new attachment handling logic is accompanied by corresponding unit tests.
| def _exceeds_max_size_bytes(raw: bytes) -> bool: | |
| try: | |
| with PILImage.open(io.BytesIO(raw)) as opened_img: | |
| return max(opened_img.size) > max_size | |
| except Exception: # noqa: BLE001 | |
| return False | |
| def _exceeds_max_size_path(path: Path) -> bool: | |
| try: | |
| with PILImage.open(path) as opened_img: | |
| return max(opened_img.size) > max_size | |
| except Exception: # noqa: BLE001 | |
| return False | |
| def _exceeds_max_size(source: bytes | Path) -> bool: | |
| try: | |
| fp = io.BytesIO(source) if isinstance(source, bytes) else source | |
| with PILImage.open(fp) as opened_img: | |
| return max(opened_img.size) > max_size | |
| except Exception: # noqa: BLE001 | |
| return False |
References
- When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
| _header, encoded = url_or_path.split(",", 1) | ||
| data = base64.b64decode(encoded) | ||
| if len(data) < min_file_size_bytes: | ||
| if len(data) < min_file_size_bytes and not _exceeds_max_size_bytes(data): |
There was a problem hiding this comment.
Update the call to use the refactored helper function.
| if len(data) < min_file_size_bytes and not _exceeds_max_size_bytes(data): | |
| if len(data) < min_file_size_bytes and not _exceeds_max_size(data): |
References
- When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
| if local_path.stat().st_size < min_file_size_bytes: | ||
| if ( | ||
| local_path.stat().st_size < min_file_size_bytes | ||
| and not _exceeds_max_size_path(local_path) |
There was a problem hiding this comment.
Update the call to use the refactored helper function.
| and not _exceeds_max_size_path(local_path) | |
| and not _exceeds_max_size(local_path) |
References
- When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
Fixes #7791.
Problem
compress_image()only triggered when the source file was larger than 1MB, so a dimension-oversized but highly-compressible image (e.g. solid-color PNG) could bypass downscaling and then be rejected by upstream vision limits (2048x2048).Change
max(width, height) > max_size.data:image/...payloads.Tests
uv run pytest -q tests/unit/test_media_utils_compress_image.pyuv run ruff format .uv run ruff check .Summary by Sourcery
Ensure oversized images are downscaled even when their file size is below the compression threshold.
Bug Fixes:
Tests: