-
Notifications
You must be signed in to change notification settings - Fork 273
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
Omit params field from requests instead of sending null value #640
base: master
Are you sure you want to change the base?
Changes from all commits
9d785c8
7bf2b7b
6854e8e
60aeb01
ececfbf
c89ced2
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 |
---|---|---|
|
@@ -30,6 +30,10 @@ pub trait Rpc { | |
#[rpc(name = "raw", params = "raw")] | ||
fn raw(&self, params: Params) -> Result<String>; | ||
|
||
/// Squares a number, default value is 1. | ||
#[rpc(name = "sqr")] | ||
fn sqr(&self, a: Option<u64>) -> Result<u64>; | ||
|
||
/// Handles a notification. | ||
#[rpc(name = "notify")] | ||
fn notify(&self, a: u64); | ||
|
@@ -55,6 +59,11 @@ impl Rpc for RpcImpl { | |
Ok("OK".into()) | ||
} | ||
|
||
fn sqr(&self, a: Option<u64>) -> Result<u64> { | ||
let a = a.unwrap_or(1); | ||
Ok(a * a) | ||
} | ||
|
||
fn notify(&self, a: u64) { | ||
println!("Received `notify` with value: {}", a); | ||
} | ||
|
@@ -222,6 +231,49 @@ fn should_accept_any_raw_params() { | |
assert_eq!(expected, result4); | ||
} | ||
|
||
#[test] | ||
fn should_accept_optional_param() { | ||
let mut io = IoHandler::new(); | ||
let rpc = RpcImpl::default(); | ||
io.extend_with(rpc.to_delegate()); | ||
|
||
// when | ||
let req1 = r#"{"jsonrpc":"2.0","id":1,"method":"sqr","params":[2]}"#; | ||
let req2 = r#"{"jsonrpc":"2.0","id":1,"method":"sqr","params":[]}"#; | ||
let req3 = r#"{"jsonrpc":"2.0","id":1,"method":"sqr","params":null}"#; | ||
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. Shouldn't this be rejected as invalid? 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's a good question, I know only KODI jsonrpc server, that rejects only 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. I'd distinguish 3 different cases:
In case (1) IMHO only Regarding 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.
hrm, really? In the client code
I'm fine with that and it might better to be explicit, mainly because it might be possible that a request contains the wrong type/format and the default value is then used without that the caller gets informed by that.... 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.
I was checking the code and it might not be what we do now. AFAICT in case of optional parameters we parse The code in PR is an improvement for the client side for sure, we might want to follow up on how we want the server to behave separately. |
||
let req4 = r#"{"jsonrpc":"2.0","id":1,"method":"sqr"}"#; | ||
|
||
let res1 = io.handle_request_sync(req1); | ||
let res2 = io.handle_request_sync(req2); | ||
let res3 = io.handle_request_sync(req3); | ||
let res4 = io.handle_request_sync(req4); | ||
let expected1 = r#"{ | ||
"jsonrpc": "2.0", | ||
"result": 4, | ||
"id": 1 | ||
}"#; | ||
let expected1: Response = serde_json::from_str(expected1).unwrap(); | ||
let expected2 = r#"{ | ||
"jsonrpc": "2.0", | ||
"result": 1, | ||
"id": 1 | ||
}"#; | ||
let expected2: Response = serde_json::from_str(expected2).unwrap(); | ||
|
||
// then | ||
let result1: Response = serde_json::from_str(&res1.unwrap()).unwrap(); | ||
assert_eq!(expected1, result1); | ||
|
||
let result2: Response = serde_json::from_str(&res2.unwrap()).unwrap(); | ||
assert_eq!(expected2, result2); | ||
|
||
let result3: Response = serde_json::from_str(&res3.unwrap()).unwrap(); | ||
assert_eq!(expected2, result3); | ||
|
||
let result4: Response = serde_json::from_str(&res4.unwrap()).unwrap(); | ||
assert_eq!(expected2, result4); | ||
} | ||
|
||
#[test] | ||
fn should_accept_only_notifications() { | ||
let mut io = IoHandler::new(); | ||
|
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.
I would like to add requests with
"params":[]
and"params":null
here otherwise looks greatThere 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.
Added those variations to the test case.