From 854c245cc4fd10f1cbb8ea7ba53a7363073adf18 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Tue, 11 Jul 2017 11:41:14 +0200 Subject: [PATCH] Tighten `parse_message` error handling logic --- src/cmd.rs | 2 +- src/server.rs | 54 ++++++++++++++++++++++++++++++++------------------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/cmd.rs b/src/cmd.rs index 251bfcac07f..5e1eeb7c632 100644 --- a/src/cmd.rs +++ b/src/cmd.rs @@ -217,7 +217,7 @@ impl ChannelMsgReader { } impl server::MessageReader for ChannelMsgReader { - fn parsed_message(&self) -> Option> { + fn parsed_message(&self) -> Option>> { let channel = self.channel.lock().unwrap(); let msg = channel.recv().expect("Error reading from channel"); Some(Ok(msg)) diff --git a/src/server.rs b/src/server.rs index 06ad0fa4353..a6611339630 100644 --- a/src/server.rs +++ b/src/server.rs @@ -117,24 +117,36 @@ macro_rules! messages { $($method_name$(($method_arg))*,)* } - // TODO: Change return type to Result>, - // since Notifications don't have id and only Requests can return a response - fn parse_message(input: &str) -> Result { + // Notifications can't return a response, hence why Err is an Option + fn parse_message(input: &str) -> Result> { trace!("parse_message `{}`", input); - let ls_command: serde_json::Value = serde_json::from_str(input).expect("Can't make value"); + let ls_command: serde_json::Value = match serde_json::from_str(input) { + Ok(value) => value, + Err(_) => return Err(Some(server_failure(Id::Null, jsonrpc::Error::parse_error()))), + }; let params = ls_command.get("params"); macro_rules! params_as { ($ty: ty) => ({ - let method: $ty = - serde_json::from_value(params.expect("no params").to_owned()).expect("Can't extract params"); + let params = match params { + Some(value) => value, + None => return Err(Some(server_failure(Id::Null, jsonrpc::Error::invalid_request()))), + }; + + let method: $ty = match serde_json::from_value(params.to_owned()) { + Ok(value) => value, + Err(_) => return Err(Some(server_failure(Id::Null, + jsonrpc::Error::invalid_params(format!("Expected {}", stringify!($ty)))))), + }; method }); } // Per JSON-RPC/LSP spec, Requests must have id, whereas Notifications can't let id = ls_command.get("id").map(|id| serde_json::from_value(id.to_owned()).unwrap()); + // TODO: We only support numeric responses, ideally we should switch from using parsed usize + // to using jsonrpc_core::Id let parsed_numeric_id = match &id { &Some(Id::Num(n)) => Some(n as usize), &Some(Id::Str(ref s)) => usize::from_str_radix(s, 10).ok(), @@ -148,7 +160,12 @@ macro_rules! messages { $method_str => { match parsed_numeric_id { Some(id) => Ok(ServerMessage::Request(Request{id, method: Method::$method_name$((params_as!($method_arg)))* })), - None => Err(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::invalid_params("Id is not a number or numeric string"))), + None => match id { + None => Err(Some(server_failure(Id::Null, jsonrpc::Error::invalid_request()))), + // FIXME: This behaviour is custom and non conformant to the protocol + Some(id) => Err(Some(server_failure(id, + jsonrpc::Error::invalid_params("Id is not a number or numeric string")))), + } } } )* @@ -159,14 +176,17 @@ macro_rules! messages { )* $( // If $other_expr is Err, then we need to pass id of actual message we're handling - $other_str => { ($other_expr).map_err(|err| server_failure(id.unwrap_or(Id::Null), err)) } + $other_str => { ($other_expr).map_err(|err| id.map(|id| server_failure(id, err))) } )* } } else { - Err(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::invalid_params("Method is not a string"))) + // Message has a "method" field, so it can be a Notification/Request - if it doesn't have id then we + // assume it's a Notification for which we can't return a response, so return Err(None) + Err(id.map(|id| server_failure(id, jsonrpc::Error::invalid_request()))) } } else { - Err(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::method_not_found())) + // FIXME: Handle possible client responses to server->client requests (which don't have "method" field) + Err(Some(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::invalid_request()))) } } @@ -277,13 +297,7 @@ messages! { "$/cancelRequest" => Cancel(CancelParams); "workspace/didChangeConfiguration" => WorkspaceChangeConfiguration(DidChangeConfigurationParams); } - // TODO handle me - "$/setTraceNotification" => Err(jsonrpc::Error { - code: jsonrpc::ErrorCode::MethodNotFound, - message: String::from("setTraceNotification"), - data: None - }); - _ => Err(jsonrpc::Error::method_not_found()); + _ => Err(jsonrpc::Error::method_not_found()); // TODO: Handle more possible messages } pub struct LsService { @@ -414,8 +428,8 @@ impl LsService { }, Err(e) => { trace!("parsing invalid message: {:?}", e); - if e.id != Id::Null { - this.output.failure(e.id, e.error); + if let Some(failure) = e { + this.output.failure(failure.id, failure.error); } }, } @@ -492,7 +506,7 @@ pub trait MessageReader { None } - fn parsed_message(&self) -> Option> { + fn parsed_message(&self) -> Option>> { self.read_message().map(|m| parse_message(&m)) } }