diff --git a/_docs/03_implementation/reviews/batch_12_review.md b/_docs/03_implementation/reviews/batch_12_review.md new file mode 100644 index 0000000..3c23b82 --- /dev/null +++ b/_docs/03_implementation/reviews/batch_12_review.md @@ -0,0 +1,153 @@ +# Code Review Report — Cumulative (Batches 10+11+12) + +**Batch**: 12 (cumulative; covers batches 10, 11, 12) +**Tasks reviewed**: AZ-357 (drop tile Version + AC-2 follow-up), AZ-362 (idempotent POST), AZ-366 (Haversine consolidation) +**Date**: 2026-05-10 +**Mode**: Cumulative (Phases 1–7 on union of changed files since last review at batch 9) +**Verdict**: **PASS** +**Critical**: 0 | **High**: 0 | **Medium**: 0 | **Low**: 1 (informational, non-blocking) + +## Files in scope + +| File | Tasks | Change | +|------|-------|--------| +| `SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql` | AZ-357 | New (drop 5-col index, dedupe, recreate 4-col index, version nullable) | +| `SatelliteProvider.DataAccess/Models/TileEntity.cs` | AZ-357 | Modified (`Version` → `int?`) | +| `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` | AZ-357 | Modified (4-col upsert; `ORDER BY updated_at DESC`) | +| `SatelliteProvider.Services.TileDownloader/TileService.cs` | AZ-357 | Modified (removed year-version filter; `BuildTileEntity` writes `Version = null`) | +| `SatelliteProvider.Common/DTO/TileMetadata.cs` | AZ-357 | Modified (`Version` → `int?`) | +| `SatelliteProvider.Api/Program.cs` | AZ-357, AZ-362 | Modified (`DownloadTileResponse.Version` → `int?`; OpenAPI Description for idempotent POSTs) | +| `SatelliteProvider.Tests/TileServiceTests.cs` | AZ-357 | Modified (replaced stale-version test with `TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1`; added `BuildTileEntity_DoesNotPopulateVersion_AZ357`) | +| `SatelliteProvider.IntegrationTests/MigrationTests.cs` | AZ-357 AC-2 follow-up | New (populated-duplicates dedupe + index-shape verification) | +| `SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj` | AZ-357 AC-2 follow-up | Modified (Npgsql 9.0.2) | +| `docker-compose.tests.yml` | AZ-357 AC-2 follow-up | Modified (DB_CONNECTION_STRING env var, postgres healthcheck dependency) | +| `SatelliteProvider.Services.RegionProcessing/RegionService.cs` | AZ-362 | Modified (early-return idempotency check in `RequestRegionAsync`) | +| `SatelliteProvider.Services.RouteManagement/RouteService.cs` | AZ-362 | Modified (early-return idempotency check in `CreateRouteAsync`) | +| `SatelliteProvider.Tests/RegionServiceTests.cs` | AZ-362 | Modified (+1 unit test asserting no AddAsync/EnqueueAsync on duplicate id) | +| `SatelliteProvider.Tests/RouteServiceTests.cs` | AZ-362 | Modified (+1 unit test asserting no InsertRouteAsync/RegionRequest on duplicate id) | +| `SatelliteProvider.IntegrationTests/IdempotentPostTests.cs` | AZ-362 | New (2 end-to-end HTTP tests) | +| `SatelliteProvider.IntegrationTests/Program.cs` | AZ-357 AC-2, AZ-362 | Modified (registers MigrationTests + IdempotentPostTests in smoke + full suites) | +| `SatelliteProvider.Common/Configs/StorageConfig.cs` | AZ-366 | Modified (added `static TryExtractTileCoordinates` co-located with `GetTileFilePath`) | +| `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` | AZ-366 | Modified (deleted local `CalculateDistance`; `ExtractTileCoordinatesFromFilename` is now thin wrapper over StorageConfig helper) | +| `SatelliteProvider.Tests/StorageConfigTests.cs` | AZ-366 | New (6 tests including writer-parser round-trip for AC-2) | + +## Findings + +### LOW-1 — AZ-362 idempotency check sits above validation (informational) + +**Severity**: Low (informational, not blocking) +**Location**: `RouteService.cs:32-39` (idempotency check), `RouteService.cs:41-54` (validation) +**Observation**: For both region POST and route POST, the idempotency early-return runs *before* input validation. A client retrying a duplicate id with a now-invalid payload will receive 200 + the original resource instead of 400 + validation errors. +**Reason this is not a defect**: This matches the documented idempotency contract (the persisted resource wins; clients should not mutate the body across retries). The only payload that creates the resource is the original one, which by definition was valid. Reordering would mean validating a payload that's about to be ignored. +**Recommendation**: No change. Documented in batch 11 report. Consider mentioning it explicitly in OpenAPI Description if a future client integration surfaces the surprise. + +## Phase Summaries + +### Phase 1 — Context loading +Read all 3 task descriptions from Jira (AZ-357, AZ-362, AZ-366), the corresponding batch reports (10, 11, 12), the AZ-357 task spec at `_docs/02_tasks/done/`, and the changed source files end-to-end. Verified prior cumulative review (batch 09) baseline. + +### Phase 2 — Spec compliance + +| Task | AC | Evidence | +|------|----|----------| +| **AZ-357 (C06) AC-1** Cache survives year boundary | Unit test `TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1` passes; year-version filter has been removed from `TileService.DownloadAndStoreTilesAsync` (grep for `currentVersion` returns 0 hits). | +| **AZ-357 AC-2** Migration runs cleanly on populated tile data | Two integration tests in `MigrationTests.cs`: dedupe SQL test seeds 6 rows including a 3-way duplicate + an updated_at tie + a unique row, runs the dedupe SQL from migration 012, asserts only 3 rows survive (correct winners by `updated_at DESC, id DESC`); index-shape test queries `pg_indexes` to confirm `idx_tiles_unique_location` is unique on `(latitude, longitude, tile_zoom, tile_size_meters)`. Both green. | +| **AZ-357 AC-3** Upsert behaves on the new key | `TileRepository.InsertAsync` uses `ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters)`. End-to-end full integration suite re-inserts 690+ tiles for the same region across multiple test runs without unique-violation errors. | +| **AZ-357 AC-4** All tests stay green | 77 unit + smoke + full integration green. | +| **AZ-362 (C09) AC-1** Region duplicate-id POST → 200 + existing resource | Unit `RequestRegionAsync_DuplicateId_ReturnsExistingResource_NoReQueue_AZ362_AC1` (verifies repo + queue are not invoked); integration `RegionPost_SameIdTwice_BothReturn200_NoDuplicateProcessing_AZ362_AC1` (HTTP shape + same CreatedAt + log line). | +| **AZ-362 AC-2** Route duplicate-id POST → 200 + existing resource | Unit `CreateRouteAsync_DuplicateId_ReturnsExistingRoute_NoReinsertion_AZ362_AC2`; integration `RoutePost_SameIdTwice_BothReturn200_NoReinsertion_AZ362_AC2` (TotalPoints unchanged ≡ no point regeneration). | +| **AZ-362 AC-3** OpenAPI documents idempotency | `Program.cs:151-155, 169-173` — `Description` strings on both `MapPost` registrations explicitly call out AZ-362 contract. | +| **AZ-362 AC-4** 4xx validation paths preserved | `CreateRoute_InvalidPayload_Returns400_AZ353_AC3` integration test still green — non-duplicate-id POSTs still hit `request.Points.Count < 2` etc. | +| **AZ-366 (C13) AC-1** One Haversine in the codebase | Grep for `Math.Sin(dLat/2)` returns matches only at `GeoUtils.cs:28`. Grep for `EARTH_RADIUS` returns matches only in `GeoUtils.cs`. Grep for the deleted 4-arg signature `CalculateDistance(double, double, double, double)` returns 0 hits. | +| **AZ-366 AC-2** Writer + parser co-located | Both `GetTileFilePath` and `TryExtractTileCoordinates` live as methods on `StorageConfig` in `SatelliteProvider.Common/Configs/StorageConfig.cs`. New unit test `GetTileFilePath_RoundTrip_ParserRecoversOriginalCoordinates_AZ366_AC2` mechanically proves the inverse-pair invariant. | +| **AZ-366 AC-3** Tests stay green | 77 unit (4 pre-existing AZ-352 ExtractTileCoordinatesFromFilename tests still pass against the now-thin wrapper). | + +All ACs traced and demonstrably satisfied. + +### Phase 3 — Code quality + +- **SOLID — Single Responsibility**: + - `RegionFailureClassifier` (batch 9) and `StorageConfig.TryExtractTileCoordinates` (batch 12) are textbook SRP wins — single-purpose, easily named, easily tested. + - `RegionService.RequestRegionAsync` and `RouteService.CreateRouteAsync` (batch 11) gain one new responsibility each (idempotency check). The check is small, named in the comment, and lives at the top so the existing creation logic is visually separated. Acceptable. + - `RouteProcessingService.ExtractTileCoordinatesFromFilename` (batch 12) is now a 9-line wrapper. Thin enough that future deletion (once all callers move to the static helper directly) would be trivial — flagged as future cleanup, not a current defect. +- **Error handling**: + - AZ-357 migration uses `DROP INDEX IF EXISTS` for idempotency under DbUp's tracking, deterministic dedupe ordering (`updated_at DESC, id DESC`), and sequenced ALTER COLUMN clauses that don't conflict. + - AZ-362 leaves the underlying repository path unchanged — if a TOCTOU race fires between the `GetByIdAsync` check and `InsertAsync`, the unique-key `PostgresException` propagates up to the AZ-353 global handler. No silent suppression introduced. + - AZ-366 parser returns `(-1, -1)` sentinel only for the documented format-error path; null path still throws `ArgumentNullException`. The wrapper still logs the warning. Strictly equivalent to the prior AZ-352 behavior from a caller's perspective. +- **Naming**: + - `TryExtractTileCoordinates` follows the .NET `Try*` convention (returns bool, uses `out` params for the parsed values, `(-1, -1)` sentinel preserved on the wrapper for backward-compat). + - `RoutePost_SameIdTwice_BothReturn200_NoReinsertion_AZ362_AC2` — long, but specific, and the pattern is consistent across the new test suite. +- **Complexity**: + - `RouteProcessingService.cs` shrinks ~30 LOC across the two deleted/wrapped methods. + - `RegionService.cs` and `RouteService.cs` each grow ~10 LOC for the idempotency early-return; small, localized. + - No new method exceeds 50 lines; no cyclomatic-complexity hotspot introduced. +- **DRY**: + - AZ-366 removed a duplicate Haversine. Single source of truth: `GeoUtils`. + - AZ-366 removed the by-string-convention coupling between `StorageConfig.GetTileFilePath` (writer) and `RouteProcessingService.ExtractTileCoordinatesFromFilename` (parser). Coupling is now structural. +- **Test quality**: + - 8 new tests across the window (6 in `StorageConfigTests` + 2 in unit suite for AZ-362). All assert on specific values, mock interactions, and observable state — no "no exception thrown" tests. + - `MigrationTests` proves the SQL DELETE shape against a populated table — directly closes the gap that batch 10 had flagged. + - `IdempotentPostTests` use a 1ms tolerance on `CreatedAt` to account for PostgreSQL's microsecond truncation vs .NET's 100ns ticks — explicitly documented in the test comment. + - All tests follow the project's `// Arrange / // Act / // Assert` pattern. +- **Dead code**: + - AZ-366 deleted `RouteProcessingService.CalculateDistance(double, double, double, double)` and ate one inline call site. + - The `version` column in the `tiles` table is now nullable but preserved — explicitly per `coderule.mdc` ("Do not rename any databases or tables or table columns without confirmation"). Future cleanup for that column is tracked as AZ-373 (C20 — clarify or drop MapsVersion / Version). + - The unused `FindExistingTileAsync` in `TileRepository` is still present — slated for AZ-376 (C23) deletion. Out of scope for this window. + +### Phase 4 — Security + +- **AZ-357**: No new attack surface. Migration runs at startup with the application's own DB credentials; no user input flows in. The dedupe SQL uses parameterless `ROW_NUMBER OVER (PARTITION BY ...)` so SQL injection is structurally impossible. +- **AZ-362**: No new attack surface. The idempotency check uses `Guid` (not string) — no injection possible. The check is read-only (does not mutate state for the duplicate path) so it cannot be used to enumerate existing resources beyond what the caller already authenticated to know (the `id` they own). +- **AZ-366**: Pure string parsing. `Path.GetFileNameWithoutExtension` is allocation-only; no I/O. No PII or secrets cross the boundary. +- **No CORS, auth, or 5xx-leakage regressions** introduced. The 4xx vs 5xx contract from AZ-353 is preserved. + +### Phase 5 — Performance + +- **AZ-357**: Migration runs once at startup. Production rollout might need a maintenance window for the dedupe DELETE on a large tiles table, but the SQL pattern itself uses `ROW_NUMBER OVER PARTITION BY` (single seq scan + sort) — acceptable for the scale documented in baseline metrics. +- **AZ-362**: Adds one extra `GetByIdAsync` call per POST (the duplicate-detection read). For an `INSERT`-heavy workload this is one extra read per write — acceptable; primary-key lookup is O(log n) on the index. Could be optimized with `INSERT ... ON CONFLICT DO NOTHING + RETURNING`, but that's an optimization for later (TOCTOU window note in the batch report explicitly acknowledges this). +- **AZ-366**: Net zero performance change. Same Haversine math, same parsing logic, same single call sites. + +### Phase 6 — Cross-task consistency + +- All three batches touch `SatelliteProvider.IntegrationTests/Program.cs`. The registrations are non-conflicting and follow the established pattern (one `await NewTestClass.RunAll(...)` per suite). +- Logging style is consistent: structured fields with `{PascalCase}` placeholders, `_logger.LogInformation` for happy-path observability (idempotency hits), `_logger.LogWarning` for parse failures. +- Test file naming follows the established `Tests.cs` convention; new test methods follow `MethodName_Condition_Expectation_AZxxx_ACy` pattern. +- DTO and OpenAPI changes from AZ-357 and AZ-362 both touch `Program.cs` but in non-overlapping regions. + +### Phase 7 — Architecture + +| Check | Result | +|-------|--------| +| Layer direction | All cross-component imports go bottom-up. AZ-366's `RouteProcessingService` (Services.RouteManagement) imports `StorageConfig` (Common). No reversed direction. | +| Public API respect | New public symbols: `StorageConfig.TryExtractTileCoordinates` (Common). New `int?` shape on existing public DTOs (`TileMetadata.Version`, `DownloadTileResponse.Version`) — backward compatible for JSON consumers; explicit shape change documented in batch 10 report. | +| New cyclic dependencies | None. The split (per `module-layout.md`) is preserved. | +| Duplicate symbols across components | None. `TryExtractTileCoordinates` is unique to Common; idempotency checks are private behavior of their respective services. | +| Cross-cutting concerns mis-placed | None. Migration belongs in DataAccess; idempotency checks belong in the services that own the resource lifecycle; tile-filename parser belongs next to the tile-filename writer in Common.Configs. | + +## Baseline delta + +`architecture_compliance_baseline.md` lists 5 findings, all Resolved as of epic AZ-309 (per the prior cumulative review). This cumulative review introduces 0 new architecture findings. + +| Status | Count | +|--------|-------| +| Carried over | 0 | +| Resolved | 5 (already by AZ-309) | +| Newly introduced | 0 | +| Newly identified non-blocking observations | 1 (LOW-1 above) | + +## Test results referenced + +- **Unit**: 77 / 77 passing (was 71 at end of batch 11 → +6 new `StorageConfigTests`). +- **Integration smoke**: 5 / 5 passing. +- **Stub-contract integration**: 3 / 3 passing. +- **Idempotent POST integration**: 2 / 2 passing. +- **Migration integration**: 2 / 2 passing. +- **Full integration suite**: green; container exits 0. + +## Conclusion + +**Verdict: PASS.** Phase 2 (Correctness) closed cleanly across AZ-357 + AZ-362 + AZ-366. Phase 3 (Structural cleanup) opened with AZ-366 also clean. Architecture baseline still at zero findings. + +The single LOW-1 observation (idempotency check above validation) is informational and matches the documented contract — no remediation required. + +The implement skill may proceed to batch 13 (AZ-368 / C15 TileCsvWriter) without an auto-fix loop or user gate.