-
Notifications
You must be signed in to change notification settings - Fork 173
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
rpc module: report error on invalid subscription #561
Conversation
@@ -73,7 +73,7 @@ impl<'a> From<ErrorCode> for ErrorObject<'a> { | |||
impl<'a> PartialEq for ErrorObject<'a> { | |||
fn eq(&self, other: &Self) -> bool { | |||
let this_raw = self.data.map(|r| r.get()); | |||
let other_raw = self.data.map(|r| r.get()); | |||
let other_raw = other.data.map(|r| r.get()); |
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.
unrelated bug I found
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.
Oops. No test for this eh?
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.
Indeed
@@ -569,3 +587,58 @@ async fn run_forever() { | |||
// Send the shutdown request from one handle and await the server on the second one. | |||
join(server_handle.clone().stop().unwrap(), server_handle).with_timeout(TIMEOUT).await.unwrap(); | |||
} | |||
|
|||
#[tokio::test] | |||
async fn unsubscribe_twice_should_indicate_error() { |
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.
ideally these tests should be in the RPC module but I have to refactor the call_with
for that to work because inner channel
is dropped in call_with/call/execute
...
@@ -73,7 +73,7 @@ impl<'a> From<ErrorCode> for ErrorObject<'a> { | |||
impl<'a> PartialEq for ErrorObject<'a> { | |||
fn eq(&self, other: &Self) -> bool { | |||
let this_raw = self.data.map(|r| r.get()); | |||
let other_raw = self.data.map(|r| r.get()); | |||
let other_raw = other.data.map(|r| r.get()); |
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.
Oops. No test for this eh?
types/src/v2/error.rs
Outdated
@@ -84,6 +98,8 @@ pub const PARSE_ERROR_CODE: i32 = -32700; | |||
pub const OVERSIZED_REQUEST_CODE: i32 = -32701; | |||
/// Oversized response error code. | |||
pub const OVERSIZED_RESPONSE_CODE: i32 = -32702; | |||
/// Oversized response error code. | |||
pub const INVALID_SUBSCRIPTION_CODE: i32 = -32703; |
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.
BTW, technically these error codes should be used for application errors.
To be really nitpicky I guess we should use -32000 to -32099 Server defined errors here
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.
Given this is a new code, do you think we should do the right thing for new error codes and stay within the right range?
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, I used -32002
for this however the message (according to the spec it should be "Server error") is poor so we would have add everything in the data field
Closing #559