- build_error_schemas: default/wildcard response keys dropped with a warn instead of emitting a dead HTTP_0 ErrorDefinition — /search never advertises a code that can't match (the runtime mapper already synthesizes HTTP_<actual> for unmapped statuses) - check_parameter_style: non-default style/explode parameter forms (spaceDelimited, pipeDelimited, deepObject, form+explode:false, simple+explode:true) fail import with a feature-naming SchemaParse; wire-equivalent defaults (form, simple) import unchanged — no more silent "[1,2]" array mis-serialization - servers overrides rejected at import at all three levels (document, path, operation) — the adapter pins one base_url at assembly time - trace-only paths: skip is now logged (warn naming path + methods), documented-as-inert instead of silent - detect_op_type + build_output_schema sweep 2XX/default keys for text/event-stream — a default-declared SSE stream classifies as Sub instead of returning one giant text body Tests: 11 new (error-drop, style rejections + default accept, servers 3-level rejections + baseline, trace skip, SSE default/2XX detection). Verified: cargo test (299), --all-features (370 + suites), clippy --all-targets -D warnings (default + all-features), fmt --check. Tasks: review-001-openapi-loud-degradation
129 lines
6.6 KiB
Markdown
129 lines
6.6 KiB
Markdown
---
|
|
id: review-001-openapi-loud-degradation
|
|
name: Loud unsupported-OpenAPI-feature handling (OAI-06)
|
|
status: completed
|
|
depends_on: []
|
|
scope: narrow
|
|
risk: low
|
|
impact: component
|
|
level: implementation
|
|
tags: [adapters, review-001, from-openapi, follow-up]
|
|
---
|
|
|
|
## Description
|
|
|
|
Review 001 finding OAI-06 — deliberately deferred during decomposition
|
|
("revisit after the input-schema work"): unsupported OpenAPI features
|
|
still degrade **silently**, each producing an op that misbehaves only at
|
|
call time (verified still true post-remediation):
|
|
|
|
- `default`/wildcard response keys become
|
|
`ErrorDefinition { code: "HTTP_0", http_status: None }`
|
|
(`from_openapi.rs:build_error_schemas`, ~:238-243) — entries that never
|
|
match a real status.
|
|
- `trace` ops silently skipped (`openapi_spec.rs`); `servers` overrides
|
|
ignored; parameter `style`/`explode` unsupported and silent (arrays
|
|
serialize `"[1,2]"`); a `default`-declared SSE stream is missed by
|
|
`detect_op_type` and would return one giant text string.
|
|
|
|
Since OAI-03's fix, the codebase already has the right pattern —
|
|
`in: cookie` fails import with a clear `SchemaParse` naming the feature
|
|
and the remediation. Extend that posture: unsupported features that
|
|
would produce a wrong-behaving op **fail import loudly** (or degrade in
|
|
a way that is documented, tested, and visibly warned), never silently.
|
|
|
|
Per-feature decision, implementer's judgment with the review's map:
|
|
reject at import (cookie-style, preferred for anything that changes
|
|
wire semantics: `style`/`explode` non-default forms, `servers` override
|
|
on a non-matching base), or support (HTTP_0's real fix is mapping
|
|
`default` → a documented catch-all code or dropping it with a warning
|
|
— pick one and test it), or document-as-inert (`trace` skip is
|
|
defensible if logged).
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [x] A spec using each unsupported feature either imports with a documented, warned, tested behavior or fails import with a feature-naming error (tests per feature)
|
|
- [x] `HTTP_0` no longer emitted (default responses mapped or dropped loudly) — `/search` never advertises a code that can't match
|
|
- [x] `style`/`explode` non-default forms do not silently mis-serialize arrays
|
|
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
|
|
|
## References
|
|
|
|
- docs/reviews/001-initial-implementation-review.md (Part E, OAI-06)
|
|
- tasks/adapters/review-001-input-schema-enforcement.md (the loud-unsupported pattern to extend)
|
|
|
|
## Notes
|
|
|
|
Per-feature decisions (the review's map, applied):
|
|
|
|
- **`default`/wildcard responses → dropped loudly (warn, not
|
|
reject)**: a catch-all code was considered and rejected — the runtime
|
|
mapper (`forward.rs::error_envelope`) already synthesizes
|
|
`HTTP_<actual>` for unmapped statuses, so a `default` ErrorDefinition
|
|
could never match anything: keeping it would only let `/search`
|
|
advertise a dead `HTTP_0` code. `build_error_schemas` now skips any
|
|
non-numeric response key at `tracing::warn` level, naming the
|
|
operation, namespace, and response key, with the remediation in the
|
|
message (declare explicit statuses). Wildcards (`5XX`, `4XX`) fall out
|
|
of the same `parse::<u16>()` check — they were the same dead entry
|
|
class.
|
|
- **`style`/`explode` non-default forms → reject at import**
|
|
(cookie-style). New `check_parameter_style` in `openapi_spec.rs`;
|
|
`parse_operation` now returns `Result<Option<Operation>,
|
|
ParameterStyleError>` so the caller converts the refusal into a
|
|
`SchemaParse` naming the parameter, the method+path, the offending
|
|
declaration, and the remediation. Wire-equivalent forms accepted
|
|
silently: `form` (query/path default, explode=true) and `simple`
|
|
(header/path default, explode=false) — a spec authoring these
|
|
explicitly gets identical serialization to omitting them. Rejected
|
|
with feature-naming errors: `spaceDelimited`, `pipeDelimited`,
|
|
`deepObject`, `matrix`, `label` (the generic arm), plus
|
|
`form+explode:false` (comma-glue, the `?a=1,2` mis-serialization the
|
|
review flagged) and `simple+explode:true`.
|
|
- **`servers` overrides → reject at import** at **all three levels**
|
|
(document root, per-path, per-operation) in one sweep in
|
|
`from_value`. The adapter pins one `base_url` at assembly time and
|
|
cannot honor per-location servers; the error names every offending
|
|
location and the remediation (remove the entries, or one import per
|
|
base URL). This check lives in `openapi_spec.rs` (parse time), so
|
|
both `from_openapi` and any future raw-doc consumer inherit it.
|
|
- **`trace` → documented-as-inert, now logged.** `HTTP_METHODS` remains
|
|
without `trace` (scope guard: no new feature support), but a path
|
|
entry carrying only unsupported methods was previously dropped
|
|
without a trace; `from_value` now emits a `tracing::warn` naming the
|
|
path and the skipped methods. Tested as "skips without erroring, the
|
|
rest of the doc imports" (the log line itself is the visibility
|
|
mechanism, consistent with the module's other warns).
|
|
- **`default`/non-200 2XX-declared SSE → supported (small superset).**
|
|
`detect_op_type` and `build_output_schema` sweep
|
|
`200..206, 226, default` for `text/event-stream` instead of only
|
|
`200`/`201` — a `default`-declared stream now classifies as `Sub`
|
|
with the SSE output schema instead of degrading to a giant single
|
|
text body. This was the one place where "reject" would have been
|
|
user-hostile: declaring streams under `default` is a real-world
|
|
pattern, and detection is a two-line change.
|
|
|
|
## Summary
|
|
|
|
- `src/adapters/from_openapi.rs`: `build_error_schemas` drops
|
|
`default`/wildcard keys loudly (never emits `HTTP_0`); `detect_op_type`
|
|
+ `build_output_schema` sweep 2XX/`default` for `text/event-stream`.
|
|
- `src/adapters/openapi_spec.rs`: document/path/operation-level
|
|
`servers` rejection; `check_parameter_style` (`ParameterStyleError`)
|
|
rejecting non-default `style`/`explode` with feature-naming errors
|
|
while accepting the wire-equivalent defaults; unsupported-method-only
|
|
paths logged at warn.
|
|
- Tests (8 new): `default_response_key_is_dropped_not_advertised_as_http_0`,
|
|
`default_declared_sse_stream_classifies_as_subscription`,
|
|
`non_default_2xx_sse_stream_classifies_as_subscription`,
|
|
`non_default_style_parameter_fails_import_naming_the_feature`,
|
|
`deep_object_style_is_rejected_like_the_other_non_default_forms`,
|
|
`form_style_with_explode_false_is_rejected`,
|
|
`default_style_and_explode_forms_still_import`,
|
|
`servers_override_at_document_level_fails_import`,
|
|
`servers_override_at_path_and_operation_level_fails_import`,
|
|
`servers_absent_baseline_still_imports`,
|
|
`trace_only_path_is_skipped_and_documented_inert`.
|
|
- Verified: `cargo test` (299), `--all-features` (370 + suites),
|
|
`clippy --all-targets -- -D warnings` (default + all-features),
|
|
`fmt --check`. |