Use jsonrpc-core data for improved and more consistent error handling flow#402
Use jsonrpc-core data for improved and more consistent error handling flow#402nrc merged 1 commit intorust-lang:masterfrom
Conversation
|
Rebased and force-pushed now |
src/test/mod.rs
Outdated
| .expect("Couldn't parse json failure response"); | ||
|
|
||
| const PARSE_ERROR: jsonrpc_core::ErrorCode = jsonrpc_core::ErrorCode::ParseError; | ||
| assert!(failure.error.code == PARSE_ERROR); |
There was a problem hiding this comment.
Now that I look at it again I guess it could be just
assert!(failure.error.code == jsonrpc_core::ErrorCode::ParseError);
nrc
left a comment
There was a problem hiding this comment.
Seems like this stuff is all improvement.
src/server.rs
Outdated
| jsonrpc::Failure { | ||
| jsonrpc: Some(version::Version::V2), | ||
| id: id, | ||
| error: error, |
src/server.rs
Outdated
| }, | ||
| None => None | ||
| let id = ls_command.get("id").expect("no id").to_owned(); | ||
| let id: jsonrpc::Id = serde_json::from_value(id).expect("Can't extract id"); |
There was a problem hiding this comment.
We should continue to be careful not to panic here. If we don't need an id, then we should not panic on an invalid id.
There was a problem hiding this comment.
Good catch! Missed that. Added handling missing id, but I now realized that according to JSON-RPC/LSP failure responses are only sent as a response to requests (and not notifications).
I'd say we should eventually change return value of parse_message to Result<ServerMessage, Option<jsonrpc::Failure>> and handle/output failures only for received invalid Requests, but with this we're not making the flow any worse wrt that and we can improve it the next iteration. Added few reminding comments. @nrc what do you think?
src/server.rs
Outdated
| code: jsonrpc::ErrorCode::MethodNotFound, | ||
| message: String::from("setTraceNotification"), | ||
| data: None | ||
| }); |
There was a problem hiding this comment.
Is there any reason to special case setTraceNotification rather than swallow it in the _ case?
There was a problem hiding this comment.
Not really, just tried to retain the same semantics when rewriting it. Should I move it under _?
src/server.rs
Outdated
| code: METHOD_NOT_FOUND, | ||
| message: message.to_owned(), | ||
| }, | ||
| fn failure_simple<M: Into<String>>(&self, id: usize, code: jsonrpc::ErrorCode, msg: M) { |
There was a problem hiding this comment.
how about failure_message rather than failure_simple?
854c245 to
d96891e
Compare
src/server.rs
Outdated
| 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()))), |
There was a problem hiding this comment.
Could we use the id in these error messages?
There was a problem hiding this comment.
We should, updated now
Laying foundations for #395, #376 and proper #360 - JSON-RPC conformance in general and tidying the server flow logic a bit.
jsonrpc_core::Idfor thenumber | string | nullid type in JSON-RPC which we didn't support until recentlyjsonrpc_corestructs instead of our custom ones for failure/error message logic.I have also some pending WIP changes planned for
{Notification,Request}Message(here), but I still need to work on stronger typifying, will probably land with gluon-lang/lsp-types#19.Moving completely to using
jsonrpc-coremeans making a lot of internal changes, so I figured it'd be nice to move a small, self-contained portion and see if the approach is okay.