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.

Sign up for GitHub

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(server): improve too big batch response msg #1107

Merged
merged 3 commits into from
May 2, 2023
Merged

fix(server): improve too big batch response msg #1107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37a851c
fix(server): improve too big batch response msg
niklasad1 May 2, 2023
e15d7dd
adjust tests
niklasad1 May 2, 2023
a87c539
fix tests
niklasad1 May 2, 2023
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
8 changes: 5 additions & 3 deletions core/src/server/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ use std::io;
use std::time::Duration;

use crate::tracing::tx_log_from_str;
use jsonrpsee_types::error::{ErrorCode, ErrorObject, OVERSIZED_RESPONSE_CODE, OVERSIZED_RESPONSE_MSG};
use jsonrpsee_types::error::{
reject_too_big_batch_response, ErrorCode, ErrorObject, OVERSIZED_RESPONSE_CODE, OVERSIZED_RESPONSE_MSG,
};
use jsonrpsee_types::{Id, InvalidRequest, Response, ResponsePayload};
use serde::Serialize;
use serde_json::value::to_raw_value;
Expand Down Expand Up @@ -268,7 +270,7 @@ impl BatchResponseBuilder {
let len = response.result.len() + self.result.len() + 1;

if len > self.max_response_size {
Err(batch_response_error(Id::Null, ErrorObject::from(ErrorCode::InvalidRequest)))
Err(batch_response_error(Id::Null, reject_too_big_batch_response(self.max_response_size)))
} else {
self.result.push_str(&response.result);
self.result.push(',');
Expand Down Expand Up @@ -365,7 +367,7 @@ mod tests {

let batch = BatchResponseBuilder::new_with_limit(63).append(&method).unwrap_err();

let exp_err = r#"{"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid request"},"id":null}"#;
let exp_err = r#"{"jsonrpc":"2.0","error":{"code":-32011,"message":"The batch response was too large","data":"Exceeded max limit of 63"},"id":null}"#;
assert_eq!(batch, exp_err);
}
}
2 changes: 1 addition & 1 deletion server/src/tests/http.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ async fn can_set_the_max_response_size_to_batch() {
// Two response will end up in a response of 102 bytes which is too big.
let req = r#"[{"jsonrpc":"2.0", "method":"anything", "id":1},{"jsonrpc":"2.0", "method":"anything", "id":2}]"#;
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap();
assert_eq!(response.body, invalid_request(Id::Null));
assert_eq!(response.body, batch_response_too_large(100));

handle.stop().unwrap();
handle.stopped().await;
Expand Down
2 changes: 1 addition & 1 deletion server/src/tests/ws.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ async fn can_set_the_max_response_size_to_batch() {
// Two response will end up in a response bigger than 100 bytes.
let req = r#"[{"jsonrpc":"2.0", "method":"anything", "id":1},{"jsonrpc":"2.0", "method":"anything", "id":2}]"#;
let response = client.send_request_text(req).await.unwrap();
assert_eq!(response, invalid_request(Id::Null));
assert_eq!(response, batch_response_too_large(100));

server_handle.stop().unwrap();
server_handle.stopped().await;
Expand Down
6 changes: 6 additions & 0 deletions test-utils/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ pub fn batches_too_large(max_limit: usize) -> String {
)
}

pub fn batch_response_too_large(max_limit: usize) -> String {
format!(
r#"{{"jsonrpc":"2.0","error":{{"code":-32011,"message":"The batch response was too large","data":"Exceeded max limit of {max_limit}"}},"id":null}}"#
)
}

pub fn oversized_response(id: Id, max_limit: u32) -> String {
format!(
r#"{{"jsonrpc":"2.0","error":{{"code":-32008,"message":"Response is too big","data":"Exceeded max limit of {}"}},"id":{}}}"#,
Expand Down
23 changes: 20 additions & 3 deletions types/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ pub const OVERSIZED_RESPONSE_CODE: i32 = -32008;
/// Server is busy error code.
pub const SERVER_IS_BUSY_CODE: i32 = -32009;
/// Batch request limit was exceed.
pub const TOO_BIG_BATCH_CODE: i32 = -32010;
pub const TOO_BIG_BATCH_REQUEST_CODE: i32 = -32010;
/// Batch request limit was exceed.
pub const TOO_BIG_BATCH_RESPONSE_CODE: i32 = -32011;

/// Parse error message
pub const PARSE_ERROR_MSG: &str = "Parse error";
Expand All @@ -160,7 +162,9 @@ pub const BATCHES_NOT_SUPPORTED_MSG: &str = "Batched requests are not supported
/// Subscription limit per connection was exceeded.
pub const TOO_MANY_SUBSCRIPTIONS_MSG: &str = "Too many subscriptions on the connection";
/// Batched requests limit was exceed.
pub const TOO_BIG_BATCH_MSG: &str = "The batch request was too large";
pub const TOO_BIG_BATCH_REQUEST_MSG: &str = "The batch request was too large";
/// Batch request response limit was exceed.
pub const TOO_BIG_BATCH_RESPONSE_MSG: &str = "The batch response was too large";

/// JSONRPC error code
#[derive(Error, Debug, PartialEq, Eq, Copy, Clone)]
Expand Down Expand Up @@ -276,7 +280,20 @@ pub fn reject_too_big_request(limit: u32) -> ErrorObjectOwned {

/// Helper to get a `JSON-RPC` error object when the maximum batch request size have been exceeded.
pub fn reject_too_big_batch_request(limit: usize) -> ErrorObjectOwned {
ErrorObjectOwned::owned(TOO_BIG_BATCH_CODE, TOO_BIG_BATCH_MSG, Some(format!("Exceeded max limit of {limit}")))
ErrorObjectOwned::owned(
TOO_BIG_BATCH_REQUEST_CODE,
TOO_BIG_BATCH_REQUEST_MSG,
Some(format!("Exceeded max limit of {limit}")),
)
}

/// Helper to get a `JSON-RPC` error object when the maximum batch response size have been exceeded.
pub fn reject_too_big_batch_response(limit: usize) -> ErrorObjectOwned {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We could also provide extra insight into how many bytes the message contained, also we could state for clarity the units (ie bytes)

Some(format!("Request length {len} exceeded max limit of {limit} bytes"))

This info will also be useful in debugging some tests that are still exceeding the 100 char limit.

Although, I leave this up to you if it makes sense! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a little weird as we terminate when the limit is exceeded as we are processing/building responses

so the entire response could be much bigger than len but for "batch request (input)" that makes sense

ErrorObjectOwned::owned(
TOO_BIG_BATCH_RESPONSE_CODE,
TOO_BIG_BATCH_RESPONSE_MSG,
Some(format!("Exceeded max limit of {limit}")),
)
}

#[cfg(test)]
Expand Down