mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 22:41:15 +00:00
[AZ-794] [AZ-795] [AZ-796] Strict input validation + z/x/y rename
AZ-794: rename inventory wire fields tileZoom/tileX/tileY -> z/x/y to match the slippy-map URL convention. Contract bumped to v2.0.0. AZ-795: shared validation infrastructure -- FluentValidation + ValidationEndpointFilter + GlobalValidatorConfig (camelCase paths). GlobalExceptionHandler now converts JsonException (UnmappedMember + JsonRequired) into RFC 7807 ValidationProblemDetails. JSON layer hardened with UnmappedMemberHandling.Disallow + camelCase naming policy. New error-shape.md contract. AZ-796: InventoryRequestValidator covers 9 rules (XOR tiles vs locationHashes, cap 1000, z 0..22, x/y in slippy bounds, hash length/charset). 16 unit tests + 16 integration tests + a manual curl probe script. Adjacent fixes uncovered by the new strict layer: - IdempotentPostTests RoutePoint payload corrected to lat/lon (the DTO has used JsonPropertyName for ages; previously silently ignored under PascalCase fallback). - TileInventoryTests slippy x/y reduced to fit z=18 bounds. - docker-compose.yml host port for Postgres moved 5432 -> 5433 to avoid sibling-project conflict; appsettings.Development + README + AGENTS + architecture + containerization docs aligned. New coderule (suite + repo): API consumer-facing OpenAPI descriptions must not contain task IDs, contract filenames, or version-bump history -- internal change tracking belongs in commits/contract docs/changelogs. Existing offending descriptions in Program.cs cleaned up. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,153 @@
|
||||
# Strict input validation across all public endpoints (FluentValidation + ProblemDetails)
|
||||
|
||||
**Task**: AZ-795_strict_validation_epic
|
||||
**Name**: Strict input validation across all public endpoints
|
||||
**Type**: Epic
|
||||
**Description**: Every public HTTP endpoint must reject malformed input with structured 4xx errors instead of silently coercing missing fields to zero / ignoring unknown fields. Recommended approach: FluentValidation + global ProblemDetails filter + `JsonSerializerOptions.UnmappedMemberHandling.Disallow`.
|
||||
**Complexity**: — (epic; rolls up children. Estimate: 5–8 pts shared infra + ~3 pts per per-endpoint child)
|
||||
**Dependencies**: — (per-endpoint children depend on shared infra landing first)
|
||||
**Component**: SatelliteProvider.Api (DI wiring + global filter + DTOs + validators)
|
||||
**Tracker**: AZ-795 (https://denyspopov.atlassian.net/browse/AZ-795)
|
||||
**Children**: AZ-796 (inventory endpoint — first concrete child); sibling per-endpoint tasks to be added by parent-suite team
|
||||
**Originating ticket**: gps-denied-onboard AZ-777 Phase 1 (Jetson probe, 2026-05-22)
|
||||
|
||||
## Origin
|
||||
|
||||
Discovered during gps-denied-onboard AZ-777 Phase 1 Jetson probing on 2026-05-22. A hand-typed inventory request with the wrong field names (`{"z","x","y"}` instead of the current `{"tileZoom","tileX","tileY"}`) returned **HTTP 200** with `(0,0,0)` coordinates and an identical `locationHash` for every entry. Real client bugs masquerade as valid results because the deserializer silently treats unknown fields as missing and missing fields as `default(int) = 0`.
|
||||
|
||||
For a service that's the single source of truth about which satellite tiles exist, permissive parsing is actively dangerous: corruption downstream, confident wrong answers, hours of debugging on the consumer side.
|
||||
|
||||
Jira AZ-795 is the authoritative spec; this file mirrors the in-workspace-only sections that the satellite-provider implementer will need.
|
||||
|
||||
## Problem
|
||||
|
||||
Every public-facing JSON endpoint on satellite-provider inherits the same Postel-permissive parsing default:
|
||||
- Missing required fields → silently `default(T)` (e.g. `0` for `int`).
|
||||
- Unknown fields → silently dropped (no `[JsonExtensionData]` capture, no log entry).
|
||||
- Wrong types → silently coerced where possible, silently dropped where not.
|
||||
|
||||
No structured error response. The only contract-level signal a misbehaving client gets today is downstream weirdness (wrong `locationHash`, repeated identical results, etc.) — many hops away from the actual cause.
|
||||
|
||||
## Outcome
|
||||
|
||||
- Every public-facing JSON endpoint rejects malformed input with **HTTP 400 + RFC 7807 ProblemDetails** body naming the offending field(s).
|
||||
- Validators are testable in isolation (unit tests per `RuleFor`) and enforced by the HTTP layer without per-controller try/catch boilerplate.
|
||||
- Unknown-field rejection is wired at the deserializer level so typos can't reach a validator.
|
||||
- Uniform error response shape across all endpoints.
|
||||
- New `_docs/02_document/contracts/api/error-shape.md` v1.0.0 documenting the ProblemDetails contract every endpoint conforms to.
|
||||
|
||||
## Recommended approach
|
||||
|
||||
1. **FluentValidation** for input DTOs (declarative, composable, validators are testable units). Final stack choice belongs to the parent-suite team; if FluentValidation is ruled out by existing constraints, alternatives are stock DataAnnotations + custom model binders or hand-written `IValidator<T>`.
|
||||
2. **Global error filter / ASP.NET model-state behavior** that emits RFC 7807 ProblemDetails for every validation failure. No per-endpoint try/catch boilerplate.
|
||||
3. **Unknown-field rejection** at the deserializer: `JsonSerializerOptions.UnmappedMemberHandling.Disallow` (.NET 8+) or `Newtonsoft.Json` `MissingMemberHandling.Error`. Catches typos like `{"Z":12}` (uppercase) that no validator can catch after deserialization.
|
||||
|
||||
## Error response contract (uniform across all endpoints)
|
||||
|
||||
```json
|
||||
{
|
||||
"type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
|
||||
"title": "One or more validation errors occurred.",
|
||||
"status": 400,
|
||||
"errors": {
|
||||
"tiles[0].z": ["The z field is required."],
|
||||
"tiles[1]": ["Unexpected field: 'tileZoom'."]
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Stable enough for consumers to pattern-match. Field names in `errors` paths must use the same casing as the request body (post-AZ-794 short names for the inventory endpoint).
|
||||
|
||||
## Scope
|
||||
|
||||
### Included — Shared infrastructure (this epic owns)
|
||||
|
||||
- DI wiring for FluentValidation (or chosen alternative) in `SatelliteProvider.Api/Program.cs` (or appropriate composition root).
|
||||
- Global error filter / `Configure<ApiBehaviorOptions>(...)` for ProblemDetails formatting.
|
||||
- `JsonSerializerOptions` configuration for unknown-field rejection.
|
||||
- A validator-coverage table in `_docs/02_document/architecture.md` (or equivalent) listing each public endpoint and its validator class.
|
||||
- Shared test fixtures for ProblemDetails assertions in `SatelliteProvider.IntegrationTests`.
|
||||
- New contract artifact: `_docs/02_document/contracts/api/error-shape.md` v1.0.0 (the ProblemDetails shape every endpoint conforms to).
|
||||
|
||||
### Included — Per-endpoint child tasks
|
||||
|
||||
- One child Task per public-facing endpoint that has a JSON body. Each consumes the shared infra.
|
||||
- Each child uses the AC template below.
|
||||
- Parent-suite team enumerates the full endpoint surface from `/swagger/v1/swagger.json` / route map and creates the children.
|
||||
- First child (concrete reference implementation): **AZ-796** — inventory endpoint.
|
||||
|
||||
### Excluded
|
||||
|
||||
- Authentication / authorization changes (JWT contract owned by AZ-494).
|
||||
- Endpoint renaming (**AZ-794** owns the inventory body-field rename).
|
||||
- Rate-limiting / quota (separate concern).
|
||||
- Internal-only admin endpoints, health probes, metrics scrapers (parent-suite team owns in/out decision per endpoint).
|
||||
|
||||
## Acceptance Criteria template (every child task must satisfy)
|
||||
|
||||
**AC-1: Missing required field → 400**
|
||||
Given a POST body that omits a required field
|
||||
When the endpoint is called
|
||||
Then HTTP 400 with `errors.<field>` listing the missing field.
|
||||
|
||||
**AC-2: Unknown field → 400**
|
||||
Given a POST body with an unrecognized field at root or in nested objects
|
||||
When the endpoint is called
|
||||
Then HTTP 400 with `errors[].<location>` naming the unexpected field.
|
||||
|
||||
**AC-3: Wrong type → 400**
|
||||
Given a POST body with a field of unexpected JSON type (e.g. string where integer expected)
|
||||
When the endpoint is called
|
||||
Then HTTP 400 with `errors.<field>` describing the type mismatch.
|
||||
|
||||
**AC-4: Out-of-range value → 400**
|
||||
Given a POST body with a value outside its supported range
|
||||
When the endpoint is called
|
||||
Then HTTP 400 with `errors.<field>` describing the valid range.
|
||||
|
||||
**AC-5: Empty array where non-empty required → 400**
|
||||
Given a POST body where a required non-empty collection is empty
|
||||
When the endpoint is called
|
||||
Then HTTP 400 with `errors.<field>` describing the constraint.
|
||||
|
||||
**AC-6: Validator class is its own file + unit-tested**
|
||||
A `IValidator<RequestDto>` (or equivalent) class exists in its own file under `SatelliteProvider.Api/Validators/` (or per-suite convention), with a unit test per `RuleFor(...)`.
|
||||
|
||||
**AC-7: Integration tests cover one happy + one failure path per AC**
|
||||
`SatelliteProvider.IntegrationTests` adds a fixture that POSTs each bad-payload variant and asserts `status == 400` + ProblemDetails shape + specific `errors[].<location>` path.
|
||||
|
||||
**AC-8: OpenAPI / Swagger spec accuracy**
|
||||
`/swagger/v1/swagger.json` marks required fields, declares ranges, and documents the new 400 response shape.
|
||||
|
||||
## Test requirements
|
||||
|
||||
- **Unit**: one xUnit class per validator. Tests cover each `RuleFor(...)` / equivalent.
|
||||
- **Integration**: `SatelliteProvider.IntegrationTests` adds one fixture per endpoint covering all AC variants (~7–10 new tests per endpoint).
|
||||
- **Contract**: OpenAPI spec snapshot test confirms the published schema rejects what the validator rejects.
|
||||
- **Cross-cutting**: shared `ProblemDetailsAssertions` helper in test infra so every endpoint's failure tests use the same assertion shape.
|
||||
|
||||
## Migration / breaking-change strategy
|
||||
|
||||
Tightening validation is a **breaking behavior change**: clients that today get 200 OK with nonsense will start getting 400. Three approaches the parent-suite team can pick from:
|
||||
|
||||
1. **Hard switch** — ship in one release with a clear "Breaking" note. Cleanest for low-consumer-count (currently 1).
|
||||
2. **Soft warning then enforce** — log warnings for one release when malformed input arrives; enforce in the next.
|
||||
3. **API versioning** — keep `/v1` permissive, add `/v2` strict, migrate consumers, remove `/v1`.
|
||||
|
||||
Recommendation: **#1** while the consumer set is small (currently 1 known: `gps-denied-onboard`).
|
||||
|
||||
## Constraints
|
||||
|
||||
- Shared infra MUST land before any per-endpoint child task — children are gated on it.
|
||||
- Coordinate with **AZ-794** (inventory rename) — recommended ordering ships AZ-794 first so this epic's validators use the final names from day one.
|
||||
- Parent-suite team enumerates the full consumer set before deciding rollout cadence (not just `gps-denied-onboard`).
|
||||
- Per-endpoint child tasks added by parent-suite team after enumerating endpoint surface from OpenAPI / route map. Do NOT create all children up-front — let them be added as the team decomposes.
|
||||
|
||||
## References
|
||||
|
||||
- Jira AZ-795: https://denyspopov.atlassian.net/browse/AZ-795
|
||||
- First child: AZ-796 (inventory endpoint)
|
||||
- Related: AZ-794 (inventory rename), AZ-777 (originating discovery in gps-denied-onboard)
|
||||
- Originating discovery: gps-denied-onboard AZ-777 Phase 1 Jetson probe (2026-05-22)
|
||||
- ASP.NET ProblemDetails reference: https://learn.microsoft.com/en-us/aspnet/core/web-api/handle-errors
|
||||
- FluentValidation reference: https://docs.fluentvalidation.net/
|
||||
Reference in New Issue
Block a user