From 409c53e26f1e23640a9dcbffd9d61585aeac2187 Mon Sep 17 00:00:00 2001 From: Gosku Date: Wed, 5 Aug 2026 22:14:36 +1000 Subject: [PATCH 1/6] Report per-file outcomes from the folder import use case An image whose EXIF cannot produce a valid recipe raised InvalidFujifilmRecipeData out of process_image, which nothing on the sequential ingest path caught. A single such file aborted the whole folder run in the lite/SQLite install. Record those files as skipped alongside the ones carrying no Fujifilm metadata, and return a FolderImportSummary so the management command can report processed and skipped counts instead of only the total found. Co-Authored-By: Claude Opus 5 --- .../usecases/images/process_images.py | 57 +++++++-- src/domain/images/events.py | 5 + .../management/commands/process_images.py | 17 ++- tests/functional/test_process_images.py | 52 ++++++++- .../application/images/test_process_images.py | 108 ++++++++++++++++++ 5 files changed, 219 insertions(+), 20 deletions(-) create mode 100644 tests/unit/application/images/test_process_images.py diff --git a/src/application/usecases/images/process_images.py b/src/application/usecases/images/process_images.py index b38949b..71e2dca 100644 --- a/src/application/usecases/images/process_images.py +++ b/src/application/usecases/images/process_images.py @@ -1,19 +1,32 @@ +import attrs from django.conf import settings from src.domain.images import events, operations, queries +from src.domain.recipes import validation as recipe_validation from src.services import workertasks -def import_images_from_folder(*, folder: str) -> int: +@attrs.frozen +class FolderImportSummary: """ - Process all JPG images in *folder*, dispatching async or sync based on settings. + Counts produced by an :func:`import_images_from_folder` run. + + ``skipped`` holds the files that cannot produce a recipe. It is always empty + in async mode, where each file's outcome is only known inside the worker. + """ + + total: int + processed: int = 0 + skipped: tuple[str, ...] = () - Returns the total number of images found. + +def import_images_from_folder(*, folder: str) -> FolderImportSummary: + """ + Process all JPG images in *folder*, dispatching async or sync based on settings. """ if settings.USE_ASYNC_TASKS: - return _enqueue_images_in_folder(folder=folder) - total, _ = _process_images_in_folder(folder=folder) - return total + return FolderImportSummary(total=_enqueue_images_in_folder(folder=folder)) + return _process_images_in_folder(folder=folder) def _enqueue_images_in_folder(*, folder: str) -> int: @@ -33,18 +46,38 @@ def _enqueue_images_in_folder(*, folder: str) -> int: return len(paths) -def _process_images_in_folder(*, folder: str) -> tuple[int, list[str]]: +def _process_images_in_folder(*, folder: str) -> FolderImportSummary: """ - Process all JPG images in *folder* sequentially, skipping those without Fujifilm metadata. + Process all JPG images in *folder* sequentially, one file at a time. - Returns: - A tuple of (total_found, skipped_paths). + A file that carries no Fujifilm metadata, or whose EXIF cannot produce a + valid recipe, is recorded as skipped so it never aborts the rest of the run. """ paths = queries.collect_image_paths(folder=folder) - skipped = [] + processed = 0 + skipped: list[str] = [] for path in paths: try: operations.process_image(image_path=path) except operations.NoFilmSimulationError: skipped.append(path) - return len(paths), skipped + events.publish_event( + event_type=events.IMAGE_IMPORT_SKIPPED, + image_path=path, + reason=events.SKIP_REASON_NO_FILM_SIMULATION, + ) + except recipe_validation.InvalidFujifilmRecipeData as exc: + skipped.append(path) + events.publish_event( + event_type=events.IMAGE_IMPORT_SKIPPED, + image_path=path, + reason=events.SKIP_REASON_INVALID_RECIPE_DATA, + recipe_field=exc.field, + ) + else: + processed += 1 + return FolderImportSummary( + total=len(paths), + processed=processed, + skipped=tuple(skipped), + ) diff --git a/src/domain/images/events.py b/src/domain/images/events.py index 62510e4..2b4d730 100644 --- a/src/domain/images/events.py +++ b/src/domain/images/events.py @@ -22,10 +22,15 @@ IMAGE_RATING_SET = "image.rating.set" IMAGE_RATING_FAILED = "image.rating.failed" IMAGE_DEDUP_FILE_MISSING = "image.dedup.file.missing" +IMAGE_IMPORT_SKIPPED = "image.import.skipped" TASK_IMAGE_ENQUEUED = "task.image.enqueued" TASK_IMAGE_STARTED = "task.image.started" TASK_IMAGE_COMPLETED = "task.image.completed" +# Values carried on the `reason` field of IMAGE_IMPORT_SKIPPED. +SKIP_REASON_NO_FILM_SIMULATION = "no_film_simulation" +SKIP_REASON_INVALID_RECIPE_DATA = "invalid_recipe_data" + def publish_event(*, event_type: str, **kwargs: object) -> None: """ diff --git a/src/interfaces/management/commands/process_images.py b/src/interfaces/management/commands/process_images.py index 02e2429..692ee24 100644 --- a/src/interfaces/management/commands/process_images.py +++ b/src/interfaces/management/commands/process_images.py @@ -16,9 +16,18 @@ def handle(self, *args: object, **options: Any) -> None: folder = options["folder"] self.stdout.write(f"Scanning {folder} for JPG files…") - total = process_images.import_images_from_folder(folder=folder) + summary = process_images.import_images_from_folder(folder=folder) if settings.USE_ASYNC_TASKS: - self.stdout.write(self.style.SUCCESS(f"Successfully enqueued {total} tasks.")) - else: - self.stdout.write(self.style.SUCCESS(f"Successfully processed {total} images.")) + self.stdout.write(self.style.SUCCESS(f"Successfully enqueued {summary.total} tasks.")) + return + + self.stdout.write(self.style.SUCCESS( + f"Successfully processed {summary.processed} of {summary.total} images." + )) + if summary.skipped: + self.stdout.write(f"Skipped {len(summary.skipped)} image(s) that cannot produce a recipe.") + # The full list is behind -v 2 so a large skip run does not swamp the output. + if options["verbosity"] >= 2: + for path in summary.skipped: + self.stdout.write(f" skipped: {path}") diff --git a/tests/functional/test_process_images.py b/tests/functional/test_process_images.py index b8fd6dc..43b44d8 100644 --- a/tests/functional/test_process_images.py +++ b/tests/functional/test_process_images.py @@ -1,3 +1,4 @@ +import shutil from pathlib import Path import pytest @@ -7,13 +8,17 @@ from src.data import models from src.domain.images import events -FIXTURES_DIR = str(Path(__file__).resolve().parent.parent / "fixtures" / "images") +FIXTURES_DIR = Path(__file__).resolve().parent.parent / "fixtures" / "images" +# The camera set this file's Saturation, so its EXIF cannot produce a valid recipe. +CAMERA_CONTROLLED_FIXTURE = ( + Path(__file__).resolve().parent.parent / "fixtures" / "recipe" / "film_simulation_eterna.jpg" +) @pytest.mark.django_db class TestProcessImagesCommand: def test_processes_all_images_in_folder(self, capsys, captured_logs): - call_command("process_images", FIXTURES_DIR) + call_command("process_images", str(FIXTURES_DIR)) assert models.Image.objects.count() == 6 @@ -36,8 +41,47 @@ def test_processes_all_images_in_folder(self, capsys, captured_logs): class TestProcessImagesCommandSync: def test_processes_images_sequentially(self, capsys): with override_settings(USE_ASYNC_TASKS=False): - call_command("process_images", FIXTURES_DIR) + call_command("process_images", str(FIXTURES_DIR)) assert models.Image.objects.count() == 6 captured = capsys.readouterr() - assert "Successfully processed 7 images." in captured.out + assert "Successfully processed 6 of 7 images." in captured.out + + +@pytest.mark.django_db +class TestProcessImagesCommandSkips: + def test_reports_skipped_images_without_aborting_the_run(self, capsys, tmp_path): + shutil.copy(FIXTURES_DIR / "XS107114.JPG", tmp_path / "good.jpg") + shutil.copy(CAMERA_CONTROLLED_FIXTURE, tmp_path / "camera_controlled.jpg") + + with override_settings(USE_ASYNC_TASKS=False): + call_command("process_images", str(tmp_path)) + + # The valid image is still imported: the unusable one must not abort the run. + assert models.Image.objects.count() == 1 + captured = capsys.readouterr() + assert "Successfully processed 1 of 2 images." in captured.out + assert "Skipped 1 image(s) that cannot produce a recipe." in captured.out + assert "camera_controlled.jpg" not in captured.out + + def test_lists_skipped_paths_at_higher_verbosity(self, capsys, tmp_path): + shutil.copy(CAMERA_CONTROLLED_FIXTURE, tmp_path / "camera_controlled.jpg") + + with override_settings(USE_ASYNC_TASKS=False): + call_command("process_images", str(tmp_path), verbosity=2) + + assert models.Image.objects.count() == 0 + captured = capsys.readouterr() + assert "skipped: " in captured.out + assert "camera_controlled.jpg" in captured.out + + def test_publishes_an_import_skipped_event(self, tmp_path, captured_logs): + shutil.copy(CAMERA_CONTROLLED_FIXTURE, tmp_path / "camera_controlled.jpg") + + with override_settings(USE_ASYNC_TASKS=False): + call_command("process_images", str(tmp_path)) + + skipped = [e for e in captured_logs if e.get("event_type") == events.IMAGE_IMPORT_SKIPPED] + assert len(skipped) == 1 + assert skipped[0]["reason"] == events.SKIP_REASON_INVALID_RECIPE_DATA + assert skipped[0]["recipe_field"] == "color" diff --git a/tests/unit/application/images/test_process_images.py b/tests/unit/application/images/test_process_images.py new file mode 100644 index 0000000..9c5e797 --- /dev/null +++ b/tests/unit/application/images/test_process_images.py @@ -0,0 +1,108 @@ +from unittest.mock import patch + +from django.test import override_settings + +from src.application.usecases.images import process_images as uc +from src.domain.images import events +from src.domain.images.queries import NoFilmSimulationError +from src.domain.recipes.validation import InvalidFujifilmRecipeData + +_COLLECT = "src.application.usecases.images.process_images.queries.collect_image_paths" +_PROCESS = "src.application.usecases.images.process_images.operations.process_image" + + +def _skip_events(captured_logs: list[dict[str, object]]) -> list[dict[str, object]]: + return [e for e in captured_logs if e.get("event_type") == events.IMAGE_IMPORT_SKIPPED] + + +class TestProcessImagesInFolder: + def test_continues_after_an_image_that_fails_recipe_validation(self) -> None: + paths = ["a.jpg", "bad.jpg", "c.jpg"] + side_effect = [None, InvalidFujifilmRecipeData("color", None), None] + + with patch(_COLLECT, return_value=paths): + with patch(_PROCESS, side_effect=side_effect) as mock_process: + summary = uc._process_images_in_folder(folder="/photos") + + assert mock_process.call_count == 3 + assert summary.processed == 2 + assert summary.skipped == ("bad.jpg",) + + def test_continues_after_an_image_with_no_film_simulation(self) -> None: + paths = ["a.jpg", "canon.jpg", "c.jpg"] + side_effect = [None, NoFilmSimulationError("canon.jpg"), None] + + with patch(_COLLECT, return_value=paths): + with patch(_PROCESS, side_effect=side_effect) as mock_process: + summary = uc._process_images_in_folder(folder="/photos") + + assert mock_process.call_count == 3 + assert summary.processed == 2 + assert summary.skipped == ("canon.jpg",) + + def test_reports_processed_and_skipped_counts(self) -> None: + paths = ["a.jpg", "canon.jpg", "bad.jpg"] + side_effect = [ + None, + NoFilmSimulationError("canon.jpg"), + InvalidFujifilmRecipeData("color", None), + ] + + with patch(_COLLECT, return_value=paths): + with patch(_PROCESS, side_effect=side_effect): + summary = uc._process_images_in_folder(folder="/photos") + + assert summary.total == 3 + assert summary.processed == 1 + assert summary.skipped == ("canon.jpg", "bad.jpg") + + def test_publishes_an_import_skipped_event_per_skipped_image(self, captured_logs) -> None: + paths = ["canon.jpg", "bad.jpg"] + side_effect = [ + NoFilmSimulationError("canon.jpg"), + InvalidFujifilmRecipeData("color", None), + ] + + with patch(_COLLECT, return_value=paths): + with patch(_PROCESS, side_effect=side_effect): + uc._process_images_in_folder(folder="/photos") + + skipped = _skip_events(captured_logs) + assert len(skipped) == 2 + assert skipped[0]["image_path"] == "canon.jpg" + assert skipped[0]["reason"] == events.SKIP_REASON_NO_FILM_SIMULATION + assert skipped[1]["image_path"] == "bad.jpg" + assert skipped[1]["reason"] == events.SKIP_REASON_INVALID_RECIPE_DATA + assert skipped[1]["recipe_field"] == "color" + + def test_publishes_no_event_when_every_image_is_processed(self, captured_logs) -> None: + with patch(_COLLECT, return_value=["a.jpg", "b.jpg"]): + with patch(_PROCESS, return_value=None): + summary = uc._process_images_in_folder(folder="/photos") + + assert summary.skipped == () + assert _skip_events(captured_logs) == [] + + +class TestImportImagesFromFolder: + def test_reports_only_the_enqueued_total_in_async_mode(self) -> None: + with override_settings(USE_ASYNC_TASKS=True): + with patch(_COLLECT, return_value=["a.jpg", "b.jpg"]): + with patch("src.application.usecases.images.process_images.workertasks.enqueue_task"): + summary = uc.import_images_from_folder(folder="/photos") + + assert summary.total == 2 + assert summary.processed == 0 + assert summary.skipped == () + + def test_reports_per_file_outcomes_in_sync_mode(self) -> None: + side_effect = [None, InvalidFujifilmRecipeData("color", None)] + + with override_settings(USE_ASYNC_TASKS=False): + with patch(_COLLECT, return_value=["a.jpg", "bad.jpg"]): + with patch(_PROCESS, side_effect=side_effect): + summary = uc.import_images_from_folder(folder="/photos") + + assert summary.total == 2 + assert summary.processed == 1 + assert summary.skipped == ("bad.jpg",) From 2d0c9f7e3611fe9dde0135d11044d26e851c3f52 Mon Sep 17 00:00:00 2001 From: Gosku Date: Wed, 5 Aug 2026 22:15:34 +1000 Subject: [PATCH 2/6] Publish an event when an image import is skipped Skipped images were silent: neither the Celery task nor the sequential loop recorded why a file produced no recipe, so a skip was only visible as a missing row. Publish image.import.skipped with the reason, and the offending recipe field when the EXIF failed validation. On the async path this is the only place a skip surfaces, since the management command returns before any worker has run. Co-Authored-By: Claude Opus 5 --- src/interfaces/tasks.py | 14 +++++ tests/functional/test_process_image_task.py | 64 +++++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 tests/functional/test_process_image_task.py diff --git a/src/interfaces/tasks.py b/src/interfaces/tasks.py index b85db14..3666de6 100644 --- a/src/interfaces/tasks.py +++ b/src/interfaces/tasks.py @@ -7,6 +7,7 @@ from src.application.usecases.library.process_synced_image import process_synced_image from src.domain.images import events, operations from src.domain.images.thumbnails import operations as thumbnail_operations +from src.domain.recipes import validation as recipe_validation @shared_task(name="domain.process_image", bind=True, queue=settings.PROCESS_IMAGE_QUEUE) @@ -22,7 +23,20 @@ def process_image_task(self: Any, /, *, image_path: str, **kwargs: object) -> st try: recipe = operations.process_image(image_path=image_path) except operations.NoFilmSimulationError: + events.publish_event( + event_type=events.IMAGE_IMPORT_SKIPPED, + image_path=image_path, + reason=events.SKIP_REASON_NO_FILM_SIMULATION, + ) return f"Skipped {image_path} (no film simulation)" + except recipe_validation.InvalidFujifilmRecipeData as exc: + events.publish_event( + event_type=events.IMAGE_IMPORT_SKIPPED, + image_path=image_path, + reason=events.SKIP_REASON_INVALID_RECIPE_DATA, + recipe_field=exc.field, + ) + return f"Skipped {image_path} (invalid recipe data)" events.publish_event( event_type=events.TASK_IMAGE_COMPLETED, image_path=image_path, diff --git a/tests/functional/test_process_image_task.py b/tests/functional/test_process_image_task.py new file mode 100644 index 0000000..3a8b4dd --- /dev/null +++ b/tests/functional/test_process_image_task.py @@ -0,0 +1,64 @@ +import shutil +from pathlib import Path + +import pytest + +from src.data import models +from src.domain.images import events +from src.interfaces.tasks import process_image_task + +FIXTURES_DIR = Path(__file__).resolve().parent.parent / "fixtures" / "images" +FUJIFILM_FIXTURE = FIXTURES_DIR / "XS107114.JPG" +NON_FUJIFILM_FIXTURE = FIXTURES_DIR / "sub-folder" / "img_4968_dng_embedded.jpg" +# The camera set this file's Saturation, so its EXIF cannot produce a valid recipe. +CAMERA_CONTROLLED_FIXTURE = ( + Path(__file__).resolve().parent.parent / "fixtures" / "recipe" / "film_simulation_eterna.jpg" +) + + +def _skip_events(captured_logs): + return [e for e in captured_logs if e.get("event_type") == events.IMAGE_IMPORT_SKIPPED] + + +def _run_task(fixture: Path, tmp_path: Path) -> str: + image_path = tmp_path / fixture.name + shutil.copy(fixture, image_path) + return process_image_task.apply(kwargs={"image_path": str(image_path)}).get() + + +@pytest.mark.django_db +class TestProcessImageTask: + def test_processes_a_valid_image(self, tmp_path): + result = _run_task(FUJIFILM_FIXTURE, tmp_path) + + assert "Processed" in result + assert models.Image.objects.count() == 1 + + def test_skips_an_image_whose_recipe_data_is_invalid(self, tmp_path): + result = _run_task(CAMERA_CONTROLLED_FIXTURE, tmp_path) + + assert "Skipped" in result + assert "invalid recipe data" in result + assert models.Image.objects.count() == 0 + + def test_skips_an_image_with_no_film_simulation(self, tmp_path): + result = _run_task(NON_FUJIFILM_FIXTURE, tmp_path) + + assert "Skipped" in result + assert "no film simulation" in result + assert models.Image.objects.count() == 0 + + def test_publishes_an_import_skipped_event_for_invalid_recipe_data(self, tmp_path, captured_logs): + _run_task(CAMERA_CONTROLLED_FIXTURE, tmp_path) + + skipped = _skip_events(captured_logs) + assert len(skipped) == 1 + assert skipped[0]["reason"] == events.SKIP_REASON_INVALID_RECIPE_DATA + assert skipped[0]["recipe_field"] == "color" + + def test_publishes_an_import_skipped_event_for_no_film_simulation(self, tmp_path, captured_logs): + _run_task(NON_FUJIFILM_FIXTURE, tmp_path) + + skipped = _skip_events(captured_logs) + assert len(skipped) == 1 + assert skipped[0]["reason"] == events.SKIP_REASON_NO_FILM_SIMULATION From f1760091429dee61921111790e287b94ccb70788 Mon Sep 17 00:00:00 2001 From: Gosku Date: Wed, 5 Aug 2026 22:16:32 +1000 Subject: [PATCH 3/6] Skip images that fail recipe validation during library sync A file whose EXIF cannot produce a valid recipe fell through to the catch-all branch, so a sync run reported it as an error and logged a traceback for an outcome that is expected and harmless. Count it as skipped instead, next to the files carrying no Fujifilm metadata, leaving the error counter for genuine failures. Co-Authored-By: Claude Opus 5 --- .../usecases/library/process_synced_image.py | 24 ++++++++++++--- .../library/test_process_synced_image.py | 30 +++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/application/usecases/library/process_synced_image.py b/src/application/usecases/library/process_synced_image.py index 0531e8a..397c39d 100644 --- a/src/application/usecases/library/process_synced_image.py +++ b/src/application/usecases/library/process_synced_image.py @@ -1,9 +1,11 @@ import structlog +from src.domain.images import events as image_events from src.domain.images import operations as image_operations from src.domain.images.queries import NoFilmSimulationError from src.domain.library import operations as library_operations from src.domain.library import queries as library_queries +from src.domain.recipes import validation as recipe_validation logger = structlog.get_logger("application.library.process_synced_image") @@ -13,10 +15,11 @@ def process_synced_image(*, image_path: str, sync_run_id: int) -> None: Process a single image as part of sync run *sync_run_id* and record progress. Composes the pure image-processing operation with sync-run bookkeeping so the - processing operation stays unaware of sync: non-Fujifilm files count as - skipped, unexpected failures are logged and counted as errors (the run - continues), and successful imports count as processed. When every image in the - run has a terminal outcome, the run is completed. + processing operation stays unaware of sync: files that cannot produce a recipe + count as skipped, whether because they carry no Fujifilm metadata or because + their EXIF fails recipe validation; unexpected failures are logged and counted + as errors (the run continues); and successful imports count as processed. When + every image in the run has a terminal outcome, the run is completed. If the run no longer exists (its folder was removed while this work was queued), the call returns without processing. @@ -30,6 +33,19 @@ def process_synced_image(*, image_path: str, sync_run_id: int) -> None: image_operations.process_image(image_path=image_path) except NoFilmSimulationError: run.record_skipped() + image_events.publish_event( + event_type=image_events.IMAGE_IMPORT_SKIPPED, + image_path=image_path, + reason=image_events.SKIP_REASON_NO_FILM_SIMULATION, + ) + except recipe_validation.InvalidFujifilmRecipeData as exc: + run.record_skipped() + image_events.publish_event( + event_type=image_events.IMAGE_IMPORT_SKIPPED, + image_path=image_path, + reason=image_events.SKIP_REASON_INVALID_RECIPE_DATA, + recipe_field=exc.field, + ) except Exception: logger.exception("Failed to process image during sync") run.record_error() diff --git a/tests/integration/application/library/test_process_synced_image.py b/tests/integration/application/library/test_process_synced_image.py index 638eefe..62d1ef0 100644 --- a/tests/integration/application/library/test_process_synced_image.py +++ b/tests/integration/application/library/test_process_synced_image.py @@ -6,11 +6,17 @@ from src.application.usecases.library.process_synced_image import process_synced_image from src.data import models +from src.domain.images import events from tests.factories import SyncRunFactory FIXTURES_DIR = Path(__file__).resolve().parent.parent.parent.parent / "fixtures" / "images" FUJIFILM_FIXTURE = FIXTURES_DIR / "XS107114.JPG" NON_FUJIFILM_FIXTURE = FIXTURES_DIR / "sub-folder" / "img_4968_dng_embedded.jpg" +# The camera set this file's Saturation, so its EXIF cannot produce a valid recipe. +CAMERA_CONTROLLED_FIXTURE = ( + Path(__file__).resolve().parent.parent.parent.parent + / "fixtures" / "recipe" / "film_simulation_eterna.jpg" +) @pytest.mark.django_db @@ -39,6 +45,30 @@ def test_records_skipped_for_non_fujifilm_image(self, tmp_path): assert run.skipped == 1 assert run.processed == 0 + def test_records_skipped_for_an_image_that_fails_recipe_validation(self, tmp_path): + image_path = tmp_path / CAMERA_CONTROLLED_FIXTURE.name + shutil.copy(CAMERA_CONTROLLED_FIXTURE, image_path) + run = SyncRunFactory(state=models.SyncRun.STATE_PROCESSING, total=1) + + process_synced_image(image_path=str(image_path), sync_run_id=run.pk) + + run.refresh_from_db() + assert run.skipped == 1 + assert run.errors == 0 + assert run.processed == 0 + + def test_publishes_an_import_skipped_event_for_invalid_recipe_data(self, tmp_path, captured_logs): + image_path = tmp_path / CAMERA_CONTROLLED_FIXTURE.name + shutil.copy(CAMERA_CONTROLLED_FIXTURE, image_path) + run = SyncRunFactory(state=models.SyncRun.STATE_PROCESSING, total=1) + + process_synced_image(image_path=str(image_path), sync_run_id=run.pk) + + skipped = [e for e in captured_logs if e.get("event_type") == events.IMAGE_IMPORT_SKIPPED] + assert len(skipped) == 1 + assert skipped[0]["reason"] == events.SKIP_REASON_INVALID_RECIPE_DATA + assert skipped[0]["recipe_field"] == "color" + def test_records_error_and_continues_on_unexpected_failure(self): run = SyncRunFactory(state=models.SyncRun.STATE_PROCESSING, total=1) From 508f18eff17c6af0d33b1f2ef467797f3310349d Mon Sep 17 00:00:00 2001 From: Gosku Date: Wed, 5 Aug 2026 22:19:15 +1000 Subject: [PATCH 4/6] Skip uploaded files that fail recipe validation An uploaded JPG whose EXIF cannot produce a valid recipe raised InvalidFujifilmRecipeData past the use case and into the import view, returning a 500 instead of telling the user which file was rejected. Record it in ImportRecipesResult.failed alongside the non-Fujifilm files, so the whole upload is reported rather than aborted. Co-Authored-By: Claude Opus 5 --- .../import_recipes_from_uploaded_files.py | 8 +++-- ...test_import_recipes_from_uploaded_files.py | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/application/usecases/recipes/import_recipes_from_uploaded_files.py b/src/application/usecases/recipes/import_recipes_from_uploaded_files.py index 4d5b3f4..112fa44 100644 --- a/src/application/usecases/recipes/import_recipes_from_uploaded_files.py +++ b/src/application/usecases/recipes/import_recipes_from_uploaded_files.py @@ -5,6 +5,7 @@ from src.domain.images.queries import NoFilmSimulationError from src.domain.recipes import dataclasses as recipe_dataclasses from src.domain.recipes import operations +from src.domain.recipes import validation as recipe_validation def import_recipes_from_uploaded_files( @@ -17,8 +18,9 @@ def import_recipes_from_uploaded_files( the recipe is extracted, and the temporary file is deleted immediately afterwards — whether the operation succeeds or fails. - Files that do not contain Fujifilm recipe EXIF data are recorded as - failures; processing continues with the remaining files. + Files that do not contain Fujifilm recipe EXIF data, and files whose EXIF + cannot produce a valid recipe, are recorded as failures; processing + continues with the remaining files. Returns an ImportRecipesResult describing which files were imported and which failed. @@ -34,7 +36,7 @@ def import_recipes_from_uploaded_files( tmp_path = tmp.name recipe, _ = operations.get_or_create_recipe_from_filepath(filepath=tmp_path) imported.append(recipe) - except NoFilmSimulationError: + except (NoFilmSimulationError, recipe_validation.InvalidFujifilmRecipeData): failed.append(file.name) finally: if tmp_path is not None: diff --git a/tests/integration/application/recipes/test_import_recipes_from_uploaded_files.py b/tests/integration/application/recipes/test_import_recipes_from_uploaded_files.py index 98f348e..b72af67 100644 --- a/tests/integration/application/recipes/test_import_recipes_from_uploaded_files.py +++ b/tests/integration/application/recipes/test_import_recipes_from_uploaded_files.py @@ -9,6 +9,11 @@ from src.domain.recipes.dataclasses import ImportRecipesResult, UploadedFile FIXTURES_DIR = Path(__file__).resolve().parent.parent.parent.parent / "fixtures" / "images" +# The camera set this file's Saturation, so its EXIF cannot produce a valid recipe. +CAMERA_CONTROLLED_FIXTURE = ( + Path(__file__).resolve().parent.parent.parent.parent + / "fixtures" / "recipe" / "film_simulation_eterna.jpg" +) def uploaded_file_from_fixture(filename: str) -> UploadedFile: @@ -62,6 +67,33 @@ def test_records_failure_for_non_fujifilm_file(self): assert result.imported == () assert result.failed == ("canon.jpg",) + def test_records_failure_for_a_file_that_fails_recipe_validation(self): + files = [ + UploadedFile( + name="camera_controlled.jpg", + content=CAMERA_CONTROLLED_FIXTURE.read_bytes(), + ) + ] + + result = import_recipes_from_uploaded_files(files=files) + + assert result.imported == () + assert result.failed == ("camera_controlled.jpg",) + + def test_continues_after_a_file_that_fails_recipe_validation(self): + files = [ + UploadedFile( + name="camera_controlled.jpg", + content=CAMERA_CONTROLLED_FIXTURE.read_bytes(), + ), + uploaded_file_from_fixture("XS107114.JPG"), + ] + + result = import_recipes_from_uploaded_files(files=files) + + assert len(result.imported) == 1 + assert result.failed == ("camera_controlled.jpg",) + def test_continues_after_failure_and_processes_remaining_files(self): non_fujifilm = UploadedFile(name="bad.jpg", content=b"\xff\xd8\xff\xd9") files = [ From 1a01224dba79951add6030f0840534e736f50522 Mon Sep 17 00:00:00 2001 From: Gosku Date: Wed, 5 Aug 2026 22:20:45 +1000 Subject: [PATCH 5/6] Document that process_image rejects invalid recipe data The ingest entry points could raise InvalidFujifilmRecipeData without saying so, which is how the exception went unhandled on every path. Record it in the docstrings and cover it directly, including that the surrounding transaction leaves no FujifilmExif row behind when a recipe is rejected. Co-Authored-By: Claude Opus 5 --- src/domain/images/operations.py | 2 ++ src/domain/recipes/operations.py | 4 +++ .../domain/images/test_operations.py | 28 +++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/src/domain/images/operations.py b/src/domain/images/operations.py index a183c0a..b5d5aa0 100644 --- a/src/domain/images/operations.py +++ b/src/domain/images/operations.py @@ -126,6 +126,8 @@ def process_image(*, image_path: str) -> models.Image: Raises: NoFilmSimulationError: If the image has no film simulation EXIF data. + InvalidFujifilmRecipeData: If the image's EXIF cannot produce a valid + recipe. Nothing is persisted: the whole call is one transaction. """ metadata = queries.read_image_exif(image_path=image_path) diff --git a/src/domain/recipes/operations.py b/src/domain/recipes/operations.py index 8a4276d..fd31052 100644 --- a/src/domain/recipes/operations.py +++ b/src/domain/recipes/operations.py @@ -387,6 +387,9 @@ def get_or_create_recipe_from_metadata( Create or retrieve a FujifilmRecipe for the given parsed EXIF data. :raises NoFilmSimulationError: If the EXIF data contains no known film simulation. + :raises InvalidFujifilmRecipeData: If the EXIF data cannot produce a valid + recipe, e.g. a colour simulation whose Color the camera set rather than + the user, leaving no value to store. """ try: recipe_data = image_queries.exif_to_recipe(exif=metadata) @@ -402,6 +405,7 @@ def get_or_create_recipe_from_filepath( Read EXIF from *filepath* and return the matching FujifilmRecipe, creating it if needed. :raises NoFilmSimulationError: If the file is not a Fujifilm image or has no film simulation. + :raises InvalidFujifilmRecipeData: If the file's EXIF cannot produce a valid recipe. """ metadata = image_queries.read_image_exif(image_path=filepath) if metadata.camera_make.upper() != "FUJIFILM": diff --git a/tests/integration/domain/images/test_operations.py b/tests/integration/domain/images/test_operations.py index 469dd54..35c2cbf 100644 --- a/tests/integration/domain/images/test_operations.py +++ b/tests/integration/domain/images/test_operations.py @@ -9,6 +9,7 @@ from src.domain.images import events from src.domain.images.dataclasses import ImageExifData from src.domain.images.operations import NoFilmSimulationError, process_image +from src.domain.recipes.validation import InvalidFujifilmRecipeData FIXTURES_DIR = Path(__file__).resolve().parent.parent.parent.parent / "fixtures" / "images" FIXTURE_IMAGE = str(FIXTURES_DIR / "XS107114.JPG") @@ -185,3 +186,30 @@ def test_raises_when_film_simulation_and_color_are_empty(self): process_image(image_path="any/path.jpg") assert models.Image.objects.count() == 0 + + +@pytest.mark.django_db +class TestProcessImageInvalidRecipeData: + def test_raises_for_an_image_whose_color_the_camera_set(self): + """The camera writes the sentinel 'Film Simulation' into Color when it, not + the user, drives saturation. A colour simulation with no Color value cannot + produce a valid recipe.""" + image_path = str(RECIPE_FIXTURES_DIR / "film_simulation_eterna.jpg") + + with pytest.raises(InvalidFujifilmRecipeData) as exc_info: + process_image(image_path=image_path) + + assert exc_info.value.field == "color" + assert exc_info.value.value is None + + def test_leaves_no_partial_rows_behind(self): + """process_image is one transaction, so a rejected recipe must not leave the + FujifilmExif row it creates before validation runs.""" + image_path = str(RECIPE_FIXTURES_DIR / "film_simulation_eterna.jpg") + + with pytest.raises(InvalidFujifilmRecipeData): + process_image(image_path=image_path) + + assert models.Image.objects.count() == 0 + assert models.FujifilmExif.objects.count() == 0 + assert models.FujifilmRecipe.objects.count() == 0 From 48ac917420f633e1781b8059e815a3d5d63769d0 Mon Sep 17 00:00:00 2001 From: Gosku Date: Wed, 5 Aug 2026 22:21:39 +1000 Subject: [PATCH 6/6] Correct the EXIF mapping docs for camera-controlled values The Color section claimed the recipe stores "N/A" for a non-numeric value, which is what Sharpness does; Color returns None. It also framed the sentinel as a property of certain film simulations, when the same simulations carry numeric values on almost every shot. Describe what the value actually means, and document how the resulting skips are reported. Co-Authored-By: Claude Opus 5 --- docs/exif_mapping.md | 12 +++++++++--- docs/management_commands.md | 7 +++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/docs/exif_mapping.md b/docs/exif_mapping.md index d62ac8c..28e4596 100644 --- a/docs/exif_mapping.md +++ b/docs/exif_mapping.md @@ -102,9 +102,14 @@ if color is a numeric label (e.g. "0 (normal)", "+2 (high)"): → saturation adjustment; decode using table below elif color is a non-numeric string (e.g. "Acros", "None (B&W)", "Film Simulation"): - → film simulation name or special case; recipe returns "N/A" for the Color field + → film simulation name or special case; recipe returns None for the Color field ``` +Note that Color returns `None`, not the string `"N/A"` that Sharpness uses for its +equivalent case. For a monochromatic simulation that absence is correct and the +recipe is stored. For a colour simulation it is not: a colour recipe with no Color +value fails `validate_recipe_data`, and the image is skipped at ingest. + ### Numeric saturation mapping table | Camera display value | EXIF stored value | Recipe output | @@ -136,8 +141,9 @@ elif color is a non-numeric string (e.g. "Acros", "None (B&W)", "Film Simulation ### Key observations -- `Film Simulation` appears for some film simulations (e.g. Eterna, Astia, Pro Neg. Std); it means the film profile controls saturation internally and the user cannot override it. -- For B&W/Acros/Sepia simulations the `Film Mode` EXIF field is absent; `Saturation` encodes the simulation name instead. In those cases the Color recipe field is `"N/A"` — there is no separate saturation adjustment. +- `Film Simulation` means the camera, not the user, drove saturation for that shot. It is not a property of the film simulation: it has been observed on Eterna, Astia and Pro Neg. Std images, but the same simulations carry numeric values on the great majority of shots. Nothing else in the EXIF flags the condition, so this value is the only signal. +- A colour simulation whose `Saturation` is `Film Simulation` records no recipe of the user's, so the image is skipped at ingest rather than stored with an empty Color. See [management_commands.md](management_commands.md) for how skips are reported. +- For B&W/Acros/Sepia simulations the `Film Mode` EXIF field is absent; `Saturation` encodes the simulation name instead. In those cases the Color recipe field is `None` — there is no separate saturation adjustment, and the recipe is stored normally. - Recipe output for numeric values is a signed integer string: `"-2"`, `"+3"`, `"0"`. --- diff --git a/docs/management_commands.md b/docs/management_commands.md index cd4b284..a521846 100644 --- a/docs/management_commands.md +++ b/docs/management_commands.md @@ -63,6 +63,13 @@ Behaviour depends on your install mode: - **Full install** (`USE_ASYNC_TASKS=True`): one Celery task is enqueued per image and processed in parallel by the worker (start it first with `make worker`). +Files that cannot produce a recipe are skipped, never aborting the run. That covers +images carrying no Fujifilm metadata, and images whose EXIF fails recipe validation, +such as a colour simulation whose Color the camera set rather than the user. In lite +mode the command reports how many were skipped; add `--verbosity 2` to list their +paths. In full install mode each skip is recorded as an `image.import.skipped` event +in the worker log, carrying the reason and the offending recipe field. + --- ## Rating images in bulk