Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

handle both number and string ids #361

Merged
merged 1 commit into from
Jul 5, 2017
Merged

handle both number and string ids #361

merged 1 commit into from
Jul 5, 2017

Conversation

Trolldemorted
Copy link
Contributor

fixes #360

@Xanewok
Copy link
Member

Xanewok commented Jun 20, 2017

I think ideally we should switch from using usize to NumberOrString (from languageserver-types) in Requests, as it's already being used for protocol data serialization in languageserver-types.

@nrc
Copy link
Member

nrc commented Jun 20, 2017

I think ideally we should switch from using usize to NumberOrString (from languageserver-types) in Requests,

I agree with this. My worry is that technically a client could send "foo" as an id and we'd still crash. If we're going to fix this, we should fix it as strongly as possible.

@Trolldemorted
Copy link
Contributor Author

I understand that you want this solved properly, this ist just a cheap fix for lsp clients like lsp4j.

You might want to merge this nevertheless until a proper solution is implemented, it took some time of debugging rls and lsp4j for me to realize what was going wrong.

@nrc
Copy link
Member

nrc commented Jul 4, 2017

You might want to merge this nevertheless until a proper solution is implemented, it took some time of debugging rls and lsp4j for me to realize what was going wrong.

Fair enough, lets land this PR. I left some comments inline.

nrc
nrc reviewed Jul 4, 2017
View reviewed changes
src/server.rs Outdated
} else if id_value.is_string() {
id = Some(usize::from_str_radix(id_value.as_str().unwrap(), 10).unwrap());
} else {
return Err(ParseError::new(ErrorKind::InvalidData, "Id is not a number or numeric string", id))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not return early here, since if we don't need the id, we might be able to process the message and ignore the id. Where we currently return an error, you could add the 'numeric string' bit.

src/server.rs Outdated
@@ -127,7 +127,15 @@ macro_rules! messages {
});
}

let id = ls_command.get("id").and_then(|id| id.as_u64()).map(|id| id as usize);
let id_value = ls_command.get("id").unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid the unwrap here, and just make id None?

src/server.rs Outdated
if id_value.is_u64() {
id = Some(ls_command.get("id").unwrap().as_u64().unwrap() as usize);
} else if id_value.is_string() {
id = Some(usize::from_str_radix(id_value.as_str().unwrap(), 10).unwrap());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these unwraps too.

@Trolldemorted
Copy link
Contributor Author

Trolldemorted commented Jul 4, 2017 •
edited
Loading

Done!

Now all unwraps that remain are guarded by is_u64() and is_string().

@nrc
Copy link
Member

nrc commented Jul 5, 2017

Thanks!

@nrc nrc merged commit bdfe6a8 into rust-lang:master Jul 5, 2017
@Trolldemorted Trolldemorted mentioned this pull request Jul 5, 2017
Closed
bors added a commit that referenced this pull request Feb 1, 2019
@bors
Auto merge of #1279 - rust-lang:dependabot/cargo/jsonrpc-core-10.0.1,…
3b4f83f 
… r=alexheretic

Bump jsonrpc-core from 9.0.0 to 10.0.1

Bumps [jsonrpc-core](https://github.com/paritytech/jsonrpc) from 9.0.0 to 10.0.1.
<details>
<summary>Release notes</summary>

*Sourced from [jsonrpc-core's releases](https://github.com/paritytech/jsonrpc/releases).*

> ## JSON-RPC v10.0.1
> Fixes documentation links and authors.
>
> ```
> jsonrpc-core = "10.0.1"
> jsonrpc-macros = "10.0.1"
> jsonrpc-derive = "10.0.1"
> jsonrpc-pubsub = "10.0.1"
> jsonrpc-server-utils = "10.0.1"
> jsonrpc-http-server = "10.0.1"
> jsonrpc-ipc-server = "10.0.1"
> jsonrpc-tcp-server = "10.0.1"
> jsonrpc-ws-server = "10.0.1"
> jsonrpc-stdio-server = "10.0.1"
> jsonrpc-test = "10.0.1"
> ```
>
> ## JSON-RPC v10.0.0
> This is an exciting release, because it's the first time we release all parts of the suite to crates.io! So long github dependencies!
>
> Most notable changes:
> 1. Deprecating `jsonrpc-macros` in favour of `jsonrpc-derive` (proc-macro based)
> 2. Rewrting all crates to `edition = 2018`
>
> Other changes:
> - Adding `Metadata` to unsubscribe methods (when they are called explicitly)
> - Derive `Clone` for all `core` types
> - Fix custom bounds on the types in `jsonrpc-macros`
> - Fix charset case sensitivity for http server
>
>
> Full list of changes: paritytech/jsonrpc@v9.0.0...v10.0.0
>
> ```
> jsonrpc-core = "10.0.0"
> jsonrpc-macros = "10.0.0"
> jsonrpc-pubsub = "10.0.0"
> jsonrpc-server-utils = "10.0.0"
> jsonrpc-http-server = "10.0.0"
> jsonrpc-ipc-server = "10.0.0"
> jsonrpc-tcp-server = "10.0.0"
> jsonrpc-ws-server = "10.0.0"
> jsonrpc-stdio-server = "10.0.0"
> jsonrpc-test = "10.0.0"
> ```
</details>
<details>
<summary>Commits</summary>

- [`8ffc7f3`](paritytech/jsonrpc@8ffc7f3) Add jsonrpc-derive details. ([#372](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/372))
- [`2fe3bfc`](paritytech/jsonrpc@2fe3bfc) One way serialize for subscriber type ([#371](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/371))
- [`37b62a3`](paritytech/jsonrpc@37b62a3) Migrate to edition 2018 ([#368](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/368))
- [`5a7be5f`](paritytech/jsonrpc@5a7be5f) Bump version of stdio ([#367](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/367))
- [`2b47a5c`](paritytech/jsonrpc@2b47a5c) Use parity-ws-rs from crates.io ([#361](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/361))
- [`8160eb5`](paritytech/jsonrpc@8160eb5) ci: bring appveyor back ([#364](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/364))
- [`80205d4`](paritytech/jsonrpc@80205d4) Support multiple trailing arguments ([#365](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/365))
- [`ec5249e`](paritytech/jsonrpc@ec5249e) Use procedural macros  ([#340](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/340))
- [`789c74d`](paritytech/jsonrpc@789c74d) Derive Clone for all core types::* ([#359](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/359))
- [`16ed9f5`](paritytech/jsonrpc@16ed9f5) Fix tokio deprecation warnings ([#358](https://github-redirect.dependabot.com/paritytech/jsonrpc/issues/358))
- Additional commits viewable in [compare view](paritytech/jsonrpc@v9.0.0...v10.0.1)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=jsonrpc-core&package-manager=cargo&previous-version=9.0.0&new-version=10.0.1)](https://dependabot.com/compatibility-score.html?dependency-name=jsonrpc-core&package-manager=cargo&previous-version=9.0.0&new-version=10.0.1)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nrc nrc nrc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rls does not cope with non-numeric strings as request ids
3 participants
@Trolldemorted @Xanewok @nrc