Skip to content

Commit

Permalink
Improve deserialization errors for the JsonBody extractor. (#67)
Browse files Browse the repository at this point in the history
Using `serde_path_to_error` will give us better error messages.

I've added a snapshot test to check the error out, as well as fixing a
broken doc test that slipped through the cracks.
  • Loading branch information
LukeMathWalker committed Jun 3, 2023
1 parent 198e2fd commit 51ddb19
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 8 deletions.
10 changes: 10 additions & 0 deletions libs/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions libs/pavex_runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ serde_html_form = "0.1"

# Json body extractor
serde_json = "1"
serde_path_to_error = "0.1"

[dev-dependencies]
serde = { version = "1", features = ["derive"] }
Expand Down
2 changes: 1 addition & 1 deletion libs/pavex_runtime/src/extract/body/buffered_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ use hyper::body::to_bytes;
/// You can leverage nesting for this purpose:
///
/// ```rust
/// use pavex_builder::{f, Blueprint, constructor::Lifecycle};
/// use pavex_builder::{f, Blueprint, constructor::Lifecycle, router::{GET, POST}};
/// use pavex_runtime::extract::body::BodySizeLimit;
/// # pub fn home() -> String { todo!() }
/// # pub fn upload() -> String { todo!() }
Expand Down
17 changes: 13 additions & 4 deletions libs/pavex_runtime/src/extract/body/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ pub enum ExtractJsonBodyError {
/// See [`JsonContentTypeMismatch`] for details.
ContentTypeMismatch(#[from] JsonContentTypeMismatch),
#[error(transparent)]
// TODO: should we use an opaque `JsonDeserializationError` instead?
DeserializationError(#[from] serde_json::Error),
/// See [`JsonDeserializationError`] for details.
DeserializationError(#[from] JsonDeserializationError),
}

impl ExtractJsonBodyError {
Expand All @@ -28,9 +28,9 @@ impl ExtractJsonBodyError {
.status(StatusCode::UNSUPPORTED_MEDIA_TYPE)
.body(format!("{}", self))
.unwrap(),
ExtractJsonBodyError::DeserializationError(e) => Response::builder()
ExtractJsonBodyError::DeserializationError(_) => Response::builder()
.status(StatusCode::BAD_REQUEST)
.body(format!("Invalid JSON.\n{}", e))
.body(format!("{}", self))
.unwrap(),
}
}
Expand Down Expand Up @@ -100,6 +100,15 @@ pub struct UnexpectedBufferError {
/// another `application/*+json` MIME type.
pub struct MissingJsonContentType;

#[derive(Debug, thiserror::Error)]
#[error("Failed to deserialize the body as a JSON document.\n{source}")]
#[non_exhaustive]
/// Something went wrong when deserializing the request body into the specified type.
pub struct JsonDeserializationError {
#[source]
pub(super) source: serde_path_to_error::Error<serde_json::Error>,
}

#[derive(Debug, thiserror::Error)]
#[error(
"The `Content-Type` header was set to `{actual}`. This endpoint expects requests with a `Content-Type` header set to `application/json`, or another `application/*+json` MIME type"
Expand Down
69 changes: 66 additions & 3 deletions libs/pavex_runtime/src/extract/body/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ use crate::request::RequestHead;

use super::{
buffered_body::BufferedBody,
errors::{ExtractJsonBodyError, JsonContentTypeMismatch, MissingJsonContentType},
errors::{
ExtractJsonBodyError, JsonContentTypeMismatch, JsonDeserializationError,
MissingJsonContentType,
},
};

#[doc(alias = "Json")]
#[derive(Debug)]
/// Parse the body of an incoming request as JSON.
///
/// # Sections
Expand Down Expand Up @@ -130,8 +134,9 @@ impl<T> JsonBody<T> {
T: Deserialize<'body>,
{
check_json_content_type(&request_head.headers)?;
// TODO: improve error message if deserialization fails here.
let body = serde_json::from_slice(&buffered_body.bytes)?;
let mut deserializer = serde_json::Deserializer::from_slice(buffered_body.bytes.as_ref());
let body = serde_path_to_error::deserialize(&mut deserializer)
.map_err(|e| JsonDeserializationError { source: e })?;
Ok(JsonBody(body))
}
}
Expand Down Expand Up @@ -167,6 +172,8 @@ fn check_json_content_type(headers: &HeaderMap) -> Result<(), ExtractJsonBodyErr

#[cfg(test)]
mod tests {
use crate::extract::body::JsonBody;

#[test]
fn missing_content_type() {
let headers = http::HeaderMap::new();
Expand Down Expand Up @@ -249,4 +256,60 @@ mod tests {
let outcome = super::check_json_content_type(&headers);
assert!(outcome.is_ok());
}

#[test]
/// Let's check the error quality when the request body is missing
/// a required field.
fn missing_json_field() {
// Arrange
#[derive(serde::Deserialize, Debug)]
#[allow(dead_code)]
struct BodySchema {
name: String,
surname: String,
age: u8,
}

let mut headers = http::HeaderMap::new();
headers.insert(
http::header::CONTENT_TYPE,
"application/json; charset=utf-8".parse().unwrap(),
);
let request_head = crate::request::RequestHead {
headers,
method: http::Method::GET,
uri: "/".parse().unwrap(),
version: http::Version::HTTP_11,
};
let body = serde_json::json!({
"name": "John Doe",
"age": 43,
});

// Act
let buffered_body = crate::extract::body::BufferedBody {
bytes: serde_json::to_vec(&body).unwrap().into(),
};
let outcome: Result<JsonBody<BodySchema>, _> =
JsonBody::extract(&request_head, &buffered_body);

// Assert
let err = outcome.unwrap_err();
insta::assert_display_snapshot!(err, @r###"
Failed to deserialize the body as a JSON document.
missing field `surname` at line 1 column 28
"###);
insta::assert_debug_snapshot!(err, @r###"
DeserializationError(
JsonDeserializationError {
source: Error {
path: Path {
segments: [],
},
original: Error("missing field `surname`", line: 1, column: 28),
},
},
)
"###);
}
}

0 comments on commit 51ddb19

Please sign in to comment.