sdk: allow types to abstract over encoding

[djc]

What was changed

So far, the sdk and sdk-core-protos crates have specialized JSON encoding of payloads with the AsJsonPayloadExt and FromJsonPayloadExt traits. Some implementations in the Rust SDK have effectively encoded that in the API so that it is harder or non-intuitive to use any other encoding. This PR interposes Encoder and Decoder traits so each type that must be converted into/from a Payload has to explicitly opt in to this behavior, and gets to chose which encoding is used. A Json encoding is provided that makes it easy to regain the previous behavior of encoding as JSON.

Why?

The Temporal infrastructure/gRPC protocol allows encoding in a number of different ways and this is exposed to the other SDKs. However, the Rust SDK so far has facilitated JSON to the point where it is hard to use anything else. Given Rust's type system, we can do better and make it statically required for types to opt in to a particular encoding, and for the SDK to rely on these implementations to encode to and decode from the Payload type.

This will also help users of the Rust SDK to implement a type system boundary that makes sure any types used in Temporal workflows/activities must be encrypted, because opting into an encoding is required.

Some previous discussion in this Slack thread: https://temporalio.slack.com/archives/C02MTL0GEKH/p1708568506832879.

Checklist

1. How was this tested: it compiles and passes the tests
2. Any docs updates needed? I don't think so?

[djc]

This needed to move to prevent orphan issues with the ActivityOutput trait impls. Seemed like an okay price to pay if it makes the rest of this work out...

[djc]

(The previous run of the integration tests seemed to pass, and they passed locally, too.)

[Sushisource]

Sorry, I swear I had already reviewed this a while ago but I think GitHub might have been having problems at the time and ate my review.

Thanks for doing this! I think we just need a little more flexibility, even in the SDK's prerelease state. If we have the right structure here (even incomplete) a lot of this can be retained and used for future releases.

[Sushisource]

I don't think we want encoder to be an associated type of this trait (same for decoder in from).

Reason being it locks types to a particular encoder forever and the user can't change it. EX: Later you define PayloadExt for String as using the Json encoder - but what if someone wants to encode strings using a binary format?

Thus I think we need to have an overall DataConverter trait like the other SDKs. For example in .NET: https://github.com/temporalio/sdk-dotnet/blob/e11dfd577599aeed57671657b2f18df7a21d0245/src/Temporalio/Converters/DataConverter.cs#L9

Here a user can assemble the ser[de] portion (Payload converter) and the bytes-to-bytes codec.

Then users provide a DataConverter to the worker. We should implement To/FromPayload automatically for anything serde serializable in conjunction with the user simply providing their serde backing (ex serde_json) for the actual format.

[djc]

Reason being it locks types to a particular encoder forever and the user can't change it. EX: Later you define PayloadExt for String as using the Json encoder - but what if someone wants to encode strings using a binary format?

Downstream crates will be unable to implement traits this crate provides for types from std anyway, so I'm not sure how you see this working.

Also, I feel like the ideal developer experience would be that workflows/activities do not need to manually encode/decode, which kind of requires (as far as I can tell) that the types bind themselves to an encoder/decoder. So this is trying to maintain the (desirable, IMO) status quo that the choice of encoding is bound to the type (currently, JSON via serde), rather than relying on an explicit conversion to Payload.

The latter is also a valid design choice but IMO less attractive. For user-defined types, it seems unlikely that callers want to encode with more than one different schemes (and if they still do, handling that via a newtype seems straightforward enough).

Then users provide a DataConverter to the worker. We should implement To/FromPayload automatically for anything serde serializable in conjunction with the user simply providing their serde backing (ex serde_json) for the actual format.

To me, specializing all of the conversions on serde also seems potentially problematic. What if a downstream user prefers to use rkyv (for its superfast direct to memory deserialization), or abomonation, or nanoserde? The setup proposed in this PR makes the serde case really pretty easy without imposing that everything is serde all the time.

[Sushisource]

Yeah, they won't need to manually encode or decode - the point is that they provide a DataConverter to the worker, and all Payloads go through this. The type of encoding shouldn't be bound to the type being converted from/to, it should be bound to the DataConverter. Then anything that is serializable and being used as input / a return value gets run through the converter on the way out/in.

As for the problem with downstream crates not being able to implement To/FromPayload directly & not forcing always using serde... I can think of a few options.

One of them is to just handle all this at runtime, allow returning anything that can be turned into a Box<dyn Any>, then do casting and the checking to see if something is serializable at runtime.

Second option is to have a special case for serde, and everything else just needs one extra line at the end of the workflow: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=76d0eafab777cfb157e00684a01c7493 -- I like this option a lot because the vast majority of people are going to be using / happy with serde, but it gives a way out for the people who want to do something special.

The third option, which I didn't get all the way compiling because it's pretty tricky and I didn't want to spend forever on this, but probably does work long run, is to parameterize workflow functions over the user-provided serializer/dataconverter. IE: You have something like:

fn my_workflow_fn<S: TemporalSerializer>() -> impl CanBeSerializedWith<S> {}

In the real future SDK, they wouldn't have to write that generic in the workflow function because we'd handle it with a macro - but they would get compile time breaks if the thing they return from a workflow can't be serialized with the serializer they provided when constructing the worker. I have a feeling this approach is overkill though.

[djc]

Will think about this some more, just asking for some clarification:

Yeah, they won't need to manually encode or decode - the point is that they provide a DataConverter to the worker, and all Payloads go through this. The type of encoding shouldn't be bound to the type being converted from/to, it should be bound to the DataConverter. Then anything that is serializable and being used as input / a return value gets run through the converter on the way out/in.

Do you mean that the Worker should some kind of Box<dyn DataConverter> which should then try to convert the input and output types to Payload? IMO using trait objects like this isn't really playing to Rust's strengths, because it would not allow compile-time checking that the Worker's DataConverter can handle the type (unless you're okay with the worker taking a type parameter for the DataConverter?).

In the real future SDK, they wouldn't have to write that generic in the workflow function because we'd handle it with a macro - but they would get compile time breaks if the thing they return from a workflow can't be serialized with the serializer they provided when constructing the worker. I have a feeling this approach is overkill though.

I'd like to soon start working on a workflow trait and an activity trait, because stringly typing workflow and actitivy types feels very error-prone. Can you sketch what you have in mind for how the macro would look?

[Sushisource]

Yes, so that's why I like option 2 - you at least get compile time guarantees for serde which works for the vast majority of usecases. Option 3 does exactly what you said, parameterize the worker over the converter, and then in turn the workflows over the converter. I think that'd probably be a step too far. Compile time safe, but, annoyingly intrusive generics.

There is a PR here where I show (mostly) my ideas about the future workflow API #550 - I definitely landed on wanting to use procmacros to help make things nice. I'm a little hesitant to encourage contribution here though because it's quite a large chunk of work, and we have a lot of lessons learned from other languages about what makes sense to do. Of course, I can see how annoying it is to say "don't do this we want to do it in house, but also we're not prioritizing it yet". So, if you're really into it, just be aware I might be slow to review and of course it is likely subject to change again later.

[djc]

I'm leaving the company where I was working with Temporal, so I probably won't be pushing this forward anymore. Feel free to reuse any of the code here for future changes.