From 9c8d898d59eb23b268301c598a321f9e2f4429b1 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 25 Nov 2024 12:32:10 -0800 Subject: [PATCH] add tests and test utils for custom errors --- dropshot/src/test_util.rs | 220 ++++++++++++- .../tests/integration-tests/custom_errors.rs | 304 ++++++++++++++++++ dropshot/tests/integration-tests/main.rs | 1 + dropshot/tests/integration-tests/openapi.rs | 16 + .../test_openapi_custom_error_types.json | 263 +++++++++++++++ 5 files changed, 801 insertions(+), 3 deletions(-) create mode 100644 dropshot/tests/integration-tests/custom_errors.rs create mode 100644 dropshot/tests/test_openapi_custom_error_types.json diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index ce6b23d3a..84840fcf9 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -20,6 +20,7 @@ use std::convert::TryFrom; use std::fmt::Debug; use std::fs; use std::iter::Iterator; +use std::marker::PhantomData; use std::net::SocketAddr; use std::path::Path; use std::sync::atomic::AtomicU32; @@ -105,6 +106,25 @@ impl ClientTestContext { .expect("attempted to construct invalid URI") } + /// Temporarily configures the client to expect `E`-typed error responses, + /// rather than [`dropshot::HttpError`] error responses. + /// + /// `ClientTestContext` expects that all error responses are + /// `dropshot::HttpError`. For testing APIs that return other error types, this + /// method borrows the `ClientTestContext` and returns a + /// [`TypedErrorClientTestContext`]``, which expects `E`-typed error + /// responses from the API server, instead. + /// + /// Because the `TypedClientErrorTestContext` is a borrowed wrapper around + /// the underlying `ClientTestContext`, the same client may be used to + /// expect multiple error types from different endpoints of the same API. + pub fn with_error_type(&self) -> TypedErrorClientTestContext<'_, E> + where + E: DeserializeOwned + std::fmt::Debug, + { + TypedErrorClientTestContext { client: self, _error: PhantomData } + } + /// Execute an HTTP request against the test server and perform basic /// validation of the result, including: /// @@ -236,6 +256,29 @@ impl ClientTestContext { request: Request, expected_status: StatusCode, ) -> Result, HttpErrorResponseBody> { + self.make_request_inner::( + request, + expected_status, + ) + .await + .map_err(|(request_id_header, error_body)| { + assert_eq!(error_body.request_id, request_id_header); + error_body + }) + } + + /// Internal implementation detail of `make_request_with_request` and + /// `make_request_with_error` that's generic over the error type, and + /// returns both the parsed error and the request ID header in the error + /// case. + async fn make_request_inner( + &self, + request: Request, + expected_status: StatusCode, + ) -> Result, (String, E)> + where + E: DeserializeOwned + std::fmt::Debug, + { let time_before = chrono::offset::Utc::now().timestamp(); info!(self.client_log, "client request"; "method" => %request.method(), @@ -334,10 +377,181 @@ impl ClientTestContext { // We got an error. Parse the response body to make sure it's valid and // then return that. - let error_body: HttpErrorResponseBody = read_json(&mut response).await; + let error_body: E = read_json(&mut response).await; info!(self.client_log, "client error"; "error_body" => ?error_body); - assert_eq!(error_body.request_id, request_id_header); - Err(error_body) + Err((request_id_header, error_body)) + } +} + +/// A `ClientTestContext` wrapper which expects that the API server under test +/// will return `E`-typed error responses. +/// +/// `ClientTestContext` expects that all error responses are +/// `dropshot::HttpError`. For testing APIs that return other error types, the +/// [`ClientTestContext::with_error_type`]`` method allows constructing a +/// `TypedErrorClientTestContext`, which expects `E`-typed error responses from +/// the API server, instead. +/// +/// In order to make API requests with a `TypedErrorTestContext`, `E` must +/// implement [`DeserializeOwned`] and [`std::fmt::Debug`]. +pub struct TypedErrorClientTestContext<'client, E> { + pub client: &'client ClientTestContext, + _error: PhantomData, +} + +impl Clone for TypedErrorClientTestContext<'_, E> { + fn clone(&self) -> Self { + Self { client: self.client, _error: PhantomData } + } +} + +impl TypedErrorClientTestContext<'_, E> +where + E: DeserializeOwned + std::fmt::Debug, +{ + /// Given the path for an API endpoint (e.g., "/projects"), return a Uri that + /// we can use to invoke this endpoint from the client. This essentially + /// appends the path to a base URL constructed from the server's IP address + /// and port. + pub fn url(&self, path: &str) -> Uri { + self.client.url(path) + } + + /// Execute an HTTP request against the test server and perform basic + /// validation of the result, including: + /// + /// - the expected status code + /// - the expected Date header (within reason) + /// - for error responses: the expected body content + /// - header names are in allowed list + /// - any other semantics that can be verified in general + /// + /// The body will be JSON encoded. + pub async fn make_request( + &self, + method: Method, + path: &str, + request_body: Option, + expected_status: StatusCode, + ) -> Result, E> { + let body = match request_body { + None => Body::empty(), + Some(input) => serde_json::to_string(&input).unwrap().into(), + }; + + self.make_request_with_body(method, path, body, expected_status).await + } + + /// Execute an HTTP request against the test server and perform basic + /// validation of the result like [`ClientTestContext::make_request`], but + /// with a content type of "application/x-www-form-urlencoded". + pub async fn make_request_url_encoded< + RequestBodyType: Serialize + Debug, + >( + &self, + method: Method, + path: &str, + request_body: Option, + expected_status: StatusCode, + ) -> Result, E> { + let body: Body = match request_body { + None => Body::empty(), + Some(input) => serde_urlencoded::to_string(&input).unwrap().into(), + }; + + self.make_request_with_body_url_encoded( + method, + path, + body, + expected_status, + ) + .await + } + + pub async fn make_request_no_body( + &self, + method: Method, + path: &str, + expected_status: StatusCode, + ) -> Result, E> { + self.make_request_with_body( + method, + path, + Body::empty(), + expected_status, + ) + .await + } + + /// Fetches a resource for which we expect to get an error response. + pub async fn make_request_error( + &self, + method: Method, + path: &str, + expected_status: StatusCode, + ) -> E { + self.make_request_with_body(method, path, "".into(), expected_status) + .await + .unwrap_err() + } + + /// Fetches a resource for which we expect to get an error response. + /// TODO-cleanup the make_request_error* interfaces are slightly different + /// than the non-error ones (and probably a bit more ergonomic). + pub async fn make_request_error_body( + &self, + method: Method, + path: &str, + body: T, + expected_status: StatusCode, + ) -> E { + self.make_request(method, path, Some(body), expected_status) + .await + .unwrap_err() + } + + pub async fn make_request_with_body( + &self, + method: Method, + path: &str, + body: Body, + expected_status: StatusCode, + ) -> Result, E> { + let uri = self.url(path); + let request = Request::builder() + .method(method) + .uri(uri) + .body(body) + .expect("attempted to construct invalid request"); + self.make_request_with_request(request, expected_status).await + } + + pub async fn make_request_with_body_url_encoded( + &self, + method: Method, + path: &str, + body: Body, + expected_status: StatusCode, + ) -> Result, E> { + let uri = self.url(path); + let request = Request::builder() + .method(method) + .header(http::header::CONTENT_TYPE, CONTENT_TYPE_URL_ENCODED) + .uri(uri) + .body(body) + .expect("attempted to construct invalid request"); + self.make_request_with_request(request, expected_status).await + } + + pub async fn make_request_with_request( + &self, + request: Request, + expected_status: StatusCode, + ) -> Result, E> { + self.client + .make_request_inner::(request, expected_status) + .await + .map_err(|(_, error_body)| error_body) } } diff --git a/dropshot/tests/integration-tests/custom_errors.rs b/dropshot/tests/integration-tests/custom_errors.rs new file mode 100644 index 000000000..220e1ad5d --- /dev/null +++ b/dropshot/tests/integration-tests/custom_errors.rs @@ -0,0 +1,304 @@ +// Copyright 2024 Oxide Computer Company + +//! Test cases for user-defined error types. + +use dropshot::endpoint; +use dropshot::ApiDescription; +use dropshot::ErrorStatusCode; +use dropshot::HttpError; +use dropshot::HttpResponseError; +use dropshot::HttpResponseOk; +use dropshot::Path; +use dropshot::RequestContext; +use http::Method; +use http::StatusCode; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; + +use crate::common; +// Define an enum error type. +#[derive(Debug, thiserror::Error, Serialize, JsonSchema)] +enum EnumError { + // A user-defined custom error variant. This one is one of the classic + // confusing TeX error messages. + #[error("overfull \\hbox (badness {badness}) at line {line}")] + OverfullHbox { badness: i32, line: u32 }, + // Variant constructed from Dropshot `HttpError`s. + #[error("{internal_message}")] + HttpError { + message: String, + error_code: Option, + #[serde(skip)] + internal_message: String, + #[serde(skip)] + status: ErrorStatusCode, + }, +} + +impl From for EnumError { + fn from(e: HttpError) -> Self { + EnumError::HttpError { + message: e.external_message, + error_code: e.error_code, + internal_message: e.internal_message, + status: e.status_code, + } + } +} + +impl HttpResponseError for EnumError { + fn status_code(&self) -> ErrorStatusCode { + match self { + EnumError::OverfullHbox { .. } => { + ErrorStatusCode::INTERNAL_SERVER_ERROR + } + EnumError::HttpError { status, .. } => *status, + } + } +} + +/// This is the same as `EnumError`, but with the non-serialized fields removed. +/// This should match what the client receives. +#[derive(Debug, Deserialize, Eq, PartialEq)] +enum DeserializedEnumError { + OverfullHbox { badness: i32, line: u32 }, + HttpError { message: String, error_code: Option }, +} + +// Also, define a struct error type wrapping an enum. +#[derive(Debug, thiserror::Error, Serialize, JsonSchema)] +#[error("{message}")] +struct StructError { + message: String, + kind: ErrorKind, + #[serde(skip)] + status: ErrorStatusCode, +} + +#[derive(Debug, Deserialize, Serialize, JsonSchema, Eq, PartialEq)] +enum ErrorKind { + /// Can't get ye flask. + CantGetYeFlask, + /// Flagrant error, + Other, +} + +/// This is the same as `StructError`, but with the non-serialized fields removed. +/// This should match what the client receives. +#[derive(Debug, Deserialize, Eq, PartialEq)] +struct DeserializedStructError { + message: String, + kind: ErrorKind, +} + +impl From for StructError { + fn from(e: HttpError) -> Self { + Self { + message: e.external_message, + kind: ErrorKind::Other, + status: e.status_code, + } + } +} + +impl HttpResponseError for StructError { + fn status_code(&self) -> ErrorStatusCode { + self.status + } +} + +// The test endpoints take a boolean parameter that determines whether to return +// an error or a success. This is used primarily so that we can send a request +// with a path param that *doesn't* parse as a boolean, in order to trigger an +// extractor error and exercise the conversion of dropshot `HttpError`s into the +// user error type. +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +struct HandlerPathParam { + should_error: bool, +} + +#[endpoint { + method = GET, + path = "/test/enum-error/{should_error}", +}] +async fn enum_error_handler( + _rqctx: RequestContext<()>, + path: Path, +) -> Result, EnumError> { + let HandlerPathParam { should_error } = path.into_inner(); + if should_error { + Err(EnumError::OverfullHbox { badness: 10000, line: 42 }) + } else { + Ok(HttpResponseOk(())) + } +} + +#[endpoint { + method = GET, + path = "/test/struct-error/{should_error}", +}] +async fn struct_error_handler( + _rqctx: RequestContext<()>, + path: Path, +) -> Result, StructError> { + let HandlerPathParam { should_error } = path.into_inner(); + if should_error { + Err(StructError { + kind: ErrorKind::CantGetYeFlask, + message: "can't get ye flask".to_string(), + status: ErrorStatusCode::NOT_FOUND, + }) + } else { + Ok(HttpResponseOk(())) + } +} + +// A handler that returns a `dropshot::HttpError`. This is used by the OpenAPI +// tests for custom errors in order to ensure that handlers returning +// `HttpError` can coexist with those that return user-defined types. +#[endpoint { + method = GET, + path = "/test/dropshot-error/", +}] +async fn dropshot_error_handler( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Err(HttpError::for_internal_error("something bad happened".to_string())) +} + +pub(crate) fn api() -> ApiDescription<()> { + let mut api = ApiDescription::new(); + api.register(enum_error_handler).unwrap(); + api.register(struct_error_handler).unwrap(); + + api.register(dropshot_error_handler).unwrap(); + api +} + +// Test case: the enum error handler returns the user-defiend error variant. +#[tokio::test] +async fn test_enum_user_error() { + let api = api(); + let testctx = common::test_setup_with_context( + "test_enum_user_error", + api, + (), + dropshot::HandlerTaskMode::Detached, + ); + let json = testctx + .client_testctx + .with_error_type::() + .make_request_error( + Method::GET, + "/test/enum-error/true", + StatusCode::INTERNAL_SERVER_ERROR, + ) + .await; + assert_eq!( + dbg!(json), + DeserializedEnumError::OverfullHbox { badness: 10000, line: 42 } + ); + + testctx.teardown().await; +} + +// Test case: the enum error handler converts a Dropshot `HttpError` from an +// extractor into its user-defined error type. +#[tokio::test] +async fn test_enum_extractor_error() { + let api = api(); + let testctx = common::test_setup_with_context( + "test_enum_extractor_error", + api, + (), + dropshot::HandlerTaskMode::Detached, + ); + // This time, instead of /test/enum-error/true, let's send something that + // doesn't parse as a bool, so that the path param extractor returns an error. + let json = testctx + .client_testctx + .with_error_type::() + .make_request_error( + Method::GET, + "/test/enum-error/asdf", + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!( + dbg!(json), + DeserializedEnumError::HttpError { + message: + "bad parameter in URL path: unable to parse 'asdf' as bool" + .to_string(), + error_code: None + }, + ); + + testctx.teardown().await; +} + +// Test case: the struct error handler returns the user-defiend error variant. +#[tokio::test] +async fn test_struct_user_error() { + let api = api(); + let testctx = common::test_setup_with_context( + "test_struct_user_error", + api, + (), + dropshot::HandlerTaskMode::Detached, + ); + let json = testctx + .client_testctx + .with_error_type::() + .make_request_error( + Method::GET, + "/test/struct-error/true", + StatusCode::NOT_FOUND, + ) + .await; + assert_eq!( + dbg!(json), + DeserializedStructError { + message: "can't get ye flask".to_string(), + kind: ErrorKind::CantGetYeFlask, + } + ); + + testctx.teardown().await; +} + +// Test case: the struct error handler converts a Dropshot `HttpError` from an +// extractor into its user-defined error type. +#[tokio::test] +async fn test_struct_extractor_error() { + let api = api(); + let testctx = common::test_setup_with_context( + "test_struct_extractor_error", + api, + (), + dropshot::HandlerTaskMode::Detached, + ); + // This time, instead of /test/struct-error/true, let's send something that + // doesn't parse as a bool, so that the path param extractor returns an error. + let json = testctx + .client_testctx + .with_error_type::() + .make_request_error( + Method::GET, + "/test/struct-error/this-wont-work", + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!( + dbg!(json), + DeserializedStructError { + message: + "bad parameter in URL path: unable to parse 'this-wont-work' as bool" + .to_string(), + kind: ErrorKind::Other, + }, + ); + + testctx.teardown().await; +} diff --git a/dropshot/tests/integration-tests/main.rs b/dropshot/tests/integration-tests/main.rs index cd28c6818..ec49604c1 100644 --- a/dropshot/tests/integration-tests/main.rs +++ b/dropshot/tests/integration-tests/main.rs @@ -13,6 +13,7 @@ extern crate lazy_static; mod api_trait; mod common; mod config; +mod custom_errors; mod demo; mod detached_shutdown; mod multipart; diff --git a/dropshot/tests/integration-tests/openapi.rs b/dropshot/tests/integration-tests/openapi.rs index 9f2c3cc98..989b66bf8 100644 --- a/dropshot/tests/integration-tests/openapi.rs +++ b/dropshot/tests/integration-tests/openapi.rs @@ -582,3 +582,19 @@ fn test_openapi_fuller() -> anyhow::Result<()> { expectorate::assert_contents("tests/test_openapi_fuller.json", actual); Ok(()) } + +#[test] +fn test_openapi_custom_error_types() -> anyhow::Result<()> { + let api = super::custom_errors::api(); + let mut output = Cursor::new(Vec::new()); + + let _ = + api.openapi("test", semver::Version::new(3, 5, 0)).write(&mut output); + let actual = from_utf8(output.get_ref()).unwrap(); + + expectorate::assert_contents( + "tests/test_openapi_custom_error_types.json", + actual, + ); + Ok(()) +} diff --git a/dropshot/tests/test_openapi_custom_error_types.json b/dropshot/tests/test_openapi_custom_error_types.json new file mode 100644 index 000000000..02374ae3b --- /dev/null +++ b/dropshot/tests/test_openapi_custom_error_types.json @@ -0,0 +1,263 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "test", + "version": "3.5.0" + }, + "paths": { + "/test/dropshot-error": { + "get": { + "operationId": "dropshot_error_handler", + "responses": { + "4XX": { + "description": "client error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "5XX": { + "description": "server error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } + } + } + } + }, + "/test/enum-error/{should_error}": { + "get": { + "operationId": "enum_error_handler", + "parameters": [ + { + "in": "path", + "name": "should_error", + "required": true, + "schema": { + "type": "boolean" + } + } + ], + "responses": { + "4XX": { + "description": "client error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/EnumError" + } + } + } + }, + "5XX": { + "description": "server error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/EnumError" + } + } + } + }, + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } + } + } + } + }, + "/test/struct-error/{should_error}": { + "get": { + "operationId": "struct_error_handler", + "parameters": [ + { + "in": "path", + "name": "should_error", + "required": true, + "schema": { + "type": "boolean" + } + } + ], + "responses": { + "4XX": { + "description": "client error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/StructError" + } + } + } + }, + "5XX": { + "description": "server error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/StructError" + } + } + } + }, + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "EnumError": { + "oneOf": [ + { + "type": "object", + "properties": { + "OverfullHbox": { + "type": "object", + "properties": { + "badness": { + "type": "integer", + "format": "int32" + }, + "line": { + "type": "integer", + "format": "uint32", + "minimum": 0 + } + }, + "required": [ + "badness", + "line" + ] + } + }, + "required": [ + "OverfullHbox" + ], + "additionalProperties": false + }, + { + "type": "object", + "properties": { + "HttpError": { + "type": "object", + "properties": { + "error_code": { + "nullable": true, + "type": "string" + }, + "message": { + "type": "string" + } + }, + "required": [ + "message" + ] + } + }, + "required": [ + "HttpError" + ], + "additionalProperties": false + } + ] + }, + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + }, + "ErrorKind": { + "oneOf": [ + { + "description": "Can't get ye flask.", + "type": "string", + "enum": [ + "CantGetYeFlask" + ] + }, + { + "description": "Flagrant error,", + "type": "string", + "enum": [ + "Other" + ] + } + ] + }, + "StructError": { + "type": "object", + "properties": { + "kind": { + "$ref": "#/components/schemas/ErrorKind" + }, + "message": { + "type": "string" + } + }, + "required": [ + "kind", + "message" + ] + } + } + } +}