Revise EntityInstanceId implementation and add checking#205
Conversation
sebastianburckhardt
added
P1
durable-entities
jviau
left a comment
jviau
left a comment
There was a problem hiding this comment.
The Check.NotEntity addition is a behavior breaking change. We should discuss what impact this is to customers and if we have any alternatives.
| /// </summary> | ||
| /// <param name="instanceId">The string representation of the entity ID.</param> | ||
| /// <returns>the constructed entity instance ID.</returns> | ||
| public static EntityInstanceId FromString(string instanceId) |
There was a problem hiding this comment.
nit: put static methods before instance.
src/Abstractions/TaskOptions.cs
Outdated
| public SubOrchestrationOptions(TaskRetryOptions? retry = null, string? instanceId = null) | ||
| : base(retry) | ||
| { | ||
| Check.NotEntity(instanceId); |
There was a problem hiding this comment.
This is a behavior breaking change. Customers scheduling orchestrations with @ before no longer can.
There was a problem hiding this comment.
Right. Note that it does not break just because of this check. The backend also treats '@' instance ids differently. So the check is actually necessary for giving good error messages as opposed to mysterious internal errors.
There was a problem hiding this comment.
Do we want to support an 'entity-opt-out' switch?
There was a problem hiding this comment.
I suppose we could - would be a good use of AppContext. Default is opted in, add way to opt out.
…associated checking
| /// 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. | ||
| /// </summary> | ||
| public bool SupportEntities { get; set; } |
There was a problem hiding this comment.
What do you think of EnableEntitySupport?
There was a problem hiding this comment.
yes, slightly better I think.
There was a problem hiding this comment.
I have changed it for both DurableTaskClientOptions and DurableTaskWorkerOptions.
src/Shared/Core/Validation/Check.cs
Outdated
| { | ||
| if (argument?.Length > 0 && argument[0] == '@') | ||
| { | ||
| throw new ArgumentException("Parameter cannot be an entity instance id", name); |
There was a problem hiding this comment.
nit: should clarify what entity instance ID means:
Orchestration instance IDs cannot start '@' when entity support is enabled. '@' is reserved for entities. Instance ID: '{argument}'.
| public override async Task<OrchestrationMetadata> WaitForInstanceStartAsync( | ||
| string instanceId, bool getInputsAndOutputs = false, CancellationToken cancellation = default) | ||
| { | ||
| if (this.options.SupportEntities) |
There was a problem hiding this comment.
Recommend add a helper to the options object:
internal void CheckNotEntity(string instanceId)
{
if (this.SupportEntities)
{
Check.NotEntity(instanceId);
}
}Then all other calls can be this.options.CheckNotEntity(instanceId);
There was a problem hiding this comment.
I think I also have to add the [CallerArgumentExpression("argument")] string? name = default for the magic of compiler-generated argument descriptor to work.
There was a problem hiding this comment.
Since we need the same thing for DurableTaskWorkerOptions I am instead going to just pass in the EnableEntitySupport as a boolean argument to Check.NotEntity.
| } | ||
| else | ||
| { | ||
| throw new NotSupportedException($"Durable entities are disabled because DurableTaskWorkerOptions.SupportEntities=false"); |
There was a problem hiding this comment.
It is long and ugly, but will save from any renames:
| throw new NotSupportedException($"Durable entities are disabled because DurableTaskWorkerOptions.SupportEntities=false"); | |
| throw new NotSupportedException($"Durable entities are disabled because {nameof(DurableTaskWorkerOptions)}.{nameof(DurableTaskWorkerOptions.SupportEntities)}=false"); |
sebastianburckhardt
deleted the
core-entities/entity-instance-id-normalization
branch
2 participants
Changes to EntityInstanceId:
Changes to parameter checking throughout: