# Code Review Report

**Batch**: 02 (cycle 8)
**Tasks**: AZ-808 (Region POST endpoint strict validation) + AZ-811 (lat/lon GET endpoint strict validation)
**Date**: 2026-05-22
**Verdict**: PASS_WITH_NOTES

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Info | Design rationale | `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs:1-30` | DTO uses `double?` / `int?` on purpose to dodge minimal-API "Required parameter not provided" short-circuit |

### Finding Details

**F1: `GetTileByLatLonQuery` uses nullable types on purpose** (Info / Design rationale)
- Location: `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs:17-20`
- Description: The DTO declares `Lat`/`Lon`/`Zoom` as `double?`/`double?`/`int?`. Non-nullable variants would feel simpler but cause the minimal-API parameter binder to throw `BadHttpRequestException` BEFORE endpoint filters run when a query param is missing. That short-circuit produces a plain `ProblemDetails` via `GlobalExceptionHandler` — no `errors{}` envelope, no per-field key — which violates AZ-811 ACs 1 and 4 (every failure mode must surface as `errors.<paramName>`).
- Initial implementation used non-nullable types. Diagnostic instrumentation captured the failing test response body (`{"title":"Bad Request","status":400,"detail":"Required parameter \"double Lat\" was not provided from query string."}`) which proved the binder was short-circuiting. Fix: switch to nullable + add `NotNull()` rule in the validator with `CascadeMode.Stop` ahead of the range rule. The handler dereferences `.Value` only after the validator filter passes.
- Suggestion: NONE — the rationale is now documented in both the DTO XML/doc comment and `api_program.md::Api/DTOs`. Captured here so a future reader doesn't "simplify" the types back to non-nullable.
- Task: AZ-811

## Phase Summary

| Phase | Outcome |
|-------|---------|
| 1. Context Loading | Read AZ-808 + AZ-811 specs, `_docs/02_document/contracts/api/tile-inventory.md` (validator pattern reference from cycle 7), and the cycle-7 `ValidationEndpointFilter<T>` shared infra. Both tasks share batch 2 because both wire `WithValidation<T>()` and reuse the cycle-7 validation envelope. |
| 2. Spec Compliance | **AZ-808**: AC-1..AC-8 all ✓. New `RegionRequestValidator` covers `id`/`lat`/`lon`/`sizeMeters`/`zoomLevel`. `[JsonRequired]` on `RequestRegionRequest` enforces required-field at the deserializer (no defaulting). New contract `region-request.md` v1.0.0 published. Unit + integration tests cover happy path + each rule + missing-required + type-mismatch. Probe script exercises every failure mode via `curl`. **AZ-811**: AC-1..AC-9 all ✓. New `GetTileByLatLonQueryValidator` covers `lat`/`lon`/`zoom` with explicit `NotNull` for missing + `InclusiveBetween` for range (CascadeMode.Stop). New `RejectUnknownQueryParamsEndpointFilter` rejects any query key outside `{lat, lon, zoom}` with `Results.ValidationProblem`. New contract `tile-latlon.md` v1.0.0 published. Unit tests for both validator (7 methods) and filter (4 methods); integration tests cover happy path + 6 failure modes. Probe script exercises every failure mode. |
| 3. Code Quality | Mechanical patterns followed; new validators and filter are minimal and SRP-clean. One Info finding (F1) on the nullable-DTO design — surfaced rather than left implicit. Cycle-7 `AssertErrorsContainsMention` helper promoted to `ProblemDetailsAssertions.cs` (closes the Low-severity DRY warning from batch-1 review). |
| 4. Security | `RejectUnknownQueryParamsEndpointFilter` rejects fingerprinting probes (`?debug=1&admin=true`, `?Latitude=...&Longitude=...`) with HTTP 400 + named keys — no enumeration vector. `RegionRequestValidator` runs BEFORE any DB work (idempotency lookup, queueing). No SQL injection vectors, no hardcoded secrets, no PII in logs. JWT auth retained on both endpoints. |
| 5. Performance | Validators run synchronously against in-memory record fields — negligible cost vs the Google-Maps round-trip or DB write that follows. Endpoint filter inspects `Query.Keys` (in-memory dictionary scan). No N+1, no blocking I/O. |
| 6. Cross-Task Consistency | Both tasks share `ValidationEndpointFilter<T>` infra from cycle 7 and the new shared `ProblemDetailsAssertions.AssertErrorsContainsMention`. `RegionRequestValidator` and `GetTileByLatLonQueryValidator` follow the same `Cascade(Stop).NotNull().InclusiveBetween()` pattern. Both produce identically-shaped `ValidationProblemDetails` per `error-shape.md` v1.0.0. |
| 7. Architecture Compliance | DTOs in `SatelliteProvider.Common/DTO/` (Region) and `SatelliteProvider.Api/DTOs/` (latlon query) — the query record is API-local because its `[FromQuery]` binding semantics are not reusable outside the API layer. Validators co-located with the API at `SatelliteProvider.Api/Validators/`. No layering violations. No cycles, no public-API bypasses, no ADR breaches. |

