Skip to content
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: fix
packages:
- "@typespec/rest"
---

Fix @actionSeparator decorator to only accept Operation, Interface, and Namespace targets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Fix @actionSeparator decorator to only accept Operation, Interface, and Namespace targets
Fix `@actionSeparator` decorator to only accept Operation, Interface, and Namespace targets

6 changes: 5 additions & 1 deletion packages/rest/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,17 @@ Specify this operation is an action. (Scoped to a resource item /pets/{petId}/my

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

When applied to a namespace, the separator applies to all action operations in that namespace and its sub-namespaces.
When applied to an interface, the separator applies to all action operations in that interface and overrides any namespace-level separator.
When applied to an operation, the separator applies only to that operation and overrides any interface or namespace-level separator.

```typespec
@TypeSpec.Rest.actionSeparator(seperator: valueof "/" | ":" | "/:")
```

##### Target

`Model | ModelProperty | Operation`
`Operation | Interface | Namespace`

##### Parameters

Expand Down
7 changes: 6 additions & 1 deletion packages/rest/generated-defs/TypeSpec.Rest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
Interface,
Model,
ModelProperty,
Namespace,
Operation,
} from "@typespec/compiler";

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

Expand Down
6 changes: 5 additions & 1 deletion packages/rest/lib/rest-decorators.tsp
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,14 @@ extern dec segmentOf(target: Operation, type: Model);
/**
* Defines the separator string that is inserted before the action name in auto-generated routes for actions.
*
* When applied to a namespace, the separator applies to all action operations in that namespace and its sub-namespaces.
* When applied to an interface, the separator applies to all action operations in that interface and overrides any namespace-level separator.
* When applied to an operation, the separator applies only to that operation and overrides any interface or namespace-level separator.
*
* @param seperator Seperator seperating the action segment from the rest of the url
*/
extern dec actionSeparator(
target: Model | ModelProperty | Operation,
target: Operation | Interface | Namespace,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it a problem that we don't allow on MOdel and model propert now?

seperator: valueof "/" | ":" | "/:"
);

