-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reserve __temporal prefix #410
Changes from 3 commits
30229d1
b127715
4ecc6c5
ac0697d
5573759
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
{ | ||
"FormattingOptions": { | ||
"EnableEditorConfigSupport": true | ||
}, | ||
"RoslynExtensionsOptions": { | ||
"enableAnalyzersSupport": true | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,11 @@ namespace Temporalio.Runtime | |
/// </remarks> | ||
public sealed class TemporalRuntime | ||
{ | ||
/// <summary> | ||
/// Prefix for reserved handler and definition names. | ||
/// </summary> | ||
internal const string ReservedNamePrefix = "__temporal"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See up here: #410 (comment) Yeah, that's what's written there, but, also that just seems wrong. Probably we should change that spec. |
||
|
||
private static readonly Lazy<TemporalRuntime> LazyDefault = | ||
new(() => new TemporalRuntime(new TemporalRuntimeOptions())); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -722,4 +722,4 @@ public override void Heartbeat(HeartbeatInput input) | |
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -424,4 +424,4 @@ internal void OnTaskCompleted(WorkflowInstance instance, Exception? failureExcep | |
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
using Temporalio.Common; | ||
using Temporalio.Converters; | ||
using Temporalio.Exceptions; | ||
using Temporalio.Runtime; | ||
using Temporalio.Worker.Interceptors; | ||
using Temporalio.Workflows; | ||
|
||
|
@@ -939,7 +940,11 @@ private Task ApplyDoUpdateAsync(DoUpdate update) | |
var updates = mutableUpdates.IsValueCreated ? mutableUpdates.Value : Definition.Updates; | ||
if (!updates.TryGetValue(update.Name, out var updateDefn)) | ||
{ | ||
updateDefn = DynamicUpdate; | ||
// Do not fall back onto dynamic update if using the reserved prefix | ||
if (!update.Name.StartsWith(TemporalRuntime.ReservedNamePrefix)) | ||
{ | ||
updateDefn = DynamicUpdate; | ||
} | ||
if (updateDefn == null) | ||
{ | ||
var knownUpdates = updates.Keys.OrderBy(k => k); | ||
|
@@ -1140,13 +1145,13 @@ private void ApplyQueryWorkflow(QueryWorkflow query) | |
if (query.QueryType == "__stack_trace") | ||
{ | ||
Func<string> getter = GetStackTrace; | ||
queryDefn = WorkflowQueryDefinition.CreateWithoutAttribute( | ||
queryDefn = WorkflowQueryDefinition.CreateWithoutAttributeReservedName( | ||
"__stack_trace", getter); | ||
} | ||
else if (query.QueryType == "__temporal_workflow_metadata") | ||
{ | ||
Func<Api.Sdk.V1.WorkflowMetadata> getter = GetWorkflowMetadata; | ||
queryDefn = WorkflowQueryDefinition.CreateWithoutAttribute( | ||
queryDefn = WorkflowQueryDefinition.CreateWithoutAttributeReservedName( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For these two, we need to not go through interceptors to invoke them. Would be ok if they aren't even "query definitions" and just invoked directly or something (also saves you from having that bypass constructor param for query definition) |
||
"__temporal_workflow_metadata", getter); | ||
} | ||
else | ||
|
@@ -1155,7 +1160,11 @@ private void ApplyQueryWorkflow(QueryWorkflow query) | |
var queries = mutableQueries.IsValueCreated ? mutableQueries.Value : Definition.Queries; | ||
if (!queries.TryGetValue(query.QueryType, out queryDefn)) | ||
{ | ||
queryDefn = DynamicQuery; | ||
// Do not fall back onto dynamic query if using the reserved prefix | ||
if (!query.QueryType.StartsWith(TemporalRuntime.ReservedNamePrefix)) | ||
{ | ||
queryDefn = DynamicQuery; | ||
} | ||
if (queryDefn == null) | ||
{ | ||
var knownQueries = queries.Keys.OrderBy(k => k); | ||
|
@@ -1267,7 +1276,11 @@ private void ApplySignalWorkflow(SignalWorkflow signal) | |
var signals = mutableSignals.IsValueCreated ? mutableSignals.Value : Definition.Signals; | ||
if (!signals.TryGetValue(signal.SignalName, out var signalDefn)) | ||
{ | ||
signalDefn = DynamicSignal; | ||
// Do not fall back onto dynamic signal if using the reserved prefix | ||
if (!signal.SignalName.StartsWith(TemporalRuntime.ReservedNamePrefix)) | ||
{ | ||
signalDefn = DynamicSignal; | ||
} | ||
if (signalDefn == null) | ||
{ | ||
// No definition found, buffer | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1339,4 +1339,4 @@ public static class Unsafe | |
public static bool IsReplaying => Context.IsReplaying; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ | |||||
using System.Collections.Concurrent; | ||||||
using System.Reflection; | ||||||
using System.Threading.Tasks; | ||||||
using Temporalio.Runtime; | ||||||
|
||||||
namespace Temporalio.Workflows | ||||||
{ | ||||||
|
@@ -10,11 +11,31 @@ namespace Temporalio.Workflows | |||||
/// </summary> | ||||||
public class WorkflowQueryDefinition | ||||||
{ | ||||||
/// <summary> | ||||||
/// All known reserved query handler prefixes. | ||||||
/// </summary> | ||||||
internal static readonly string[] ReservedQueryHandlerPrefixes = | ||||||
Sushisource marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
{ | ||||||
TemporalRuntime.ReservedNamePrefix, | ||||||
"__stack_trace", | ||||||
"__enhanced_stack_trace", | ||||||
}; | ||||||
|
||||||
private static readonly ConcurrentDictionary<MethodInfo, WorkflowQueryDefinition> MethodDefinitions = new(); | ||||||
private static readonly ConcurrentDictionary<PropertyInfo, WorkflowQueryDefinition> PropertyDefinitions = new(); | ||||||
|
||||||
private WorkflowQueryDefinition(string? name, string? description, MethodInfo? method, Delegate? del) | ||||||
private WorkflowQueryDefinition(string? name, string? description, MethodInfo? method, Delegate? del, bool bypassReserved = false) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you now remove this constructor parameter for bypassing since it is never true anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woops, thanks |
||||||
{ | ||||||
if (!bypassReserved && name != null) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to save lines, add
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't save all that much if you want to keep the prefix name in the error which I do, but, changed. |
||||||
{ | ||||||
foreach (var reservedQ in ReservedQueryHandlerPrefixes) | ||||||
{ | ||||||
if (name.StartsWith(reservedQ)) | ||||||
{ | ||||||
throw new ArgumentException($"Query handler name {name} cannot start with {reservedQ}"); | ||||||
} | ||||||
} | ||||||
} | ||||||
Name = name; | ||||||
Description = description; | ||||||
Method = method; | ||||||
|
@@ -106,6 +127,21 @@ public static WorkflowQueryDefinition CreateWithoutAttribute( | |||||
return new(name, description, null, del); | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Internal version of <see cref="CreateWithoutAttribute" /> that bypasses reserved name checks. | ||||||
/// </summary> | ||||||
/// <param name="name">Query name. Null for dynamic query.</param> | ||||||
/// <param name="del">Query delegate.</param> | ||||||
/// <param name="description">Optional description. WARNING: This setting is experimental. | ||||||
/// </param> | ||||||
/// <returns>Query definition.</returns> | ||||||
internal static WorkflowQueryDefinition CreateWithoutAttributeReservedName( | ||||||
string name, Delegate del, string? description = null) | ||||||
{ | ||||||
AssertValid(del.Method, dynamic: name == null); | ||||||
return new(name, description, null, del, bypassReserved: true); | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Gets the query name for calling or fail if no attribute or if dynamic. | ||||||
/// </summary> | ||||||
|
@@ -168,4 +204,4 @@ private static void AssertValid(MethodInfo method, bool dynamic) | |||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remember, am a bit curious where in code this was triggered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All over. I think it's just a matter of how the default "low" severity shows up in different lang servers. All the low severity stuff shows up in omnisharp, which can be kinda nice to auto fix some stuff sometimes, but, this one is obviously a little silly.