-
Notifications
You must be signed in to change notification settings - Fork 336
Fix @actionSeparator decorator to accept Operation, Interface, and Namespace targets with hierarchy support #8609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d0b0916
c0ac5d3
dd9f71a
7cbbe36
2abb35a
3c2e1c1
be5887b
685317a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "/" | ":" | "/:" | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { | |
| Interface, | ||
| Model, | ||
| ModelProperty, | ||
| Namespace, | ||
| Operation, | ||
| Program, | ||
| Scalar, | ||
|
|
@@ -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( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shoudd that be updated to |
||
| context: DecoratorContext, | ||
| entity: Model | ModelProperty | Operation, | ||
| entity: Operation | Interface | Namespace, | ||
| separator: "/" | ":" | "/:", | ||
| ) { | ||
| context.program.stateMap(actionSeparatorKey).set(entity, separator); | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| 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", () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(":") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think those test should be able to use just |
||
| 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, [ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
| }, | ||
| ]); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.