diff --git a/.gitignore b/.gitignore index 1e10283..333906f 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ TestResults/ coverage.cobertura.xml coverage.opencover.xml *.coverage +_docs/03_implementation/test_runs/ diff --git a/SatelliteProvider.Common/Enums/TileSourceConverter.cs b/SatelliteProvider.Common/Enums/TileSourceConverter.cs new file mode 100644 index 0000000..9d1db4f --- /dev/null +++ b/SatelliteProvider.Common/Enums/TileSourceConverter.cs @@ -0,0 +1,36 @@ +namespace SatelliteProvider.Common.Enums; + +// AZ-484: contract v1.0.0 stores TileSource as a snake_case string +// ('google_maps', 'uav'). Dapper's TypeHandler for enum types is bypassed +// during deserialization (Dapper issue #259), so we cannot rely on a Dapper +// handler to round-trip enum<->wire string. Instead, TileEntity stores the +// wire value as a plain string and producers/consumers convert through this +// helper at the boundary, preserving type safety in business code while +// avoiding the Dapper enum read bug. +public static class TileSourceConverter +{ + public const string GoogleMapsWireValue = "google_maps"; + public const string UavWireValue = "uav"; + + public static string ToWireValue(TileSource value) => value switch + { + TileSource.GoogleMaps => GoogleMapsWireValue, + TileSource.Uav => UavWireValue, + _ => throw new ArgumentOutOfRangeException(nameof(value), value, "Unknown TileSource"), + }; + + public static TileSource FromWireValue(string wireValue) + { + ArgumentNullException.ThrowIfNull(wireValue); + + return wireValue.ToLowerInvariant() switch + { + GoogleMapsWireValue => TileSource.GoogleMaps, + UavWireValue => TileSource.Uav, + _ => throw new ArgumentException($"'{wireValue}' is not a defined member of enum TileSource", nameof(wireValue)), + }; + } + + public static bool IsValidWireValue(string? wireValue) => + wireValue is GoogleMapsWireValue or UavWireValue; +} diff --git a/SatelliteProvider.DataAccess/Models/TileEntity.cs b/SatelliteProvider.DataAccess/Models/TileEntity.cs index b69c038..53d04a6 100644 --- a/SatelliteProvider.DataAccess/Models/TileEntity.cs +++ b/SatelliteProvider.DataAccess/Models/TileEntity.cs @@ -16,7 +16,11 @@ public class TileEntity public string? MapsVersion { get; set; } public int? Version { get; set; } public string FilePath { get; set; } = string.Empty; - public TileSource Source { get; set; } + // AZ-484: stored as the contract wire value (snake_case string). See + // TileSourceConverter for enum<->wire conversion. Cannot use the + // TileSource enum directly because Dapper bypasses TypeHandler for + // enum types during read deserialization (Dapper issue #259). + public string Source { get; set; } = TileSourceConverter.GoogleMapsWireValue; public DateTime CapturedAt { get; set; } public DateTime CreatedAt { get; set; } public DateTime UpdatedAt { get; set; } diff --git a/SatelliteProvider.DataAccess/Repositories/TileRepository.cs b/SatelliteProvider.DataAccess/Repositories/TileRepository.cs index 91bcad7..c7f63f8 100644 --- a/SatelliteProvider.DataAccess/Repositories/TileRepository.cs +++ b/SatelliteProvider.DataAccess/Repositories/TileRepository.cs @@ -70,7 +70,9 @@ public class TileRepository : ITileRepository // AZ-484 selection rule: at most one row per (lat, lon, zoom, size) cell, picking // the most-recent across sources via DISTINCT ON, with deterministic tie-break on // (captured_at DESC, updated_at DESC, id DESC). The outer ORDER BY restores the - // pre-AZ-484 caller-facing order (latitude DESC, longitude ASC, updated_at DESC). + // pre-AZ-484 caller-facing order (latitude DESC, longitude ASC); the pre-AZ-484 + // updated_at DESC tiebreak is unreachable here because DISTINCT ON already + // guarantees one row per (latitude, longitude, ...) tuple. const string sql = $@" SELECT * FROM ( SELECT DISTINCT ON (latitude, longitude, tile_zoom, tile_size_meters) @@ -82,7 +84,7 @@ public class TileRepository : ITileRepository ORDER BY latitude, longitude, tile_zoom, tile_size_meters, captured_at DESC, updated_at DESC, id DESC ) deduped - ORDER BY latitude DESC, longitude ASC, updated_at DESC"; + ORDER BY latitude DESC, longitude ASC"; var stopwatch = Stopwatch.StartNew(); var tiles = await connection.QueryAsync(sql, new diff --git a/SatelliteProvider.DataAccess/TypeHandlers/EnumStringTypeHandler.cs b/SatelliteProvider.DataAccess/TypeHandlers/EnumStringTypeHandler.cs index db9a138..221acb8 100644 --- a/SatelliteProvider.DataAccess/TypeHandlers/EnumStringTypeHandler.cs +++ b/SatelliteProvider.DataAccess/TypeHandlers/EnumStringTypeHandler.cs @@ -47,6 +47,5 @@ public static class DapperEnumTypeHandlers SqlMapper.AddTypeHandler(new EnumStringTypeHandler()); SqlMapper.AddTypeHandler(new EnumStringTypeHandler()); - SqlMapper.AddTypeHandler(new TileSourceTypeHandler()); } } diff --git a/SatelliteProvider.DataAccess/TypeHandlers/TileSourceTypeHandler.cs b/SatelliteProvider.DataAccess/TypeHandlers/TileSourceTypeHandler.cs deleted file mode 100644 index 9fcdf77..0000000 --- a/SatelliteProvider.DataAccess/TypeHandlers/TileSourceTypeHandler.cs +++ /dev/null @@ -1,49 +0,0 @@ -using System.Data; -using Dapper; -using SatelliteProvider.Common.Enums; - -namespace SatelliteProvider.DataAccess.TypeHandlers; - -// AZ-484: TileSource needs an explicit string mapping because the multi-word -// member 'GoogleMaps' must round-trip as the snake_case wire value 'google_maps' -// per the v1.0.0 tile-storage contract. The generic EnumStringTypeHandler -// only does ToString().ToLowerInvariant(), which would emit 'googlemaps'. -public class TileSourceTypeHandler : SqlMapper.TypeHandler -{ - public const string GoogleMapsWireValue = "google_maps"; - public const string UavWireValue = "uav"; - - public override TileSource Parse(object value) - { - if (value is null || value is DBNull) - { - throw new DataException("Cannot parse null DB value into enum TileSource"); - } - - var s = value as string ?? value.ToString(); - if (string.IsNullOrEmpty(s)) - { - throw new DataException("Cannot parse empty DB value into enum TileSource"); - } - - return s.ToLowerInvariant() switch - { - GoogleMapsWireValue => TileSource.GoogleMaps, - UavWireValue => TileSource.Uav, - _ => throw new DataException($"DB value '{s}' is not a defined member of enum TileSource"), - }; - } - - public override void SetValue(IDbDataParameter parameter, TileSource value) - { - parameter.Value = ToWireValue(value); - parameter.DbType = DbType.String; - } - - public static string ToWireValue(TileSource value) => value switch - { - TileSource.GoogleMaps => GoogleMapsWireValue, - TileSource.Uav => UavWireValue, - _ => throw new ArgumentOutOfRangeException(nameof(value), value, "Unknown TileSource"), - }; -} diff --git a/SatelliteProvider.Services.TileDownloader/TileService.cs b/SatelliteProvider.Services.TileDownloader/TileService.cs index 045a68f..5da38e9 100644 --- a/SatelliteProvider.Services.TileDownloader/TileService.cs +++ b/SatelliteProvider.Services.TileDownloader/TileService.cs @@ -158,7 +158,7 @@ public class TileService : ITileService MapsVersion = null, Version = null, FilePath = downloaded.FilePath, - Source = TileSource.GoogleMaps, + Source = TileSourceConverter.ToWireValue(TileSource.GoogleMaps), CapturedAt = now, CreatedAt = now, UpdatedAt = now diff --git a/SatelliteProvider.Tests/EnumStringTypeHandlerTests.cs b/SatelliteProvider.Tests/EnumStringTypeHandlerTests.cs index c040cb0..58b2c06 100644 --- a/SatelliteProvider.Tests/EnumStringTypeHandlerTests.cs +++ b/SatelliteProvider.Tests/EnumStringTypeHandlerTests.cs @@ -137,107 +137,4 @@ public class EnumStringTypeHandlerTests DapperEnumTypeHandlers.RegisterAll(); DapperEnumTypeHandlers.RegisterAll(); } - - [Theory] - [InlineData(TileSource.GoogleMaps, "google_maps")] - [InlineData(TileSource.Uav, "uav")] - public void TileSourceHandler_SetValue_WritesContractWireValue_AZ484(TileSource value, string expected) - { - // Arrange - var handler = new TileSourceTypeHandler(); - var param = new NpgsqlParameter(); - - // Act - handler.SetValue(param, value); - - // Assert - param.Value.Should().Be(expected); - param.DbType.Should().Be(DbType.String); - } - - [Theory] - [InlineData("google_maps", TileSource.GoogleMaps)] - [InlineData("GOOGLE_MAPS", TileSource.GoogleMaps)] - [InlineData("uav", TileSource.Uav)] - [InlineData("UAV", TileSource.Uav)] - public void TileSourceHandler_Parse_AcceptsContractWireValue_AZ484(string raw, TileSource expected) - { - // Arrange - var handler = new TileSourceTypeHandler(); - - // Act - var result = handler.Parse(raw); - - // Assert - result.Should().Be(expected); - } - - [Theory] - [InlineData(TileSource.GoogleMaps)] - [InlineData(TileSource.Uav)] - public void TileSourceHandler_RoundTrip_PreservesValue_AZ484(TileSource value) - { - // Arrange - var handler = new TileSourceTypeHandler(); - var param = new NpgsqlParameter(); - handler.SetValue(param, value); - - // Act - var roundTripped = handler.Parse(param.Value!); - - // Assert - roundTripped.Should().Be(value); - } - - [Fact] - public void TileSourceHandler_Parse_UnknownString_ThrowsDataException_AZ484() - { - // Arrange - var handler = new TileSourceTypeHandler(); - - // Act - Action act = () => handler.Parse("satar"); - - // Assert — Inv-1: unknown sources surface, not silently coerce (coderule.mdc: never suppress errors). - act.Should().Throw().WithMessage("*satar*"); - } - - [Fact] - public void TileSourceHandler_Parse_NullValue_ThrowsDataException_AZ484() - { - // Arrange - var handler = new TileSourceTypeHandler(); - - // Act - Action act = () => handler.Parse(null!); - - // Assert - act.Should().Throw(); - } - - [Fact] - public void TileSourceHandler_Parse_DbNullValue_ThrowsDataException_AZ484() - { - // Arrange - var handler = new TileSourceTypeHandler(); - - // Act - Action act = () => handler.Parse(DBNull.Value); - - // Assert - act.Should().Throw(); - } - - [Fact] - public void RegisterAll_RegistersTileSourceHandler_AZ484() - { - // Arrange / Act - DapperEnumTypeHandlers.RegisterAll(); - - // Assert — registration is idempotent and the handler emits the contract wire value. - var probe = new TileSourceTypeHandler(); - var param = new NpgsqlParameter(); - probe.SetValue(param, TileSource.GoogleMaps); - param.Value.Should().Be("google_maps"); - } } diff --git a/SatelliteProvider.Tests/TileServiceTests.cs b/SatelliteProvider.Tests/TileServiceTests.cs index 91732b3..f02c01f 100644 --- a/SatelliteProvider.Tests/TileServiceTests.cs +++ b/SatelliteProvider.Tests/TileServiceTests.cs @@ -94,7 +94,7 @@ public class TileServiceTests FilePath = "tiles/18/0/0/cached.jpg", TileSizePixels = 256, ImageType = "jpg", - Source = TileSource.GoogleMaps, + Source = TileSourceConverter.ToWireValue(TileSource.GoogleMaps), CapturedAt = DateTime.UtcNow, }, }; @@ -151,7 +151,7 @@ public class TileServiceTests FilePath = "tiles/18/0/0/cached_prior_year.jpg", TileSizePixels = 256, ImageType = "jpg", - Source = TileSource.GoogleMaps, + Source = TileSourceConverter.ToWireValue(TileSource.GoogleMaps), CapturedAt = DateTime.UtcNow.AddYears(-1), }, }; @@ -276,7 +276,7 @@ public class TileServiceTests ImageType = "jpg", FilePath = "tiles/18/0/0/x.jpg", Version = 2026, - Source = TileSource.GoogleMaps, + Source = TileSourceConverter.ToWireValue(TileSource.GoogleMaps), CapturedAt = DateTime.UtcNow, }; var tileRepo = new Mock(); @@ -353,7 +353,7 @@ public class TileServiceTests TileX = x, TileY = y, FilePath = tempPath, - Source = TileSource.GoogleMaps, + Source = TileSourceConverter.ToWireValue(TileSource.GoogleMaps), CapturedAt = DateTime.UtcNow, }); @@ -484,8 +484,8 @@ public class TileServiceTests // Assert var after = DateTime.UtcNow; captured.Should().NotBeNull(); - captured!.Source.Should().Be(TileSource.GoogleMaps, - "AZ-484 AC-5: the Google Maps download path stamps Source=GoogleMaps"); + captured!.Source.Should().Be(TileSourceConverter.ToWireValue(TileSource.GoogleMaps), + "AZ-484 AC-5: the Google Maps download path stamps Source='google_maps' (contract wire value)"); captured.CapturedAt.Kind.Should().NotBe(DateTimeKind.Local, "captured_at must be UTC per the v1.0.0 storage contract"); captured.CapturedAt.Should().BeOnOrAfter(before).And.BeOnOrBefore(after, diff --git a/SatelliteProvider.Tests/TileSourceConverterTests.cs b/SatelliteProvider.Tests/TileSourceConverterTests.cs new file mode 100644 index 0000000..a5be01e --- /dev/null +++ b/SatelliteProvider.Tests/TileSourceConverterTests.cs @@ -0,0 +1,81 @@ +using FluentAssertions; +using SatelliteProvider.Common.Enums; + +namespace SatelliteProvider.Tests; + +public class TileSourceConverterTests +{ + [Theory] + [InlineData(TileSource.GoogleMaps, "google_maps")] + [InlineData(TileSource.Uav, "uav")] + public void ToWireValue_EmitsContractWireValue_AZ484(TileSource value, string expected) + { + // Act + var result = TileSourceConverter.ToWireValue(value); + + // Assert + result.Should().Be(expected); + } + + [Theory] + [InlineData("google_maps", TileSource.GoogleMaps)] + [InlineData("GOOGLE_MAPS", TileSource.GoogleMaps)] + [InlineData("uav", TileSource.Uav)] + [InlineData("UAV", TileSource.Uav)] + public void FromWireValue_AcceptsContractWireValue_AZ484(string raw, TileSource expected) + { + // Act + var result = TileSourceConverter.FromWireValue(raw); + + // Assert + result.Should().Be(expected); + } + + [Theory] + [InlineData(TileSource.GoogleMaps)] + [InlineData(TileSource.Uav)] + public void RoundTrip_PreservesValue_AZ484(TileSource value) + { + // Act + var roundTripped = TileSourceConverter.FromWireValue(TileSourceConverter.ToWireValue(value)); + + // Assert + roundTripped.Should().Be(value); + } + + [Fact] + public void FromWireValue_UnknownString_ThrowsArgumentException_AZ484() + { + // Act + Action act = () => TileSourceConverter.FromWireValue("satar"); + + // Assert — Inv-1: unknown sources surface, not silently coerce (coderule.mdc: never suppress errors). + act.Should().Throw().WithMessage("*satar*"); + } + + [Fact] + public void FromWireValue_NullValue_ThrowsArgumentNullException_AZ484() + { + // Act + Action act = () => TileSourceConverter.FromWireValue(null!); + + // Assert + act.Should().Throw(); + } + + [Theory] + [InlineData("google_maps", true)] + [InlineData("uav", true)] + [InlineData("GoogleMaps", false)] + [InlineData("satar", false)] + [InlineData("", false)] + [InlineData(null, false)] + public void IsValidWireValue_OnlyExactWireValuesPass_AZ484(string? raw, bool expected) + { + // Act + var result = TileSourceConverter.IsValidWireValue(raw); + + // Assert + result.Should().Be(expected); + } +} diff --git a/_docs/02_document/contracts/data-access/tile-storage.md b/_docs/02_document/contracts/data-access/tile-storage.md index ff0846c..46a8a49 100644 --- a/_docs/02_document/contracts/data-access/tile-storage.md +++ b/_docs/02_document/contracts/data-access/tile-storage.md @@ -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` 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 diff --git a/_docs/02_document/module-layout.md b/_docs/02_document/module-layout.md index cb66857..07e1ae5 100644 --- a/_docs/02_document/module-layout.md +++ b/_docs/02_document/module-layout.md @@ -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` diff --git a/_docs/03_implementation/implementation_completeness_cycle1_report.md b/_docs/03_implementation/implementation_completeness_cycle1_report.md index fecabe0..b99d4da 100644 --- a/_docs/03_implementation/implementation_completeness_cycle1_report.md +++ b/_docs/03_implementation/implementation_completeness_cycle1_report.md @@ -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` 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. diff --git a/_docs/03_implementation/implementation_report_multi_source_tile_storage_cycle1.md b/_docs/03_implementation/implementation_report_multi_source_tile_storage_cycle1.md index 8a06146..af60c46 100644 --- a/_docs/03_implementation/implementation_report_multi_source_tile_storage_cycle1.md +++ b/_docs/03_implementation/implementation_report_multi_source_tile_storage_cycle1.md @@ -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` 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` 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` 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`. Integration tests revealed that **Dapper bypasses TypeHandler 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` 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 diff --git a/_docs/LESSONS.md b/_docs/LESSONS.md new file mode 100644 index 0000000..46bdaf7 --- /dev/null +++ b/_docs/LESSONS.md @@ -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` 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` — 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` 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. + +--- diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 7789e7f..fc79e12 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -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