Ft/firebase storage workflow #157
Conversation
WalkthroughAdds image upload, processing, storage, and deletion workflows for user avatars and group images. Introduces Firebase-backed storage service, image processing module, new API endpoints, and background task updates. Expands configuration via env vars. Adds tests for image processing, storage workflows, and user service. Updates requirements and gitignore. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API (User/Group Routes)
participant Storage as StorageService
participant Proc as Image Processor
participant FB as Firebase Storage
participant BG as Background Task (DB)
Client->>API: POST /users/me/avatar or /groups/{id}/image (file)
API->>Storage: upload_image_workflow(file, folder, entity_id)
Storage->>Storage: validate_file()
Storage->>Storage: save_to_quarantine()
Storage->>Proc: process_image(bytes)
Proc-->>Storage: {thumbnail, medium, full} bytes
Storage->>FB: upload variants
alt Emulator
Storage-->>API: local URLs
else Production
Storage->>Storage: generate_signed_url()
Storage-->>API: signed URLs
end
API->>BG: update imageUrl (full)
API-->>Client: ImageUploadResponse {success, urls, message}
sequenceDiagram
autonumber
actor Client
participant API as API (User/Group Routes)
participant Storage as StorageService
participant FB as Firebase Storage
participant BG as Background Task (DB)
Client->>API: DELETE /users/me/avatar or /groups/{id}/image
API->>API: read current imageUrl
API->>Storage: extract_path_from_url(url)
API->>Storage: delete_image(path)
Storage->>FB: delete thumbnail/medium/full
alt Delete succeeded
API->>BG: clear imageUrl (None)
API-->>Client: {success, message}
else Delete failed
API-->>Client: 500 error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/image_processor.py (1)
75-123: Avoid blocking the event loop; improve exception chaining and resource handling
- process_image performs CPU-bound work (decode, resize, encode) in an async def, blocking the event loop under load.
- Chain exceptions with “from e” to preserve tracebacks (B904).
- Ensure watermark file handles are closed.
+import asyncio @@ -async def process_image(file_content: bytes) -> Dict[str, bytes]: +async def process_image(file_content: bytes) -> Dict[str, bytes]: @@ - try: - validate_magic_bytes(file_content) - - img = Image.open(BytesIO(file_content)) - img_format = img.format.upper() + try: + # Lightweight checks on the event loop + validate_magic_bytes(file_content) + # Offload heavy processing to a worker thread + return await asyncio.to_thread(_process_image_sync, file_content) + except UnidentifiedImageError as e: + logger.exception("Uploaded file is not a valid image.") + raise ValueError("Invalid image content.") from e + except Exception as e: + logger.exception(f"Image processing error: {e}") + raise RuntimeError("Image processing failed.") from e - # Validate format - if img_format not in ["JPEG", "PNG", "WEBP"]: - raise ValueError(f"Unsupported image format: {img_format}") - - img = strip_exif(img) - - if WATERMARK_PATH: - watermark = Image.open(WATERMARK_PATH) - else: - watermark = None - - results = {} - - for label, size in RESIZE_CONFIG.items(): - resized = resize_image(img.copy(), size) - - if watermark: - resized = add_watermark(resized, watermark) - - # Save to memory in WebP format - buffer = BytesIO() - resized.save( - buffer, format="WEBP", quality=85, method=6 - ) # High quality with compression - buffer.seek(0) - - results[label] = buffer.read() - - return results - - except UnidentifiedImageError: - logger.exception("Uploaded file is not a valid image.") - raise ValueError("Invalid image content.") - except Exception as e: - logger.exception(f"Image processing error: {e}") - raise RuntimeError("Image processing failed.") + +def _process_image_sync(file_content: bytes) -> Dict[str, bytes]: + with Image.open(BytesIO(file_content)) as img: + img_format = (img.format or "").upper() + if img_format not in ["JPEG", "PNG", "WEBP"]: + raise ValueError(f"Unsupported image format: {img_format}") + img = strip_exif(img) + wm = None + if WATERMARK_PATH: + with Image.open(WATERMARK_PATH) as _wm: + wm = _wm.copy() + results: Dict[str, bytes] = {} + for label, size in RESIZE_CONFIG.items(): + resized = resize_image(img.copy(), size) + if wm: + resized = add_watermark(resized, wm) + buf = BytesIO() + resized.save(buf, format="WEBP", quality=85, method=6) + results[label] = buf.getvalue() + return results
🧹 Nitpick comments (27)
backend/requirements.txt (1)
21-22: Pin image deps and note libmagic system requirement
- Align with the rest of the file’s exact/pegged versions to avoid surprises on CI; also guard next majors.
- python-magic needs the libmagic system library present in CI/Docker.
Apply:
-pillow>=10.0.0 -python-magic>=0.4.27 +pillow>=10.0.0,<11.0.0 +python-magic>=0.4.27,<0.5If Windows support is needed, consider documenting/use of
python-magic-binthere; otherwise ensurelibmagicis installed in the runtime image.backend/app/services/schemas.py (1)
6-10: Tighten response model types and defaultsUse stricter typing for URLs and provide sensible defaults to simplify usage.
Apply:
-from typing import Dict, Optional +from typing import Dict, Optional, Literal -from pydantic import BaseModel +from pydantic import BaseModel, AnyUrl @@ -class ImageUploadResponse(BaseModel): - success: bool - urls: Dict[str, str] # {"thumbnail": "url", "medium": "url", "full": "url"} - message: str - processing_id: Optional[str] = None +class ImageUploadResponse(BaseModel): + success: bool = True + # Use AnyUrl to allow gs:// during internal workflows; swap to HttpUrl if only https is expected. + urls: Dict[Literal["thumbnail", "medium", "full"], AnyUrl] + message: Optional[str] = None + processing_id: Optional[str] = Nonebackend/.gitignore (1)
66-75: Add Python ignores and harden secret/key ignoresThe Node/Firebase patterns are fine, but for this Python backend we should also ignore Python build/test artifacts and explicitly ignore Firebase credential files.
Append:
+# Python artifacts +__pycache__/ +*.py[cod] +*.pyo +.pytest_cache/ +.mypy_cache/ +.ruff_cache/ +.coverage +coverage.xml +htmlcov/ +.venv/ +venv/ + +# Firebase credentials (do not commit) +app/services/firebase/ +app/services/firebase/*.json +app/services/firebase/*.pem +app/services/firebase/*.p12 +app/services/firebase/*.p8 + +# Keep quarantined dir out of VCS but allow placeholder +app/services/quarantine/ +!app/services/quarantine/.gitkeepI can add a
.gitkeepto the quarantined dir in this PR if desired.backend/tests/user/test_user_service.py (3)
309-347: Silence Ruff ARG001 by removing unused autouse fixture paramsmock_get_database is autouse; you don't need to include it as an argument. This will also tidy up Ruff ARG001 warnings.
Example for one test (replicate for others in this block):
-async def test_update_user_avatar_url_success(mock_db_client, mock_get_database): +async def test_update_user_avatar_url_success(mock_db_client):Also applies to: 360-397
309-397: pytest asserts are fine; consider per-file ignore to quiet S101Ruff S101 flags assert usage. In pytest tests, assert is idiomatic. Consider configuring Ruff to ignore S101 under backend/tests/** or add a per-file ignore.
Example pyproject.toml:
[tool.ruff.lint.per-file-ignores] "backend/tests/**" = ["S101"]
2-2: Remove unused import 'patch'patch is not used in this module.
-from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMockbackend/app/services/image_processor.py (1)
41-57: Close watermark files and avoid mutating inputs unexpectedly
- add_watermark mutates the passed image; fine, but document it or copy before modification.
- When loading a watermark file (later), ensure it’s closed.
-def add_watermark(image: Image.Image, watermark: Image.Image) -> Image.Image: +def add_watermark(image: Image.Image, watermark: Image.Image) -> Image.Image: """ Adds watermark (bottom-right). Image and watermark must be RGBA. """ - image = image.convert("RGBA") + image = image.convert("RGBA") # mutates; callers pass .copy() if neededAnd load watermark via context manager in process_image (see next comment).
backend/tests/services/test_image_processor.py (1)
81-97: Global toggle reset is correct; minor robustness suggestionYou correctly reset WATERMARK_PATH. For extra safety, use monkeypatch.context() or a fixture to auto-restore even if the test fails midway.
def test_process_image_with_watermark(monkeypatch, tmp_path): with monkeypatch.context() as m: m.setattr(ip, "WATERMARK_PATH", str(watermark_file)) ...backend/app/config.py (1)
23-27: Consider SecretStr for JWT secretPydantic’s SecretStr improves accidental logging safety of sensitive values.
-from pydantic import Field +from pydantic import Field, SecretStr @@ - secret_key: str = "your-super-secret-jwt-key-change-this-in-production" + secret_key: SecretStr = SecretStr("your-super-secret-jwt-key-change-this-in-production")backend/app/groups/routes.py (1)
189-218: Reduce DB round-trips and chain exceptions; reuse membership check resultYou fetch the group (get_group_by_id) and then also call ensure_user_in_group. The latter already returns the raw group document; reuse it and drop the extra call. Also chain exceptions for observability.
Apply:
-async def delete_group_avatar( - group_id: str, - current_user: dict = Depends(get_current_user), - background_tasks: BackgroundTasks = BackgroundTasks(), -): - group = await group_service.get_group_by_id(group_id, current_user["_id"]) - if not group: - raise HTTPException(status_code=404, detail="Group not found") - - await group_service.ensure_user_in_group(group_id, current_user["_id"]) +async def delete_group_avatar( + group_id: str, + current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008 + background_tasks: BackgroundTasks, +): + group = await group_service.ensure_user_in_group(group_id, current_user["_id"]) image_url = group.get("imageUrl") if not image_url: raise HTTPException(status_code=404, detail="Group avatar not found") try: file_path = storage_service.extract_path_from_url(image_url) deleted = await storage_service.delete_image(file_path) - except Exception as e: - raise HTTPException(status_code=500, detail="Failed to delete group avatar") + except Exception as e: + raise HTTPException(status_code=500, detail="Failed to delete group avatar") from eNote: Please verify extract_path_from_url returns a blob path compatible with delete_image (i.e., decoded without the "o/" prefix). If not, we should normalize/percent-decode in storage.extract_path_from_url.
backend/tests/services/test_storage_service.py (3)
120-128: Consolidate settings monkeypatch to avoid overwritingYou patch settings twice; the first assignment is overwritten immediately.
Apply:
- monkeypatch.setattr( - "app.services.storage.settings", - type("obj", (), {"MAX_FILE_SIZE": 5 * 1024 * 1024}), - ) - monkeypatch.setattr( - "app.services.storage.settings", - type("obj", (), {"MAX_FILE_SIZE": 5 * 1024 * 1024, "MAX_IMAGE_PIXELS": 10000}), - ) + monkeypatch.setattr( + "app.services.storage.settings", + type("obj", (), {"MAX_FILE_SIZE": 5 * 1024 * 1024, "MAX_IMAGE_PIXELS": 10000}), + )
69-104: Add a round-trip test to ensure extract_path_from_url output works with delete_imageCurrent tests don’t verify that extract_path_from_url returns a blob path accepted by delete_image. Given percent-encoding and the "o/" prefix, this is easy to regress.
Proposed test:
@pytest.mark.asyncio async def test_delete_with_extracted_path(monkeypatch): url = "https://dummy-project.firebasestorage.app/o/groups%2Fgid%2Ffile_full.webp?alt=media" path = service.extract_path_from_url(url) # should become "groups/gid/file" # Mock storage bucket and blob deletion mock_blob = MagicMock() mock_bucket = MagicMock() mock_bucket.blob.return_value = mock_blob monkeypatch.setattr(storage_module.storage, "bucket", lambda name: mock_bucket) monkeypatch.setattr(service, "get_bucket", lambda: "bucket_name") result = await service.delete_image(path) assert result is True # Validate proper blob paths were constructed (no "o/" and no %2F) expected = [f"groups/gid/file_{s}.webp" for s in ["thumbnail", "medium", "full"]] actual = [call[0][0] for call in mock_bucket.blob.call_args_list] assert actual == expectedIf this fails, normalize the path in extract_path_from_url by removing a leading "o/" and percent-decoding.
1-663: Ruff in tests: allow asserts/fixtures or configure ignores for testsMost Ruff warnings here (S101, ARG001/ARG005) are standard pytest patterns. Prefer configuring Ruff to ignore these in tests, e.g.:
- pyproject.toml:
- [tool.ruff.lint.per-file-ignores]: {"backend/tests/**.py": ["S101","ARG001","ARG005","TRY003"]}
backend/app/user/routes.py (1)
121-124: Graceful handling and exception chaining on delete failuresConsider chaining exceptions; optionally treat missing blobs as success to keep DB consistent.
Apply:
- if not await storage_service.delete_image(file_path): - raise HTTPException(status_code=500, detail="Failed to delete image") + if not await storage_service.delete_image(file_path): + # Option A (current behavior): surface 500 with cause chained + raise HTTPException(status_code=500, detail="Failed to delete image") + # Option B (idempotent delete): log and proceed to clear DB anyway + # logger.warning("Blob(s) missing during delete; clearing DB pointer for idempotency.") + # passAnd chain the original cause in the try/except above if you decide to catch exceptions explicitly.
backend/app/services/storage.py (13)
165-177: Use config-driven size in logs and (optionally) allow truncated loads per setting.Hardcoded “5MB” log;
LOAD_TRUNCATED_IMAGESunused.content = await file.read() await file.seek(0) if len(content) > settings.MAX_FILE_SIZE: - logger.warning("File size larger than 5MB.") + max_mb = settings.MAX_FILE_SIZE / (1024 * 1024) + logger.warning(f"File size larger than {max_mb:.1f} MB.") return False @@ - try: + try: + if LOAD_TRUNCATED_IMAGES: + from PIL import ImageFile + ImageFile.LOAD_TRUNCATED_IMAGES = True img = Image.open(io.BytesIO(content)) img.load()
256-274: Prefer V4 signing; keep emulator fallback separate.Not mandatory, but less brittle and interoperable with GCS.
- return blob.generate_signed_url(expiration=expiry, method="GET") + return blob.generate_signed_url(expiration=expiry, method="GET", version="v4")
303-308: Remove unused variable and standardize exception logging.- except FirebaseError as fe: + except FirebaseError: logger.exception("Firebase error during image upload.") raise - except Exception as e: + except Exception: logger.exception("Image upload to Firebase failed.") raise RuntimeError("Failed to upload image to Firebase.") from eAlso drop
from eor keep the alias; if you keepfrom e, name itas eabove to satisfy Ruff.
349-357: Drop unused exception variable.- except Exception as e: + except Exception: logger.exception("Failed to move file from quarantine to public.") raise
385-387: Drop unused exception variable.- except Exception as e: + except Exception: logger.exception(f"Failed to delete images at {file_path}") return False
120-164: Remove dead, triple-quoted ClamAV block or implement it; keep imports clean.Leaving large commented code +
subprocessimport adds noise.I can wire
_scan_with_clamav_asyncbehindCLAMAV_ENABLEDand tests, or delete it and reintroduce later via a feature flag. Your call.
371-383: Initialize before Storage ops and tolerate missing objects on delete.Small resilience improvement.
async def delete_image(self, file_path: str) -> bool: @@ - try: - bucket_name = self.get_bucket() - bucket = storage.bucket(bucket_name) + try: + ensure_firebase_initialized() + bucket = storage.bucket() for size in ["thumbnail", "medium", "full"]: blob_path = f"{file_path}_{size}.webp" blob = bucket.blob(blob_path) - blob.delete() + try: + blob.delete() + except Exception: + logger.info(f"Image not found (already deleted?): {blob_path}") logger.info(f"Deleted image: {blob_path}") return True
76-81: Unused settings/value cleanup (minor).
LOAD_TRUNCATED_IMAGESis unused prior to proposed change; ensure it’s either used (see above) or drop it. Likewise, consider droppingsubprocessimport if ClamAV stays disabled.
400-423: Double read is fine but consider streaming to quarantine to cap memory.Currently the entire file is read into memory twice across the workflow. Optional: stream
UploadFiledirectly to quarantine and re-read from disk for processing.
1-21: Imports: align with usage.
- Add
quote(used in emulator URL diff).- If ClamAV remains disabled, drop
subprocess. Also consider dropping the unuseduuidimport (you already importuuid4).-import subprocess -import uuid +import subprocess # remove if ClamAV stays disabled @@ -from urllib.parse import urlparse +from urllib.parse import urlparse, quote
279-301: Set sensible cache headers on uploads (optional).Helps CDN/browser caching for public images; safe with versioned keys (UUIDs).
- blob = bucket.blob(blob_path) - blob.upload_from_string(content, content_type="image/webp") + blob = bucket.blob(blob_path) + blob.cache_control = "public, max-age=31536000, immutable" + blob.upload_from_string(content, content_type="image/webp", timeout=60)
165-208: Consider raising typed errors instead of boolean returns from validation.Improves API UX and testability (exact failure reason), but optional.
34-39: Ruff hints: prefer succinct raises and avoid long messages in exceptions.Minor polish; not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
backend/.gitignore(1 hunks)backend/app/config.py(3 hunks)backend/app/groups/routes.py(2 hunks)backend/app/groups/service.py(1 hunks)backend/app/services/image_processor.py(1 hunks)backend/app/services/schemas.py(1 hunks)backend/app/services/storage.py(1 hunks)backend/app/user/routes.py(2 hunks)backend/app/user/service.py(2 hunks)backend/requirements.txt(1 hunks)backend/tests/services/test_image_processor.py(1 hunks)backend/tests/services/test_storage_service.py(1 hunks)backend/tests/user/test_user_service.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend code must be implemented using FastAPI with Python 3.9+ and organized under the /backend/ directory.
Files:
backend/app/services/schemas.pybackend/app/groups/service.pybackend/tests/services/test_storage_service.pybackend/app/user/routes.pybackend/tests/services/test_image_processor.pybackend/tests/user/test_user_service.pybackend/app/groups/routes.pybackend/app/user/service.pybackend/app/config.pybackend/app/services/image_processor.pybackend/app/services/storage.py
backend/app/{auth,user,groups,expenses}/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
backend/app/{auth,user,groups,expenses}/**/*.py: Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
When adding a new API endpoint, add the route to the appropriate service router file in the backend.
Files:
backend/app/groups/service.pybackend/app/user/routes.pybackend/app/groups/routes.pybackend/app/user/service.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend tests must be placed in the /backend/tests/ directory and run using pytest.
Files:
backend/tests/services/test_storage_service.pybackend/tests/services/test_image_processor.pybackend/tests/user/test_user_service.py
backend/app/config.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend configuration settings must be managed in backend/app/config.py.
Files:
backend/app/config.py
🧠 Learnings (1)
📚 Learning: 2025-07-26T09:41:01.332Z
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/config.py : Backend configuration settings must be managed in backend/app/config.py.
Applied to files:
backend/app/config.py
🧬 Code graph analysis (10)
backend/app/services/schemas.py (2)
backend/app/expenses/schemas.py (1)
AttachmentUploadResponse(223-225)backend/app/auth/schemas.py (1)
SuccessResponse(63-65)
backend/app/groups/service.py (3)
backend/app/user/service.py (1)
get_db(14-15)backend/app/auth/service.py (1)
get_db(77-81)backend/tests/groups/test_groups_service.py (2)
test_update_group_not_admin(340-355)test_update_group_invalid_objectid(576-582)
backend/tests/services/test_storage_service.py (1)
backend/app/services/storage.py (11)
FirebaseStorageService(83-427)generate_secure_file_path(90-101)extract_path_from_url(103-118)validate_file(165-207)save_to_quarantine(209-254)generate_signed_url(256-273)upload_to_firebase(275-308)move_quarantine_to_public(310-357)process_image(359-369)delete_image(371-387)upload_image_workflow(389-427)
backend/app/user/routes.py (4)
backend/app/services/schemas.py (1)
ImageUploadResponse(6-10)backend/app/user/schemas.py (1)
DeleteUserResponse(23-25)backend/app/services/storage.py (3)
upload_image_workflow(389-427)extract_path_from_url(103-118)delete_image(371-387)backend/app/user/service.py (2)
update_user_avatar_url(96-101)get_user_by_id(55-64)
backend/tests/services/test_image_processor.py (1)
backend/app/services/image_processor.py (5)
strip_exif(23-29)validate_magic_bytes(32-38)resize_image(59-72)add_watermark(41-56)process_image(75-122)
backend/tests/user/test_user_service.py (2)
backend/app/user/service.py (1)
update_user_avatar_url(96-101)backend/tests/groups/test_groups_service.py (1)
test_update_group_invalid_objectid(576-582)
backend/app/groups/routes.py (5)
backend/app/services/schemas.py (1)
ImageUploadResponse(6-10)backend/app/dependencies.py (1)
get_current_user(12-59)backend/app/groups/service.py (3)
ensure_user_in_group(476-499)update_group_image_url(501-514)get_group_by_id(167-194)backend/app/services/storage.py (3)
upload_image_workflow(389-427)extract_path_from_url(103-118)delete_image(371-387)backend/app/groups/schemas.py (1)
DeleteGroupResponse(65-67)
backend/app/user/service.py (4)
backend/app/groups/service.py (1)
get_db(16-17)backend/app/auth/service.py (1)
get_db(77-81)backend/scripts/migrate_avatar_to_imageurl.py (1)
migrate_avatar_to_imageurl(59-118)backend/app/user/schemas.py (1)
UserProfileUpdateRequest(17-20)
backend/app/services/image_processor.py (1)
backend/app/services/storage.py (1)
process_image(359-369)
backend/app/services/storage.py (1)
backend/app/services/image_processor.py (1)
process_image(75-122)
🪛 Ruff (0.12.2)
backend/app/groups/service.py
485-485: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
486-486: Do not catch blind exception: Exception
(BLE001)
488-488: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/tests/services/test_storage_service.py
35-35: Unused function argument: fixed_uuid
(ARG001)
37-37: Use of assert detected
(S101)
38-38: Use of assert detected
(S101)
41-41: Unused function argument: fixed_uuid
(ARG001)
43-43: Use of assert detected
(S101)
46-46: Unused function argument: fixed_uuid
(ARG001)
48-48: Use of assert detected
(S101)
49-49: Use of assert detected
(S101)
52-52: Unused function argument: fixed_uuid
(ARG001)
54-54: Use of assert detected
(S101)
55-55: Use of assert detected
(S101)
58-58: Unused function argument: fixed_uuid
(ARG001)
60-60: Use of assert detected
(S101)
63-63: Unused function argument: fixed_uuid
(ARG001)
65-65: Use of assert detected
(S101)
66-66: Use of assert detected
(S101)
73-73: Use of assert detected
(S101)
81-81: Use of assert detected
(S101)
88-88: Use of assert detected
(S101)
96-96: Use of assert detected
(S101)
103-103: Use of assert detected
(S101)
131-131: Use of assert detected
(S101)
149-149: Use of assert detected
(S101)
153-153: Unused function argument: monkeypatch
(ARG001)
160-160: Use of assert detected
(S101)
179-179: Use of assert detected
(S101)
197-197: Use of assert detected
(S101)
211-211: Unused lambda argument: args
(ARG005)
211-211: Unused lambda argument: kwargs
(ARG005)
224-224: Use of assert detected
(S101)
237-237: Unused lambda argument: args
(ARG005)
237-237: Unused lambda argument: kwargs
(ARG005)
252-252: Use of assert detected
(S101)
261-261: Unused lambda argument: a
(ARG005)
261-261: Unused lambda argument: kw
(ARG005)
274-274: Unused lambda argument: a
(ARG005)
274-274: Unused lambda argument: kw
(ARG005)
292-292: Unused lambda argument: a
(ARG005)
292-292: Unused lambda argument: kw
(ARG005)
314-314: Use of assert detected
(S101)
339-339: Unused lambda argument: name
(ARG005)
345-345: Use of assert detected
(S101)
346-346: Use of assert detected
(S101)
348-348: Use of assert detected
(S101)
361-361: Unused lambda argument: name
(ARG005)
369-369: Use of assert detected
(S101)
388-388: Unused lambda argument: name
(ARG005)
411-411: Unused lambda argument: name
(ARG005)
425-425: Unused function argument: name
(ARG001)
459-459: Use of assert detected
(S101)
460-460: Use of assert detected
(S101)
464-464: Unused function argument: monkeypatch
(ARG001)
491-491: Use of assert detected
(S101)
515-515: Use of assert detected
(S101)
533-533: Use of assert detected
(S101)
560-560: Unused lambda argument: name
(ARG005)
564-564: Use of assert detected
(S101)
565-565: Use of assert detected
(S101)
570-570: Use of assert detected
(S101)
578-578: Unused function argument: name
(ARG001)
579-579: Create your own exception
(TRY002)
579-579: Avoid specifying long messages outside the exception class
(TRY003)
585-585: Use of assert detected
(S101)
612-612: Unused lambda argument: filename
(ARG005)
617-617: Use of assert detected
(S101)
658-658: Unused lambda argument: filename
(ARG005)
backend/app/user/routes.py
73-73: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
74-74: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
75-75: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-88: Do not catch blind exception: Exception
(BLE001)
88-88: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
90-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
106-106: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
107-107: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/tests/services/test_image_processor.py
20-20: Use of assert detected
(S101)
21-21: Use of assert detected
(S101)
22-22: Use of assert detected
(S101)
43-43: Use of assert detected
(S101)
45-45: Use of assert detected
(S101)
54-54: Use of assert detected
(S101)
62-62: Use of assert detected
(S101)
63-63: Use of assert detected
(S101)
94-94: Use of assert detected
(S101)
backend/tests/user/test_user_service.py
309-309: Unused function argument: mock_get_database
(ARG001)
321-321: Use of assert detected
(S101)
329-329: Unused function argument: mock_get_database
(ARG001)
342-342: Use of assert detected
(S101)
347-347: Unused function argument: mock_get_database
(ARG001)
353-353: Do not assert blind exception: Exception
(B017)
362-362: Unused function argument: mock_get_database
(ARG001)
375-375: Use of assert detected
(S101)
382-382: Unused function argument: mock_get_database
(ARG001)
394-394: Use of assert detected
(S101)
backend/app/groups/routes.py
163-163: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
164-164: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
165-165: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
176-176: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
177-177: Do not catch blind exception: Exception
(BLE001)
178-178: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
192-192: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
193-193: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
208-208: Do not catch blind exception: Exception
(BLE001)
208-208: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
209-209: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/services/image_processor.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Abstract raise to an inner function
(TRY301)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
115-115: Consider moving this statement to an else block
(TRY300)
119-119: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Do not catch blind exception: Exception
(BLE001)
122-122: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
backend/app/services/storage.py
34-34: Abstract raise to an inner function
(TRY301)
34-34: Avoid specifying long messages outside the exception class
(TRY003)
39-39: Abstract raise to an inner function
(TRY301)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
184-184: Do not catch blind exception: Exception
(BLE001)
203-203: Consider moving this statement to an else block
(TRY300)
205-205: Do not catch blind exception: Exception
(BLE001)
239-241: Abstract raise to an inner function
(TRY301)
239-241: Avoid specifying long messages outside the exception class
(TRY003)
252-252: Avoid specifying long messages outside the exception class
(TRY003)
270-270: Consider moving this statement to an else block
(TRY300)
273-273: Avoid specifying long messages outside the exception class
(TRY003)
301-301: Consider moving this statement to an else block
(TRY300)
303-303: Local variable fe is assigned to but never used
Remove assignment to unused variable fe
(F841)
308-308: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Abstract raise to an inner function
(TRY301)
325-325: Avoid specifying long messages outside the exception class
(TRY003)
350-350: Do not catch blind exception: Exception
(BLE001)
353-353: Consider moving this statement to an else block
(TRY300)
355-355: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
369-369: Avoid specifying long messages outside the exception class
(TRY003)
383-383: Consider moving this statement to an else block
(TRY300)
385-385: Do not catch blind exception: Exception
(BLE001)
385-385: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
402-402: Abstract raise to an inner function
(TRY301)
402-402: Avoid specifying long messages outside the exception class
(TRY003)
427-427: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Run Backend Tests & Analytics
backend/app/services/storage.py
[error] 34-34: FIREBASE_PROJECT_ID environment variable is not set. Command: pytest --cov=app --cov-report=xml:coverage.xml --cov-report=term-missing --junit-xml=test-results.xml --tb=short -v tests/
🔇 Additional comments (6)
backend/app/user/service.py (1)
96-102: Ignore suggested hardening changes: conflicts with existing tests
- Tests (
test_update_user_avatar_url_empty_image_url&test_update_user_avatar_url_none_image_url) expect empty/Noneimage_urlto be accepted and onlyimageUrlset (noupdated_at)- Current use of
modified_countaligns withtest_update_user_avatar_url_no_document_modified; switching tomatched_countor validating URLs will break these testsLikely an incorrect or invalid review comment.
backend/tests/services/test_image_processor.py (2)
57-79: Message assertions are resilient; keep them substring-basedMatching “Image processing failed” (without punctuation) is robust to minor wording changes. Good.
25-37: If switching away from imghdr, keep these tests — they still validate behaviorOnce validate_magic_bytes uses python-magic/Pillow, these tests remain valid and will continue to assert ValueError for GIF and random bytes.
backend/app/services/storage.py (3)
393-398: Docstring mismatch with behavior; clarify or change behavior.Current code always processes immediately; docstring claims “leave in quarantine when ClamAV disabled”.
- - If ClamAV disabled: leave in quarantine for later processing + - If ClamAV disabled: process immediately (no AV scan)If you actually want delayed processing, return the
quarantine_pathand enqueue a background job; I can draft that.
430-430: Global service instance is fine; ensure lazy init guards are in place.With
ensure_firebase_initialized()added, this remains safe for tests and production.
285-301: Ignore outdated/v0/bsuggestion: emulator URLs still use/storage/, as tests assert.Likely an incorrect or invalid review comment.
| use_firebase_emulator: bool = Field( | ||
| default=False, env="USE_FIREBASE_EMULATOR" | ||
| ) # type:ignore | ||
| firebase_project_id: Optional[str] = Field( | ||
| default=None, env="FIREBASE_PROJECT_ID" | ||
| ) # type:ignore | ||
| firebase_service_account_path: str = Field( | ||
| default="./firebase-service-account.json", env="FIREBASE_SERVICE_ACCOUNT_PATH" | ||
| ) # type:ignore |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Firebase creds load from environment explicitly
These optional fields won’t map to uppercase env vars unless explicitly bound. Add env=... to avoid surprises in different deployments.
@@
- firebase_type: Optional[str] = None
- firebase_private_key_id: Optional[str] = None
- firebase_private_key: Optional[str] = None
- firebase_client_email: Optional[str] = None
- firebase_client_id: Optional[str] = None
- firebase_auth_uri: Optional[str] = None
- firebase_token_uri: Optional[str] = None
- firebase_auth_provider_x509_cert_url: Optional[str] = None
- firebase_client_x509_cert_url: Optional[str] = None
+ firebase_type: Optional[str] = Field(default=None, env="FIREBASE_TYPE")
+ firebase_private_key_id: Optional[str] = Field(default=None, env="FIREBASE_PRIVATE_KEY_ID")
+ firebase_private_key: Optional[str] = Field(default=None, env="FIREBASE_PRIVATE_KEY")
+ firebase_client_email: Optional[str] = Field(default=None, env="FIREBASE_CLIENT_EMAIL")
+ firebase_client_id: Optional[str] = Field(default=None, env="FIREBASE_CLIENT_ID")
+ firebase_auth_uri: Optional[str] = Field(default=None, env="FIREBASE_AUTH_URI")
+ firebase_token_uri: Optional[str] = Field(default=None, env="FIREBASE_TOKEN_URI")
+ firebase_auth_provider_x509_cert_url: Optional[str] = Field(default=None, env="FIREBASE_AUTH_PROVIDER_X509_CERT_URL")
+ firebase_client_x509_cert_url: Optional[str] = Field(default=None, env="FIREBASE_CLIENT_X509_CERT_URL")Also applies to: 38-47
🤖 Prompt for AI Agents
In backend/app/config.py around lines 28-36 (and also apply same fix to lines
38-47), the Optional/str Firebase fields lack explicit env bindings for the
uppercase variable names, which can cause them to not map to environment
variables in some deployments; update each Field to include the explicit env
parameter matching the intended uppercase names (e.g.,
env="USE_FIREBASE_EMULATOR", env="FIREBASE_PROJECT_ID",
env="FIREBASE_SERVICE_ACCOUNT_PATH" etc.), ensuring optional types remain
correct and no other behavior changes.
| # Image validation configs | ||
| LOAD_TRUNCATED_IMAGES: bool = False | ||
| MAX_IMAGE_PIXELS: int = 50_00_000 | ||
| MAX_FILE_SIZE: int = 5 * 1024 * 1024 | ||
| SIGNED_URL_EXPIRY_SECONDS: int = Field( | ||
| default=3600, env="SIGNED_URL_EXPIRY_SECONDS" | ||
| ) # type:ignore | ||
| CLAMAV_ENABLED: bool = False | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply Pillow limits from settings and drop unused imports
You import Image/ImageFile but don’t apply settings. Centralize this here to avoid duplication in services.
@@
- LOAD_TRUNCATED_IMAGES: bool = False
- MAX_IMAGE_PIXELS: int = 50_00_000
+ LOAD_TRUNCATED_IMAGES: bool = False
+ MAX_IMAGE_PIXELS: int = 50_00_000
@@
settings = Settings()
+# Apply image limits globally for Pillow
+ImageFile.LOAD_TRUNCATED_IMAGES = settings.LOAD_TRUNCATED_IMAGES
+Image.MAX_IMAGE_PIXELS = settings.MAX_IMAGE_PIXELSAlso applies to: 69-70
🤖 Prompt for AI Agents
In backend/app/config.py around lines 47 to 55 (and also apply the same change
for lines 69-70), you need to apply the Pillow global settings from these config
values and remove the unnecessary Image/ImageFile imports elsewhere: after
defining LOAD_TRUNCATED_IMAGES and MAX_IMAGE_PIXELS, set
ImageFile.LOAD_TRUNCATED_IMAGES = LOAD_TRUNCATED_IMAGES and
Image.MAX_IMAGE_PIXELS = MAX_IMAGE_PIXELS so Pillow uses the centralized config;
then remove unused direct imports of Image/ImageFile from other modules and any
redundant local settings there, relying on this centralized application instead.
| @router.post("/{group_id}/image", response_model=ImageUploadResponse) | ||
| async def upload_group_image( | ||
| group_id: str, | ||
| file: UploadFile = File(...), | ||
| background_tasks: BackgroundTasks = BackgroundTasks(), | ||
| current_user: dict = Depends(get_current_user), | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix FastAPI parameter injection: don’t instantiate BackgroundTasks in defaults; keep Depends/File with noqa for B008
FastAPI injects BackgroundTasks without a default. Instantiating it in the signature creates a global object at import time. Also, Ruff’s B008 on File/Depends is a false-positive for FastAPI; add noqa if you keep it.
Apply:
-async def upload_group_image(
- group_id: str,
- file: UploadFile = File(...),
- background_tasks: BackgroundTasks = BackgroundTasks(),
- current_user: dict = Depends(get_current_user),
-):
+async def upload_group_image(
+ group_id: str,
+ file: UploadFile = File(...), # noqa: B008 - FastAPI dependency
+ background_tasks: BackgroundTasks,
+ current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008
+):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @router.post("/{group_id}/image", response_model=ImageUploadResponse) | |
| async def upload_group_image( | |
| group_id: str, | |
| file: UploadFile = File(...), | |
| background_tasks: BackgroundTasks = BackgroundTasks(), | |
| current_user: dict = Depends(get_current_user), | |
| ): | |
| @router.post("/{group_id}/image", response_model=ImageUploadResponse) | |
| async def upload_group_image( | |
| group_id: str, | |
| file: UploadFile = File(...), # noqa: B008 - FastAPI dependency | |
| background_tasks: BackgroundTasks, | |
| current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008 | |
| ): | |
| ... |
🧰 Tools
🪛 Ruff (0.12.2)
163-163: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
164-164: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
165-165: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
In backend/app/groups/routes.py around lines 160-166, remove the default
instantiation of BackgroundTasks in the route signature (do not use
BackgroundTasks() as a default) so FastAPI can inject a fresh BackgroundTasks
per request; change the parameter to background_tasks: BackgroundTasks (no
default). Also keep the File(...) and Depends(...) usages but add a # noqa: B008
comment on those parameters to silence Ruff false-positives if you choose to
keep explicit defaults, or simply ensure no instantiated defaults remain; update
the function signature accordingly.
| await group_service.ensure_user_in_group(group_id, current_user["_id"]) | ||
|
|
||
| try: | ||
| urls = await storage_service.upload_image_workflow( | ||
| file=file, folder="groups", entity_id=group_id | ||
| ) | ||
|
|
||
| except ValueError as ve: | ||
| raise HTTPException(status_code=400, detail=str(ve)) | ||
| except Exception: | ||
| raise HTTPException(status_code=500, detail="Group image upload failed") | ||
|
|
||
| background_tasks.add_task( | ||
| group_service.update_group_image_url, group_id, urls.get("full") | ||
| ) | ||
|
|
||
| return ImageUploadResponse( | ||
| success=True, urls=urls, message="Group image uploaded successfully." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure invalid files return 400; add pre-validation and proper exception chaining
upload_image_workflow wraps ValueError into RuntimeError, so this route currently returns 500 for invalid files. Guard with a pre-check and chain exceptions.
Apply:
await group_service.ensure_user_in_group(group_id, current_user["_id"])
- try:
+ # Pre-validate to ensure 400 on bad input
+ if not await storage_service.validate_file(file):
+ raise HTTPException(status_code=400, detail="Invalid file type or size.")
+ try:
urls = await storage_service.upload_image_workflow(
file=file, folder="groups", entity_id=group_id
)
- except ValueError as ve:
- raise HTTPException(status_code=400, detail=str(ve))
- except Exception:
- raise HTTPException(status_code=500, detail="Group image upload failed")
+ except ValueError as ve:
+ # Defensive: in case workflow preserves ValueError later
+ raise HTTPException(status_code=400, detail=str(ve)) from ve
+ except Exception as e:
+ raise HTTPException(status_code=500, detail="Group image upload failed") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await group_service.ensure_user_in_group(group_id, current_user["_id"]) | |
| try: | |
| urls = await storage_service.upload_image_workflow( | |
| file=file, folder="groups", entity_id=group_id | |
| ) | |
| except ValueError as ve: | |
| raise HTTPException(status_code=400, detail=str(ve)) | |
| except Exception: | |
| raise HTTPException(status_code=500, detail="Group image upload failed") | |
| background_tasks.add_task( | |
| group_service.update_group_image_url, group_id, urls.get("full") | |
| ) | |
| return ImageUploadResponse( | |
| success=True, urls=urls, message="Group image uploaded successfully." | |
| ) | |
| await group_service.ensure_user_in_group(group_id, current_user["_id"]) | |
| # Pre-validate to ensure 400 on bad input | |
| if not await storage_service.validate_file(file): | |
| raise HTTPException(status_code=400, detail="Invalid file type or size.") | |
| try: | |
| urls = await storage_service.upload_image_workflow( | |
| file=file, folder="groups", entity_id=group_id | |
| ) | |
| except ValueError as ve: | |
| # Defensive: in case workflow preserves ValueError later | |
| raise HTTPException(status_code=400, detail=str(ve)) from ve | |
| except Exception as e: | |
| raise HTTPException(status_code=500, detail="Group image upload failed") from e | |
| background_tasks.add_task( | |
| group_service.update_group_image_url, group_id, urls.get("full") | |
| ) | |
| return ImageUploadResponse( | |
| success=True, urls=urls, message="Group image uploaded successfully." | |
| ) |
🧰 Tools
🪛 Ruff (0.12.2)
176-176: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
177-177: Do not catch blind exception: Exception
(BLE001)
178-178: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/groups/routes.py around lines 168 to 186, the route currently
returns 500 for invalid image files because upload_image_workflow wraps
ValueError in a RuntimeError; add a pre-validation for the uploaded file (e.g.,
check mime type and allowed extensions and file size) and raise
HTTPException(status_code=400) immediately for invalid inputs, then call
storage_service.upload_image_workflow in a try/except that also catches
RuntimeError and inspects its __cause__ (or the message) to detect an underlying
ValueError and re-raise HTTPException(status_code=400, detail=...) using
exception chaining (raise ... from runtime_error) for proper traceback; leave
other exceptions as 500.
| background_tasks.add_task( | ||
| group_service.update_group_image_url, group_id, urls.get("full") | ||
| ) | ||
|
|
||
| return ImageUploadResponse( | ||
| success=True, urls=urls, message="Group image uploaded successfully." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid persisting expiring signed URLs; store a stable storage key instead
urls.get("full") is a signed URL (expires). Persisting it in DB will break avatars when it expires. Persist a stable storage key (e.g., the base file_path returned by storage) and generate fresh signed URLs at read time, or make objects public (less preferred).
Minimal approach:
- Extend upload_image_workflow to also return storage_key (e.g., folder/entity/uuid).
- Store storage_key in DB.
- Expose a small endpoint to return fresh signed URLs for a given storage_key when needed.
Do you want a patch for storage.py + schemas and the router updates?
🤖 Prompt for AI Agents
In backend/app/groups/routes.py around lines 180-186, the code currently
persists a signed URL (urls.get("full")) which expires; instead modify the
upload flow to return and persist a stable storage_key (e.g.,
folder/entity/uuid) from storage.upload, update the background task call to pass
and save that storage_key (not the signed URL), update the
ImageUploadResponse/schema to include storage_key (or keep it internal) and
change database column to store storage_key, and add a small endpoint that,
given a storage_key, generates and returns a fresh signed URL using
storage.sign_url at read time; also update storage.py and related schemas to
return storage_key alongside any signed URLs.
| @router.post("/me/avatar", response_model=ImageUploadResponse) | ||
| async def upload_user_avatar( | ||
| file: UploadFile = File(...), | ||
| background_tasks: BackgroundTasks = BackgroundTasks(), | ||
| current_user: dict = Depends(get_current_user), | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix FastAPI parameter injection; type current_user precisely
Avoid instantiating BackgroundTasks in defaults; annotate for injection. Keep File/Depends with noqa for B008. Use Dict[str, Any] for consistency.
Apply:
-async def upload_user_avatar(
- file: UploadFile = File(...),
- background_tasks: BackgroundTasks = BackgroundTasks(),
- current_user: dict = Depends(get_current_user),
-):
+async def upload_user_avatar(
+ file: UploadFile = File(...), # noqa: B008 - FastAPI dependency
+ background_tasks: BackgroundTasks,
+ current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008
+):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @router.post("/me/avatar", response_model=ImageUploadResponse) | |
| async def upload_user_avatar( | |
| file: UploadFile = File(...), | |
| background_tasks: BackgroundTasks = BackgroundTasks(), | |
| current_user: dict = Depends(get_current_user), | |
| ): | |
| @router.post("/me/avatar", response_model=ImageUploadResponse) | |
| async def upload_user_avatar( | |
| file: UploadFile = File(...), # noqa: B008 - FastAPI dependency | |
| background_tasks: BackgroundTasks, | |
| current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008 | |
| ): | |
| ... |
🧰 Tools
🪛 Ruff (0.12.2)
73-73: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
74-74: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
75-75: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
In backend/app/user/routes.py around lines 71 to 76, the endpoint currently
instantiates BackgroundTasks in the function signature and uses a loose dict for
current_user; change the signature so BackgroundTasks is injected (no default
instance) and annotate its type for injection, keep File(...) and Depends(...)
as-is (with noqa for B008 if linter required), and replace current_user: dict
with current_user: Dict[str, Any] (importing typing.Dict and Any) so the
parameter is precisely typed and FastAPI injects dependencies correctly.
| try: | ||
| # Validate, process, upload | ||
| urls = await storage_service.upload_image_workflow( | ||
| file=file, folder="users", entity_id=user_id | ||
| ) | ||
| except ValueError as ve: | ||
| raise HTTPException(status_code=400, detail=str(ve)) | ||
| except FileNotFoundError: | ||
| raise HTTPException(status_code=404, detail="Storage location not found.") | ||
| except Exception as e: | ||
| logger.exception(f"Unexpected error during avatar upload for user {user_id}") | ||
| raise HTTPException( | ||
| status_code=500, detail="Image upload failed due to an internal error." | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return 400 for invalid files; chain exceptions and remove unused variable
Pre-validate so user errors return 400 regardless of workflow wrapping; also chain exceptions and drop the unused e.
Apply:
- try:
+ # Pre-validate to map bad uploads to 400
+ if not await storage_service.validate_file(file):
+ raise HTTPException(status_code=400, detail="Invalid file type or size.")
+ try:
# Validate, process, upload
urls = await storage_service.upload_image_workflow(
file=file, folder="users", entity_id=user_id
)
except ValueError as ve:
- raise HTTPException(status_code=400, detail=str(ve))
+ raise HTTPException(status_code=400, detail=str(ve)) from ve
except FileNotFoundError:
- raise HTTPException(status_code=404, detail="Storage location not found.")
- except Exception as e:
+ raise HTTPException(status_code=404, detail="Storage location not found.")
+ except Exception as e:
logger.exception(f"Unexpected error during avatar upload for user {user_id}")
- raise HTTPException(
- status_code=500, detail="Image upload failed due to an internal error."
- )
+ raise HTTPException(
+ status_code=500, detail="Image upload failed due to an internal error."
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| # Validate, process, upload | |
| urls = await storage_service.upload_image_workflow( | |
| file=file, folder="users", entity_id=user_id | |
| ) | |
| except ValueError as ve: | |
| raise HTTPException(status_code=400, detail=str(ve)) | |
| except FileNotFoundError: | |
| raise HTTPException(status_code=404, detail="Storage location not found.") | |
| except Exception as e: | |
| logger.exception(f"Unexpected error during avatar upload for user {user_id}") | |
| raise HTTPException( | |
| status_code=500, detail="Image upload failed due to an internal error." | |
| ) | |
| # Pre-validate to map bad uploads to 400 | |
| if not await storage_service.validate_file(file): | |
| raise HTTPException(status_code=400, detail="Invalid file type or size.") | |
| try: | |
| # Validate, process, upload | |
| urls = await storage_service.upload_image_workflow( | |
| file=file, folder="users", entity_id=user_id | |
| ) | |
| except ValueError as ve: | |
| raise HTTPException(status_code=400, detail=str(ve)) from ve | |
| except FileNotFoundError: | |
| raise HTTPException(status_code=404, detail="Storage location not found.") | |
| except Exception as e: | |
| logger.exception(f"Unexpected error during avatar upload for user {user_id}") | |
| raise HTTPException( | |
| status_code=500, detail="Image upload failed due to an internal error." | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-88: Do not catch blind exception: Exception
(BLE001)
88-88: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
90-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/user/routes.py around lines 79 to 93, pre-validate the incoming
file (e.g., check content-type/size/extension) before calling
storage_service.upload_image_workflow so client-side validation always yields a
400; when catching ValueError, re-raise the HTTPException chaining the original
exception (use "from ve") so trace is preserved; remove the unused exception
variable in the broad except (use "except Exception:"), keep
logger.exception(...) to record the stack, and for FileNotFoundError consider
chaining as well (raise HTTPException(...) from that exception) to preserve
context.
| background_tasks.add_task( | ||
| user_service.update_user_avatar_url, user_id, urls.get("full") | ||
| ) | ||
|
|
||
| return ImageUploadResponse( | ||
| success=True, urls=urls, message="Avatar uploaded successfully." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid persisting signed URLs; store a stable key and generate URLs on demand
Storing urls.get("full") will break after expiry. Persist a storage_key (e.g., folder/entity/uuid) and generate signed URLs per request, or make objects public.
I can draft a small helper (e.g., user_service.get_avatar_url) that signs on read and keeps DB normalized to storage_key. Want a patch?
🤖 Prompt for AI Agents
backend/app/user/routes.py lines 95-101: the code currently persists the signed
URL (urls.get("full")) which will expire; instead persist a stable storage_key
(e.g. folder/entity/uuid) and generate signed URLs on demand. Update the call to
background_tasks.add_task to pass the storage_key (not the signed URL) and
change user_service.update_user_avatar_url to store that storage_key. Add a
helper user_service.get_avatar_url(storage_key, expires=...) that signs/returns
a temporary URL for reads, and use that helper to generate the URL returned in
ImageUploadResponse so the DB stays normalized and signed links are created per
request.
| @router.delete("/me/avatar", response_model=DeleteUserResponse) | ||
| async def delete_user_avatar( | ||
| current_user: dict = Depends(get_current_user), | ||
| background_tasks: BackgroundTasks = BackgroundTasks(), | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix FastAPI injection for BackgroundTasks and type current_user; no default instantiation
Same rationale as the upload endpoint.
Apply:
-async def delete_user_avatar(
- current_user: dict = Depends(get_current_user),
- background_tasks: BackgroundTasks = BackgroundTasks(),
-):
+async def delete_user_avatar(
+ current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008
+ background_tasks: BackgroundTasks,
+):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @router.delete("/me/avatar", response_model=DeleteUserResponse) | |
| async def delete_user_avatar( | |
| current_user: dict = Depends(get_current_user), | |
| background_tasks: BackgroundTasks = BackgroundTasks(), | |
| ): | |
| @router.delete("/me/avatar", response_model=DeleteUserResponse) | |
| async def delete_user_avatar( | |
| current_user: Dict[str, Any] = Depends(get_current_user), # noqa: B008 | |
| background_tasks: BackgroundTasks, | |
| ): | |
| ... |
🧰 Tools
🪛 Ruff (0.12.2)
106-106: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
107-107: Do not perform function call BackgroundTasks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
In backend/app/user/routes.py around lines 104-108, the endpoint currently
default-instantiates BackgroundTasks and types current_user as dict; update the
signature to rely on FastAPI dependency injection by removing the
BackgroundTasks default (accept BackgroundTasks as a parameter injected by
FastAPI) and change current_user from dict to the concrete user model type used
elsewhere in your app (e.g., User or UserInDB) while keeping
Depends(get_current_user). Do not instantiate BackgroundTasks in the parameter
list and ensure the imported user model type is used for typing.
| async def test_update_user_avatar_url_success(mock_db_client, mock_get_database): | ||
| """Test successful user avatar URL update""" | ||
| user_id = "642f1e4a9b3c2d1f6a1b2c3d" | ||
| image_url = "https://example.com/avatar.jpg" | ||
|
|
||
| # Mock successful update (1 document modified) | ||
| mock_result = AsyncMock() | ||
| mock_result.modified_count = 1 | ||
| mock_db_client.users.update_one.return_value = mock_result | ||
|
|
||
| result = await user_service.update_user_avatar_url(user_id, image_url) | ||
|
|
||
| assert result is True | ||
| mock_db_client.users.update_one.assert_called_once_with( | ||
| {"_id": ObjectId(user_id)}, {"$set": {"imageUrl": image_url}} | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_user_avatar_url_no_document_modified( | ||
| mock_db_client, mock_get_database | ||
| ): | ||
| """Test when no document is modified (user not found)""" | ||
| user_id = "642f1e4a9b3c2d1f6a1b2c3d" | ||
| image_url = "https://example.com/avatar.jpg" | ||
|
|
||
| # Mock no documents modified | ||
| mock_result = AsyncMock() | ||
| mock_result.modified_count = 0 | ||
| mock_db_client.users.update_one.return_value = mock_result | ||
|
|
||
| result = await user_service.update_user_avatar_url(user_id, image_url) | ||
|
|
||
| assert result is False | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_user_avatar_url_invalid_object_id( | ||
| mock_db_client, mock_get_database | ||
| ): | ||
| """Test with invalid ObjectId format""" | ||
| invalid_user_id = "invalid_object_id" # Not a 24-char hex string | ||
| image_url = "https://example.com/avatar.jpg" | ||
|
|
||
| with pytest.raises(Exception): # ObjectId will raise an exception | ||
| await user_service.update_user_avatar_url(invalid_user_id, image_url) | ||
|
|
||
| # Should never hit the database | ||
| mock_db_client.users.update_one.assert_not_called() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_user_avatar_url_empty_image_url( | ||
| mock_db_client, mock_get_database | ||
| ): | ||
| """Test updating with empty image URL""" | ||
| user_id = "642f1e4a9b3c2d1f6a1b2c3d" | ||
| image_url = "" | ||
|
|
||
| # Mock successful update | ||
| mock_result = AsyncMock() | ||
| mock_result.modified_count = 1 | ||
| mock_db_client.users.update_one.return_value = mock_result | ||
|
|
||
| result = await user_service.update_user_avatar_url(user_id, image_url) | ||
|
|
||
| assert result is True | ||
| mock_db_client.users.update_one.assert_called_once_with( | ||
| {"_id": ObjectId(user_id)}, {"$set": {"imageUrl": image_url}} | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_user_avatar_url_none_image_url(mock_db_client, mock_get_database): | ||
| """Test updating with None image URL""" | ||
| user_id = "642f1e4a9b3c2d1f6a1b2c3d" | ||
| image_url = None | ||
|
|
||
| # Mock successful update | ||
| mock_result = AsyncMock() | ||
| mock_result.modified_count = 1 | ||
| mock_db_client.users.update_one.return_value = mock_result | ||
|
|
||
| result = await user_service.update_user_avatar_url(user_id, image_url) | ||
|
|
||
| assert result is True | ||
| mock_db_client.users.update_one.assert_called_once_with( | ||
| {"_id": ObjectId(user_id)}, {"$set": {"imageUrl": image_url}} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align invalid ObjectId handling and type with existing service patterns
- In prior tests (get_user_by_id, update_user_profile, delete_user), invalid ObjectIds do not raise; they return None/False and skip DB calls. Here, test_update_user_avatar_url_invalid_object_id expects an Exception, which is inconsistent and will propagate 500s from the service.
- Also, tests pass None to image_url, but the service signature is image_url: str. Make it Optional[str] to reflect intended use.
Apply these diffs:
- Service: validate ObjectId and accept Optional[str]
# file: backend/app/user/service.py
@@
-async def update_user_avatar_url(self, user_id: str, image_url: str) -> bool:
- db = self.get_db()
- result = await db.users.update_one(
- {"_id": ObjectId(user_id)}, {"$set": {"imageUrl": image_url}}
- )
- return result.modified_count == 1
+from bson.errors import InvalidId
+
+async def update_user_avatar_url(self, user_id: str, image_url: Optional[str]) -> bool:
+ # Validate ObjectId up front to avoid exceptions bubbling to the API layer
+ if not ObjectId.is_valid(user_id):
+ return False
+ try:
+ db = self.get_db()
+ result = await db.users.update_one(
+ {"_id": ObjectId(user_id)}, {"$set": {"imageUrl": image_url}}
+ )
+ return result.modified_count == 1
+ except InvalidId:
+ return False- Tests: return False for invalid ids, not exception
@@
-@pytest.mark.asyncio
-async def test_update_user_avatar_url_invalid_object_id(
- mock_db_client, mock_get_database
-):
- """Test with invalid ObjectId format"""
- invalid_user_id = "invalid_object_id" # Not a 24-char hex string
- image_url = "https://example.com/avatar.jpg"
-
- with pytest.raises(Exception): # ObjectId will raise an exception
- await user_service.update_user_avatar_url(invalid_user_id, image_url)
-
- # Should never hit the database
- mock_db_client.users.update_one.assert_not_called()
+@pytest.mark.asyncio
+async def test_update_user_avatar_url_invalid_object_id(mock_db_client, mock_get_database):
+ """Invalid ObjectId should return False and skip DB call"""
+ invalid_user_id = "invalid_object_id"
+ image_url = "https://example.com/avatar.jpg"
+ result = await user_service.update_user_avatar_url(invalid_user_id, image_url)
+ assert result is False
+ mock_db_client.users.update_one.assert_not_called()Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
309-309: Unused function argument: mock_get_database
(ARG001)
321-321: Use of assert detected
(S101)
329-329: Unused function argument: mock_get_database
(ARG001)
342-342: Use of assert detected
(S101)
347-347: Unused function argument: mock_get_database
(ARG001)
353-353: Do not assert blind exception: Exception
(B017)
362-362: Unused function argument: mock_get_database
(ARG001)
375-375: Use of assert detected
(S101)
382-382: Unused function argument: mock_get_database
(ARG001)
394-394: Use of assert detected
(S101)
🤖 Prompt for AI Agents
In backend/tests/user/test_user_service.py around lines 309 to 397, the invalid
ObjectId test currently expects an exception and tests pass None for image_url
despite the service signature declaring image_url: str; update the service to
validate the user_id with ObjectId.is_valid and return False when invalid
(instead of raising), change the service signature to accept Optional[str] for
image_url, and update this test to assert False and that the DB update is not
called (no exception), aligning behavior with other user tests.
1 participant
✨ Summary
This PR introduces secure image upload handling and integrates Firebase Cloud Storage into the backend, replacing the earlier base64-based avatar storage. The goal is to provide a scalable, efficient, and secure way to handle user and group avatars.
🔧 Key Changes
app/services/storage.pyandapp/services/image_processor.pyfor Firebase storage and image validation.userandgroupsservices to support Firebase-hosted avatar URLs.scripts/migrate_avatar_to_imageurl.pyfor existing avatars.tests/services,tests/groups, andtests/user.📂 Firebase Directory
A
firebase/directory exists underapp/services/but was not committed (contains credentials & project-specific configs).Admin should link the project to the official Firebase account instead.
Tree for reference:
✅ Notes
Summary by CodeRabbit