From c6a50734b549ce86c2877cfdcbb9f03c38ca34b0 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 19 Oct 2022 10:55:36 -0700 Subject: [PATCH 1/6] Allow @route and @routeReset to accept shared property to suppress validation. --- packages/rest/src/http/decorators.ts | 13 +++++++--- packages/rest/src/http/operations.ts | 4 +++ packages/rest/src/http/route.ts | 12 +++++++-- packages/rest/src/http/types.ts | 7 ++++++ packages/rest/test/http-decorators.test.ts | 29 ++++++++++++++++++++++ 5 files changed, 60 insertions(+), 5 deletions(-) diff --git a/packages/rest/src/http/decorators.ts b/packages/rest/src/http/decorators.ts index 631a4a52f87..2de7848bcef 100644 --- a/packages/rest/src/http/decorators.ts +++ b/packages/rest/src/http/decorators.ts @@ -563,17 +563,24 @@ export function getAuthentication( * * `@route` can only be applied to operations, namespaces, and interfaces. */ -export function $route(context: DecoratorContext, entity: Type, path: string) { +export function $route(context: DecoratorContext, entity: Type, path: string, parameters?: Model) { setRoute(context, entity, { path, isReset: false, + parameters, }); } -export function $routeReset(context: DecoratorContext, entity: Type, path: string) { +export function $routeReset( + context: DecoratorContext, + entity: Type, + path: string, + parameters?: Model +) { setRoute(context, entity, { path, isReset: true, + parameters, }); } @@ -594,7 +601,7 @@ export function getRouteOptionsForNamespace( } const routesKey = createStateSymbol("routes"); -function setRoute(context: DecoratorContext, entity: Type, details: RoutePath) { +function setRoute(context: DecoratorContext, entity: Type, details: RoutePath, parameters?: Model) { if ( !validateDecoratorTarget(context, entity, "@route", ["Namespace", "Interface", "Operation"]) ) { diff --git a/packages/rest/src/http/operations.ts b/packages/rest/src/http/operations.ts index 91aed5809b6..eff7b398a91 100644 --- a/packages/rest/src/http/operations.ts +++ b/packages/rest/src/http/operations.ts @@ -109,6 +109,9 @@ export function validateRouteUnique(diagnostics: DiagnosticCollector, operations if (operation.overloading !== undefined && isOverloadSameEndpoint(operation as any)) { continue; } + if (operation.pathShared) { + continue; + } let map = grouped.get(path); if (map === undefined) { map = new Map(); @@ -175,6 +178,7 @@ function getHttpOperationInternal( const httpOperation: HttpOperation = { path: route.path, pathSegments: route.pathSegments, + pathShared: route.shared, verb: route.parameters.verb, container: operation.interface ?? operation.namespace ?? program.getGlobalNamespaceType(), parameters: route.parameters, diff --git a/packages/rest/src/http/route.ts b/packages/rest/src/http/route.ts index c25a2151d7c..464062a1f08 100644 --- a/packages/rest/src/http/route.ts +++ b/packages/rest/src/http/route.ts @@ -1,4 +1,5 @@ import { + BooleanLiteral, createDiagnosticCollector, Diagnostic, Interface, @@ -106,7 +107,7 @@ export function resolvePathAndParameters( overloadBase: HttpOperation | undefined, options: RouteResolutionOptions ): [ - { path: string; pathSegments: string[]; parameters: HttpOperationParameters }, + { path: string; pathSegments: string[]; parameters: HttpOperationParameters; shared: boolean }, readonly Diagnostic[] ] { let segments: string[] = []; @@ -161,10 +162,14 @@ export function resolvePathAndParameters( } } + const shared = + (getRoutePath(program, operation)?.parameters?.properties.get("shared")?.type as BooleanLiteral) + ?.value ?? false; return diagnostics.wrap({ path: buildPath(segments), pathSegments: segments, parameters, + shared, }); } @@ -195,7 +200,10 @@ function getRouteSegments( program: Program, target: Operation | Interface | Namespace ): [string[], RouteOptions | undefined] { - const route = getRoutePath(program, target)?.path; + const routePath = getRoutePath(program, target); + const route = routePath?.path; + const shared = (routePath?.parameters?.properties.get("shared")?.type as BooleanLiteral)?.value; + // FIXME: Plumb this through... const seg = route ? [route] : []; const [parentSegments, parentOptions] = getParentSegments(program, target); const options = diff --git a/packages/rest/src/http/types.ts b/packages/rest/src/http/types.ts index 5e1c941dae4..a8be7538d91 100644 --- a/packages/rest/src/http/types.ts +++ b/packages/rest/src/http/types.ts @@ -1,6 +1,7 @@ import { Interface, ListOperationOptions, + Model, ModelProperty, Namespace, Operation, @@ -222,6 +223,11 @@ export interface HttpOperation { */ pathSegments: string[]; + /** + * Whether the route path is shared + */ + pathShared: boolean; + /** * Route verb. */ @@ -261,6 +267,7 @@ export interface HttpOperation { export interface RoutePath { path: string; isReset: boolean; + parameters?: Model; } export type StatusCode = `${number}` | "*"; diff --git a/packages/rest/test/http-decorators.test.ts b/packages/rest/test/http-decorators.test.ts index dea556bcc8b..42a499cf77c 100644 --- a/packages/rest/test/http-decorators.test.ts +++ b/packages/rest/test/http-decorators.test.ts @@ -139,6 +139,35 @@ describe("rest: http decorators", () => { }); }); + describe("@route", () => { + it("emit diagnostics when duplicated unshared routes are applied", async () => { + const diagnostics = await runner.diagnose(` + @route("/test") op test(): string; + @route("/test") op test2(): string; + `); + + expectDiagnostics(diagnostics, [ + { + code: "@cadl-lang/rest/duplicate-operation", + message: `Duplicate operation "test" routed at "get /test".`, + }, + { + code: "@cadl-lang/rest/duplicate-operation", + message: `Duplicate operation "test2" routed at "get /test".`, + }, + ]); + }); + + 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; + `); + + expectDiagnosticEmpty(diagnostics); + }); + }); + describe("@path", () => { it("emit diagnostics when @path is not used on model property", async () => { const diagnostics = await runner.diagnose(` From 868f695c4ae81deb2d6d28566534fc4979f54bcd Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 19 Oct 2022 10:56:38 -0700 Subject: [PATCH 2/6] Changelog item. --- .../rest/rest-RouteUpdate_2022-10-19-17-56.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/@cadl-lang/rest/rest-RouteUpdate_2022-10-19-17-56.json diff --git a/common/changes/@cadl-lang/rest/rest-RouteUpdate_2022-10-19-17-56.json b/common/changes/@cadl-lang/rest/rest-RouteUpdate_2022-10-19-17-56.json new file mode 100644 index 00000000000..7650b54831f --- /dev/null +++ b/common/changes/@cadl-lang/rest/rest-RouteUpdate_2022-10-19-17-56.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/rest", + "comment": "Allow `@route` and `@resetRoute` to accept a `shared` property to suppress route uniqueness validation.", + "type": "minor" + } + ], + "packageName": "@cadl-lang/rest" +} \ No newline at end of file From ee5098697cf57fa6bd75edfe6ecdb5514986f8e6 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 19 Oct 2022 11:02:19 -0700 Subject: [PATCH 3/6] Revert unnecessary change. --- packages/rest/src/http/route.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/rest/src/http/route.ts b/packages/rest/src/http/route.ts index 464062a1f08..7342dd17de7 100644 --- a/packages/rest/src/http/route.ts +++ b/packages/rest/src/http/route.ts @@ -200,10 +200,7 @@ function getRouteSegments( program: Program, target: Operation | Interface | Namespace ): [string[], RouteOptions | undefined] { - const routePath = getRoutePath(program, target); - const route = routePath?.path; - const shared = (routePath?.parameters?.properties.get("shared")?.type as BooleanLiteral)?.value; - // FIXME: Plumb this through... + const route = getRoutePath(program, target)?.path; const seg = route ? [route] : []; const [parentSegments, parentOptions] = getParentSegments(program, target); const options = From cd097b827c2c551289cf02671bd64d4a635868ec Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 19 Oct 2022 11:11:10 -0700 Subject: [PATCH 4/6] Update packages/rest/src/http/types.ts Co-authored-by: David Wilson --- packages/rest/src/http/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rest/src/http/types.ts b/packages/rest/src/http/types.ts index a8be7538d91..4fe5546a4d0 100644 --- a/packages/rest/src/http/types.ts +++ b/packages/rest/src/http/types.ts @@ -224,7 +224,7 @@ export interface HttpOperation { pathSegments: string[]; /** - * Whether the route path is shared + * Whether the route path is shared with other operations */ pathShared: boolean; From 97754c4d6ec0f99069436a423f9773cc534ec19d Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 19 Oct 2022 12:02:51 -0700 Subject: [PATCH 5/6] =?UTF-8?q?=EF=BB=BFCode=20review=20feedback.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/rest/src/http/decorators.ts | 7 ++++--- packages/rest/src/http/operations.ts | 13 +++++++++---- packages/rest/src/http/route.ts | 11 +++++------ packages/rest/src/http/types.ts | 8 +------- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/packages/rest/src/http/decorators.ts b/packages/rest/src/http/decorators.ts index 2de7848bcef..e6e3d69979e 100644 --- a/packages/rest/src/http/decorators.ts +++ b/packages/rest/src/http/decorators.ts @@ -1,4 +1,5 @@ import { + BooleanLiteral, cadlTypeToJson, createDecoratorDefinition, createDiagnosticCollector, @@ -567,7 +568,7 @@ export function $route(context: DecoratorContext, entity: Type, path: string, pa setRoute(context, entity, { path, isReset: false, - parameters, + shared: (parameters?.properties.get("shared")?.type as BooleanLiteral)?.value ?? false, }); } @@ -580,7 +581,7 @@ export function $routeReset( setRoute(context, entity, { path, isReset: true, - parameters, + shared: (parameters?.properties.get("shared")?.type as BooleanLiteral)?.value ?? false, }); } @@ -601,7 +602,7 @@ export function getRouteOptionsForNamespace( } const routesKey = createStateSymbol("routes"); -function setRoute(context: DecoratorContext, entity: Type, details: RoutePath, parameters?: Model) { +function setRoute(context: DecoratorContext, entity: Type, details: RoutePath) { if ( !validateDecoratorTarget(context, entity, "@route", ["Namespace", "Interface", "Operation"]) ) { diff --git a/packages/rest/src/http/operations.ts b/packages/rest/src/http/operations.ts index eff7b398a91..6b0a62d8cba 100644 --- a/packages/rest/src/http/operations.ts +++ b/packages/rest/src/http/operations.ts @@ -10,6 +10,7 @@ import { Program, } from "@cadl-lang/compiler"; import { createDiagnostic, reportDiagnostic } from "../lib.js"; +import { getRoutePath } from "./decorators.js"; import { getResponsesForOperation } from "./responses.js"; import { resolvePathAndParameters } from "./route.js"; import { @@ -72,7 +73,7 @@ export function getAllHttpServices( }) ); - validateRouteUnique(diagnostics, httpOperations); + validateRouteUnique(program, diagnostics, httpOperations); const service: HttpService = { namespace: serviceNamespace, operations: httpOperations, @@ -100,7 +101,11 @@ export function reportIfNoRoutes(program: Program, routes: HttpOperation[]) { } } -export function validateRouteUnique(diagnostics: DiagnosticCollector, operations: HttpOperation[]) { +export function validateRouteUnique( + program: Program, + diagnostics: DiagnosticCollector, + operations: HttpOperation[] +) { const grouped = new Map>(); for (const operation of operations) { @@ -109,7 +114,8 @@ export function validateRouteUnique(diagnostics: DiagnosticCollector, operations if (operation.overloading !== undefined && isOverloadSameEndpoint(operation as any)) { continue; } - if (operation.pathShared) { + const pathShared = getRoutePath(program, operation.operation)?.shared ?? false; + if (pathShared) { continue; } let map = grouped.get(path); @@ -178,7 +184,6 @@ function getHttpOperationInternal( const httpOperation: HttpOperation = { path: route.path, pathSegments: route.pathSegments, - pathShared: route.shared, verb: route.parameters.verb, container: operation.interface ?? operation.namespace ?? program.getGlobalNamespaceType(), parameters: route.parameters, diff --git a/packages/rest/src/http/route.ts b/packages/rest/src/http/route.ts index 7342dd17de7..d002de172f2 100644 --- a/packages/rest/src/http/route.ts +++ b/packages/rest/src/http/route.ts @@ -1,5 +1,4 @@ import { - BooleanLiteral, createDiagnosticCollector, Diagnostic, Interface, @@ -107,7 +106,11 @@ export function resolvePathAndParameters( overloadBase: HttpOperation | undefined, options: RouteResolutionOptions ): [ - { path: string; pathSegments: string[]; parameters: HttpOperationParameters; shared: boolean }, + { + path: string; + pathSegments: string[]; + parameters: HttpOperationParameters; + }, readonly Diagnostic[] ] { let segments: string[] = []; @@ -162,14 +165,10 @@ export function resolvePathAndParameters( } } - const shared = - (getRoutePath(program, operation)?.parameters?.properties.get("shared")?.type as BooleanLiteral) - ?.value ?? false; return diagnostics.wrap({ path: buildPath(segments), pathSegments: segments, parameters, - shared, }); } diff --git a/packages/rest/src/http/types.ts b/packages/rest/src/http/types.ts index 4fe5546a4d0..76221089385 100644 --- a/packages/rest/src/http/types.ts +++ b/packages/rest/src/http/types.ts @@ -1,7 +1,6 @@ import { Interface, ListOperationOptions, - Model, ModelProperty, Namespace, Operation, @@ -223,11 +222,6 @@ export interface HttpOperation { */ pathSegments: string[]; - /** - * Whether the route path is shared with other operations - */ - pathShared: boolean; - /** * Route verb. */ @@ -267,7 +261,7 @@ export interface HttpOperation { export interface RoutePath { path: string; isReset: boolean; - parameters?: Model; + shared: boolean; } export type StatusCode = `${number}` | "*"; From a93ce2e2983ee10794ea72a60d13bad0fe5b2580 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 19 Oct 2022 12:43:36 -0700 Subject: [PATCH 6/6] Validate shared. --- packages/rest/src/http/decorators.ts | 22 +++++++++++++++++++--- packages/rest/src/lib.ts | 7 +++++++ packages/rest/test/http-decorators.test.ts | 12 ++++++++++++ 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/rest/src/http/decorators.ts b/packages/rest/src/http/decorators.ts index e6e3d69979e..aae025d5bd1 100644 --- a/packages/rest/src/http/decorators.ts +++ b/packages/rest/src/http/decorators.ts @@ -1,5 +1,4 @@ import { - BooleanLiteral, cadlTypeToJson, createDecoratorDefinition, createDiagnosticCollector, @@ -555,6 +554,23 @@ 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 * @@ -568,7 +584,7 @@ export function $route(context: DecoratorContext, entity: Type, path: string, pa setRoute(context, entity, { path, isReset: false, - shared: (parameters?.properties.get("shared")?.type as BooleanLiteral)?.value ?? false, + shared: extractSharedValue(context, parameters), }); } @@ -581,7 +597,7 @@ export function $routeReset( setRoute(context, entity, { path, isReset: true, - shared: (parameters?.properties.get("shared")?.type as BooleanLiteral)?.value ?? false, + shared: extractSharedValue(context, parameters), }); } diff --git a/packages/rest/src/lib.ts b/packages/rest/src/lib.ts index ba977cae78c..1bc21a5eac7 100644 --- a/packages/rest/src/lib.ts +++ b/packages/rest/src/lib.ts @@ -139,6 +139,13 @@ 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.", + }, + }, }, } as const; diff --git a/packages/rest/test/http-decorators.test.ts b/packages/rest/test/http-decorators.test.ts index 42a499cf77c..1b29d89fa7e 100644 --- a/packages/rest/test/http-decorators.test.ts +++ b/packages/rest/test/http-decorators.test.ts @@ -166,6 +166,18 @@ describe("rest: http decorators", () => { expectDiagnosticEmpty(diagnostics); }); + + it("emit diagnostic when wrong type for shared is provided", async () => { + const diagnostics = await runner.diagnose(` + @route("/test", {shared: "yes"}) op test(): string; + `); + expectDiagnostics(diagnostics, [ + { + code: "@cadl-lang/rest/shared-boolean", + message: `shared parameter must be a boolean.`, + }, + ]); + }); }); describe("@path", () => {