[AZ-484] Fix multi-source tile reads: drop Dapper enum handler

Two integration-test failures uncovered after the initial commit:

1) GetTilesByRegionAsync outer ORDER BY referenced 'updated_at' but
   the inner DISTINCT ON subquery aliased it to 'UpdatedAt' (Postgres
   folds to 'updatedat'). DISTINCT ON already guarantees one row per
   (latitude, longitude, ...) so the third tiebreak was unreachable;
   removed it.

2) Dapper 2.1.35 silently bypasses SqlMapper.TypeHandler<T> for enum
   types during read deserialization (Dapper issue #259). The
   TileSourceTypeHandler worked for writes but reads fell through to
   Enum.TryParse, which cannot map 'google_maps' to GoogleMaps.

   Pivoted: TileEntity.Source is now a string (the wire value).
   TileSource enum stays as the public producer surface in
   Common.Enums; TileSourceConverter (Common.Enums) provides
   ToWireValue / FromWireValue / IsValidWireValue at the boundary.
   TileSourceTypeHandler deleted; registration removed from
   DapperEnumTypeHandlers.RegisterAll.

   tile-storage.md Inv-5 amended to document the storage choice.
   _docs/LESSONS.md L-001 records the Dapper bypass for future cycles.

Full suite passes (213 unit + integration suite incl. AZ-484
AC-1..AC-5, security SEC-01..SEC-04, AZ-356/362/357).

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-11 06:44:34 +03:00
parent 687d6bdd5b
commit e9d6db077c
16 changed files with 181 additions and 184 deletions
@@ -80,7 +80,7 @@ The selection rule is **most-recent across all sources** ordered by `captured_at
- **Inv-2**: Every row has a non-null `captured_at` in UTC.
- **Inv-3**: At most one row exists per `(latitude, longitude, tile_zoom, tile_size_meters, source)`.
- **Inv-4**: For any cell with one or more rows, the row returned by `GetByTileCoordinatesAsync` and the per-cell row returned by `GetTilesByRegionAsync` are identical.
- **Inv-5**: The `source` column value space is closed: only the strings emitted by `TileSource.ToString()` are valid. Adding a new producer requires a new enum member AND a contract version bump (minor).
- **Inv-5**: The `source` column value space is closed: only the snake_case wire values defined in `SatelliteProvider.Common.Enums.TileSourceConverter` (`"google_maps"`, `"uav"`) are valid. Adding a new producer requires a new `TileSource` enum member, a corresponding wire value in `TileSourceConverter`, AND a contract version bump (minor). Note: `TileEntity.Source` is stored as the wire string (not the C# enum) because Dapper's `TypeHandler<T>` for enum types is bypassed during read deserialization (Dapper issue #259); `TileSourceConverter.{ToWireValue,FromWireValue}` is the documented bridge.
- **Inv-6**: `captured_at` semantics are producer-defined per the Field Reference table above; consumers MUST NOT reinterpret it (e.g., consumers MUST NOT assume `captured_at` from `google_maps` reflects original imagery date).
## Non-Goals
+1
View File
@@ -30,6 +30,7 @@
- `SatelliteProvider.Common/Enums/RegionStatus.cs`
- `SatelliteProvider.Common/Enums/RoutePointType.cs`
- `SatelliteProvider.Common/Enums/TileSource.cs` (added by AZ-484; backed by the `tile-storage` v1.0.0 contract)
- `SatelliteProvider.Common/Enums/TileSourceConverter.cs` (added by AZ-484; converts `TileSource` enum to/from the snake_case wire string used by `TileEntity.Source`)
- `SatelliteProvider.Common/Exceptions/RateLimitException.cs`
- `SatelliteProvider.Common/Interfaces/*.cs` (all service interfaces)
- `SatelliteProvider.Common/Utils/GeoUtils.cs`
@@ -20,15 +20,15 @@
| Per-source UPSERT semantics on insert | `TileRepository.InsertAsync` (`ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters, source) DO UPDATE`) |
| `UpdateAsync` includes the new fields | `TileRepository.UpdateAsync` SET clause |
| Google Maps download path stamps `Source` + `CapturedAt` | `TileService.BuildTileEntity` |
| Snake_case wire format for `TileSource` | `TileSourceTypeHandler` (registered in `DapperEnumTypeHandlers.RegisterAll`) |
| Snake_case wire format for `TileSource` | `SatelliteProvider.Common.Enums.TileSourceConverter` (`ToWireValue` / `FromWireValue` / `IsValidWireValue`). `TileEntity.Source` stores the wire string directly because Dapper bypasses `TypeHandler<T>` for enum reads — see `_docs/LESSONS.md` L-001. |
| Architecture Vision amended | `_docs/02_document/architecture.md` (Architecture Vision principles + System Context) |
| Glossary amended | `_docs/02_document/glossary.md` (`Tile Source`, `Captured At`; Layer 1 / Layer 2 disambiguation) |
| Module-layout amended | `_docs/02_document/module-layout.md` (Common Public API list) |
| Contract `tile-storage.md` v1.0.0 frozen | `_docs/02_document/contracts/data-access/tile-storage.md` Status: `frozen` |
**Unresolved markers** (`TODO`, `placeholder`, `NotImplemented`, etc.) under owned files (`SatelliteProvider.Common/Enums/TileSource.cs`, `SatelliteProvider.DataAccess/Migrations/013_AddTileSourceAndCapturedAt.sql`, `SatelliteProvider.DataAccess/TypeHandlers/TileSourceTypeHandler.cs`): none found.
**Unresolved markers** (`TODO`, `placeholder`, `NotImplemented`, etc.) under owned files (`SatelliteProvider.Common/Enums/TileSource.cs`, `SatelliteProvider.Common/Enums/TileSourceConverter.cs`, `SatelliteProvider.DataAccess/Migrations/013_AddTileSourceAndCapturedAt.sql`, `SatelliteProvider.DataAccess/Models/TileEntity.cs`, `SatelliteProvider.DataAccess/Repositories/TileRepository.cs`, `SatelliteProvider.Services.TileDownloader/TileService.cs`): none found.
**Named runtime dependencies**: AZ-484 introduces no new external dependency. It uses the existing Dapper / Npgsql / DbUp stack already integrated. The new `TileSourceTypeHandler` is wired into `DapperEnumTypeHandlers.RegisterAll`, which is invoked at API startup as part of DI registration.
**Named runtime dependencies**: AZ-484 introduces no new external dependency. It uses the existing Dapper / Npgsql / DbUp stack already integrated. `TileSourceConverter` is a pure helper with no DI registration required. The deleted `TileSourceTypeHandler` registration was removed from `DapperEnumTypeHandlers.RegisterAll` after the Dapper-enum-bypass discovery (see L-001).
**Internal vs external**: every promised capability is an internal product capability and is implemented in production code. No promise is blocked on an external prerequisite. The deferred AZ-485 (UAV upload endpoint) is explicitly out of scope for this task.
@@ -13,19 +13,19 @@
AZ-484 introduces multi-source tile storage to the `tiles` table and freezes the v1.0.0 `tile-storage` contract that future producers (T2 — UAV upload AZ-485, future SatAR provider, etc.) will consume. The implementation:
- Migration `013_AddTileSourceAndCapturedAt.sql` adds `source` (`VARCHAR(32) NOT NULL`) and `captured_at` (`TIMESTAMP NOT NULL`) to `tiles`, backfills existing rows to `(source='google_maps', captured_at=created_at)`, drops the legacy 4-column unique index `idx_tiles_unique_location`, and creates the new 5-column unique index `idx_tiles_unique_location_source`. The whole migration runs inside a single transaction so a failure mid-flight cannot leave the table without its unique index or with partially backfilled rows.
- `SatelliteProvider.Common.Enums.TileSource { GoogleMaps, Uav }` is the producer enum. Because the v1.0.0 contract requires snake_case wire values (`google_maps`, `uav`) and the existing generic `EnumStringTypeHandler<T>` only emits `value.ToString().ToLowerInvariant()`, AZ-484 introduces a focused `TileSourceTypeHandler` that owns the bidirectional mapping. Registration is added to `DapperEnumTypeHandlers.RegisterAll`.
- `TileEntity` exposes `Source` and `CapturedAt` properties.
- `SatelliteProvider.Common.Enums.TileSource { GoogleMaps, Uav }` is the public producer enum. Because the v1.0.0 contract requires snake_case wire values (`google_maps`, `uav`) AND because Dapper 2.1.35 silently bypasses `SqlMapper.TypeHandler<TEnum>` during read deserialization (Dapper issue #259 — see `_docs/LESSONS.md` L-001), `TileEntity.Source` is stored as a plain `string` (the wire value) and `SatelliteProvider.Common.Enums.TileSourceConverter` provides the boundary conversion (`ToWireValue`, `FromWireValue`, `IsValidWireValue`). The first attempt at this batch used a `TileSourceTypeHandler : SqlMapper.TypeHandler<TileSource>` registered via `DapperEnumTypeHandlers.RegisterAll`; that worked for writes but failed at integration-test time on reads, leading to the converter pivot.
- `TileEntity` exposes `Source` (string, default `TileSourceConverter.GoogleMapsWireValue`) and `CapturedAt` properties.
- `TileRepository` is updated end-to-end:
- `ColumnList` includes `source` and `captured_at as CapturedAt`.
- `GetByTileCoordinatesAsync` returns the most-recent row across sources via `ORDER BY captured_at DESC, updated_at DESC, id DESC LIMIT 1`.
- `GetTilesByRegionAsync` uses `DISTINCT ON (latitude, longitude, tile_zoom, tile_size_meters)` with the same tie-break tuple, then restores the pre-AZ-484 caller-facing row order.
- `InsertAsync` UPSERTs on the 5-column conflict key and refreshes `captured_at` and `updated_at` on conflict.
- `UpdateAsync` writes `source` and `captured_at` alongside the other columns.
- `TileService.BuildTileEntity` stamps `Source = TileSource.GoogleMaps` and `CapturedAt = DateTime.UtcNow` so every Google-Maps-originated row carries the contract-required fields.
- `TileService.BuildTileEntity` stamps `Source = TileSourceConverter.ToWireValue(TileSource.GoogleMaps)` (i.e., the wire string `"google_maps"`) and `CapturedAt = DateTime.UtcNow` so every Google-Maps-originated row carries the contract-required fields.
- Documentation:
- `_docs/02_document/architecture.md` Architecture Vision and System Context describe the N-source model, append-by-source storage, and most-recent-across-sources reads, and point at the contract as authoritative.
- `_docs/02_document/glossary.md` adds `Tile Source` and `Captured At`, and disambiguates the historic `Layer 1` / `Layer 2` terms against the new `TileSource` enum.
- `_docs/02_document/module-layout.md` lists `SatelliteProvider.Common/Enums/TileSource.cs` (and the previously implicit `RegionStatus.cs`, `RoutePointType.cs`) under the Common Public API surface.
- `_docs/02_document/module-layout.md` lists `SatelliteProvider.Common/Enums/TileSource.cs` and `TileSourceConverter.cs` (and the previously implicit `RegionStatus.cs`, `RoutePointType.cs`) under the Common Public API surface.
- `_docs/02_document/contracts/data-access/tile-storage.md` is now Status `frozen` at v1.0.0.
## AC Coverage (7/7 — see batch report for the full table)
@@ -51,22 +51,18 @@ AZ-484 introduces multi-source tile storage to the `tiles` table and freezes the
## Deviations from Spec
- **Wire-format handler**: the spec says "string-stored via the existing `EnumStringTypeHandler` pattern". The existing pattern emits `ToString().ToLowerInvariant()`, which would produce `'googlemaps'`. The contract requires `'google_maps'`. Implementation follows the contract by introducing a focused `TileSourceTypeHandler`. The "pattern" — generic Dapper type handler registered through `DapperEnumTypeHandlers.RegisterAll` — is preserved.
- **Wire-format handler**: the spec says "string-stored via the existing `EnumStringTypeHandler` pattern". The existing pattern emits `ToString().ToLowerInvariant()`, which would produce `'googlemaps'` (contract requires `'google_maps'`). The first attempt fixed this by introducing `TileSourceTypeHandler : SqlMapper.TypeHandler<TileSource>`. Integration tests revealed that **Dapper bypasses TypeHandler<T> for enum types during read deserialization** (Dapper issue #259, see `_docs/LESSONS.md` L-001). Final implementation pivots to: store `TileEntity.Source` as a `string` (the wire value), and provide `TileSourceConverter` for explicit enum<->wire conversion at the service-layer boundary. Producers still author with the `TileSource` enum and convert at the entity boundary; consumers compare against the wire-value constants on `TileSourceConverter`. The contract (`tile-storage.md` Inv-5) was amended to document this storage choice.
- **Test site count**: the spec estimated ~12 sites needing mock fixups including ~3 in `RegionServiceTests`. Actual count: 5 sites in `TileServiceTests.cs` and 1 ColumnList assertion in `RepositoryRefactorTests.cs`. `RegionServiceTests.cs` and `InfrastructureTests.cs` did not require `TileEntity` field updates (the latter only constructs mocks, not entities).
## Handoff to Step 11 (Run Tests)
## Step 11 Run Tests result
Per `/implement` skill Step 16: this cycle does not execute the full test suite locally because the autodev next step is Run Tests, which owns the full-suite gate.
Full suite (`scripts/run-tests.sh --full`) passed on attempt 3 (logs in `_docs/03_implementation/test_runs/`):
Recommended Step-11 invocation:
- Unit suite: `scripts/run-tests.sh --unit-only` (covers the new AC-5 + handler tests).
- Smoke suite: `scripts/run-tests.sh --smoke` (covers AC-1..AC-4 integration migration tests AND verifies AC-6 regression — the 5 smoke scenarios continue to pass).
- **Attempt 1** (committed `687d6bd`): integration test failed — `column "updated_at" does not exist`. Root cause: outer `ORDER BY ... updated_at DESC` in `GetTilesByRegionAsync` referenced a column that the inner `DISTINCT ON` subquery had aliased to `UpdatedAt` (Postgres folded to `updatedat`). Fix: removed the unnecessary outer tiebreak — `DISTINCT ON` already guarantees one row per `(latitude, longitude, ...)` so the third sort term was unreachable.
- **Attempt 2**: integration test failed — `Error parsing column 12 (source=google_maps - String)`. Root cause: Dapper bypasses `TypeHandler<T>` for enum reads (Dapper issue #259); `RegionStatus`/`RoutePointType` accidentally worked because their wire values match member names case-insensitively, but `TileSource.GoogleMaps``"google_maps"` does not. Fix: pivoted to string-stored `TileEntity.Source` + `TileSourceConverter`; deleted the bypassed `TileSourceTypeHandler`; added `TileSourceConverterTests`.
- **Attempt 3**: PASSED. 213 unit tests + full integration suite (multi-source migration, AC-1..AC-4, security SEC-01..SEC-04, AZ-356 stubs, AZ-362 idempotency, AZ-357 migration 012, all route/region happy paths). Wall clock ≈ 5m40s.
If `test-run` reports a failure, the highest-signal checks are:
1. `BuildTileEntity_SetsGoogleMapsSourceAndUtcCapturedAt_AZ484_AC5` — confirms the production write path.
2. `MostRecentAcrossSourcesSelection_AZ484_AC2` — confirms the new SQL ORDER BY shape.
3. `NewUniqueConstraintIncludesSourceColumn_AZ484_AC1` — confirms migration 013 ran correctly against the live schema.
4. The 5 smoke scenarios — confirms region/route flows behave identically to the pre-T1 baseline (AC-6).
The two pivots are recorded in `_docs/LESSONS.md` (L-001 covers the Dapper bypass).
## Deferred work
+29
View File
@@ -0,0 +1,29 @@
# Engineering Lessons
Recurring bugs, surprising library behaviors, and process insights extracted from completed cycles. Newest at the top. Keep entries short — this is for fast scanning at the start of new cycles, not exhaustive history.
---
## L-001 — Dapper `TypeHandler<T>` is bypassed for enum types during read deserialization
**Cycle**: 1 (AZ-484)
**Discovered by**: integration test failure (`Error parsing column 12 (source=google_maps - String)`); root-caused via web search to long-standing Dapper issue [#259](https://github.com/DapperLib/Dapper/issues/259).
**Affects**: Dapper 2.1.35 (and most other versions until the proposed `Settings.PreferTypeHandlersForEnums` opt-in in PR #2200, not yet merged).
**What happens**
Registering `SqlMapper.AddTypeHandler(new MyEnumHandler())` for an `enum` type — even via `SqlMapper.TypeHandler<TEnum>` — works for **writes** (the handler's `SetValue` is invoked for parameter binding) but is **silently bypassed for reads**. Dapper's IL-emitted deserializer checks `IsEnum` first and falls back to `Enum.TryParse(string, ignoreCase: true)`.
**Why this is dangerous**
If the enum's wire string happens to match a member name case-insensitively (e.g., `RegionStatus.Failed``"failed"`), the bypass goes unnoticed and round-trip works *accidentally*. The bug only surfaces when the wire format diverges from the C# member name (e.g., `TileSource.GoogleMaps``"google_maps"``Enum.TryParse("google_maps")` does not match `GoogleMaps` because of the underscore).
**Recommended approach**
- Do **not** rely on `SqlMapper.TypeHandler<TEnum>` for read-side enum mapping unless the wire values match the enum member names case-insensitively.
- For enums whose wire format diverges (snake_case, kebab-case, custom IDs), store the entity field as `string` and provide an explicit converter (`*Converter.ToWireValue` / `FromWireValue`) for use at the service-layer boundary. This is what AZ-484 does for `TileEntity.Source``TileSourceConverter`.
- Unit-test the converter directly. Do not assume that round-tripping through Dapper proves anything for enums.
**Detection**
- Unit tests of the type handler in isolation will pass even when the handler is bypassed at runtime.
- Failure surfaces only at integration-test time when the actual SELECT runs.
- If you must keep an enum-typed field, write at minimum one integration test that reads the enum back through Dapper from a real database row.
---
+2 -2
View File
@@ -2,8 +2,8 @@
## Current Step
flow: existing-code
step: 11
name: Run Tests
step: 12
name: Test-Spec Sync
status: not_started
sub_step:
phase: 0