From 0b9c7b8cf705310703d90bfe602be404d0590750 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 6 Apr 2023 15:41:12 +0300 Subject: [PATCH 01/13] Add `@sharedRoute` decorator and decouple it from `@route` --- .../split-shared-route_2023-04-06-12-41.json | 10 ++++ packages/http/src/decorators.ts | 14 ++++- packages/http/src/route.ts | 25 ++++++-- packages/http/test/routes.test.ts | 57 ++++++++++++++++++- 4 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 common/changes/@typespec/http/split-shared-route_2023-04-06-12-41.json diff --git a/common/changes/@typespec/http/split-shared-route_2023-04-06-12-41.json b/common/changes/@typespec/http/split-shared-route_2023-04-06-12-41.json new file mode 100644 index 00000000000..aab447a5c81 --- /dev/null +++ b/common/changes/@typespec/http/split-shared-route_2023-04-06-12-41.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/http", + "comment": "Add `@sharedRoute` decorator for marking operations as sharing a route with other operations", + "type": "none" + } + ], + "packageName": "@typespec/http" +} \ No newline at end of file diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index c7e5506ceaf..d4ec12fc833 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -19,7 +19,7 @@ import { validateDecoratorUniqueOnNode, } from "@typespec/compiler"; import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; -import { setRoute } from "./route.js"; +import { setRoute, setSharedRoute } from "./route.js"; import { AuthenticationOption, HeaderFieldOptions, @@ -596,6 +596,18 @@ export function $route(context: DecoratorContext, entity: Type, path: string, pa }); } +/** + * `@sharedRoute` marks the operation as sharing a route path with other operations. + * + * When an operation is marked with `@sharedRoute`, it enables other operations to share the same + * route path as long as those operations are also marked with `@sharedRoute`. + * + * `@sharedRoute` can only be applied directly to operations. + */ +export function $sharedRoute(context: DecoratorContext, entity: Operation) { + setSharedRoute(context.program, entity); +} + const includeInapplicableMetadataInPayloadKey = createStateSymbol( "includeInapplicableMetadataInPayload" ); diff --git a/packages/http/src/route.ts b/packages/http/src/route.ts index a226dccf98b..d8b9d9d77a8 100644 --- a/packages/http/src/route.ts +++ b/packages/http/src/route.ts @@ -216,8 +216,8 @@ export function setRoute(context: DecoratorContext, entity: Type, details: Route const state = context.program.stateMap(routesKey); if (state.has(entity) && entity.kind === "Namespace") { - const existingValue: RoutePath = state.get(entity); - if (existingValue.path !== details.path) { + const existingPath: string | undefined = state.get(entity); + if (existingPath !== details.path) { reportDiagnostic(context.program, { code: "duplicate-route-decorator", messageId: "namespace", @@ -225,19 +225,34 @@ export function setRoute(context: DecoratorContext, entity: Type, details: Route }); } } else { - state.set(entity, details); + state.set(entity, details.path); + if (entity.kind === "Operation" && details.shared) { + setSharedRoute(context.program, entity as Operation); + } } } +const sharedRoutesKey = createStateSymbol("sharedRoutes"); + +export function setSharedRoute(program: Program, operation: Operation) { + program.stateMap(sharedRoutesKey).set(operation, true); +} + export function isSharedRoute(program: Program, operation: Operation): boolean { - return program.stateMap(routesKey).get(operation)?.shared; + return program.stateMap(sharedRoutesKey).get(operation) === true; } export function getRoutePath( program: Program, entity: Namespace | Interface | Operation ): RoutePath | undefined { - return program.stateMap(routesKey).get(entity); + const path = program.stateMap(routesKey).get(entity); + return path + ? { + path, + shared: entity.kind === "Operation" && isSharedRoute(program, entity as Operation), + } + : undefined; } const routeOptionsKey = createStateSymbol("routeOptions"); diff --git a/packages/http/test/routes.test.ts b/packages/http/test/routes.test.ts index 35c556eee02..3f4e04d230d 100644 --- a/packages/http/test/routes.test.ts +++ b/packages/http/test/routes.test.ts @@ -1,7 +1,13 @@ +import { Operation } from "@typespec/compiler"; import { expectDiagnosticEmpty, expectDiagnostics } from "@typespec/compiler/testing"; import { deepStrictEqual, strictEqual } from "assert"; -import { HttpOperation } from "../src/index.js"; -import { compileOperations, getOperations, getRoutesFor } from "./test-host.js"; +import { getRoutePath, HttpOperation } from "../src/index.js"; +import { + compileOperations, + createHttpTestRunner, + getOperations, + getRoutesFor, +} from "./test-host.js"; describe("http: routes", () => { // Describe how routes should be included. @@ -545,4 +551,51 @@ describe("http: routes", () => { { verb: "put", path: "/things/{thingId}", params: ["thingId"] }, ]); }); + + describe("shared routes", () => { + it("@sharedRoute decorator makes routes shared", async () => { + const runner = await createHttpTestRunner(); + const { get1, get2 } = (await runner.compile(` + @route("/test") + namespace Foo { + @test + @sharedRoute + @route("/get1") + op get1(): string; + } + + @route("/test") + namespace Foo { + @test + @route("/get2") + op get2(): string; + } + `)) as { get1: Operation; get2: Operation }; + + strictEqual(getRoutePath(runner.program, get1)?.shared, true); + strictEqual(getRoutePath(runner.program, get2)?.shared, false); + }); + + it("legacy `shared: true parameter` still works", async () => { + const runner = await createHttpTestRunner(); + const { get1, get2 } = (await runner.compile(` + @route("/test") + namespace Foo { + @test + @route("/get1", { shared: true }) + op get1(): string; + } + + @route("/test") + namespace Foo { + @test + @route("/get2", { shared: false }) + op get2(): string; + } + `)) as { get1: Operation; get2: Operation }; + + strictEqual(getRoutePath(runner.program, get1)?.shared, true); + strictEqual(getRoutePath(runner.program, get2)?.shared, false); + }); + }); }); From ebc926cb970801d7d6579862eac909a13a2e1b85 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 7 Apr 2023 17:43:24 +0300 Subject: [PATCH 02/13] Add decorator definitions for @route and @sharedRoute --- packages/http/lib/http-decorators.tsp | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/http/lib/http-decorators.tsp b/packages/http/lib/http-decorators.tsp index 37f8a68c638..32243be8608 100644 --- a/packages/http/lib/http-decorators.tsp +++ b/packages/http/lib/http-decorators.tsp @@ -204,3 +204,35 @@ extern dec useAuth(target: Namespace, auth: object | Union | object[]); * Specify if inapplicable metadata should be included in the payload for the given entity. */ extern dec includeInapplicableMetadataInPayload(target: unknown, value: boolean); + +/** + * `@route` defines the relative route URI for the target operation + * + * The first argument should be a URI fragment that may contain one or more path parameter fields. + * If the namespace or interface that contains the operation is also marked with a `@route` decorator, + * it will be used as a prefix to the route URI of the operation. + * + * `@route` can only be applied to operations, namespaces, and interfaces. + * + * ```typespec + * @route("/widgets") + * op getWidget(@path id: string): Widget; + * ``` + */ +extern dec route(target: Namespace | Interface | Operation, path: string, options?: object); + +/** + * `@sharedRoute` marks the operation as sharing a route path with other operations. + * + * When an operation is marked with `@sharedRoute`, it enables other operations to share the same + * route path as long as those operations are also marked with `@sharedRoute`. + * + * `@sharedRoute` can only be applied directly to operations. + * + * ```typespec + * @sharedRoute + * @route("/widgets") + * op getWidget(@path id: string): Widget; + * ``` + */ +extern dec sharedRoute(target: Operation); From aa639c6d0f5509f483149a25fbbf69ebfcd4bebd Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 7 Apr 2023 17:48:16 +0300 Subject: [PATCH 03/13] Add autoRoute shared route tests --- .../split-shared-route_2023-04-07-14-49.json | 10 ++++++++ packages/rest/test/routes.test.ts | 25 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 common/changes/@typespec/rest/split-shared-route_2023-04-07-14-49.json diff --git a/common/changes/@typespec/rest/split-shared-route_2023-04-07-14-49.json b/common/changes/@typespec/rest/split-shared-route_2023-04-07-14-49.json new file mode 100644 index 00000000000..64135d54e31 --- /dev/null +++ b/common/changes/@typespec/rest/split-shared-route_2023-04-07-14-49.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/rest", + "comment": "", + "type": "none" + } + ], + "packageName": "@typespec/rest" +} \ No newline at end of file diff --git a/packages/rest/test/routes.test.ts b/packages/rest/test/routes.test.ts index 459aef33019..303f5cd7b75 100644 --- a/packages/rest/test/routes.test.ts +++ b/packages/rest/test/routes.test.ts @@ -1,7 +1,13 @@ import { ModelProperty, Operation } from "@typespec/compiler"; import { expectDiagnostics } from "@typespec/compiler/testing"; +import { isSharedRoute } from "@typespec/http"; import { deepStrictEqual, strictEqual } from "assert"; -import { compileOperations, getOperations, getRoutesFor } from "./test-host.js"; +import { + compileOperations, + createRestTestRunner, + getOperations, + getRoutesFor, +} from "./test-host.js"; describe("rest: routes", () => { it("always produces a route starting with /", async () => { @@ -441,4 +447,21 @@ describe("rest: routes", () => { { verb: "put", path: "/things/{thingId}", params: ["thingId"] }, ]); }); + + it("@autoRoute operations can also be shared routes", async () => { + const runner = await createRestTestRunner(); + const { get1, get2 } = (await runner.compile(` + @test + @autoRoute + @sharedRoute + op get1(@path name: string): string; + + @test + @autoRoute + op get2(@path name: string): string; + `)) as { get1: Operation; get2: Operation }; + + strictEqual(isSharedRoute(runner.program, get1), true); + strictEqual(isSharedRoute(runner.program, get2), false); + }); }); From d2564b39ab365dfa791326f34fd13566a17c7ff8 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 7 Apr 2023 18:35:48 +0300 Subject: [PATCH 04/13] Update generated documentation --- .../http/reference/decorators.md | 57 +++++++++++++++++++ docs/standard-library/http/reference/index.md | 2 + 2 files changed, 59 insertions(+) diff --git a/docs/standard-library/http/reference/decorators.md b/docs/standard-library/http/reference/decorators.md index 53ffef9c121..0656e5fa854 100644 --- a/docs/standard-library/http/reference/decorators.md +++ b/docs/standard-library/http/reference/decorators.md @@ -295,3 +295,60 @@ dec TypeSpec.Http.includeInapplicableMetadataInPayload(target: unknown, value: b | Name | Type | Description | | ----- | ---------------- | ----------- | | value | `scalar boolean` | | + +### `@route` {#@TypeSpec.Http.route} + +`@route` defines the relative route URI for the target operation + +The first argument should be a URI fragment that may contain one or more path parameter fields. +If the namespace or interface that contains the operation is also marked with a `@route` decorator, +it will be used as a prefix to the route URI of the operation. + +`@route` can only be applied to operations, namespaces, and interfaces. + +```typespec +@route("/widgets") +op getWidget(@path id: string): Widget; +``` + +```typespec +dec TypeSpec.Http.route(target: Namespace | Interface | Operation, path: string, options?: object) +``` + +#### Target + +`union Namespace | Interface | Operation` + +#### Parameters + +| Name | Type | Description | +| ------- | --------------- | ----------- | +| path | `scalar string` | | +| options | `model object` | | + +### `@sharedRoute` {#@TypeSpec.Http.sharedRoute} + +`@sharedRoute` marks the operation as sharing a route path with other operations. + +When an operation is marked with `@sharedRoute`, it enables other operations to share the same +route path as long as those operations are also marked with `@sharedRoute`. + +`@sharedRoute` can only be applied directly to operations. + +```typespec +@sharedRoute +@route("/widgets") +op getWidget(@path id: string): Widget; +``` + +```typespec +dec TypeSpec.Http.sharedRoute(target: Operation) +``` + +#### Target + +`Operation` + +#### Parameters + +None diff --git a/docs/standard-library/http/reference/index.md b/docs/standard-library/http/reference/index.md index 7c0c6d32acc..7ac4a1f2e1f 100644 --- a/docs/standard-library/http/reference/index.md +++ b/docs/standard-library/http/reference/index.md @@ -20,7 +20,9 @@ toc_max_heading_level: 3 - [`@post`](./decorators.md#@TypeSpec.Http.post) - [`@put`](./decorators.md#@TypeSpec.Http.put) - [`@query`](./decorators.md#@TypeSpec.Http.query) +- [`@route`](./decorators.md#@TypeSpec.Http.route) - [`@server`](./decorators.md#@TypeSpec.Http.server) +- [`@sharedRoute`](./decorators.md#@TypeSpec.Http.sharedRoute) - [`@statusCode`](./decorators.md#@TypeSpec.Http.statusCode) - [`@useAuth`](./decorators.md#@TypeSpec.Http.useAuth) From 311fabbb03e215040be0d073616448f97a05a09a Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 09:54:59 +0300 Subject: [PATCH 05/13] Deprecate `shared` option for `@route` parameter --- packages/http/src/decorators.ts | 35 +++++++++++----------- packages/http/test/http-decorators.test.ts | 24 ++++++++++++--- packages/http/test/routes.test.ts | 2 ++ packages/rest/test/routes.test.ts | 4 +-- 4 files changed, 41 insertions(+), 24 deletions(-) diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index cbea500f12d..b6754ee0184 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -3,6 +3,7 @@ import { DecoratorContext, Diagnostic, DiagnosticTarget, + reportDeprecated, getDoc, isArrayModelType, Model, @@ -559,23 +560,6 @@ export function getAuthentication( return program.stateMap(authenticationKey).get(namespace); } -function extractSharedValue(context: DecoratorContext, parameters?: Model): boolean { - const sharedType = parameters?.properties.get("shared")?.type; - if (sharedType === undefined) { - return false; - } - switch (sharedType.kind) { - case "Boolean": - return sharedType.value; - default: - reportDiagnostic(context.program, { - code: "shared-boolean", - target: sharedType, - }); - return false; - } -} - /** * `@route` defines the relative route URI for the target operation * @@ -588,9 +572,24 @@ function extractSharedValue(context: DecoratorContext, parameters?: Model): bool export function $route(context: DecoratorContext, entity: Type, path: string, parameters?: Model) { validateDecoratorUniqueOnNode(context, entity, $route); + // Handle the deprecated `shared` option + let shared = false; + const sharedValue = parameters?.properties.get("shared")?.type; + if (sharedValue !== undefined) { + reportDeprecated(context.program, "The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", entity); + if (sharedValue.kind === "Boolean") { + shared = sharedValue.value; + } else { + reportDiagnostic(context.program, { + code: "shared-boolean", + target: sharedValue, + }); + } + } + setRoute(context, entity, { path, - shared: extractSharedValue(context, parameters), + shared, }); } diff --git a/packages/http/test/http-decorators.test.ts b/packages/http/test/http-decorators.test.ts index c43eae583ef..6bf9ba8d142 100644 --- a/packages/http/test/http-decorators.test.ts +++ b/packages/http/test/http-decorators.test.ts @@ -245,10 +245,22 @@ describe("http: decorators", () => { ]); }); - it("emit diagnostics when not all duplicated routes are declared shared", async () => { + it("emits diagnostic when deprecated `shared` option is used", async () => { const diagnostics = await runner.diagnose(` @route("/test", { shared: true }) op test(): string; - @route("/test", { shared: true }) op test2(): string; + `); + expectDiagnostics(diagnostics, [ + { + code: "deprecated", + message: "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead." + }, + ]); + }); + + it("emit diagnostics when not all duplicated routes are declared shared", async () => { + const diagnostics = await runner.diagnose(` + @route("/test") @sharedRoute op test(): string; + @route("/test") @sharedRoute op test2(): string; @route("/test") op test3(): string; `); expectDiagnostics(diagnostics, [ @@ -261,8 +273,8 @@ describe("http: decorators", () => { it("do not emit diagnostics when duplicated shared routes are applied", async () => { const diagnostics = await runner.diagnose(` - @route("/test", {shared: true}) op test(): string; - @route("/test", {shared: true}) op test2(): string; + @route("/test") @sharedRoute op test(): string; + @route("/test") @sharedRoute op test2(): string; `); expectDiagnosticEmpty(diagnostics); @@ -273,6 +285,10 @@ describe("http: decorators", () => { @route("/test", {shared: "yes"}) op test(): string; `); expectDiagnostics(diagnostics, [ + { + code: "deprecated", + message: "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead." + }, { code: "@typespec/http/shared-boolean", message: `shared parameter must be a boolean.`, diff --git a/packages/http/test/routes.test.ts b/packages/http/test/routes.test.ts index 3f4e04d230d..5cec90b38f6 100644 --- a/packages/http/test/routes.test.ts +++ b/packages/http/test/routes.test.ts @@ -581,6 +581,7 @@ describe("http: routes", () => { const { get1, get2 } = (await runner.compile(` @route("/test") namespace Foo { + #suppress "deprecated" @test @route("/get1", { shared: true }) op get1(): string; @@ -588,6 +589,7 @@ describe("http: routes", () => { @route("/test") namespace Foo { + #suppress "deprecated" @test @route("/get2", { shared: false }) op get2(): string; diff --git a/packages/rest/test/routes.test.ts b/packages/rest/test/routes.test.ts index 303f5cd7b75..b75c0b34982 100644 --- a/packages/rest/test/routes.test.ts +++ b/packages/rest/test/routes.test.ts @@ -454,11 +454,11 @@ describe("rest: routes", () => { @test @autoRoute @sharedRoute - op get1(@path name: string): string; + op get1(@path foo: string): string; @test @autoRoute - op get2(@path name: string): string; + op get2(@path bar: string): string; `)) as { get1: Operation; get2: Operation }; strictEqual(isSharedRoute(runner.program, get1), true); From 8cf9bd26a66f6e7bdf33b040a481a01404b5885b Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 09:55:51 +0300 Subject: [PATCH 06/13] Format source --- packages/http/src/decorators.ts | 18 +++++++++++------- packages/http/test/http-decorators.test.ts | 6 ++++-- packages/http/test/routes.test.ts | 2 +- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index b6754ee0184..d079972a6a8 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -1,21 +1,21 @@ import { - createDiagnosticCollector, DecoratorContext, Diagnostic, DiagnosticTarget, - reportDeprecated, - getDoc, - isArrayModelType, Model, ModelProperty, Namespace, Operation, Program, - setTypeSpecNamespace, Tuple, Type, - typespecTypeToJson, Union, + createDiagnosticCollector, + getDoc, + isArrayModelType, + reportDeprecated, + setTypeSpecNamespace, + typespecTypeToJson, validateDecoratorTarget, validateDecoratorUniqueOnNode, } from "@typespec/compiler"; @@ -576,7 +576,11 @@ export function $route(context: DecoratorContext, entity: Type, path: string, pa let shared = false; const sharedValue = parameters?.properties.get("shared")?.type; if (sharedValue !== undefined) { - reportDeprecated(context.program, "The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", entity); + reportDeprecated( + context.program, + "The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", + entity + ); if (sharedValue.kind === "Boolean") { shared = sharedValue.value; } else { diff --git a/packages/http/test/http-decorators.test.ts b/packages/http/test/http-decorators.test.ts index 6bf9ba8d142..d2230dfd83e 100644 --- a/packages/http/test/http-decorators.test.ts +++ b/packages/http/test/http-decorators.test.ts @@ -252,7 +252,8 @@ describe("http: decorators", () => { expectDiagnostics(diagnostics, [ { code: "deprecated", - message: "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead." + message: + "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", }, ]); }); @@ -287,7 +288,8 @@ describe("http: decorators", () => { expectDiagnostics(diagnostics, [ { code: "deprecated", - message: "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead." + message: + "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", }, { code: "@typespec/http/shared-boolean", diff --git a/packages/http/test/routes.test.ts b/packages/http/test/routes.test.ts index 5cec90b38f6..5f12ade3370 100644 --- a/packages/http/test/routes.test.ts +++ b/packages/http/test/routes.test.ts @@ -1,7 +1,7 @@ import { Operation } from "@typespec/compiler"; import { expectDiagnosticEmpty, expectDiagnostics } from "@typespec/compiler/testing"; import { deepStrictEqual, strictEqual } from "assert"; -import { getRoutePath, HttpOperation } from "../src/index.js"; +import { HttpOperation, getRoutePath } from "../src/index.js"; import { compileOperations, createHttpTestRunner, From edcc87dd4b1dc02b15bb3853e8d987b947b5e5ce Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 10:05:47 +0300 Subject: [PATCH 07/13] Add `options` signature for `route` decorator --- packages/http/lib/http-decorators.tsp | 8 +++++++- packages/http/src/decorators.ts | 7 ++----- packages/http/src/lib.ts | 6 ------ packages/http/test/http-decorators.test.ts | 9 ++------- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/packages/http/lib/http-decorators.tsp b/packages/http/lib/http-decorators.tsp index f1f67b183cf..61da390f27f 100644 --- a/packages/http/lib/http-decorators.tsp +++ b/packages/http/lib/http-decorators.tsp @@ -224,7 +224,13 @@ extern dec includeInapplicableMetadataInPayload(target: unknown, value: boolean) * op getWidget(@path id: string): Widget; * ``` */ -extern dec route(target: Namespace | Interface | Operation, path: string, options?: object); +extern dec route( + target: Namespace | Interface | Operation, + path: string, + options?: { + shared?: boolean, + } +); /** * `@sharedRoute` marks the operation as sharing a route path with other operations. diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index d079972a6a8..1fb5942f544 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -581,13 +581,10 @@ export function $route(context: DecoratorContext, entity: Type, path: string, pa "The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", entity ); + + // The type checker should have raised a diagnostic if the value isn't boolean if (sharedValue.kind === "Boolean") { shared = sharedValue.value; - } else { - reportDiagnostic(context.program, { - code: "shared-boolean", - target: sharedValue, - }); } } diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index e0542bf3b14..931d53a3634 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -100,12 +100,6 @@ const libDefinition = { default: paramMessage`@useAuth ${"kind"} only accept Auth model, Tuple of auth model or union of auth model.`, }, }, - "shared-boolean": { - severity: "error", - messages: { - default: "shared parameter must be a boolean.", - }, - }, "shared-inconsistency": { severity: "error", messages: { diff --git a/packages/http/test/http-decorators.test.ts b/packages/http/test/http-decorators.test.ts index d2230dfd83e..7991aa559be 100644 --- a/packages/http/test/http-decorators.test.ts +++ b/packages/http/test/http-decorators.test.ts @@ -287,13 +287,8 @@ describe("http: decorators", () => { `); expectDiagnostics(diagnostics, [ { - code: "deprecated", - message: - "Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead.", - }, - { - code: "@typespec/http/shared-boolean", - message: `shared parameter must be a boolean.`, + code: "invalid-argument", + message: `Argument '(anonymous model)' is not assignable to parameter of type '(anonymous model)'`, }, ]); }); From 665dcf9ba498e412cdec5642a14ee918ccde4b77 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 17:04:59 +0300 Subject: [PATCH 08/13] Ensure @action operations have explicit name when using @sharedRoute --- packages/rest/src/lib.ts | 6 ++++ packages/rest/src/rest.ts | 56 +++++++++++++++++++++++++++---- packages/rest/src/validate.ts | 32 ++++++++++++++++++ packages/rest/test/routes.test.ts | 45 +++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/packages/rest/src/lib.ts b/packages/rest/src/lib.ts index f46d2003e86..0f5882c5993 100644 --- a/packages/rest/src/lib.ts +++ b/packages/rest/src/lib.ts @@ -39,6 +39,12 @@ const libDefinition = { default: "Action name cannot be empty string.", }, }, + "shared-route-unspecified-action-name": { + severity: "error", + messages: { + default: paramMessage`An operation marked as '@sharedRoute' must have an explicit collection action name passed to '${"decoratorName"}'.`, + }, + }, }, } as const; diff --git a/packages/rest/src/rest.ts b/packages/rest/src/rest.ts index 6ad7837fd4f..3bf8dffdccd 100644 --- a/packages/rest/src/rest.ts +++ b/packages/rest/src/rest.ts @@ -421,8 +421,11 @@ function lowerCaseFirstChar(str: string): string { return str[0].toLocaleLowerCase() + str.substring(1); } -function makeActionName(op: Operation, name: string | undefined): string { - return lowerCaseFirstChar(name || op.name); +function makeActionName(op: Operation, name: string | undefined): ActionDetails { + return { + name: lowerCaseFirstChar(name || op.name), + kind: name ? "specified" : "automatic", + }; } const actionsSegmentKey = createStateSymbol("actionSegment"); @@ -435,6 +438,22 @@ export function getActionSegment(program: Program, entity: Type): string | undef return program.stateMap(actionsSegmentKey).get(entity); } +/** + * Provides details about an action or collection action. + */ +export interface ActionDetails { + /** + * The name of the action + */ + name: string; + + /** + * Identifies whether the action's name was generated from the original + * operation name or if it was explicitly specified. + */ + kind: "automatic" | "specified"; +} + const actionsKey = createStateSymbol("actions"); export function $action(context: DecoratorContext, entity: Operation, name?: string) { if (name === "") { @@ -444,15 +463,26 @@ export function $action(context: DecoratorContext, entity: Operation, name?: str }); return; } + const action = makeActionName(entity, name); - context.call($actionSegment, entity, action); + context.call($actionSegment, entity, action.name); context.program.stateMap(actionsKey).set(entity, action); } -export function getAction(program: Program, operation: Operation): string | null | undefined { +/** + * Gets the ActionDetails for the specified operation if it has previously been marked with @action. + */ +export function getActionDetails( + program: Program, + operation: Operation +): ActionDetails | undefined { return program.stateMap(actionsKey).get(operation); } +export function getAction(program: Program, operation: Operation): string | null | undefined { + return getActionDetails(program, operation)?.name; +} + const collectionActionsKey = createStateSymbol("collectionActions"); export function $collectionAction( @@ -473,16 +503,28 @@ export function $collectionAction( } const action = makeActionName(entity, name); - context.call($actionSegment, entity, action); + context.call($actionSegment, entity, action.name); - context.program.stateMap(collectionActionsKey).set(entity, `${segment}/${action}`); + // Update the final name and store it + action.name = `${segment}/${action.name}`; + context.program.stateMap(collectionActionsKey).set(entity, action); +} + +/** + * Gets the ActionDetails for the specified operation if it has previously been marked with @collectionAction. + */ +export function getCollectionActionDetails( + program: Program, + operation: Operation +): ActionDetails | undefined { + return program.stateMap(collectionActionsKey).get(operation); } export function getCollectionAction( program: Program, operation: Operation ): string | null | undefined { - return program.stateMap(collectionActionsKey).get(operation); + return getCollectionActionDetails(program, operation)?.name; } const resourceLocationsKey = createStateSymbol("resourceLocations"); diff --git a/packages/rest/src/validate.ts b/packages/rest/src/validate.ts index 48f6e09c3e1..3da0f5af67d 100644 --- a/packages/rest/src/validate.ts +++ b/packages/rest/src/validate.ts @@ -7,8 +7,10 @@ import { navigateTypesInNamespace, Program, } from "@typespec/compiler"; +import { isSharedRoute } from "@typespec/http"; import { reportDiagnostic } from "./lib.js"; import { getParentResource, getResourceTypeKey, ResourceKey } from "./resource.js"; +import { getActionDetails, getCollectionActionDetails } from "./rest.js"; function checkForDuplicateResourceKeyNames(program: Program) { const seenTypes = new Set(); @@ -60,8 +62,38 @@ function checkForDuplicateResourceKeyNames(program: Program) { } } +function checkForSharedRouteUnnamedActions(program: Program) { + for (const service of listServices(program)) { + // If the model type is defined under the service namespace, check that the + // parent resource type(s) don't have the same key name as the + // current resource type. + navigateTypesInNamespace(service.type, { + operation: (op) => { + const actionDetails = getActionDetails(program, op); + if ( + isSharedRoute(program, op) && + (actionDetails?.kind === "automatic" || + getCollectionActionDetails(program, op)?.kind === "automatic") + ) { + reportDiagnostic(program, { + code: "shared-route-unspecified-action-name", + target: op, + format: { + decoratorName: actionDetails ? "@action" : "@collectionAction", + }, + }); + } + }, + }); + } +} + export function $onValidate(program: Program) { // Make sure any defined resource types don't have any conflicts with parent // resource type key names checkForDuplicateResourceKeyNames(program); + + // Ensure that all shared routes with an `@action` decorator have an + // explicitly named action + checkForSharedRouteUnnamedActions(program); } diff --git a/packages/rest/test/routes.test.ts b/packages/rest/test/routes.test.ts index b75c0b34982..6f9aac1cd2d 100644 --- a/packages/rest/test/routes.test.ts +++ b/packages/rest/test/routes.test.ts @@ -464,4 +464,49 @@ describe("rest: routes", () => { strictEqual(isSharedRoute(runner.program, get1), true); strictEqual(isSharedRoute(runner.program, get2), false); }); + + it("emits a diagnostic when @sharedRoute is used on action without explicit name", async () => { + const [_, diagnostics] = await compileOperations( + ` + model Thing { + @key + @segment("things") + thingId: string; + } + + @action + @autoRoute + @sharedRoute + op badAction(): {}; + + @action("good") + @autoRoute + @sharedRoute + op goodAction(): {}; + + @autoRoute + @sharedRoute + @collectionAction(Thing) + op badCollectionAction(): {}; + + @autoRoute + @sharedRoute + @collectionAction(Thing, "goodCollection") + op goodCollectionAction(): {}; + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@typespec/rest/shared-route-unspecified-action-name", + message: + "An operation marked as '@sharedRoute' must have an explicit collection action name passed to '@action'.", + }, + { + code: "@typespec/rest/shared-route-unspecified-action-name", + message: + "An operation marked as '@sharedRoute' must have an explicit collection action name passed to '@collectionAction'.", + }, + ]); + }); }); From d2e7e258fdbfe40bcceeb4258020b8250c20aa06 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 17:07:05 +0300 Subject: [PATCH 09/13] Update Rush change files --- .../http/split-shared-route_2023-04-18-14-06.json | 10 ++++++++++ .../rest/split-shared-route_2023-04-18-14-06.json | 10 ++++++++++ 2 files changed, 20 insertions(+) create mode 100644 common/changes/@typespec/http/split-shared-route_2023-04-18-14-06.json create mode 100644 common/changes/@typespec/rest/split-shared-route_2023-04-18-14-06.json diff --git a/common/changes/@typespec/http/split-shared-route_2023-04-18-14-06.json b/common/changes/@typespec/http/split-shared-route_2023-04-18-14-06.json new file mode 100644 index 00000000000..c7b42480d16 --- /dev/null +++ b/common/changes/@typespec/http/split-shared-route_2023-04-18-14-06.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/http", + "comment": "Deprecate the `shared` option in the `@route` decorator. `@sharedRoute` is the new way to accomplish the same behavior.", + "type": "none" + } + ], + "packageName": "@typespec/http" +} \ No newline at end of file diff --git a/common/changes/@typespec/rest/split-shared-route_2023-04-18-14-06.json b/common/changes/@typespec/rest/split-shared-route_2023-04-18-14-06.json new file mode 100644 index 00000000000..3dbd1229d14 --- /dev/null +++ b/common/changes/@typespec/rest/split-shared-route_2023-04-18-14-06.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/rest", + "comment": "Add validation to ensure that @action or @collectionAction operations have a specified name when used with @sharedRoute", + "type": "none" + } + ], + "packageName": "@typespec/rest" +} \ No newline at end of file From 279feea79d1e7ff18d126b744c2cc3fa064fd07c Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 17:07:55 +0300 Subject: [PATCH 10/13] Update documentation --- docs/standard-library/http/reference/decorators.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/standard-library/http/reference/decorators.md b/docs/standard-library/http/reference/decorators.md index f2cdc4d8196..5076c235dcb 100644 --- a/docs/standard-library/http/reference/decorators.md +++ b/docs/standard-library/http/reference/decorators.md @@ -246,7 +246,7 @@ it will be used as a prefix to the route URI of the operation. `@route` can only be applied to operations, namespaces, and interfaces. ```typespec -dec TypeSpec.Http.route(target: Namespace | Interface | Operation, path: string, options?: object) +dec TypeSpec.Http.route(target: Namespace | Interface | Operation, path: string, options?: (anonymous model)) ``` #### Target @@ -255,10 +255,10 @@ dec TypeSpec.Http.route(target: Namespace | Interface | Operation, path: string, #### Parameters -| Name | Type | Description | -| ------- | --------------- | ----------------------------------------------------- | -| path | `scalar string` | Relative route path. Cannot include query parameters. | -| options | `model object` | | +| Name | Type | Description | +| ------- | ------------------------- | ----------------------------------------------------- | +| path | `scalar string` | Relative route path. Cannot include query parameters. | +| options | `model (anonymous model)` | | ### `@server` {#@TypeSpec.Http.server} From 60d7fbb135383c58351c09bed645803fc6f77924 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 18 Apr 2023 17:27:05 +0300 Subject: [PATCH 11/13] Update openapi3 shared route tests to use @sharedRoute --- .../split-shared-route_2023-04-18-14-27.json | 10 ++++++ packages/openapi3/test/shared-routes.test.ts | 36 ++++++++++++------- 2 files changed, 34 insertions(+), 12 deletions(-) create mode 100644 common/changes/@typespec/openapi3/split-shared-route_2023-04-18-14-27.json diff --git a/common/changes/@typespec/openapi3/split-shared-route_2023-04-18-14-27.json b/common/changes/@typespec/openapi3/split-shared-route_2023-04-18-14-27.json new file mode 100644 index 00000000000..d9a70151de5 --- /dev/null +++ b/common/changes/@typespec/openapi3/split-shared-route_2023-04-18-14-27.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/openapi3", + "comment": "", + "type": "none" + } + ], + "packageName": "@typespec/openapi3" +} \ No newline at end of file diff --git a/packages/openapi3/test/shared-routes.test.ts b/packages/openapi3/test/shared-routes.test.ts index 81523e44b16..49893304096 100644 --- a/packages/openapi3/test/shared-routes.test.ts +++ b/packages/openapi3/test/shared-routes.test.ts @@ -41,12 +41,14 @@ describe("openapi3: shared routes", () => { id: string; } + @sharedRoute @operationId("List_ResourceGroup") - @route("/sharedroutes/resources", { shared: true }) + @route("/sharedroutes/resources") op listByResourceGroup(...Resource, @query resourceGroup: string, @query foo: string): Resource[]; + @sharedRoute @operationId("List_Subscription") - @route("/sharedroutes/resources", { shared: true }) + @route("/sharedroutes/resources") op listBySubscription(...Resource, @query subscription: string, @query foo: string): Resource[]; } ` @@ -96,10 +98,12 @@ describe("openapi3: shared routes", () => { id: string; } - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op listByResourceGroup(...Resource, @query filter: "resourceGroup"): Resource[]; - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op listBySubscription(...Resource, @query filter: "subscription"): Resource[]; } ` @@ -135,10 +139,12 @@ describe("openapi3: shared routes", () => { id: string; } - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op listByResourceGroup(...Resource, @query filter: "resourceGroup"): Resource[]; - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op listBySubscription(...Resource, @header filter: "subscription"): Resource[]; } ` @@ -183,10 +189,12 @@ describe("openapi3: shared routes", () => { id: string; } - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op returnsInt(...Resource, @query options: string): int32; - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op returnsString(...Resource, @query options: string): string; } ` @@ -224,10 +232,12 @@ describe("openapi3: shared routes", () => { ` @service({title: "My Service"}) namespace Foo { - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op processInt(@body body: int32, @query options: string): void; - @route("/sharedroutes/resources", { shared: true }) + @sharedRoute + @route("/sharedroutes/resources") op processString(@body body: string, @query options: string): void; } ` @@ -261,10 +271,12 @@ describe("openapi3: shared routes", () => { ` @service({title: "My Service"}) namespace Foo { - @route("/process", { shared: true }) + @sharedRoute + @route("/process") op processInt(@body body: int32, @query options: string): int32; - @route("/process", { shared: true }) + @sharedRoute + @route("/process") op processString(@body body: string, @query options: string): string; } ` From 1c3507189795b97a26fda589530188dae1fd7faf Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 19 Apr 2023 14:15:16 +0300 Subject: [PATCH 12/13] PR feedback --- packages/rest/src/rest.ts | 8 +++++++- packages/rest/test/routes.test.ts | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/rest/src/rest.ts b/packages/rest/src/rest.ts index 3bf8dffdccd..7ce7f9fad23 100644 --- a/packages/rest/src/rest.ts +++ b/packages/rest/src/rest.ts @@ -79,7 +79,7 @@ function getResourceOperationHttpVerb( return ( getOperationVerb(program, operation) ?? (resourceOperation && resourceOperationToVerb[resourceOperation.operation]) ?? - (getAction(program, operation) || getCollectionAction(program, operation) ? "post" : undefined) + (getActionDetails(program, operation) || getCollectionActionDetails(program, operation) ? "post" : undefined) ); } @@ -479,6 +479,9 @@ export function getActionDetails( return program.stateMap(actionsKey).get(operation); } +/** + * @deprecated Use getActionDetails instead. + */ export function getAction(program: Program, operation: Operation): string | null | undefined { return getActionDetails(program, operation)?.name; } @@ -520,6 +523,9 @@ export function getCollectionActionDetails( return program.stateMap(collectionActionsKey).get(operation); } +/** + * @deprecated Use getCollectionActionDetails instead. + */ export function getCollectionAction( program: Program, operation: Operation diff --git a/packages/rest/test/routes.test.ts b/packages/rest/test/routes.test.ts index 6f9aac1cd2d..7c49f11f1e0 100644 --- a/packages/rest/test/routes.test.ts +++ b/packages/rest/test/routes.test.ts @@ -454,11 +454,11 @@ describe("rest: routes", () => { @test @autoRoute @sharedRoute - op get1(@path foo: string): string; + op get1(@segment("get1") @path name: string): string; @test @autoRoute - op get2(@path bar: string): string; + op get2(@segment("get2") @path name: string): string; `)) as { get1: Operation; get2: Operation }; strictEqual(isSharedRoute(runner.program, get1), true); From 54a0d0464c934d19cbf826a564fbc89b8d800575 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 19 Apr 2023 14:33:45 +0300 Subject: [PATCH 13/13] Format --- packages/rest/src/rest.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/rest/src/rest.ts b/packages/rest/src/rest.ts index 7ce7f9fad23..73ab672b8c3 100644 --- a/packages/rest/src/rest.ts +++ b/packages/rest/src/rest.ts @@ -79,7 +79,9 @@ function getResourceOperationHttpVerb( return ( getOperationVerb(program, operation) ?? (resourceOperation && resourceOperationToVerb[resourceOperation.operation]) ?? - (getActionDetails(program, operation) || getCollectionActionDetails(program, operation) ? "post" : undefined) + (getActionDetails(program, operation) || getCollectionActionDetails(program, operation) + ? "post" + : undefined) ); }