satellite-provider/_docs/03_implementation/reviews/batch_03_cycle8_review.md

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.