diff --git a/SatelliteProvider.IntegrationTests/MigrationTests.cs b/SatelliteProvider.IntegrationTests/MigrationTests.cs new file mode 100644 index 0000000..c00fd22 --- /dev/null +++ b/SatelliteProvider.IntegrationTests/MigrationTests.cs @@ -0,0 +1,173 @@ +using Npgsql; + +namespace SatelliteProvider.IntegrationTests; + +public static class MigrationTests +{ + public static async Task RunAll() + { + Console.WriteLine(); + Console.WriteLine("Test: Migration 012 (AZ-357 / C06)"); + Console.WriteLine("=================================="); + Console.WriteLine(); + + var connectionString = Environment.GetEnvironmentVariable("DB_CONNECTION_STRING") + ?? "Host=postgres;Port=5432;Database=satelliteprovider;Username=postgres;Password=postgres"; + + await DedupeSqlCollapsesDuplicatesByLatestUpdatedAt_AZ357_AC2(connectionString); + await NewUniqueConstraintExistsOnFourColumns_AZ357_AC2(connectionString); + + Console.WriteLine("✓ Migration 012 tests: PASSED"); + } + + private static async Task DedupeSqlCollapsesDuplicatesByLatestUpdatedAt_AZ357_AC2(string connectionString) + { + Console.WriteLine("AZ-357 AC-2 part 1: dedupe SQL keeps row with highest updated_at, tie-breaks on id"); + + // Arrange + await using var conn = new NpgsqlConnection(connectionString); + await conn.OpenAsync(); + // Session-scoped TEMP table (auto-dropped when the connection closes). + // Do NOT use ON COMMIT DROP — Npgsql commits implicitly after each command, + // which would drop the table before the next INSERT runs. + await ExecAsync(conn, """ + CREATE TEMP TABLE tiles_dedupe_test ( + id UUID PRIMARY KEY, + latitude DOUBLE PRECISION NOT NULL, + longitude DOUBLE PRECISION NOT NULL, + tile_zoom INT NOT NULL, + tile_size_meters DOUBLE PRECISION NOT NULL, + version INT, + updated_at TIMESTAMP NOT NULL + ); + """); + + // Three rows that all share (lat=10.0, lon=20.0, zoom=18, size=100): + // - row A: 2024 version, oldest updated_at -> should be deleted + // - row B: 2025 version, middle updated_at -> should be deleted + // - row C: 2026 version, newest updated_at -> should survive + // Two rows that share (lat=11.0, lon=21.0, zoom=18, size=100) but tie on updated_at: + // - row D: id larger, same updated_at as E -> should survive (id-tiebreak wins) + // - row E: id smaller, same updated_at as D -> should be deleted + // One unique row (lat=12.0, lon=22.0, zoom=18, size=100): + // - row F: should always survive + var idA = Guid.Parse("11111111-1111-1111-1111-111111111111"); + var idB = Guid.Parse("22222222-2222-2222-2222-222222222222"); + var idC = Guid.Parse("33333333-3333-3333-3333-333333333333"); + var idD = Guid.Parse("ffffffff-ffff-ffff-ffff-ffffffffffff"); + var idE = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"); + var idF = Guid.Parse("99999999-9999-9999-9999-999999999999"); + + await ExecAsync(conn, """ + INSERT INTO tiles_dedupe_test (id, latitude, longitude, tile_zoom, tile_size_meters, version, updated_at) VALUES + (@idA, 10.0, 20.0, 18, 100, 2024, '2024-06-01 00:00:00'), + (@idB, 10.0, 20.0, 18, 100, 2025, '2025-06-01 00:00:00'), + (@idC, 10.0, 20.0, 18, 100, 2026, '2026-06-01 00:00:00'), + (@idD, 11.0, 21.0, 18, 100, 2025, '2025-09-01 00:00:00'), + (@idE, 11.0, 21.0, 18, 100, 2026, '2025-09-01 00:00:00'), + (@idF, 12.0, 22.0, 18, 100, 2025, '2025-01-01 00:00:00'); + """, + ("idA", idA), ("idB", idB), ("idC", idC), ("idD", idD), ("idE", idE), ("idF", idF)); + + // Act — run the same dedupe pattern that 012_DropTileVersionConstraint.sql uses, against the temp table + await ExecAsync(conn, """ + DELETE FROM tiles_dedupe_test + 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_dedupe_test + ) ranked + WHERE rn > 1 + ); + """); + + // Assert + var survivors = await QueryGuidsAsync(conn, "SELECT id FROM tiles_dedupe_test ORDER BY id;"); + var expected = new HashSet { idC, idD, idF }; + var actual = new HashSet(survivors); + + if (!actual.SetEquals(expected)) + { + throw new Exception( + $"AZ-357 AC-2 dedupe failed.\n" + + $" Expected survivors: {string.Join(", ", expected)}\n" + + $" Actual survivors: {string.Join(", ", actual)}"); + } + + Console.WriteLine(" ✓ Dedupe collapsed 3-way duplicate to row with newest updated_at (idC)"); + Console.WriteLine(" ✓ Dedupe broke updated_at tie by largest id (idD survived, idE removed)"); + Console.WriteLine(" ✓ Unique row (idF) preserved"); + } + + private static async Task NewUniqueConstraintExistsOnFourColumns_AZ357_AC2(string connectionString) + { + Console.WriteLine(); + Console.WriteLine("AZ-357 AC-2 part 2: post-migration unique index has the new 4-column shape"); + + // Arrange / Act + await using var conn = new NpgsqlConnection(connectionString); + await conn.OpenAsync(); + + const string sql = @" + SELECT indexdef + FROM pg_indexes + WHERE schemaname = 'public' + AND tablename = 'tiles' + AND indexname = 'idx_tiles_unique_location';"; + + await using var cmd = new NpgsqlCommand(sql, conn); + var indexDef = (string?)await cmd.ExecuteScalarAsync(); + + // Assert + if (indexDef == null) + { + throw new Exception("AZ-357 AC-2: idx_tiles_unique_location does not exist on tiles table after migration 012"); + } + + // Expected shape after migration 012 — 4 cols, no version, UNIQUE + var lower = indexDef.ToLowerInvariant(); + if (!lower.Contains("unique")) + { + throw new Exception($"AZ-357 AC-2: idx_tiles_unique_location is not UNIQUE. Definition: {indexDef}"); + } + foreach (var col in new[] { "latitude", "longitude", "tile_zoom", "tile_size_meters" }) + { + if (!lower.Contains(col)) + { + throw new Exception($"AZ-357 AC-2: idx_tiles_unique_location missing column '{col}'. Definition: {indexDef}"); + } + } + if (lower.Contains("version")) + { + throw new Exception($"AZ-357 AC-2: idx_tiles_unique_location still includes 'version' column — migration did not drop it. Definition: {indexDef}"); + } + + Console.WriteLine($" ✓ Index present with new shape: {indexDef}"); + } + + private static async Task ExecAsync(NpgsqlConnection conn, string sql, params (string Name, object Value)[] parameters) + { + await using var cmd = new NpgsqlCommand(sql, conn); + foreach (var (name, value) in parameters) + { + cmd.Parameters.AddWithValue(name, value); + } + await cmd.ExecuteNonQueryAsync(); + } + + private static async Task> QueryGuidsAsync(NpgsqlConnection conn, string sql) + { + await using var cmd = new NpgsqlCommand(sql, conn); + await using var reader = await cmd.ExecuteReaderAsync(); + var result = new List(); + while (await reader.ReadAsync()) + { + result.Add(reader.GetGuid(0)); + } + return result; + } +} diff --git a/SatelliteProvider.IntegrationTests/Program.cs b/SatelliteProvider.IntegrationTests/Program.cs index 75c020b..1a00af3 100644 --- a/SatelliteProvider.IntegrationTests/Program.cs +++ b/SatelliteProvider.IntegrationTests/Program.cs @@ -68,6 +68,7 @@ class Program await ExtendedRouteTests.RunRouteWithTilesZipTest(httpClient); await SecurityTests.RunAll(httpClient); await StubAndErrorContractTests.RunAll(httpClient); + await MigrationTests.RunAll(); } static async Task RunFullSuite(HttpClient httpClient) @@ -87,6 +88,7 @@ class Program await SecurityTests.RunAll(httpClient); await StubAndErrorContractTests.RunAll(httpClient); + await MigrationTests.RunAll(); } static async Task WaitForApiReady(HttpClient httpClient, int maxRetries = 30) diff --git a/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj b/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj index 206b89a..7f966bb 100644 --- a/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj +++ b/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj @@ -7,4 +7,8 @@ enable + + + + diff --git a/_docs/03_implementation/batch_10_report.md b/_docs/03_implementation/batch_10_report.md index 2b7e4af..2f23320 100644 --- a/_docs/03_implementation/batch_10_report.md +++ b/_docs/03_implementation/batch_10_report.md @@ -57,21 +57,24 @@ Single-task batch — DB migration is higher risk and benefits from dedicated re | 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-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. | -### Known coverage gap (AC-2, partial) +### AC-2 follow-up (closed) -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 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: -- 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. +- **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. -**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. +**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. -This gap is recorded in `_docs/_process_leftovers/` if user wants follow-up tracking; otherwise treat as accepted risk consistent with prior migrations. +**Result**: AC-2 now fully covered. Tests run on every integration suite invocation (smoke + full), green. ## Behavior preservation diff --git a/docker-compose.tests.yml b/docker-compose.tests.yml index 74bba3c..b1a0987 100644 --- a/docker-compose.tests.yml +++ b/docker-compose.tests.yml @@ -17,12 +17,15 @@ services: environment: - API_URL=http://api:8080 - INTEGRATION_TESTS_MODE=${INTEGRATION_TESTS_MODE:-full} + - DB_CONNECTION_STRING=Host=postgres;Port=5432;Database=satelliteprovider;Username=postgres;Password=postgres volumes: - ./ready:/app/ready - ./tiles:/app/tiles depends_on: api: condition: service_started + postgres: + condition: service_healthy restart: "no" volumes: