Reserve __temporal prefix #410

Sushisource · 2025-02-09T19:44:05Z

What was changed

See temporalio/features#576

Why?

Future flexibility for us to implement built-in handlers

Checklist

Closes [Feature Request] Special behavior for Temporal built-in prefixes #391
How was this tested:
New tests & test upgrades
Any docs updates needed?

cretz · 2025-02-10T14:53:00Z

.editorconfig

@@ -47,6 +47,9 @@ dotnet_diagnostic.CA2237.severity = none
 # Warn on unused imports
 dotnet_diagnostic.IDE0005.severity = warning

+# Don't warn on using var
+dotnet_diagnostic.IDE0008.severity = none


If you remember, am a bit curious where in code this was triggered

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.

cretz · 2025-02-10T14:55:11Z

src/Temporalio/Worker/WorkflowInstance.cs

+        /// This interceptor ensures that handlers using the reserved prefix bypass user
+        /// interceptors.
+        /// </summary>
+        private class InboundReservedPrefixInterceptor : WorkflowInboundInterceptor


I do not believe this should be implemented as an interceptor/wrapper that now becomes the outer interceptor always just for this small reason. I believe in ApplyQueryWorkflow we need to just make the two reserved queries call query logic directly without going through the interceptor. And it is there we should fail if it's reserved and not one of those. Can also fail update and signal in their apply-level places too.

Sure, I did this because it would be robust in the face of different ways to handle built-in operations. If you're not worried about that then I'll get rid of it.

cretz · 2025-02-10T15:01:31Z

src/Temporalio/Workflows/WorkflowDefinition.cs

+            void CheckNamesForPrefix(IEnumerable<string> keys, string type)
+            {
+                foreach (var key in keys)
+                {
+                    if (key.StartsWith(Constants.ReservedNamePrefix))
+                    {
+                        errs.Add($"{type} handler name {key} cannot start with {Constants.ReservedNamePrefix}");
+                    }
+                    else if (type == "Query")
+                    {
+                        foreach (var reservedQ in Workflow.ReservedQueryHandlerPrefixes)
+                        {
+                            if (key.StartsWith(reservedQ))
+                            {
+                                errs.Add($"{type} handler name {key} cannot start with {reservedQ}");
+                            }
+                        }
+                    }
+                }
+            }
+            CheckNamesForPrefix(signals.Keys, "Signal");
+            CheckNamesForPrefix(queries.Keys, "Query");
+            CheckNamesForPrefix(updates.Keys, "Update");


I think you can just check signal name prefix in signal definition, update in update definition, and query in query definition. It is clearer to keep validation with the definition. Can just do it in those constructors if you want.

The downside of this is now you need a specific bypassing constructor -- see my last commit

cretz · 2025-02-10T15:02:38Z

src/Temporalio/Common/Constants.cs

+        /// <summary>
+        /// Prefix for reserved handler and definition names.
+        /// </summary>
+        internal const string ReservedNamePrefix = "__temporal";


