Use jsonrpc-core data for improved and more consistent error handling flow #402
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);
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?
Xanewok
force-pushed
the
json-rpc
branch
2 times, most recently
from
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?
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.