Fix: use json payloads

[laurentsenta]

Fixes #33,

• Uses a json payload for publish / subscribes,
• introduce publish-subscribe example, which demonstrate leader / follower pattern with JSON payloads.

[laurentsenta]

(Sorry for the noise, please ignore for now, I'll make sure the interop test are passing before review)

[mxinden]

@laurentsenta thanks for working on this. Let me know in case you need any help.

[mxinden]

Thinking about this some more and running the failing tests myself, I think instead of serializing and deserializing a JSON structure in sdk-rust, we should leave it up to the user of sdk-rust what to send, and instead do the serialization and deserialization in the concrete test plans.

The benefit is, that a user can publish any format and is not bound to JSON.

@laurentsenta what do you think?

[mxinden]

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)

[laurentsenta]

Thinking about this some more and running the failing tests myself, I think instead of serializing and deserializing a JSON structure in sdk-rust, we should leave it up to the user of sdk-rust what to send, and instead do the serialization and deserialization in the concrete test plans.

The benefit is, that a user can publish any format and is not bound to JSON.

Could you share the benefits you see?
My thinking might be "tainted" by the go-sdk but I think defaulting to JSON is a good thing:

• Test implementers don't have to think about serialization schemes,
• SDKs can automate encoding/decoding (the go SDK uses reflection to convert JSON payloads from/to Go structures),
• It's hard to create a JSON payload that cannot be used from another language
  • I'm seeing this now with the libp2p/ping go test, they publish a JSON we can use in Rust without much effort. If they had come up with their scheme, best case scenario we would use JSON, worst case we now have to support protocol buffer or a custom encoding.

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:
https://github.com/testground/sync-service/blob/d3e9e3e56403ed994f350942813d3d8aa746debc/types.go#L21
https://github.com/testground/sync-service/blob/d3e9e3e56403ed994f350942813d3d8aa746debc/types.go#L69
(string response)

[mxinden]

@SionoiS you probably have an opinion here?

[SionoiS]

The problem was that serde can't deal with nested json strings AFAIK.

Either we don't deserialize OR we write something custom.

Writing something could allow to remove the ugly RawResponse -> Response conversion.

sdk-rust/src/responses.rs

Line 44 in d6af289

impl From<RawResponse> for Response {

@laurentsenta is going in the right direction I think. Using serde_json::Value allow users to use their own types.

edit: I assumed Json will always be used in Testground.

[mxinden]

Thanks @laurentsenta and @SionoiS for the quick responses.

👍 for proceeding in the direction this pull request is heading.

[mxinden]

Thank you @laurentsenta for the fix.

This looks good to me overall.

I am not sure why CI is failing, since the container reports a success.

[mxinden]

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 for fn publish_subscribe to replace it. Just an idea to reduce complexity. Not a strong opinion.

[laurentsenta]

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.

[laurentsenta]

@mxinden that's exciting, we might have a minimal example of testground going unstable. Investigating

[laurentsenta]

@mxinden I might have a patch, but it'll take some time to finalize (testground/testground#1407) do you think we could merge on a green pass (since the reason why the test fail is testground related)?

[mxinden]

🙏 Thanks @laurentsenta for tackling this. Moving forward sounds good to me.