Conversation
There was a problem hiding this comment.
Pull request overview
Adds a main_picture field to the asset list response so clients can identify an asset’s designated main image.
Changes:
- Extend
AssetResponseschema to includemain_picture: Optional[int]. - Populate
main_pictureinlist_assets_for_club()from the asset’s pictures markedis_main.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
asset_management/app/assets/services.py |
Adds logic to compute main_picture during asset list mapping. |
asset_management/app/assets/schemas.py |
Adds the main_picture field to the asset response schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| location=asset.location, | ||
| created_at=asset.created_at, | ||
| max_rental_days=asset.max_rental_days, | ||
| main_picture=next((p.id for p in asset.pictures if p.is_main), None), |
There was a problem hiding this comment.
asset.pictures access here will lazily load the full Picture rows for each asset (including the BLOB data column) and will also do it per-asset, creating a large N+1 query + memory cost on the list endpoint. Prefer fetching only the main picture id via a single query (e.g., join/correlated subquery) or eager-load a restricted set of columns (e.g., selectinload(...).load_only(Picture.id, Picture.is_main)) so listing assets doesn’t pull image binaries into memory.
| main_picture=next((p.id for p in asset.pictures if p.is_main), None), | |
| main_picture=getattr(asset, "main_picture_id", None), |
| location: Optional[str] = None | ||
| created_at: datetime | ||
| max_rental_days: Optional[int] = None | ||
| main_picture: Optional[int] = None |
There was a problem hiding this comment.
This PR adds a new response field (main_picture) but there’s no test asserting it’s present/correct when a main picture exists. Consider extending the existing asset list API test to upload a picture, mark it as main, then verify main_picture equals that picture id (and is null when no main picture is set).
2 participants
No description provided.