# Code Review Report

**Batch**: 03 (cycle 8)
**Tasks**: AZ-809 (POST /api/satellite/route strict validation)
**Date**: 2026-05-22
**Verdict**: PASS_WITH_NOTES

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | API alignment | `SatelliteProvider.Api/Validators/CreateRouteRequestValidator.cs:68-77` | `OverridePropertyName` is required on deep expressions because FluentValidation drops the parent path on `req.Geofences!.Polygons` |
| 2 | Info | Defence-in-depth | `SatelliteProvider.Services.RouteManagement/RouteValidator.cs` (existing) | Service-layer `RouteValidator` is now strictly weaker than the cycle-8 `CreateRouteRequestValidator` and could be deleted, but is retained as a defence-in-depth backstop |
| 3 | Info | Wire-shape asymmetry | `SatelliteProvider.Common/DTO/RoutePoint.cs` (input) vs `SatelliteProvider.Common/DTO/RoutePointDto.cs` (output) | The input wire uses short OSM `lat`/`lon`; the response wire uses long `latitude`/`longitude`. Pre-existing — AZ-809 documented but did not unify |

### Finding Details

**F1: `OverridePropertyName` is mandatory on the geofences chain** (Low / API alignment)
- Location: `SatelliteProvider.Api/Validators/CreateRouteRequestValidator.cs:68-77` and `SatelliteProvider.Api/Validators/RoutePointValidator.cs:30-40`.
- Description: FluentValidation's default property-name policy drops the parent on deep member expressions like `req => req.Geofences!.Polygons`. Without `OverridePropertyName("geofences.polygons")`, the error keys emitted are leaf-only (`polygons`, `polygons[0].northWest`) instead of the full wire path the spec mandates (`geofences.polygons`, `geofences.polygons[0].northWest`). The fix lives in code AND in a comment explaining WHY; without the comment a future reader would "simplify" the rule chain and silently break wire compatibility. Same pattern applies to `RoutePointValidator` where C# property `Latitude` must surface as wire `lat` — handled by `OverridePropertyName` chained AFTER the first concrete rule (a generic-type-inference quirk: the extension is defined on `IRuleBuilderOptions<T, TProperty>`, which only becomes inferable after the first `.InclusiveBetween()` etc.).
- Suggestion: NONE — rationale captured in code comments AND in `api_program.md::Api/Validators` so the next reader cannot break it by accident.
- Task: AZ-809

**F2: Service-layer `RouteValidator` retained as defence-in-depth** (Info / Defence-in-depth)
- Location: `SatelliteProvider.Services.RouteManagement/RouteValidator.cs`.
- Description: The pre-cycle-8 service-layer validator (`RouteValidator`) covered approximately the same surface as the new `CreateRouteRequestValidator` (id non-empty, points count ≥ 2, geofence corner sanity). Now that the API layer rejects every invalid request before the service runs, `RouteValidator` is strictly redundant for HTTP-driven paths. It is, however, also called from the route processing service (background queue) where some bypass path could in principle smuggle an invalid payload — keeping it as a backstop costs ~30 lines and one extra unit-test pass. Removal is tracked as an advisory in `route-creation.md` ("Validator Cleanup Advisory") so the next cycle can decide whether to consolidate.
- Suggestion: Defer to a follow-up PBI. Do NOT delete in this batch.
- Task: AZ-809

**F3: Input/output naming asymmetry on route points** (Info / Wire-shape asymmetry)
- Location: `SatelliteProvider.Common/DTO/RoutePoint.cs` (input) vs `SatelliteProvider.Common/DTO/RoutePointDto.cs` (output).
- Description: Request points use `[JsonPropertyName("lat")]` / `[JsonPropertyName("lon")]`; response points serialize the underlying C# `Latitude`/`Longitude` properties verbatim. This asymmetry existed before cycle 8. AZ-809 documents it in `route-creation.md` v1.0.0 and `common_dtos.md`, but does not unify because changing the response wire would be a breaking change to existing clients of `GET /api/satellite/route/{id}`. Tracked as an advisory.
- Suggestion: Open a successor PBI to consider unifying via a `lat`/`lon` rename on `RoutePointDto` (would be a v2.0.0 of `route-creation.md`).
- Task: AZ-809

## Phase Summary

