Skip to content

Commit 1d5ae0d

Browse files
committed
feat: Enhance TerminateInstanceOptions with type guards and factory function
1 parent 7faedb1 commit 1d5ae0d

4 files changed

Lines changed: 223 additions & 9 deletions

File tree

packages/durabletask-js/src/client/client.ts

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { TimeoutError } from "../exception/timeout-error";
1818
import { PurgeResult } from "../orchestration/orchestration-purge-result";
1919
import { PurgeInstanceCriteria } from "../orchestration/orchestration-purge-criteria";
2020
import { PurgeInstanceOptions } from "../orchestration/orchestration-purge-options";
21-
import { TerminateInstanceOptions } from "../orchestration/orchestration-terminate-options";
21+
import { TerminateInstanceOptions, isTerminateInstanceOptions } from "../orchestration/orchestration-terminate-options";
2222
import { callWithMetadata, MetadataGenerator } from "../utils/grpc-helper.util";
2323
import { OrchestrationQuery, ListInstanceIdsOptions, DEFAULT_PAGE_SIZE } from "../orchestration/orchestration-query";
2424
import { Page, AsyncPageable, createAsyncPageable } from "../orchestration/page";
@@ -390,7 +390,21 @@ export class TaskHubGrpcClient {
390390
*
391391
* @param {string} instanceId - orchestrator instance id to terminate.
392392
* @param {any | TerminateInstanceOptions} outputOrOptions - The optional output to set for the terminated orchestrator instance,
393-
* or an options object that can include both output and recursive termination settings.
393+
* or a TerminateInstanceOptions object created with `terminateOptions()` that can include both
394+
* output and recursive termination settings.
395+
*
396+
* @example
397+
* ```typescript
398+
* // Simple termination with output
399+
* await client.terminateOrchestration(instanceId, { reason: "cancelled" });
400+
*
401+
* // Recursive termination with options (use terminateOptions helper)
402+
* import { terminateOptions } from "@microsoft/durabletask-js";
403+
* await client.terminateOrchestration(instanceId, terminateOptions({
404+
* output: { reason: "cancelled" },
405+
* recursive: true
406+
* }));
407+
* ```
394408
*/
395409
async terminateOrchestration(
396410
instanceId: string,
@@ -402,12 +416,9 @@ export class TaskHubGrpcClient {
402416
let output: any = null;
403417
let recursive = false;
404418

405-
// Check if outputOrOptions is a TerminateInstanceOptions object
406-
if (
407-
outputOrOptions !== null &&
408-
typeof outputOrOptions === "object" &&
409-
("recursive" in outputOrOptions || "output" in outputOrOptions)
410-
) {
419+
// Use type guard to safely detect TerminateInstanceOptions
420+
// This avoids false positives when user output happens to have 'recursive' or 'output' properties
421+
if (isTerminateInstanceOptions(outputOrOptions)) {
411422
output = outputOrOptions.output ?? null;
412423
recursive = outputOrOptions.recursive ?? false;
413424
} else {

packages/durabletask-js/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export { ActivityContext } from "./task/context/activity-context";
1313
// Orchestration types and utilities
1414
export { PurgeInstanceCriteria } from "./orchestration/orchestration-purge-criteria";
1515
export { PurgeInstanceOptions } from "./orchestration/orchestration-purge-options";
16-
export { TerminateInstanceOptions } from "./orchestration/orchestration-terminate-options";
16+
export { TerminateInstanceOptions, terminateOptions, isTerminateInstanceOptions, TERMINATE_OPTIONS_SYMBOL } from "./orchestration/orchestration-terminate-options";
1717
export { OrchestrationStatus } from "./orchestration/enum/orchestration-status.enum";
1818
export { OrchestrationState } from "./orchestration/orchestration-state";
1919

packages/durabletask-js/src/orchestration/orchestration-terminate-options.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,22 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4+
/**
5+
* Symbol used to identify TerminateInstanceOptions objects.
6+
* This prevents confusion when user output happens to have 'recursive' or 'output' properties.
7+
*/
8+
export const TERMINATE_OPTIONS_SYMBOL = Symbol.for("durabletask.TerminateInstanceOptions");
9+
410
/**
511
* Options for terminating orchestration instances.
612
*/
713
export interface TerminateInstanceOptions {
14+
/**
15+
* Internal marker to identify this as a TerminateInstanceOptions object.
16+
* @internal
17+
*/
18+
readonly [TERMINATE_OPTIONS_SYMBOL]?: true;
19+
820
/**
921
* Whether to recursively terminate sub-orchestrations as well.
1022
* When true, all child orchestrations spawned by the target orchestration
@@ -19,3 +31,42 @@ export interface TerminateInstanceOptions {
1931
*/
2032
output?: any;
2133
}
34+
35+
/**
36+
* Creates a TerminateInstanceOptions object with proper type identification.
37+
* Use this function instead of creating plain objects to ensure correct behavior.
38+
*
39+
* @param options - The terminate options
40+
* @returns A properly typed TerminateInstanceOptions object
41+
*
42+
* @example
43+
* ```typescript
44+
* // Terminate with recursive option
45+
* await client.terminateOrchestration(instanceId, terminateOptions({ recursive: true }));
46+
*
47+
* // Terminate with output and recursive
48+
* await client.terminateOrchestration(instanceId, terminateOptions({
49+
* output: { reason: "cancelled by user" },
50+
* recursive: true
51+
* }));
52+
* ```
53+
*/
54+
export function terminateOptions(options: Omit<TerminateInstanceOptions, typeof TERMINATE_OPTIONS_SYMBOL>): TerminateInstanceOptions {
55+
return {
56+
...options,
57+
[TERMINATE_OPTIONS_SYMBOL]: true as const,
58+
};
59+
}
60+
61+
/**
62+
* Type guard to check if a value is a TerminateInstanceOptions object.
63+
* @internal
64+
*/
65+
export function isTerminateInstanceOptions(value: unknown): value is TerminateInstanceOptions {
66+
return (
67+
value !== null &&
68+
typeof value === "object" &&
69+
TERMINATE_OPTIONS_SYMBOL in value &&
70+
(value as TerminateInstanceOptions)[TERMINATE_OPTIONS_SYMBOL] === true
71+
);
72+
}
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
import {
5+
terminateOptions,
6+
isTerminateInstanceOptions,
7+
TERMINATE_OPTIONS_SYMBOL,
8+
} from "../src";
9+
10+
describe("TerminateInstanceOptions", () => {
11+
describe("terminateOptions factory function", () => {
12+
it("should create options with recursive flag", () => {
13+
const options = terminateOptions({ recursive: true });
14+
15+
expect(options.recursive).toBe(true);
16+
expect(isTerminateInstanceOptions(options)).toBe(true);
17+
});
18+
19+
it("should create options with output", () => {
20+
const options = terminateOptions({ output: { reason: "cancelled" } });
21+
22+
expect(options.output).toEqual({ reason: "cancelled" });
23+
expect(isTerminateInstanceOptions(options)).toBe(true);
24+
});
25+
26+
it("should create options with both recursive and output", () => {
27+
const options = terminateOptions({
28+
recursive: true,
29+
output: { reason: "cancelled" },
30+
});
31+
32+
expect(options.recursive).toBe(true);
33+
expect(options.output).toEqual({ reason: "cancelled" });
34+
expect(isTerminateInstanceOptions(options)).toBe(true);
35+
});
36+
37+
it("should create options with empty object", () => {
38+
const options = terminateOptions({});
39+
40+
expect(options.recursive).toBeUndefined();
41+
expect(options.output).toBeUndefined();
42+
expect(isTerminateInstanceOptions(options)).toBe(true);
43+
});
44+
});
45+
46+
describe("isTerminateInstanceOptions type guard", () => {
47+
it("should return true for options created with terminateOptions()", () => {
48+
const options = terminateOptions({ recursive: true });
49+
expect(isTerminateInstanceOptions(options)).toBe(true);
50+
});
51+
52+
it("should return false for plain objects with recursive property", () => {
53+
// This is the key test - user output that happens to have 'recursive' property
54+
const userOutput = { recursive: true, data: "some value" };
55+
expect(isTerminateInstanceOptions(userOutput)).toBe(false);
56+
});
57+
58+
it("should return false for plain objects with output property", () => {
59+
// User output that happens to have 'output' property
60+
const userOutput = { output: "some value", status: "done" };
61+
expect(isTerminateInstanceOptions(userOutput)).toBe(false);
62+
});
63+
64+
it("should return false for plain objects with both recursive and output properties", () => {
65+
// User output that coincidentally has both properties
66+
const userOutput = { recursive: true, output: "some data", extra: "field" };
67+
expect(isTerminateInstanceOptions(userOutput)).toBe(false);
68+
});
69+
70+
it("should return false for null", () => {
71+
expect(isTerminateInstanceOptions(null)).toBe(false);
72+
});
73+
74+
it("should return false for undefined", () => {
75+
expect(isTerminateInstanceOptions(undefined)).toBe(false);
76+
});
77+
78+
it("should return false for primitives", () => {
79+
expect(isTerminateInstanceOptions("string")).toBe(false);
80+
expect(isTerminateInstanceOptions(123)).toBe(false);
81+
expect(isTerminateInstanceOptions(true)).toBe(false);
82+
});
83+
84+
it("should return false for arrays", () => {
85+
expect(isTerminateInstanceOptions([1, 2, 3])).toBe(false);
86+
expect(isTerminateInstanceOptions([{ recursive: true }])).toBe(false);
87+
});
88+
89+
it("should return false for arbitrary objects", () => {
90+
expect(isTerminateInstanceOptions({ foo: "bar" })).toBe(false);
91+
expect(isTerminateInstanceOptions({ status: "done", data: {} })).toBe(false);
92+
});
93+
94+
it("should return false for objects that look like options but lack the symbol", () => {
95+
// This simulates the old detection bug - objects with matching property names
96+
const fakeOptions = { recursive: true, output: { reason: "test" } };
97+
expect(isTerminateInstanceOptions(fakeOptions)).toBe(false);
98+
});
99+
});
100+
101+
describe("TERMINATE_OPTIONS_SYMBOL", () => {
102+
it("should be a symbol", () => {
103+
expect(typeof TERMINATE_OPTIONS_SYMBOL).toBe("symbol");
104+
});
105+
106+
it("should be consistent across imports (Symbol.for)", () => {
107+
const symbol1 = Symbol.for("durabletask.TerminateInstanceOptions");
108+
const symbol2 = Symbol.for("durabletask.TerminateInstanceOptions");
109+
expect(symbol1).toBe(symbol2);
110+
expect(symbol1).toBe(TERMINATE_OPTIONS_SYMBOL);
111+
});
112+
113+
it("should be present in options created with terminateOptions()", () => {
114+
const options = terminateOptions({ recursive: true });
115+
expect(TERMINATE_OPTIONS_SYMBOL in options).toBe(true);
116+
expect(options[TERMINATE_OPTIONS_SYMBOL]).toBe(true);
117+
});
118+
});
119+
120+
describe("edge cases for user output disambiguation", () => {
121+
it("should treat plain object with recursive:true as output, not options", () => {
122+
// This was the original bug - user passing output like {recursive: true, data: {...}}
123+
const userOutput = { recursive: true, data: { value: 123 } };
124+
125+
// Should NOT be detected as TerminateInstanceOptions
126+
expect(isTerminateInstanceOptions(userOutput)).toBe(false);
127+
128+
// When used with terminateOptions, it works correctly
129+
const options = terminateOptions({ output: userOutput, recursive: false });
130+
expect(isTerminateInstanceOptions(options)).toBe(true);
131+
expect(options.output).toEqual(userOutput);
132+
});
133+
134+
it("should correctly handle nested objects with problematic property names", () => {
135+
const complexOutput = {
136+
recursive: true,
137+
output: "nested",
138+
metadata: {
139+
recursive: false,
140+
output: "deeply nested"
141+
}
142+
};
143+
144+
// Plain object should not be detected as options
145+
expect(isTerminateInstanceOptions(complexOutput)).toBe(false);
146+
147+
// Wrapped in terminateOptions should work
148+
const options = terminateOptions({ output: complexOutput });
149+
expect(isTerminateInstanceOptions(options)).toBe(true);
150+
});
151+
});
152+
});

0 commit comments

Comments
 (0)