From 65f6a4b18b5b9654740da7726d9c3c6f1284f85f Mon Sep 17 00:00:00 2001 From: AdrianoDev Date: Mon, 21 Sep 2026 22:43:35 +0200 Subject: [PATCH] feat(vision): immagini di riferimento non orfane, GET/DELETE (#38.5) save_reference_image scrive il file dopo il flush riuscito; il router lo cancella se il commit successivo fallisce. Aggiunti GET /api/vision/reference-images?task_id= (lista, Maker) e DELETE /api/vision/reference-images/{id} (cancella riga e file, Maker). Co-Authored-By: Claude Sonnet 5 --- src/backend/api/routers/vision.py | 37 ++++++- src/backend/models/api/vision.py | 17 +++- src/backend/services/vision_service.py | 64 +++++++++++- src/backend/tests/test_vision_reference.py | 107 +++++++++++++++++++++ 4 files changed, 217 insertions(+), 8 deletions(-) diff --git a/src/backend/api/routers/vision.py b/src/backend/api/routers/vision.py index 433c54f..0e8bb92 100644 --- a/src/backend/api/routers/vision.py +++ b/src/backend/api/routers/vision.py @@ -12,7 +12,9 @@ from src.backend.api.middleware.api_key import require_maker, require_measuremen from src.backend.api.routers.files import ALLOWED_IMAGE_TYPES, validate_file_size from src.backend.config import settings from src.backend.database import get_db -from src.backend.models.api.vision import VisionExecuteResponse, VisionMeasurementResult +from src.backend.models.api.vision import ( + VisionExecuteResponse, VisionMeasurementResult, VisionReferenceImageResponse, +) from src.backend.models.orm.task import RecipeTask from src.backend.models.orm.user import User from src.backend.services import vision_service @@ -128,7 +130,7 @@ async def execute( # Composing a recipe and keeping a reference image are Maker operations, not -# measuring: both endpoints below gate on `require_maker`, not the +# measuring: every endpoint below gates on `require_maker`, not the # `require_measurement_tec` that `execute` above uses. @@ -146,10 +148,39 @@ async def add_reference_image( db, task.id, content, note=note, extension=_IMAGE_EXTENSIONS[image.content_type], ) - await db.commit() + try: + await db.commit() + except Exception: + # The row never became durable - the file `save_reference_image` + # wrote after its flush would otherwise outlive it as an orphan. + vision_service.remove_reference_file(reference) + raise return {"id": reference.id, "path": reference.path, "note": reference.note} +@router.get("/reference-images", response_model=list[VisionReferenceImageResponse]) +async def get_reference_images( + task_id: int, + db: AsyncSession = Depends(get_db), + current_user: User = Depends(require_maker), +) -> list[VisionReferenceImageResponse]: + await _task_or_404(db, task_id) + return await vision_service.list_reference_images(db, task_id) + + +@router.delete("/reference-images/{reference_id}", status_code=204) +async def delete_reference_image( + reference_id: int, + db: AsyncSession = Depends(get_db), + current_user: User = Depends(require_maker), +) -> None: + reference = await vision_service.delete_reference_image(db, reference_id) + await db.commit() + # Only now: if the commit above had failed, the row would still exist and + # the file must still be there for it. + vision_service.remove_reference_file(reference) + + @router.post("/preview") async def preview( task_id: int = Form(...), diff --git a/src/backend/models/api/vision.py b/src/backend/models/api/vision.py index 6180b5e..4a8f466 100644 --- a/src/backend/models/api/vision.py +++ b/src/backend/models/api/vision.py @@ -1,5 +1,7 @@ """Pydantic schemas for the vision execution response.""" -from pydantic import BaseModel +from datetime import datetime + +from pydantic import BaseModel, ConfigDict class VisionMeasurementResult(BaseModel): @@ -14,3 +16,16 @@ class VisionExecuteResponse(BaseModel): vision_result_id: int engine_version: str measurements: list[VisionMeasurementResult] + + +class VisionReferenceImageResponse(BaseModel): + """One saved reference image, as `GET /api/vision/reference-images` lists it.""" + model_config = ConfigDict(from_attributes=True) + + id: int + task_id: int + path: str + note: str | None = None + station_id: int | None = None + device_code: str | None = None + acquired_at: datetime diff --git a/src/backend/services/vision_service.py b/src/backend/services/vision_service.py index 3682bd3..3166633 100644 --- a/src/backend/services/vision_service.py +++ b/src/backend/services/vision_service.py @@ -260,6 +260,21 @@ async def execute_task( return result, saved +def _reference_file_path(reference: VisionReferenceImage) -> Path: + return settings.upload_path / reference.path + + +def remove_reference_file(reference: VisionReferenceImage) -> None: + """Delete a reference image's file, if it is still there. + + Called by the router only once the row's fate is durable: after a + successful commit when the row was just deleted, or to undo a + `save_reference_image` whose row failed to commit (see both call sites). + `missing_ok` because "already gone" must never be an error here. + """ + _reference_file_path(reference).unlink(missing_ok=True) + + async def save_reference_image( db: AsyncSession, task_id: int, @@ -276,17 +291,22 @@ async def save_reference_image( `extension` must match the upload's real content type (the router derives it from `image.content_type`) - writing a jpeg to disk as `.png` would be a lie the filename tells forever. + + The file is written only after the row has flushed successfully - a bad + FK or constraint fails at `flush()`, before anything exists on disk, so + there is nothing to clean up. The caller still has to remove the file if + the *commit* that follows fails (`remove_reference_file`, from the + router): a flush is not durable, and an orphaned file from a rolled-back + row is exactly what #38 found. """ # `upload_path` is the resolved Path, `upload_dir` the bare setting: the # rest of the codebase uses the former (see production_export_service). - folder = settings.upload_path / "vision" / "reference" / str(task_id) - folder.mkdir(parents=True, exist_ok=True) name = f"{uuid4().hex}.{extension}" - (folder / name).write_bytes(image_bytes) + relative_path = str(Path("vision") / "reference" / str(task_id) / name) reference = VisionReferenceImage( task_id=task_id, - path=str(Path("vision") / "reference" / str(task_id) / name), + path=relative_path, note=note, station_id=station_id, device_code=device_code, @@ -294,10 +314,46 @@ async def save_reference_image( ) db.add(reference) await db.flush() + + folder = _reference_file_path(reference).parent + folder.mkdir(parents=True, exist_ok=True) + _reference_file_path(reference).write_bytes(image_bytes) + await db.refresh(reference) return reference +async def list_reference_images( + db: AsyncSession, task_id: int, +) -> list[VisionReferenceImage]: + result = await db.execute( + select(VisionReferenceImage) + .where(VisionReferenceImage.task_id == task_id) + .order_by(VisionReferenceImage.acquired_at) + ) + return list(result.scalars().all()) + + +async def delete_reference_image( + db: AsyncSession, reference_id: int, +) -> VisionReferenceImage: + """Flush the row's deletion. The caller (router) removes the file only + once the commit that makes the deletion durable has actually succeeded - + deleting it any earlier would leave the row pointing at a file that is + already gone if the commit then failed. + """ + reference = (await db.execute( + select(VisionReferenceImage).where(VisionReferenceImage.id == reference_id) + )).scalar_one_or_none() + if reference is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail="Reference image not found", + ) + await db.delete(reference) + await db.flush() + return reference + + async def preview(db: AsyncSession, task: RecipeTask, image_bytes: bytes) -> dict: """Run the graph and save nothing. Composing is not measuring.""" if not task.vision_json: diff --git a/src/backend/tests/test_vision_reference.py b/src/backend/tests/test_vision_reference.py index e9a1993..0342fd2 100644 --- a/src/backend/tests/test_vision_reference.py +++ b/src/backend/tests/test_vision_reference.py @@ -85,6 +85,113 @@ async def test_a_reference_image_keeps_its_provenance( assert saved.path +@pytest.mark.asyncio +async def test_the_file_survives_a_failed_commit_as_nothing( + client, db_session, maker_user, monkeypatch, +): + """#38.5: the file is written only after a successful flush, and removed + if the commit that follows still fails - never an orphan on disk.""" + recipe = await create_test_recipe(db_session, maker_user.id) + version, task, subtask = await recipe_parts(db_session, recipe.id) + await db_session.commit() + + original_commit = db_session.commit + + async def boom(*args, **kwargs): + raise RuntimeError("simulated commit failure") + monkeypatch.setattr(db_session, "commit", boom) + + try: + # The ASGI transport re-raises an unhandled app exception rather + # than turning it into a response - the same way it would surface + # in production logs. What matters here is what is left on disk. + with pytest.raises(RuntimeError, match="simulated commit failure"): + await client.post( + "/api/vision/reference-images", + headers=auth_headers(maker_user), + files={"image": ("ref.png", an_image(), "image/png")}, + data={"task_id": str(task.id)}, + ) + finally: + # The db_session fixture commits again on its own teardown - restore + # the real commit so that does not also blow up. + monkeypatch.setattr(db_session, "commit", original_commit) + + folder = settings.upload_path / "vision" / "reference" / str(task.id) + assert not folder.exists() or list(folder.iterdir()) == [] + + +@pytest.mark.asyncio +async def test_reference_images_are_listed_for_their_task( + client, db_session, maker_user, +): + recipe = await create_test_recipe(db_session, maker_user.id) + version, task, subtask = await recipe_parts(db_session, recipe.id) + await db_session.commit() + + for _ in range(2): + response = await client.post( + "/api/vision/reference-images", + headers=auth_headers(maker_user), + files={"image": ("ref.png", an_image(), "image/png")}, + data={"task_id": str(task.id)}, + ) + assert response.status_code == 200 + + listing = await client.get( + "/api/vision/reference-images", + headers=auth_headers(maker_user), + params={"task_id": task.id}, + ) + + assert listing.status_code == 200 + body = listing.json() + assert len(body) == 2 + assert {row["task_id"] for row in body} == {task.id} + + +@pytest.mark.asyncio +async def test_deleting_a_reference_image_removes_the_row_and_the_file( + client, db_session, maker_user, +): + recipe = await create_test_recipe(db_session, maker_user.id) + version, task, subtask = await recipe_parts(db_session, recipe.id) + await db_session.commit() + + created = await client.post( + "/api/vision/reference-images", + headers=auth_headers(maker_user), + files={"image": ("ref.png", an_image(), "image/png")}, + data={"task_id": str(task.id)}, + ) + reference_id = created.json()["id"] + file_path = settings.upload_path / created.json()["path"] + assert file_path.exists() + + response = await client.delete( + f"/api/vision/reference-images/{reference_id}", + headers=auth_headers(maker_user), + ) + + assert response.status_code == 204 + assert (await db_session.execute( + select(VisionReferenceImage).where(VisionReferenceImage.id == reference_id) + )).scalar_one_or_none() is None + assert not file_path.exists() + + +@pytest.mark.asyncio +async def test_deleting_an_unknown_reference_image_is_404( + client, db_session, maker_user, +): + response = await client.delete( + "/api/vision/reference-images/999999", + headers=auth_headers(maker_user), + ) + + assert response.status_code == 404 + + @pytest.mark.asyncio async def test_preview_runs_the_graph_and_saves_no_measurement( client, db_session, maker_user, worker_says,