Add one image per place: schema, processing and API (task 249, backend) #192

Merged

rob merged 6 commits from feature/task-249-place-images into main 2026-08-17 11:43:49 +00:00

Conversation 2 Commits 6 Files changed 40 +2989 -52

rob commented 2026-08-17 09:14:13 +00:00

Owner

Copy link

Backend half of task 249. UI (upload/replace/remove in PlaceFormPanel, thumbnail and popup viewer in PlaceDetailPanel, WebUI tests, E2E journey) follows in a second PR — this one is schema, domain, infrastructure, endpoints and their own tests, so it's reviewable on its own rather than landing as one large change.

What's here

• ADR-0163: one place_images row per place (place_id itself as the PK — a second upload replaces the first via ON CONFLICT), cascading from places. Resize/crop arithmetic lives in PlaceMark.Domain with zero packages (ImageBoundingBoxCalculator, ThumbnailCropCalculator), proved against plain integers; PlaceMark.Infrastructure.Places.PlaceImageProcessor (SkiaSharp — MIT, licence read before adding; ImageSharp's split licence was considered and rejected) applies it to real pixels, rotates upright from EXIF orientation, then re-encodes with no EXIF. Explicitly reverses ADR-0006's "no photo attachments" exclusion at Rob's own request — ADR-0006 gains Partially superseded by: 0163 for that one bullet only.
• ADR-0164: images are served from their own endpoints (GET /api/places/{id}/image, .../image/thumbnail), never inlined in the places list. PlaceResponse/PlaceForCallerResponse carry only ImageVersion (optional, defaults to null — no existing call site needed touching). Cached public, max-age=31536000, immutable, keyed by the client appending ?v={ImageVersion} rather than a conditional-GET round trip. Corrected during review: this record now says plainly that the WebUI (#194) can't put that URL in the DOM at all — the endpoints need a bearer token a plain <img src> can't carry, so it fetches via HttpClient and renders a data: URI, which the browser's own image cache never sees.
• Authorisation reuses ADR-0043: Member to view, MayEditPlaces to add/replace/delete — the same thresholds the rest of PlaceEndpoints already uses. All four image routes carry RequireGroupCapabilityForPlace, none skipped.
• docs/data-model.md gets its own place_images section and FK row; SchemaVersionHealthCheck.RequiredScript and every test pinned to "the newest script" moved to 0005-place-images.sql.

Method

Test-first throughout. The two arithmetic rules were each proved to redden against a deliberately wrong implementation (a naive squash to 240×135 for the thumbnail; the FK cascade removed outright) before being trusted. The EXIF rotation is proved against a JPEG this test suite constructs itself — a genuine, standards-compliant EXIF Orientation tag spliced onto a plain encode, not a downloaded sample.

Corrected during review, and stated precisely rather than rounded up: PlaceImageProcessorTests asserts real pixel positions after rotation for five of the eight EXIF orientation values — 2, 3, 4, 6 and 8 — each verified to redden against a deliberately wrong transform (6/8 confirmed by swapping their rotation directions). Orientation 1 is the identity transform and needs no positional proof; every other test in the class exercises it implicitly. Orientations 5 and 7 have only the EXIF-stripped theory tests (Process_AnImageCarryingExifOrientation_StripsAllExifFromTheFullImage/...Thumbnail) and no positional proof of their own rotation — disclosed, not silently missing: both are mirrored-and-rotated values a consumer phone or camera essentially never produces, unlike 6 and 8, the two ordinary portrait-hold rotations every phone photograph actually carries, which now do carry positional proof. All positional and stripping assertions run against both outputs; the rotation direction was verified empirically rather than only derived on paper.

The endpoint tests found a real bug this way: minimal API's automatic antiforgery requirement for IFormFile-binding endpoints 500ed until .DisableAntiforgery() was added, which is correct here since this API has no cookie session for antiforgery to protect.

Judgement calls worth flagging

• ImageVersion defaults to null rather than being a required positional member, so this doesn't touch PlaceFormPanel.razor.cs or any WebUI test's existing positional construction — deliberately, to keep this PR backend-only and avoid colliding with the parallel agent on task 245.
• SkiaSharp over ImageSharp: capability was never the deciding factor, the licence was — ImageSharp's Split License is conditional on this project's own licence/revenue staying under a threshold; SkiaSharp's MIT terms aren't conditional on anything. Independently confirmed MIT via NuGet and the upstream LICENSE.md; present only in PlaceMark.Infrastructure.
• A 100-megapixel decoded-dimension ceiling, checked from the codec header before a bitmap is allocated, independent of the 10 MB raw upload cap — a defence against a small file claiming an enormous pixel grid, proved with a patched SOF0 marker rather than a giant bitmap.

Tests

Rebased twice, onto main after #195 and again after #193 (task 250's tile-buffer/marker-retention change — conflicted only in docs/adr/README.md's generated index, resolved and confirmed identical to AdrIndex.GenerateTable()'s own canonical output via scripts/regenerate-adr-readme.cs). Current counts: 622 in PlaceMark.Api.Tests, 360 in PlaceMark.Infrastructure.Tests, 39 in PlaceMark.Domain.Tests, 131 in PlaceMark.Architecture.Tests, 844 in PlaceMark.WebUI.Tests (840 passing + 4 pre-existing skips) — all green locally against the pinned SDK, plus a full-solution build with zero warnings, dotnet format --verify-no-changes, and the coverage ratchet (every assembly at or above baseline). CI green at the current head.

Backend half of task 249. UI (upload/replace/remove in `PlaceFormPanel`, thumbnail and popup viewer in `PlaceDetailPanel`, WebUI tests, E2E journey) follows in a second PR — this one is schema, domain, infrastructure, endpoints and their own tests, so it's reviewable on its own rather than landing as one large change. ## What's here - **ADR-0163**: one `place_images` row per place (`place_id` itself as the PK — a second upload replaces the first via `ON CONFLICT`), cascading from `places`. Resize/crop arithmetic lives in `PlaceMark.Domain` with zero packages (`ImageBoundingBoxCalculator`, `ThumbnailCropCalculator`), proved against plain integers; `PlaceMark.Infrastructure.Places.PlaceImageProcessor` (SkiaSharp — MIT, licence read before adding; ImageSharp's split licence was considered and rejected) applies it to real pixels, rotates upright from EXIF orientation, then re-encodes with no EXIF. Explicitly reverses ADR-0006's "no photo attachments" exclusion at Rob's own request — ADR-0006 gains `Partially superseded by: 0163` for that one bullet only. - **ADR-0164**: images are served from their own endpoints (`GET /api/places/{id}/image`, `.../image/thumbnail`), never inlined in the places list. `PlaceResponse`/`PlaceForCallerResponse` carry only `ImageVersion` (optional, defaults to `null` — no existing call site needed touching). Cached `public, max-age=31536000, immutable`, keyed by the client appending `?v={ImageVersion}` rather than a conditional-GET round trip. Corrected during review: this record now says plainly that the WebUI (#194) can't put that URL in the DOM at all — the endpoints need a bearer token a plain `<img src>` can't carry, so it fetches via `HttpClient` and renders a `data:` URI, which the browser's own image cache never sees. - Authorisation reuses ADR-0043: `Member` to view, `MayEditPlaces` to add/replace/delete — the same thresholds the rest of `PlaceEndpoints` already uses. All four image routes carry `RequireGroupCapabilityForPlace`, none skipped. - `docs/data-model.md` gets its own `place_images` section and FK row; `SchemaVersionHealthCheck.RequiredScript` and every test pinned to "the newest script" moved to `0005-place-images.sql`. ## Method Test-first throughout. The two arithmetic rules were each proved to redden against a deliberately wrong implementation (a naive squash to 240×135 for the thumbnail; the FK cascade removed outright) before being trusted. The EXIF rotation is proved against a JPEG this test suite constructs itself — a genuine, standards-compliant EXIF `Orientation` tag spliced onto a plain encode, not a downloaded sample. **Corrected during review, and stated precisely rather than rounded up:** `PlaceImageProcessorTests` asserts real pixel positions after rotation for five of the eight EXIF orientation values — 2, 3, 4, 6 and 8 — each verified to redden against a deliberately wrong transform (6/8 confirmed by swapping their rotation directions). Orientation 1 is the identity transform and needs no positional proof; every other test in the class exercises it implicitly. Orientations 5 and 7 have only the EXIF-stripped theory tests (`Process_AnImageCarryingExifOrientation_StripsAllExifFromTheFullImage`/`...Thumbnail`) and no positional proof of their own rotation — disclosed, not silently missing: both are mirrored-and-rotated values a consumer phone or camera essentially never produces, unlike 6 and 8, the two ordinary portrait-hold rotations every phone photograph actually carries, which now do carry positional proof. All positional and stripping assertions run against both outputs; the rotation direction was verified empirically rather than only derived on paper. The endpoint tests found a real bug this way: minimal API's automatic antiforgery requirement for `IFormFile`-binding endpoints 500ed until `.DisableAntiforgery()` was added, which is correct here since this API has no cookie session for antiforgery to protect. ## Judgement calls worth flagging - `ImageVersion` defaults to `null` rather than being a required positional member, so this doesn't touch `PlaceFormPanel.razor.cs` or any WebUI test's existing positional construction — deliberately, to keep this PR backend-only and avoid colliding with the parallel agent on task 245. - SkiaSharp over ImageSharp: capability was never the deciding factor, the licence was — ImageSharp's Split License is conditional on this project's own licence/revenue staying under a threshold; SkiaSharp's MIT terms aren't conditional on anything. Independently confirmed MIT via NuGet and the upstream `LICENSE.md`; present only in `PlaceMark.Infrastructure`. - A 100-megapixel decoded-dimension ceiling, checked from the codec header before a bitmap is allocated, independent of the 10 MB raw upload cap — a defence against a small file claiming an enormous pixel grid, proved with a patched `SOF0` marker rather than a giant bitmap. ## Tests Rebased twice, onto `main` after #195 and again after #193 (task 250's tile-buffer/marker-retention change — conflicted only in `docs/adr/README.md`'s generated index, resolved and confirmed identical to `AdrIndex.GenerateTable()`'s own canonical output via `scripts/regenerate-adr-readme.cs`). Current counts: 622 in `PlaceMark.Api.Tests`, 360 in `PlaceMark.Infrastructure.Tests`, 39 in `PlaceMark.Domain.Tests`, 131 in `PlaceMark.Architecture.Tests`, 844 in `PlaceMark.WebUI.Tests` (840 passing + 4 pre-existing skips) — all green locally against the pinned SDK, plus a full-solution build with zero warnings, `dotnet format --verify-no-changes`, and the coverage ratchet (every assembly at or above baseline). CI green at the current head.

rob added 1 commit 2026-08-17 09:14:13 +00:00

Add one image per place: schema, processing, and API (task 249, backend) ... ced1f09382

ADR-0163 stores a place's image as full/thumbnail bytea in place_images,
resized with SkiaSharp and rotated upright from EXIF before stripping it.
ADR-0164 serves both through their own cached endpoints, never inline in
the places list. UI surfaces land in a follow-up PR.

rob added 1 commit 2026-08-17 09:42:01 +00:00

Cover every EXIF orientation and the megapixel guard ... ead543d5ca

The coverage ratchet caught four untested rotation cases and the
declared-dimensions ceiling; each now has a real assertion, not just
a line hit.

rob referenced this pull request 2026-08-17 09:47:47 +00:00

Add image upload, thumbnail and full-image viewer to the WebUI (task 249, UI) #194

rob added 1 commit 2026-08-17 09:49:16 +00:00

Fix import ordering dotnet format flagged 130f528191

rob force-pushed feature/task-249-place-images from 130f528191 to 0593ff1311 2026-08-17 10:52:22 +00:00 Compare

rob force-pushed feature/task-249-place-images from 0593ff1311 to 9e050624a6 2026-08-17 11:06:30 +00:00 Compare

rob reviewed 2026-08-17 11:08:10 +00:00

rob left a comment

Copy link

Verdict: changes needed

Reviewed at 0593ff13.

EXIF orientation 8 has no pixel-position assertion, and the gap isn't disclosed. PlaceImageProcessorTests only asserts pixel positions for orientations 2, 3, 4 and 6 (Process_AnImageWithExifOrientation{6,3,2,4}_...). Orientations 1, 5, 7 and 8 are exercised only by the two [Theory] tests that check EXIF is stripped — those assert nothing about correct rotation. That's four cases without positional proof, not two: the PR describes 5 and 7 as intentionally decode/strip-only (honest), but says nothing about 8, which is a real, non-trivial transform (Translate(0, height); RotateDegrees(270) in ApplyOrientation) sharing no code path with 6's RotateDegrees(90) case. A swapped rotation direction or wrong translate there would silently sideways-rotate real photos from that orientation — exactly the failure mode task 249 calls out — and nothing in this suite would catch it. Please add a positional case for orientation 8 (same technique as the existing 6/3/2/4 tests), or if that's deliberately deferred, correct the claim rather than leaving it unstated. (Orientation 1 is the true no-op case, so its absence is lower stakes, but it's also not disclosed.)

Everything else held up:

• Resize/crop arithmetic — hand-checked ImageBoundingBoxCalculator/ThumbnailCropCalculator against every listed test input (panorama, tall, square, already-smaller, exact-fit, sub-135 shrunk height, narrower-than-240 source, odd-remainder crop rounding). All correct; the tests genuinely discriminate a naive squash (they assert the intermediate ShrunkHeight, not just final output).
• Decode ceiling — checked from the codec header before SKBitmap.Decode allocates; the bomb test patches only the JPEG's SOF0 header, proving the check doesn't require materialising the claimed pixel grid.
• Rotate-then-strip — re-encoding from decoded pixels (never the original stream) is what strips EXIF; order is correct in Process.
• Authorisation — all four routes (PUT/DELETE/GET image/GET thumbnail) chain RequireGroupCapabilityForPlace, Member for the two GETs and MayEditPlaces for the writes, reusing the existing ADR-0043 extension rather than a bespoke check. No route skips it.
• Payload discipline — PlaceResponse/PlaceForCallerResponse carry only ImageVersion (nullable, defaults null); PlaceRepository's new correlated subqueries select only xmin, never full_image/thumbnail.
• Schema/cascade — place_id as PK doubles as the one-image-per-place guarantee; ON DELETE CASCADE proved against real Postgres in DeletingThePlace_CascadesToRemoveItsImage, which would fail (FK violation) rather than pass if the cascade clause were removed.
• ADR-0006 — only the frontmatter (Status, new Partially superseded by) changed; body untouched. ADR-0107's conditions are correctly reasoned through in ADR-0163 rather than assumed. ADR-0164's caching-consequence correction is stated plainly, as requested.
• Licence — independently checked: SkiaSharp 4.151.1 on NuGet and its upstream LICENSE.md both confirm MIT. Package sits only in PlaceMark.Infrastructure.
• coverage-baseline.json — every changed figure moved up, none down, so no ratchet was loosened. The WebUI jump (87.9→91.4) is odd for a PR touching zero WebUI files — likely inherited from the main rebase (PR #193) rather than authored here — but since it's an increase it doesn't block.

The orientation-8 gap is the one thing I'd want fixed (or knowingly and explicitly accepted) before merge; everything else is solid, careful work.

Verdict: changes needed Reviewed at `0593ff13`. **EXIF orientation 8 has no pixel-position assertion, and the gap isn't disclosed.** `PlaceImageProcessorTests` only asserts pixel positions for orientations 2, 3, 4 and 6 (`Process_AnImageWithExifOrientation{6,3,2,4}_...`). Orientations 1, 5, 7 and 8 are exercised only by the two `[Theory]` tests that check EXIF is stripped — those assert nothing about correct rotation. That's four cases without positional proof, not two: the PR describes 5 and 7 as intentionally decode/strip-only (honest), but says nothing about 8, which is a real, non-trivial transform (`Translate(0, height); RotateDegrees(270)` in `ApplyOrientation`) sharing no code path with 6's `RotateDegrees(90)` case. A swapped rotation direction or wrong translate there would silently sideways-rotate real photos from that orientation — exactly the failure mode task 249 calls out — and nothing in this suite would catch it. Please add a positional case for orientation 8 (same technique as the existing 6/3/2/4 tests), or if that's deliberately deferred, correct the claim rather than leaving it unstated. (Orientation 1 is the true no-op case, so its absence is lower stakes, but it's also not disclosed.) Everything else held up: - **Resize/crop arithmetic** — hand-checked `ImageBoundingBoxCalculator`/`ThumbnailCropCalculator` against every listed test input (panorama, tall, square, already-smaller, exact-fit, sub-135 shrunk height, narrower-than-240 source, odd-remainder crop rounding). All correct; the tests genuinely discriminate a naive squash (they assert the intermediate `ShrunkHeight`, not just final output). - **Decode ceiling** — checked from the codec header before `SKBitmap.Decode` allocates; the bomb test patches only the JPEG's SOF0 header, proving the check doesn't require materialising the claimed pixel grid. - **Rotate-then-strip** — re-encoding from decoded pixels (never the original stream) is what strips EXIF; order is correct in `Process`. - **Authorisation** — all four routes (`PUT`/`DELETE`/`GET image`/`GET thumbnail`) chain `RequireGroupCapabilityForPlace`, `Member` for the two `GET`s and `MayEditPlaces` for the writes, reusing the existing ADR-0043 extension rather than a bespoke check. No route skips it. - **Payload discipline** — `PlaceResponse`/`PlaceForCallerResponse` carry only `ImageVersion` (nullable, defaults `null`); `PlaceRepository`'s new correlated subqueries select only `xmin`, never `full_image`/`thumbnail`. - **Schema/cascade** — `place_id` as PK doubles as the one-image-per-place guarantee; `ON DELETE CASCADE` proved against real Postgres in `DeletingThePlace_CascadesToRemoveItsImage`, which would fail (FK violation) rather than pass if the cascade clause were removed. - **ADR-0006** — only the frontmatter (`Status`, new `Partially superseded by`) changed; body untouched. `ADR-0107`'s conditions are correctly reasoned through in ADR-0163 rather than assumed. ADR-0164's caching-consequence correction is stated plainly, as requested. - **Licence** — independently checked: SkiaSharp 4.151.1 on NuGet and its upstream `LICENSE.md` both confirm MIT. Package sits only in `PlaceMark.Infrastructure`. - **coverage-baseline.json** — every changed figure moved up, none down, so no ratchet was loosened. The WebUI jump (87.9→91.4) is odd for a PR touching zero WebUI files — likely inherited from the `main` rebase (PR #193) rather than authored here — but since it's an increase it doesn't block. The orientation-8 gap is the one thing I'd want fixed (or knowingly and explicitly accepted) before merge; everything else is solid, careful work.

rob added 1 commit 2026-08-17 11:18:45 +00:00

Add the missing positional proof for EXIF orientation 8 450005c55c

rob reviewed 2026-08-17 11:31:25 +00:00

rob left a comment

Copy link

Verdict: mergeable

Reviewed at 450005c5, confirming against the prior changes needed.

• Orientation 8 assertion genuinely discriminates. It asserts real pixel positions (x:16,y:16 → Red, x:48,y:16 → Blue), which is the true mirror of orientation 6's expectation, and I independently checked the geometry: CCW rotation moves the source's top edge to the destination's left (red at x:16) and its bottom edge to the right (blue at x:48) — exactly opposite to 6's CW mapping (blue left, red right). Swapping RotateDegrees(270) for 90 in the LeftBottom case would produce orientation 6's arrangement instead, which fails this exact assertion — the claimed mutation would genuinely redden it.
• Coverage remark is accurate on the substance — 2/3/4/6/8 positional, 5/7 disclosed strip-only, 1 identity — matching what the test file actually does. One small overstatement: it says the non-orientation tests prove 1 via "the dimensions and colours it expects," but those tests (ALandscapeImage..., AnImageAlreadySmallerThanTheBox..., etc.) only assert width/height, never colour. Low stakes given 1 is a literal source.Copy() no-op, but worth a follow-up tightening of the wording — not a blocker.
• Rebase didn't disturb anything cleared before. PlaceImageEndpoints.cs, PlaceImageProcessor.cs, PlaceMark.Infrastructure.csproj and PlaceRepository.cs show identical diff sizes to the previous review, and the ADR index has all three new rows (0163, 0164, 0165) with nothing else moved.

No further changes needed.

Verdict: mergeable Reviewed at `450005c5`, confirming against the prior `changes needed`. - **Orientation 8 assertion genuinely discriminates.** It asserts real pixel positions (`x:16,y:16` → Red, `x:48,y:16` → Blue), which is the true mirror of orientation 6's expectation, and I independently checked the geometry: CCW rotation moves the source's top edge to the destination's left (red at x:16) and its bottom edge to the right (blue at x:48) — exactly opposite to 6's CW mapping (blue left, red right). Swapping `RotateDegrees(270)` for `90` in the `LeftBottom` case would produce orientation 6's arrangement instead, which fails this exact assertion — the claimed mutation would genuinely redden it. - **Coverage remark is accurate on the substance** — 2/3/4/6/8 positional, 5/7 disclosed strip-only, 1 identity — matching what the test file actually does. One small overstatement: it says the non-orientation tests prove 1 via "the dimensions and colours it expects," but those tests (`ALandscapeImage...`, `AnImageAlreadySmallerThanTheBox...`, etc.) only assert width/height, never colour. Low stakes given 1 is a literal `source.Copy()` no-op, but worth a follow-up tightening of the wording — not a blocker. - **Rebase didn't disturb anything cleared before.** `PlaceImageEndpoints.cs`, `PlaceImageProcessor.cs`, `PlaceMark.Infrastructure.csproj` and `PlaceRepository.cs` show identical diff sizes to the previous review, and the ADR index has all three new rows (0163, 0164, 0165) with nothing else moved. No further changes needed.

rob added 1 commit 2026-08-17 11:33:04 +00:00

Stop the coverage remark claiming a colour check orientation 1 lacks cbc7d272ef

rob merged commit 969442bf12 into main 2026-08-17 11:43:49 +00:00

rob referenced this pull request from a commit 2026-08-17 11:43:50 +00:00

Store one image per place, served from its own endpoints (#192)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!192

No description provided.