Add transfer type payload conversion hooks#795
Conversation
2c28d33 to
d1228be
Compare
tconley1428
changed the title
| $"{attr.ConverterType}."); | ||
| } | ||
|
|
||
| if (Activator.CreateInstance(attr.ConverterType) is not ITemporalTransferTypeConverter converter) |
There was a problem hiding this comment.
While you check a few cases above, there are other ones, such as:
- Type could be abstract e.g.
attr.ConverterType.IsAbstract - Type constructor could have parameters e.g.
!attr.ConverterType.IsValueType && attr.ConverterType.GetConstructor(Type.EmptyTypes) == null; all value types (e.g.structhave a default parameterless constructor, so you can ignore those. - Type constructor must be public;
GetConstructorwill returnnullif not public instance constructor.
| $"{attr.ConverterType}."); | ||
| } | ||
|
|
||
| if (Activator.CreateInstance(attr.ConverterType) is not ITemporalTransferTypeConverter converter) |
There was a problem hiding this comment.
Activator.CreateInstance can throw exceptions, especially for all of the cases you are checking. But it can also throw exceptions due to execution problems, such as the target type constructor raises an exception, the static constructor for the type raises an exception, a dependent library cannot be loaded, etc. All different types of exceptions you cannot anticipate the types of (or you can, there's just a lot of them). Not sure if you want to let those bubble out as-is, which would probably be fine to do. Just bringing awareness.
| /// This API is experimental and may change in a future release. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = true)] | ||
| public sealed class TemporalTransferTypeAttribute : Attribute |
There was a problem hiding this comment.
Rename to TemporalTransferTypeConverterAttribute to match .NET conventions.
| /// <see cref="ITemporalTransferTypeConverter"/>. | ||
| /// This API is experimental and may change in a future release. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = true)] |
There was a problem hiding this comment.
I think we should disallow inheritance (set Inherited = false). The reason being that if someone gets clever to create a base type with this attribute and a derived type without the attribute, then the converter on the base type better know how to dynamically inspect the transfer type to turn it back into the model type; seems like something easy to stumble into. With Inherited = false, each derivation needs to apply the attribute and declare which converter understands it; if the converter is polymorphic aware, it can be explicitly applied to each derivation.
I'd add a bunch of tests around this.
|
|
||
| private static ITemporalTransferTypeConverter? CreateConverter(Type type) | ||
| { | ||
| var attr = type.GetCustomAttribute<TemporalTransferTypeAttribute>(inherit: true); |
There was a problem hiding this comment.
Flip to false to match https://github.com/temporalio/sdk-dotnet/pull/795/changes#r3641267623. I think we want the runtime type to actually declare it's converter rather than getting something via inheritance.
| /// <summary> | ||
| /// Gets the payload converter. | ||
| /// </summary> | ||
| public IPayloadConverter PayloadConverter |
There was a problem hiding this comment.
I would not do this since it breaks the record semantics. I think if you construct two separate DataConverter instances with the same payload converter, failure converter, and payload codec, before this change they would appear to be equal but after this change they would appear to be unequal.
This becomes acute for plugins, which could try to wrap a payload converter from the DataConverter given to it with it's own. That'll cause an old TemporalTransferTypePayloadConverter to be sandwiched between a user-defined and a plugin-defined payload converter, and all wrapped by another TemporalTransferTypePayloadConverter payload converter. Maybe not detrimental, but definitely unexpected.
I would apply this at TemporalClient.ConnectAsync just after plugins do their adjustments and before we actually consume the options within the client infrastructure. The other place that DataConverter is provided is via HeartbeatAsyncActivityInput, but I'm not sure we really need to wrap that there. If just for system Nexus operations, probably not, but if as a general transfer type mechanism, then probably yes.
| public IPayloadConverter PayloadConverter | ||
| { | ||
| get => payloadConverter; | ||
| init => payloadConverter = TemporalTransferTypePayloadConverter.Wrap(value); |
There was a problem hiding this comment.
I don't think the init is necessary; everyone is already constructing the DataConverter by passing the payload converter via the constructor.
2 participants
Adds experimental SDK payload-converter support for values and target types marked with
TemporalTransferTypeAttribute.The attribute points at an
ITemporalTransferTypeConverterimplementation. The converter supplies theTransferTypehanded to the configured payload converter, converts user values into that representation for encoding, and reconstructs user values from it for decoding. General transfer type hooks intentionally do not receive a payload converter.This keeps normal SDK payload-converter behavior in one place, including codec handling and serialization-context propagation.
This change:
DataConverter, includingwith { PayloadConverter = ... }TemporalTransferTypeAttributeandITemporalTransferTypeConverterAPIs