From b959d246ac72c48f3f09c5648a70fe960bf1f5c5 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 22 Jan 2025 15:26:45 -0500 Subject: [PATCH 1/6] fix: missing id and missing params --- src/pubsub/shared.rs | 9 ++++--- src/types/req.rs | 56 ++++++++++++++++++++++++++++++-------------- tests/common/mod.rs | 44 ++++++++++++++++++++++++++++++---- tests/ipc.rs | 20 +++++++--------- tests/ws.rs | 23 ++++++++---------- 5 files changed, 103 insertions(+), 49 deletions(-) diff --git a/src/pubsub/shared.rs b/src/pubsub/shared.rs index 464964d..54e97ea 100644 --- a/src/pubsub/shared.rs +++ b/src/pubsub/shared.rs @@ -202,9 +202,12 @@ where break; }; - let Ok(req) = Request::try_from(item) else { - tracing::warn!("inbound request is malformatted"); - continue + let req = match Request::try_from(item) { + Ok(req) => req, + Err(err) => { + tracing::warn!(%err, "inbound request is malformatted"); + continue + } }; let span = debug_span!("ipc request handling", id = req.id(), method = req.method()); diff --git a/src/types/req.rs b/src/types/req.rs index 6c26a43..9505d99 100644 --- a/src/types/req.rs +++ b/src/types/req.rs @@ -31,7 +31,7 @@ pub struct Request { /// /// This field is generated by deserializing to a [`RawValue`] and then /// calculating the offset of the backing slice within the `bytes` field. - id: Range, + id: Option>, /// A range of the `bytes` field that represents the method field of the /// JSON-RPC request. /// @@ -49,7 +49,7 @@ pub struct Request { /// /// This field is generated by deserializing to a [`RawValue`] and then /// calculating the offset of the backing slice within the `bytes` field. - params: Range, + params: Option>, } impl core::fmt::Debug for Request { @@ -67,11 +67,11 @@ impl core::fmt::Debug for Request { #[derive(serde::Deserialize)] struct DeserHelper<'a> { #[serde(borrow)] - id: &'a RawValue, + id: Option<&'a RawValue>, #[serde(borrow)] method: &'a RawValue, #[serde(borrow)] - params: &'a RawValue, + params: Option<&'a RawValue>, } impl TryFrom for Request { @@ -80,12 +80,19 @@ impl TryFrom for Request { fn try_from(bytes: Bytes) -> Result { let DeserHelper { id, method, params } = serde_json::from_slice(bytes.as_ref())?; - let id = find_range!(bytes, id.get()); - // Ensure the id is not too long - let id_len = id.end - id.start; - if id_len > ID_LEN_LIMIT { - return Err(RequestError::IdTooLarge(id_len)); - } + let id = if let Some(id) = id { + let id = find_range!(bytes, id.get()); + + // Ensure the id is not too long + let id_len = id.end - id.start; + if id_len > ID_LEN_LIMIT { + return Err(RequestError::IdTooLarge(id_len)); + } + + Some(id) + } else { + None + }; // Ensure method is a string, and not too long, and trim the quotes // from it @@ -101,7 +108,7 @@ impl TryFrom for Request { return Err(RequestError::MethodTooLarge(method_len)); } - let params = find_range!(bytes, params.get()); + let params = params.map(|params| find_range!(bytes, params.get())); Ok(Self { bytes, @@ -122,11 +129,20 @@ impl TryFrom for Request { } impl Request { - /// Return a reference to the serialized ID field. + /// Return a reference to the serialized ID field. If the ID field is + /// missing, this will return `"null"`, ensuring that response correctly + /// have a null ID, as per [the JSON-RPC spec]. + /// + /// [the JSON-RPC spec]: https://www.jsonrpc.org/specification#response_object pub fn id(&self) -> &str { - // SAFETY: `id` is guaranteed to be valid JSON, - // and a valid slice of `bytes`. - unsafe { core::str::from_utf8_unchecked(self.bytes.get_unchecked(self.id.clone())) } + self.id + .as_ref() + .map(|range| { + // SAFETY: `range` is guaranteed to be valid JSON, and a valid + // slice of `bytes`. + unsafe { core::str::from_utf8_unchecked(self.bytes.get_unchecked(range.clone())) } + }) + .unwrap_or("null") } /// Return an owned version of the serialized ID field. @@ -161,9 +177,13 @@ impl Request { /// Return a reference to the serialized params field. pub fn params(&self) -> &str { - // SAFETY: `params` is guaranteed to be valid JSON, - // and a valid slice of `bytes`. - unsafe { core::str::from_utf8_unchecked(self.bytes.get_unchecked(self.params.clone())) } + if let Some(range) = &self.params { + // SAFETY: `range` is guaranteed to be valid JSON, and a valid + // slice of `bytes`. + unsafe { core::str::from_utf8_unchecked(self.bytes.get_unchecked(range.clone())) } + } else { + "null" + } } /// Deserialize the params field into a type. diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 18e12cf..5615042 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -28,8 +28,42 @@ pub fn test_router() -> ajj::Router<()> { /// Test clients pub trait TestClient { - async fn send(&mut self, method: &str, params: &S); + fn next_id(&mut self) -> usize; + + fn last_id(&self) -> usize; + + async fn send_raw(&mut self, msg: &S); + async fn recv(&mut self) -> D; + + async fn send(&mut self, method: &str, params: &S) { + let id = self.next_id(); + self.send_raw(&serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "method": method, + "params": params, + })) + .await; + } +} + +async fn test_missing_id(client: &mut T) { + client + .send_raw(&serde_json::json!( + {"jsonrpc": "2.0", "method": "ping"} + )) + .await; + + let next: Value = client.recv().await; + assert_eq!( + next, + serde_json::json!({ + "jsonrpc": "2.0", + "result": "pong", + "id": null, + }) + ); } /// basic tests of the test router @@ -39,14 +73,14 @@ pub async fn basic_tests(mut client: T) { let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": 0, "jsonrpc": "2.0", "result": "pong"}) + serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": "pong"}) ); client.send("double", &5).await; let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": 1, "jsonrpc": "2.0", "result": 10}) + serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": 10}) ); client.send("notify", &()).await; @@ -56,7 +90,7 @@ pub async fn basic_tests(mut client: T) { let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": 2, "jsonrpc": "2.0", "result": null}) + serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": null}) ); let next: Value = client.recv().await; @@ -65,4 +99,6 @@ pub async fn basic_tests(mut client: T) { next, serde_json::json!({"method": "notify", "result": "notified"}) ); + + test_missing_id(&mut client).await; } diff --git a/tests/ipc.rs b/tests/ipc.rs index 6acb228..c593263 100644 --- a/tests/ipc.rs +++ b/tests/ipc.rs @@ -12,6 +12,7 @@ use interprocess::local_socket::{ use serde_json::Value; use tempfile::{NamedTempFile, TempPath}; use tokio::io::AsyncWriteExt; +use tracing::Level; pub(crate) fn to_name(path: &std::ffi::OsStr) -> std::io::Result> { if cfg!(windows) && !path.as_encoded_bytes().starts_with(br"\\.\pipe\") { @@ -64,24 +65,21 @@ impl IpcClient { async fn recv_inner(&mut self) -> serde_json::Value { self.recv_half.next().await.unwrap() } +} +impl TestClient for IpcClient { fn next_id(&mut self) -> usize { let id = self.id; self.id += 1; id } -} -impl TestClient for IpcClient { - async fn send(&mut self, method: &str, params: &S) { - let id = self.next_id(); - self.send_inner(&serde_json::json!({ - "jsonrpc": "2.0", - "id": id, - "method": method, - "params": params, - })) - .await; + fn last_id(&self) -> usize { + self.id - 1 + } + + async fn send_raw(&mut self, msg: &S) { + self.send_inner(msg).await; } async fn recv(&mut self) -> D { diff --git a/tests/ws.rs b/tests/ws.rs index 83bac25..b44f6b6 100644 --- a/tests/ws.rs +++ b/tests/ws.rs @@ -20,7 +20,7 @@ async fn serve_ws() -> ServerShutdown { struct WsClient { socket: WebSocketStream>, - id: u64, + id: usize, } impl WsClient { @@ -37,24 +37,21 @@ impl WsClient { _ => panic!("unexpected message type"), } } +} - fn next_id(&mut self) -> u64 { +impl TestClient for WsClient { + fn next_id(&mut self) -> usize { let id = self.id; self.id += 1; id } -} -impl TestClient for WsClient { - async fn send(&mut self, method: &str, params: &S) { - let id = self.next_id(); - self.send_inner(&serde_json::json!({ - "jsonrpc": "2.0", - "id": id, - "method": method, - "params": params, - })) - .await; + fn last_id(&self) -> usize { + self.id as usize - 1 + } + + async fn send_raw(&mut self, msg: &S) { + self.send_inner(msg).await; } async fn recv(&mut self) -> D { From 3d9cad3f90b2baf9433f6f44145452f5876e41ed Mon Sep 17 00:00:00 2001 From: James Date: Wed, 22 Jan 2025 15:27:28 -0500 Subject: [PATCH 2/6] lint: clippy --- tests/ws.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ws.rs b/tests/ws.rs index b44f6b6..1186266 100644 --- a/tests/ws.rs +++ b/tests/ws.rs @@ -47,7 +47,7 @@ impl TestClient for WsClient { } fn last_id(&self) -> usize { - self.id as usize - 1 + self.id - 1 } async fn send_raw(&mut self, msg: &S) { From 46d48598401c6e8e2d7bd698270e08511e338a65 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 22 Jan 2025 15:28:28 -0500 Subject: [PATCH 3/6] lint: clippy again --- tests/ipc.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/ipc.rs b/tests/ipc.rs index c593263..3c781f9 100644 --- a/tests/ipc.rs +++ b/tests/ipc.rs @@ -12,7 +12,6 @@ use interprocess::local_socket::{ use serde_json::Value; use tempfile::{NamedTempFile, TempPath}; use tokio::io::AsyncWriteExt; -use tracing::Level; pub(crate) fn to_name(path: &std::ffi::OsStr) -> std::io::Result> { if cfg!(windows) && !path.as_encoded_bytes().starts_with(br"\\.\pipe\") { From 42e51549c5b6d0557912ef996c227ad210280e77 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 22 Jan 2025 15:30:50 -0500 Subject: [PATCH 4/6] test: reorganize and break up --- tests/common/mod.rs | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 5615042..585f5f7 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -48,6 +48,17 @@ pub trait TestClient { } } +/// basic tests of the test router +pub async fn basic_tests(mut client: T) { + test_ping(&mut client).await; + + test_double(&mut client).await; + + test_notify(&mut client).await; + + test_missing_id(&mut client).await; +} + async fn test_missing_id(client: &mut T) { client .send_raw(&serde_json::json!( @@ -66,39 +77,40 @@ async fn test_missing_id(client: &mut T) { ); } -/// basic tests of the test router -pub async fn basic_tests(mut client: T) { - client.send("ping", &()).await; +async fn test_notify(client: &mut T) { + client.send("notify", &()).await; + + let now = std::time::Instant::now(); let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": "pong"}) + serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": null}) ); - client.send("double", &5).await; let next: Value = client.recv().await; + assert!(now.elapsed().as_millis() >= 100); assert_eq!( next, - serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": 10}) + serde_json::json!({"method": "notify", "result": "notified"}) ); +} - client.send("notify", &()).await; - - let now = std::time::Instant::now(); - +async fn test_double(client: &mut T) { + client.send("double", &5).await; let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": null}) + serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": 10}) ); +} + +async fn test_ping(client: &mut T) { + client.send("ping", &()).await; let next: Value = client.recv().await; - assert!(now.elapsed().as_millis() >= 100); assert_eq!( next, - serde_json::json!({"method": "notify", "result": "notified"}) + serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": "pong"}) ); - - test_missing_id(&mut client).await; } From 0aa03d56aae19dcae6048f93990937a0f207261e Mon Sep 17 00:00:00 2001 From: James Date: Wed, 22 Jan 2025 15:37:28 -0500 Subject: [PATCH 5/6] clean: test organization and ids --- tests/common/mod.rs | 17 ++++++++--------- tests/ipc.rs | 4 ---- tests/ws.rs | 4 ---- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 585f5f7..8c831d8 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -30,13 +30,11 @@ pub fn test_router() -> ajj::Router<()> { pub trait TestClient { fn next_id(&mut self) -> usize; - fn last_id(&self) -> usize; - async fn send_raw(&mut self, msg: &S); async fn recv(&mut self) -> D; - async fn send(&mut self, method: &str, params: &S) { + async fn send(&mut self, method: &str, params: &S) -> usize { let id = self.next_id(); self.send_raw(&serde_json::json!({ "jsonrpc": "2.0", @@ -45,6 +43,7 @@ pub trait TestClient { "params": params, })) .await; + id } } @@ -78,14 +77,14 @@ async fn test_missing_id(client: &mut T) { } async fn test_notify(client: &mut T) { - client.send("notify", &()).await; + let id = client.send("notify", &()).await; let now = std::time::Instant::now(); let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": null}) + serde_json::json!({"id": id, "jsonrpc": "2.0", "result": null}) ); let next: Value = client.recv().await; @@ -97,20 +96,20 @@ async fn test_notify(client: &mut T) { } async fn test_double(client: &mut T) { - client.send("double", &5).await; + let id = client.send("double", &5).await; let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": 10}) + serde_json::json!({"id": id, "jsonrpc": "2.0", "result": 10}) ); } async fn test_ping(client: &mut T) { - client.send("ping", &()).await; + let id = client.send("ping", &()).await; let next: Value = client.recv().await; assert_eq!( next, - serde_json::json!({"id": client.last_id(), "jsonrpc": "2.0", "result": "pong"}) + serde_json::json!({"id": id, "jsonrpc": "2.0", "result": "pong"}) ); } diff --git a/tests/ipc.rs b/tests/ipc.rs index 3c781f9..7000947 100644 --- a/tests/ipc.rs +++ b/tests/ipc.rs @@ -73,10 +73,6 @@ impl TestClient for IpcClient { id } - fn last_id(&self) -> usize { - self.id - 1 - } - async fn send_raw(&mut self, msg: &S) { self.send_inner(msg).await; } diff --git a/tests/ws.rs b/tests/ws.rs index 1186266..c2aacb8 100644 --- a/tests/ws.rs +++ b/tests/ws.rs @@ -46,10 +46,6 @@ impl TestClient for WsClient { id } - fn last_id(&self) -> usize { - self.id - 1 - } - async fn send_raw(&mut self, msg: &S) { self.send_inner(msg).await; } From a5a86c8279a54f8e9f35e2e99cebda669ce14ef0 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 22 Jan 2025 15:39:14 -0500 Subject: [PATCH 6/6] chore: bump to 0.1.2 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 2f097f3..a319567 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ description = "Simple, modern, ergonomic JSON-RPC 2.0 router built with tower an keywords = ["json-rpc", "jsonrpc", "json"] categories = ["web-programming::http-server", "web-programming::websocket"] -version = "0.1.1" +version = "0.1.2" edition = "2021" rust-version = "1.81" authors = ["init4", "James Prestwich"]