mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 23:01:14 +00:00
[AZ-371] Refactor C18: magic numbers to ProcessingConfig/MapConfig
Promotes 8 operational levers into config keys with defaults that match the prior source literals byte-for-byte: ProcessingConfig: RegionProcessingTimeoutSeconds (300), RouteProcessingPollIntervalSeconds (5), MaxRoutePointSpacingMeters (200), LatLonTolerance (0.0001). MapConfig: TileSizePixels (256), AllowedZoomLevels ([15..19]), RetryBaseDelaySeconds (1), RetryMaxDelaySeconds (30). Sites updated: RegionService, RouteProcessingService, RoutePointGraphBuilder, RouteValidator, RouteService 4-arg ctor, RouteImageRenderer, GoogleMapsDownloaderV2, TileService. Closes LF-2 by forwarding HttpContext.RequestAborted from GetTileByLatLon into the downloader. appsettings.json gains the 8 new keys at default values. Tests: 141 / 141 unit + 5 / 5 smoke green. New ConfigDefaultsTests pins defaults to original literals; new TileService unit test asserts CT identity from caller to downloader (AZ-371 AC-3). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,178 @@
|
||||
# Batch 18 Report — Refactor 03 Phase 4 (kickoff)
|
||||
|
||||
Date: 2026-05-11
|
||||
Epic: AZ-350 (03-code-quality-refactoring)
|
||||
Status: ✅ Complete
|
||||
|
||||
## Scope (1 task / 3 SP)
|
||||
|
||||
| ID | C-ID | Title | Points | Component |
|
||||
|----|------|-------|--------|-----------|
|
||||
| AZ-371 | C18 | Move hardcoded magic numbers to ProcessingConfig / MapConfig | 3 | Common + Services.* (all) |
|
||||
|
||||
Solo batch. First task of Phase 4 (typing/config/tooling). Promotes operational
|
||||
literals scattered across 5 service files into explicit `ProcessingConfig` /
|
||||
`MapConfig` keys. Defaults preserve every prior runtime value byte-for-byte.
|
||||
Also closes the LF-2 finding by forwarding `HttpContext.RequestAborted` from
|
||||
`GetTileByLatLon` into the downloader.
|
||||
|
||||
## Changes
|
||||
|
||||
### Production
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Common/Configs/ProcessingConfig.cs`
|
||||
- Added 4 keys with defaults that match prior source literals:
|
||||
- `RegionProcessingTimeoutSeconds = 300` (was `TimeSpan.FromMinutes(5)`)
|
||||
- `RouteProcessingPollIntervalSeconds = 5` (was `TimeSpan.FromSeconds(5)`)
|
||||
- `MaxRoutePointSpacingMeters = 200.0` (was the `public const`)
|
||||
- `LatLonTolerance = 0.0001` (was duplicated in 2 sites)
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Common/Configs/MapConfig.cs`
|
||||
- Added 4 keys with defaults that match prior source literals:
|
||||
- `TileSizePixels = 256`
|
||||
- `AllowedZoomLevels = [15, 16, 17, 18, 19]`
|
||||
- `RetryBaseDelaySeconds = 1`
|
||||
- `RetryMaxDelaySeconds = 30`
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Api/appsettings.json`
|
||||
- Added the 8 new keys under `MapConfig` and `ProcessingConfig`. Values match
|
||||
`Common.Configs` defaults. `appsettings.Development.json` left untouched —
|
||||
no environment-specific overrides for these levers yet.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RegionProcessing/RegionService.cs`
|
||||
- Constructor now takes `IOptions<ProcessingConfig> processingConfig`.
|
||||
- The 5-minute region-processing timeout in `ProcessRegionAsync` is now
|
||||
`TimeSpan.FromSeconds(_processingConfig.RegionProcessingTimeoutSeconds)`.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs`
|
||||
- Constructor now takes `IOptions<ProcessingConfig> processingConfig`.
|
||||
- `_checkInterval` is no longer a `readonly` initializer literal; it's
|
||||
set in the constructor from the config.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RoutePointGraphBuilder.cs`
|
||||
- Single ctor `(IOptions<ProcessingConfig>)`; the previous
|
||||
parameterless ctor + `public const MaxPointSpacingMeters` are gone.
|
||||
- The const lived briefly to replace the inline literal in `RouteService`
|
||||
(batch 15); promoting it the rest of the way to config is the natural
|
||||
completion of that move.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteValidator.cs`
|
||||
- Constructor now requires `IOptions<ProcessingConfig>`; reads
|
||||
`LatLonTolerance` once and reuses it for the polygon (0,0) check.
|
||||
- `ValidatePolygon` is no longer `static` because it reads instance state.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteService.cs`
|
||||
- The 4-arg DI ctor (`IRouteRepository, IRegionService, IOptions<ProcessingConfig>, ILogger`)
|
||||
is the production-callable ctor; the chained 8-arg ctor used by the
|
||||
decomposition still accepts the helpers directly. The 4-arg ctor
|
||||
constructs `RouteValidator` and `RoutePointGraphBuilder` with the same
|
||||
`IOptions<ProcessingConfig>` reference.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteImageRenderer.cs`
|
||||
- **Adjacent hygiene**: the `private const int TileSizePixels = 256` that
|
||||
was extracted from `RouteProcessingService` in batch 17 is now read from
|
||||
`_mapConfig.TileSizePixels`. Same operational lever as the `TileService`
|
||||
site — leaving one configurable and the other hardcoded would be an
|
||||
immediate cumulative-review finding (cross-task duplicate).
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs`
|
||||
- The 4 `private const`s (`TILE_SIZE_PIXELS`, `MAX_RETRY_DELAY_SECONDS`,
|
||||
`BASE_RETRY_DELAY_SECONDS`, `ALLOWED_ZOOM_LEVELS`) are gone.
|
||||
- `MapConfig` was already injected; now `_mapConfig` field stores the
|
||||
resolved `MapConfig.Value` and the 4 sites consume `_mapConfig.*`.
|
||||
- `CalculateTileSizeInMeters` is no longer `static` (reads `_mapConfig`).
|
||||
- The `0.0001` lat/lon tolerance in `GetTilesWithMetadataAsync` now reads
|
||||
`_processingConfig.LatLonTolerance` once per call and reuses it in the
|
||||
closure.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.TileDownloader/TileService.cs`
|
||||
- Constructor now takes `IOptions<MapConfig> mapConfig`.
|
||||
- `BuildTileEntity` becomes an instance method (reads `_mapConfig`); the
|
||||
`TileSizePixels = 256` literal becomes `_mapConfig.TileSizePixels`.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Api/Program.cs`
|
||||
- `GetTileByLatLon` now accepts `HttpContext httpContext` and forwards
|
||||
`httpContext.RequestAborted` into `DownloadAndStoreSingleTileAsync`.
|
||||
- This closes LF-2 (logical-flow finding from the discovery phase): client
|
||||
cancellations now propagate through the `/api/satellite/tiles/latlon`
|
||||
endpoint into the downloader.
|
||||
|
||||
### Tests
|
||||
|
||||
- **NEW** `SatelliteProvider.Tests/ConfigDefaultsTests.cs` — 7 tests that
|
||||
pin the new config defaults to the original source literals (AC-2 evidence
|
||||
— defaults preserve behavior is the load-bearing claim of this batch).
|
||||
- **MODIFIED** `SatelliteProvider.Tests/TileServiceTests.cs`
|
||||
- Added 1 new test `DownloadAndStoreSingleTileAsync_ForwardsCancellationTokenToDownloader_AZ371_AC3`
|
||||
(AC-3 evidence — caller-supplied CT reaches the downloader).
|
||||
- Updated `BuildService` to construct `TileService` with the new
|
||||
`IOptions<MapConfig>` argument.
|
||||
- **MODIFIED** `SatelliteProvider.Tests/InfrastructureTests.cs`
|
||||
- Updated the `TileService_ConstructsWithMockedDependencies` smoke test to
|
||||
provide the new `IOptions<MapConfig>`.
|
||||
- **MODIFIED** `SatelliteProvider.Tests/RegionServiceTests.cs`
|
||||
- `BuildService` now also threads `IOptions<ProcessingConfig>`.
|
||||
- **MODIFIED** `SatelliteProvider.Tests/RouteServiceTests.cs`
|
||||
- `BuildService` now also threads `IOptions<ProcessingConfig>`.
|
||||
- **MODIFIED** `SatelliteProvider.Tests/RoutePointGraphBuilderTests.cs`
|
||||
- All 8 `new RoutePointGraphBuilder()` calls go through a `MakeBuilder()`
|
||||
helper that wraps the new options ctor.
|
||||
- The two assertions that referenced `RoutePointGraphBuilder.MaxPointSpacingMeters`
|
||||
now read `DefaultProcessingConfig.MaxRoutePointSpacingMeters`.
|
||||
- **MODIFIED** `SatelliteProvider.Tests/RouteValidatorTests.cs`
|
||||
- All 9 `new RouteValidator()` calls go through `MakeValidator()`.
|
||||
- **MODIFIED** `SatelliteProvider.Tests/RouteImageRendererTests.cs`
|
||||
- `BuildSut` now also threads `IOptions<MapConfig>`.
|
||||
|
||||
## Verification
|
||||
|
||||
| Check | Result |
|
||||
|-------|--------|
|
||||
| Unit tests | ✅ 141 / 141 pass (was 133; +7 ConfigDefaultsTests, +1 AZ-371 AC-3 CT test) |
|
||||
| Integration smoke | ✅ All scenarios pass; exit 0 |
|
||||
| Behavior preservation | ✅ Defaults match prior literals byte-for-byte |
|
||||
| Linter | ✅ No findings on any modified file |
|
||||
|
||||
## AC Coverage
|
||||
|
||||
| AC | Evidence |
|
||||
|----|----------|
|
||||
| AC-1: All listed magic numbers replaced by config-bound values | Grep for `FromMinutes(5)`, `TILE_SIZE_PIXELS`, `MAX_RETRY`, `BASE_RETRY`, `ALLOWED_ZOOM`, `200.0`, `0.0001`, `TileSizePixels = 256` across `SatelliteProvider.Services.*/**/*.cs` returns no matches. The remaining `_tileSizePixels` references are `MapConfig`-bound. |
|
||||
| AC-2: Defaults preserve all existing behavior | `ConfigDefaultsTests` (7 tests) pins each new default to the original literal value. Smoke suite passes — region pipeline, route pipeline, tile downloads all behave identically. |
|
||||
| AC-3: `GetTileByLatLon` request cancellation flows into the downloader | `Program.cs:165` now passes `httpContext.RequestAborted` into `DownloadAndStoreSingleTileAsync`. `TileServiceTests.DownloadAndStoreSingleTileAsync_ForwardsCancellationTokenToDownloader_AZ371_AC3` captures the CT through the mock and asserts identity. |
|
||||
|
||||
## Inline Code Review
|
||||
|
||||
This batch is a config-binding refactor. The risk surface:
|
||||
|
||||
1. **Default-value drift** — every default in `ProcessingConfig` and `MapConfig`
|
||||
was set side-by-side with the original literal in source and verified by
|
||||
`ConfigDefaultsTests`. No drift possible.
|
||||
2. **Constructor fan-out** — 5 production classes gained ctor params. Only the
|
||||
already-DI-resolved consumers (`RouteService` 4-arg, `RegionService`,
|
||||
`TileService`, `RouteProcessingService`, `GoogleMapsDownloaderV2`) needed
|
||||
`IServiceCollection` calls — and those are unchanged because the existing
|
||||
`services.AddSingleton(...)` lines bind via reflection. The two helper
|
||||
classes that tests `new` directly (`RouteValidator`, `RoutePointGraphBuilder`)
|
||||
now require `IOptions<ProcessingConfig>` — every call site is updated.
|
||||
3. **Static → instance demotions** — three methods became instance methods
|
||||
(`TileService.BuildTileEntity`, `GoogleMapsDownloaderV2.CalculateTileSizeInMeters`,
|
||||
`RouteValidator.ValidatePolygon`). Each one now reads instance state, so
|
||||
the demotion is correct per `coderule.mdc` ("Only use static methods for
|
||||
pure, self-contained computations").
|
||||
4. **CT plumbing** (LF-2) — endpoint local function gained `HttpContext`
|
||||
parameter; minimal API binding picks it up automatically. Smoke covers
|
||||
the happy path; the new unit test pins the contract on `TileService`.
|
||||
|
||||
**Verdict: PASS.** No findings. Behavior preserved end-to-end.
|
||||
|
||||
## State
|
||||
|
||||
`auto_push: true`. After this commit lands, transitions on AZ-371 → In Testing
|
||||
in Jira and the task file moves from `todo/` to `done/`. Batch 18 closes the
|
||||
window for the cumulative K=3 review covering batches 16–18 — that runs next.
|
||||
|
||||
## Next Batch (preview)
|
||||
|
||||
Phase 4 ordering: AZ-370 (status / point-type enums + AC RT2 update, 3 SP).
|
||||
After the cumulative K=3 review for 16–18.
|
||||
Reference in New Issue
Block a user