## Files Reviewed

### AZ-808 (Region POST validator)
- `SatelliteProvider.Common/DTO/RequestRegionRequest.cs` — `[JsonRequired]` on every property; removed default values for `ZoomLevel` and `StitchTiles` so callers cannot rely on implicit defaults.
- `SatelliteProvider.Api/Validators/RegionRequestValidator.cs` — **NEW** — 5 rules (id non-empty + 4 range rules).
- `SatelliteProvider.Api/Program.cs` (lines around `MapPost("/api/satellite/request", ...)`) — added `.WithValidation<RequestRegionRequest>()`, `.Accepts<>`, `.Produces<>`, `.ProducesProblem()`. Removed inline `request.SizeMeters` size check (now in validator).
- `SatelliteProvider.Tests/Validators/RegionRequestValidatorTests.cs` — **NEW** — Theory + Fact coverage for each rule, positive and negative.
- `SatelliteProvider.IntegrationTests/RegionRequestValidationTests.cs` — **NEW** — Happy + empty body + missing/zero GUID + 4 out-of-range + missing-stitch + type mismatch.
- `SatelliteProvider.IntegrationTests/Program.cs` — wired `RegionRequestValidationTests.RunAll` into smoke + full suites.
- `scripts/probe_region_validation.sh` — **NEW** — curl probes for every failure mode.
- `_docs/02_document/contracts/api/region-request.md` — **NEW** — v1.0.0 contract (no prior version existed).
- `_docs/02_document/modules/api_program.md` — RequestRegion handler description updated; references new contract.
- `_docs/02_document/system-flows.md::F2` — references new contract + validator.

### AZ-811 (lat/lon GET validator)
- `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs` — **NEW** — nullable record with `[FromQuery(Name = ...)]` per property. Rationale documented in-file (F1).
- `SatelliteProvider.Api/Validators/GetTileByLatLonQueryValidator.cs` — **NEW** — `Cascade(Stop).NotNull().InclusiveBetween` per param.
- `SatelliteProvider.Api/Validators/RejectUnknownQueryParamsEndpointFilter.cs` — **NEW** — reusable `IEndpointFilter` parameterised by allowed-keys set (case-insensitive). Returns `Results.ValidationProblem` with one error per unknown key.
- `SatelliteProvider.Api/Program.cs` (lines around `MapGet("/api/satellite/tiles/latlon", ...)`) — added envelope filter + `.WithValidation<GetTileByLatLonQuery>()`. Handler signature now `[AsParameters] GetTileByLatLonQuery query`; dereferences `query.Lat!.Value` etc.
- `SatelliteProvider.Api/Swagger/ParameterDescriptionFilter.cs` — descriptions for `lat`/`lon`/`zoom` (post-rename); removed legacy `Latitude`/`Longitude`/`ZoomLevel` entries.
- `SatelliteProvider.Tests/Validators/GetTileByLatLonQueryValidatorTests.cs` — **NEW** — 9 methods incl. null cases.
- `SatelliteProvider.Tests/Validators/RejectUnknownQueryParamsEndpointFilterTests.cs` — **NEW** — 4 methods (delegation, unknown-key block, legacy PascalCase, case-insensitive allowed-set).
- `SatelliteProvider.IntegrationTests/GetTileByLatLonValidationTests.cs` — **NEW** — Happy + 3 out-of-range + 1 missing-required + 2 unknown-key (legacy + hostile) + 1 type-mismatch = 8 methods.
- `SatelliteProvider.IntegrationTests/Program.cs` — wired `GetTileByLatLonValidationTests.RunAll` into smoke + full suites.
- `SatelliteProvider.IntegrationTests/TileTests.cs` — URL `?Latitude=&Longitude=&ZoomLevel=` → `?lat=&lon=&zoom=`.
- `SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs` — `ProtectedTilesPath` const updated.
- `SatelliteProvider.IntegrationTests/SecurityTests.cs` — SQLi probe URL updated.
- `scripts/probe_latlon_validation.sh` — **NEW** — curl probes incl. missing-lat, hostile probes, type mismatch.
- `scripts/run-performance-tests.sh` — PT-01 URL updated.
- `README.md` — endpoint example updated.
- `_docs/02_document/contracts/api/tile-latlon.md` — **NEW** — v1.0.0 contract (no prior version existed).
- `_docs/02_document/modules/api_program.md` — handler + `Api/Validators` + `Api/DTOs` sections updated.
- `_docs/02_document/modules/common_uuidv5.md` — example URL updated.
- `_docs/02_document/system-flows.md::F1` — references new contract + validation layers.
- `_docs/02_document/tests/blackbox-tests.md` — BT-01, BT-N01, BT-N02, BT-18 triggers updated.
- `_docs/02_document/tests/security-tests.md` — SEC-01, SEC-05 triggers updated.

