-
Notifications
You must be signed in to change notification settings - Fork 175
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
feat(servers): add max_response_size
#711
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -405,3 +405,46 @@ async fn run_forever() { | |
server_handle.handle.take(); | ||
server_handle.with_timeout(TIMEOUT).await.unwrap(); | ||
} | ||
|
||
#[tokio::test] | ||
async fn can_set_the_max_request_body_size() { | ||
let addr = "127.0.0.1:0"; | ||
// Rejects all requests larger than 100 bytes | ||
let server = HttpServerBuilder::default().max_request_body_size(100).build(addr).unwrap(); | ||
let mut module = RpcModule::new(()); | ||
module.register_method("anything", |_p, _cx| Ok("a".repeat(100))).unwrap(); | ||
let addr = server.local_addr().unwrap(); | ||
let uri = to_http_uri(addr); | ||
let handle = server.start(module).unwrap(); | ||
|
||
// Invalid: too long | ||
let req = format!(r#"{{"jsonrpc":"2.0", "method":{}, "id":1}}"#, "a".repeat(100)); | ||
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap(); | ||
assert_eq!(response.body, oversized_request()); | ||
|
||
// Max request body size should not override the max response size | ||
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#; | ||
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap(); | ||
assert_eq!(response.body, ok_response(JsonValue::String("a".repeat(100)), Id::Num(1))); | ||
|
||
handle.stop().unwrap(); | ||
} | ||
|
||
#[tokio::test] | ||
async fn can_set_the_max_response_size() { | ||
let addr = "127.0.0.1:0"; | ||
// Set the max response size to 100 bytes | ||
let server = HttpServerBuilder::default().max_response_body_size(100).build(addr).unwrap(); | ||
let mut module = RpcModule::new(()); | ||
module.register_method("anything", |_p, _cx| Ok("a".repeat(101))).unwrap(); | ||
let addr = server.local_addr().unwrap(); | ||
let uri = to_http_uri(addr); | ||
let handle = server.start(module).unwrap(); | ||
|
||
// Oversized response. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above was done above test, is it worth also checking that the request size is not an issue when only the response size is limited? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean that actual max response works when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it won't work unless you register another method for that. |
||
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#; | ||
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap(); | ||
assert_eq!(response.body, oversized_response(Id::Num(1), 100)); | ||
|
||
handle.stop().unwrap(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,7 +183,7 @@ async fn can_set_the_max_request_body_size() { | |
init_logger(); | ||
|
||
let addr = "127.0.0.1:0"; | ||
// Rejects all requests larger than 10 bytes | ||
// Rejects all requests larger than 100 bytes | ||
let server = WsServerBuilder::default().max_request_body_size(100).build(addr).await.unwrap(); | ||
let mut module = RpcModule::new(()); | ||
module.register_method("anything", |_p, _cx| Ok("a".repeat(100))).unwrap(); | ||
|
@@ -197,6 +197,28 @@ async fn can_set_the_max_request_body_size() { | |
let response = client.send_request_text(req).await.unwrap(); | ||
assert_eq!(response, oversized_request()); | ||
|
||
// Max request body size should not override the max response body size | ||
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#; | ||
let response = client.send_request_text(req).await.unwrap(); | ||
assert_eq!(response, ok_response(JsonValue::String("a".repeat(100)), Id::Num(1))); | ||
|
||
handle.stop().unwrap(); | ||
} | ||
|
||
#[tokio::test] | ||
async fn can_set_the_max_response_body_size() { | ||
init_logger(); | ||
|
||
let addr = "127.0.0.1:0"; | ||
// Set the max response body size to 100 bytes | ||
let server = WsServerBuilder::default().max_response_body_size(100).build(addr).await.unwrap(); | ||
let mut module = RpcModule::new(()); | ||
module.register_method("anything", |_p, _cx| Ok("a".repeat(101))).unwrap(); | ||
let addr = server.local_addr().unwrap(); | ||
let handle = server.start(module).unwrap(); | ||
|
||
let mut client = WebSocketTestClient::new(addr).await.unwrap(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as my above comment; should we check that setting one hasn't impacted the other here too? |
||
// Oversized response. | ||
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#; | ||
let response = client.send_request_text(req).await.unwrap(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would it make more sense to define this as
MAX_RESPONSE_BODY_SIZE
instead ofTEN_MB_SIZE_BYTES
, to decouple naming from underlying value? But I think this is outside the scope of this PR, as the constant was already there 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, could make sense :)