From cbd665fca612051f7fbe076f4f65d7b7556453db Mon Sep 17 00:00:00 2001 From: Mateo Date: Sun, 26 Apr 2026 16:08:46 +0200 Subject: [PATCH 1/9] feat: support optional crop image upload on detection create Adds a nullable crop_bucket_key column to detections (model + migration), extends POST /detections/ to accept an optional `crop` file alongside the main frame, and exposes the resulting presigned URL as `crop_url` on GET /detections/{id}/url. Cameras that do not send a crop continue to work unchanged. Also updates pyroclient.create_detection to accept an optional `crop` argument and aligns update_last_image / update_pose_image content types with the JPEG payloads actually sent. --- client/pyroclient/client.py | 11 +++++--- src/app/api/api_v1/endpoints/detections.py | 16 +++++------ src/app/models.py | 1 + src/app/schemas/detections.py | 2 ++ ...a9b3e_add_crop_bucket_key_to_detections.py | 27 +++++++++++++++++++ 5 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py diff --git a/client/pyroclient/client.py b/client/pyroclient/client.py index 6fc3fb4d..478333ab 100644 --- a/client/pyroclient/client.py +++ b/client/pyroclient/client.py @@ -153,7 +153,7 @@ def update_last_image(self, media: bytes) -> Response: return requests.patch( urljoin(self._route_prefix, ClientRoute.CAMERAS_IMAGE), headers=self.headers, - files={"file": ("logo.png", media, "image/png")}, + files={"file": ("logo.jpg", media, "image/jpeg")}, timeout=self.timeout, ) @@ -245,7 +245,7 @@ def update_pose_image(self, pose_id: int, media: bytes) -> Response: return requests.patch( urljoin(self._route_prefix, ClientRoute.POSES_IMAGE.format(pose_id=pose_id)), headers=self.headers, - files={"file": ("image.png", media, "image/png")}, + files={"file": ("image.jpg", media, "image/jpeg")}, timeout=self.timeout, ) @@ -344,6 +344,7 @@ def create_detection( media: bytes, bboxes: List[Tuple[float, float, float, float, float]], pose_id: int, + crop: bytes | None = None, ) -> Response: """Notify the detection of a wildfire on the picture taken by a camera. @@ -356,6 +357,7 @@ def create_detection( media: byte data of the picture bboxes: list of tuples where each tuple is a relative coordinate in order xmin, ymin, xmax, ymax, conf pose_id: pose_id of the detection + crop: optional byte data of a cropped picture associated with the detection Returns: HTTP response @@ -366,12 +368,15 @@ def create_detection( "bboxes": _dump_bbox_to_json(bboxes), } data["pose_id"] = str(pose_id) + files: Dict[str, Tuple[str, bytes, str]] = {"file": ("frame.jpg", media, "image/jpeg")} + if crop is not None: + files["crop"] = ("crop.jpg", crop, "image/jpeg") return requests.post( urljoin(self._route_prefix, ClientRoute.DETECTIONS_CREATE), headers=self.headers, data=data, timeout=self.timeout, - files={"file": ("logo.png", media, "image/png")}, + files=files, ) def get_detection_url(self, detection_id: int) -> Response: diff --git a/src/app/api/api_v1/endpoints/detections.py b/src/app/api/api_v1/endpoints/detections.py index c6345c64..f33619b8 100644 --- a/src/app/api/api_v1/endpoints/detections.py +++ b/src/app/api/api_v1/endpoints/detections.py @@ -353,6 +353,7 @@ async def create_detection( ), pose_id: int = Form(..., gt=0, description="pose id of the detection"), file: UploadFile = File(..., alias="file"), + crop_file: Optional[UploadFile] = File(None, alias="crop"), detections: DetectionCRUD = Depends(get_detection_crud), webhooks: WebhookCRUD = Depends(get_webhook_crud), organizations: OrganizationCRUD = Depends(get_organization_crud), @@ -373,6 +374,9 @@ async def create_detection( # Upload media bucket_key = await upload_file(file, token_payload.organization_id, token_payload.sub) + crop_bucket_key: Optional[str] = None + if crop_file is not None: + crop_bucket_key = await upload_file(crop_file, token_payload.organization_id, token_payload.sub) pose = cast(Pose, await poses.get(pose_id, strict=True)) if pose.camera_id != token_payload.sub: raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access forbidden.") @@ -393,6 +397,7 @@ async def create_detection( camera_id=token_payload.sub, pose_id=pose_id, bucket_key=bucket_key, + crop_bucket_key=crop_bucket_key, bbox=single_bboxes, others_bboxes=others_bboxes, ) @@ -529,17 +534,12 @@ async def get_detection_url( # Check in DB detection = cast(Detection, await detections.get(detection_id, strict=True)) - if UserRole.ADMIN in token_payload.scopes: - camera = cast(Camera, await cameras.get(detection.camera_id, strict=True)) - bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(camera.organization_id)) - return DetectionUrl(url=bucket.get_public_url(detection.bucket_key)) - camera = cast(Camera, await cameras.get(detection.camera_id, strict=True)) - if token_payload.organization_id != camera.organization_id: + if UserRole.ADMIN not in token_payload.scopes and token_payload.organization_id != camera.organization_id: raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access forbidden.") - # Check in bucket bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(camera.organization_id)) - return DetectionUrl(url=bucket.get_public_url(detection.bucket_key)) + crop_url = bucket.get_public_url(detection.crop_bucket_key) if detection.crop_bucket_key else None + return DetectionUrl(url=bucket.get_public_url(detection.bucket_key), crop_url=crop_url) @router.get("/", status_code=status.HTTP_200_OK, summary="Fetch all the detections") diff --git a/src/app/models.py b/src/app/models.py index c3b30b3c..6dbc07e4 100644 --- a/src/app/models.py +++ b/src/app/models.py @@ -86,6 +86,7 @@ class Detection(SQLModel, table=True): pose_id: int = Field(..., foreign_key="poses.id", nullable=False) sequence_id: Union[int, None] = Field(None, foreign_key="sequences.id", nullable=True) bucket_key: str + crop_bucket_key: Union[str, None] = Field(default=None, nullable=True) bbox: str = Field(..., min_length=2, max_length=settings.MAX_BBOX_STR_LENGTH_SINGLE, nullable=False) others_bboxes: Union[str, None] = Field(default=None, max_length=settings.MAX_BBOX_STR_LENGTH_OTHERS, nullable=True) created_at: datetime = Field(default_factory=datetime.utcnow, nullable=False) diff --git a/src/app/schemas/detections.py b/src/app/schemas/detections.py index f85f7c71..57242596 100644 --- a/src/app/schemas/detections.py +++ b/src/app/schemas/detections.py @@ -29,6 +29,7 @@ class DetectionCreate(BaseModel): camera_id: int = Field(..., gt=0) pose_id: int = Field(..., gt=0) bucket_key: str + crop_bucket_key: Optional[str] = None bbox: str = Field( ..., min_length=2, @@ -41,6 +42,7 @@ class DetectionCreate(BaseModel): class DetectionUrl(BaseModel): url: str = Field(..., description="temporary URL to access the media content") + crop_url: Optional[str] = Field(None, description="temporary URL to access the cropped media content, if any") class DetectionRead(Detection): diff --git a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py new file mode 100644 index 00000000..54019a0d --- /dev/null +++ b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py @@ -0,0 +1,27 @@ +"""Add crop_bucket_key to detections + +Revision ID: 7f1c4d2a9b3e +Revises: 307a1d6d490d +Create Date: 2026-04-26 12:00:00.000000 + +""" + +from typing import Sequence, Union + +import sqlalchemy as sa +import sqlmodel +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "7f1c4d2a9b3e" +down_revision: Union[str, None] = "307a1d6d490d" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column("detections", sa.Column("crop_bucket_key", sqlmodel.sql.sqltypes.AutoString(), nullable=True)) + + +def downgrade() -> None: + op.drop_column("detections", "crop_bucket_key") From a1eda3363fcc91708c8cae68ec44fd024ae4d6a4 Mon Sep 17 00:00:00 2001 From: Mateo Date: Sun, 26 Apr 2026 16:13:37 +0200 Subject: [PATCH 2/9] fix(migration): chain crop_bucket_key onto the new alembic baseline PR #572 collapses prior migrations into a single baseline revision 9700bbccb2f1. Repoint this migration's down_revision so it applies cleanly on top of the new baseline. --- ..._26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py index 54019a0d..1e480da2 100644 --- a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py +++ b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py @@ -1,7 +1,7 @@ """Add crop_bucket_key to detections Revision ID: 7f1c4d2a9b3e -Revises: 307a1d6d490d +Revises: 9700bbccb2f1 Create Date: 2026-04-26 12:00:00.000000 """ @@ -14,7 +14,7 @@ # revision identifiers, used by Alembic. revision: str = "7f1c4d2a9b3e" -down_revision: Union[str, None] = "307a1d6d490d" +down_revision: Union[str, None] = "9700bbccb2f1" branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None From b2e7433c27807d8f4118589e98f86c4e98855314 Mon Sep 17 00:00:00 2001 From: Mateo Date: Sun, 26 Apr 2026 16:18:49 +0200 Subject: [PATCH 3/9] fix(detections): authorize pose before uploading & cover crop flow with tests Move the pose ownership check ahead of any S3 upload so a forbidden request with another camera's pose_id no longer leaves the main image (or, after this branch, the optional crop) orphaned in the bucket. Add tests covering the new crop behavior: - crop upload persists crop_bucket_key on the detection, - omitting crop leaves crop_bucket_key null, - GET /detections/{id}/url returns crop_url when a crop is present and null otherwise, - a 403 from a foreign pose_id never invokes upload_file (verified via monkeypatched stub). DET_TABLE in conftest gains an explicit crop_bucket_key=None so the existing detection JSON-equality assertions still match. --- src/app/api/api_v1/endpoints/detections.py | 12 +- src/tests/conftest.py | 4 + src/tests/endpoints/test_detections.py | 128 +++++++++++++++++++++ 3 files changed, 139 insertions(+), 5 deletions(-) diff --git a/src/app/api/api_v1/endpoints/detections.py b/src/app/api/api_v1/endpoints/detections.py index f33619b8..cb409cbf 100644 --- a/src/app/api/api_v1/endpoints/detections.py +++ b/src/app/api/api_v1/endpoints/detections.py @@ -372,11 +372,7 @@ async def create_detection( detail="xmin & ymin are expected to be respectively smaller than xmax & ymax", ) - # Upload media - bucket_key = await upload_file(file, token_payload.organization_id, token_payload.sub) - crop_bucket_key: Optional[str] = None - if crop_file is not None: - crop_bucket_key = await upload_file(crop_file, token_payload.organization_id, token_payload.sub) + # Authorize before any S3 upload to avoid orphan objects on 403 pose = cast(Pose, await poses.get(pose_id, strict=True)) if pose.camera_id != token_payload.sub: raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access forbidden.") @@ -385,6 +381,12 @@ async def create_detection( if not bbox_strings: raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="Invalid bbox format.") + # Upload media + bucket_key = await upload_file(file, token_payload.organization_id, token_payload.sub) + crop_bucket_key: Optional[str] = None + if crop_file is not None: + crop_bucket_key = await upload_file(crop_file, token_payload.organization_id, token_payload.sub) + created: List[Detection] = [] camera = cast(Camera, await cameras.get(token_payload.sub, strict=True)) diff --git a/src/tests/conftest.py b/src/tests/conftest.py index 6359915d..7763f041 100644 --- a/src/tests/conftest.py +++ b/src/tests/conftest.py @@ -149,6 +149,7 @@ "pose_id": 1, "sequence_id": 1, "bucket_key": "my_file", + "crop_bucket_key": None, "bbox": "[(.1,.1,.7,.8,.9)]", "others_bboxes": None, "created_at": datetime.strptime("2023-11-07T15:08:19.226673", dt_format), @@ -159,6 +160,7 @@ "pose_id": 1, "sequence_id": 1, "bucket_key": "my_file", + "crop_bucket_key": None, "bbox": "[(.1,.1,.7,.8,.9)]", "others_bboxes": None, "created_at": datetime.strptime("2023-11-07T15:18:19.226673", dt_format), @@ -169,6 +171,7 @@ "pose_id": 1, "sequence_id": 1, "bucket_key": "my_file", + "crop_bucket_key": None, "bbox": "[(.1,.1,.7,.8,.9)]", "others_bboxes": None, "created_at": datetime.strptime("2023-11-07T15:28:19.226673", dt_format), @@ -179,6 +182,7 @@ "pose_id": 3, "sequence_id": 2, "bucket_key": "my_file", + "crop_bucket_key": None, "bbox": "[(.1,.1,.7,.8,.9)]", "others_bboxes": None, "created_at": datetime.strptime("2023-11-07T16:08:19.226673", dt_format), diff --git a/src/tests/endpoints/test_detections.py b/src/tests/endpoints/test_detections.py index e153b653..de02b696 100644 --- a/src/tests/endpoints/test_detections.py +++ b/src/tests/endpoints/test_detections.py @@ -1357,3 +1357,131 @@ async def test_attach_sequence_does_not_bridge_to_distant_alert(detection_sessio mappings_res = await detection_session.exec(select(AlertSequence).where(AlertSequence.alert_id == smoke_a_alert_id)) seqs_in_a = {m.sequence_id for m in mappings_res.all()} assert seq_cam2.id not in seqs_in_a + + +@pytest.mark.asyncio +async def test_create_detection_persists_crop_bucket_key( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes +): + auth = pytest.get_token( + pytest.camera_table[1]["id"], + ["camera"], + pytest.camera_table[1]["organization_id"], + ) + payload = {"pose_id": 3, "bboxes": "[(0.6,0.6,0.7,0.7,0.6)]"} + response = await async_client.post( + "/detections", + data=payload, + files={ + "file": ("frame.jpg", mock_img, "image/jpeg"), + "crop": ("crop.jpg", mock_img, "image/jpeg"), + }, + headers=auth, + ) + assert response.status_code == 201, response.text + data = response.json() + assert isinstance(data["crop_bucket_key"], str) + assert data["crop_bucket_key"] + assert data["crop_bucket_key"] != data["bucket_key"] + + det = await detection_session.get(Detection, data["id"]) + assert det is not None + assert det.crop_bucket_key == data["crop_bucket_key"] + + +@pytest.mark.asyncio +async def test_create_detection_without_crop_leaves_field_null( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes +): + auth = pytest.get_token( + pytest.camera_table[1]["id"], + ["camera"], + pytest.camera_table[1]["organization_id"], + ) + payload = {"pose_id": 3, "bboxes": "[(0.6,0.6,0.7,0.7,0.6)]"} + response = await async_client.post( + "/detections", + data=payload, + files={"file": ("frame.jpg", mock_img, "image/jpeg")}, + headers=auth, + ) + assert response.status_code == 201, response.text + assert response.json()["crop_bucket_key"] is None + + +@pytest.mark.asyncio +async def test_get_detection_url_returns_crop_url( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes +): + from app.services.storage import s3_service + + detection = await detection_session.get(Detection, pytest.detection_table[0]["id"]) + assert detection is not None + bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(pytest.camera_table[0]["organization_id"])) + crop_key = "crop_for_url_test.jpg" + bucket.upload_file(crop_key, io.BytesIO(mock_img)) + detection.crop_bucket_key = crop_key + detection_session.add(detection) + await detection_session.commit() + + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + response = await async_client.get(f"/detections/{detection.id}/url", headers=auth) + assert response.status_code == 200, response.text + body = response.json() + assert isinstance(body["url"], str) and body["url"].startswith("http://") + assert isinstance(body["crop_url"], str) and body["crop_url"].startswith("http://") + assert body["crop_url"] != body["url"] + + bucket.delete_file(crop_key) + + +@pytest.mark.asyncio +async def test_get_detection_url_crop_url_null_without_crop( + async_client: AsyncClient, detection_session: AsyncSession +): + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + detection_id = pytest.detection_table[0]["id"] + response = await async_client.get(f"/detections/{detection_id}/url", headers=auth) + assert response.status_code == 200, response.text + assert response.json()["crop_url"] is None + + +@pytest.mark.asyncio +async def test_create_detection_authorizes_pose_before_uploading( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes, monkeypatch +): + upload_calls: List[str] = [] + + async def fake_upload_file(file: UploadFile, organization_id: int, camera_id: int) -> str: + upload_calls.append(file.filename or "") + return "should-never-persist" + + monkeypatch.setattr(detections_api, "upload_file", fake_upload_file) + + # Camera 1's token paired with pose 3 (owned by camera 2) must 403 before any upload. + auth = pytest.get_token( + pytest.camera_table[0]["id"], + ["camera"], + pytest.camera_table[0]["organization_id"], + ) + payload = {"pose_id": 3, "bboxes": "[(0.6,0.6,0.7,0.7,0.6)]"} + response = await async_client.post( + "/detections", + data=payload, + files={ + "file": ("frame.jpg", mock_img, "image/jpeg"), + "crop": ("crop.jpg", mock_img, "image/jpeg"), + }, + headers=auth, + ) + assert response.status_code == 403, response.text + assert response.json()["detail"] == "Access forbidden." + assert upload_calls == [] From 240bb7deac0e93daee2c9b2a9d9298f8e4db343b Mon Sep 17 00:00:00 2001 From: Mateo Date: Sun, 26 Apr 2026 16:58:22 +0200 Subject: [PATCH 4/9] feat(sequences): expose crop_url on /sequences/{id}/detections The platform consumes detections through GET /sequences/{id}/detections (not the per-detection /url endpoint), so the crop_url addition needs to land on that response too. Extend DetectionWithUrl with an optional crop_url and populate it from crop_bucket_key when present. Add a with_crop query parameter (default true) so callers that don't need crops can skip the extra S3 head requests that get_public_url performs. Mirror the parameter on pyroclient.fetch_sequences_detections. Tests: assert crop_url is presigned when a crop exists, that with_crop= false suppresses it, and that the existing equality check now also strips crop_url alongside url. --- client/pyroclient/client.py | 11 +++- src/app/api/api_v1/endpoints/sequences.py | 18 ++++-- src/app/schemas/detections.py | 1 + src/tests/endpoints/test_sequences.py | 69 ++++++++++++++++++++++- 4 files changed, 90 insertions(+), 9 deletions(-) diff --git a/client/pyroclient/client.py b/client/pyroclient/client.py index 478333ab..d7621dea 100644 --- a/client/pyroclient/client.py +++ b/client/pyroclient/client.py @@ -474,7 +474,13 @@ def fetch_latest_sequences(self) -> Response: timeout=self.timeout, ) - def fetch_sequences_detections(self, sequence_id: int, limit: int = 10, desc: bool = True) -> Response: + def fetch_sequences_detections( + self, + sequence_id: int, + limit: int = 10, + desc: bool = True, + with_crop: bool = True, + ) -> Response: """List the detections of a sequence >>> from pyroclient import client @@ -485,6 +491,7 @@ def fetch_sequences_detections(self, sequence_id: int, limit: int = 10, desc: bo sequence_id: ID of the associated sequence entry limit: maximum number of detections to fetch desc: whether to order the detections by created_at in descending order + with_crop: whether to include the crop_url for detections that have a crop Returns: HTTP response @@ -492,7 +499,7 @@ def fetch_sequences_detections(self, sequence_id: int, limit: int = 10, desc: bo return requests.get( urljoin(self._route_prefix, ClientRoute.SEQUENCES_FETCH_DETECTIONS.format(seq_id=sequence_id)), headers=self.headers, - params={"limit": limit, "desc": desc}, + params={"limit": limit, "desc": desc, "with_crop": with_crop}, timeout=self.timeout, ) diff --git a/src/app/api/api_v1/endpoints/sequences.py b/src/app/api/api_v1/endpoints/sequences.py index be1c87ba..7358174a 100644 --- a/src/app/api/api_v1/endpoints/sequences.py +++ b/src/app/api/api_v1/endpoints/sequences.py @@ -105,6 +105,10 @@ async def fetch_sequence_detections( sequence_id: int = Path(..., gt=0), limit: int = Query(10, description="Maximum number of detections to fetch", ge=1, le=100), desc: bool = Query(True, description="Whether to order the detections by created_at in descending order"), + with_crop: bool = Query( + True, + description="If true, presign and include crop_url for detections that have a crop. Set to false to skip the extra S3 head requests when crops are not needed.", + ), cameras: CameraCRUD = Depends(get_camera_crud), detections: DetectionCRUD = Depends(get_detection_crud), sequences: SequenceCRUD = Depends(get_sequence_crud), @@ -118,17 +122,19 @@ async def fetch_sequence_detections( # Get the bucket of the camera's organization bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(camera.organization_id)) + fetched = await detections.fetch_all( + filters=("sequence_id", sequence_id), + order_by="created_at", + order_desc=desc, + limit=limit, + ) return [ DetectionWithUrl( **DetectionRead(**elt.model_dump()).model_dump(), url=bucket.get_public_url(elt.bucket_key), + crop_url=(bucket.get_public_url(elt.crop_bucket_key) if with_crop and elt.crop_bucket_key else None), ) - for elt in await detections.fetch_all( - filters=("sequence_id", sequence_id), - order_by="created_at", - order_desc=desc, - limit=limit, - ) + for elt in fetched ] diff --git a/src/app/schemas/detections.py b/src/app/schemas/detections.py index 57242596..3c58dc5e 100644 --- a/src/app/schemas/detections.py +++ b/src/app/schemas/detections.py @@ -51,6 +51,7 @@ class DetectionRead(Detection): class DetectionWithUrl(Detection): url: str = Field(..., description="temporary URL to access the media content") + crop_url: Optional[str] = Field(None, description="temporary URL to access the cropped media content, if any") class DetectionSequence(BaseModel): diff --git a/src/tests/endpoints/test_sequences.py b/src/tests/endpoints/test_sequences.py index 7569f247..5dbc7975 100644 --- a/src/tests/endpoints/test_sequences.py +++ b/src/tests/endpoints/test_sequences.py @@ -50,8 +50,11 @@ async def test_fetch_sequence_detections( if isinstance(status_detail, str): assert response.json()["detail"] == status_detail if response.status_code // 100 == 2 and expected_result is not None: - assert [{k: v for k, v in det.items() if k != "url"} for det in response.json()] == expected_result + assert [ + {k: v for k, v in det.items() if k not in {"url", "crop_url"}} for det in response.json() + ] == expected_result assert all(det["url"].startswith("http://") for det in response.json()) + assert all(det["crop_url"] is None for det in response.json()) @pytest.mark.parametrize( @@ -589,3 +592,67 @@ async def test_unit_label_sequence_forbidden_for_wrong_org(): ) assert exc_info.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_fetch_sequence_detections_includes_crop_url( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes +): + from app.services.storage import s3_service + + detection = await detection_session.get(Detection, pytest.detection_table[0]["id"]) + assert detection is not None + bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(pytest.camera_table[0]["organization_id"])) + crop_key = "crop_for_sequence_test.jpg" + bucket.upload_file(crop_key, __import__("io").BytesIO(mock_img)) + detection.crop_bucket_key = crop_key + detection_session.add(detection) + await detection_session.commit() + + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + sequence_id = pytest.detection_table[0]["sequence_id"] + response = await async_client.get(f"/sequences/{sequence_id}/detections", headers=auth) + assert response.status_code == 200, response.text + payload = response.json() + enriched = next(det for det in payload if det["id"] == detection.id) + assert isinstance(enriched["crop_url"], str) + assert enriched["crop_url"].startswith("http://") + assert enriched["crop_url"] != enriched["url"] + other = [det for det in payload if det["id"] != detection.id] + assert all(det["crop_url"] is None for det in other) + + bucket.delete_file(crop_key) + + +@pytest.mark.asyncio +async def test_fetch_sequence_detections_with_crop_false_skips_crop_url( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes +): + from app.services.storage import s3_service + + detection = await detection_session.get(Detection, pytest.detection_table[0]["id"]) + assert detection is not None + bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(pytest.camera_table[0]["organization_id"])) + crop_key = "crop_for_sequence_off_test.jpg" + bucket.upload_file(crop_key, __import__("io").BytesIO(mock_img)) + detection.crop_bucket_key = crop_key + detection_session.add(detection) + await detection_session.commit() + + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + sequence_id = pytest.detection_table[0]["sequence_id"] + response = await async_client.get( + f"/sequences/{sequence_id}/detections", params={"with_crop": "false"}, headers=auth + ) + assert response.status_code == 200, response.text + assert all(det["crop_url"] is None for det in response.json()) + + bucket.delete_file(crop_key) From 5dccbc623dd92493975ccca44ca3dbc5837331fe Mon Sep 17 00:00:00 2001 From: Mateo Date: Sun, 26 Apr 2026 17:02:42 +0200 Subject: [PATCH 5/9] fix(detections): prevent crop/frame bucket-key collision on identical bytes upload_file derives keys from camera_id, second-precision timestamp, and the SHA-256 prefix of the bytes. When a crop happens to be byte-identical to the frame (e.g. a full-frame crop sent in the same request), both calls produced the same key, the second upload overwrote the first, and crop_url ended up pointing at the frame. Add an optional key_prefix parameter to upload_file and call the crop upload with key_prefix="crop_". Regression test asserts the two keys remain distinct when the bytes match. --- src/app/api/api_v1/endpoints/detections.py | 4 +++- src/app/services/storage.py | 9 +++++--- src/tests/endpoints/test_detections.py | 26 ++++++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/app/api/api_v1/endpoints/detections.py b/src/app/api/api_v1/endpoints/detections.py index cb409cbf..0649fa75 100644 --- a/src/app/api/api_v1/endpoints/detections.py +++ b/src/app/api/api_v1/endpoints/detections.py @@ -385,7 +385,9 @@ async def create_detection( bucket_key = await upload_file(file, token_payload.organization_id, token_payload.sub) crop_bucket_key: Optional[str] = None if crop_file is not None: - crop_bucket_key = await upload_file(crop_file, token_payload.organization_id, token_payload.sub) + crop_bucket_key = await upload_file( + crop_file, token_payload.organization_id, token_payload.sub, key_prefix="crop_" + ) created: List[Detection] = [] camera = cast(Camera, await cameras.get(token_payload.sub, strict=True)) diff --git a/src/app/services/storage.py b/src/app/services/storage.py index 8ffa4945..82011c88 100644 --- a/src/app/services/storage.py +++ b/src/app/services/storage.py @@ -155,7 +155,9 @@ def resolve_bucket_name(organization_id: int) -> str: return f"{settings.SERVER_NAME}-alert-api-{organization_id!s}" -async def upload_file(file: UploadFile, organization_id: int, camera_id: int) -> str: +async def upload_file( + file: UploadFile, organization_id: int, camera_id: int, key_prefix: str = "" +) -> str: """Upload a file to S3 storage and return the public URL""" # Concatenate the first 8 chars (to avoid system interactions issues) of SHA256 hash with file extension sha_hash = hashlib.sha256(file.file.read()).hexdigest() @@ -165,8 +167,9 @@ async def upload_file(file: UploadFile, organization_id: int, camera_id: int) -> await file.seek(0) # guess_extension will return none if this fails extension = guess_extension(magic.from_buffer(file.file.read(), mime=True)) or "" - # Concatenate timestamp & hash - bucket_key = f"{camera_id}-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}-{sha_hash[:8]}{extension}" + # Concatenate timestamp & hash; key_prefix lets callers segregate distinct uploads in the + # same request (e.g. frame vs crop) so identical bytes don't collide on the same key. + bucket_key = f"{key_prefix}{camera_id}-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}-{sha_hash[:8]}{extension}" # Reset byte position of the file (cf. https://fastapi.tiangolo.com/tutorial/request-files/#uploadfile) await file.seek(0) bucket_name = s3_service.resolve_bucket_name(organization_id) diff --git a/src/tests/endpoints/test_detections.py b/src/tests/endpoints/test_detections.py index de02b696..1b9f6776 100644 --- a/src/tests/endpoints/test_detections.py +++ b/src/tests/endpoints/test_detections.py @@ -1454,6 +1454,32 @@ async def test_get_detection_url_crop_url_null_without_crop( assert response.json()["crop_url"] is None +@pytest.mark.asyncio +async def test_create_detection_with_identical_crop_bytes_produces_distinct_keys( + async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes +): + auth = pytest.get_token( + pytest.camera_table[1]["id"], + ["camera"], + pytest.camera_table[1]["organization_id"], + ) + payload = {"pose_id": 3, "bboxes": "[(0.6,0.6,0.7,0.7,0.6)]"} + # Same bytes for frame and crop must not collide on the same bucket key. + response = await async_client.post( + "/detections", + data=payload, + files={ + "file": ("frame.jpg", mock_img, "image/jpeg"), + "crop": ("crop.jpg", mock_img, "image/jpeg"), + }, + headers=auth, + ) + assert response.status_code == 201, response.text + data = response.json() + assert data["bucket_key"] != data["crop_bucket_key"] + assert data["crop_bucket_key"].startswith("crop_") + + @pytest.mark.asyncio async def test_create_detection_authorizes_pose_before_uploading( async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes, monkeypatch From 6facf2f780e5ab0bb594f88b9f50118bd6f23c81 Mon Sep 17 00:00:00 2001 From: Mateo Date: Sun, 26 Apr 2026 19:05:55 +0200 Subject: [PATCH 6/9] style: apply ruff formatting and split combined assertions Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/services/storage.py | 4 +--- src/tests/endpoints/test_detections.py | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/app/services/storage.py b/src/app/services/storage.py index 82011c88..f16b40af 100644 --- a/src/app/services/storage.py +++ b/src/app/services/storage.py @@ -155,9 +155,7 @@ def resolve_bucket_name(organization_id: int) -> str: return f"{settings.SERVER_NAME}-alert-api-{organization_id!s}" -async def upload_file( - file: UploadFile, organization_id: int, camera_id: int, key_prefix: str = "" -) -> str: +async def upload_file(file: UploadFile, organization_id: int, camera_id: int, key_prefix: str = "") -> str: """Upload a file to S3 storage and return the public URL""" # Concatenate the first 8 chars (to avoid system interactions issues) of SHA256 hash with file extension sha_hash = hashlib.sha256(file.file.read()).hexdigest() diff --git a/src/tests/endpoints/test_detections.py b/src/tests/endpoints/test_detections.py index 1b9f6776..101d068b 100644 --- a/src/tests/endpoints/test_detections.py +++ b/src/tests/endpoints/test_detections.py @@ -1432,17 +1432,17 @@ async def test_get_detection_url_returns_crop_url( response = await async_client.get(f"/detections/{detection.id}/url", headers=auth) assert response.status_code == 200, response.text body = response.json() - assert isinstance(body["url"], str) and body["url"].startswith("http://") - assert isinstance(body["crop_url"], str) and body["crop_url"].startswith("http://") + assert isinstance(body["url"], str) + assert body["url"].startswith("http://") + assert isinstance(body["crop_url"], str) + assert body["crop_url"].startswith("http://") assert body["crop_url"] != body["url"] bucket.delete_file(crop_key) @pytest.mark.asyncio -async def test_get_detection_url_crop_url_null_without_crop( - async_client: AsyncClient, detection_session: AsyncSession -): +async def test_get_detection_url_crop_url_null_without_crop(async_client: AsyncClient, detection_session: AsyncSession): auth = pytest.get_token( pytest.user_table[0]["id"], pytest.user_table[0]["role"].split(), @@ -1486,7 +1486,7 @@ async def test_create_detection_authorizes_pose_before_uploading( ): upload_calls: List[str] = [] - async def fake_upload_file(file: UploadFile, organization_id: int, camera_id: int) -> str: + async def fake_upload_file(file: UploadFile, organization_id: int, camera_id: int) -> str: # noqa: RUF029 upload_calls.append(file.filename or "") return "should-never-persist" From 54412b702d02d45eda6a9e3f12d753189fa0f571 Mon Sep 17 00:00:00 2001 From: Mateo Date: Mon, 27 Apr 2026 08:52:48 +0200 Subject: [PATCH 7/9] =?UTF-8?q?test(detections):=20polish=20crop=20tests?= =?UTF-8?q?=20=E2=80=94=20try/finally=20cleanup,=20hoist=20imports,=20pref?= =?UTF-8?q?ix=20assertion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.7 (1M context) --- src/tests/endpoints/test_detections.py | 44 +++++++------- src/tests/endpoints/test_sequences.py | 84 +++++++++++++------------- 2 files changed, 65 insertions(+), 63 deletions(-) diff --git a/src/tests/endpoints/test_detections.py b/src/tests/endpoints/test_detections.py index 101d068b..02c51e74 100644 --- a/src/tests/endpoints/test_detections.py +++ b/src/tests/endpoints/test_detections.py @@ -32,6 +32,7 @@ from app.models import Alert, AlertSequence, Camera, Detection, Organization, Pose, Role, Sequence, Webhook from app.schemas.login import TokenPayload from app.services.cones import resolve_cone +from app.services.storage import s3_service @pytest.mark.parametrize( @@ -877,6 +878,7 @@ async def test_create_detection_sequence_flow_direct(detection_session: AsyncSes bboxes="[(0.2,0.2,0.3,0.3,0.9)]", pose_id=pose_id, file=upload, + crop_file=None, detections=detections, webhooks=webhooks, organizations=organizations, @@ -914,6 +916,7 @@ async def fake_fetch_all(*args, **kwargs): bboxes="[(0.25,0.25,0.35,0.35,0.9)]", pose_id=pose_id, file=upload_again, + crop_file=None, detections=detections, webhooks=webhooks, organizations=organizations, @@ -1381,7 +1384,7 @@ async def test_create_detection_persists_crop_bucket_key( assert response.status_code == 201, response.text data = response.json() assert isinstance(data["crop_bucket_key"], str) - assert data["crop_bucket_key"] + assert data["crop_bucket_key"].startswith("crop_") assert data["crop_bucket_key"] != data["bucket_key"] det = await detection_session.get(Detection, data["id"]) @@ -1413,32 +1416,31 @@ async def test_create_detection_without_crop_leaves_field_null( async def test_get_detection_url_returns_crop_url( async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes ): - from app.services.storage import s3_service - detection = await detection_session.get(Detection, pytest.detection_table[0]["id"]) assert detection is not None bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(pytest.camera_table[0]["organization_id"])) crop_key = "crop_for_url_test.jpg" bucket.upload_file(crop_key, io.BytesIO(mock_img)) - detection.crop_bucket_key = crop_key - detection_session.add(detection) - await detection_session.commit() + try: + detection.crop_bucket_key = crop_key + detection_session.add(detection) + await detection_session.commit() - auth = pytest.get_token( - pytest.user_table[0]["id"], - pytest.user_table[0]["role"].split(), - pytest.user_table[0]["organization_id"], - ) - response = await async_client.get(f"/detections/{detection.id}/url", headers=auth) - assert response.status_code == 200, response.text - body = response.json() - assert isinstance(body["url"], str) - assert body["url"].startswith("http://") - assert isinstance(body["crop_url"], str) - assert body["crop_url"].startswith("http://") - assert body["crop_url"] != body["url"] - - bucket.delete_file(crop_key) + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + response = await async_client.get(f"/detections/{detection.id}/url", headers=auth) + assert response.status_code == 200, response.text + body = response.json() + assert isinstance(body["url"], str) + assert body["url"].startswith("http://") + assert isinstance(body["crop_url"], str) + assert body["crop_url"].startswith("http://") + assert body["crop_url"] != body["url"] + finally: + bucket.delete_file(crop_key) @pytest.mark.asyncio diff --git a/src/tests/endpoints/test_sequences.py b/src/tests/endpoints/test_sequences.py index 5dbc7975..896fe993 100644 --- a/src/tests/endpoints/test_sequences.py +++ b/src/tests/endpoints/test_sequences.py @@ -1,3 +1,4 @@ +import io from datetime import datetime, timedelta from typing import Any, Dict, List, Union from unittest.mock import AsyncMock, MagicMock, patch @@ -12,6 +13,7 @@ from app.models import Alert, AlertSequence, AnnotationType, Camera, Detection, Pose, Sequence, UserRole from app.schemas.login import TokenPayload from app.schemas.sequences import SequenceLabel +from app.services.storage import s3_service @pytest.mark.parametrize( @@ -598,61 +600,59 @@ async def test_unit_label_sequence_forbidden_for_wrong_org(): async def test_fetch_sequence_detections_includes_crop_url( async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes ): - from app.services.storage import s3_service - detection = await detection_session.get(Detection, pytest.detection_table[0]["id"]) assert detection is not None bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(pytest.camera_table[0]["organization_id"])) crop_key = "crop_for_sequence_test.jpg" - bucket.upload_file(crop_key, __import__("io").BytesIO(mock_img)) - detection.crop_bucket_key = crop_key - detection_session.add(detection) - await detection_session.commit() + bucket.upload_file(crop_key, io.BytesIO(mock_img)) + try: + detection.crop_bucket_key = crop_key + detection_session.add(detection) + await detection_session.commit() - auth = pytest.get_token( - pytest.user_table[0]["id"], - pytest.user_table[0]["role"].split(), - pytest.user_table[0]["organization_id"], - ) - sequence_id = pytest.detection_table[0]["sequence_id"] - response = await async_client.get(f"/sequences/{sequence_id}/detections", headers=auth) - assert response.status_code == 200, response.text - payload = response.json() - enriched = next(det for det in payload if det["id"] == detection.id) - assert isinstance(enriched["crop_url"], str) - assert enriched["crop_url"].startswith("http://") - assert enriched["crop_url"] != enriched["url"] - other = [det for det in payload if det["id"] != detection.id] - assert all(det["crop_url"] is None for det in other) - - bucket.delete_file(crop_key) + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + sequence_id = pytest.detection_table[0]["sequence_id"] + response = await async_client.get(f"/sequences/{sequence_id}/detections", headers=auth) + assert response.status_code == 200, response.text + payload = response.json() + enriched = next(det for det in payload if det["id"] == detection.id) + assert isinstance(enriched["crop_url"], str) + assert enriched["crop_url"].startswith("http://") + assert enriched["crop_url"] != enriched["url"] + other = [det for det in payload if det["id"] != detection.id] + assert all(det["crop_url"] is None for det in other) + finally: + bucket.delete_file(crop_key) @pytest.mark.asyncio async def test_fetch_sequence_detections_with_crop_false_skips_crop_url( async_client: AsyncClient, detection_session: AsyncSession, mock_img: bytes ): - from app.services.storage import s3_service - detection = await detection_session.get(Detection, pytest.detection_table[0]["id"]) assert detection is not None bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(pytest.camera_table[0]["organization_id"])) crop_key = "crop_for_sequence_off_test.jpg" - bucket.upload_file(crop_key, __import__("io").BytesIO(mock_img)) - detection.crop_bucket_key = crop_key - detection_session.add(detection) - await detection_session.commit() - - auth = pytest.get_token( - pytest.user_table[0]["id"], - pytest.user_table[0]["role"].split(), - pytest.user_table[0]["organization_id"], - ) - sequence_id = pytest.detection_table[0]["sequence_id"] - response = await async_client.get( - f"/sequences/{sequence_id}/detections", params={"with_crop": "false"}, headers=auth - ) - assert response.status_code == 200, response.text - assert all(det["crop_url"] is None for det in response.json()) + bucket.upload_file(crop_key, io.BytesIO(mock_img)) + try: + detection.crop_bucket_key = crop_key + detection_session.add(detection) + await detection_session.commit() - bucket.delete_file(crop_key) + auth = pytest.get_token( + pytest.user_table[0]["id"], + pytest.user_table[0]["role"].split(), + pytest.user_table[0]["organization_id"], + ) + sequence_id = pytest.detection_table[0]["sequence_id"] + response = await async_client.get( + f"/sequences/{sequence_id}/detections", params={"with_crop": "false"}, headers=auth + ) + assert response.status_code == 200, response.text + assert all(det["crop_url"] is None for det in response.json()) + finally: + bucket.delete_file(crop_key) From af48653c3c7c3f89fcec9f9ecf58f63f315be852 Mon Sep 17 00:00:00 2001 From: Mateo Date: Mon, 27 Apr 2026 09:01:38 +0200 Subject: [PATCH 8/9] fix(migration): point crop_bucket_key down_revision to the real head 307a1d6d490d The previous down_revision (9700bbccb2f1) does not exist in the migration history, so `alembic upgrade head` failed at backend boot and the docker healthcheck never went green. Tests pass because they use SQLModel.metadata.create_all and bypass alembic. Co-Authored-By: Claude Opus 4.7 (1M context) --- ..._26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py index 1e480da2..54019a0d 100644 --- a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py +++ b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py @@ -1,7 +1,7 @@ """Add crop_bucket_key to detections Revision ID: 7f1c4d2a9b3e -Revises: 9700bbccb2f1 +Revises: 307a1d6d490d Create Date: 2026-04-26 12:00:00.000000 """ @@ -14,7 +14,7 @@ # revision identifiers, used by Alembic. revision: str = "7f1c4d2a9b3e" -down_revision: Union[str, None] = "9700bbccb2f1" +down_revision: Union[str, None] = "307a1d6d490d" branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None From b2015f846876a19ce8d2e1e7a854e4b0acafe678 Mon Sep 17 00:00:00 2001 From: Mateo Date: Mon, 27 Apr 2026 09:03:40 +0200 Subject: [PATCH 9/9] Revert "fix(migration): point crop_bucket_key down_revision to the real head 307a1d6d490d" This reverts commit af48653c3c7c3f89fcec9f9ecf58f63f315be852. --- ..._26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py index 54019a0d..1e480da2 100644 --- a/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py +++ b/src/migrations/versions/2026_04_26_1200-7f1c4d2a9b3e_add_crop_bucket_key_to_detections.py @@ -1,7 +1,7 @@ """Add crop_bucket_key to detections Revision ID: 7f1c4d2a9b3e -Revises: 307a1d6d490d +Revises: 9700bbccb2f1 Create Date: 2026-04-26 12:00:00.000000 """ @@ -14,7 +14,7 @@ # revision identifiers, used by Alembic. revision: str = "7f1c4d2a9b3e" -down_revision: Union[str, None] = "307a1d6d490d" +down_revision: Union[str, None] = "9700bbccb2f1" branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None