From 1c02eb563868f37c3cdbd0ca4d95deda79a7d9c1 Mon Sep 17 00:00:00 2001 From: sebastianburckhardt Date: Wed, 4 Oct 2023 16:34:28 -0700 Subject: [PATCH 1/5] revise EntityInstanceId implementation and checking --- src/Abstractions/Entities/EntityInstanceId.cs | 66 ++++++++++++++++++- src/Abstractions/TaskOptions.cs | 2 + src/Client/Core/Entities/EntityQuery.cs | 21 +++++- src/Client/Grpc/GrpcDurableTaskClient.cs | 14 ++++ src/Shared/Core/Validation/Check.cs | 15 +++++ src/Worker/Core/Shims/TaskEntityShim.cs | 5 +- .../Shims/TaskOrchestrationContextWrapper.cs | 1 + .../Shims/TaskOrchestrationEntityContext.cs | 12 +--- 8 files changed, 119 insertions(+), 17 deletions(-) diff --git a/src/Abstractions/Entities/EntityInstanceId.cs b/src/Abstractions/Entities/EntityInstanceId.cs index d0f18a41a..66993f2ea 100644 --- a/src/Abstractions/Entities/EntityInstanceId.cs +++ b/src/Abstractions/Entities/EntityInstanceId.cs @@ -1,15 +1,75 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System.Text.Json; +using System.Text.Json.Serialization; + namespace Microsoft.DurableTask.Entities; /// /// Represents the ID of an entity. /// -/// The name of the entity. -/// The key for this entity. -public readonly record struct EntityInstanceId(string Name, string Key) +[JsonConverter(typeof(EntityInstanceId.JsonConverter))] +public readonly record struct EntityInstanceId { + /// + /// Initializes a new instance of the class. + /// + /// The entity name. + /// The entity key. + public EntityInstanceId(string name, string key) + { + Check.NotNullOrEmpty(name); + Check.NotNull(key); + this.Name = name.ToLowerInvariant(); + this.Key = key; + } + + /// + /// Gets the entity name. Entity names are normalized to lower case. + /// + public string Name { get; } + + /// + /// Gets the entity key. + /// + public string Key { get; } + /// public override string ToString() => $"@{this.Name}@{this.Key}"; + + /// + /// Constructs a from a string containing the instance ID. + /// + /// The string representation of the entity ID. + /// the constructed entity instance ID. + public static EntityInstanceId FromString(string instanceId) + { + Check.NotNullOrEmpty(instanceId); + var pos = instanceId.IndexOf('@', 1); + if (pos <= 0 || instanceId[0] != '@') + { + throw new ArgumentException($"Instance ID '{instanceId}' is not a valid entity ID.", nameof(instanceId)); + } + + var entityName = instanceId.Substring(1, pos - 1); + var entityKey = instanceId.Substring(pos + 1); + return new EntityInstanceId(entityName, entityKey); + } + + /// + /// We override the default json conversion so we can use a more compact string representation for entity instance ids. + /// + class JsonConverter : JsonConverter + { + public override EntityInstanceId Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + return EntityInstanceId.FromString(reader.GetString()!); + } + + public override void Write(Utf8JsonWriter writer, EntityInstanceId value, JsonSerializerOptions options) + { + writer.WriteStringValue(value.ToString()!); + } + } } diff --git a/src/Abstractions/TaskOptions.cs b/src/Abstractions/TaskOptions.cs index 63f943825..37b84cd1e 100644 --- a/src/Abstractions/TaskOptions.cs +++ b/src/Abstractions/TaskOptions.cs @@ -66,6 +66,7 @@ public record SubOrchestrationOptions : TaskOptions public SubOrchestrationOptions(TaskRetryOptions? retry = null, string? instanceId = null) : base(retry) { + Check.NotEntity(instanceId); this.InstanceId = instanceId; } @@ -77,6 +78,7 @@ public SubOrchestrationOptions(TaskRetryOptions? retry = null, string? instanceI public SubOrchestrationOptions(TaskOptions options, string? instanceId = null) : base(options) { + Check.NotEntity(instanceId); this.InstanceId = instanceId; if (instanceId is null && options is SubOrchestrationOptions derived) { diff --git a/src/Client/Core/Entities/EntityQuery.cs b/src/Client/Core/Entities/EntityQuery.cs index 15ef7a644..a356ebdef 100644 --- a/src/Client/Core/Entities/EntityQuery.cs +++ b/src/Client/Core/Entities/EntityQuery.cs @@ -39,9 +39,24 @@ public string? InstanceIdStartsWith get => this.instanceIdStartsWith; init { - // prefix '@' if filter value provided and not already prefixed with '@'. - this.instanceIdStartsWith = value?.Length > 0 && value[0] != '@' - ? $"@{value}" : value; + if (value != null) + { + // prefix '@' if filter value provided and not already prefixed with '@'. + string prefix = value.Length == 0 || value[0] != '@' ? $"@{value}" : value; + + // check if there is a name-key separator in the string + int pos = prefix.IndexOf('@', 1); + if (pos != -1) + { + // selectively normalize only the part up until that separator + this.instanceIdStartsWith = prefix.Substring(0, pos).ToLowerInvariant() + prefix.Substring(pos); + } + else + { + // normalize the entire prefix + this.instanceIdStartsWith = prefix.ToLowerInvariant(); + } + } } } diff --git a/src/Client/Grpc/GrpcDurableTaskClient.cs b/src/Client/Grpc/GrpcDurableTaskClient.cs index a6ad1b12e..a2a08bee0 100644 --- a/src/Client/Grpc/GrpcDurableTaskClient.cs +++ b/src/Client/Grpc/GrpcDurableTaskClient.cs @@ -70,6 +70,8 @@ public override async Task ScheduleNewOrchestrationInstanceAsync( StartOrchestrationOptions? options = null, CancellationToken cancellation = default) { + Check.NotEntity(options?.InstanceId); + var request = new P.CreateInstanceRequest { Name = orchestratorName.Name, @@ -102,6 +104,7 @@ public override async Task RaiseEventAsync( { Check.NotNullOrEmpty(instanceId); Check.NotNullOrEmpty(eventName); + Check.NotEntity(instanceId); P.RaiseEventRequest request = new() { @@ -118,6 +121,7 @@ public override async Task TerminateInstanceAsync( string instanceId, object? output = null, CancellationToken cancellation = default) { Check.NotNullOrEmpty(instanceId); + Check.NotEntity(instanceId); this.logger.TerminatingInstance(instanceId); string? serializedOutput = this.DataConverter.Serialize(output); @@ -134,6 +138,8 @@ await this.sidecarClient.TerminateInstanceAsync( public override async Task SuspendInstanceAsync( string instanceId, string? reason = null, CancellationToken cancellation = default) { + Check.NotEntity(instanceId); + P.SuspendRequest request = new() { InstanceId = instanceId, @@ -155,6 +161,8 @@ public override async Task SuspendInstanceAsync( public override async Task ResumeInstanceAsync( string instanceId, string? reason = null, CancellationToken cancellation = default) { + Check.NotEntity(instanceId); + P.ResumeRequest request = new() { InstanceId = instanceId, @@ -176,6 +184,8 @@ public override async Task ResumeInstanceAsync( public override async Task GetInstancesAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { + Check.NotEntity(instanceId); + if (string.IsNullOrEmpty(instanceId)) { throw new ArgumentNullException(nameof(instanceId)); @@ -201,6 +211,8 @@ public override async Task ResumeInstanceAsync( /// public override AsyncPageable GetAllInstancesAsync(OrchestrationQuery? filter = null) { + Check.NotEntity(filter?.InstanceIdPrefix); + return Pageable.Create(async (continuation, pageSize, cancellation) => { P.QueryInstancesRequest request = new() @@ -250,6 +262,7 @@ public override AsyncPageable GetAllInstancesAsync(Orches public override async Task WaitForInstanceStartAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { + Check.NotEntity(instanceId); this.logger.WaitingForInstanceStart(instanceId, getInputsAndOutputs); P.GetInstanceRequest request = new() @@ -275,6 +288,7 @@ public override async Task WaitForInstanceStartAsync( public override async Task WaitForInstanceCompletionAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { + Check.NotEntity(instanceId); this.logger.WaitingForInstanceCompletion(instanceId, getInputsAndOutputs); P.GetInstanceRequest request = new() diff --git a/src/Shared/Core/Validation/Check.cs b/src/Shared/Core/Validation/Check.cs index f7acd310f..7310f5866 100644 --- a/src/Shared/Core/Validation/Check.cs +++ b/src/Shared/Core/Validation/Check.cs @@ -94,6 +94,21 @@ public static string NotNullOrEmpty( return argument; } + /// + /// Checks if the provided string is an entity instance id, throwing if it is. + /// Throws if the conditions are not met. + /// + /// The string to check. + /// The name of the string for the exception. + public static void NotEntity( + string? argument, [CallerArgumentExpression("argument")] string? name = default) + { + if (argument?.Length > 0 && argument[0] == '@') + { + throw new ArgumentException("Parameter cannot be an entity instance id", name); + } + } + /// /// Checks if the supplied type is a concrete non-abstract type and implements the provided generic type. /// Throws if the conditions are not met. diff --git a/src/Worker/Core/Shims/TaskEntityShim.cs b/src/Worker/Core/Shims/TaskEntityShim.cs index 23d76c1af..5fad513b9 100644 --- a/src/Worker/Core/Shims/TaskEntityShim.cs +++ b/src/Worker/Core/Shims/TaskEntityShim.cs @@ -195,8 +195,7 @@ public void Reset() public override void SignalEntity(EntityInstanceId id, string operationName, object? input = null, SignalEntityOptions? options = null) { - Check.NotNullOrEmpty(id.Name); - Check.NotNull(id.Key); + Check.NotDefault(id); this.operationActions.Add(new SendSignalOperationAction() { @@ -209,6 +208,8 @@ public override void SignalEntity(EntityInstanceId id, string operationName, obj public override void StartOrchestration(TaskName name, object? input = null, StartOrchestrationOptions? options = null) { + Check.NotEntity(options?.InstanceId); + this.operationActions.Add(new StartNewOrchestrationOperationAction() { Name = name.Name, diff --git a/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs b/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs index b9e1484fb..ecf228e05 100644 --- a/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs +++ b/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs @@ -233,6 +233,7 @@ public override Task WaitForExternalEvent(string eventName, CancellationTo /// public override void SendEvent(string instanceId, string eventName, object eventData) { + Check.NotEntity(instanceId); this.innerContext.SendEvent(new OrchestrationInstance { InstanceId = instanceId }, eventName, eventData); } diff --git a/src/Worker/Core/Shims/TaskOrchestrationEntityContext.cs b/src/Worker/Core/Shims/TaskOrchestrationEntityContext.cs index 5f47b43ae..514abbe5e 100644 --- a/src/Worker/Core/Shims/TaskOrchestrationEntityContext.cs +++ b/src/Worker/Core/Shims/TaskOrchestrationEntityContext.cs @@ -89,9 +89,7 @@ public override async Task LockEntitiesAsync(IEnumerable public override async Task CallEntityAsync(EntityInstanceId id, string operationName, object? input = null, CallEntityOptions? options = null) { - Check.NotNullOrEmpty(id.Name); - Check.NotNull(id.Key); - + Check.NotDefault(id); OperationResult operationResult = await this.CallEntityInternalAsync(id, operationName, input); if (operationResult.IsError) @@ -107,9 +105,7 @@ public override async Task CallEntityAsync(EntityInstanceId id /// public override async Task CallEntityAsync(EntityInstanceId id, string operationName, object? input = null, CallEntityOptions? options = null) { - Check.NotNullOrEmpty(id.Name); - Check.NotNull(id.Key); - + Check.NotDefault(id); OperationResult operationResult = await this.CallEntityInternalAsync(id, operationName, input); if (operationResult.IsError) @@ -121,9 +117,7 @@ public override async Task CallEntityAsync(EntityInstanceId id, string operation /// public override Task SignalEntityAsync(EntityInstanceId id, string operationName, object? input = null, SignalEntityOptions? options = null) { - Check.NotNullOrEmpty(id.Name); - Check.NotNull(id.Key); - + Check.NotDefault(id); this.SendOperationMessage(id.ToString(), operationName, input, oneWay: true, scheduledTime: options?.SignalTime); return Task.CompletedTask; } From 6627f9f7cc30862a41a9e197a10f5f33f258c81f Mon Sep 17 00:00:00 2001 From: sebastianburckhardt Date: Thu, 5 Oct 2023 15:37:31 -0700 Subject: [PATCH 2/5] address PR feedback: add switch to enable/disable entity support and associated checking --- src/Client/Core/DurableTaskClientOptions.cs | 7 +++ src/Client/Grpc/GrpcDurableTaskClient.cs | 60 +++++++++++++++---- src/Worker/Core/DurableTaskWorkerOptions.cs | 7 +++ .../Shims/TaskOrchestrationContextWrapper.cs | 30 +++++++++- 4 files changed, 90 insertions(+), 14 deletions(-) diff --git a/src/Client/Core/DurableTaskClientOptions.cs b/src/Client/Core/DurableTaskClientOptions.cs index 1c84eb949..351207c0c 100644 --- a/src/Client/Core/DurableTaskClientOptions.cs +++ b/src/Client/Core/DurableTaskClientOptions.cs @@ -46,6 +46,12 @@ public DataConverter DataConverter } } + /// + /// Gets or sets a value indicating whether this client supports entities. If true, all instance ids starting with '@' are reserved for entities, + /// and validation checks are performed where appropriate. + /// + public bool SupportEntities { get; set; } + /// /// Gets a value indicating whether was explicitly set or not. /// @@ -67,6 +73,7 @@ internal void ApplyTo(DurableTaskClientOptions other) { // Make sure to keep this up to date as values are added. other.DataConverter = this.DataConverter; + other.SupportEntities = this.SupportEntities; } } } diff --git a/src/Client/Grpc/GrpcDurableTaskClient.cs b/src/Client/Grpc/GrpcDurableTaskClient.cs index a2a08bee0..8b996fbe6 100644 --- a/src/Client/Grpc/GrpcDurableTaskClient.cs +++ b/src/Client/Grpc/GrpcDurableTaskClient.cs @@ -20,7 +20,7 @@ public sealed class GrpcDurableTaskClient : DurableTaskClient readonly ILogger logger; readonly TaskHubSidecarServiceClient sidecarClient; readonly GrpcDurableTaskClientOptions options; - readonly DurableEntityClient entityClient; + readonly DurableEntityClient? entityClient; AsyncDisposable asyncDisposable; /// @@ -49,11 +49,16 @@ public GrpcDurableTaskClient(string name, GrpcDurableTaskClientOptions options, this.options = Check.NotNull(options); this.asyncDisposable = GetCallInvoker(options, out CallInvoker callInvoker); this.sidecarClient = new TaskHubSidecarServiceClient(callInvoker); - this.entityClient = new GrpcDurableEntityClient(this.Name, this.DataConverter, this.sidecarClient, logger); + + if (this.options.SupportEntities) + { + this.entityClient = new GrpcDurableEntityClient(this.Name, this.DataConverter, this.sidecarClient, logger); + } } /// - public override DurableEntityClient Entities => this.entityClient; + public override DurableEntityClient Entities => this.entityClient + ?? throw new NotSupportedException($"Durable entities are disabled because DurableTaskClientOptions.SupportEntities=false"); DataConverter DataConverter => this.options.DataConverter; @@ -70,7 +75,10 @@ public override async Task ScheduleNewOrchestrationInstanceAsync( StartOrchestrationOptions? options = null, CancellationToken cancellation = default) { - Check.NotEntity(options?.InstanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(options?.InstanceId); + } var request = new P.CreateInstanceRequest { @@ -104,7 +112,11 @@ public override async Task RaiseEventAsync( { Check.NotNullOrEmpty(instanceId); Check.NotNullOrEmpty(eventName); - Check.NotEntity(instanceId); + + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } P.RaiseEventRequest request = new() { @@ -121,7 +133,11 @@ public override async Task TerminateInstanceAsync( string instanceId, object? output = null, CancellationToken cancellation = default) { Check.NotNullOrEmpty(instanceId); - Check.NotEntity(instanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } + this.logger.TerminatingInstance(instanceId); string? serializedOutput = this.DataConverter.Serialize(output); @@ -138,7 +154,10 @@ await this.sidecarClient.TerminateInstanceAsync( public override async Task SuspendInstanceAsync( string instanceId, string? reason = null, CancellationToken cancellation = default) { - Check.NotEntity(instanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } P.SuspendRequest request = new() { @@ -161,7 +180,10 @@ public override async Task SuspendInstanceAsync( public override async Task ResumeInstanceAsync( string instanceId, string? reason = null, CancellationToken cancellation = default) { - Check.NotEntity(instanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } P.ResumeRequest request = new() { @@ -184,7 +206,10 @@ public override async Task ResumeInstanceAsync( public override async Task GetInstancesAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { - Check.NotEntity(instanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } if (string.IsNullOrEmpty(instanceId)) { @@ -211,7 +236,10 @@ public override async Task ResumeInstanceAsync( /// public override AsyncPageable GetAllInstancesAsync(OrchestrationQuery? filter = null) { - Check.NotEntity(filter?.InstanceIdPrefix); + if (this.options.SupportEntities) + { + Check.NotEntity(filter?.InstanceIdPrefix); + } return Pageable.Create(async (continuation, pageSize, cancellation) => { @@ -262,7 +290,11 @@ public override AsyncPageable GetAllInstancesAsync(Orches public override async Task WaitForInstanceStartAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { - Check.NotEntity(instanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } + this.logger.WaitingForInstanceStart(instanceId, getInputsAndOutputs); P.GetInstanceRequest request = new() @@ -288,7 +320,11 @@ public override async Task WaitForInstanceStartAsync( public override async Task WaitForInstanceCompletionAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { - Check.NotEntity(instanceId); + if (this.options.SupportEntities) + { + Check.NotEntity(instanceId); + } + this.logger.WaitingForInstanceCompletion(instanceId, getInputsAndOutputs); P.GetInstanceRequest request = new() diff --git a/src/Worker/Core/DurableTaskWorkerOptions.cs b/src/Worker/Core/DurableTaskWorkerOptions.cs index 4a5b7c98e..9f6fd026f 100644 --- a/src/Worker/Core/DurableTaskWorkerOptions.cs +++ b/src/Worker/Core/DurableTaskWorkerOptions.cs @@ -44,6 +44,12 @@ public DataConverter DataConverter } } + /// + /// Gets or sets a value indicating whether this client supports entities. If true, all instance ids starting with '@' are reserved for entities, + /// and validation checks are performed where appropriate. + /// + public bool SupportEntities { get; set; } + /// /// Gets or sets the maximum timer interval for the /// method. @@ -99,6 +105,7 @@ internal void ApplyTo(DurableTaskWorkerOptions other) // Make sure to keep this up to date as values are added. other.DataConverter = this.DataConverter; other.MaximumTimerInterval = this.MaximumTimerInterval; + other.SupportEntities = this.SupportEntities; } } } diff --git a/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs b/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs index ecf228e05..239def994 100644 --- a/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs +++ b/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs @@ -61,7 +61,24 @@ public TaskOrchestrationContextWrapper( /// public override TaskOrchestrationEntityFeature Entities - => this.entityFeature ??= new TaskOrchestrationEntityContext(this); + { + get + { + if (this.entityFeature == null) + { + if (this.invocationContext.Options.SupportEntities) + { + this.entityFeature = new TaskOrchestrationEntityContext(this); + } + else + { + throw new NotSupportedException($"Durable entities are disabled because DurableTaskWorkerOptions.SupportEntities=false"); + } + } + + return this.entityFeature; + } + } /// /// Gets the DataConverter to use for inputs, outputs, and entity states. @@ -134,6 +151,11 @@ public override async Task CallSubOrchestratorAsync( => options is SubOrchestrationOptions derived ? derived.InstanceId : null; string instanceId = GetInstanceId(options) ?? this.NewGuid().ToString("N"); + if (this.invocationContext.Options.SupportEntities) + { + Check.NotEntity(instanceId); + } + // if this orchestration uses entities, first validate that the suborchsestration call is allowed in the current context if (this.entityFeature != null && !this.entityFeature.EntityContext.ValidateSuborchestrationTransition(out string? errorMsg)) { @@ -233,7 +255,11 @@ public override Task WaitForExternalEvent(string eventName, CancellationTo /// public override void SendEvent(string instanceId, string eventName, object eventData) { - Check.NotEntity(instanceId); + if (this.invocationContext.Options.SupportEntities) + { + Check.NotEntity(instanceId); + } + this.innerContext.SendEvent(new OrchestrationInstance { InstanceId = instanceId }, eventName, eventData); } From 2575fbc322c4ececd00384c265a28dff6ed0b87c Mon Sep 17 00:00:00 2001 From: sebastianburckhardt Date: Fri, 6 Oct 2023 14:54:59 -0700 Subject: [PATCH 3/5] one more validation check for entity names --- src/Abstractions/Entities/EntityInstanceId.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Abstractions/Entities/EntityInstanceId.cs b/src/Abstractions/Entities/EntityInstanceId.cs index 66993f2ea..0722e42fd 100644 --- a/src/Abstractions/Entities/EntityInstanceId.cs +++ b/src/Abstractions/Entities/EntityInstanceId.cs @@ -20,6 +20,11 @@ public readonly record struct EntityInstanceId public EntityInstanceId(string name, string key) { Check.NotNullOrEmpty(name); + if (name.Contains('@')) + { + throw new ArgumentException("entity names may not contain `@` characters.", nameof(name)); + } + Check.NotNull(key); this.Name = name.ToLowerInvariant(); this.Key = key; From f6a2299161fe4a672e2a6dea8b05163f043df60f Mon Sep 17 00:00:00 2001 From: sebastianburckhardt Date: Fri, 6 Oct 2023 14:55:32 -0700 Subject: [PATCH 4/5] remove checks that were supposed to be removed but got re-added during merge mess --- src/Abstractions/TaskOptions.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Abstractions/TaskOptions.cs b/src/Abstractions/TaskOptions.cs index 37b84cd1e..63f943825 100644 --- a/src/Abstractions/TaskOptions.cs +++ b/src/Abstractions/TaskOptions.cs @@ -66,7 +66,6 @@ public record SubOrchestrationOptions : TaskOptions public SubOrchestrationOptions(TaskRetryOptions? retry = null, string? instanceId = null) : base(retry) { - Check.NotEntity(instanceId); this.InstanceId = instanceId; } @@ -78,7 +77,6 @@ public SubOrchestrationOptions(TaskRetryOptions? retry = null, string? instanceI public SubOrchestrationOptions(TaskOptions options, string? instanceId = null) : base(options) { - Check.NotEntity(instanceId); this.InstanceId = instanceId; if (instanceId is null && options is SubOrchestrationOptions derived) { From 1afff32511bfb5163b0aad79ef10c61a23697737 Mon Sep 17 00:00:00 2001 From: sebastianburckhardt Date: Fri, 6 Oct 2023 14:57:54 -0700 Subject: [PATCH 5/5] address PR feedback --- src/Abstractions/Entities/EntityInstanceId.cs | 6 +-- src/Client/Core/DurableTaskClientOptions.cs | 6 +-- src/Client/Grpc/GrpcDurableTaskClient.cs | 49 +++++-------------- src/Shared/Core/Validation/Check.cs | 15 +++--- src/Worker/Core/DurableTaskWorkerOptions.cs | 6 +-- src/Worker/Core/Shims/TaskEntityShim.cs | 2 +- .../Shims/TaskOrchestrationContextWrapper.cs | 14 ++---- 7 files changed, 32 insertions(+), 66 deletions(-) diff --git a/src/Abstractions/Entities/EntityInstanceId.cs b/src/Abstractions/Entities/EntityInstanceId.cs index 0722e42fd..52712a24a 100644 --- a/src/Abstractions/Entities/EntityInstanceId.cs +++ b/src/Abstractions/Entities/EntityInstanceId.cs @@ -40,9 +40,6 @@ public EntityInstanceId(string name, string key) /// public string Key { get; } - /// - public override string ToString() => $"@{this.Name}@{this.Key}"; - /// /// Constructs a from a string containing the instance ID. /// @@ -62,6 +59,9 @@ public static EntityInstanceId FromString(string instanceId) return new EntityInstanceId(entityName, entityKey); } + /// + public override string ToString() => $"@{this.Name}@{this.Key}"; + /// /// We override the default json conversion so we can use a more compact string representation for entity instance ids. /// diff --git a/src/Client/Core/DurableTaskClientOptions.cs b/src/Client/Core/DurableTaskClientOptions.cs index 351207c0c..142be7e52 100644 --- a/src/Client/Core/DurableTaskClientOptions.cs +++ b/src/Client/Core/DurableTaskClientOptions.cs @@ -47,10 +47,10 @@ public DataConverter DataConverter } /// - /// Gets or sets a value indicating whether this client supports entities. If true, all instance ids starting with '@' are reserved for entities, + /// Gets or sets a value indicating whether this client should support entities. If true, all instance ids starting with '@' are reserved for entities, /// and validation checks are performed where appropriate. /// - public bool SupportEntities { get; set; } + public bool EnableEntitySupport { get; set; } /// /// Gets a value indicating whether was explicitly set or not. @@ -73,7 +73,7 @@ internal void ApplyTo(DurableTaskClientOptions other) { // Make sure to keep this up to date as values are added. other.DataConverter = this.DataConverter; - other.SupportEntities = this.SupportEntities; + other.EnableEntitySupport = this.EnableEntitySupport; } } } diff --git a/src/Client/Grpc/GrpcDurableTaskClient.cs b/src/Client/Grpc/GrpcDurableTaskClient.cs index 8b996fbe6..b0efbb933 100644 --- a/src/Client/Grpc/GrpcDurableTaskClient.cs +++ b/src/Client/Grpc/GrpcDurableTaskClient.cs @@ -50,7 +50,7 @@ public GrpcDurableTaskClient(string name, GrpcDurableTaskClientOptions options, this.asyncDisposable = GetCallInvoker(options, out CallInvoker callInvoker); this.sidecarClient = new TaskHubSidecarServiceClient(callInvoker); - if (this.options.SupportEntities) + if (this.options.EnableEntitySupport) { this.entityClient = new GrpcDurableEntityClient(this.Name, this.DataConverter, this.sidecarClient, logger); } @@ -58,7 +58,7 @@ public GrpcDurableTaskClient(string name, GrpcDurableTaskClientOptions options, /// public override DurableEntityClient Entities => this.entityClient - ?? throw new NotSupportedException($"Durable entities are disabled because DurableTaskClientOptions.SupportEntities=false"); + ?? throw new NotSupportedException($"Durable entities are disabled because {nameof(DurableTaskClientOptions)}.{nameof(DurableTaskClientOptions.EnableEntitySupport)}=false"); DataConverter DataConverter => this.options.DataConverter; @@ -75,10 +75,7 @@ public override async Task ScheduleNewOrchestrationInstanceAsync( StartOrchestrationOptions? options = null, CancellationToken cancellation = default) { - if (this.options.SupportEntities) - { - Check.NotEntity(options?.InstanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, options?.InstanceId); var request = new P.CreateInstanceRequest { @@ -113,10 +110,7 @@ public override async Task RaiseEventAsync( Check.NotNullOrEmpty(instanceId); Check.NotNullOrEmpty(eventName); - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); P.RaiseEventRequest request = new() { @@ -133,10 +127,7 @@ public override async Task TerminateInstanceAsync( string instanceId, object? output = null, CancellationToken cancellation = default) { Check.NotNullOrEmpty(instanceId); - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); this.logger.TerminatingInstance(instanceId); @@ -154,10 +145,7 @@ await this.sidecarClient.TerminateInstanceAsync( public override async Task SuspendInstanceAsync( string instanceId, string? reason = null, CancellationToken cancellation = default) { - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); P.SuspendRequest request = new() { @@ -180,10 +168,7 @@ public override async Task SuspendInstanceAsync( public override async Task ResumeInstanceAsync( string instanceId, string? reason = null, CancellationToken cancellation = default) { - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); P.ResumeRequest request = new() { @@ -206,10 +191,7 @@ public override async Task ResumeInstanceAsync( public override async Task GetInstancesAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); if (string.IsNullOrEmpty(instanceId)) { @@ -236,10 +218,7 @@ public override async Task ResumeInstanceAsync( /// public override AsyncPageable GetAllInstancesAsync(OrchestrationQuery? filter = null) { - if (this.options.SupportEntities) - { - Check.NotEntity(filter?.InstanceIdPrefix); - } + Check.NotEntity(this.options.EnableEntitySupport, filter?.InstanceIdPrefix); return Pageable.Create(async (continuation, pageSize, cancellation) => { @@ -290,10 +269,7 @@ public override AsyncPageable GetAllInstancesAsync(Orches public override async Task WaitForInstanceStartAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); this.logger.WaitingForInstanceStart(instanceId, getInputsAndOutputs); @@ -320,10 +296,7 @@ public override async Task WaitForInstanceStartAsync( public override async Task WaitForInstanceCompletionAsync( string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) { - if (this.options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.options.EnableEntitySupport, instanceId); this.logger.WaitingForInstanceCompletion(instanceId, getInputsAndOutputs); diff --git a/src/Shared/Core/Validation/Check.cs b/src/Shared/Core/Validation/Check.cs index 7310f5866..76c154749 100644 --- a/src/Shared/Core/Validation/Check.cs +++ b/src/Shared/Core/Validation/Check.cs @@ -95,17 +95,16 @@ public static string NotNullOrEmpty( } /// - /// Checks if the provided string is an entity instance id, throwing if it is. - /// Throws if the conditions are not met. + /// Checks that, if entity support is enabled, the given string is not an entity instance id, and throws an otherwise. /// - /// The string to check. - /// The name of the string for the exception. - public static void NotEntity( - string? argument, [CallerArgumentExpression("argument")] string? name = default) + /// Whether entity support is enabled. + /// The instance id. + /// The name of the argument. + public static void NotEntity(bool entitySupportEnabled, string? instanceId, [CallerArgumentExpression("instanceId")] string? argument = default) { - if (argument?.Length > 0 && argument[0] == '@') + if (entitySupportEnabled && instanceId?.Length > 0 && instanceId[0] == '@') { - throw new ArgumentException("Parameter cannot be an entity instance id", name); + throw new ArgumentException("Instance IDs starting with '@' are reserved for entities, and must not be used for orchestrations, when entity support is enabled.", argument); } } diff --git a/src/Worker/Core/DurableTaskWorkerOptions.cs b/src/Worker/Core/DurableTaskWorkerOptions.cs index 9f6fd026f..bc1e7fc4f 100644 --- a/src/Worker/Core/DurableTaskWorkerOptions.cs +++ b/src/Worker/Core/DurableTaskWorkerOptions.cs @@ -45,10 +45,10 @@ public DataConverter DataConverter } /// - /// Gets or sets a value indicating whether this client supports entities. If true, all instance ids starting with '@' are reserved for entities, + /// Gets or sets a value indicating whether this client should support entities. If true, all instance ids starting with '@' are reserved for entities, /// and validation checks are performed where appropriate. /// - public bool SupportEntities { get; set; } + public bool EnableEntitySupport { get; set; } /// /// Gets or sets the maximum timer interval for the @@ -105,7 +105,7 @@ internal void ApplyTo(DurableTaskWorkerOptions other) // Make sure to keep this up to date as values are added. other.DataConverter = this.DataConverter; other.MaximumTimerInterval = this.MaximumTimerInterval; - other.SupportEntities = this.SupportEntities; + other.EnableEntitySupport = this.EnableEntitySupport; } } } diff --git a/src/Worker/Core/Shims/TaskEntityShim.cs b/src/Worker/Core/Shims/TaskEntityShim.cs index 5fad513b9..3be50b56c 100644 --- a/src/Worker/Core/Shims/TaskEntityShim.cs +++ b/src/Worker/Core/Shims/TaskEntityShim.cs @@ -208,7 +208,7 @@ public override void SignalEntity(EntityInstanceId id, string operationName, obj public override void StartOrchestration(TaskName name, object? input = null, StartOrchestrationOptions? options = null) { - Check.NotEntity(options?.InstanceId); + Check.NotEntity(true, options?.InstanceId); this.operationActions.Add(new StartNewOrchestrationOperationAction() { diff --git a/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs b/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs index 239def994..08a7ae824 100644 --- a/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs +++ b/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs @@ -66,13 +66,13 @@ public override TaskOrchestrationEntityFeature Entities { if (this.entityFeature == null) { - if (this.invocationContext.Options.SupportEntities) + if (this.invocationContext.Options.EnableEntitySupport) { this.entityFeature = new TaskOrchestrationEntityContext(this); } else { - throw new NotSupportedException($"Durable entities are disabled because DurableTaskWorkerOptions.SupportEntities=false"); + throw new NotSupportedException($"Durable entities are disabled because {nameof(DurableTaskWorkerOptions)}.{nameof(DurableTaskWorkerOptions.EnableEntitySupport)}=false"); } } @@ -151,10 +151,7 @@ public override async Task CallSubOrchestratorAsync( => options is SubOrchestrationOptions derived ? derived.InstanceId : null; string instanceId = GetInstanceId(options) ?? this.NewGuid().ToString("N"); - if (this.invocationContext.Options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.invocationContext.Options.EnableEntitySupport, instanceId); // if this orchestration uses entities, first validate that the suborchsestration call is allowed in the current context if (this.entityFeature != null && !this.entityFeature.EntityContext.ValidateSuborchestrationTransition(out string? errorMsg)) @@ -255,10 +252,7 @@ public override Task WaitForExternalEvent(string eventName, CancellationTo /// public override void SendEvent(string instanceId, string eventName, object eventData) { - if (this.invocationContext.Options.SupportEntities) - { - Check.NotEntity(instanceId); - } + Check.NotEntity(this.invocationContext.Options.EnableEntitySupport, instanceId); this.innerContext.SendEvent(new OrchestrationInstance { InstanceId = instanceId }, eventName, eventData); }