Special behaviour for temporal prefixes #1644
Conversation
|
In this change, users are prevented from registering signals/updates/queries with reserved names from the handler. Users can still Alternatively, we can do the blocking in |
… test needs to be fixed, need to add behaviour reserving prefixes from workflows, and waiting for default update to be merged to add behaviour preventing default update handler to be called with reserved names
THardy98
force-pushed
the
handle_temporal_builtin_prefixes
branch
from
8170bc5 to
5f5103a
Compare
THardy98
changed the title
THardy98
changed the title
THardy98
changed the title
|
this is blocked on #1640 so we can handle the same prefix cases for updates as well |
| @@ -2528,3 +2534,83 @@ test('Signals/Updates/Activities/Timers - Trace promises completion order - 1.11 | |||
| ); | |||
| } | |||
| }); | |||
|
|
|||
| test('Default query handler fail activations with reserved names - workflowWithDefaultHandlers', async (t) => { | |||
There was a problem hiding this comment.
Why did you fallback to unit tests for default handlers?
| @@ -619,6 +625,8 @@ export class Activator implements ActivationHandler { | |||
| protected queryWorkflowNextHandler({ queryName, args }: QueryInput): Promise<unknown> { | |||
| let fn = this.queryHandlers.get(queryName)?.handler; | |||
| if (fn === undefined && this.defaultQueryHandler !== undefined) { | |||
| // Do not call default query handler with reserved query name. | |||
| throwIfReservedName('query', queryName); | |||
There was a problem hiding this comment.
I think we should not be throwing this way here. Code below is clearly designed to make sure that we are not throwing synchronously. Also, UI expects that we'll answer back with a list of accepted query types.
Actually, I'd just skip the default handler if query name is reserved. We'll still throw below, but we'll then be going the same code path as if that wasn't a reserved type name.
| @@ -797,6 +816,8 @@ export class Activator implements ActivationHandler { | |||
| if (fn) { | |||
| return await fn(...args); | |||
| } else if (this.defaultSignalHandler) { | |||
| // Do not call default signal handler with reserved signal name. | |||
| throwIfReservedName('signal', signalName); | |||
There was a problem hiding this comment.
We definitely shouldn't throw here, as that means that adding reserved signals in the future would make those poison pills for any workers that don't yet know about them if there's a default signal handler registered. In fact, we should simply not enter signalWorkflowNextHandler if that's a reserved name and there is no registered signalHandlers for that name.
What was changed
Special behavior handling reserved temporal prefixes. Prevent users from:
This change makes the reserved metrics names reserved across all entities (workflows, activities, signals, etc..).
Checklist
Some remaining items:
Closes [Feature Request] Special behavior for Temporal built-in prefixes #1599
How was this tested:
unit/integration tests
Any docs updates needed?
I'm not sure.