| Phase | Outcome |
|-------|---------|
| 1. Context Loading | Read AZ-809 spec, `_docs/02_document/contracts/api/region-request.md` (batch-2 pattern), `_docs/02_document/contracts/api/error-shape.md` (failure shape), and the cycle-7 + cycle-8 (batch-2) validation infra. The route endpoint differs from batch-2 endpoints because it has nested DTOs (RoutePoint, Geofences/GeofencePolygon/GeoPoint) requiring child validators. |
| 2. Spec Compliance | All 9 ACs ✓. New `CreateRouteRequestValidator` covers the 14 documented rules across deserializer-layer + validator-layer. Nested `RoutePointValidator` + `GeofencePolygonValidator` co-validators wired via `RuleForEach.SetValidator(...)`. Cross-field invariants enforced at both the root (`createTilesZip ⇒ requestMaps`) and per-polygon (`NW.Lat > SE.Lat`, `NW.Lon < SE.Lon`). New contract `route-creation.md` v1.0.0 published. 16 integration tests + 26 unit tests cover happy path + each rule + missing-required + type-mismatch + cross-field. Probe script exercises every failure mode via `curl`. |
| 3. Code Quality | Mechanical patterns followed; three new validators are minimal and SRP-clean. The `OverridePropertyName` requirement on deep expressions (F1) is non-obvious and was discovered via failing unit tests + diagnostic instrumentation; the workaround is captured in both code comments and module docs so it cannot be silently regressed. `RoutePointValidator` and `GeofencePolygonValidator` are file-private — inner `GeoCornerValidator` is nested inside `GeofencePolygonValidator` because the polygon corners are its only consumer; if a future sibling endpoint needs point-shape validation, the spec says to promote and rename. |
| 4. Security | Validators run BEFORE any DB work (route persistence, intermediate-point computation, queue enqueue). The cross-field invariants prevent NaN-geometry payloads from reaching the GeoUtils interpolator (which is not designed for NW=SE corners). No SQL injection vectors, no hardcoded secrets, no PII in logs. JWT auth retained on the endpoint. Probe script tests `?debug=1` / extra root fields → all rejected. |
| 5. Performance | Validators run synchronously against in-memory record fields — negligible cost (microseconds) vs the route's interpolation pass + DB inserts. Even worst-case `points.Count = 500` with all `geofences.polygons.Count` runs ~500 microsecond. No N+1, no blocking I/O. |
| 6. Cross-Task Consistency | Uses the same `ValidationEndpointFilter<T>` infra from cycle 7 + batch-2 of cycle 8 and the shared `ProblemDetailsAssertions.AssertErrorsContainsMention`. Error keys follow the same camelCase JSON-path policy (`points[i].lat`, `geofences.polygons[i].northWest`) per `error-shape.md` v1.0.0 Inv-4. All produce identically-shaped `ValidationProblemDetails` bodies. |
| 7. Architecture Compliance | Route DTOs live in `SatelliteProvider.Common/DTO/` (shared with the service layer + integration tests). Validators co-located with the API at `SatelliteProvider.Api/Validators/`. No layering violations. The service-layer `RouteValidator` retention is documented as defence-in-depth (F2). No cycles, no public-API bypasses, no ADR breaches. |

## Files Reviewed

### AZ-809 (route-creation validator)
- `SatelliteProvider.Common/DTO/CreateRouteRequest.cs` — `[JsonRequired]` on every non-optional axis. Removed implicit defaults so callers cannot rely on them.
- `SatelliteProvider.Common/DTO/RoutePoint.cs` — `[JsonRequired]` on Latitude/Longitude.
- `SatelliteProvider.Common/DTO/GeofencePolygon.cs` — `[JsonRequired]` on `NorthWest`/`SouthEast` in `GeofencePolygon`; `[JsonRequired]` on `Polygons` in `Geofences`.
- `SatelliteProvider.Common/DTO/GeoPoint.cs` — `[JsonRequired]` on Lat/Lon (used by `GeofencePolygon` corners).
- `SatelliteProvider.Api/Validators/CreateRouteRequestValidator.cs` — **NEW** — 7 root rules (id non-empty + 4 range rules + points count + cross-field) plus `RuleForEach(req => req.Points).SetValidator(...)` and `RuleForEach(req => req.Geofences!.Polygons).SetValidator(...).OverridePropertyName(...)` for nested chains.
- `SatelliteProvider.Api/Validators/RoutePointValidator.cs` — **NEW** — `OverridePropertyName("lat"/"lon")` chained AFTER `.InclusiveBetween()` so the type parameter is inferable.
- `SatelliteProvider.Api/Validators/GeofencePolygonValidator.cs` — **NEW** — `CascadeMode.Stop` + `NotNull` + nested `GeoCornerValidator` for per-corner ranges; cross-field `Must` rules `.WithName("northWest")` for the invariant errors.
- `SatelliteProvider.Api/Program.cs` (lines around `MapPost("/api/satellite/route", ...)`) — added `.WithValidation<CreateRouteRequest>()`, `.Accepts<>`, `.Produces<>`, `.ProducesProblem()`.
- `SatelliteProvider.Tests/Validators/CreateRouteRequestValidatorTests.cs` — **NEW** — Theory + Fact coverage for each rule, positive and negative; 16 methods. Diagnostic-led: two assertions were converted from `polygons` / `polygons[0].northWest` to `geofences.polygons` / `geofences.polygons[0].northWest` after `OverridePropertyName` was added.
- `SatelliteProvider.Tests/Validators/RoutePointValidatorTests.cs` — **NEW** — 4 methods (lat/lon range, positive + negative).
- `SatelliteProvider.Tests/Validators/GeofencePolygonValidatorTests.cs` — **NEW** — 6 methods incl. NotNull on corners, range on corners, NW-of-SE invariants.
- `SatelliteProvider.IntegrationTests/CreateRouteValidationTests.cs` — **NEW** — Happy + empty body + missing/zero GUID + 4 out-of-range + insufficient-points + per-point lat/lon out-of-range + geofence invariant + missing-requestMaps + cross-field createTilesZip + unknown-root + nested type-mismatch = 16 methods.
- `SatelliteProvider.IntegrationTests/Program.cs` — wired `CreateRouteValidationTests.RunAll` into smoke + full suites.
- `scripts/probe_route_validation.sh` — **NEW** — curl probes for every failure mode + happy path.
- `_docs/02_document/contracts/api/route-creation.md` — **NEW** — v1.0.0 contract (no prior version existed). Includes the nested DTO chain + invariants + per-field test cases table + advisory on the legacy `RouteValidator` + the input/output naming asymmetry.
- `_docs/02_document/modules/api_program.md` — `CreateRoute Handler` section added; `Api/Validators` section bumped to AZ-808/AZ-809/AZ-811.
- `_docs/02_document/modules/common_dtos.md` — `CreateRouteRequest`/`RoutePoint`/`Geofences`/`GeofencePolygon`/`GeoPoint` descriptions updated with `[JsonRequired]` markers and constraint summaries.
- `_docs/02_document/system-flows.md::F4` — sequence diagram extended with the validation-filter branch; preconditions + error scenarios reference the new contract.
- `_docs/02_document/tests/blackbox-tests.md::BT-06/BT-N03/BT-N04/BT-N05` — triggers and pass criteria align with the new wire format + named error keys.
- `_docs/02_document/tests/security-tests.md::SEC-04` — references `GlobalExceptionHandler`'s JsonException branch + AZ-353 correlationId.

