Skip to content

Commit

Permalink
Change StatusCode to handle unknown status codes
Browse files Browse the repository at this point in the history
Tide's design of their status code type (which we copied) as an enum,
is not resilient against new status codes or status codes unknown to
the server framework (such as ELB-specific status codes). This caused
a panic in a Cappuccino node when it received a response with a status
code it did not recognize.

This changes `StatusCode` from an enum to a wrapper around `reqwest::StatusCode`,
which is itself a wrapper around a `u16`. As such, it can represent any status
code in the allowed range without necessarily knowing what a certain status
represents.

We keep most of the same interface by reimplementing all the appropriate and
conversion traits. However, there are a couple of breaking changes:
* We no longer implement `From<StatusCode> for tide::StatusCode`; instead we
  implement `TryFrom`, so we can return an error instead of panicking if it
  is a status code that `tide` cannot handle
* Since our status code is no longer an enum, we replace enum variant
  constructors with associated constants, which is the same as what `reqwest`
  does
  • Loading branch information
jbearer committed Jun 5, 2024
1 parent eeb345c commit 00ec1a4
Show file tree
Hide file tree
Showing 13 changed files with 169 additions and 537 deletions.
13 changes: 0 additions & 13 deletions Cargo.lock

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

2 changes: 0 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ lazy_static = "1.4"
libc = "0.2"
markdown = "0.3"
maud = { version = "0.26", features = ["tide"] }
num-derive = "0.4"
num-traits = "0.2"
parking_lot = "0.12"
prometheus = "0.13"
reqwest = { version = "0.12", features = ["json"] }
Expand Down
18 changes: 9 additions & 9 deletions examples/hello-world/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl tide_disco::Error for HelloError {

impl From<RequestError> for HelloError {
fn from(err: RequestError) -> Self {
Self::catch_all(StatusCode::BadRequest, err.to_string())
Self::catch_all(StatusCode::BAD_REQUEST, err.to_string())
}
}

Expand Down Expand Up @@ -116,14 +116,14 @@ mod test {
let client = Client::new(url).await;

let res = client.get("greeting/tester").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.json::<String>().await.unwrap(), "Hello, tester");

let res = client.post("greeting/Sup").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("greeting/tester").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.json::<String>().await.unwrap(), "Sup, tester");
}

Expand All @@ -138,7 +138,7 @@ mod test {

// Check the API version.
let res = client.get("hello/version").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
let api_version = ApiVersion {
api_version: Some(env!("CARGO_PKG_VERSION").parse().unwrap()),
spec_version: "0.1.0".parse().unwrap(),
Expand All @@ -147,7 +147,7 @@ mod test {

// Check the overall version.
let res = client.get("version").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(
res.json::<AppVersion>().await.unwrap(),
AppVersion {
Expand All @@ -169,7 +169,7 @@ mod test {

// Check the API health.
let res = client.get("hello/healthcheck").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
// The example API does not have a custom healthcheck, so we just get the default response.
assert_eq!(
res.json::<HealthStatus>().await.unwrap(),
Expand All @@ -178,12 +178,12 @@ mod test {

// Check the overall health.
let res = client.get("healthcheck").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(
res.json::<AppHealth>().await.unwrap(),
AppHealth {
status: HealthStatus::Available,
modules: [("hello".to_string(), [(0, StatusCode::Ok)].into())].into(),
modules: [("hello".to_string(), [(0, StatusCode::OK)].into())].into(),
}
)
}
Expand Down
6 changes: 3 additions & 3 deletions examples/versions/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ mod test {
.unwrap()
);
assert_eq!(
StatusCode::NotFound,
StatusCode::NOT_FOUND,
client.get("v1/api/added").send().await.unwrap().status()
);

Expand All @@ -95,7 +95,7 @@ mod test {
.unwrap()
);
assert_eq!(
StatusCode::NotFound,
StatusCode::NOT_FOUND,
client.get("v2/api/deleted").send().await.unwrap().status()
);

Expand All @@ -111,7 +111,7 @@ mod test {
.unwrap()
);
assert_eq!(
StatusCode::NotFound,
StatusCode::NOT_FOUND,
client.get("api/deleted").send().await.unwrap().status()
);
}
Expand Down
8 changes: 4 additions & 4 deletions src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1545,7 +1545,7 @@ mod test {
|_req, _conn: Connection<(), (), _, StaticVer01>, _state| {
async move {
Err(ServerError::catch_all(
StatusCode::InternalServerError,
StatusCode::INTERNAL_SERVER_ERROR,
"an error message".to_string(),
))
}
Expand Down Expand Up @@ -1669,7 +1669,7 @@ mod test {
// We intentionally return a stream that never terminates, to check that simply
// yielding an error causes the connection to terminate.
repeat(Err(ServerError::catch_all(
StatusCode::InternalServerError,
StatusCode::INTERNAL_SERVER_ERROR,
"an error message".to_string(),
)))
.boxed()
Expand Down Expand Up @@ -1737,7 +1737,7 @@ mod test {
let client = Client::new(url).await;

let res = client.get("/mod/healthcheck").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(
res.json::<HealthStatus>().await.unwrap(),
HealthStatus::Available
Expand Down Expand Up @@ -1793,7 +1793,7 @@ mod test {
tracing::info!("making metrics request {i}");
let expected = format!("# HELP counter count of how many times metrics have been exported\n# TYPE counter counter\ncounter {i}\n");
let res = client.get("mod/metrics").send().await.unwrap();
assert_eq!(res.status(), StatusCode::Ok);
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await.unwrap(), expected);
}
}
Expand Down
Loading

0 comments on commit 00ec1a4

Please sign in to comment.