as encountered in oxidecomputer/oxide.rs#1352
if a request is answered with 4xx/5xx errors by levels of the stack below what's modeled in the channel handler, we should at least show clients the content of the error message, rather than just the response's headers through the unexpected response error (with only the error message's content length).
the /v1/instances/{}/serial-console/stream object contains this:
"responses": {
"default": {
"description": "",
"content": {
"*/*": {
"schema": {}
}
}
}
},contrast with /v1/instances/{}/serial-console (GET request, rather than websocket upgrade), which describes expecting 4xx and 5xx codes as well as 200 OK:
"responses": {
"200": {
"description": "successful operation",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/InstanceSerialConsoleData"
}
}
}
},
"4XX": {
"$ref": "#/components/responses/Error"
},
"5XX": {
"$ref": "#/components/responses/Error"
}
}the resulting progenitor output in generated_sdk.rs becomes
match response.status().as_u16() {
101u16 => ResponseValue::upgrade(response).await,
200..=299 => ResponseValue::upgrade(response).await,
_ => Err(Error::UnexpectedResponse(response)),
}(the 200..=299 there may be an additional minor bug in progenitor? putting the check for if dropshot_websocket before the if !success, and setting success to true in the former. https://github.com/oxidecomputer/progenitor/blob/b85633b980c25747a5b1ebbc8b5cb10317abdb5e/progenitor-impl/src/method.rs#L507-L526 however, the existing openapi doc does define a "default", which should be matched in the iterator above these conditionals, so i may be off base)
should we construct the response type for websocket channels in dropshot, in such a fashion as to provide response types for 101, 4xx and 5xx, rather than only "default"?
notes of my investigation into the in-between parts:
for there to be no explicit 4xx/5xx in the openapi schema, my current assumption is that either the endpoint.response.success or the endpoint.error for serial-console/stream must have been None at this point:
|
if let Some(code) = &endpoint.response.success { |
|
// `Ok` response has a known status code. In this case, |
|
// emit it as the response for that status code only. |
|
operation.responses.responses.insert( |
|
openapiv3::StatusCode::Code(code.as_u16()), |
|
openapiv3::ReferenceOr::Item(response), |
|
); |
|
|
|
// If the endpoint defines an error type, emit that for the 4xx |
|
// and 5xx responses. |
|
if let Some(ApiEndpointErrorResponse { |
|
ref schema, |
|
type_name, |
|
}) = endpoint.error |
followed through the proc macros this far, thus far:
pub type WebsocketEndpointResult = Result<Response<Body>, HttpError>;
-> which is ChannelParams.endpoint_result_ty
|
let endpoint_result_ty = |
|
parse_quote! { #dropshot::WebsocketEndpointResult }; |
->
endpoint_result_ty is
adapter_name's return type
-> in
ApiEndpoint::new,
ApiEndpointErrorResponse::for_type for
<HandlerType as HttpResponseFunc>::Error, whose
::Error is defined by
impl_HttpHandlerFunc_for_func_with_params! to be the
ErrorType of the future's returned
Result
|
let error = ApiEndpointErrorResponse::for_type::<HandlerType::Error>(); |
-> in
ApiEndpoint::new_for_types<FuncParams, ResultType>, the
WebsocketEndpointResult is passed explicitly as
ResultType. still calls
ApiEndpointErrorResponse::for_type on it, and its
<WebsocketEndpointResult as HttpResultType>::Error is
HttpError.
|
let error = ApiEndpointErrorResponse::for_type::<ResultType::Error>(); |
->
ApiEndpointErrorResponse::for_type's only path to returning
None is
<T as HttpResponseContent>::content_metadata()?
|
impl ApiEndpointErrorResponse { |
|
fn for_type<T: HttpResponseError>() -> Option<Self> { |
|
let schema = T::content_metadata()?; |
|
Some(Self { schema, type_name: std::any::type_name::<T>() }) |
|
} |
|
} |
-> for which
HttpError unconditionally returns
Some.
and sure enough, changing the type of WebsocketEndpointResult to be something whose ::Ok type impls HttpResponse with the appropriate status code, gives us an OpenAPI description with 101 and 4XX/5XX codes for the endpoint! (but! even if i provide ApiEndpointResponse.description, it is set to empty string in the schema -- unless i made ApiEndpointResponse.schema be Some, which would make us dishonestly claim our response type was application/json rather than a completely separate plane of existence without a mimetype)
as encountered in oxidecomputer/oxide.rs#1352
if a request is answered with 4xx/5xx errors by levels of the stack below what's modeled in the channel handler, we should at least show clients the content of the error message, rather than just the response's headers through the unexpected response error (with only the error message's content length).
the
/v1/instances/{}/serial-console/streamobject contains this:contrast with
/v1/instances/{}/serial-console(GET request, rather than websocket upgrade), which describes expecting 4xx and 5xx codes as well as 200 OK:the resulting progenitor output in
generated_sdk.rsbecomes(the
200..=299there may be an additional minor bug in progenitor? putting the check forif dropshot_websocketbefore theif !success, and setting success to true in the former. https://github.com/oxidecomputer/progenitor/blob/b85633b980c25747a5b1ebbc8b5cb10317abdb5e/progenitor-impl/src/method.rs#L507-L526 however, the existing openapi doc does define a "default", which should be matched in the iterator above these conditionals, so i may be off base)should we construct the response type for websocket channels in dropshot, in such a fashion as to provide response types for 101, 4xx and 5xx, rather than only "default"?
notes of my investigation into the in-between parts:
for there to be no explicit 4xx/5xx in the openapi schema, my current assumption is that either the
endpoint.response.successor theendpoint.errorfor serial-console/stream must have beenNoneat this point:dropshot/dropshot/src/api_description.rs
Lines 1008 to 1021 in 5366c3f
followed through the proc macros this far, thus far:
pub type WebsocketEndpointResult = Result<Response<Body>, HttpError>;-> which is
ChannelParams.endpoint_result_tydropshot/dropshot_endpoint/src/channel.rs
Lines 294 to 295 in 5366c3f
->
endpoint_result_tyisadapter_name's return type-> in
ApiEndpoint::new,ApiEndpointErrorResponse::for_typefor<HandlerType as HttpResponseFunc>::Error, whose::Erroris defined byimpl_HttpHandlerFunc_for_func_with_params!to be theErrorTypeof the future's returnedResultdropshot/dropshot/src/api_description.rs
Line 89 in 5366c3f
-> in
ApiEndpoint::new_for_types<FuncParams, ResultType>, theWebsocketEndpointResultis passed explicitly asResultType. still callsApiEndpointErrorResponse::for_typeon it, and its<WebsocketEndpointResult as HttpResultType>::ErrorisHttpError.dropshot/dropshot/src/api_description.rs
Line 196 in 5366c3f
->
ApiEndpointErrorResponse::for_type's only path to returningNoneis<T as HttpResponseContent>::content_metadata()?dropshot/dropshot/src/api_description.rs
Lines 382 to 387 in 5366c3f
-> for which
HttpErrorunconditionally returnsSome.and sure enough, changing the type of
WebsocketEndpointResultto be something whose::Oktype implsHttpResponsewith the appropriate status code, gives us an OpenAPI description with 101 and 4XX/5XX codes for the endpoint! (but! even if i provideApiEndpointResponse.description, it is set to empty string in the schema -- unless i madeApiEndpointResponse.schemabeSome, which would make us dishonestly claim our response type was application/json rather than a completely separate plane of existence without a mimetype)