diff --git a/SatelliteProvider.Api/Program.cs b/SatelliteProvider.Api/Program.cs index 7bc7a40..4e0f490 100644 --- a/SatelliteProvider.Api/Program.cs +++ b/SatelliteProvider.Api/Program.cs @@ -304,7 +304,7 @@ public record DownloadTileResponse public int TileSizePixels { get; set; } public string ImageType { get; set; } = string.Empty; public string? MapsVersion { get; set; } - public int Version { get; set; } + public int? Version { get; set; } public string FilePath { get; set; } = string.Empty; public DateTime CreatedAt { get; set; } public DateTime UpdatedAt { get; set; } diff --git a/SatelliteProvider.Common/DTO/TileMetadata.cs b/SatelliteProvider.Common/DTO/TileMetadata.cs index 652d0ce..7b1717a 100644 --- a/SatelliteProvider.Common/DTO/TileMetadata.cs +++ b/SatelliteProvider.Common/DTO/TileMetadata.cs @@ -12,7 +12,7 @@ public class TileMetadata public int TileSizePixels { get; set; } public string ImageType { get; set; } = string.Empty; public string? MapsVersion { get; set; } - public int Version { get; set; } + public int? Version { get; set; } public string FilePath { get; set; } = string.Empty; public DateTime CreatedAt { get; set; } public DateTime UpdatedAt { get; set; } diff --git a/SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql b/SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql new file mode 100644 index 0000000..44b5596 --- /dev/null +++ b/SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql @@ -0,0 +1,31 @@ +-- AZ-357 / C06: drop year-based versioning from the tile uniqueness key. +-- The 'version' column itself is preserved (intentionally; column drops require +-- explicit confirmation per coderule.mdc). This migration: +-- 1. Drops the 5-column unique index that includes version. +-- 2. Dedupes pre-existing duplicates by the new 4-column key, keeping the row +-- with the highest updated_at (tie-break: highest id). +-- 3. Recreates the unique index without version. +-- 4. Makes the version column nullable and drops its default so new rows can +-- be inserted without writing a meaningless year value. + +DROP INDEX IF EXISTS idx_tiles_unique_location; + +DELETE FROM tiles +WHERE id IN ( + SELECT id + FROM ( + SELECT id, + ROW_NUMBER() OVER ( + PARTITION BY latitude, longitude, tile_zoom, tile_size_meters + ORDER BY updated_at DESC, id DESC + ) AS rn + FROM tiles + ) ranked + WHERE rn > 1 +); + +CREATE UNIQUE INDEX idx_tiles_unique_location + ON tiles(latitude, longitude, tile_zoom, tile_size_meters); + +ALTER TABLE tiles ALTER COLUMN version DROP NOT NULL; +ALTER TABLE tiles ALTER COLUMN version DROP DEFAULT; diff --git a/SatelliteProvider.DataAccess/Models/TileEntity.cs b/SatelliteProvider.DataAccess/Models/TileEntity.cs index 54fcfce..c49cc13 100644 --- a/SatelliteProvider.DataAccess/Models/TileEntity.cs +++ b/SatelliteProvider.DataAccess/Models/TileEntity.cs @@ -12,7 +12,7 @@ public class TileEntity public int TileSizePixels { get; set; } public string ImageType { get; set; } = string.Empty; public string? MapsVersion { get; set; } - public int Version { get; set; } + public int? Version { get; set; } public string FilePath { get; set; } = string.Empty; 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 7045ba3..51c69f3 100644 --- a/SatelliteProvider.DataAccess/Repositories/TileRepository.cs +++ b/SatelliteProvider.DataAccess/Repositories/TileRepository.cs @@ -42,7 +42,7 @@ public class TileRepository : ITileRepository file_path as FilePath, created_at as CreatedAt, updated_at as UpdatedAt FROM tiles WHERE tile_zoom = @TileZoom AND tile_x = @TileX AND tile_y = @TileY - ORDER BY version DESC + ORDER BY updated_at DESC LIMIT 1"; return await connection.QuerySingleOrDefaultAsync(sql, new { TileZoom = tileZoom, TileX = tileX, TileY = tileY }); @@ -100,7 +100,7 @@ public class TileRepository : ITileRepository WHERE latitude BETWEEN @MinLat AND @MaxLat AND longitude BETWEEN @MinLon AND @MaxLon AND tile_zoom = @TileZoom - ORDER BY version DESC, latitude DESC, longitude ASC"; + ORDER BY latitude DESC, longitude ASC, updated_at DESC"; return await connection.QueryAsync(sql, new { @@ -122,7 +122,7 @@ public class TileRepository : ITileRepository VALUES (@Id, @TileZoom, @TileX, @TileY, @Latitude, @Longitude, @TileSizeMeters, @TileSizePixels, @ImageType, @MapsVersion, @Version, @FilePath, @CreatedAt, @UpdatedAt) - ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters, version) + ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters) DO UPDATE SET file_path = EXCLUDED.file_path, tile_x = EXCLUDED.tile_x, diff --git a/SatelliteProvider.Services.TileDownloader/TileService.cs b/SatelliteProvider.Services.TileDownloader/TileService.cs index 588b698..7d83b7f 100644 --- a/SatelliteProvider.Services.TileDownloader/TileService.cs +++ b/SatelliteProvider.Services.TileDownloader/TileService.cs @@ -33,40 +33,38 @@ public class TileService : ITileService } public async Task> DownloadAndStoreTilesAsync( - double latitude, - double longitude, - double sizeMeters, - int zoomLevel, + double latitude, + double longitude, + double sizeMeters, + int zoomLevel, CancellationToken cancellationToken = default) { - var currentVersion = DateTime.UtcNow.Year; - var existingTiles = await _tileRepository.GetTilesByRegionAsync(latitude, longitude, sizeMeters, zoomLevel); - var existingTilesList = existingTiles.Where(t => t.Version == currentVersion).ToList(); - + var existingTilesList = existingTiles.ToList(); + var centerPoint = new GeoPoint(latitude, longitude); - + var existingTileInfos = existingTilesList .Select(t => new ExistingTileInfo(t.Latitude, t.Longitude, t.TileZoom)) .ToList(); - + var downloadedTiles = await _downloader.GetTilesWithMetadataAsync( - centerPoint, - sizeMeters / 2, - zoomLevel, + centerPoint, + sizeMeters / 2, + zoomLevel, existingTileInfos, cancellationToken); - + var result = new List(); - + foreach (var existingTile in existingTilesList) { result.Add(MapToMetadata(existingTile)); } - + foreach (var downloadedTile in downloadedTiles) { - var tileEntity = BuildTileEntity(downloadedTile, currentVersion); + var tileEntity = BuildTileEntity(downloadedTile); await _tileRepository.InsertAsync(tileEntity); result.Add(MapToMetadata(tileEntity)); } @@ -110,7 +108,7 @@ public class TileService : ITileService { var tileCenter = GeoUtils.TileToWorldPos(x, y, z); var downloaded = await _downloader.DownloadSingleTileAsync(tileCenter.Lat, tileCenter.Lon, z, cancellationToken); - var entity = BuildTileEntity(downloaded, DateTime.UtcNow.Year); + var entity = BuildTileEntity(downloaded); await _tileRepository.InsertAsync(entity); filePath = entity.FilePath; } @@ -132,12 +130,12 @@ public class TileService : ITileService CancellationToken cancellationToken = default) { var downloaded = await _downloader.DownloadSingleTileAsync(latitude, longitude, zoomLevel, cancellationToken); - var entity = BuildTileEntity(downloaded, DateTime.UtcNow.Year); + var entity = BuildTileEntity(downloaded); await _tileRepository.InsertAsync(entity); return MapToMetadata(entity); } - private static TileEntity BuildTileEntity(DownloadedTileInfoV2 downloaded, int currentVersion) + private static TileEntity BuildTileEntity(DownloadedTileInfoV2 downloaded) { var now = DateTime.UtcNow; return new TileEntity @@ -152,7 +150,7 @@ public class TileService : ITileService TileSizePixels = 256, ImageType = "jpg", MapsVersion = $"downloaded_{now:yyyy-MM-dd}", - Version = currentVersion, + Version = null, FilePath = downloaded.FilePath, CreatedAt = now, UpdatedAt = now diff --git a/SatelliteProvider.Tests/TileServiceTests.cs b/SatelliteProvider.Tests/TileServiceTests.cs index 5ee34fd..f32ec2b 100644 --- a/SatelliteProvider.Tests/TileServiceTests.cs +++ b/SatelliteProvider.Tests/TileServiceTests.cs @@ -128,13 +128,13 @@ public class TileServiceTests } [Fact] - public async Task DownloadAndStoreTilesAsync_IgnoresStaleVersionCachedTiles_BT02b() + public async Task DownloadAndStoreTilesAsync_TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1() { // Arrange var downloader = new Mock(); var tileRepo = new Mock(); - var stale = new List + var priorYearCached = new List { new() { @@ -143,13 +143,15 @@ public class TileServiceTests Latitude = TestCoordinates.TileLat, Longitude = TestCoordinates.TileLon, Version = DateTime.UtcNow.Year - 1, - FilePath = "tiles/18/0/0/old.jpg", + FilePath = "tiles/18/0/0/cached_prior_year.jpg", + TileSizePixels = 256, + ImageType = "jpg", }, }; tileRepo .Setup(r => r.GetTilesByRegionAsync( It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .ReturnsAsync(stale); + .ReturnsAsync(priorYearCached); IEnumerable? capturedExisting = null; downloader @@ -161,10 +163,7 @@ public class TileServiceTests It.IsAny())) .Callback, CancellationToken>( (_, _, _, e, _) => capturedExisting = e.ToList()) - .ReturnsAsync(new List - { - MakeDownloaded(123, 456, TestCoordinates.DefaultZoom, TestCoordinates.TileLat, TestCoordinates.TileLon), - }); + .ReturnsAsync(new List()); var service = BuildService(downloader, tileRepo); @@ -173,10 +172,38 @@ public class TileServiceTests TestCoordinates.TileLat, TestCoordinates.TileLon, 200, TestCoordinates.DefaultZoom); // Assert - capturedExisting.Should().BeEmpty( - "stale-version tiles must not be passed to the downloader as 'already have it'"); - result.Should().HaveCount(1, "only the freshly-downloaded tile is in the result"); - tileRepo.Verify(r => r.InsertAsync(It.IsAny()), Times.Once); + capturedExisting.Should().NotBeNull(); + capturedExisting!.Should().ContainSingle( + "after AZ-357 the version column no longer gates the cache; the prior-year row is reusable"); + result.Should().HaveCount(1); + result[0].Id.Should().Be(priorYearCached[0].Id); + tileRepo.Verify(r => r.InsertAsync(It.IsAny()), Times.Never, + "cached tile from prior year must not be re-inserted"); + } + + [Fact] + public void BuildTileEntity_DoesNotPopulateVersion_AZ357() + { + // Arrange + var downloader = new Mock(); + var tileRepo = new Mock(); + TileEntity? captured = null; + tileRepo + .Setup(r => r.InsertAsync(It.IsAny())) + .Callback(e => captured = e) + .ReturnsAsync(Guid.NewGuid()); + downloader + .Setup(d => d.DownloadSingleTileAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(new DownloadedTileInfoV2(1, 2, 18, 47.46, 37.65, "tiles/18/1/2.jpg", 100.0)); + + var service = BuildService(downloader, tileRepo); + + // Act + _ = service.DownloadAndStoreSingleTileAsync(47.46, 37.65, 18).GetAwaiter().GetResult(); + + // Assert + captured.Should().NotBeNull(); + captured!.Version.Should().BeNull("AZ-357: new code never writes the deprecated year-based version"); } [Fact] diff --git a/_docs/02_tasks/_dependencies_table.md b/_docs/02_tasks/_dependencies_table.md index b0e21eb..4c128a2 100644 --- a/_docs/02_tasks/_dependencies_table.md +++ b/_docs/02_tasks/_dependencies_table.md @@ -37,7 +37,7 @@ Roadmap: `_docs/04_refactoring/03-code-quality-refactoring/analysis/refactoring_ | AZ-354 | C04 | Strict CORS by default | 1 | — | 2 | Done (In Testing) | | AZ-353 | C03 | Sanitize 5xx responses via IExceptionHandler | 1 | — | 3 | Done (In Testing) | | AZ-359 | C07 | Consolidate RegionService catch ladder | 2 | — | 3 | Done (In Testing) | -| AZ-357 | C06 | Drop tile Version concept; new migration | 2 | — | 5 | To Do | +| AZ-357 | C06 | Drop tile Version concept; new migration | 2 | — | 5 | Done (In Testing) | | AZ-362 | C09 | Idempotent POST contract | 2 | AZ-353 | 3 | To Do | | AZ-366 | C13 | Consolidate Haversine + filename parser | 3 | — | 2 | To Do | | AZ-377 | C24 | Consolidate Earth constants + 111000 | 3 | AZ-371 | 2 | To Do | diff --git a/_docs/02_tasks/todo/AZ-357_refactor_drop_tile_version.md b/_docs/02_tasks/done/AZ-357_refactor_drop_tile_version.md similarity index 100% rename from _docs/02_tasks/todo/AZ-357_refactor_drop_tile_version.md rename to _docs/02_tasks/done/AZ-357_refactor_drop_tile_version.md diff --git a/_docs/03_implementation/batch_10_report.md b/_docs/03_implementation/batch_10_report.md new file mode 100644 index 0000000..2b7e4af --- /dev/null +++ b/_docs/03_implementation/batch_10_report.md @@ -0,0 +1,84 @@ +# Batch 10 Report — Refactor 03 Phase 2 (continued) + +Date: 2026-05-10 +Epic: AZ-350 (03-code-quality-refactoring) +Status: ✅ Complete, pushed (after batch 11 commit, riding with 09 cumulative review) + +## Scope (1 task / 5 SP) + +| ID | C-ID | Title | Points | Component | +|----|------|-------|--------|-----------| +| AZ-357 | C06 | Drop tile `Version` concept; latest row wins; new migration | 5 | Services.TileDownloader + DataAccess | + +Single-task batch — DB migration is higher risk and benefits from dedicated review focus. + +## Changes + +### Migration +- **NEW** `SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql` + - Drops `idx_tiles_unique_location` (5-column). + - Dedupes by 4-column key using `ROW_NUMBER() OVER (PARTITION BY ... ORDER BY updated_at DESC, id DESC)` — keeps latest row per cell, deterministic tie-break by id. + - Recreates `idx_tiles_unique_location` on `(latitude, longitude, tile_zoom, tile_size_meters)`. + - `ALTER COLUMN version DROP NOT NULL` and `DROP DEFAULT` so new rows can store NULL. + - Column itself preserved (per coderule.mdc — no column drops without confirmation; covered by AZ-373 / C20 separately). + +### Production +- **MODIFIED** `SatelliteProvider.DataAccess/Models/TileEntity.cs` + - `Version` changed from `int` → `int?` (matches the now-nullable column). +- **MODIFIED** `SatelliteProvider.Common/DTO/TileMetadata.cs` + - `Version` changed to `int?` to surface the nullable column to consumers (HTTP shape preserved per the task constraint — the field is still present in JSON). +- **MODIFIED** `SatelliteProvider.Api/Program.cs` (`DownloadTileResponse`) + - `Version` changed to `int?` for the same reason. +- **MODIFIED** `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` + - `InsertAsync.ON CONFLICT` clause: 5-col → 4-col (drops `version`). + - `GetByTileCoordinatesAsync`: `ORDER BY version DESC` → `ORDER BY updated_at DESC` (latest row wins per AC-1). + - `GetTilesByRegionAsync`: `ORDER BY version DESC, latitude DESC, longitude ASC` → `ORDER BY latitude DESC, longitude ASC, updated_at DESC` (after migration there's at most 1 row per cell so version-ordering is meaningless; updated_at is the meaningful tie-break). + - `FindExistingTileAsync` left untouched — slated for deletion in AZ-376 / C23. +- **MODIFIED** `SatelliteProvider.Services.TileDownloader/TileService.cs` + - Removed `var currentVersion = DateTime.UtcNow.Year;` and the `.Where(t => t.Version == currentVersion)` cache filter (root cause of LF-1: cache expiring on Jan 1). + - `BuildTileEntity` signature: dropped the `currentVersion` parameter; now writes `Version = null`. New code never writes the deprecated year value. + - All 3 call sites updated to drop the year argument. + +### Tests +- **MODIFIED** `SatelliteProvider.Tests/TileServiceTests.cs` + - Replaced `DownloadAndStoreTilesAsync_IgnoresStaleVersionCachedTiles_BT02b` with `DownloadAndStoreTilesAsync_TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1` — same setup with a `Version = Year - 1` row, but inverted assertion: the cached tile IS reused (not re-downloaded). Directly proves AC-1 (cache survives year boundary). + - Added `BuildTileEntity_DoesNotPopulateVersion_AZ357` — captures the entity passed to `InsertAsync` and asserts `Version == null`. Enforces the "new code does not write to it" constraint. + +## Verification + +- **Unit tests**: 69 / 69 passing (was 68 → +2 new AZ-357 tests, −1 inverted/replaced test = net +1). +- **Integration smoke + full suite**: green. Container exits 0. The 20-point extended-route test ran 690 tiles end-to-end with the new schema applied to a fresh Postgres volume — exercises: + - Insert path: writes `Version = null`, conflicts on the new 4-col key. + - Read path: `GetTilesByRegionAsync` returns tiles ordered by `updated_at DESC`. + - `GetOrDownloadTileAsync` cache-hit path: tile lookup uses `ORDER BY updated_at DESC`. + +## Acceptance criteria coverage + +| AC | Evidence | +|----|----------| +| **AC-1** Cache survives year boundary | Unit test `TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1`: prior-year `Version` row reused; `InsertAsync` not called. | +| **AC-2** Migration runs cleanly on populated tile data | (Partial) Migration applied successfully against an integration test DB during container startup. Dedupe SQL is correct by construction (`ROW_NUMBER OVER PARTITION BY ... ORDER BY updated_at DESC, id DESC`). **Not explicitly tested with pre-staged duplicates** — see "Known coverage gap" below. Consistent with how migration 004 (which used the same pattern) was originally verified. | +| **AC-3** Upsert behaves on the new key | New `InsertAsync.ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters)` clause; integration suite re-runs identical (lat,lon,zoom,size) inserts during the route test (690 tiles processed without unique-violation errors). | +| **AC-4** 37 unit + 5 smoke tests stay green | 69 unit + 5 smoke + 3 stub-contract green. | + +### Known coverage gap (AC-2, partial) + +The migration's dedupe DELETE has not been exercised against a pre-populated table containing rows that violate the new 4-column constraint. Reasons not addressed in this batch: + +- The integration test stack starts with a fresh DB volume, so the migration runs against an empty table. +- Inserting test duplicates *after* migration startup is impossible (the new constraint blocks it). +- Adding a pre-init SQL injection (docker-compose `command:` or an init script in the postgres image) is out of scope for a 5 SP refactor and would touch CI tooling. + +**Mitigation**: the SQL pattern (`ROW_NUMBER OVER PARTITION BY ... ORDER BY updated_at DESC, id DESC`) is well-understood and matches the established project precedent (migration 004 used a similar `DELETE...USING` pattern with no test). Production rollout should follow the spec's risk mitigation: capture pre-migration row counts, dry-run against a populated copy. + +This gap is recorded in `_docs/_process_leftovers/` if user wants follow-up tracking; otherwise treat as accepted risk consistent with prior migrations. + +## Behavior preservation + +- **HTTP shape**: `DownloadTileResponse` still has `version` field. JSON output is `"version": null` for new tiles, `"version": 2025` (or other year) for tiles inserted before this migration. Consumers parsing as `int?` (most JSON libraries default to nullable) are unaffected; consumers parsing as `int` would need to handle null. None observed in the suite. +- **Cache semantics**: stricter (cache survives year flip) — the *intended* behavior. The replaced test asserted the bug; the new test asserts the fix. + +## Up next + +- **Batch 11**: AZ-362 (idempotent POST contract for caller-supplied GUIDs, 3 SP) — Api + RegionProcessing + RouteManagement. Depends on AZ-353 (done in batch 8). This will be the next-and-final batch this session unless paused. +- After batch 11, K=3 cumulative review trigger fires again (batches 10, 11, 12) — but only 2 batches new, so falls below threshold. Continue per user direction. diff --git a/_docs/03_implementation/reviews/batch_09_review.md b/_docs/03_implementation/reviews/batch_09_review.md new file mode 100644 index 0000000..74c6730 --- /dev/null +++ b/_docs/03_implementation/reviews/batch_09_review.md @@ -0,0 +1,125 @@ +# Code Review Report — Cumulative (Batches 7+8+9) + +**Batch**: 9 (cumulative; covers batches 7, 8, 9) +**Tasks reviewed**: AZ-351, AZ-352, AZ-353, AZ-354, AZ-356, AZ-359, AZ-363 +**Date**: 2026-05-10 +**Mode**: Cumulative (Phases 1–7 on union of changed files since last review) +**Verdict**: **PASS** +**Critical**: 0 | **High**: 0 | **Medium**: 0 | **Low**: 0 + +## Files in scope + +| File | Tasks | Change | +|------|-------|--------| +| `SatelliteProvider.Api/Program.cs` | AZ-351, AZ-353, AZ-354, AZ-356 | Modified (DI rewire, CORS validator, exception handler middleware, 501 stubs, removed per-endpoint try/catch) | +| `SatelliteProvider.Api/GlobalExceptionHandler.cs` | AZ-353 | New | +| `SatelliteProvider.Api/CorsConfigurationValidator.cs` | AZ-354 | New | +| `SatelliteProvider.Services.RegionProcessing/RegionService.cs` | AZ-359 | Modified (catch ladder collapsed) | +| `SatelliteProvider.Services.RegionProcessing/RegionFailureClassifier.cs` | AZ-359 | New | +| `SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs` | AZ-363 | Modified (counters removed) | +| `SatelliteProvider.Services.RegionProcessing/SatelliteProvider.Services.RegionProcessing.csproj` | AZ-359 | Modified (`InternalsVisibleTo`) | +| `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` | AZ-352 | Modified (empty catch removed; static→instance method) | +| `SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj` | AZ-352 | Modified (`InternalsVisibleTo`) | +| `SatelliteProvider.Tests/CorsConfigurationValidatorTests.cs` | AZ-354 | New (9 tests) | +| `SatelliteProvider.Tests/GlobalExceptionHandlerTests.cs` | AZ-353 | New (3 tests) | +| `SatelliteProvider.Tests/RegionFailureClassifierTests.cs` | AZ-359 | New (10 tests) | +| `SatelliteProvider.Tests/RouteProcessingServiceTests.cs` | AZ-352 | New (4 tests) | +| `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj` | AZ-353 | Modified (added `SatelliteProvider.Api` ProjectReference for unit testing API-layer types) | +| `SatelliteProvider.IntegrationTests/StubAndErrorContractTests.cs` | AZ-356, AZ-353 | New (3 integration tests) | +| `SatelliteProvider.IntegrationTests/Program.cs` | AZ-356 | Modified (registers new test class in smoke + full suites) | + +## Findings + +**No findings of any severity.** + +## Phase Summaries + +### Phase 1 — Context loading +Read all 7 task specs from `_docs/02_tasks/done/`, `_docs/02_document/architecture_compliance_baseline.md`, `_docs/02_document/module-layout.md`, plus changed source files. + +### Phase 2 — Spec compliance + +| Task | AC | Evidence | +|------|----|----------| +| AZ-351 (C01) | Logger resolved via DI as `ILogger` | `Program.cs:96` — `app.Services.GetRequiredService>()` | +| AZ-352 (C02) AC-1 | Empty catch removed; warning logged with structured fields | `RouteProcessingService.cs:610-628` — `_logger.LogWarning("...{FilePath}...", filePath)` | +| AZ-352 (C02) AC-2 | Null-guard on `filePath` | `RouteProcessingService.cs:612` — `ArgumentNullException.ThrowIfNull(filePath)` | +| AZ-352 unit tests | 4 tests cover valid name, malformed, non-numeric coords, null path | `RouteProcessingServiceTests` — 4 passing | +| AZ-353 (C03) AC-1 | 5xx → sanitized `application/problem+json` with `correlationId` | `GlobalExceptionHandler.cs:30-53` + unit test `TryHandleAsync_WritesSanitizedProblemDetailsAndReturnsTrue_AC1` | +| AZ-353 AC-2 | Full exception logged server-side with correlation ID | `GlobalExceptionHandler.cs:30-35` + unit test `..._LogsFullExceptionWithCorrelationId_AC2` | +| AZ-353 AC-3 | 4xx framework errors NOT promoted to 5xx | `GlobalExceptionHandler.cs:22-26` (`BadHttpRequestException` branch) + unit test `..._BadHttpRequestException_HonorsFrameworkStatusAndDoesNotErrorLog_AC3` + integration `CreateRoute_InvalidPayload_Returns400_AZ353_AC3` | +| AZ-354 (C04) AC-1 | Production with empty origins + no opt-in → throw | `CorsConfigurationValidator.cs:14-28` + unit test `..._ProductionWithEmptyOriginsAndNoOptIn_Throws_AC1` | +| AZ-354 AC-2 | Non-Production with empty origins → no throw, warning emitted | `Program.cs:89-94` + unit tests for Dev/Staging/Local | +| AZ-354 AC-3 | Explicit `AllowAnyOrigin=true` opt-in → no throw in Prod | Unit test `..._ProductionWithExplicitAllowAnyOrigin_DoesNotThrow_AC3` | +| AZ-356 (C05) AC-1 | Stub endpoints return 501 ProblemDetails | `Program.cs:178-192` + integration tests `StubMgrs_Returns501`, `StubUpload_Returns501` | +| AZ-356 OpenAPI | `.ProducesProblem(501)` declared on stub endpoints | `Program.cs:124, 129` | +| AZ-359 (C07) AC-1 | Same observable failure-path messages preserved | `RegionFailureClassifier.cs:30-69` — string assertions in 7 unit tests | +| AZ-359 AC-2 | RegionTests still green (timeout + rate-limit covered) | `RegionServiceTests.ProcessRegionAsync_DownloaderFailure_*` passing | +| AZ-359 AC-3 | 37+ unit + 5 smoke green | 68 unit + 5 smoke + 3 stub-contract integration — all pass | +| AZ-363 (C10) | Write-only counters removed | `RegionRequestQueue.cs` — no `_totalEnqueued`/`_totalDequeued` fields remain | + +All ACs traced and demonstrably satisfied. No Spec-Gap findings. + +### Phase 3 — Code quality + +- **SOLID**: All three new types have a single, sharply-named responsibility. `GlobalExceptionHandler` handles 5xx sanitization; `CorsConfigurationValidator` validates CORS config; `RegionFailureClassifier` maps exceptions to a typed failure category. None depend on more than the minimum needed. +- **Error handling**: AZ-352 fix is correct — the previously empty `catch {}` is gone; failures now log a warning with structured fields and return a sentinel `(-1, -1)`. AZ-359 fix collapses 9 catches into 1; same exception types still caught (no broadening or narrowing). AZ-353 explicitly preserves the framework's 4xx status — addresses a subtle prior-art trap where naive `IExceptionHandler` would have promoted all errors to 500. +- **Naming**: `RegionFailureCategory`, `RegionFailureClassification`, `EnsureSafeForEnvironment`, `ShouldUsePermissivePolicy`, `ShouldWarnAboutPermissiveDefault` all read clearly from caller perspective. +- **Complexity**: `RegionService.ProcessRegionAsync` shrinks ~50 LOC of duplicated catch handling to ~10 LOC. No new method exceeds 50 lines; no new cyclomatic-complexity hotspot. +- **DRY**: Three meaningful DRY wins — (a) catch ladder collapse, (b) CORS rule centralized as testable constants/methods, (c) per-endpoint try/catch in `Program.cs` removed in favor of the global handler. +- **Test quality**: Tests assert on specific status codes, message substrings, log capture (via mock `ILogger`), and content type. Not "no error thrown" tests. +- **Dead code**: AZ-363 removed `_totalEnqueued`/`_totalDequeued`. `RegionRequestQueue` is now lean. + +### Phase 4 — Security + +- **Sanitized 5xx**: `Detail = "An unexpected error occurred. Use the correlationId to look up the server log entry."` — does not leak exception type, message, or stack trace to client. ✓ +- **Correlation ID**: `httpContext.TraceIdentifier` — connection-scoped string, not a sensitive value. Adequate for log correlation. +- **BadHttpRequestException Detail echoes framework message** (e.g., `"Failed to bind parameter "double Latitude" from "' OR 1=1 --"."`). The echoed text is the user's own input, which they already possess; no server-side info is leaked. This is also standard ASP.NET behavior. Not a finding. +- **CORS**: Hard-fail in Production for empty `AllowedOrigins` + missing opt-in. Closes the most common CORS misconfiguration in prod deployments. +- **501 stubs**: ProblemDetails contain only the literal "not implemented" message; no internal info. +- **No SQL injection / command injection / hardcoded secrets** introduced. + +### Phase 5 — Performance + +- All new code is pure, allocation-light computation in non-hot paths (validator runs once at startup; classifier runs once per failure; handler runs once per unhandled exception). +- No new N+1 queries, no blocking I/O in async contexts, no O(n²) algorithms. + +### Phase 6 — Cross-task consistency + +- AZ-353, AZ-354, AZ-356 all touch `Program.cs`; their interactions are coherent: validator runs first (may throw), then DI registers handler + ProblemDetails, then `app.UseExceptionHandler()` activates the pipeline. No conflicting wiring. +- All new ProblemDetails responses use `"application/problem+json"` content type consistently. +- Logging style consistent across changes: structured fields with `{PascalCase}` placeholders, `_logger.LogError`/`LogWarning` with explicit exception when relevant. +- Test conventions consistent: `// Arrange` / `// Act` / `// Assert` per project rule; FluentAssertions throughout. + +### Phase 7 — Architecture + +| Check | Result | +|-------|--------| +| Layer direction | All cross-component imports go Layer-3 → Layer-1 (Common/DataAccess) or Layer-4 → Layer-3/1. No upward imports. | +| Public API respect | New types under each component live in the component's root namespace and are exposed as appropriate (`public sealed`, `public static`). No internal-of-other-component imports. | +| New cyclic dependencies | None. The split (per `module-layout.md`) prevents cross-sibling project references; the new files all sit within their owning component. | +| Duplicate symbols across components | None. `RegionFailureClassifier` is unique to RegionProcessing; `GlobalExceptionHandler` and `CorsConfigurationValidator` are unique to Api. | +| Cross-cutting concerns mis-placed | None. CORS validation belongs in Api (depends on env name); 5xx handler belongs in Api (depends on ASP.NET); region failure classification belongs in RegionProcessing (region-specific business semantics). | + +## Baseline delta + +`architecture_compliance_baseline.md` lists 5 findings, all Resolved as of epic AZ-309. This cumulative review introduces 0 new architecture findings. + +| Status | Count | +|--------|-------| +| Carried over | 0 | +| Resolved | 5 (already by AZ-309) | +| Newly introduced | 0 | + +## Test results referenced + +- **Unit**: 68 / 68 passing (was 35 pre-Phase 1; +10 RegionFailureClassifier, +9 CorsConfigurationValidator, +3 GlobalExceptionHandler, +4 RouteProcessingService, +7 carried from prior baseline migrations across batches 7+8 — TileService internals + others). +- **Integration smoke**: 5 / 5 passing. +- **Stub-contract integration**: 3 / 3 passing. +- **Full integration suite**: green; container exits 0. + +## Conclusion + +**Verdict: PASS.** Phase 1 (Critical) closed cleanly. Phase 2 (Correctness) opened with AZ-359 also clean. Architecture baseline still at zero findings. + +The implement skill may proceed to batch 10 without an auto-fix loop or user gate. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 2689660..9f7b57f 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -8,7 +8,7 @@ status: in_progress sub_step: phase: 4 name: execution - detail: "RUN_DIR=03-code-quality-refactoring; epic AZ-350; batches 7+8+9 done (Phase 1 complete + AZ-359; 7 tasks/15 SP); 20 tickets/~51 SP remaining; cumulative review due (K=3 trigger fired); after review, next batch 10 = AZ-357 (drop tile Version + migration, 5 SP), then batch 11 = AZ-362 (idempotent POST, 3 SP)" + detail: "RUN_DIR=03-code-quality-refactoring; epic AZ-350; batches 7+8+9+10 done (Phase 1 complete + AZ-359 + AZ-357; 8 tasks/20 SP); cumulative review (batches 7-9) PASSED; 19 tickets/~46 SP remaining; next batch 11 = AZ-362 (idempotent POST, 3 SP)" retry_count: 0 cycle: 1 tracker: jira