If you can think of any other class, would be ok moving there (e.g. even TemporalRuntime since it's internal) instead of a whole new class/file for a single constant.

Well there's always going to be something that'd be the first reason to have a common place for constants - no where else felt like an accurate fit, but I can move it to Runtime.

src/Temporalio/Workflows/Workflow.cs

cretz · 2025-02-10T16:07:04Z

src/Temporalio/Common/Constants.cs

+        /// <summary>
+        /// Prefix for reserved handler and definition names.
+        /// </summary>
+        internal const string ReservedNamePrefix = "__temporal";


Technically __temporal_, but I am ok with this without trailing underscore

I think we should probably actually make it specifically be without that. Seems very odd you could register __temporalmything but not __temporal_mything

Yeah, was unsure here because in spec I added underscore not thinking and other SDKs already merged theirs, though I doubt an inconsistency here matters.

cretz · 2025-02-10T22:29:09Z

src/Temporalio/Worker/WorkflowInstance.cs

+                        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(


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)

cretz · 2025-02-10T22:31:38Z

src/Temporalio/Workflows/WorkflowQueryDefinition.cs

        {
+            if (!bypassReserved && name != null)


If you want to save lines, add using System.Linq; import and then:

Suggested change

if (!bypassReserved && name != null)

if (!bypassReserved && name != null && ReservedQueryHandlerPrefixes.Any(p => name.StartsWith(p))

Don't save all that much if you want to keep the prefix name in the error which I do, but, changed.

src/Temporalio/Workflows/WorkflowQueryDefinition.cs

cretz

LGTM, one minor change about removing constructor param

cretz · 2025-02-11T14:01:48Z

src/Temporalio/Workflows/WorkflowQueryDefinition.cs

        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)


Can you now remove this constructor parameter for bypassing since it is never true anymore?

Woops, thanks

Quinn-With-Two-Ns · 2025-02-11T17:20:21Z

src/Temporalio/Runtime/TemporalRuntime.cs

+        /// <summary>
+        /// Prefix for reserved handler and definition names.
+        /// </summary>
+        internal const string ReservedNamePrefix = "__temporal";


Shouldn't it be __temporal_ per temporalio/features#576

See up here: #410 (comment)

Yeah, that's what's written there, but, also that just seems wrong. Probably we should change that spec.

Sushisource force-pushed the reserved-prefixes branch from ec1d8de to 0d4b240 Compare February 9, 2025 19:54

Deal with reserved __temporal prefix

30229d1

Sushisource force-pushed the reserved-prefixes branch from 0d4b240 to 30229d1 Compare February 9, 2025 20:16

cretz reviewed Feb 10, 2025

View reviewed changes

Sushisource added 2 commits February 10, 2025 13:36

Review comments

b127715

Need a specific bypass constructor for queries now

4ecc6c5

Sushisource force-pushed the reserved-prefixes branch from e59ee58 to 4ecc6c5 Compare February 10, 2025 22:30

cretz reviewed Feb 10, 2025

View reviewed changes

cretz reviewed Feb 11, 2025

View reviewed changes

Sushisource force-pushed the reserved-prefixes branch from ccf3354 to 0f9795b Compare February 11, 2025 16:52

More review comments

ac0697d

Sushisource force-pushed the reserved-prefixes branch from 0f9795b to ac0697d Compare February 11, 2025 17:16

Quinn-With-Two-Ns reviewed Feb 11, 2025

View reviewed changes

cretz approved these changes Feb 11, 2025

View reviewed changes

Merge branch 'main' into reserved-prefixes

5573759

Sushisource merged commit f2ba7dd into main Feb 11, 2025
9 checks passed

Sushisource deleted the reserved-prefixes branch February 11, 2025 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reserve __temporal prefix #410

Reserve __temporal prefix #410

Sushisource commented Feb 9, 2025

cretz Feb 10, 2025

Sushisource Feb 10, 2025

cretz Feb 10, 2025

Sushisource Feb 10, 2025

cretz Feb 10, 2025 •

edited

Loading

Sushisource Feb 10, 2025

cretz Feb 10, 2025

Sushisource Feb 10, 2025

cretz Feb 10, 2025

Sushisource Feb 10, 2025

cretz Feb 10, 2025

cretz Feb 10, 2025

cretz Feb 10, 2025

Sushisource Feb 11, 2025

cretz left a comment

cretz Feb 11, 2025

Sushisource Feb 11, 2025

Quinn-With-Two-Ns Feb 11, 2025

Sushisource Feb 11, 2025

	if (!bypassReserved && name != null)
	if (!bypassReserved && name != null && ReservedQueryHandlerPrefixes.Any(p => name.StartsWith(p))

Reserve __temporal prefix #410

Reserve __temporal prefix #410

Conversation

Sushisource commented Feb 9, 2025

What was changed

Why?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 10, 2025 •

edited

Loading