-
Notifications
You must be signed in to change notification settings - Fork 217
[types]: ID type instead of serde_json::RawValue #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
653cf78
get started
niklasad1 00e7636
add additional test
niklasad1 d03e092
fix nits
niklasad1 3affd41
cargo fmt
niklasad1 eb07a01
[types]: write some tests.
niklasad1 e5e9312
[http server]: send empty response on notifs
niklasad1 3c7d20f
[http server]: fix tests
niklasad1 ace5cb2
[rpc module]: send subscription response
niklasad1 9eddc0c
Update types/src/v2/error.rs
niklasad1 78fec4f
fix nits
niklasad1 e95b6c3
Merge branch 'na-jsonrpc-types-id-and-params' of github.com:paritytec…
niklasad1 659ce7d
cargo fmt
niklasad1 d5c9da9
Update types/src/v2/params.rs
niklasad1 f81eb77
remove needless clone
niklasad1 8097e33
Merge branch 'na-jsonrpc-types-id-and-params' of github.com:paritytec…
niklasad1 26d3031
remove dead code
niklasad1 a2264bb
[types]: impl PartialEq for JsonErrorObject + test
niklasad1 84c00cd
use beef::Cow
niklasad1 7f901b4
Update http-server/src/tests.rs
niklasad1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| use crate::error::InvalidParams; | ||
| use alloc::collections::BTreeMap; | ||
| use std::borrow::Cow; | ||
|
niklasad1 marked this conversation as resolved.
Outdated
|
||
| use serde::de::{self, Deserializer, Unexpected, Visitor}; | ||
| use serde::ser::Serializer; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::{value::RawValue, Value as JsonValue}; | ||
|
|
||
|
niklasad1 marked this conversation as resolved.
Outdated
|
||
| use std::fmt; | ||
|
|
||
| /// JSON-RPC parameter values for subscriptions. | ||
|
|
@@ -26,7 +28,7 @@ pub struct JsonRpcNotificationParamsAlloc<T> { | |
| } | ||
|
|
||
| /// JSON-RPC v2 marker type. | ||
| #[derive(Debug, Default, PartialEq)] | ||
| #[derive(Clone, Copy, Debug, Default, PartialEq)] | ||
| pub struct TwoPointZero; | ||
|
|
||
| struct TwoPointZeroVisitor; | ||
|
|
@@ -157,16 +159,17 @@ impl From<SubscriptionId> for JsonValue { | |
| #[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clone should be cheap as deserialize just borrows the String AFAIU. Then in the client code we just use Thus, should never allocate in server at least?! |
||
| #[serde(deny_unknown_fields)] | ||
| #[serde(untagged)] | ||
| pub enum Id { | ||
| pub enum Id<'a> { | ||
| /// Null | ||
| Null, | ||
| /// Numeric id | ||
| Number(u64), | ||
| /// String id | ||
| Str(String), | ||
| #[serde(borrow)] | ||
| Str(Cow<'a, str>), | ||
| } | ||
|
|
||
| impl Id { | ||
| impl<'a> Id<'a> { | ||
| /// If the Id is a number, returns the associated number. Returns None otherwise. | ||
| pub fn as_number(&self) -> Option<&u64> { | ||
| match self { | ||
|
|
@@ -190,8 +193,49 @@ impl Id { | |
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// Creates owned data from borrowed data, allocates only for Strings. | ||
| pub fn to_owned(&self) -> Id<'static> { | ||
|
niklasad1 marked this conversation as resolved.
Outdated
|
||
| match self { | ||
| Id::Null => Id::Null, | ||
| Id::Number(n) => Id::Number(*n), | ||
| Id::Str(Cow::Borrowed(s)) => Id::Str(Cow::Owned(s.to_string())), | ||
| Id::Str(Cow::Owned(s)) => Id::Str(Cow::Owned(s.clone())), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Untyped JSON-RPC ID. | ||
| // TODO(niklasad1): this should be enforced to only accept: String, Number, or Null. | ||
| pub type JsonRpcRawId<'a> = Option<&'a serde_json::value::RawValue>; | ||
| #[cfg(test)] | ||
| mod test { | ||
| use super::{Cow, Id}; | ||
|
|
||
| #[test] | ||
| fn id_deserialization() { | ||
| let s = r#""2""#; | ||
| let deserialized: Id = serde_json::from_str(s).unwrap(); | ||
| assert_eq!(deserialized, Id::Str("2".into())); | ||
|
|
||
| let s = r#"2"#; | ||
| let deserialized: Id = serde_json::from_str(s).unwrap(); | ||
| assert_eq!(deserialized, Id::Number(2)); | ||
|
|
||
| let s = r#""2x""#; | ||
| let deserialized: Id = serde_json::from_str(s).unwrap(); | ||
| assert_eq!(deserialized, Id::Str(Cow::Borrowed("2x"))); | ||
|
|
||
| let s = r#"[1337]"#; | ||
| assert!(serde_json::from_str::<Id>(s).is_err()); | ||
|
|
||
| let s = r#"[null, 0, 2, "3"]"#; | ||
| let deserialized: Vec<Id> = serde_json::from_str(s).unwrap(); | ||
| assert_eq!(deserialized, vec![Id::Null, Id::Number(0), Id::Number(2), Id::Str("3".into())]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn id_serialization() { | ||
| let d = | ||
| vec![Id::Null, Id::Number(0), Id::Number(2), Id::Number(3), Id::Str("3".into()), Id::Str("test".into())]; | ||
| let serialized = serde_json::to_string(&d).unwrap(); | ||
| assert_eq!(serialized, r#"[null,0,2,3,"3","test"]"#); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I changed the error type to a String in
Error::Requestto get rid of the alloc type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I noticed that. I don't think there much else we can do right?