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
Fix: use json payloads #34
Changes from 9 commits
d0d6a7d
2062388
2d83ec7
e579285
d234386
c180388
ae04c00
6712d35
cd8b9a0
94bfd55
c760054
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ pub struct RawResponse { | |
pub enum ResponseType { | ||
SignalEntry { seq: u64 }, | ||
Publish { seq: u64 }, | ||
Subscribe(String), | ||
Subscribe(serde_json::Value), | ||
Error(String), | ||
Barrier, | ||
} | ||
|
@@ -54,14 +54,14 @@ impl From<RawResponse> for Response { | |
let response = match (error, subscribe, signal_entry, publish) { | ||
(None, None, None, None) => ResponseType::Barrier, | ||
(Some(error), None, None, None) => { | ||
//Hack to remove extra escape characters | ||
// Hack to remove extra escape characters | ||
let error = serde_json::from_str(&error).expect("JSON Deserialization"); | ||
ResponseType::Error(error) | ||
} | ||
(None, Some(msg), None, None) => { | ||
//Hack to remove extra escape characters | ||
let msg = serde_json::from_str(&msg).expect("JSON Deserialization"); | ||
ResponseType::Subscribe(msg) | ||
Comment on lines
-62
to
-64
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. With the suggestion above, we would only do the following patch here: - //Hack to remove extra escape characters
- let msg = serde_json::from_str(&msg).expect("JSON Deserialization");
ResponseType::Subscribe(msg) 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.
Could you share the benefits you see?
For what it's worth, it looks like there is a type issue in the API that makes it confusing, but we don't have to leak this on the user side: |
||
// the Subscribe payload is a json encoded string, so we need to deserialize it | ||
laurentsenta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let payload = serde_json::from_str(&msg).expect("JSON Deserialization"); | ||
ResponseType::Subscribe(payload) | ||
} | ||
(None, None, Some(signal), None) => ResponseType::SignalEntry { seq: signal.seq }, | ||
(None, None, None, Some(publish)) => ResponseType::Publish { seq: publish.seq }, | ||
|
@@ -89,7 +89,8 @@ mod tests { | |
|
||
#[test] | ||
fn serde_test() { | ||
let raw_response = "{\"id\":\"0\",\"error\":\"\",\"subscribe\":\"\",\"publish\":{\"seq\":1},\"signal_entry\":null}"; | ||
let raw_response = | ||
"{\"id\":\"0\",\"error\":\"\",\"subscribe\":\"\",\"publish\":{\"seq\":1},\"signal_entry\":null}"; | ||
|
||
let response: RawResponse = serde_json::from_str(raw_response).unwrap(); | ||
|
||
|
@@ -103,4 +104,23 @@ mod tests { | |
response | ||
); | ||
} | ||
#[test] | ||
fn serde_test_complex_subscribe() { | ||
let raw_response = "{\"id\":\"1\",\"error\":\"\",\"subscribe\":\"{\\\"Addrs\\\":[\\\"/ip4/16.3.0.3/tcp/45369\\\"],\\\"ID\\\":\\\"QmbSLMEMackm7vHiUGMB2EFAPbzeJNpeB9yTpzYKoojDWc\\\"}\"}"; | ||
|
||
let response: RawResponse = serde_json::from_str(raw_response).unwrap(); | ||
|
||
let response: Response = response.into(); | ||
|
||
assert_eq!( | ||
Response { | ||
id: "1".to_owned(), | ||
response: ResponseType::Subscribe(serde_json::json!({ | ||
"Addrs": ["/ip4/16.3.0.3/tcp/45369"], | ||
"ID": "QmbSLMEMackm7vHiUGMB2EFAPbzeJNpeB9yTpzYKoojDWc" | ||
})) | ||
}, | ||
response | ||
); | ||
} | ||
} |
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.
Thank you for providing an example along with your patch. This is great engineering.
Given that the old
fn example
is quite simple, I would be fine forfn publish_subscribe
to replace it. Just an idea to reduce complexity. Not a strong opinion.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.
Thanks for the feedback @mxinden!
If you don't mind I would keep it, not as a test case, I agree it's probably not super useful, but as an example for future users. I like the idea of having a bare minimum "here is how you write a test that passes in testground", before moving into more complex synchronization patterns.