Expand Down
63 changes: 59 additions & 4 deletions packages/rest/src/rest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
Interface,
Model,
ModelProperty,
Namespace,
Operation,
Program,
Scalar,
Expand Down Expand Up @@ -273,11 +274,11 @@ const actionSeparatorKey = createStateSymbol("actionSeparator");
* `@actionSeparator` defines the separator string that is used to precede the action name
* in auto-generated actions.
*
* `@actionSeparator` can only be applied to model properties, operation parameters, or operations.
* `@actionSeparator` can only be applied to operations, interfaces, or namespaces.
*/
export function $actionSeparator(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoudd that be updated to export const $actionSpearator: ActionSeparatorDecorator = ...

context: DecoratorContext,
entity: Model | ModelProperty | Operation,
entity: Operation | Interface | Namespace,
separator: "/" | ":" | "/:",
) {
context.program.stateMap(actionSeparatorKey).set(entity, separator);
Expand All @@ -286,10 +287,64 @@ export function $actionSeparator(
/**
* @param program the TypeSpec program
* @param entity the target entity
* @returns the action separator string
* @returns the action separator string, checking the hierarchy: operation -> interface -> namespace
*/
export function getActionSeparator(program: Program, entity: Type): string | undefined {
return program.stateMap(actionSeparatorKey).get(entity);
const stateMap = program.stateMap(actionSeparatorKey);

// First, check if the entity itself has an action separator
const directSeparator = stateMap.get(entity);
if (directSeparator !== undefined) {
return directSeparator;
}

// If entity is an operation, check its interface, then namespace
if (entity.kind === "Operation") {
// Check the interface
if (entity.interface) {
const interfaceSeparator = stateMap.get(entity.interface);
if (interfaceSeparator !== undefined) {
return interfaceSeparator;
}

// Check the namespace of the interface
if (entity.interface.namespace) {
return getNamespaceActionSeparator(program, entity.interface.namespace);
}
}

// Check the namespace directly if no interface
if (entity.namespace) {
return getNamespaceActionSeparator(program, entity.namespace);
}
}

// If entity is an interface, check its namespace
if (entity.kind === "Interface" && entity.namespace) {
return getNamespaceActionSeparator(program, entity.namespace);
}

return undefined;
}

/**
* Helper function to recursively check namespace hierarchy for action separator
*/
function getNamespaceActionSeparator(program: Program, namespace: Namespace): string | undefined {
const stateMap = program.stateMap(actionSeparatorKey);

// Check current namespace
const separator = stateMap.get(namespace);
if (separator !== undefined) {
return separator;
}

// Check parent namespace recursively
if (namespace.namespace) {
return getNamespaceActionSeparator(program, namespace.namespace);
}

return undefined;
}

/**
Expand Down
232 changes: 232 additions & 0 deletions packages/rest/test/action-separator.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
import { expectDiagnostics } from "@typespec/compiler/testing";
import { strictEqual } from "assert";
import { describe, it } from "vitest";
import { Tester, getRoutesFor } from "./test-host.js";

describe("rest: @actionSeparator decorator", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need this top level describe

describe("valid targets", () => {
it("works on Operation and affects routing", async () => {
const routes = await getRoutesFor(`
@autoRoute
interface Things {
@action
@TypeSpec.Rest.actionSeparator(":")
@put op customAction(@segment("things") @path thingId: string): string;
}
`);

strictEqual(routes.length, 1);
strictEqual(routes[0].path, "/things/{thingId}:customAction");
});

it("accepts Interface as target without compilation errors", async () => {
// This test verifies that @actionSeparator can be applied to interfaces without errors
const diagnostics = await Tester.diagnose(`
@TypeSpec.Rest.actionSeparator(":")
interface TestInterface {
op test(): void;
}
`);

// No diagnostics means the decorator accepts interfaces as valid targets
strictEqual(diagnostics.length, 0);
});

it("accepts Namespace as target without compilation errors", async () => {
// This test verifies that @actionSeparator can be applied to namespaces without errors
const diagnostics = await Tester.diagnose(`
@TypeSpec.Rest.actionSeparator(":")
namespace TestNamespace {
op test(): void;
}
`);

// No diagnostics means the decorator accepts namespaces as valid targets
strictEqual(diagnostics.length, 0);
});

it("supports all separator values in routing", async () => {
const routes = await getRoutesFor(`
@autoRoute
interface Things {
@action
@TypeSpec.Rest.actionSeparator("/")
@put op action1(@segment("things") @path thingId: string): string;

@action
@TypeSpec.Rest.actionSeparator(":")
@put op action2(@segment("things") @path thingId: string): string;

@action
@TypeSpec.Rest.actionSeparator("/:")
@put op action3(@segment("things") @path thingId: string): string;
}
`);

strictEqual(routes.length, 3);
strictEqual(routes[0].path, "/things/{thingId}/action1");
strictEqual(routes[1].path, "/things/{thingId}:action2");
strictEqual(routes[2].path, "/things/{thingId}/:action3");
});
});

describe("hierarchy behavior", () => {
it("interface-level separator applies to all actions in interface", async () => {
const routes = await getRoutesFor(`
@autoRoute
@TypeSpec.Rest.actionSeparator(":")
interface Things {
@action
@put op action1(@segment("things") @path thingId: string): string;

@action
@put op action2(@segment("things") @path thingId: string): string;
}
`);

strictEqual(routes.length, 2);
strictEqual(routes[0].path, "/things/{thingId}:action1");
strictEqual(routes[1].path, "/things/{thingId}:action2");
});

it("namespace-level separator applies to all actions in namespace", async () => {
const routes = await getRoutesFor(`
@TypeSpec.Rest.actionSeparator(":")
namespace TestNs {
@autoRoute
interface Things {
@action
@put op action1(@segment("things") @path thingId: string): string;
}
}
`);

strictEqual(routes.length, 1);
strictEqual(routes[0].path, "/things/{thingId}:action1");
});

it("operation-level separator overrides interface-level separator", async () => {
const routes = await getRoutesFor(`
@autoRoute
@TypeSpec.Rest.actionSeparator(":")
interface Things {
@action
@TypeSpec.Rest.actionSeparator("/")
@put op action1(@segment("things") @path thingId: string): string;

@action
@put op action2(@segment("things") @path thingId: string): string;
}
`);

strictEqual(routes.length, 2);
strictEqual(routes[0].path, "/things/{thingId}/action1"); // Uses operation-level "/"
strictEqual(routes[1].path, "/things/{thingId}:action2"); // Uses interface-level ":"
});

it("interface-level separator overrides namespace-level separator", async () => {
const routes = await getRoutesFor(`
@TypeSpec.Rest.actionSeparator("/:")
namespace TestNs {
@autoRoute
@TypeSpec.Rest.actionSeparator(":")
interface Things {
@action
@put op action1(@segment("things") @path thingId: string): string;
}

@autoRoute
interface Other {
@action
@put op action2(@segment("other") @path otherId: string): string;
}
}
`);

strictEqual(routes.length, 2);
strictEqual(routes[0].path, "/things/{thingId}:action1"); // Uses interface-level ":"
strictEqual(routes[1].path, "/other/{otherId}/:action2"); // Uses namespace-level "/:"
});

it("namespace separator applies to subnamespaces", async () => {
const routes = await getRoutesFor(`
@TypeSpec.Rest.actionSeparator(":")
namespace Parent {
namespace Child {
@autoRoute
interface Things {
@action
@put op action1(@segment("things") @path thingId: string): string;
}
}
}
`);

strictEqual(routes.length, 1);
strictEqual(routes[0].path, "/things/{thingId}:action1"); // Uses parent namespace-level ":"
});

it("operation in namespace without interface uses namespace separator", async () => {
const routes = await getRoutesFor(`
@TypeSpec.Rest.actionSeparator(":")
namespace TestNs {
@autoRoute
@action
@put op action1(@segment("things") @path thingId: string): string;
}
`);

strictEqual(routes.length, 1);
strictEqual(routes[0].path, "/things/{thingId}:action1");
});
});

describe("invalid targets", () => {
it("rejects Model", async () => {
const diagnostics = await Tester.diagnose(`
@TypeSpec.Rest.actionSeparator(":")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those test should be able to use just @actionSeparator without the fully qualified name

model TestModel {
id: string;
}
`);

expectDiagnostics(diagnostics, [
{
code: "decorator-wrong-target",
message:
"Cannot apply @actionSeparator decorator to TestModel since it is not assignable to Operation | Interface | Namespace",
},
]);
});

it("rejects ModelProperty", async () => {
const diagnostics = await Tester.diagnose(`
model TestModel {
@TypeSpec.Rest.actionSeparator(":")
id: string;
}
`);

expectDiagnostics(diagnostics, [
{
code: "decorator-wrong-target",
message:
/Cannot apply @actionSeparator decorator to .* since it is not assignable to Operation \| Interface \| Namespace/,
},
]);
});

it("rejects invalid separator values", async () => {
const diagnostics = await Tester.diagnose(`
@TypeSpec.Rest.actionSeparator("invalid")
op test(): void;
`);

expectDiagnostics(diagnostics, [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer not using the array form when there is only one diagnositc

{
code: "invalid-argument",
},
]);
});
});
});
Loading
Loading