From 059525965af4b4b28ca5c6bb25cf24e8c7d72b28 Mon Sep 17 00:00:00 2001 From: Tim Conley Date: Tue, 21 Jul 2026 09:12:57 -0700 Subject: [PATCH 1/3] Add data model payload conversion hooks --- CHANGELOG.md | 4 + src/Temporalio/Converters/DataConverter.cs | 12 +++ .../Converters/ITemporalDataModelConverter.cs | 31 ++++++ .../Converters/TemporalDataModelAttribute.cs | 28 +++++ .../TemporalDataModelPayloadConverter.cs | 102 ++++++++++++++++++ .../Converters/DataConverterTests.cs | 21 +++- .../Converters/PayloadConverterTests.cs | 83 +++++++++++++- .../Worker/WorkflowWorkerTests.cs | 2 +- 8 files changed, 277 insertions(+), 6 deletions(-) create mode 100644 src/Temporalio/Converters/ITemporalDataModelConverter.cs create mode 100644 src/Temporalio/Converters/TemporalDataModelAttribute.cs create mode 100644 src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 772c35e0..0579be77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,10 @@ to docs, or any other relevant information. you disable size enforcement by setting `DisablePayloadErrorLimit` to `true` on the worker. ### Added +- Added SDK payload converter support for values and target types that expose + Temporal data-model conversion hooks. This lets hook-aware types delegate + their wire representation to the configured payload converter, preserving SDK + behavior such as serialization contexts for nested payload fields. - Added the experimental `TemporalWorkerOptions.PatchActivationCallback`, allowing workers to decide whether a first non-replay `Workflow.Patched` call should activate a patch during rolling deployments. diff --git a/src/Temporalio/Converters/DataConverter.cs b/src/Temporalio/Converters/DataConverter.cs index 0bc95b7c..25e183db 100644 --- a/src/Temporalio/Converters/DataConverter.cs +++ b/src/Temporalio/Converters/DataConverter.cs @@ -12,6 +12,18 @@ public record DataConverter( IPayloadCodec? PayloadCodec = null) : IWithSerializationContext { + private readonly IPayloadConverter payloadConverter = + TemporalDataModelPayloadConverter.Wrap(PayloadConverter); + + /// + /// Gets the payload converter. + /// + public IPayloadConverter PayloadConverter + { + get => payloadConverter; + init => payloadConverter = TemporalDataModelPayloadConverter.Wrap(value); + } + /// /// Gets default data converter instance. /// diff --git a/src/Temporalio/Converters/ITemporalDataModelConverter.cs b/src/Temporalio/Converters/ITemporalDataModelConverter.cs new file mode 100644 index 00000000..bc2d785c --- /dev/null +++ b/src/Temporalio/Converters/ITemporalDataModelConverter.cs @@ -0,0 +1,31 @@ +using System; + +namespace Temporalio.Converters +{ + /// + /// Converter for a type marked with . + /// + public interface ITemporalDataModelConverter + { + /// + /// Gets the data-model type handed to or read from the payload converter. + /// + Type DataModelType { get; } + + /// + /// Convert a value to the data-model value that should be passed to the payload converter. + /// + /// Value to convert. + /// Payload converter to use for nested payload fields. + /// Data-model value to pass to the payload converter. + object? ToDataModel(object? value, IPayloadConverter payloadConverter); + + /// + /// Convert a data-model value from the payload converter to the marked type. + /// + /// Data-model value returned by the payload converter. + /// Payload converter to use for nested payload fields. + /// Converted value. + object? FromDataModel(object? dataModel, IPayloadConverter payloadConverter); + } +} diff --git a/src/Temporalio/Converters/TemporalDataModelAttribute.cs b/src/Temporalio/Converters/TemporalDataModelAttribute.cs new file mode 100644 index 00000000..4e799ddb --- /dev/null +++ b/src/Temporalio/Converters/TemporalDataModelAttribute.cs @@ -0,0 +1,28 @@ +using System; + +namespace Temporalio.Converters +{ + /// + /// Marks a type as converting to and from another data-model type before payload conversion. + /// + /// + /// This is used by the SDK payload converter to delegate hook-aware values to the configured + /// payload converter using the data-model representation provided by + /// . + /// + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = true)] + public sealed class TemporalDataModelAttribute : Attribute + { + /// + /// Initializes a new instance of the class. + /// + /// Converter type implementing + /// . + public TemporalDataModelAttribute(Type converterType) => ConverterType = converterType; + + /// + /// Gets the converter type. + /// + public Type ConverterType { get; } + } +} diff --git a/src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs b/src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs new file mode 100644 index 00000000..ca82573a --- /dev/null +++ b/src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs @@ -0,0 +1,102 @@ +using System; +using System.Collections.Concurrent; +using System.Reflection; +using Temporalio.Api.Common.V1; + +namespace Temporalio.Converters +{ + /// + /// Payload converter wrapper that applies Temporal data-model hooks. + /// + internal sealed class TemporalDataModelPayloadConverter : + IPayloadConverter, + IWithSerializationContext + { + private static readonly ConcurrentDictionary Converters = new(); + private readonly IPayloadConverter inner; + + private TemporalDataModelPayloadConverter(IPayloadConverter inner) => this.inner = inner; + + /// + /// Wrap a payload converter unless it is already wrapped. + /// + /// Payload converter to wrap. + /// Wrapped payload converter. + public static IPayloadConverter Wrap(IPayloadConverter payloadConverter) => + payloadConverter is TemporalDataModelPayloadConverter ? + payloadConverter : new TemporalDataModelPayloadConverter(payloadConverter); + + /// + public Payload ToPayload(object? value) + { + var converter = value == null ? null : Converters.GetOrAdd(value.GetType(), CreateConverter); + if (converter != null) + { + value = converter.ToDataModel(value, inner); + } + return inner.ToPayload(value); + } + + /// + public object? ToValue(Payload payload, Type type) + { + var converter = Converters.GetOrAdd(type, CreateConverter); + if (converter == null) + { + return inner.ToValue(payload, type); + } + + var dataModelValue = inner.ToValue(payload, converter.DataModelType); + return converter.FromDataModel(dataModelValue, inner); + } + + /// + public IPayloadConverter WithSerializationContext(ISerializationContext context) + { + if (inner is not IWithSerializationContext withContext) + { + return this; + } + + var contextInner = withContext.WithSerializationContext(context); + return ReferenceEquals(contextInner, inner) ? this : new TemporalDataModelPayloadConverter(contextInner); + } + + private static ITemporalDataModelConverter? CreateConverter(Type type) + { + var attr = type.GetCustomAttribute(inherit: true); + if (attr == null) + { + return null; + } + + if (!typeof(ITemporalDataModelConverter).IsAssignableFrom(attr.ConverterType)) + { + throw new InvalidOperationException( + $"Type {type} has a Temporal data-model converter type " + + $"{attr.ConverterType} that does not implement {nameof(ITemporalDataModelConverter)}."); + } + if (attr.ConverterType.ContainsGenericParameters) + { + throw new InvalidOperationException( + $"Type {type} has an open generic Temporal data-model converter type " + + $"{attr.ConverterType}."); + } + + if (Activator.CreateInstance(attr.ConverterType) is not ITemporalDataModelConverter converter) + { + throw new InvalidOperationException( + $"Type {type} has a Temporal data-model converter type " + + $"{attr.ConverterType} that could not be instantiated."); + } + if (converter.DataModelType == null) + { + throw new InvalidOperationException( + $"Type {type} has a Temporal data-model converter type " + + $"{attr.ConverterType} with a null data-model type."); + } + + return converter; + } + } +} diff --git a/tests/Temporalio.Tests/Converters/DataConverterTests.cs b/tests/Temporalio.Tests/Converters/DataConverterTests.cs index 0eeec464..15557ff4 100644 --- a/tests/Temporalio.Tests/Converters/DataConverterTests.cs +++ b/tests/Temporalio.Tests/Converters/DataConverterTests.cs @@ -1,6 +1,7 @@ namespace Temporalio.Tests.Converters; using System; +using Google.Protobuf; using Temporalio.Api.Common.V1; using Temporalio.Converters; using Xunit; @@ -16,17 +17,29 @@ public DataConverterTests(ITestOutputHelper output) [Fact] public void NewDataConverter_WithPayloadConverter_ProperlyInitializes() { + var payloadConverter = new MyPayloadConverter(); var newConverter = DataConverter.Default with { - PayloadConverter = new MyPayloadConverter(), + PayloadConverter = payloadConverter, }; - Assert.IsType(newConverter.PayloadConverter); + Assert.NotSame(payloadConverter, newConverter.PayloadConverter); + Assert.Equal( + "payload", + newConverter.PayloadConverter.ToValue( + newConverter.PayloadConverter.ToPayload("payload"), typeof(string))); } public class MyPayloadConverter : IPayloadConverter { - public Payload ToPayload(object? value) => throw new NotImplementedException(); + public Payload ToPayload(object? value) => new() + { + Metadata = + { + ["encoding"] = ByteString.CopyFromUtf8("test/plain"), + }, + Data = ByteString.CopyFromUtf8((string)value!), + }; - public object? ToValue(Payload payload, Type type) => throw new NotImplementedException(); + public object? ToValue(Payload payload, Type type) => payload.Data.ToStringUtf8(); } } diff --git a/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs b/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs index 931f0acc..25666b3d 100644 --- a/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs +++ b/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs @@ -107,6 +107,23 @@ public void ToValue_WrongProtoType_Fails() Assert.Contains(ActivityType.Descriptor.FullName, e.Message); } + [Fact] + public void ToPayload_DataModelHooks_Succeed() + { + var dataConverter = new DataConverter( + new ContextStringPayloadConverter(), + new DefaultFailureConverter()).WithSerializationContext( + new ISerializationContext.Workflow("default", "workflow-id")); + var value = new DataModelHookValue("payload-value"); + + var payload = dataConverter.PayloadConverter.ToPayload(value); + var memo = (Memo)new DefaultPayloadConverter().ToValue(payload, typeof(Memo))!; + Assert.Equal("workflow-id:payload-value", memo.Fields["value"].Data.ToStringUtf8()); + Assert.Equal( + value, + dataConverter.PayloadConverter.ToValue(payload, typeof(DataModelHookValue))); + } + private static Payload AssertPayload( object? value, string expectedEncoding, @@ -160,7 +177,7 @@ public class NoJsonProtoPayloadConverter : DefaultPayloadConverter { public NoJsonProtoPayloadConverter() : base( - ((DefaultPayloadConverter)DataConverter.Default.PayloadConverter). + new DefaultPayloadConverter(). EncodingConverters.Where(c => c is not JsonProtoConverter).ToArray()) { } @@ -174,4 +191,68 @@ public CamelCasePayloadConverter() { } } + + [TemporalDataModel(typeof(DataModelHookValueConverter))] + public sealed record DataModelHookValue(string Value); + + public class DataModelHookValueConverter : ITemporalDataModelConverter + { + public Type DataModelType => typeof(Memo); + + public object ToDataModel(object? value, IPayloadConverter payloadConverter) => + new Memo + { + Fields = + { + ["value"] = payloadConverter.ToPayload(((DataModelHookValue)value!).Value), + }, + }; + + public object FromDataModel(object? dataModel, IPayloadConverter payloadConverter) => + new DataModelHookValue( + (string)payloadConverter.ToValue(((Memo)dataModel!).Fields["value"], typeof(string))!); + } + + public class ContextStringPayloadConverter : + IPayloadConverter, + IWithSerializationContext + { + private static readonly DefaultPayloadConverter FallbackPayloadConverter = + new DefaultPayloadConverter(); + + private readonly string? workflowId; + + public ContextStringPayloadConverter(string? workflowId = null) => + this.workflowId = workflowId; + + public Payload ToPayload(object? value) + { + if (value is string str) + { + return new() + { + Metadata = + { + ["encoding"] = ByteString.CopyFromUtf8("test/context-string"), + }, + Data = ByteString.CopyFromUtf8($"{workflowId}:{str}"), + }; + } + return FallbackPayloadConverter.ToPayload(value); + } + + public object? ToValue(Payload payload, Type type) + { + if (type == typeof(string) && + payload.Metadata["encoding"].ToStringUtf8() == "test/context-string") + { + var encoded = payload.Data.ToStringUtf8(); + return encoded[(encoded.IndexOf(':') + 1)..]; + } + return FallbackPayloadConverter.ToValue(payload, type); + } + + public IPayloadConverter WithSerializationContext(ISerializationContext context) => + new ContextStringPayloadConverter(((ISerializationContext.IHasWorkflow)context).WorkflowId); + } } diff --git a/tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs b/tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs index ae018eda..745d8ae1 100644 --- a/tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs +++ b/tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs @@ -7876,7 +7876,7 @@ public async Task ExecuteWorkflowAsync_ConverterContext_ProperlyAvailable() // Context-aware data converter and client var dataConverter = new DataConverter( PayloadConverter: new DefaultPayloadConverter( - ((DefaultPayloadConverter)Client.Options.DataConverter.PayloadConverter).EncodingConverters.Select(e => + new DefaultPayloadConverter().EncodingConverters.Select(e => e is JsonPlainConverter ? new ContextJsonPlainConverter() : e).ToArray()), FailureConverter: new ContextFailureConverter(), PayloadCodec: new ContextPayloadCodec()); From ffbb23c4806a89c26ca83bda6ce4f07fdd385498 Mon Sep 17 00:00:00 2001 From: Tim Conley Date: Wed, 22 Jul 2026 11:51:54 -0700 Subject: [PATCH 2/3] Rename data model hooks to transfer types --- CHANGELOG.md | 6 +-- src/Temporalio/Converters/DataConverter.cs | 4 +- .../Converters/ITemporalDataModelConverter.cs | 31 -------------- .../ITemporalTransferTypeConverter.cs | 32 ++++++++++++++ ...te.cs => TemporalTransferTypeAttribute.cs} | 15 +++---- ...> TemporalTransferTypePayloadConverter.cs} | 42 +++++++++---------- .../Converters/PayloadConverterTests.cs | 31 +++++--------- 7 files changed, 77 insertions(+), 84 deletions(-) delete mode 100644 src/Temporalio/Converters/ITemporalDataModelConverter.cs create mode 100644 src/Temporalio/Converters/ITemporalTransferTypeConverter.cs rename src/Temporalio/Converters/{TemporalDataModelAttribute.cs => TemporalTransferTypeAttribute.cs} (53%) rename src/Temporalio/Converters/{TemporalDataModelPayloadConverter.cs => TemporalTransferTypePayloadConverter.cs} (62%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0579be77..55b4fd58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,10 +32,10 @@ to docs, or any other relevant information. you disable size enforcement by setting `DisablePayloadErrorLimit` to `true` on the worker. ### Added -- Added SDK payload converter support for values and target types that expose - Temporal data-model conversion hooks. This lets hook-aware types delegate +- Added experimental SDK payload converter support for values and target types that expose + Temporal transfer type conversion hooks. This lets hook-aware types delegate their wire representation to the configured payload converter, preserving SDK - behavior such as serialization contexts for nested payload fields. + behavior such as serialization contexts. - Added the experimental `TemporalWorkerOptions.PatchActivationCallback`, allowing workers to decide whether a first non-replay `Workflow.Patched` call should activate a patch during rolling deployments. diff --git a/src/Temporalio/Converters/DataConverter.cs b/src/Temporalio/Converters/DataConverter.cs index 25e183db..9f265ae1 100644 --- a/src/Temporalio/Converters/DataConverter.cs +++ b/src/Temporalio/Converters/DataConverter.cs @@ -13,7 +13,7 @@ public record DataConverter( : IWithSerializationContext { private readonly IPayloadConverter payloadConverter = - TemporalDataModelPayloadConverter.Wrap(PayloadConverter); + TemporalTransferTypePayloadConverter.Wrap(PayloadConverter); /// /// Gets the payload converter. @@ -21,7 +21,7 @@ public record DataConverter( public IPayloadConverter PayloadConverter { get => payloadConverter; - init => payloadConverter = TemporalDataModelPayloadConverter.Wrap(value); + init => payloadConverter = TemporalTransferTypePayloadConverter.Wrap(value); } /// diff --git a/src/Temporalio/Converters/ITemporalDataModelConverter.cs b/src/Temporalio/Converters/ITemporalDataModelConverter.cs deleted file mode 100644 index bc2d785c..00000000 --- a/src/Temporalio/Converters/ITemporalDataModelConverter.cs +++ /dev/null @@ -1,31 +0,0 @@ -using System; - -namespace Temporalio.Converters -{ - /// - /// Converter for a type marked with . - /// - public interface ITemporalDataModelConverter - { - /// - /// Gets the data-model type handed to or read from the payload converter. - /// - Type DataModelType { get; } - - /// - /// Convert a value to the data-model value that should be passed to the payload converter. - /// - /// Value to convert. - /// Payload converter to use for nested payload fields. - /// Data-model value to pass to the payload converter. - object? ToDataModel(object? value, IPayloadConverter payloadConverter); - - /// - /// Convert a data-model value from the payload converter to the marked type. - /// - /// Data-model value returned by the payload converter. - /// Payload converter to use for nested payload fields. - /// Converted value. - object? FromDataModel(object? dataModel, IPayloadConverter payloadConverter); - } -} diff --git a/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs b/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs new file mode 100644 index 00000000..da219be3 --- /dev/null +++ b/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs @@ -0,0 +1,32 @@ +using System; + +namespace Temporalio.Converters +{ + /// + /// Converter for a type marked with . + /// + /// + /// This API is experimental and may change in a future release. + /// + public interface ITemporalTransferTypeConverter + { + /// + /// Gets the transfer type handed to or read from the payload converter. + /// + Type TransferType { get; } + + /// + /// Convert a value to the transfer type value that should be passed to the payload converter. + /// + /// Value to convert. + /// Transfer type value to pass to the payload converter. + object? ToTransferType(object? value); + + /// + /// Convert a transfer type value from the payload converter to the marked type. + /// + /// Transfer type value returned by the payload converter. + /// Converted value. + object? FromTransferType(object? transferType); + } +} diff --git a/src/Temporalio/Converters/TemporalDataModelAttribute.cs b/src/Temporalio/Converters/TemporalTransferTypeAttribute.cs similarity index 53% rename from src/Temporalio/Converters/TemporalDataModelAttribute.cs rename to src/Temporalio/Converters/TemporalTransferTypeAttribute.cs index 4e799ddb..d63e76a7 100644 --- a/src/Temporalio/Converters/TemporalDataModelAttribute.cs +++ b/src/Temporalio/Converters/TemporalTransferTypeAttribute.cs @@ -3,22 +3,23 @@ namespace Temporalio.Converters { /// - /// Marks a type as converting to and from another data-model type before payload conversion. + /// Marks a type as converting to and from a transfer type before payload conversion. /// /// /// This is used by the SDK payload converter to delegate hook-aware values to the configured - /// payload converter using the data-model representation provided by - /// . + /// payload converter using the transfer type representation provided by + /// . + /// This API is experimental and may change in a future release. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = true)] - public sealed class TemporalDataModelAttribute : Attribute + public sealed class TemporalTransferTypeAttribute : Attribute { /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the class. /// /// Converter type implementing - /// . - public TemporalDataModelAttribute(Type converterType) => ConverterType = converterType; + /// . + public TemporalTransferTypeAttribute(Type converterType) => ConverterType = converterType; /// /// Gets the converter type. diff --git a/src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs b/src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs similarity index 62% rename from src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs rename to src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs index ca82573a..803cc3eb 100644 --- a/src/Temporalio/Converters/TemporalDataModelPayloadConverter.cs +++ b/src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs @@ -6,16 +6,16 @@ namespace Temporalio.Converters { /// - /// Payload converter wrapper that applies Temporal data-model hooks. + /// Payload converter wrapper that applies Temporal transfer type hooks. /// - internal sealed class TemporalDataModelPayloadConverter : + internal sealed class TemporalTransferTypePayloadConverter : IPayloadConverter, IWithSerializationContext { - private static readonly ConcurrentDictionary Converters = new(); + private static readonly ConcurrentDictionary Converters = new(); private readonly IPayloadConverter inner; - private TemporalDataModelPayloadConverter(IPayloadConverter inner) => this.inner = inner; + private TemporalTransferTypePayloadConverter(IPayloadConverter inner) => this.inner = inner; /// /// Wrap a payload converter unless it is already wrapped. @@ -23,8 +23,8 @@ internal sealed class TemporalDataModelPayloadConverter : /// Payload converter to wrap. /// Wrapped payload converter. public static IPayloadConverter Wrap(IPayloadConverter payloadConverter) => - payloadConverter is TemporalDataModelPayloadConverter ? - payloadConverter : new TemporalDataModelPayloadConverter(payloadConverter); + payloadConverter is TemporalTransferTypePayloadConverter ? + payloadConverter : new TemporalTransferTypePayloadConverter(payloadConverter); /// public Payload ToPayload(object? value) @@ -32,7 +32,7 @@ public Payload ToPayload(object? value) var converter = value == null ? null : Converters.GetOrAdd(value.GetType(), CreateConverter); if (converter != null) { - value = converter.ToDataModel(value, inner); + value = converter.ToTransferType(value); } return inner.ToPayload(value); } @@ -46,8 +46,8 @@ public Payload ToPayload(object? value) return inner.ToValue(payload, type); } - var dataModelValue = inner.ToValue(payload, converter.DataModelType); - return converter.FromDataModel(dataModelValue, inner); + var transferTypeValue = inner.ToValue(payload, converter.TransferType); + return converter.FromTransferType(transferTypeValue); } /// @@ -59,41 +59,41 @@ public IPayloadConverter WithSerializationContext(ISerializationContext context) } var contextInner = withContext.WithSerializationContext(context); - return ReferenceEquals(contextInner, inner) ? this : new TemporalDataModelPayloadConverter(contextInner); + return ReferenceEquals(contextInner, inner) ? this : new TemporalTransferTypePayloadConverter(contextInner); } - private static ITemporalDataModelConverter? CreateConverter(Type type) + private static ITemporalTransferTypeConverter? CreateConverter(Type type) { - var attr = type.GetCustomAttribute(inherit: true); + var attr = type.GetCustomAttribute(inherit: true); if (attr == null) { return null; } - if (!typeof(ITemporalDataModelConverter).IsAssignableFrom(attr.ConverterType)) + if (!typeof(ITemporalTransferTypeConverter).IsAssignableFrom(attr.ConverterType)) { throw new InvalidOperationException( - $"Type {type} has a Temporal data-model converter type " + - $"{attr.ConverterType} that does not implement {nameof(ITemporalDataModelConverter)}."); + $"Type {type} has a Temporal transfer type converter type " + + $"{attr.ConverterType} that does not implement {nameof(ITemporalTransferTypeConverter)}."); } if (attr.ConverterType.ContainsGenericParameters) { throw new InvalidOperationException( - $"Type {type} has an open generic Temporal data-model converter type " + + $"Type {type} has an open generic Temporal transfer type converter type " + $"{attr.ConverterType}."); } - if (Activator.CreateInstance(attr.ConverterType) is not ITemporalDataModelConverter converter) + if (Activator.CreateInstance(attr.ConverterType) is not ITemporalTransferTypeConverter converter) { throw new InvalidOperationException( - $"Type {type} has a Temporal data-model converter type " + + $"Type {type} has a Temporal transfer type converter type " + $"{attr.ConverterType} that could not be instantiated."); } - if (converter.DataModelType == null) + if (converter.TransferType == null) { throw new InvalidOperationException( - $"Type {type} has a Temporal data-model converter type " + - $"{attr.ConverterType} with a null data-model type."); + $"Type {type} has a Temporal transfer type converter type " + + $"{attr.ConverterType} with a null transfer type."); } return converter; diff --git a/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs b/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs index 25666b3d..bcaaf4cc 100644 --- a/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs +++ b/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs @@ -108,20 +108,19 @@ public void ToValue_WrongProtoType_Fails() } [Fact] - public void ToPayload_DataModelHooks_Succeed() + public void ToPayload_TransferTypeHooks_Succeed() { var dataConverter = new DataConverter( new ContextStringPayloadConverter(), new DefaultFailureConverter()).WithSerializationContext( new ISerializationContext.Workflow("default", "workflow-id")); - var value = new DataModelHookValue("payload-value"); + var value = new TransferTypeHookValue("payload-value"); var payload = dataConverter.PayloadConverter.ToPayload(value); - var memo = (Memo)new DefaultPayloadConverter().ToValue(payload, typeof(Memo))!; - Assert.Equal("workflow-id:payload-value", memo.Fields["value"].Data.ToStringUtf8()); + Assert.Equal("workflow-id:payload-value", payload.Data.ToStringUtf8()); Assert.Equal( value, - dataConverter.PayloadConverter.ToValue(payload, typeof(DataModelHookValue))); + dataConverter.PayloadConverter.ToValue(payload, typeof(TransferTypeHookValue))); } private static Payload AssertPayload( @@ -192,25 +191,17 @@ public CamelCasePayloadConverter() } } - [TemporalDataModel(typeof(DataModelHookValueConverter))] - public sealed record DataModelHookValue(string Value); + [TemporalTransferType(typeof(TransferTypeHookValueConverter))] + public sealed record TransferTypeHookValue(string Value); - public class DataModelHookValueConverter : ITemporalDataModelConverter + public class TransferTypeHookValueConverter : ITemporalTransferTypeConverter { - public Type DataModelType => typeof(Memo); + public Type TransferType => typeof(string); - public object ToDataModel(object? value, IPayloadConverter payloadConverter) => - new Memo - { - Fields = - { - ["value"] = payloadConverter.ToPayload(((DataModelHookValue)value!).Value), - }, - }; + public object ToTransferType(object? value) => ((TransferTypeHookValue)value!).Value; - public object FromDataModel(object? dataModel, IPayloadConverter payloadConverter) => - new DataModelHookValue( - (string)payloadConverter.ToValue(((Memo)dataModel!).Fields["value"], typeof(string))!); + public object FromTransferType(object? transferType) => + new TransferTypeHookValue((string)transferType!); } public class ContextStringPayloadConverter : From ceb5be8cd8ecc2f173c28b6f073fbdb40f393336 Mon Sep 17 00:00:00 2001 From: Tim Conley Date: Mon, 27 Jul 2026 09:47:37 -0700 Subject: [PATCH 3/3] Address transfer type payload converter review --- .../Client/TemporalClient.AsyncActivity.cs | 15 +- src/Temporalio/Client/TemporalClient.cs | 2 + src/Temporalio/Converters/DataConverter.cs | 12 -- .../ITemporalTransferTypeConverter.cs | 2 +- ...TemporalTransferTypeConverterAttribute.cs} | 10 +- .../TemporalTransferTypePayloadConverter.cs | 28 +++- tests/Temporalio.Tests/Common/PluginTests.cs | 25 ++- .../Converters/DataConverterTests.cs | 13 +- .../Converters/PayloadConverterTests.cs | 153 +++++++++++++++++- 9 files changed, 232 insertions(+), 28 deletions(-) rename src/Temporalio/Converters/{TemporalTransferTypeAttribute.cs => TemporalTransferTypeConverterAttribute.cs} (72%) diff --git a/src/Temporalio/Client/TemporalClient.AsyncActivity.cs b/src/Temporalio/Client/TemporalClient.AsyncActivity.cs index 7debe586..33ef205b 100644 --- a/src/Temporalio/Client/TemporalClient.AsyncActivity.cs +++ b/src/Temporalio/Client/TemporalClient.AsyncActivity.cs @@ -25,7 +25,7 @@ internal partial class Impl /// public override async Task HeartbeatAsyncActivityAsync(HeartbeatAsyncActivityInput input) { - var converter = input.DataConverterOverride ?? Client.Options.DataConverter; + var converter = DataConverterForAsyncActivity(input.DataConverterOverride); Payloads? details = null; if (input.Options?.Details != null && input.Options.Details.Count > 0) { @@ -80,7 +80,7 @@ await converter.ToPayloadsAsync(input.Options.Details).ConfigureAwait(false), /// public override async Task CompleteAsyncActivityAsync(CompleteAsyncActivityInput input) { - var converter = input.DataConverterOverride ?? Client.Options.DataConverter; + var converter = DataConverterForAsyncActivity(input.DataConverterOverride); var result = await converter.ToPayloadAsync(input.Result).ConfigureAwait(false); if (input.Activity is AsyncActivityHandle.IdReference idRef) { @@ -117,7 +117,7 @@ await Client.Connection.WorkflowService.RespondActivityTaskCompletedAsync( /// public override async Task FailAsyncActivityAsync(FailAsyncActivityInput input) { - var converter = input.DataConverterOverride ?? Client.Options.DataConverter; + var converter = DataConverterForAsyncActivity(input.DataConverterOverride); var failure = await converter.ToFailureAsync(input.Exception).ConfigureAwait(false); Payloads? lastHeartbeatDetails = null; if (input.Options?.LastHeartbeatDetails != null && @@ -170,7 +170,7 @@ await Client.Connection.WorkflowService.RespondActivityTaskFailedAsync( public override async Task ReportCancellationAsyncActivityAsync( ReportCancellationAsyncActivityInput input) { - var converter = input.DataConverterOverride ?? Client.Options.DataConverter; + var converter = DataConverterForAsyncActivity(input.DataConverterOverride); Payloads? details = null; if (input.Options?.Details != null && input.Options.Details.Count > 0) { @@ -213,6 +213,11 @@ await Client.Connection.WorkflowService.RespondActivityTaskCanceledAsync( throw new ArgumentException("Unrecognized activity reference type"); } } + + private DataConverter DataConverterForAsyncActivity(DataConverter? dataConverterOverride) => + dataConverterOverride == null ? + Client.Options.DataConverter : + TemporalTransferTypePayloadConverter.Wrap(dataConverterOverride); } } -} \ No newline at end of file +} diff --git a/src/Temporalio/Client/TemporalClient.cs b/src/Temporalio/Client/TemporalClient.cs index 88a905e1..efde967a 100644 --- a/src/Temporalio/Client/TemporalClient.cs +++ b/src/Temporalio/Client/TemporalClient.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Threading.Tasks; +using Temporalio.Converters; namespace Temporalio.Client { @@ -29,6 +30,7 @@ public TemporalClient(ITemporalConnection connection, TemporalClientOptions opti } } + options.DataConverter = TemporalTransferTypePayloadConverter.Wrap(options.DataConverter); Connection = connection; Options = options; OutboundInterceptor = new Impl(this); diff --git a/src/Temporalio/Converters/DataConverter.cs b/src/Temporalio/Converters/DataConverter.cs index 9f265ae1..0bc95b7c 100644 --- a/src/Temporalio/Converters/DataConverter.cs +++ b/src/Temporalio/Converters/DataConverter.cs @@ -12,18 +12,6 @@ public record DataConverter( IPayloadCodec? PayloadCodec = null) : IWithSerializationContext { - private readonly IPayloadConverter payloadConverter = - TemporalTransferTypePayloadConverter.Wrap(PayloadConverter); - - /// - /// Gets the payload converter. - /// - public IPayloadConverter PayloadConverter - { - get => payloadConverter; - init => payloadConverter = TemporalTransferTypePayloadConverter.Wrap(value); - } - /// /// Gets default data converter instance. /// diff --git a/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs b/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs index da219be3..f116dae4 100644 --- a/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs +++ b/src/Temporalio/Converters/ITemporalTransferTypeConverter.cs @@ -3,7 +3,7 @@ namespace Temporalio.Converters { /// - /// Converter for a type marked with . + /// Converter for a type marked with . /// /// /// This API is experimental and may change in a future release. diff --git a/src/Temporalio/Converters/TemporalTransferTypeAttribute.cs b/src/Temporalio/Converters/TemporalTransferTypeConverterAttribute.cs similarity index 72% rename from src/Temporalio/Converters/TemporalTransferTypeAttribute.cs rename to src/Temporalio/Converters/TemporalTransferTypeConverterAttribute.cs index d63e76a7..b99aaa1f 100644 --- a/src/Temporalio/Converters/TemporalTransferTypeAttribute.cs +++ b/src/Temporalio/Converters/TemporalTransferTypeConverterAttribute.cs @@ -11,15 +11,17 @@ namespace Temporalio.Converters /// . /// This API is experimental and may change in a future release. /// - [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = true)] - public sealed class TemporalTransferTypeAttribute : Attribute + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = false)] + public sealed class TemporalTransferTypeConverterAttribute : Attribute { /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the + /// class. /// /// Converter type implementing /// . - public TemporalTransferTypeAttribute(Type converterType) => ConverterType = converterType; + public TemporalTransferTypeConverterAttribute(Type converterType) => + ConverterType = converterType; /// /// Gets the converter type. diff --git a/src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs b/src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs index 803cc3eb..ec00e3f7 100644 --- a/src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs +++ b/src/Temporalio/Converters/TemporalTransferTypePayloadConverter.cs @@ -26,6 +26,18 @@ public static IPayloadConverter Wrap(IPayloadConverter payloadConverter) => payloadConverter is TemporalTransferTypePayloadConverter ? payloadConverter : new TemporalTransferTypePayloadConverter(payloadConverter); + /// + /// Wrap the data converter's payload converter unless it is already wrapped. + /// + /// Data converter to wrap. + /// Data converter with a wrapped payload converter. + public static DataConverter Wrap(DataConverter dataConverter) + { + var payloadConverter = Wrap(dataConverter.PayloadConverter); + return ReferenceEquals(payloadConverter, dataConverter.PayloadConverter) ? + dataConverter : dataConverter with { PayloadConverter = payloadConverter }; + } + /// public Payload ToPayload(object? value) { @@ -64,7 +76,8 @@ public IPayloadConverter WithSerializationContext(ISerializationContext context) private static ITemporalTransferTypeConverter? CreateConverter(Type type) { - var attr = type.GetCustomAttribute(inherit: true); + var attr = type.GetCustomAttribute( + inherit: false); if (attr == null) { return null; @@ -76,12 +89,25 @@ public IPayloadConverter WithSerializationContext(ISerializationContext context) $"Type {type} has a Temporal transfer type converter type " + $"{attr.ConverterType} that does not implement {nameof(ITemporalTransferTypeConverter)}."); } + if (attr.ConverterType.IsAbstract) + { + throw new InvalidOperationException( + $"Type {type} has an abstract Temporal transfer type converter type " + + $"{attr.ConverterType}."); + } if (attr.ConverterType.ContainsGenericParameters) { throw new InvalidOperationException( $"Type {type} has an open generic Temporal transfer type converter type " + $"{attr.ConverterType}."); } + if (!attr.ConverterType.IsValueType && + attr.ConverterType.GetConstructor(Type.EmptyTypes) == null) + { + throw new InvalidOperationException( + $"Type {type} has a Temporal transfer type converter type " + + $"{attr.ConverterType} without a public parameterless constructor."); + } if (Activator.CreateInstance(attr.ConverterType) is not ITemporalTransferTypeConverter converter) { diff --git a/tests/Temporalio.Tests/Common/PluginTests.cs b/tests/Temporalio.Tests/Common/PluginTests.cs index af6c12cc..e8d077de 100644 --- a/tests/Temporalio.Tests/Common/PluginTests.cs +++ b/tests/Temporalio.Tests/Common/PluginTests.cs @@ -2,6 +2,7 @@ using Temporalio.Client; using Temporalio.Common; using Temporalio.Converters; +using Temporalio.Tests.Converters; using Temporalio.Worker; using Temporalio.Workflows; using Xunit.Abstractions; @@ -205,6 +206,28 @@ public void TestSimplePlugin_Function() Assert.NotNull(client.Options.DataConverter.PayloadCodec); } + [Fact] + public void TestClientPlugin_DataConverter_WrapsTransferTypePayloadConverter() + { + var plugin = new SimplePlugin("SimplePlugin", new SimplePluginOptions() + { + DataConverterOption = new SimplePluginOptions.SimplePluginOption( + (_) => new DataConverter( + new PayloadConverterTests.ContextStringPayloadConverter(), + new DefaultFailureConverter())), + }); + var newOptions = (TemporalClientOptions)Client.Options.Clone(); + newOptions.Plugins = new[] { plugin }; + + var client = new TemporalClient(Env.Client.Connection, newOptions); + var dataConverter = client.Options.DataConverter.WithSerializationContext( + new ISerializationContext.Workflow("default", "workflow-id")); + var payload = dataConverter.PayloadConverter.ToPayload( + new PayloadConverterTests.TransferTypeHookValue("payload-value")); + + Assert.Equal("workflow-id:payload-value", payload.Data.ToStringUtf8()); + } + [Fact] public async Task TestSimplePlugin_RunContext() { @@ -236,4 +259,4 @@ public async Task TestSimplePlugin_RunContext() Assert.Contains("Beginning", transitions); Assert.Contains("Ending", transitions); } -} \ No newline at end of file +} diff --git a/tests/Temporalio.Tests/Converters/DataConverterTests.cs b/tests/Temporalio.Tests/Converters/DataConverterTests.cs index 15557ff4..84bf9189 100644 --- a/tests/Temporalio.Tests/Converters/DataConverterTests.cs +++ b/tests/Temporalio.Tests/Converters/DataConverterTests.cs @@ -22,13 +22,24 @@ public void NewDataConverter_WithPayloadConverter_ProperlyInitializes() { PayloadConverter = payloadConverter, }; - Assert.NotSame(payloadConverter, newConverter.PayloadConverter); + Assert.Same(payloadConverter, newConverter.PayloadConverter); Assert.Equal( "payload", newConverter.PayloadConverter.ToValue( newConverter.PayloadConverter.ToPayload("payload"), typeof(string))); } + [Fact] + public void NewDataConverter_EquivalentConverters_AreEqual() + { + var payloadConverter = new MyPayloadConverter(); + var failureConverter = new DefaultFailureConverter(); + + Assert.Equal( + new DataConverter(payloadConverter, failureConverter), + new DataConverter(payloadConverter, failureConverter)); + } + public class MyPayloadConverter : IPayloadConverter { public Payload ToPayload(object? value) => new() diff --git a/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs b/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs index bcaaf4cc..a78db766 100644 --- a/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs +++ b/tests/Temporalio.Tests/Converters/PayloadConverterTests.cs @@ -110,9 +110,9 @@ public void ToValue_WrongProtoType_Fails() [Fact] public void ToPayload_TransferTypeHooks_Succeed() { - var dataConverter = new DataConverter( + var dataConverter = TemporalTransferTypePayloadConverter.Wrap(new DataConverter( new ContextStringPayloadConverter(), - new DefaultFailureConverter()).WithSerializationContext( + new DefaultFailureConverter())).WithSerializationContext( new ISerializationContext.Workflow("default", "workflow-id")); var value = new TransferTypeHookValue("payload-value"); @@ -123,6 +123,58 @@ public void ToPayload_TransferTypeHooks_Succeed() dataConverter.PayloadConverter.ToValue(payload, typeof(TransferTypeHookValue))); } + [Fact] + public void ToPayload_TransferTypeHooks_DoesNotUseInheritedAttribute() + { + var converter = TemporalTransferTypePayloadConverter.Wrap(new ContextStringPayloadConverter()); + + var payload = converter.ToPayload(new DerivedTransferTypeHookValue("payload-value")); + + Assert.NotEqual("test/context-string", payload.Metadata["encoding"].ToStringUtf8()); + } + + [Fact] + public void ToPayload_TransferTypeHooks_BaseTypeWithAttribute_Succeed() + { + var converter = TemporalTransferTypePayloadConverter.Wrap(new ContextStringPayloadConverter()); + var value = new BaseTransferTypeHookValue("payload-value"); + + var payload = converter.ToPayload(value); + + Assert.Equal(":base:payload-value", payload.Data.ToStringUtf8()); + Assert.Equal(value, converter.ToValue(payload, typeof(BaseTransferTypeHookValue))); + } + + [Fact] + public void ToPayload_TransferTypeHooks_DerivedTypeWithAttribute_Succeed() + { + var converter = TemporalTransferTypePayloadConverter.Wrap(new ContextStringPayloadConverter()); + var value = new AnnotatedDerivedTransferTypeHookValue("payload-value"); + + var payload = converter.ToPayload(value); + + Assert.Equal(":derived:payload-value", payload.Data.ToStringUtf8()); + Assert.Equal(value, converter.ToValue(payload, typeof(AnnotatedDerivedTransferTypeHookValue))); + } + + [Theory] + [InlineData(typeof(NonAssignableConverterHookValue), "does not implement")] + [InlineData(typeof(AbstractConverterHookValue), "abstract")] + [InlineData(typeof(OpenGenericConverterHookValue), "open generic")] + [InlineData(typeof(ParameterConstructorConverterHookValue), "public parameterless constructor")] + [InlineData(typeof(PrivateConstructorConverterHookValue), "public parameterless constructor")] + public void ToPayload_TransferTypeHooks_InvalidConverterType_Fails( + Type type, + string expectedMessage) + { + var converter = TemporalTransferTypePayloadConverter.Wrap(new DefaultPayloadConverter()); + var value = Activator.CreateInstance(type, "payload-value"); + + var exception = Assert.Throws(() => converter.ToPayload(value)); + + Assert.Contains(expectedMessage, exception.Message); + } + private static Payload AssertPayload( object? value, string expectedEncoding, @@ -191,7 +243,7 @@ public CamelCasePayloadConverter() } } - [TemporalTransferType(typeof(TransferTypeHookValueConverter))] + [TemporalTransferTypeConverter(typeof(TransferTypeHookValueConverter))] public sealed record TransferTypeHookValue(string Value); public class TransferTypeHookValueConverter : ITemporalTransferTypeConverter @@ -204,6 +256,101 @@ public object FromTransferType(object? transferType) => new TransferTypeHookValue((string)transferType!); } + [TemporalTransferTypeConverter(typeof(BaseTransferTypeHookValueConverter))] + public record BaseTransferTypeHookValue(string Value); + + public record DerivedTransferTypeHookValue(string Value) : + BaseTransferTypeHookValue(Value); + + [TemporalTransferTypeConverter(typeof(AnnotatedDerivedTransferTypeHookValueConverter))] + public record AnnotatedDerivedTransferTypeHookValue(string Value) : + BaseTransferTypeHookValue(Value); + + public class BaseTransferTypeHookValueConverter : ITemporalTransferTypeConverter + { + public Type TransferType => typeof(string); + + public object ToTransferType(object? value) => + $"base:{((BaseTransferTypeHookValue)value!).Value}"; + + public object FromTransferType(object? transferType) => + new BaseTransferTypeHookValue(((string)transferType!)[5..]); + } + + public class AnnotatedDerivedTransferTypeHookValueConverter : + ITemporalTransferTypeConverter + { + public Type TransferType => typeof(string); + + public object ToTransferType(object? value) => + $"derived:{((AnnotatedDerivedTransferTypeHookValue)value!).Value}"; + + public object FromTransferType(object? transferType) => + new AnnotatedDerivedTransferTypeHookValue(((string)transferType!)[8..]); + } + + [TemporalTransferTypeConverter(typeof(string))] + public sealed record NonAssignableConverterHookValue(string Value); + + [TemporalTransferTypeConverter(typeof(AbstractTransferTypeHookValueConverter))] + public sealed record AbstractConverterHookValue(string Value); + + public abstract class AbstractTransferTypeHookValueConverter : + ITemporalTransferTypeConverter + { + public abstract Type TransferType { get; } + + public abstract object? ToTransferType(object? value); + + public abstract object? FromTransferType(object? transferType); + } + + [TemporalTransferTypeConverter(typeof(GenericTransferTypeHookValueConverter<>))] + public sealed record OpenGenericConverterHookValue(string Value); + + public class GenericTransferTypeHookValueConverter : ITemporalTransferTypeConverter + { + public Type TransferType => typeof(string); + + public object? ToTransferType(object? value) => throw new NotImplementedException(); + + public object? FromTransferType(object? transferType) => throw new NotImplementedException(); + } + + [TemporalTransferTypeConverter(typeof(ParameterConstructorTransferTypeHookValueConverter))] + public sealed record ParameterConstructorConverterHookValue(string Value); + + public class ParameterConstructorTransferTypeHookValueConverter : + ITemporalTransferTypeConverter + { + public ParameterConstructorTransferTypeHookValueConverter(string value) + { + } + + public Type TransferType => typeof(string); + + public object? ToTransferType(object? value) => throw new NotImplementedException(); + + public object? FromTransferType(object? transferType) => throw new NotImplementedException(); + } + + [TemporalTransferTypeConverter(typeof(PrivateConstructorTransferTypeHookValueConverter))] + public sealed record PrivateConstructorConverterHookValue(string Value); + + public class PrivateConstructorTransferTypeHookValueConverter : + ITemporalTransferTypeConverter + { + private PrivateConstructorTransferTypeHookValueConverter() + { + } + + public Type TransferType => typeof(string); + + public object? ToTransferType(object? value) => throw new NotImplementedException(); + + public object? FromTransferType(object? transferType) => throw new NotImplementedException(); + } + public class ContextStringPayloadConverter : IPayloadConverter, IWithSerializationContext