## Test Evidence

`./scripts/run-tests.sh --smoke` (`integration-tests` container exit code 0):

```
Test: POST /api/satellite/route strict validation (AZ-809)
==========================================================

AZ-809 AC-2: well-formed body → HTTP 200 (no background processing — requestMaps=false)
  ✓ Well-formed body accepted with HTTP 200

AZ-809 rule 1: empty body → HTTP 400
  ✓ Empty body rejected with HTTP 400

AZ-809 rule 2 (probe-confirmed gap): missing `id` → HTTP 400 (no silent zero-Guid coercion)
  ✓ Missing `id` rejected with HTTP 400 (no silent coercion)

AZ-809 rule 2: zero-Guid `id` → HTTP 400
  ✓ Zero-Guid `id` rejected with errors["id"]

AZ-809 rule 3: empty `name` → HTTP 400
  ✓ Empty `name` rejected with errors["name"]

AZ-809 rule 5: `regionSizeMeters` out of range (100..10000) → HTTP 400
  ✓ `regionSizeMeters=1000000` rejected with errors["regionSizeMeters"]

AZ-809 rule 6: `zoomLevel` out of range (0..22) → HTTP 400
  ✓ `zoomLevel=30` rejected with errors["zoomLevel"]

AZ-809 rule 7: `points` count < 2 → HTTP 400
  ✓ `points` count=1 rejected with errors["points"]

AZ-809 rule 8: per-point `lat` out of range → HTTP 400 (errors[points[i].lat])
  ✓ `points[1].lat=91` rejected with errors["points[1].lat"]

AZ-809 rule 8: per-point `lon` out of range → HTTP 400 (errors[points[i].lon])
  ✓ `points[1].lon=181` rejected with errors["points[1].lon"]

AZ-809 rule 9: geofence NW.lat <= SE.lat → HTTP 400 (cross-field invariant)
  ✓ NW.lat <= SE.lat rejected by cross-field invariant

AZ-809 rule 10: missing `requestMaps` → HTTP 400 (no defaulting)
  ✓ Missing `requestMaps` rejected

AZ-809 rule 12: `createTilesZip=true` AND `requestMaps=false` → HTTP 400 (cross-field invariant)
  ✓ `createTilesZip=true requestMaps=false` rejected by cross-field invariant

AZ-809 rule 13: unknown root field → HTTP 400 (UnmappedMemberHandling.Disallow)
  ✓ Unknown root field `debug` rejected with errors mention

AZ-809 rule 14: nested type mismatch (`points[0].lat` as string) → HTTP 400
  ✓ `points[0].lat:"fifty"` rejected with HTTP 400
✓ Create-route validation tests: PASSED
```

`=== All tests passed (mode=smoke) ===` — no regressions in cycle-7 inventory or batch-1/batch-2 cycle-8 (AZ-812/AZ-808/AZ-811) tests, no regressions in the migration/leaflet/route/tile/security suites.

## Verdict Logic

- No Critical, no High, no Medium findings.
- 1 Low finding (F1) — the `OverridePropertyName` requirement is captured in code comments + module docs; not a regression, but worth flagging so it cannot be silently regressed.
- 2 Info findings (F2 defence-in-depth retention, F3 input/output naming asymmetry) — both pre-existing, documented as advisories for a follow-up PBI.
- **PASS_WITH_NOTES**.