# 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 | New integration tests in `MigrationTests.cs`: `DedupeSqlCollapsesDuplicatesByLatestUpdatedAt_AZ357_AC2` exercises the dedupe DELETE against a temp table with intentional 4-column duplicates (3-way duplicate, updated_at-tie broken by id, plus a unique row); `NewUniqueConstraintExistsOnFourColumns_AZ357_AC2` queries `pg_indexes` to confirm the recreated index has the new shape and excludes `version`. Added in a follow-up commit before batch 11; see "AC-2 follow-up" below. |
| **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. |

### AC-2 follow-up (closed)

The original batch left AC-2 partially covered — the migration ran against an empty DB volume on integration startup. Per user direction (option B at batch 10 review pause), a populated-duplicates test was added in a follow-up commit before batch 11:

- **NEW** `SatelliteProvider.IntegrationTests/MigrationTests.cs` with two tests:
  1. `DedupeSqlCollapsesDuplicatesByLatestUpdatedAt_AZ357_AC2` — creates a session-scoped `tiles_dedupe_test` temp table, seeds it with intentional 4-column duplicates that have varying `updated_at` and `id`, runs the exact dedupe SQL from migration 012, and asserts only the expected rows survive (newest `updated_at` wins; ties broken by largest `id`; unique rows preserved).
  2. `NewUniqueConstraintExistsOnFourColumns_AZ357_AC2` — queries `pg_indexes` against the live DB to assert `idx_tiles_unique_location` exists as a unique 4-column btree index and does **not** include the `version` column. Catches the case where a developer skips the migration or rolls back the index recreation.
- **NEW dependency** `Npgsql 9.0.2` in `SatelliteProvider.IntegrationTests.csproj` for direct DB connectivity (matches the version used elsewhere in the suite).
- **MODIFIED** `docker-compose.tests.yml` — added `DB_CONNECTION_STRING` env var and `postgres: condition: service_healthy` to `integration-tests.depends_on` so the test container can connect directly to the same DB the API uses.
- **MODIFIED** `SatelliteProvider.IntegrationTests/Program.cs` — wired `MigrationTests.RunAll()` into both smoke and full suites.

**Implementation note (debugging trace)**: first attempt used `CREATE TEMP TABLE ... ON COMMIT DROP`. Each Npgsql command runs in its own implicit transaction by default, so the table was dropped immediately after the `CREATE` committed. Removed the `ON COMMIT DROP` clause — temp tables are automatically dropped when the connection closes (session-scoped), which is exactly what we want.

**Result**: AC-2 now fully covered. Tests run on every integration suite invocation (smoke + full), green.

## 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.