diff --git a/SatelliteProvider.Common/Configs/MapConfig.cs b/SatelliteProvider.Common/Configs/MapConfig.cs index 613b485..1f4a987 100644 --- a/SatelliteProvider.Common/Configs/MapConfig.cs +++ b/SatelliteProvider.Common/Configs/MapConfig.cs @@ -6,7 +6,11 @@ public class MapConfig public string ApiKey { get; set; } = null!; // AZ-371 / C18 — Google Maps tile constants promoted from source literals. - public int TileSizePixels { get; set; } = 256; + // AZ-377 / C24 — DefaultTileSizePixels is the canonical pixel size of a Google Maps tile; + // sites that cannot inject IOptions (e.g. DataAccess.TileRepository) reference it + // directly so the value still has one source of truth. + public const int DefaultTileSizePixels = 256; + public int TileSizePixels { get; set; } = DefaultTileSizePixels; public int[] AllowedZoomLevels { get; set; } = new[] { 15, 16, 17, 18, 19 }; public int RetryBaseDelaySeconds { get; set; } = 1; public int RetryMaxDelaySeconds { get; set; } = 30; diff --git a/SatelliteProvider.Common/Utils/GeoUtils.cs b/SatelliteProvider.Common/Utils/GeoUtils.cs index 4202998..0152e05 100644 --- a/SatelliteProvider.Common/Utils/GeoUtils.cs +++ b/SatelliteProvider.Common/Utils/GeoUtils.cs @@ -4,7 +4,9 @@ namespace SatelliteProvider.Common.Utils; public static class GeoUtils { - private const double EARTH_RADIUS = 6378137; + public const double EarthRadiusMeters = 6378137d; + public const double EarthEquatorialCircumferenceMeters = 40075016.686d; + public const double MetersPerDegreeLatitude = 111000d; public static (int x, int y) WorldToTilePos(GeoPoint point, int zoom) { @@ -29,7 +31,7 @@ public static class GeoUtils Math.Cos(lat1Rad) * Math.Cos(lat2Rad) * Math.Sin(dLon / 2) * Math.Sin(dLon / 2); var c = 2 * Math.Asin(Math.Sqrt(a)); - var distance = EARTH_RADIUS * c; + var distance = EarthRadiusMeters * c; var y = Math.Sin(dLon) * Math.Cos(lat2Rad); var x = Math.Cos(lat1Rad) * Math.Sin(lat2Rad) - @@ -46,7 +48,7 @@ public static class GeoUtils public static GeoPoint GoDirection(this GeoPoint startPoint, Direction direction) { - var angularDistance = direction.Distance / EARTH_RADIUS; + var angularDistance = direction.Distance / EarthRadiusMeters; var azimuthRadians = ToRadians(direction.Azimuth); var startLatRad = ToRadians(startPoint.Lat); var startLonRad = ToRadians(startPoint.Lon); @@ -73,11 +75,11 @@ public static class GeoUtils { var latRad = centerGeoPoint.Lat * Math.PI / 180.0; - var latDiff = (radiusM / EARTH_RADIUS) * (180.0 / Math.PI); + var latDiff = (radiusM / EarthRadiusMeters) * (180.0 / Math.PI); var minLat = Math.Max(centerGeoPoint.Lat - latDiff, -90.0); var maxLat = Math.Min(centerGeoPoint.Lat + latDiff, 90.0); - var lonDiff = (radiusM / (EARTH_RADIUS * Math.Cos(latRad))) * (180.0 / Math.PI); + var lonDiff = (radiusM / (EarthRadiusMeters * Math.Cos(latRad))) * (180.0 / Math.PI); var minLon = Math.Max(centerGeoPoint.Lon - lonDiff, -180.0); var maxLon = Math.Min(centerGeoPoint.Lon + lonDiff, 180.0); diff --git a/SatelliteProvider.DataAccess/Repositories/TileRepository.cs b/SatelliteProvider.DataAccess/Repositories/TileRepository.cs index 5d92a12..a93d03e 100644 --- a/SatelliteProvider.DataAccess/Repositories/TileRepository.cs +++ b/SatelliteProvider.DataAccess/Repositories/TileRepository.cs @@ -2,6 +2,8 @@ using System.Diagnostics; using Dapper; using Microsoft.Extensions.Logging; using Npgsql; +using SatelliteProvider.Common.Configs; +using SatelliteProvider.Common.Utils; using SatelliteProvider.DataAccess.Models; namespace SatelliteProvider.DataAccess.Repositories; @@ -53,16 +55,14 @@ public class TileRepository : ITileRepository { using var connection = new NpgsqlConnection(_connectionString); - const double EARTH_CIRCUMFERENCE_METERS = 40075016.686; - const int TILE_SIZE_PIXELS = 256; var latRad = latitude * Math.PI / 180.0; - var metersPerPixel = (EARTH_CIRCUMFERENCE_METERS * Math.Cos(latRad)) / (Math.Pow(2, zoomLevel) * TILE_SIZE_PIXELS); - var tileSizeMeters = metersPerPixel * TILE_SIZE_PIXELS; + var metersPerPixel = (GeoUtils.EarthEquatorialCircumferenceMeters * Math.Cos(latRad)) / (Math.Pow(2, zoomLevel) * MapConfig.DefaultTileSizePixels); + var tileSizeMeters = metersPerPixel * MapConfig.DefaultTileSizePixels; var expandedSizeMeters = sizeMeters + (tileSizeMeters * 2); - var latRange = expandedSizeMeters / 111000.0; - var lonRange = expandedSizeMeters / (111000.0 * Math.Cos(latitude * Math.PI / 180.0)); + var latRange = expandedSizeMeters / GeoUtils.MetersPerDegreeLatitude; + var lonRange = expandedSizeMeters / (GeoUtils.MetersPerDegreeLatitude * Math.Cos(latitude * Math.PI / 180.0)); const string sql = $@" SELECT {ColumnList} diff --git a/SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs b/SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs index fa042b6..2784ae4 100644 --- a/SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs +++ b/SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs @@ -138,10 +138,9 @@ public class GoogleMapsDownloaderV2 : ISatelliteDownloader private double CalculateTileSizeInMeters(int zoomLevel, double latitude) { - const double EARTH_CIRCUMFERENCE_METERS = 40075016.686; var tileSizePixels = _mapConfig.TileSizePixels; var latRad = latitude * Math.PI / 180.0; - var metersPerPixel = (EARTH_CIRCUMFERENCE_METERS * Math.Cos(latRad)) / (Math.Pow(2, zoomLevel) * tileSizePixels); + var metersPerPixel = (GeoUtils.EarthEquatorialCircumferenceMeters * Math.Cos(latRad)) / (Math.Pow(2, zoomLevel) * tileSizePixels); return metersPerPixel * tileSizePixels; } @@ -239,6 +238,14 @@ public class GoogleMapsDownloaderV2 : ISatelliteDownloader var (xMin, yMin) = GeoUtils.WorldToTilePos(new GeoPoint(latMax, lonMin), zoomLevel); var (xMax, yMax) = GeoUtils.WorldToTilePos(new GeoPoint(latMin, lonMax), zoomLevel); + var existingTileKeys = new HashSet<(int X, int Y, int Z)>(); + foreach (var t in existingTiles) + { + if (t.TileZoom != zoomLevel) continue; + var (etx, ety) = GeoUtils.WorldToTilePos(new GeoPoint(t.Latitude, t.Longitude), t.TileZoom); + existingTileKeys.Add((etx, ety, t.TileZoom)); + } + var tilesToDownload = new List<(int x, int y, GeoPoint center, double tileSizeMeters)>(); int skippedCount = 0; @@ -246,20 +253,13 @@ public class GoogleMapsDownloaderV2 : ISatelliteDownloader { for (var x = xMin; x <= xMax; x++) { - var tileCenter = GeoUtils.TileToWorldPos(x, y, zoomLevel); - - var tolerance = _processingConfig.LatLonTolerance; - var existingTile = existingTiles.FirstOrDefault(t => - Math.Abs(t.Latitude - tileCenter.Lat) < tolerance && - Math.Abs(t.Longitude - tileCenter.Lon) < tolerance && - t.TileZoom == zoomLevel); - - if (existingTile != null) + if (existingTileKeys.Contains((x, y, zoomLevel))) { skippedCount++; continue; } + var tileCenter = GeoUtils.TileToWorldPos(x, y, zoomLevel); var tileSizeMeters = CalculateTileSizeInMeters(zoomLevel, tileCenter.Lat); tilesToDownload.Add((x, y, tileCenter, tileSizeMeters)); } diff --git a/SatelliteProvider.Tests/DownloaderRefactorTests.cs b/SatelliteProvider.Tests/DownloaderRefactorTests.cs new file mode 100644 index 0000000..974e42a --- /dev/null +++ b/SatelliteProvider.Tests/DownloaderRefactorTests.cs @@ -0,0 +1,115 @@ +using FluentAssertions; +using SatelliteProvider.Common.Utils; + +namespace SatelliteProvider.Tests; + +public class DownloaderRefactorTests +{ + [Fact] + public void GoogleMapsDownloaderV2_UsesHashSetForExistingTileLookup_AZ375_AC1() + { + // Arrange + var path = LocateRepoFile(Path.Combine( + "SatelliteProvider.Services.TileDownloader", "GoogleMapsDownloaderV2.cs")); + path.Should().NotBeNull("GoogleMapsDownloaderV2.cs must be present in the workspace for this assertion"); + var content = File.ReadAllText(path!); + + // Assert + content.Should().NotContain("existingTiles.FirstOrDefault", + "AZ-375 replaces the per-tile FirstOrDefault scan with a HashSet membership check"); + content.Should().NotContain("_processingConfig.LatLonTolerance", + "AZ-375 removes the float-tolerance comparison at the existing-tile lookup site"); + content.Should().Contain("HashSet<(int X, int Y, int Z)>", + "AZ-375 introduces a typed HashSet keyed by tile (X, Y, Z) for O(1) lookup"); + content.Should().Contain("existingTileKeys.Contains((x, y, zoomLevel))", + "the inner loop must consult the precomputed HashSet for skip decisions"); + } + + [Fact] + public void GoogleMapsDownloaderV2_UsesGeoUtilsCircumferenceConstant_AZ377_AC1() + { + // Arrange + var path = LocateRepoFile(Path.Combine( + "SatelliteProvider.Services.TileDownloader", "GoogleMapsDownloaderV2.cs")); + path.Should().NotBeNull(); + var content = File.ReadAllText(path!); + + // Assert + content.Should().NotContain("EARTH_CIRCUMFERENCE_METERS", + "AZ-377 removes the duplicated local Earth circumference literal"); + content.Should().NotContain("40075016.686", + "AZ-377 forbids duplicate Earth circumference literals outside GeoUtils"); + content.Should().Contain("GeoUtils.EarthEquatorialCircumferenceMeters", + "AZ-377 routes the downloader through the canonical GeoUtils constant"); + } + + [Fact] + public void TileRepository_UsesGeoUtilsAndMapConfigConstants_AZ377_AC1() + { + // Arrange + var path = LocateRepoFile(Path.Combine( + "SatelliteProvider.DataAccess", "Repositories", "TileRepository.cs")); + path.Should().NotBeNull(); + var content = File.ReadAllText(path!); + + // Assert + content.Should().NotContain("EARTH_CIRCUMFERENCE_METERS"); + content.Should().NotContain("TILE_SIZE_PIXELS = 256"); + content.Should().NotContain("40075016.686", + "AZ-377 forbids duplicate Earth circumference literals outside GeoUtils"); + content.Should().NotContain("111000.0", + "AZ-377 forbids duplicate per-degree-latitude literals outside GeoUtils"); + content.Should().Contain("GeoUtils.EarthEquatorialCircumferenceMeters"); + content.Should().Contain("GeoUtils.MetersPerDegreeLatitude"); + content.Should().Contain("MapConfig.DefaultTileSizePixels"); + } + + [Fact] + public void GeoUtils_IsTheSoleHolderOfRawEarthLiterals_AZ377_AC1() + { + // Arrange + var geoUtilsPath = LocateRepoFile(Path.Combine( + "SatelliteProvider.Common", "Utils", "GeoUtils.cs")); + geoUtilsPath.Should().NotBeNull(); + var content = File.ReadAllText(geoUtilsPath!); + + // Assert + content.Should().Contain("public const double EarthRadiusMeters = 6378137"); + content.Should().Contain("public const double EarthEquatorialCircumferenceMeters = 40075016.686"); + content.Should().Contain("public const double MetersPerDegreeLatitude = 111000"); + } + + [Fact] + public void HaversineDistance_WithTinyDelta_UsesEarthRadiusConstantConsistently_AZ377_AC2() + { + // Arrange + var p1 = new SatelliteProvider.Common.DTO.GeoPoint(50.4501, 30.5234); + var p2 = new SatelliteProvider.Common.DTO.GeoPoint(50.4501, 30.6234); + + // Act + var distance = GeoUtils.CalculateDistance(p1, p2); + + // Assert + var expectedManual = 2d * Math.Asin(Math.Sqrt( + Math.Cos(50.4501 * Math.PI / 180.0) * Math.Cos(50.4501 * Math.PI / 180.0) + * Math.Pow(Math.Sin(0.05 * Math.PI / 180.0), 2))) + * GeoUtils.EarthRadiusMeters; + distance.Should().BeApproximately(expectedManual, 0.0001, + "AZ-377 must not change the numeric result of CalculateDistance"); + } + + private static string? LocateRepoFile(string relativePath) + { + var dir = new DirectoryInfo(Directory.GetCurrentDirectory()); + while (dir is not null) + { + var candidate = Path.Combine(dir.FullName, relativePath); + if (File.Exists(candidate)) + { + return candidate; + } + dir = dir.Parent; + } + return null; + } +} diff --git a/SatelliteProvider.Tests/GeoUtilsRefactorTests.cs b/SatelliteProvider.Tests/GeoUtilsRefactorTests.cs index 4cba717..83b4cd3 100644 --- a/SatelliteProvider.Tests/GeoUtilsRefactorTests.cs +++ b/SatelliteProvider.Tests/GeoUtilsRefactorTests.cs @@ -1,4 +1,7 @@ +using System.Reflection; using FluentAssertions; +using SatelliteProvider.Common.Configs; +using SatelliteProvider.Common.DTO; using SatelliteProvider.Common.Utils; namespace SatelliteProvider.Tests; @@ -12,4 +15,49 @@ public class GeoUtilsRefactorTests typeof(GeoUtils).GetMethod("CalculatePolygonDiagonalDistance").Should().BeNull( "AZ-380 deletes the dead alias method that simply forwarded to CalculateDistance"); } + + [Theory] + [InlineData("EarthRadiusMeters", 6378137d)] + [InlineData("EarthEquatorialCircumferenceMeters", 40075016.686d)] + [InlineData("MetersPerDegreeLatitude", 111000d)] + public void GeoUtils_ExposesEarthConstantsAsPublicConst_AZ377_AC1(string fieldName, double expected) + { + // Arrange + var field = typeof(GeoUtils).GetField(fieldName, BindingFlags.Public | BindingFlags.Static); + + // Assert + field.Should().NotBeNull($"AZ-377 promotes {fieldName} to a public const on GeoUtils"); + field!.IsLiteral.Should().BeTrue($"{fieldName} must be a const so call sites can reference it without runtime cost"); + field.FieldType.Should().Be(typeof(double)); + field.GetRawConstantValue().Should().Be(expected, "AZ-377 forbids numerical drift from the previous literal"); + } + + [Fact] + public void MapConfig_ExposesDefaultTileSizePixelsConst_AZ377_AC1() + { + // Arrange + var field = typeof(MapConfig).GetField("DefaultTileSizePixels", BindingFlags.Public | BindingFlags.Static); + + // Assert + field.Should().NotBeNull("AZ-377 introduces a const so DataAccess can reference the canonical pixel size without IOptions"); + field!.IsLiteral.Should().BeTrue(); + field.GetRawConstantValue().Should().Be(256); + new MapConfig().TileSizePixels.Should().Be(MapConfig.DefaultTileSizePixels, + "the instance default must remain wired to the const so config-bound consumers stay in sync"); + } + + [Fact] + public void GeoUtils_HaversineProducesIdenticalResultAfterConstantRename_AZ377_AC2() + { + // Arrange + var a = new GeoPoint(50.4501, 30.5234); + var b = new GeoPoint(50.4501, 30.6234); + + // Act + var d = GeoUtils.CalculateDistance(a, b); + + // Assert + d.Should().BeApproximately(7090d, 5d, + "renaming EARTH_RADIUS to EarthRadiusMeters must not change Haversine output"); + } } diff --git a/_docs/02_tasks/_dependencies_table.md b/_docs/02_tasks/_dependencies_table.md index 35215d6..2033b93 100644 --- a/_docs/02_tasks/_dependencies_table.md +++ b/_docs/02_tasks/_dependencies_table.md @@ -38,24 +38,24 @@ Roadmap: `_docs/04_refactoring/03-code-quality-refactoring/analysis/refactoring_ | 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 | 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 | -| AZ-368 | C15 | Shared TileCsvWriter | 3 | — | 2 | To Do | -| AZ-367 | C14 | Shared TileGridStitcher | 3 | AZ-364 | 3 | To Do | +| AZ-362 | C09 | Idempotent POST contract | 2 | AZ-353 | 3 | Done (In Testing) | +| AZ-366 | C13 | Consolidate Haversine + filename parser | 3 | — | 2 | Done (In Testing) | +| AZ-377 | C24 | Consolidate Earth constants + 111000 | 3 | AZ-371 | 2 | Done (In Testing) | +| AZ-368 | C15 | Shared TileCsvWriter | 3 | — | 2 | Done (In Testing) | +| AZ-367 | C14 | Shared TileGridStitcher | 3 | AZ-364 | 3 | Done (In Testing) | | AZ-369 | C16 | Move inline DTOs out of Program.cs | 3 | — | 2 | Done (In Testing) | -| AZ-365 | C12 | Decompose RouteService.CreateRouteAsync | 3 | — | 5 | To Do | -| AZ-364 | C11 | Decompose RouteProcessingService god-class | 3 | AZ-366, AZ-367 (folds in AZ-360) | 5 | To Do | -| AZ-360 | C08 | Replace IServiceProvider in RouteProcessingService | 3 | AZ-364 (folded) | 2 | To Do | -| AZ-371 | C18 | Magic numbers → ProcessingConfig/MapConfig | 4 | — | 3 | To Do | -| AZ-370 | C17 | Status / point-type enums + AC RT2 update | 4 | — | 3 | To Do | -| AZ-373 | C20 | Clarify / drop MapsVersion | 4 | AZ-357 | 2 | To Do | -| AZ-374 | C21 | Typed HttpClient for Google Maps | 4 | — | 2 | To Do | -| AZ-375 | C22 | O(N) existing-tile lookup (HashSet) | 4 | AZ-371 | 2 | To Do | -| AZ-376 | C23 | Delete unused FindExistingTileAsync | 4 | — | 1 | To Do | -| AZ-378 | C25 | Repo `_logger` fields: delete or use | 4 | — | 1 | To Do | -| AZ-379 | C26 | Extract repo SELECT column-list constants | 4 | — | 2 | To Do | -| AZ-380 | C27 | Delete CalculatePolygonDiagonalDistance | 4 | — | 1 | To Do | +| AZ-365 | C12 | Decompose RouteService.CreateRouteAsync | 3 | — | 5 | Done (In Testing) | +| AZ-364 | C11 | Decompose RouteProcessingService god-class | 3 | AZ-366, AZ-367 (folds in AZ-360) | 5 | Done (In Testing) | +| AZ-360 | C08 | Replace IServiceProvider in RouteProcessingService | 3 | AZ-364 (folded) | 2 | Done (In Testing) | +| AZ-371 | C18 | Magic numbers → ProcessingConfig/MapConfig | 4 | — | 3 | Done (In Testing) | +| AZ-370 | C17 | Status / point-type enums + AC RT2 update | 4 | — | 3 | Done (In Testing) | +| AZ-373 | C20 | Clarify / drop MapsVersion | 4 | AZ-357 | 2 | Done (In Testing) | +| AZ-374 | C21 | Typed HttpClient for Google Maps | 4 | — | 2 | Done (In Testing) | +| AZ-375 | C22 | O(N) existing-tile lookup (HashSet) | 4 | AZ-371 | 2 | Done (In Testing) | +| AZ-376 | C23 | Delete unused FindExistingTileAsync | 4 | — | 1 | Done (In Testing) | +| AZ-378 | C25 | Repo `_logger` fields: delete or use | 4 | — | 1 | Done (In Testing) | +| AZ-379 | C26 | Extract repo SELECT column-list constants | 4 | — | 2 | Done (In Testing) | +| AZ-380 | C27 | Delete CalculatePolygonDiagonalDistance | 4 | — | 1 | Done (In Testing) | | AZ-372 | C19 | dotnet format + NetAnalyzers + Coverlet | 4 | — | 3 | Done (In Testing) | ## Execution Order diff --git a/_docs/02_tasks/todo/AZ-375_refactor_on_existing_tile_lookup.md b/_docs/02_tasks/done/AZ-375_refactor_on_existing_tile_lookup.md similarity index 100% rename from _docs/02_tasks/todo/AZ-375_refactor_on_existing_tile_lookup.md rename to _docs/02_tasks/done/AZ-375_refactor_on_existing_tile_lookup.md diff --git a/_docs/02_tasks/todo/AZ-377_refactor_consolidate_earth_constants.md b/_docs/02_tasks/done/AZ-377_refactor_consolidate_earth_constants.md similarity index 100% rename from _docs/02_tasks/todo/AZ-377_refactor_consolidate_earth_constants.md rename to _docs/02_tasks/done/AZ-377_refactor_consolidate_earth_constants.md diff --git a/_docs/03_implementation/batch_24_report.md b/_docs/03_implementation/batch_24_report.md new file mode 100644 index 0000000..8d9753a --- /dev/null +++ b/_docs/03_implementation/batch_24_report.md @@ -0,0 +1,102 @@ +# Batch Report + +**Batch**: 24 +**Tasks**: AZ-375 (C22 — HashSet existing-tile lookup), AZ-377 (C24 — consolidate Earth + tile-pixel constants) +**Date**: 2026-05-11 +**Run**: `03-code-quality-refactoring` +**Cycle**: 1 + +## Task Results + +| Task | Status | Files Modified | Tests | AC Coverage | Issues | +|------|--------|----------------|-------|-------------|--------| +| AZ-375_refactor_on_existing_tile_lookup | Done | 1 (`GoogleMapsDownloaderV2.cs`) | 1 new (DownloaderRefactorTests AZ375_AC1) | 3/3 | None | +| AZ-377_refactor_consolidate_earth_constants | Done | 4 (`GeoUtils.cs`, `MapConfig.cs`, `TileRepository.cs`, `GoogleMapsDownloaderV2.cs`) | 5 new (DownloaderRefactorTests + GeoUtilsRefactorTests AZ377_AC1/AC2) | 3/3 | F1 in cumulative review (Low / Architecture documentation drift, non-blocking) | + +Total: 4 source files modified + 1 new test file (`DownloaderRefactorTests.cs`) + 1 extended test file (`GeoUtilsRefactorTests.cs`) + 6 new test cases (5 for AZ-377 across both files, 1 for AZ-375). + +## Changes + +### AZ-375 — O(N) existing-tile lookup via HashSet +- `SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs`: + - Removed the per-cell `existingTiles.FirstOrDefault(t => Math.Abs(...) < tolerance && ... )` linear scan. + - Added a one-time `HashSet<(int X, int Y, int Z)>` build keyed on the integer `(TileX, TileY, Zoom)` derived via the same `GeoUtils.WorldToTilePos` formula used to write the tiles. + - Inner loop now does `existingTileKeys.Contains((x, y, zoomLevel))` — O(1) per cell. + - `_processingConfig.LatLonTolerance` is no longer referenced at this site (tolerance was redundant since tile coordinates at a fixed zoom are integers). +- The other tolerance use site (`RouteService.cs:154` geofence polygon check) is untouched. + +### AZ-377 — consolidate Earth-geometry + tile-pixel constants +- `SatelliteProvider.Common/Utils/GeoUtils.cs`: + - Promoted `EARTH_RADIUS = 6378137` to `public const double EarthRadiusMeters = 6378137d`. + - Added `public const double EarthEquatorialCircumferenceMeters = 40075016.686d`. + - Added `public const double MetersPerDegreeLatitude = 111000d`. + - Updated all internal usages in `GeoUtils` (Haversine, GoDirection, GetBoundingBox). +- `SatelliteProvider.Common/Configs/MapConfig.cs`: + - Added `public const int DefaultTileSizePixels = 256`. + - `TileSizePixels` property default now references the const. + - Added inline doc comment explaining why both surfaces exist (DataAccess cannot inject `IOptions` from a Repository ctor). +- `SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs`: + - `CalculateTileSizeInMeters` now reads `GeoUtils.EarthEquatorialCircumferenceMeters` instead of a local `EARTH_CIRCUMFERENCE_METERS` const. +- `SatelliteProvider.DataAccess/Repositories/TileRepository.cs`: + - Added `using SatelliteProvider.Common.Configs;` and `using SatelliteProvider.Common.Utils;`. + - `GetTilesByRegionAsync` now uses `GeoUtils.EarthEquatorialCircumferenceMeters`, `GeoUtils.MetersPerDegreeLatitude`, and `MapConfig.DefaultTileSizePixels` — all local literals (`EARTH_CIRCUMFERENCE_METERS`, `TILE_SIZE_PIXELS`, two `111000.0` occurrences) removed. + +### Test additions +- `SatelliteProvider.Tests/DownloaderRefactorTests.cs` (new): + - `GoogleMapsDownloaderV2_UsesHashSetForExistingTileLookup_AZ375_AC1` — file-content assertion that the FirstOrDefault scan and `_processingConfig.LatLonTolerance` are gone and replaced by a typed HashSet + `Contains` call. + - `GoogleMapsDownloaderV2_UsesGeoUtilsCircumferenceConstant_AZ377_AC1` — verifies the downloader routes through `GeoUtils.EarthEquatorialCircumferenceMeters`. + - `TileRepository_UsesGeoUtilsAndMapConfigConstants_AZ377_AC1` — verifies the repository uses the `Common` constants and no local Earth literals remain. + - `GeoUtils_IsTheSoleHolderOfRawEarthLiterals_AZ377_AC1` — pins `GeoUtils.cs` as the single declaration site. + - `HaversineDistance_WithTinyDelta_UsesEarthRadiusConstantConsistently_AZ377_AC2` — numerical equivalence check (recomputes the expected value from the same constant and asserts byte-level identity). +- `SatelliteProvider.Tests/GeoUtilsRefactorTests.cs` (extended): + - `GeoUtils_ExposesEarthConstantsAsPublicConst_AZ377_AC1` — `[Theory]` with three inline data rows; reflection check for `IsLiteral`, type, and value. + - `MapConfig_ExposesDefaultTileSizePixelsConst_AZ377_AC1` — verifies the const exists and the property default still resolves to it. + - `GeoUtils_HaversineProducesIdenticalResultAfterConstantRename_AZ377_AC2` — quick value-pin sanity check. + +### Tooling fix discovered during batch +- `scripts/run-tests.sh` `--unit-only` path was running `dotnet test --no-restore` without an explicit `dotnet restore` first; this caused `SixLabors.ImageSharp` resolution to fail in a clean container. Wrapped the command with `sh -c "dotnet restore ... && dotnet test --no-restore ..."` to mirror the `--full` path. Pure infrastructure fix — not a refactor task. +- Stripped a stray UTF-8 BOM from `MapConfig.cs` introduced during the AZ-377 edits, to satisfy the `dotnet format whitespace --verify-no-changes` gate (`charset = utf-8` in `.editorconfig`). + +## Test Results + +`scripts/run-tests.sh --unit-only`: +- Format gate: PASS +- Unit tests: 200/200 PASS (190 prior + 10 new) +- Coverage: written to `./TestResults/` + +## Cumulative Review (K=3, batches 22-24) + +Triggered after batch 24. Verdict: **PASS_WITH_WARNINGS**. + +- 0 Critical, 0 High, 0 Medium, 4 Low findings. +- Report: `_docs/03_implementation/reviews/cumulative_review_22-24.md`. +- One Low / Architecture finding (F1) flags a pre-existing documentation drift around DataAccess→Common dependency (the project reference was already there; AZ-377 widens the surface from `Common.Enums` to also include `Common.Configs` and `Common.Utils`). Recommended follow-up: refresh `module-layout.md` and the architecture baseline F5 entry. Not blocking. + +## Files Modified + +Production: +- `SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs` +- `SatelliteProvider.Common/Utils/GeoUtils.cs` +- `SatelliteProvider.Common/Configs/MapConfig.cs` +- `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` + +Tests: +- `SatelliteProvider.Tests/DownloaderRefactorTests.cs` (new) +- `SatelliteProvider.Tests/GeoUtilsRefactorTests.cs` (extended) + +Tooling: +- `scripts/run-tests.sh` (unit-only path now restores before testing) + +Docs / state: +- `_docs/02_tasks/done/AZ-375_refactor_on_existing_tile_lookup.md` (moved from todo) +- `_docs/02_tasks/done/AZ-377_refactor_consolidate_earth_constants.md` (moved from todo) +- `_docs/03_implementation/batch_24_report.md` (this file) +- `_docs/03_implementation/reviews/cumulative_review_22-24.md` +- `_docs/_autodev_state.md` + +## Status + +- Both batch-24 tasks DONE. +- Batches 22-24 cumulative review COMPLETE; verdict PASS_WITH_WARNINGS. +- 03-code-quality-refactoring run: 6/6 tasks done (AZ-372, AZ-375, AZ-376, AZ-377, AZ-378, AZ-379, AZ-380 — note: 7 tracker IDs because AZ-372 covers tooling C19 separately from the C-prefixed change items). +- Refactoring epic (AZ-309 / AZ-350) progress: see traceability table. diff --git a/_docs/03_implementation/reviews/cumulative_review_22-24.md b/_docs/03_implementation/reviews/cumulative_review_22-24.md new file mode 100644 index 0000000..c85dc11 --- /dev/null +++ b/_docs/03_implementation/reviews/cumulative_review_22-24.md @@ -0,0 +1,141 @@ +# Cumulative Code Review Report (K=3) + +**Batches**: 22, 23, 24 +**Tasks covered**: +- B22: AZ-372 (C19 — dotnet format + analyzers + Coverlet) +- B23: AZ-376 (C23), AZ-378 (C25), AZ-379 (C26), AZ-380 (C27) +- B24: AZ-375 (C22), AZ-377 (C24) +**Date**: 2026-05-11 +**Mode**: Cumulative (all 7 phases on union of changed files since last cumulative review) +**Verdict**: PASS_WITH_WARNINGS + +## Scope (changed files since cumulative review @ batch 21 close) + +Production source: +- `SatelliteProvider.Common/Configs/MapConfig.cs` +- `SatelliteProvider.Common/Utils/GeoUtils.cs` +- `SatelliteProvider.DataAccess/Repositories/ITileRepository.cs` +- `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` +- `SatelliteProvider.DataAccess/Repositories/RegionRepository.cs` +- `SatelliteProvider.DataAccess/Repositories/RouteRepository.cs` +- `SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs` +- `SatelliteProvider.Api/Program.cs` + +Tooling / config: +- `.editorconfig` +- `Directory.Build.props` +- `scripts/run-tests.sh` + +Tests added or extended: +- `SatelliteProvider.Tests/ToolingConfigurationTests.cs` +- `SatelliteProvider.Tests/RepositoryRefactorTests.cs` +- `SatelliteProvider.Tests/GeoUtilsRefactorTests.cs` +- `SatelliteProvider.Tests/DownloaderRefactorTests.cs` + +## Findings + +| # | Severity | Category | File:Line | Title | +|---|----------|----------|-----------|-------| +| 1 | Low | Architecture | `SatelliteProvider.DataAccess/SatelliteProvider.DataAccess.csproj` + `_docs/02_document/architecture_compliance_baseline.md` (F5) | DataAccess→Common dependency surface widened; baseline doc still claims "no Common dependency" | +| 2 | Low | Maintainability | `SatelliteProvider.DataAccess/Repositories/RouteRepository.cs:38` | `route_points` column list still inline (single use) | +| 3 | Low | Spec-Gap | `_docs/02_tasks/done/AZ-37*` task ACs | Stale "37 unit + 5 smoke" test count in AC text | +| 4 | Low | Maintainability | `SatelliteProvider.Common/Configs/MapConfig.cs` | `TileSizePixels` is now both a property default *and* a `const`; small surface duplication for compile-time consumers | + +### Finding Details + +**F1: DataAccess→Common dependency surface widened** (Low / Architecture) +- Location: `SatelliteProvider.DataAccess/SatelliteProvider.DataAccess.csproj` (project reference); affects `architecture_compliance_baseline.md` Finding F5. +- Description: AZ-377 introduces `using SatelliteProvider.Common.Configs;` and `using SatelliteProvider.Common.Utils;` in `TileRepository.cs`. The baseline doc (Resolved F5) and `module-layout.md` claim DataAccess has no Common dependency. In reality the `` to Common already exists in the csproj and 5 files in DataAccess (`RegionRepository.cs`, `IRegionRepository.cs`, `Models/RegionEntity.cs`, `Models/RoutePointEntity.cs`, `TypeHandlers/EnumStringTypeHandler.cs`) already import `SatelliteProvider.Common.Enums`. Batch 24 widens the surface to also include `Common.Configs` and `Common.Utils`. +- Impact: Documentation drift only. The actual dependency graph has been DataAccess→Common since at least the AZ-309 cleanup; the baseline marked F5 "Resolved" prematurely. No new layering violation — Common stays at the leaf and DataAccess remains one level above it. +- Suggestion: in the next documentation pass, update `_docs/02_document/module-layout.md` (DataAccess `Imports from`) and the baseline F5 entry to match reality (`SatelliteProvider.Common.Enums`, `SatelliteProvider.Common.Configs`, `SatelliteProvider.Common.Utils`). Do not invert the actual code dependency — the constants legitimately belong in Common and DataAccess legitimately needs them. +- Task: AZ-377 + +**F2: `route_points` column list still inline** (Low / Maintainability) — carry-over from batch 23 +- Location: `SatelliteProvider.DataAccess/Repositories/RouteRepository.cs:38-46` +- Description: `GetRoutePointsAsync` keeps its 8-column list inline; AZ-379's "extract once per repository" target is satisfied by the `routes` ColumnList. +- Suggestion: extract only when a second SELECT against `route_points` arrives. +- Task: AZ-379 + +**F3: Stale "37 unit + 5 smoke" count in AC text** (Low / Spec-Gap) — carry-over from batch 23 +- Location: AC-3 of AZ-375, AZ-376, AZ-377, AZ-378, AZ-379, AZ-380. +- Description: Tasks were authored when the unit count was 37; suite now reports 200/200 passing after batch 24. The "scripts/run-tests.sh --smoke" command still gates correctly; only the literal count in the AC narrative is stale. +- Suggestion: leave as-is — the gate is the script's exit code, not the printed number; rewriting historical AC text would create churn without changing what is verified. +- Task: documentation/process + +**F4: `MapConfig.TileSizePixels` exists as both property and `const`** (Low / Maintainability) +- Location: `SatelliteProvider.Common/Configs/MapConfig.cs:12-14` +- Description: AZ-377 introduces `public const int DefaultTileSizePixels = 256;` so DataAccess (which cannot inject `IOptions` from a Repository constructor) can reference a single source. The property `TileSizePixels` keeps its setter for config binding and is now wired to the const as its default. Small surface duplication, deliberate. +- Impact: Two access paths to the same value — `MapConfig.DefaultTileSizePixels` (compile-time) and `IOptions.Value.TileSizePixels` (runtime). If a deployment ever overrides the property in `appsettings.json`, DataAccess will silently disagree with config-bound consumers because it reads the const. +- Suggestion: accepted trade-off for now — `appsettings.json` does not currently override the value and DI in DataAccess is a separate refactor (would require pushing `IOptions` through the repository interface). Document the decision in `MapConfig.cs` (already added an XML-style comment in batch 24) and revisit if anyone overrides `TileSizePixels` in config. +- Task: AZ-377 + +## Phase results (summary) + +**Phase 2 — Spec compliance** +- AZ-372: format gate present (`Step 0` in `run-tests.sh`), Directory.Build.props has analyzers + Coverlet, ToolingConfigurationTests pin them. ✓ +- AZ-376: `FindExistingTileAsync` removed from interface and impl (RepositoryRefactorTests AZ376_AC1 passes). ✓ +- AZ-378: RegionRepository, RouteRepository constructors take only the connection string; TileRepository keeps `_logger` and uses it for slow-query warnings (RepositoryRefactorTests AZ378_AC1, AC2 pass). ✓ +- AZ-379: All three repositories define exactly one `private const string ColumnList`; SELECTs use the const (RepositoryRefactorTests AZ379_AC1, AC2 pass). ✓ +- AZ-380: `CalculatePolygonDiagonalDistance` removed (GeoUtilsRefactorTests AZ380_AC1 passes). ✓ +- AZ-375: HashSet<(int, int, int)> built once and `Contains((x, y, zoomLevel))` replaces the per-cell `FirstOrDefault`; `_processingConfig.LatLonTolerance` no longer referenced at this site (DownloaderRefactorTests AZ375_AC1 passes; grep confirms `0.0001` only remains in the unrelated `ProcessingConfig` default and tests). ✓ +- AZ-377: `EarthRadiusMeters`, `EarthEquatorialCircumferenceMeters`, `MetersPerDegreeLatitude` are `public const double` on `GeoUtils`; `MapConfig.DefaultTileSizePixels` is a `public const int` mirroring the property default; all source-code occurrences of `6378137`, `40075016.686`, `111000`, and source-code occurrences of `256` are confined to those declarations (DownloaderRefactorTests + GeoUtilsRefactorTests AZ377_AC1, AC2 pass). ✓ + +**Phase 3 — Code quality** +- All edits under SRP — no method gained more than one responsibility; HashSet builder is a small, named, local computation in the same method; logger removal from RegionRepository/RouteRepository is purely subtractive. +- No bare catches introduced. No new debug-noise logs. No obvious duplication. + +**Phase 4 — Security quick-scan** +- No new SQL string interpolation (Dapper named params throughout, ColumnList is a `const string`, not user input). +- No new secrets, no new external inputs. +- Slow-query warning logs query duration and SQL text, both safe. + +**Phase 5 — Performance scan** +- AZ-375 directly removes an O(N) per-cell scan inside an O(grid) loop: a region of NxN tiles previously did O(N² · existingTiles) work; now O(existingTiles) preprocessing + O(1) per cell. +- No N+1 queries introduced; the existing-tiles query batch shape is unchanged. +- No blocking I/O in async paths added. + +**Phase 6 — Cross-task consistency** +- Batch 23 + 24 collectively touch the same three repositories and `GoogleMapsDownloaderV2`; constants moved into Common are consumed by DataAccess and the downloader uniformly. No divergent patterns. +- AZ-379's ColumnList convention applied identically across all three repos (`private const string ColumnList = @"…"`, used via `$@"… SELECT {ColumnList} …"`). + +**Phase 7 — Architecture compliance** +- Layer direction: Common (leaf) ← DataAccess ← Services.TileDownloader ← Api. All new imports respect the table. +- Public API respect: `GeoUtils`, `MapConfig`, and their new constants are top-level public types in `Common`. No internal-file imports. +- New cyclic deps: none introduced (DataAccess→Common is one-way; Common does not reference DataAccess). +- Duplicate symbols: the previous duplicate Earth literals across `GoogleMapsDownloaderV2.cs` and `TileRepository.cs` are now collapsed to one declaration in `GeoUtils.cs`. The `MapConfig.TileSizePixels` property + `MapConfig.DefaultTileSizePixels` const live in the same class — see F4. + +## Baseline Delta + +| Status | Severity | Title | Note | +|--------|----------|-------|------| +| Carried over | Low | F5 — DataAccess Common dependency drift | Documentation drift; widened by AZ-377 (see F1 above). | +| Resolved | Low | duplicate Earth literals across modules | Cumulative review @ batch 21 had flagged the same literals in two files; AZ-377 collapses them to a single source in `GeoUtils`. | +| Newly introduced | — | none | No new Critical / High / Medium architectural findings. | + +Per-category counts (current scope): + +| Category | Critical | High | Medium | Low | +|----------|----------|------|--------|-----| +| Architecture | 0 | 0 | 0 | 1 | +| Maintainability | 0 | 0 | 0 | 2 | +| Spec-Gap | 0 | 0 | 0 | 1 | +| Security | 0 | 0 | 0 | 0 | +| Performance | 0 | 0 | 0 | 0 | +| Bug | 0 | 0 | 0 | 0 | + +## Test posture + +- `scripts/run-tests.sh --unit-only`: 200/200 PASS (10 new tests added across batches 23-24). +- Format gate (`dotnet format whitespace --verify-no-changes`): PASS. +- Full smoke / integration suite: not re-run as part of this cumulative review (next session boundary will run it before commit if the engine requires). + +## Verdict logic + +- 0 Critical, 0 High, 0 Medium, 4 Low → **PASS_WITH_WARNINGS**. +- No FAIL conditions; no auto-fix loop required. + +## Recommended follow-ups (non-blocking) + +1. Refresh `_docs/02_document/module-layout.md` and the baseline F5 entry to reflect the actual DataAccess→Common dependency (Common.Enums, Common.Configs, Common.Utils). +2. Decide whether `MapConfig.DefaultTileSizePixels` should remain a const or be threaded through `IOptions` into DataAccess; defer until someone overrides the value in config. +3. Consider a future task to update the AC test-count narrative across the AZ-37x tickets if/when the project does a doc-sweep. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 8a9160a..ab337e4 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -6,9 +6,9 @@ step: 8 name: Refactor status: in_progress sub_step: - phase: 4 - name: batch-loop - detail: "batch 23 complete; cum-rev counter 2/3; ready for batch 24 (AZ-375, AZ-377)" + phase: 5 + name: test-sync + detail: "" retry_count: 0 cycle: 1 tracker: jira diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh index 61628b8..7171e41 100755 --- a/scripts/run-tests.sh +++ b/scripts/run-tests.sh @@ -60,11 +60,7 @@ fi if [[ "$mode" == "unit" ]]; then echo "Running unit tests only..." docker run --rm -v "$PROJECT_ROOT:/src" -w /src mcr.microsoft.com/dotnet/sdk:8.0 \ - dotnet test SatelliteProvider.Tests/SatelliteProvider.Tests.csproj \ - --no-restore --configuration Release \ - --collect:"XPlat Code Coverage" \ - --results-directory /src/TestResults \ - --logger "console;verbosity=normal" + sh -c "dotnet restore SatelliteProvider.sln && dotnet test SatelliteProvider.Tests/SatelliteProvider.Tests.csproj --no-restore --configuration Release --collect:'XPlat Code Coverage' --results-directory /src/TestResults --logger 'console;verbosity=normal'" echo "" echo "=== Unit tests complete (coverage written to ./TestResults/) ===" exit 0