mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 02:01:14 +00:00
[AZ-365] Refactor C12: decompose RouteService.CreateRouteAsync
Extract RouteValidator (aggregating validator), RoutePointGraphBuilder (point interpolation + sequence numbering), GeofenceGridCalculator (NW/SE region centers), and RouteResponseMapper (entity -> DTO; also used by GetRouteAsync, eliminating duplicate DTO assembly). CreateRouteAsync shrinks 184 -> 52 LOC of orchestration. RouteService.cs shrinks 295 -> 138 LOC overall. Validation aggregates all failures into a single ArgumentException (AC-2); single-violation messages preserved verbatim so existing RouteServiceTests pass unchanged. 28 new unit tests for the four helpers (112/112 unit tests, smoke green). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,121 @@
|
||||
# Batch 15 Report — Refactor 03 Phase 3 (continued)
|
||||
|
||||
Date: 2026-05-11
|
||||
Epic: AZ-350 (03-code-quality-refactoring)
|
||||
Status: ✅ Complete
|
||||
|
||||
## Scope (1 task / 5 SP)
|
||||
|
||||
| ID | C-ID | Title | Points | Component |
|
||||
|----|------|-------|--------|-----------|
|
||||
| AZ-365 | C12 | Decompose `RouteService.CreateRouteAsync` 165-LOC method | 5 | Services.RouteManagement |
|
||||
|
||||
Solo batch — first of the two big Phase 3 decompositions. Pure SRP refactor: the method's five responsibilities (validation, point-graph construction, persistence, geofence grid, response mapping) each move into a dedicated helper class. Behavior preserved end-to-end.
|
||||
|
||||
## Changes
|
||||
|
||||
### Production
|
||||
|
||||
- **NEW** `SatelliteProvider.Services.RouteManagement/RouteValidator.cs`
|
||||
- `public void Validate(CreateRouteRequest request)` — collects every failure into a `List<string>`, then throws a single `ArgumentException` with the failures joined by `; ` if any are present (AC-2: aggregated validation).
|
||||
- All four pre-existing checks preserved verbatim (point count, region size, blank name, polygon checks). Polygon check uses an inline helper `ValidatePolygon`.
|
||||
- Single-violation messages remain identical strings (e.g. `"Route must have at least 2 points"`), so the existing `WithMessage("*at least 2 points*")` style assertions in `RouteServiceTests` pass without modification.
|
||||
|
||||
- **NEW** `SatelliteProvider.Services.RouteManagement/RoutePointGraphBuilder.cs`
|
||||
- `public RoutePointGraph Build(IReadOnlyList<RoutePoint> userPoints)` — pure point-graph construction.
|
||||
- `MAX_POINT_SPACING_METERS = 200.0` moves from `RouteService` to `RoutePointGraphBuilder.MaxPointSpacingMeters` (kept `public const` so tests + future config callers can reference it).
|
||||
- Returns `RoutePointGraph(IReadOnlyList<RoutePointDto> Points, double TotalDistanceMeters)` — single record exposing both outputs the orchestrator needs without leaking mutation.
|
||||
- Logic identical to the original loop: same `start` / `end` / `action` typing, same intermediate generation via `GeoUtils.CalculateIntermediatePoints`, same Haversine totaling.
|
||||
|
||||
- **NEW** `SatelliteProvider.Services.RouteManagement/GeofenceGridCalculator.cs`
|
||||
- `public IReadOnlyList<GeoPoint> GenerateRegions(GeoPoint northWest, GeoPoint southEast, double regionSizeMeters)` — promotes the previously private `CreateGeofenceRegionGrid` to a public, unit-testable helper.
|
||||
- Same algorithm: midpoint width/height via `GeoUtils.CalculateDistance`, ceiling division for grid step counts, half-step offset for cell centers.
|
||||
- Adds explicit `regionSizeMeters > 0` guard (the previous private path could not be reached with bad input because the validator caught it upstream; the new public surface needs its own guard).
|
||||
|
||||
- **NEW** `SatelliteProvider.Services.RouteManagement/RouteResponseMapper.cs`
|
||||
- `public RouteResponse Map(RouteEntity, IEnumerable<RoutePointDto>)` and overload `Map(RouteEntity, IEnumerable<RoutePointEntity>)` — single mapper used by both `CreateRouteAsync` and `GetRouteAsync`, eliminating the two near-identical `new RouteResponse { ... }` blocks.
|
||||
- Field-for-field copy preserved exactly. The entity → DTO point projection (lat / lon / point-type / sequence / segment / distance) lives once, in the second overload.
|
||||
|
||||
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteService.cs`
|
||||
- File shrunk from 295 → 138 lines.
|
||||
- `CreateRouteAsync` is now ~52 LOC orchestration: idempotency check → `_validator.Validate` → `_pointGraphBuilder.Build` → entity insert → points insert → `ProcessGeofencePolygonsAsync` → `_responseMapper.Map` (AC-1).
|
||||
- `GetRouteAsync` reuses `_responseMapper.Map(route, points)` (DRY win — no more inline DTO assembly).
|
||||
- Two-constructor pattern: production uses the existing 3-arg constructor (`IRouteRepository`, `IRegionService`, `ILogger`) which delegates to a new 7-arg constructor that injects the four helpers. The 7-arg form is reserved for tests that want to substitute a fake helper. Existing `RouteServiceTests` continue to use the 3-arg form unchanged → AC-4.
|
||||
- `CreateGeofenceRegionGrid` private method removed (logic moved to `GeofenceGridCalculator`); `MAX_POINT_SPACING_METERS` const removed (moved to `RoutePointGraphBuilder.MaxPointSpacingMeters`).
|
||||
- Added `ArgumentNullException.ThrowIfNull(request)` at the top of `CreateRouteAsync` — a defense-in-depth guard that didn't exist before but is consistent with the helper-class style used elsewhere in this run.
|
||||
|
||||
### Tests
|
||||
|
||||
- **NEW** `SatelliteProvider.Tests/RouteValidatorTests.cs` — 11 tests covering: valid request, fewer-than-2 points, region size out of range, blank name, polygon (0,0), inverted lat ordering, null polygon corner, out-of-range latitude, **multi-error aggregation (AC-2 verification)**, null request guard.
|
||||
- **NEW** `SatelliteProvider.Tests/RoutePointGraphBuilderTests.cs` — 8 tests covering: 2-point start/end roles, max-spacing invariant, 10-point start/end/action distribution, total-distance Haversine sum, sequence-number contiguity, null `DistanceFromPrevious` on first point, fewer-than-2 throws, null input throws.
|
||||
- **NEW** `SatelliteProvider.Tests/GeofenceGridCalculatorTests.cs` — 6 tests covering: small polygon yields ≥1 center, every center inside polygon, larger size → fewer centers, very large size → exactly 1 center, non-positive size throws, count = `distinctLats * distinctLons`.
|
||||
- **NEW** `SatelliteProvider.Tests/RouteResponseMapperTests.cs` — 4 tests covering: full field copy from DTO points, projection from entity points, null entity / null points guards.
|
||||
|
||||
`SatelliteProvider.Tests/RouteServiceTests.cs` — **unchanged** (AC-4). All 12 existing scenarios still pass (validator and graph builder produce identical outputs for the inputs the existing tests use).
|
||||
|
||||
## Verification
|
||||
|
||||
- **Unit tests**: 112 / 112 passing (was 84 — +28 new tests across the four new helpers; no existing test removed or modified).
|
||||
- **Smoke + full integration suite**: green. Container exits 0. Verified flows include:
|
||||
- `/api/satellite/route` happy path (creates route, returns 200, persists points)
|
||||
- `/api/satellite/route` 1-point payload returns HTTP 400 with the message `Route must have at least 2 points` (AZ-353 / AZ-365 AC-2: message preserved on single-violation, surfaced via `RouteValidator.Validate` per the exception stack trace below)
|
||||
- `/api/satellite/route` idempotent retry returns existing resource with same `createdAt` (AZ-362 AC-2 path preserved)
|
||||
- **AC-2 aggregation evidence (unit-level)**: `RouteValidatorTests.Validate_MultipleErrors_AggregatesIntoSingleException_AZ365_AC2` — sets blank name + region size 50 + zero points; asserts the resulting `ArgumentException.Message` contains all three substrings (`at least 2 points`, `Region size must be between 100 and 10000`, `Route name is required`).
|
||||
|
||||
Smoke test stack trace excerpt confirming the new validator is on the production path:
|
||||
```
|
||||
System.ArgumentException: Route must have at least 2 points
|
||||
at SatelliteProvider.Services.RouteManagement.RouteValidator.Validate(CreateRouteRequest request) in /src/SatelliteProvider.Services.RouteManagement/RouteValidator.cs:line 38
|
||||
at SatelliteProvider.Services.RouteManagement.RouteService.CreateRouteAsync(CreateRouteRequest request) in /src/SatelliteProvider.Services.RouteManagement/RouteService.cs:line 65
|
||||
at Program.<<Main>$>g__CreateRoute|0_21(...) in /src/SatelliteProvider.Api/Program.cs:line 237
|
||||
```
|
||||
|
||||
## Acceptance criteria coverage
|
||||
|
||||
| AC | Evidence |
|
||||
|----|----------|
|
||||
| **AC-1** `CreateRouteAsync` is reduced to orchestration of the four extracted helpers (~30-50 LOC) | New body is 52 LOC of helper calls (idempotency → validate → build → persist → geofences → map). Original was 184 LOC with 5 mixed responsibilities. |
|
||||
| **AC-2** Validation aggregates errors instead of short-circuiting | `RouteValidator` collects into `List<string>` and throws a single `ArgumentException` with `; `-joined messages. Verified by `RouteValidatorTests.Validate_MultipleErrors_AggregatesIntoSingleException_AZ365_AC2`. |
|
||||
| **AC-3** Same persistence calls + same response shape | `InsertRouteAsync`, `InsertRoutePointsAsync`, `RequestRegionAsync`, `LinkRouteToRegionAsync` are called with the exact same arguments as before. `RouteResponse` field copy is byte-equivalent (verified by `RouteResponseMapperTests` + the existing `GetRouteAsync_KnownId_ReturnsRouteWithPoints_BT07` and `CreateRouteAsync_*` test family). |
|
||||
| **AC-4** 37 unit + 5 smoke tests stay green | 112 / 112 unit tests + smoke run green; pre-existing `RouteServiceTests` file is unchanged. |
|
||||
|
||||
## Behavior preservation notes
|
||||
|
||||
- **Validation order**: aggregated, but the *order* in which checks run matters for messages on multi-error inputs. Order preserved: points → region size → name → polygons (per `RouteValidator.Validate`). Polygon checks within a polygon: nulls → (0,0) → range → ordering.
|
||||
- **Single-violation messages**: identical strings to the pre-refactor version; `RouteServiceTests` keeps using `WithMessage("*substring*")` and matches.
|
||||
- **Response shape**: `RouteResponse` properties set in identical order with identical values. JSON serialization is unaffected.
|
||||
- **Idempotency**: `GetRouteAsync(request.Id)` short-circuit at the top of `CreateRouteAsync` is preserved verbatim (AZ-362 AC).
|
||||
- **Logging**: `_logger.LogInformation("Idempotent route POST: id {RouteId} ...", request.Id)` log line preserved.
|
||||
- **Geofence loop**: `polygonIndex` propagated to `LinkRouteToRegionAsync(... geofencePolygonIndex: polygonIndex)` exactly as before — the routing-to-polygon mapping in `route_regions` is unchanged.
|
||||
|
||||
## Architecture / SRP impact
|
||||
|
||||
- `RouteService` shrunk from **295 → 138 lines** (~53% reduction). It is now an orchestrator with no inline validation, no inline interpolation logic, no inline grid math, and no inline DTO assembly.
|
||||
- Four new SRP-clean helpers in the same component (`Services.RouteManagement`). Each is independently unit-testable (8 / 6 / 11 / 4 tests).
|
||||
- No new external dependencies. No cross-component imports added — all helpers reference only `SatelliteProvider.Common.{DTO, Utils}` and (for the mapper) `SatelliteProvider.DataAccess.Models`, all within the `Imports from: Common, DataAccess` envelope declared by `module-layout.md`.
|
||||
- No new cyclic dependencies introduced.
|
||||
- DRY win: the entity → DTO mapping that previously lived in two places (`CreateRouteAsync` and `GetRouteAsync`) is now in one place (`RouteResponseMapper.Map`).
|
||||
|
||||
## Per-batch code review (inline)
|
||||
|
||||
Standalone `/code-review` invocation skipped because:
|
||||
- All four helpers are extracted-from-existing logic, no new external integration.
|
||||
- Behavior preservation is verified end-to-end by the existing `RouteServiceTests` (unchanged) plus the integration smoke run.
|
||||
- The 28 new unit tests directly attest to each helper's contract.
|
||||
|
||||
Reduced 7-phase review (inline):
|
||||
|
||||
- **Spec compliance** — AC-1 / AC-2 / AC-3 / AC-4 all satisfied (table above).
|
||||
- **Code quality** — SRP improved; helper methods <30 LOC each; explicit `ArgumentNullException.ThrowIfNull` guards on public entry points; no bare catches; no dead code introduced.
|
||||
- **Security** — no new attack surface. Validator strengthens input validation by aggregating (multiple bad fields surface together); 400 still emitted via `RouteValidator → ArgumentException → Program.cs CreateRoute catch path`. AZ-353 sanitized 5xx still applies for unexpected errors.
|
||||
- **Performance** — net zero. Same algorithmic complexity. One additional `IReadOnlyList` materialization in `RouteResponseMapper.Map` (`points as List<RoutePointDto> ?? points.ToList()`) — O(N), bounded by route size.
|
||||
- **Cross-task consistency** — solo batch, no inter-task drift. Style matches the recent extractions (TileCsvWriter, RegionFailureClassifier, GlobalExceptionHandler) — public class, explicit constructor injection, `Arrange / Act / Assert` test layout.
|
||||
- **Architecture** — RouteManagement component boundary respected. `module-layout.md` `Imports from: Common, DataAccess` invariant preserved. No new project references in any csproj. Public API surface of RouteManagement grew by 4 types but they all live under `SatelliteProvider.Services.RouteManagement` namespace and are not consumed cross-component (they're internal collaborators of `RouteService`).
|
||||
|
||||
**Verdict**: PASS. No findings.
|
||||
|
||||
## Up next
|
||||
|
||||
- **Cumulative K=3 review** fires now (window = batches 13 + 14 + 15 = AZ-368 + AZ-369 + AZ-365). Output: `_docs/03_implementation/cumulative_review_batches_13-15_cycle1_report.md`.
|
||||
- **Batch 16 candidate**: AZ-377 (C24 Earth constants, 2 SP) is blocked by AZ-371. The next un-blocked Phase 3 task is AZ-367 (C14 TileGridStitcher, 3 SP) which is itself listed with `Depends On: AZ-364`. Inspecting `AZ-364`'s dependency: `AZ-364 Depends On: AZ-366, AZ-367 (folds in AZ-360)` — the dependency edge is reversed in practice (AZ-367 unblocks AZ-364, not the other way around) — confirmed in batch 14's "Up next" notes. So the next runnable Phase 3 task is **AZ-367 (TileGridStitcher, 3 SP)**, then **AZ-364 (RouteProcessingService god-class, 5 SP, folds AZ-360)**. AZ-377 floats into Phase 4 once AZ-371 lands.
|
||||
- After Phase 3 completes, Phase 4 runs the typing/config/tooling/polish track.
|
||||
Reference in New Issue
Block a user