Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http server): omit jsonrpc details in health API #785

Merged
merged 4 commits into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion http-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ jsonrpsee-core = { path = "../core", version = "0.13.1", features = ["server", "
globset = "0.4"
lazy_static = "1.4"
tracing = "0.1"
serde_json = "1"
serde_json = { version = "1.0", features = ["raw_value"] }
serde = "1"
tokio = { version = "1.16", features = ["rt-multi-thread", "macros"] }
unicase = "2.6.0"

Expand Down
30 changes: 24 additions & 6 deletions http-server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,20 @@ impl<M> Builder<M> {
}

/// Enable health endpoint.
/// Allows you to expose one of the methods under GET /<path> The method will be invoked with no parameters. Error returned from the method will be converted to status 500 response.
/// Expects a tuple with (<path>, <rpc-method-name>).
pub fn health_api(mut self, path: impl Into<String>, method: impl Into<String>) -> Self {
self.health_api = Some(HealthApi { path: path.into(), method: method.into() });
self
/// Allows you to expose one of the methods under GET /<path> The method will be invoked with no parameters.
/// Error returned from the method will be converted to status 500 response.
/// Expects a tuple with (</path>, <rpc-method-name>).
///
/// Fails if the path is missing `/`.
pub fn health_api(mut self, path: impl Into<String>, method: impl Into<String>) -> Result<Self, Error> {
let path = path.into();

if !path.starts_with("/") {
return Err(Error::Custom(format!("Health endpoint path must start with `/` to work, got: {}", path)));
}
Comment on lines +181 to +183
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering; why do we care about the path starting with '/'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have this check, if / is missing that will fail and it's quite tricky to debug.

I have already done that myself but more ideally would be Path type instead of a String

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok gotcha; that makes sense!


self.health_api = Some(HealthApi { path: path, method: method.into() });
Ok(self)
}

/// Finalizes the configuration of the server with customized TCP settings on the socket and on hyper.
Expand Down Expand Up @@ -754,7 +763,16 @@ async fn process_health_request(
middleware.on_response(request_start);

match data {
Some(resp) if success => Ok(response::ok_response(resp)),
Some(data) if success => {
#[derive(serde::Deserialize)]
struct RpcPayload<'a> {
#[serde(borrow)]
result: &'a serde_json::value::RawValue,
}

let payload: RpcPayload = serde_json::from_str(&data).expect("valid JSON-RPC response is valid JSON; qed");
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved
Ok(response::ok_response(payload.result.to_string()))
}
_ => Ok(response::internal_error()),
}
}
3 changes: 2 additions & 1 deletion tests/tests/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ pub async fn http_server_with_access_control(acl: AccessControl) -> (SocketAddr,
let server = HttpServerBuilder::default()
.set_access_control(acl)
.health_api("/health", "system_health")
.unwrap()
.build("127.0.0.1:0")
.await
.unwrap();
Expand All @@ -233,7 +234,7 @@ pub async fn http_server_with_access_control(acl: AccessControl) -> (SocketAddr,
module.register_method("say_hello", |_, _| Ok("hello")).unwrap();
module.register_method("notif", |_, _| Ok("")).unwrap();

module.register_method("system_health", |_, _| Ok("im ok")).unwrap();
module.register_method("system_health", |_, _| Ok(serde_json::json!({ "health": true }))).unwrap();

let handle = server.start(module).unwrap();
(addr, handle)
Expand Down
2 changes: 1 addition & 1 deletion tests/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -767,5 +767,5 @@ async fn http_health_api_works() {

let bytes = hyper::body::to_bytes(res.into_body()).await.unwrap();
let out = String::from_utf8(bytes.to_vec()).unwrap();
assert_eq!(out, "{\"jsonrpc\":\"2.0\",\"result\":\"im ok\",\"id\":0}");
assert_eq!(out.as_str(), "{\"health\":true}");
}