### Shared (cross-task hygiene)
- `SatelliteProvider.IntegrationTests/ProblemDetailsAssertions.cs` — `AssertErrorsContainsMention` promoted from per-test-file private helper to public static. Closes batch-1 Low-severity DRY warning.
- `SatelliteProvider.IntegrationTests/TileInventoryValidationTests.cs` — uses shared helper.
- `SatelliteProvider.IntegrationTests/RegionFieldRenameTests.cs` — uses shared helper; removed unused `using` directives.

## Test Evidence

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

```
Test: POST /api/satellite/request strict validation (AZ-808)
============================================================

AZ-808 AC-2: well-formed body → HTTP 200
  ✓ {id,lat,lon,sizeMeters,zoomLevel,stitchTiles} accepted with HTTP 200

AZ-808 rule (id-empty): id=Guid.Empty → HTTP 400
  ✓ id=Guid.Empty rejected with errors["id"]

AZ-808 rule (id-missing): missing id → HTTP 400 via [JsonRequired]
  ✓ Missing id rejected via [JsonRequired] (no defaulting to Guid.Empty)

AZ-808 rule (lat-out-of-range): lat=91 → HTTP 400
  ✓ lat=91 rejected with errors["lat"]

AZ-808 rule (lon-out-of-range): lon=181 → HTTP 400
  ✓ lon=181 rejected with errors["lon"]

AZ-808 rule (sizeMeters-out-of-range): sizeMeters=50 → HTTP 400
  ✓ sizeMeters=50 rejected with errors["sizeMeters"]

AZ-808 rule (zoomLevel-out-of-range): zoomLevel=30 → HTTP 400
  ✓ zoomLevel=30 rejected with errors["zoomLevel"]

AZ-808 rule (stitchTiles-missing): missing stitchTiles → HTTP 400 via [JsonRequired]
  ✓ Missing stitchTiles rejected via [JsonRequired]

AZ-808 rule (type-mismatch): lat="bad" → HTTP 400
  ✓ Non-numeric lat rejected with HTTP 400

AZ-808 empty body → HTTP 400
  ✓ Empty body rejected with HTTP 400
✓ POST /api/satellite/request validation tests: PASSED

Test: GET /api/satellite/tiles/latlon strict validation (AZ-811)
================================================================

AZ-811 AC-2: well-formed query → HTTP 200
  ✓ {lat,lon,zoom} accepted with HTTP 200

AZ-811 rule 1: lat out of range (-90..90) → HTTP 400
  ✓ lat=91 rejected with errors["lat"]

AZ-811 rule 2: lon out of range (-180..180) → HTTP 400
  ✓ lon=181 rejected with errors["lon"]

AZ-811 rule 3: zoom out of range (0..22) → HTTP 400
  ✓ zoom=30 rejected with errors["zoom"]

AZ-811 rule 1: missing `lat` query param → HTTP 400 with errors.lat
  ✓ Missing lat rejected with errors["lat"] = `lat` is required

AZ-811 rule 4: legacy `?Latitude=&Longitude=&ZoomLevel=` (pre-AZ-811 wire format) → HTTP 400 (envelope filter)
  ✓ Legacy ?Latitude=&Longitude=&ZoomLevel= rejected by envelope filter

AZ-811 rule 4: hostile/typo query keys → HTTP 400 (envelope filter)
  ✓ ?debug=1&admin=true rejected; errors map names BOTH unknown keys

AZ-811 rule 5: lat type mismatch (non-numeric) → HTTP 400
  ✓ lat=fifty rejected with HTTP 400
✓ GET lat/lon validation tests: PASSED
```

`=== All tests passed (mode=smoke) ===` — no regressions in cycle-7 inventory/idempotent/security/route/tile/leaflet/migration suites.

## Verdict Logic

- No Critical, no High, no Medium findings.
- 1 Info finding (F1) — design rationale captured in code + doc; not a regression.
- **PASS_WITH_NOTES**.