-
Notifications
You must be signed in to change notification settings - Fork 24
Ft/firebase storage workflow #157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
be8ed1e
ac6645e
90c42da
6c455f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| # Logs | ||
| logs | ||
| *.log | ||
| npm-debug.log* | ||
| yarn-debug.log* | ||
| yarn-error.log* | ||
| firebase-debug.log* | ||
| firebase-debug.*.log* | ||
|
|
||
| # Firebase cache | ||
| .firebase/ | ||
|
|
||
| # Firebase config | ||
|
|
||
| # Uncomment this if you'd like others to create their own Firebase project. | ||
| # For a team working on the same Firebase project(s), it is recommended to leave | ||
| # it commented so all members can deploy to the same project(s) in .firebaserc. | ||
| # .firebaserc | ||
|
|
||
| # Runtime data | ||
| pids | ||
| *.pid | ||
| *.seed | ||
| *.pid.lock | ||
|
|
||
| # Directory for instrumented libs generated by jscoverage/JSCover | ||
| lib-cov | ||
|
|
||
| # Coverage directory used by tools like istanbul | ||
| coverage | ||
|
|
||
| # nyc test coverage | ||
| .nyc_output | ||
|
|
||
| # Grunt intermediate storage (http://gruntjs.com/creating-plugins#storing-task-files) | ||
| .grunt | ||
|
|
||
| # Bower dependency directory (https://bower.io/) | ||
| bower_components | ||
|
|
||
| # node-waf configuration | ||
| .lock-wscript | ||
|
|
||
| # Compiled binary addons (http://nodejs.org/api/addons.html) | ||
| build/Release | ||
|
|
||
| # Dependency directories | ||
| node_modules/ | ||
|
|
||
| # Optional npm cache directory | ||
| .npm | ||
|
|
||
| # Optional eslint cache | ||
| .eslintcache | ||
|
|
||
| # Optional REPL history | ||
| .node_repl_history | ||
|
|
||
| # Output of 'npm pack' | ||
| *.tgz | ||
|
|
||
| # Yarn Integrity file | ||
| .yarn-integrity | ||
|
|
||
| # dotenv environment variables file | ||
| .env | ||
|
|
||
| # dataconnect generated files | ||
| .dataconnect | ||
|
|
||
| # firebase-config files | ||
| app/services/firebase | ||
|
|
||
| # quarantine files | ||
| app/services/quarantine/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,15 @@ | |
| from logging.config import dictConfig | ||
| from typing import Optional | ||
|
|
||
| from dotenv import load_dotenv | ||
| from PIL import Image, ImageFile | ||
| from pydantic import Field | ||
| from pydantic_settings import BaseSettings | ||
| from starlette.middleware.base import BaseHTTPMiddleware | ||
| from starlette.requests import Request | ||
|
|
||
| load_dotenv() | ||
|
|
||
|
|
||
| class Settings(BaseSettings): | ||
| # Database | ||
|
|
@@ -20,8 +25,15 @@ class Settings(BaseSettings): | |
| access_token_expire_minutes: int = 15 | ||
| refresh_token_expire_days: int = 30 | ||
| # Firebase | ||
| firebase_project_id: Optional[str] = None | ||
| firebase_service_account_path: str = "./firebase-service-account.json" | ||
| 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 | ||
| # Firebase service account credentials as environment variables | ||
| firebase_type: Optional[str] = None | ||
| firebase_private_key_id: Optional[str] = None | ||
|
|
@@ -32,6 +44,14 @@ class Settings(BaseSettings): | |
| firebase_token_uri: Optional[str] = None | ||
| firebase_auth_provider_x509_cert_url: Optional[str] = None | ||
| firebase_client_x509_cert_url: Optional[str] = None | ||
| # 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 | ||
|
|
||
|
Comment on lines
+47
to
55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ 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 |
||
| # App | ||
| debug: bool = False | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,7 +15,17 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RemoveMemberResponse, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from app.groups.service import group_service | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from fastapi import APIRouter, Depends, HTTPException, status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from app.services.schemas import ImageUploadResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from app.services.storage import storage_service | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from fastapi import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| APIRouter, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BackgroundTasks, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Depends, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| HTTPException, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UploadFile, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router = APIRouter(prefix="/groups", tags=["Groups"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -145,3 +155,64 @@ async def remove_group_member( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not removed: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException(status_code=400, detail="Failed to remove member") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return RemoveMemberResponse(success=True, message="Member removed successfully") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @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), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+160
to
+166
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ 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
Suggested change
π§° Toolsπͺ Ruff (0.12.2)163-163: Do not perform function call (B008) 164-164: Do not perform function call (B008) 165-165: Do not perform function call (B008) π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+168
to
+186
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ 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
Suggested change
π§° Toolsπͺ Ruff (0.12.2)176-176: Within an (B904) 177-177: Do not catch blind exception: (BLE001) 178-178: Within an (B904) π€ Prompt for AI Agents
Comment on lines
+180
to
+186
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ 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:
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.delete("/{group_id}/image", response_model=DeleteGroupResponse) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not deleted: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException(status_code=500, detail="Failed to delete group avatar") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| background_tasks.add_task(group_service.update_group_image_url, group_id, None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return DeleteGroupResponse( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| success=True, message="Group image deleted successfully." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -473,5 +473,45 @@ async def remove_member(self, group_id: str, member_id: str, user_id: str) -> bo | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result.modified_count == 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def ensure_user_in_group(self, group_id: str, user_id: str) -> dict: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Ensure that the user is a member of the group. Raises HTTPException if not.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| db = self.get_db() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj_id = ObjectId(group_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except errors.InvalidId: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Invalid group_id: {group_id}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException(status_code=400, detail="Invalid group ID format") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(f"Unexpected error converting group_id to ObjectId: {e}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException(status_code=500, detail="Internal server error") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+480
to
+489
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Exception handling: drop broad catch and use exception chaining Catching bare Exception trips linters (BLE001) and obscures root causes. Chain InvalidId for clearer traces (B904). Apply: - try:
- obj_id = ObjectId(group_id)
- except errors.InvalidId:
- logger.warning(f"Invalid group_id: {group_id}")
- raise HTTPException(status_code=400, detail="Invalid group ID format")
- except Exception as e:
- logger.error(f"Unexpected error converting group_id to ObjectId: {e}")
- raise HTTPException(status_code=500, detail="Internal server error")
+ try:
+ obj_id = ObjectId(group_id)
+ except errors.InvalidId as err:
+ logger.warning(f"Invalid group_id: {group_id}")
+ raise HTTPException(status_code=400, detail="Invalid group ID format") from errπ Committable suggestion
Suggested change
π§° Toolsπͺ Ruff (0.12.2)485-485: Within an (B904) 486-486: Do not catch blind exception: (BLE001) 488-488: Within an (B904) π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| group = await db.groups.find_one( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {"_id": obj_id, "members": {"$elemMatch": {"userId": user_id}}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not group: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status_code=403, detail="You are not a member of this group" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return group # Optional return if route needs to read group data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def update_group_image_url(self, group_id: str, image_url: str) -> bool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Update the group's image URL in the database.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| db = self.get_db() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj_id = ObjectId(group_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except errors.InvalidId: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Invalid group_id: {group_id}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = await db.groups.update_one( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {"_id": obj_id}, {"$set": {"imageUrl": image_url}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result.modified_count == 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+501
to
+515
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Validate image URL and return matched_count
Apply: async def update_group_image_url(self, group_id: str, image_url: str) -> bool:
"""Update the group's image URL in the database."""
db = self.get_db()
- try:
- obj_id = ObjectId(group_id)
- except errors.InvalidId:
- logger.warning(f"Invalid group_id: {group_id}")
- return False
-
- result = await db.groups.update_one(
- {"_id": obj_id}, {"$set": {"imageUrl": image_url}}
- )
- return result.modified_count == 1
+ # Basic input validation
+ if not isinstance(image_url, str) or not image_url.strip():
+ return False
+
+ try:
+ obj_id = ObjectId(group_id)
+ except errors.InvalidId:
+ logger.warning(f"Invalid group_id: {group_id}")
+ return False
+
+ result = await db.groups.update_one(
+ {"_id": obj_id}, {"$set": {"imageUrl": image_url.strip()}}
+ )
+ return result.matched_count == 1π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| group_service = GroupService() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,122 @@ | ||||||||||||||||||
| import imghdr | ||||||||||||||||||
| from io import BytesIO | ||||||||||||||||||
| from typing import Dict, Tuple | ||||||||||||||||||
|
|
||||||||||||||||||
| from app.config import logger | ||||||||||||||||||
| from PIL import Image, ImageFile, UnidentifiedImageError | ||||||||||||||||||
|
|
||||||||||||||||||
| # Optional watermark path | ||||||||||||||||||
| WATERMARK_PATH = None # Example: "app/assets/watermark.png" | ||||||||||||||||||
|
|
||||||||||||||||||
| # Resize targets (square thumbnail + larger sizes) | ||||||||||||||||||
| RESIZE_CONFIG = { | ||||||||||||||||||
| "thumbnail": (150, 150), | ||||||||||||||||||
| "medium": (300, 300), | ||||||||||||||||||
| "full": (800, 800), | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| # Defining image file restrictions | ||||||||||||||||||
| ImageFile.LOAD_TRUNCATED_IMAGES = False | ||||||||||||||||||
| Image.MAX_IMAGE_PIXELS = 50_00_000 # 50MB in worst case | ||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
+19
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Incorrect comment and hard-coded Pillow limits; bind to settings
-from PIL import Image, ImageFile, UnidentifiedImageError
+from PIL import Image, ImageFile, UnidentifiedImageError
+from app.config import settings
@@
-ImageFile.LOAD_TRUNCATED_IMAGES = False
-Image.MAX_IMAGE_PIXELS = 50_00_000 # 50MB in worst case
+ImageFile.LOAD_TRUNCATED_IMAGES = settings.LOAD_TRUNCATED_IMAGES
+Image.MAX_IMAGE_PIXELS = settings.MAX_IMAGE_PIXELS # max safe pixelsπ Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| def strip_exif(image: Image.Image) -> Image.Image: | ||||||||||||||||||
| """ | ||||||||||||||||||
| Returns a copy of the image with EXIF metadata stripped. | ||||||||||||||||||
| """ | ||||||||||||||||||
| clean_image = Image.new(image.mode, image.size) | ||||||||||||||||||
| clean_image.putdata(list(image.getdata())) | ||||||||||||||||||
| return clean_image | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def validate_magic_bytes(file_content: bytes): | ||||||||||||||||||
| """ | ||||||||||||||||||
| Validates the actual file type of image | ||||||||||||||||||
| """ | ||||||||||||||||||
| fmt = imghdr.what(None, h=file_content) | ||||||||||||||||||
| if fmt not in ["jpeg", "png", "webp"]: | ||||||||||||||||||
| raise ValueError("Invalid or unsupported image type.") | ||||||||||||||||||
|
Comment on lines
+32
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Replace deprecated imghdr with a safer detector imghdr is deprecated and unreliable for some formats. You already depend on Pillow; optionally use python-magic if available. -import imghdr
+from typing import Optional
+try:
+ import magic # python-magic
+except Exception: # library may be unavailable in some environments
+ magic = None
@@
-def validate_magic_bytes(file_content: bytes):
- """
- Validates the actual file type of image
- """
- fmt = imghdr.what(None, h=file_content)
- if fmt not in ["jpeg", "png", "webp"]:
- raise ValueError("Invalid or unsupported image type.")
+def validate_magic_bytes(file_content: bytes) -> None:
+ """
+ Validate the real file type from magic bytes.
+ Prefer python-magic; fall back to Pillow sniff.
+ """
+ allowed = {"jpeg", "png", "webp"}
+ detected: Optional[str] = None
+ if magic:
+ mime = magic.from_buffer(file_content, mime=True) # e.g., "image/png"
+ if isinstance(mime, str) and mime.startswith("image/"):
+ detected = mime.split("/", 1)[1]
+ if detected is None:
+ try:
+ with Image.open(BytesIO(file_content)) as im:
+ detected = (im.format or "").lower()
+ except UnidentifiedImageError:
+ detected = None
+ if detected not in allowed:
+ raise ValueError("Invalid or unsupported image type.")
π§° Toolsπͺ Ruff (0.12.2)38-38: Avoid specifying long messages outside the exception class (TRY003) π€ Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| 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") | ||||||||||||||||||
| watermark = watermark.convert("RGBA") | ||||||||||||||||||
|
|
||||||||||||||||||
| # Resize watermark if larger than image | ||||||||||||||||||
| wm_width = min(watermark.width, int(image.width * 0.3)) | ||||||||||||||||||
| wm_height = int(watermark.height * (wm_width / watermark.width)) | ||||||||||||||||||
| watermark = watermark.resize((wm_width, wm_height), Image.Resampling.LANCZOS) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Paste watermark at bottom-right | ||||||||||||||||||
| position = (image.width - wm_width - 10, image.height - wm_height - 10) | ||||||||||||||||||
| image.alpha_composite(watermark, dest=position) | ||||||||||||||||||
| return image | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def resize_image(image: Image.Image, size: Tuple[int, int]) -> Image.Image: | ||||||||||||||||||
| """ | ||||||||||||||||||
| Resize image while maintaining aspect ratio and padding to square if needed. | ||||||||||||||||||
| """ | ||||||||||||||||||
| image.thumbnail(size, Image.Resampling.LANCZOS) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Pad to square if needed (for thumbnails) | ||||||||||||||||||
| if size[0] == size[1]: | ||||||||||||||||||
| padded = Image.new("RGB", size, (255, 255, 255)) | ||||||||||||||||||
| offset = ((size[0] - image.width) // 2, (size[1] - image.height) // 2) | ||||||||||||||||||
| padded.paste(image, offset) | ||||||||||||||||||
| return padded | ||||||||||||||||||
|
|
||||||||||||||||||
| return image | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| async def process_image(file_content: bytes) -> Dict[str, bytes]: | ||||||||||||||||||
| """ | ||||||||||||||||||
| Validates, processes, resizes, strips metadata, compresses to WebP, | ||||||||||||||||||
| and optionally watermarks the image. | ||||||||||||||||||
| Returns a dict of resized images in WebP format. | ||||||||||||||||||
| """ | ||||||||||||||||||
| try: | ||||||||||||||||||
| validate_magic_bytes(file_content) | ||||||||||||||||||
|
|
||||||||||||||||||
| img = Image.open(BytesIO(file_content)) | ||||||||||||||||||
| img_format = img.format.upper() | ||||||||||||||||||
|
|
||||||||||||||||||
| # 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.") | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from typing import Dict, Optional | ||
|
|
||
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class ImageUploadResponse(BaseModel): | ||
| success: bool | ||
| urls: Dict[str, str] # {"thumbnail": "url", "medium": "url", "full": "url"} | ||
| message: str | ||
| processing_id: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π οΈ 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.
Also applies to: 38-47
π€ Prompt for AI Agents