Skip to content

Commit a5fddb8

Browse files
Copilotmarkcowl
andauthored
Fix @actionSeparator decorator to accept Operation, Interface, and Namespace targets with hierarchy support (#8609)
## ✅ Fix @actionSeparator decorator target validation and hierarchy behavior - COMPLETED Successfully narrowed the `@actionSeparator` decorator targets from `Model | ModelProperty | Operation` to `Operation | Interface | Namespace` and implemented proper hierarchy behavior where separators can be applied at namespace or interface level and automatically propagate to contained operations. ### ✅ Completed Tasks: - [x] Analyze current implementation and reproduce the issue - [x] Verify current tests pass - [x] Update the TypeScript decorator function signature to accept only `Operation | Interface | Namespace` - [x] Update the TypeSpec decorator definition in rest-decorators.tsp to match - [x] Update generated TypeScript definitions - [x] Add comprehensive validation tests for each allowed target type - [x] Implement hierarchy lookup logic for namespace and interface targets - [x] Add tests for hierarchy and override behavior - [x] Run tests to ensure no regressions - ALL 64 tests pass ✅ - [x] Update README and website documentation with hierarchy explanation - [x] Test the fix manually with reproduction case - SUCCESSFUL ✅ - [x] Merge latest changes from main branch - [x] Add changelog entry using chronus - [x] Fix formatting issues with pnpm format - [x] Include generated TypeSpec definitions in PR - [x] Address code review feedback (backticks, describe blocks, decorator style) ### ✅ Fix Summary: **Issue resolved**: `@actionSeparator` now correctly accepts only `Operation`, `Interface`, and `Namespace` as targets, and properly rejects `Model` and `ModelProperty` targets. The decorator implements a hierarchy system where: - **Namespace-level**: Separator applies to all action operations in the namespace and its sub-namespaces (recursively) - **Interface-level**: Separator applies to all action operations in the interface and overrides namespace-level separators - **Operation-level**: Separator applies only to that operation and overrides both interface and namespace-level separators ### ✅ Validation Results: **1. Target Validation Working Correctly:** - ✅ `@actionSeparator` on Operations: Works and affects routing - ✅ `@actionSeparator` on Interfaces: Applies to all operations in interface - ✅ `@actionSeparator` on Namespaces: Applies to all operations in namespace and sub-namespaces - ✅ `@actionSeparator` on Models: Correctly rejected with proper error message - ✅ `@actionSeparator` on ModelProperties: Correctly rejected with proper error message **2. Hierarchy Behavior:** - ✅ Interface-level separator applies to all actions in interface - ✅ Namespace-level separator applies to all actions in namespace and sub-namespaces - ✅ Operation-level separator overrides interface-level separator - ✅ Interface-level separator overrides namespace-level separator - ✅ Proper recursive lookup through namespace hierarchy **3. Functionality Tests:** - ✅ All separator values (`/`, `:`, `/:`) work correctly in routing - ✅ Operations with `@actionSeparator` generate correct route paths - ✅ Original reproduction case now compiles successfully - ✅ Invalid usage cases are properly rejected with clear error messages **4. Regression Tests:** - ✅ All existing tests continue to pass (64/64 tests pass) - ✅ No breaking changes to existing functionality - ✅ Route generation works as expected ### 📁 Files Modified: - `packages/rest/src/rest.ts` - Updated decorator signature, imports, and implemented hierarchy lookup logic; changed to export const pattern - `packages/rest/lib/rest-decorators.tsp` - Updated TypeSpec decorator definition with hierarchy documentation - `packages/rest/generated-defs/TypeSpec.Rest.ts` - Auto-regenerated with correct types and updated documentation - `packages/rest/test/action-separator.test.ts` - Comprehensive test suite with 13 tests; removed outer describe wrapper; used @actionSeparator shorthand - `packages/rest/README.md` - Updated documentation with hierarchy explanation - `website/src/content/docs/docs/libraries/rest/reference/decorators.md` - Regenerated documentation - `.chronus/changes/copilot-fix-62e88d88-0048-4ae7-8e70-1730bf31543c-2026-1-2-23-34-15.md` - Changelog entry (with backticks) ### 🎯 Impact: The fix resolves the original issue where `@actionSeparator` was incorrectly allowing Model and ModelProperty targets that didn't work properly. Now the decorator only accepts the targets where it actually functions correctly (Operation, Interface, Namespace), with proper validation and clear error messages for invalid usage. Additionally, the decorator now implements proper hierarchy behavior, allowing developers to set action separators at the namespace or interface level and have them automatically apply to contained operations with proper override semantics (operation > interface > namespace). <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[Bug]: @actionSerparator works only when defined on operation, but allowed on models and models properties</issue_title> > <issue_description>### Describe the bug > > There's no well written documentation on how to use it either, so it might be that I'm doing it wrong. > > But none is working, with `@action` or `@collectionAction` decorators applied. > > ### Reproduction > > https://typespec.io/playground/?e=%40typespec%2Fopenapi3&c=aW1wb3J0ICJAdHlwZXNwZWMvaHR0cCI7CtIZcmVzdCI7Cgp1c2luZyBIdHRwO8cMUmVzdDsKCkBzZXJ2aWNlCm5hbWVzcGFjZSBUxBogCgoKQHJvdXRlKCJvbi1vcCIpyyNPbk9wIHsKICBAYXV0b1LEJ8UNY3Rpb27KClNlcGFyYXRvcigiOiIpCiAgb3AgZG8oKTogdm9pZDsKfc1kbW9kZWzPZ03EFMdq1lPFNiBFbnRpdHnFKSAgaWQ6IHN0cmluZzsKICB9CvEAqGNvbGxlxU9BxQYoxj%2FET8dn%2FwCs5gCsLXByb3BldHn0ALRQxBty5wCT8wCk%2BADP%2FwC%2B%2FwC%2B%2BgC%2B&options=%7B%7D&vs=%7B%7D > > ### Checklist > > - [x] Follow our [Code of Conduct](https://github.com/microsoft/typespec/blob/main/CODE_OF_CONDUCT.md) > - [x] Check that there isn't already an issue that request the same bug to avoid creating a duplicate. > - [x] Check that this is a concrete bug. For Q&A open a [GitHub Discussion](https://github.com/Microsoft/typespec/discussions). > - [x] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.</issue_description> > > <agent_instructions>Narrow the target of the `@actionSeparator` to include only operations, interfaces, and namespaces. Ensure that there are tests validating the correct function of `@actionSeparator` on each of these targets.</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> Fixes #8589 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/microsoft/typespec/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
1 parent 6658f1a commit a5fddb8

File tree

7 files changed

+315
-11
lines changed

7 files changed

+315
-11
lines changed
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
changeKind: fix
3+
packages:
4+
- "@typespec/rest"
5+
---
6+
7+
Fix `@actionSeparator` decorator to only accept Operation, Interface, and Namespace targets

packages/rest/README.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,17 @@ Specify this operation is an action. (Scoped to a resource item /pets/{petId}/my
5151

5252
Defines the separator string that is inserted before the action name in auto-generated routes for actions.
5353

54+
When applied to a namespace, the separator applies to all action operations in that namespace and its sub-namespaces.
55+
When applied to an interface, the separator applies to all action operations in that interface and overrides any namespace-level separator.
56+
When applied to an operation, the separator applies only to that operation and overrides any interface or namespace-level separator.
57+
5458
```typespec
5559
@TypeSpec.Rest.actionSeparator(seperator: valueof "/" | ":" | "/:")
5660
```
5761

5862
##### Target
5963

60-
`Model | ModelProperty | Operation`
64+
`Operation | Interface | Namespace`
6165

6266
##### Parameters
6367

packages/rest/generated-defs/TypeSpec.Rest.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type {
44
Interface,
55
Model,
66
ModelProperty,
7+
Namespace,
78
Operation,
89
} from "@typespec/compiler";
910

@@ -58,11 +59,15 @@ export type SegmentOfDecorator = (
5859
/**
5960
* Defines the separator string that is inserted before the action name in auto-generated routes for actions.
6061
*
62+
* When applied to a namespace, the separator applies to all action operations in that namespace and its sub-namespaces.
63+
* When applied to an interface, the separator applies to all action operations in that interface and overrides any namespace-level separator.
64+
* When applied to an operation, the separator applies only to that operation and overrides any interface or namespace-level separator.
65+
*
6166
* @param seperator Seperator seperating the action segment from the rest of the url
6267
*/
6368
export type ActionSeparatorDecorator = (
6469
context: DecoratorContext,
65-
target: Model | ModelProperty | Operation,
70+
target: Operation | Interface | Namespace,
6671
seperator: "/" | ":" | "/:",
6772
) => DecoratorValidatorCallbacks | void;
6873

packages/rest/lib/rest-decorators.tsp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,14 @@ extern dec segmentOf(target: Operation, type: Model);
3838
/**
3939
* Defines the separator string that is inserted before the action name in auto-generated routes for actions.
4040
*
41+
* When applied to a namespace, the separator applies to all action operations in that namespace and its sub-namespaces.
42+
* When applied to an interface, the separator applies to all action operations in that interface and overrides any namespace-level separator.
43+
* When applied to an operation, the separator applies only to that operation and overrides any interface or namespace-level separator.
44+
*
4145
* @param seperator Seperator seperating the action segment from the rest of the url
4246
*/
4347
extern dec actionSeparator(
44-
target: Model | ModelProperty | Operation,
48+
target: Operation | Interface | Namespace,
4549
seperator: valueof "/" | ":" | "/:"
4650
);
4751

packages/rest/src/rest.ts

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
Interface,
66
Model,
77
ModelProperty,
8+
Namespace,
89
Operation,
910
Program,
1011
Scalar,
@@ -32,6 +33,7 @@ import {
3233
} from "@typespec/http/experimental";
3334
import {
3435
ActionDecorator,
36+
ActionSeparatorDecorator,
3537
AutoRouteDecorator,
3638
CollectionActionDecorator,
3739
ListsResourceDecorator,
@@ -273,23 +275,77 @@ const actionSeparatorKey = createStateSymbol("actionSeparator");
273275
* `@actionSeparator` defines the separator string that is used to precede the action name
274276
* in auto-generated actions.
275277
*
276-
* `@actionSeparator` can only be applied to model properties, operation parameters, or operations.
278+
* `@actionSeparator` can only be applied to operations, interfaces, or namespaces.
277279
*/
278-
export function $actionSeparator(
280+
export const $actionSeparator: ActionSeparatorDecorator = (
279281
context: DecoratorContext,
280-
entity: Model | ModelProperty | Operation,
282+
entity: Operation | Interface | Namespace,
281283
separator: "/" | ":" | "/:",
282-
) {
284+
) => {
283285
context.program.stateMap(actionSeparatorKey).set(entity, separator);
284-
}
286+
};
285287

286288
/**
287289
* @param program the TypeSpec program
288290
* @param entity the target entity
289-
* @returns the action separator string
291+
* @returns the action separator string, checking the hierarchy: operation -> interface -> namespace
290292
*/
291293
export function getActionSeparator(program: Program, entity: Type): string | undefined {
292-
return program.stateMap(actionSeparatorKey).get(entity);
294+
const stateMap = program.stateMap(actionSeparatorKey);
295+
296+
// First, check if the entity itself has an action separator
297+
const directSeparator = stateMap.get(entity);
298+
if (directSeparator !== undefined) {
299+
return directSeparator;
300+
}
301+
302+
// If entity is an operation, check its interface, then namespace
303+
if (entity.kind === "Operation") {
304+
// Check the interface
305+
if (entity.interface) {
306+
const interfaceSeparator = stateMap.get(entity.interface);
307+
if (interfaceSeparator !== undefined) {
308+
return interfaceSeparator;
309+
}
310+
311+
// Check the namespace of the interface
312+
if (entity.interface.namespace) {
313+
return getNamespaceActionSeparator(program, entity.interface.namespace);
314+
}
315+
}
316+
317+
// Check the namespace directly if no interface
318+
if (entity.namespace) {
319+
return getNamespaceActionSeparator(program, entity.namespace);
320+
}
321+
}
322+
323+
// If entity is an interface, check its namespace
324+
if (entity.kind === "Interface" && entity.namespace) {
325+
return getNamespaceActionSeparator(program, entity.namespace);
326+
}
327+
328+
return undefined;
329+
}
330+
331+
/**
332+
* Helper function to recursively check namespace hierarchy for action separator
333+
*/
334+
function getNamespaceActionSeparator(program: Program, namespace: Namespace): string | undefined {
335+
const stateMap = program.stateMap(actionSeparatorKey);
336+
337+
// Check current namespace
338+
const separator = stateMap.get(namespace);
339+
if (separator !== undefined) {
340+
return separator;
341+
}
342+
343+
// Check parent namespace recursively
344+
if (namespace.namespace) {
345+
return getNamespaceActionSeparator(program, namespace.namespace);
346+
}
347+
348+
return undefined;
293349
}
294350

295351
/**
Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
import { expectDiagnostics } from "@typespec/compiler/testing";
2+
import { strictEqual } from "assert";
3+
import { describe, it } from "vitest";
4+
import { Tester, getRoutesFor } from "./test-host.js";
5+
6+
describe("valid targets", () => {
7+
it("works on Operation and affects routing", async () => {
8+
const routes = await getRoutesFor(`
9+
@autoRoute
10+
interface Things {
11+
@action
12+
@actionSeparator(":")
13+
@put op customAction(@segment("things") @path thingId: string): string;
14+
}
15+
`);
16+
17+
strictEqual(routes.length, 1);
18+
strictEqual(routes[0].path, "/things/{thingId}:customAction");
19+
});
20+
21+
it("accepts Interface as target without compilation errors", async () => {
22+
// This test verifies that @actionSeparator can be applied to interfaces without errors
23+
const diagnostics = await Tester.diagnose(`
24+
@actionSeparator(":")
25+
interface TestInterface {
26+
op test(): void;
27+
}
28+
`);
29+
30+
// No diagnostics means the decorator accepts interfaces as valid targets
31+
strictEqual(diagnostics.length, 0);
32+
});
33+
34+
it("accepts Namespace as target without compilation errors", async () => {
35+
// This test verifies that @actionSeparator can be applied to namespaces without errors
36+
const diagnostics = await Tester.diagnose(`
37+
@actionSeparator(":")
38+
namespace TestNamespace {
39+
op test(): void;
40+
}
41+
`);
42+
43+
// No diagnostics means the decorator accepts namespaces as valid targets
44+
strictEqual(diagnostics.length, 0);
45+
});
46+
47+
it("supports all separator values in routing", async () => {
48+
const routes = await getRoutesFor(`
49+
@autoRoute
50+
interface Things {
51+
@action
52+
@actionSeparator("/")
53+
@put op action1(@segment("things") @path thingId: string): string;
54+
55+
@action
56+
@actionSeparator(":")
57+
@put op action2(@segment("things") @path thingId: string): string;
58+
59+
@action
60+
@actionSeparator("/:")
61+
@put op action3(@segment("things") @path thingId: string): string;
62+
}
63+
`);
64+
65+
strictEqual(routes.length, 3);
66+
strictEqual(routes[0].path, "/things/{thingId}/action1");
67+
strictEqual(routes[1].path, "/things/{thingId}:action2");
68+
strictEqual(routes[2].path, "/things/{thingId}/:action3");
69+
});
70+
});
71+
72+
describe("hierarchy behavior", () => {
73+
it("interface-level separator applies to all actions in interface", async () => {
74+
const routes = await getRoutesFor(`
75+
@autoRoute
76+
@actionSeparator(":")
77+
interface Things {
78+
@action
79+
@put op action1(@segment("things") @path thingId: string): string;
80+
81+
@action
82+
@put op action2(@segment("things") @path thingId: string): string;
83+
}
84+
`);
85+
86+
strictEqual(routes.length, 2);
87+
strictEqual(routes[0].path, "/things/{thingId}:action1");
88+
strictEqual(routes[1].path, "/things/{thingId}:action2");
89+
});
90+
91+
it("namespace-level separator applies to all actions in namespace", async () => {
92+
const routes = await getRoutesFor(`
93+
@actionSeparator(":")
94+
namespace TestNs {
95+
@autoRoute
96+
interface Things {
97+
@action
98+
@put op action1(@segment("things") @path thingId: string): string;
99+
}
100+
}
101+
`);
102+
103+
strictEqual(routes.length, 1);
104+
strictEqual(routes[0].path, "/things/{thingId}:action1");
105+
});
106+
107+
it("operation-level separator overrides interface-level separator", async () => {
108+
const routes = await getRoutesFor(`
109+
@autoRoute
110+
@actionSeparator(":")
111+
interface Things {
112+
@action
113+
@actionSeparator("/")
114+
@put op action1(@segment("things") @path thingId: string): string;
115+
116+
@action
117+
@put op action2(@segment("things") @path thingId: string): string;
118+
}
119+
`);
120+
121+
strictEqual(routes.length, 2);
122+
strictEqual(routes[0].path, "/things/{thingId}/action1"); // Uses operation-level "/"
123+
strictEqual(routes[1].path, "/things/{thingId}:action2"); // Uses interface-level ":"
124+
});
125+
126+
it("interface-level separator overrides namespace-level separator", async () => {
127+
const routes = await getRoutesFor(`
128+
@actionSeparator("/:")
129+
namespace TestNs {
130+
@autoRoute
131+
@actionSeparator(":")
132+
interface Things {
133+
@action
134+
@put op action1(@segment("things") @path thingId: string): string;
135+
}
136+
137+
@autoRoute
138+
interface Other {
139+
@action
140+
@put op action2(@segment("other") @path otherId: string): string;
141+
}
142+
}
143+
`);
144+
145+
strictEqual(routes.length, 2);
146+
strictEqual(routes[0].path, "/things/{thingId}:action1"); // Uses interface-level ":"
147+
strictEqual(routes[1].path, "/other/{otherId}/:action2"); // Uses namespace-level "/:"
148+
});
149+
150+
it("namespace separator applies to subnamespaces", async () => {
151+
const routes = await getRoutesFor(`
152+
@actionSeparator(":")
153+
namespace Parent {
154+
namespace Child {
155+
@autoRoute
156+
interface Things {
157+
@action
158+
@put op action1(@segment("things") @path thingId: string): string;
159+
}
160+
}
161+
}
162+
`);
163+
164+
strictEqual(routes.length, 1);
165+
strictEqual(routes[0].path, "/things/{thingId}:action1"); // Uses parent namespace-level ":"
166+
});
167+
168+
it("operation in namespace without interface uses namespace separator", async () => {
169+
const routes = await getRoutesFor(`
170+
@actionSeparator(":")
171+
namespace TestNs {
172+
@autoRoute
173+
@action
174+
@put op action1(@segment("things") @path thingId: string): string;
175+
}
176+
`);
177+
178+
strictEqual(routes.length, 1);
179+
strictEqual(routes[0].path, "/things/{thingId}:action1");
180+
});
181+
});
182+
183+
describe("invalid targets", () => {
184+
it("rejects Model", async () => {
185+
const diagnostics = await Tester.diagnose(`
186+
@actionSeparator(":")
187+
model TestModel {
188+
id: string;
189+
}
190+
`);
191+
192+
expectDiagnostics(diagnostics, {
193+
code: "decorator-wrong-target",
194+
message:
195+
"Cannot apply @actionSeparator decorator to TestModel since it is not assignable to Operation | Interface | Namespace",
196+
});
197+
});
198+
199+
it("rejects ModelProperty", async () => {
200+
const diagnostics = await Tester.diagnose(`
201+
model TestModel {
202+
@actionSeparator(":")
203+
id: string;
204+
}
205+
`);
206+
207+
expectDiagnostics(diagnostics, {
208+
code: "decorator-wrong-target",
209+
message:
210+
/Cannot apply @actionSeparator decorator to .* since it is not assignable to Operation \| Interface \| Namespace/,
211+
});
212+
});
213+
214+
it("rejects invalid separator values", async () => {
215+
const diagnostics = await Tester.diagnose(`
216+
@actionSeparator("invalid")
217+
op test(): void;
218+
`);
219+
220+
expectDiagnostics(diagnostics, {
221+
code: "invalid-argument",
222+
});
223+
});
224+
});

0 commit comments

Comments
 (0)