feat(client): support request id as Strings. #659
Conversation
Sounds reasonable to me, I did the most naive way I could think of to test that it worked so sure shouldn't be that hard I think |
| fn into_id(self, id: u64) -> Id<'static> { | ||
| match self { | ||
| IdKind::Number => Id::Number(id), | ||
| IdKind::String => Id::Str(format!("{}", id).into()), |
There was a problem hiding this comment.
we could probably use some optimized itoa or something but should fast enough I guess for now...
There was a problem hiding this comment.
Could also subslice byte array of all numbers from 00 to 99 as a static Cow (for values < 100), but yeah, this is good enough :).
|
Hi @niklasad1, Yes, this includes what I was looking for. Thank you! |
| @@ -350,7 +350,7 @@ impl<'a> SubscriptionId<'a> { | |||
| } | |||
|
|
|||
| /// Request Id | |||
| #[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize)] | |||
| #[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize, PartialOrd, Ord)] | |||
| #[serde(deny_unknown_fields)] | |||
| #[serde(untagged)] | |||
| pub enum Id<'a> { | |||
There was a problem hiding this comment.
I didn't notice offhand; is a borrowed version of an ID ever used here? I pretty much assume every new ID will need to be an owned variant anyway. so I wonder how useful the lifetime/Cow is?
(thinking down that line, in general any serde_json::Value could be returned as an ID (whether or not certain values make sense, who knows) so I was wondering whether using that here would simplify anything)
There was a problem hiding this comment.
I think you are right at least in the point of view for the client.
However, I think we actually can use the lifetime and do zero-copy deserialization both in the servers (on new request) and clients (on the responses) with #[serde(borrow)]
|
|
||
| #[test] | ||
| fn request_id_guard_works() { | ||
| let manager = RequestIdManager::new(2); | ||
| let manager = RequestIdManager::new(2, IdKind::Number); |
There was a problem hiding this comment.
Would it make sense to have a test where IdKind::String, not sure if this is covered elsewhere residually though.
There was a problem hiding this comment.
I think that's covered by some unit tests for the whole ws client but wouldn't hurt with a unit test for String also I suppose :)
However, the important thing is the Arc which is independent of the actual id.
Supersedes #644
It makes it possible to configure the clients to generate
request object IDas a strings instead of numbers, I did the most simple way and converting thenumeric IDto strings from theRequestIdManager.We might want to use some more optimized
itoainstead of format in future or generate random IDs.However,
numeric idsis still default because it avoids some allocations.The rationale behind this is that some
JSON-RPC serverssuffers from the issue where they can't decodeIdas numbers because of theserde-json arbritary-precisionbug, so this is a workaround for that.//cc @whalelephant I think this should be what you need right?