From 9eb701939001a8ca20a5067eb28f94e762600441 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 29 Aug 2023 16:35:10 +0300 Subject: [PATCH 1/6] Emit diagnostic for any referenced operation with a container route --- .../no-route-containers_2023-08-29-13-37.json | 10 +++ packages/http/src/lib.ts | 6 ++ packages/http/src/validate.ts | 67 +++++++++++++++- packages/http/test/routes.test.ts | 80 +++++++++++++++++++ 4 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 common/changes/@typespec/http/no-route-containers_2023-08-29-13-37.json diff --git a/common/changes/@typespec/http/no-route-containers_2023-08-29-13-37.json b/common/changes/@typespec/http/no-route-containers_2023-08-29-13-37.json new file mode 100644 index 00000000000..4ff225416d4 --- /dev/null +++ b/common/changes/@typespec/http/no-route-containers_2023-08-29-13-37.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/http", + "comment": "Add validation step to check whether any operation references another operation with a route prefix defined on a parent container. This helps avoid unexpected route changes when using operation references.", + "type": "none" + } + ], + "packageName": "@typespec/http" +} \ No newline at end of file diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index ace2ee5814e..eecac0d75da 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -61,6 +61,12 @@ const libDefinition = { default: paramMessage`Duplicate operation "${"operationName"}" routed at "${"verb"} ${"path"}".`, }, }, + "operation-reference-container-route": { + severity: "warning", + messages: { + default: paramMessage`Operation ${"opName"} references an operation which has a @route prefix on its namespace or interface: "${"routePrefix"}". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.`, + }, + }, "status-code-invalid": { severity: "error", messages: { diff --git a/packages/http/src/validate.ts b/packages/http/src/validate.ts index 7bebe88938f..9f1525b12b1 100644 --- a/packages/http/src/validate.ts +++ b/packages/http/src/validate.ts @@ -1,7 +1,66 @@ -import { Program } from "@typespec/compiler"; +import { Operation, Program, navigateProgram } from "@typespec/compiler"; import { reportDiagnostic } from "./lib.js"; import { getAllHttpServices } from "./operations.js"; -import { isSharedRoute } from "./route.js"; +import { getRoutePath, isSharedRoute } from "./route.js"; +import { OperationContainer } from "./types.js"; + +function checkOperationReferenceContainerRoutes(program: Program) { + // This algorithm traces operation references for each operation encountered + // in the program. It will locate the first referenced operation which has a + // `@route` prefix on a parent namespace or interface. + const checkedOps = new Map(); + + function getContainerRoutePrefix( + container: OperationContainer | Operation | undefined + ): string | undefined { + if (container === undefined) { + return undefined; + } + + if (container.kind === "Operation") { + return ( + getContainerRoutePrefix(container.interface) ?? getContainerRoutePrefix(container.namespace) + ); + } + + const route = getRoutePath(program, container); + return route ? route.path : getContainerRoutePrefix(container.namespace); + } + + function checkOperationReferences(op: Operation | undefined, originalOp: Operation) { + if (op !== undefined) { + // Skip this reference if the original operation shares the same + // container with the referenced operation + const container = op.interface ?? op.namespace; + const originalContainer = originalOp.interface ?? originalOp.namespace; + if (container !== originalContainer) { + let route = checkedOps.get(op); + if (route === undefined) { + route = getContainerRoutePrefix(op); + checkedOps.set(op, route); + } + + if (route) { + reportDiagnostic(program, { + code: "operation-reference-container-route", + target: originalOp, + format: { opName: originalOp.name, routePrefix: route }, + }); + return; + } + } + + // Continue checking if the referenced operation didn't have a route prefix + checkOperationReferences(op.sourceOperation, originalOp); + } + } + + navigateProgram(program, { + operation: (op: Operation) => { + checkOperationReferences(op.sourceOperation, op); + }, + }); +} export function $onValidate(program: Program) { // Pass along any diagnostics that might be returned from the HTTP library @@ -25,4 +84,8 @@ export function $onValidate(program: Program) { } } } + + // Check for referenced (`op is`) operations which have a @route on one of + // their containers + checkOperationReferenceContainerRoutes(program); } diff --git a/packages/http/test/routes.test.ts b/packages/http/test/routes.test.ts index 5f12ade3370..6fa508c84a9 100644 --- a/packages/http/test/routes.test.ts +++ b/packages/http/test/routes.test.ts @@ -600,4 +600,84 @@ describe("http: routes", () => { strictEqual(getRoutePath(runner.program, get2)?.shared, false); }); }); + + describe("operation references", () => { + it("emit a diagnostic when referenced op has a route prefix on its parent container", async () => { + const [_, diagnostics] = await compileOperations(` + @route("/foo") + namespace Foo { + @route("/test") + op get(): string; + } + + @route("/ifoo") + interface IFoo { + op get(): string; + } + + @route("/bar") + namespace Bar { + interface IBar { + op get(): string; + } + } + + namespace Test { + @route("/get1") op get1 is Foo.get; + @route("/get2") op get2 is IFoo.get; + @route("/get3") op get3 is Bar.IBar.get; + @route("/get4") op get4 is get3; // Follow reference chain to find parent container + } + `); + + expectDiagnostics(diagnostics, [ + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get2 references an operation which has a @route prefix on its namespace or interface: "/ifoo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get3 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get4 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + ]); + }); + + it("does not emit for references inside of the same container", async () => { + const [_, diagnostics] = await compileOperations(` + @route("/foo") + namespace Foo { + @route("/test") + op get(): string; + } + + namespace Bar { + @route("/get1") op get1 is Foo.get; + } + + namespace Foo { + @route("/get2") op get2 is Foo.get; + } + `); + + expectDiagnostics(diagnostics, [ + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + ]); + }); + }); }); From ed0019f03a3e3513174441e2539463b31160cc70 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 5 Sep 2023 19:07:32 +0300 Subject: [PATCH 2/6] Use the new linting framework for this rule --- packages/http/src/decorators.ts | 77 +++++++-------- packages/http/src/index.ts | 2 +- packages/http/src/lib.ts | 26 ++--- packages/http/src/route.ts | 33 +++---- .../operation-reference-container-route.ts | 70 +++++++++++++ packages/http/src/state.ts | 28 ++++++ packages/http/src/validate.ts | 67 +------------ packages/http/test/routes.test.ts | 80 --------------- ...peration-reference-container-route.test.ts | 97 +++++++++++++++++++ packages/rest/src/rest.ts | 3 +- 10 files changed, 260 insertions(+), 223 deletions(-) create mode 100644 packages/http/src/rules/operation-reference-container-route.ts create mode 100644 packages/http/src/state.ts create mode 100644 packages/http/test/rules/operation-reference-container-route.test.ts diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index 09efb1ab307..7a998ae3a78 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -1,3 +1,6 @@ +import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; + +import { HttpStateKeys } from "./state.js"; import { DecoratorContext, Diagnostic, @@ -20,7 +23,6 @@ import { validateDecoratorTarget, validateDecoratorUniqueOnNode, } from "@typespec/compiler"; -import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; import { setRoute, setSharedRoute } from "./route.js"; import { AuthenticationOption, @@ -35,7 +37,6 @@ import { extractParamsFromPath } from "./utils.js"; export const namespace = "TypeSpec.Http"; -const headerFieldsKey = createStateSymbol("header"); export function $header( context: DecoratorContext, entity: ModelProperty, @@ -80,11 +81,11 @@ export function $header( target: context.decoratorTarget, }); } - context.program.stateMap(headerFieldsKey).set(entity, options); + context.program.stateMap(HttpStateKeys.headerFieldsKey).set(entity, options); } export function getHeaderFieldOptions(program: Program, entity: Type): HeaderFieldOptions { - return program.stateMap(headerFieldsKey).get(entity); + return program.stateMap(HttpStateKeys.headerFieldsKey).get(entity); } export function getHeaderFieldName(program: Program, entity: Type): string { @@ -92,10 +93,9 @@ export function getHeaderFieldName(program: Program, entity: Type): string { } export function isHeader(program: Program, entity: Type) { - return program.stateMap(headerFieldsKey).has(entity); + return program.stateMap(HttpStateKeys.headerFieldsKey).has(entity); } -const queryFieldsKey = createStateSymbol("query"); export function $query( context: DecoratorContext, entity: ModelProperty, @@ -129,11 +129,11 @@ export function $query( target: context.decoratorTarget, }); } - context.program.stateMap(queryFieldsKey).set(entity, options); + context.program.stateMap(HttpStateKeys.queryFieldsKey).set(entity, options); } export function getQueryParamOptions(program: Program, entity: Type): QueryParameterOptions { - return program.stateMap(queryFieldsKey).get(entity); + return program.stateMap(HttpStateKeys.queryFieldsKey).get(entity); } export function getQueryParamName(program: Program, entity: Type): string { @@ -141,20 +141,19 @@ export function getQueryParamName(program: Program, entity: Type): string { } export function isQueryParam(program: Program, entity: Type) { - return program.stateMap(queryFieldsKey).has(entity); + return program.stateMap(HttpStateKeys.queryFieldsKey).has(entity); } -const pathFieldsKey = createStateSymbol("path"); export function $path(context: DecoratorContext, entity: ModelProperty, paramName?: string) { const options: PathParameterOptions = { type: "path", name: paramName ?? entity.name, }; - context.program.stateMap(pathFieldsKey).set(entity, options); + context.program.stateMap(HttpStateKeys.pathFieldsKey).set(entity, options); } export function getPathParamOptions(program: Program, entity: Type): PathParameterOptions { - return program.stateMap(pathFieldsKey).get(entity); + return program.stateMap(HttpStateKeys.pathFieldsKey).get(entity); } export function getPathParamName(program: Program, entity: Type): string { @@ -162,21 +161,19 @@ export function getPathParamName(program: Program, entity: Type): string { } export function isPathParam(program: Program, entity: Type) { - return program.stateMap(pathFieldsKey).has(entity); + return program.stateMap(HttpStateKeys.pathFieldsKey).has(entity); } -const bodyFieldsKey = createStateSymbol("body"); export function $body(context: DecoratorContext, entity: ModelProperty) { - context.program.stateSet(bodyFieldsKey).add(entity); + context.program.stateSet(HttpStateKeys.bodyFieldsKey).add(entity); } export function isBody(program: Program, entity: Type): boolean { - return program.stateSet(bodyFieldsKey).has(entity); + return program.stateSet(HttpStateKeys.bodyFieldsKey).has(entity); } -const statusCodeKey = createStateSymbol("statusCode"); export function $statusCode(context: DecoratorContext, entity: ModelProperty) { - context.program.stateSet(statusCodeKey).add(entity); + context.program.stateSet(HttpStateKeys.statusCodeKey).add(entity); const codes: string[] = []; if (entity.type.kind === "String") { @@ -217,7 +214,7 @@ export function $statusCode(context: DecoratorContext, entity: ModelProperty) { } export function setStatusCode(program: Program, entity: Model | ModelProperty, codes: string[]) { - program.stateMap(statusCodeKey).set(entity, codes); + program.stateMap(HttpStateKeys.statusCodeKey).set(entity, codes); } // Check status code value: 3 digits with first digit in [1-5] @@ -236,11 +233,11 @@ function validStatusCode(program: Program, code: string, entity: Type): boolean } export function isStatusCode(program: Program, entity: Type) { - return program.stateMap(statusCodeKey).has(entity); + return program.stateMap(HttpStateKeys.statusCodeKey).has(entity); } export function getStatusCodes(program: Program, entity: Type): string[] { - return program.stateMap(statusCodeKey).get(entity) ?? []; + return program.stateMap(HttpStateKeys.statusCodeKey).get(entity) ?? []; } // Reference: https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html @@ -291,12 +288,10 @@ export function getStatusCodeDescription(statusCode: string) { return undefined; } -const operationVerbsKey = createStateSymbol("verbs"); - function setOperationVerb(program: Program, entity: Type, verb: HttpVerb): void { if (entity.kind === "Operation") { - if (!program.stateMap(operationVerbsKey).has(entity)) { - program.stateMap(operationVerbsKey).set(entity, verb); + if (!program.stateMap(HttpStateKeys.operationVerbsKey).has(entity)) { + program.stateMap(HttpStateKeys.operationVerbsKey).set(entity, verb); } else { reportDiagnostic(program, { code: "http-verb-duplicate", @@ -314,7 +309,7 @@ function setOperationVerb(program: Program, entity: Type, verb: HttpVerb): void } export function getOperationVerb(program: Program, entity: Type): HttpVerb | undefined { - return program.stateMap(operationVerbsKey).get(entity); + return program.stateMap(HttpStateKeys.operationVerbsKey).get(entity); } export function $get(context: DecoratorContext, entity: Operation) { @@ -347,7 +342,6 @@ export interface HttpServer { parameters: Map; } -const serversKey = createStateSymbol("servers"); /** * Configure the server url for the service. * @param context Decorator context @@ -376,10 +370,10 @@ export function $server( } } - let servers: HttpServer[] = context.program.stateMap(serversKey).get(target); + let servers: HttpServer[] = context.program.stateMap(HttpStateKeys.serversKey).get(target); if (servers === undefined) { servers = []; - context.program.stateMap(serversKey).set(target, servers); + context.program.stateMap(HttpStateKeys.serversKey).set(target, servers); } servers.push({ url, @@ -389,7 +383,7 @@ export function $server( } export function getServers(program: Program, type: Namespace): HttpServer[] | undefined { - return program.stateMap(serversKey).get(type); + return program.stateMap(HttpStateKeys.serversKey).get(type); } export function $plainData(context: DecoratorContext, entity: Model) { @@ -397,11 +391,11 @@ export function $plainData(context: DecoratorContext, entity: Model) { const decoratorsToRemove = ["$header", "$body", "$query", "$path", "$statusCode"]; const [headers, bodies, queries, paths, statusCodes] = [ - program.stateMap(headerFieldsKey), - program.stateSet(bodyFieldsKey), - program.stateMap(queryFieldsKey), - program.stateMap(pathFieldsKey), - program.stateMap(statusCodeKey), + program.stateMap(HttpStateKeys.headerFieldsKey), + program.stateSet(HttpStateKeys.bodyFieldsKey), + program.stateMap(HttpStateKeys.queryFieldsKey), + program.stateMap(HttpStateKeys.pathFieldsKey), + program.stateMap(HttpStateKeys.statusCodeKey), ]; for (const property of entity.properties.values()) { @@ -422,7 +416,6 @@ export function $plainData(context: DecoratorContext, entity: Model) { setTypeSpecNamespace("Private", $plainData); -const authenticationKey = createStateSymbol("authentication"); export function $useAuth( context: DecoratorContext, serviceNamespace: Namespace, @@ -440,7 +433,7 @@ export function setAuthentication( serviceNamespace: Namespace, auth: ServiceAuthentication ) { - program.stateMap(authenticationKey).set(serviceNamespace, auth); + program.stateMap(HttpStateKeys.authenticationKey).set(serviceNamespace, auth); } function extractServiceAuthentication( @@ -573,7 +566,7 @@ export function getAuthentication( program: Program, namespace: Namespace ): ServiceAuthentication | undefined { - return program.stateMap(authenticationKey).get(namespace); + return program.stateMap(HttpStateKeys.authenticationKey).get(namespace); } /** @@ -622,10 +615,6 @@ export function $sharedRoute(context: DecoratorContext, entity: Operation) { setSharedRoute(context.program, entity); } -const includeInapplicableMetadataInPayloadKey = createStateSymbol( - "includeInapplicableMetadataInPayload" -); - /** * Specifies if inapplicable metadata should be included in the payload for * the given entity. This is true by default unless changed by this @@ -655,7 +644,7 @@ export function $includeInapplicableMetadataInPayload( ) { return; } - const state = context.program.stateMap(includeInapplicableMetadataInPayloadKey); + const state = context.program.stateMap(HttpStateKeys.includeInapplicableMetadataInPayloadKey); state.set(entity, value); } @@ -672,7 +661,7 @@ export function includeInapplicableMetadataInPayload( ): boolean { let e: ModelProperty | Namespace | Model | undefined; for (e = property; e; e = e.kind === "ModelProperty" ? e.model : e.namespace) { - const value = program.stateMap(includeInapplicableMetadataInPayloadKey).get(e); + const value = program.stateMap(HttpStateKeys.includeInapplicableMetadataInPayloadKey).get(e); if (value !== undefined) { return value; } diff --git a/packages/http/src/index.ts b/packages/http/src/index.ts index 518b109b316..d51630c28ae 100644 --- a/packages/http/src/index.ts +++ b/packages/http/src/index.ts @@ -1,4 +1,4 @@ -export const namespace = "TypeSpec.Http"; +export { $lib } from "./lib.js"; export * from "./content-types.js"; export * from "./decorators.js"; diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index eecac0d75da..2fb3171654e 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -1,6 +1,7 @@ import { createTypeSpecLibrary, paramMessage } from "@typespec/compiler"; +import { operationReferenceContainerRouteRule } from "./rules/operation-reference-container-route.js"; -const libDefinition = { +export const $lib = createTypeSpecLibrary({ name: "@typespec/http", diagnostics: { "http-verb-duplicate": { @@ -61,12 +62,6 @@ const libDefinition = { default: paramMessage`Duplicate operation "${"operationName"}" routed at "${"verb"} ${"path"}".`, }, }, - "operation-reference-container-route": { - severity: "warning", - messages: { - default: paramMessage`Operation ${"opName"} references an operation which has a @route prefix on its namespace or interface: "${"routePrefix"}". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.`, - }, - }, "status-code-invalid": { severity: "error", messages: { @@ -131,9 +126,16 @@ const libDefinition = { }, }, }, -} as const; - -const httpLib = createTypeSpecLibrary(libDefinition); -const { reportDiagnostic, createDiagnostic, createStateSymbol } = httpLib; + linter: { + rules: [operationReferenceContainerRouteRule], + ruleSets: { + all: { + enable: { + [`@typespec/http/${operationReferenceContainerRouteRule.name}`]: true, + }, + }, + }, + }, +}); -export { createDiagnostic, createStateSymbol, httpLib, reportDiagnostic }; +export const { reportDiagnostic, createDiagnostic, createStateSymbol } = $lib; diff --git a/packages/http/src/route.ts b/packages/http/src/route.ts index d8b9d9d77a8..63bd399b2ae 100644 --- a/packages/http/src/route.ts +++ b/packages/http/src/route.ts @@ -1,3 +1,6 @@ +import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; + +import { HttpStateKeys } from "./state.js"; import { createDiagnosticCollector, DecoratorContext, @@ -9,7 +12,6 @@ import { Type, validateDecoratorTarget, } from "@typespec/compiler"; -import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; import { getOperationParameters } from "./parameters.js"; import { HttpOperation, @@ -129,7 +131,6 @@ function getRouteSegments( return diagnostics.wrap(result); } -const externalInterfaces = createStateSymbol("externalInterfaces"); /** * @deprecated DO NOT USE. For internal use only as a workaround. * @param program Program @@ -141,17 +142,15 @@ export function includeInterfaceRoutesInNamespace( target: Namespace, sourceInterface: string ) { - let array = program.stateMap(externalInterfaces).get(target); + let array = program.stateMap(HttpStateKeys.externalInterfaces).get(target); if (array === undefined) { array = []; - program.stateMap(externalInterfaces).set(target, array); + program.stateMap(HttpStateKeys.externalInterfaces).set(target, array); } array.push(sourceInterface); } -const routeProducerKey = createStateSymbol("routeProducer"); - export function DefaultRouteProducer( program: Program, operation: Operation, @@ -197,15 +196,13 @@ export function setRouteProducer( operation: Operation, routeProducer: RouteProducer ): void { - program.stateMap(routeProducerKey).set(operation, routeProducer); + program.stateMap(HttpStateKeys.routeProducerKey).set(operation, routeProducer); } export function getRouteProducer(program: Program, operation: Operation): RouteProducer { - return program.stateMap(routeProducerKey).get(operation); + return program.stateMap(HttpStateKeys.routeProducerKey).get(operation); } -const routesKey = createStateSymbol("routes"); - export function setRoute(context: DecoratorContext, entity: Type, details: RoutePath) { if ( !validateDecoratorTarget(context, entity, "@route", ["Namespace", "Interface", "Operation"]) @@ -213,7 +210,7 @@ export function setRoute(context: DecoratorContext, entity: Type, details: Route return; } - const state = context.program.stateMap(routesKey); + const state = context.program.stateMap(HttpStateKeys.routesKey); if (state.has(entity) && entity.kind === "Namespace") { const existingPath: string | undefined = state.get(entity); @@ -232,21 +229,19 @@ export function setRoute(context: DecoratorContext, entity: Type, details: Route } } -const sharedRoutesKey = createStateSymbol("sharedRoutes"); - export function setSharedRoute(program: Program, operation: Operation) { - program.stateMap(sharedRoutesKey).set(operation, true); + program.stateMap(HttpStateKeys.sharedRoutesKey).set(operation, true); } export function isSharedRoute(program: Program, operation: Operation): boolean { - return program.stateMap(sharedRoutesKey).get(operation) === true; + return program.stateMap(HttpStateKeys.sharedRoutesKey).get(operation) === true; } export function getRoutePath( program: Program, entity: Namespace | Interface | Operation ): RoutePath | undefined { - const path = program.stateMap(routesKey).get(entity); + const path = program.stateMap(HttpStateKeys.routesKey).get(entity); return path ? { path, @@ -255,19 +250,17 @@ export function getRoutePath( : undefined; } -const routeOptionsKey = createStateSymbol("routeOptions"); - export function setRouteOptionsForNamespace( program: Program, namespace: Namespace, options: RouteOptions ) { - program.stateMap(routeOptionsKey).set(namespace, options); + program.stateMap(HttpStateKeys.routeOptionsKey).set(namespace, options); } export function getRouteOptionsForNamespace( program: Program, namespace: Namespace ): RouteOptions | undefined { - return program.stateMap(routeOptionsKey).get(namespace); + return program.stateMap(HttpStateKeys.routeOptionsKey).get(namespace); } diff --git a/packages/http/src/rules/operation-reference-container-route.ts b/packages/http/src/rules/operation-reference-container-route.ts new file mode 100644 index 00000000000..1f5a6b009c9 --- /dev/null +++ b/packages/http/src/rules/operation-reference-container-route.ts @@ -0,0 +1,70 @@ +import { Operation, createRule, paramMessage } from "@typespec/compiler"; +import { getRoutePath } from "../route.js"; +import { OperationContainer } from "../types.js"; + +export const operationReferenceContainerRouteRule = createRule({ + name: "operation-reference-container-route", + description: + "Check for referenced (`op is`) operations which have a @route on one of their containers.", + severity: "warning", + messages: { + default: paramMessage`Operation ${"opName"} references an operation which has a @route prefix on its namespace or interface: "${"routePrefix"}". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.`, + }, + create(context) { + // This algorithm traces operation references for each operation encountered + // in the program. It will locate the first referenced operation which has a + // `@route` prefix on a parent namespace or interface. + const checkedOps = new Map(); + + function getContainerRoutePrefix( + container: OperationContainer | Operation | undefined + ): string | undefined { + if (container === undefined) { + return undefined; + } + + if (container.kind === "Operation") { + return ( + getContainerRoutePrefix(container.interface) ?? + getContainerRoutePrefix(container.namespace) + ); + } + + const route = getRoutePath(context.program, container); + return route ? route.path : getContainerRoutePrefix(container.namespace); + } + + function checkOperationReferences(op: Operation | undefined, originalOp: Operation) { + if (op !== undefined) { + // Skip this reference if the original operation shares the same + // container with the referenced operation + const container = op.interface ?? op.namespace; + const originalContainer = originalOp.interface ?? originalOp.namespace; + if (container !== originalContainer) { + let route = checkedOps.get(op); + if (route === undefined) { + route = getContainerRoutePrefix(op); + checkedOps.set(op, route); + } + + if (route) { + context.reportDiagnostic({ + target: originalOp, + format: { opName: originalOp.name, routePrefix: route }, + }); + return; + } + } + + // Continue checking if the referenced operation didn't have a route prefix + checkOperationReferences(op.sourceOperation, originalOp); + } + } + + return { + operation: (op: Operation) => { + checkOperationReferences(op.sourceOperation, op); + }, + }; + }, +}); diff --git a/packages/http/src/state.ts b/packages/http/src/state.ts new file mode 100644 index 00000000000..775e188ac0f --- /dev/null +++ b/packages/http/src/state.ts @@ -0,0 +1,28 @@ +// FIXME - This is a workaround for the circular dependency issue when loading +// createStateSymbol. +// Issue: https://github.com/microsoft/typespec/issues/2301 +function httpCreateStateSymbol(name: string): symbol { + return Symbol.for(`@typespec/http.${name}`); +} + +export const HttpStateKeys = { + // decorators.ts + authenticationKey: httpCreateStateSymbol("authentication"), + headerFieldsKey: httpCreateStateSymbol("header"), + queryFieldsKey: httpCreateStateSymbol("query"), + pathFieldsKey: httpCreateStateSymbol("path"), + bodyFieldsKey: httpCreateStateSymbol("body"), + statusCodeKey: httpCreateStateSymbol("statusCode"), + operationVerbsKey: httpCreateStateSymbol("verbs"), + serversKey: httpCreateStateSymbol("servers"), + includeInapplicableMetadataInPayloadKey: httpCreateStateSymbol( + "includeInapplicableMetadataInPayload" + ), + + // route.ts + externalInterfaces: httpCreateStateSymbol("externalInterfaces"), + routeProducerKey: httpCreateStateSymbol("routeProducer"), + routesKey: httpCreateStateSymbol("routes"), + sharedRoutesKey: httpCreateStateSymbol("sharedRoutes"), + routeOptionsKey: httpCreateStateSymbol("routeOptions"), +}; diff --git a/packages/http/src/validate.ts b/packages/http/src/validate.ts index 9f1525b12b1..7bebe88938f 100644 --- a/packages/http/src/validate.ts +++ b/packages/http/src/validate.ts @@ -1,66 +1,7 @@ -import { Operation, Program, navigateProgram } from "@typespec/compiler"; +import { Program } from "@typespec/compiler"; import { reportDiagnostic } from "./lib.js"; import { getAllHttpServices } from "./operations.js"; -import { getRoutePath, isSharedRoute } from "./route.js"; -import { OperationContainer } from "./types.js"; - -function checkOperationReferenceContainerRoutes(program: Program) { - // This algorithm traces operation references for each operation encountered - // in the program. It will locate the first referenced operation which has a - // `@route` prefix on a parent namespace or interface. - const checkedOps = new Map(); - - function getContainerRoutePrefix( - container: OperationContainer | Operation | undefined - ): string | undefined { - if (container === undefined) { - return undefined; - } - - if (container.kind === "Operation") { - return ( - getContainerRoutePrefix(container.interface) ?? getContainerRoutePrefix(container.namespace) - ); - } - - const route = getRoutePath(program, container); - return route ? route.path : getContainerRoutePrefix(container.namespace); - } - - function checkOperationReferences(op: Operation | undefined, originalOp: Operation) { - if (op !== undefined) { - // Skip this reference if the original operation shares the same - // container with the referenced operation - const container = op.interface ?? op.namespace; - const originalContainer = originalOp.interface ?? originalOp.namespace; - if (container !== originalContainer) { - let route = checkedOps.get(op); - if (route === undefined) { - route = getContainerRoutePrefix(op); - checkedOps.set(op, route); - } - - if (route) { - reportDiagnostic(program, { - code: "operation-reference-container-route", - target: originalOp, - format: { opName: originalOp.name, routePrefix: route }, - }); - return; - } - } - - // Continue checking if the referenced operation didn't have a route prefix - checkOperationReferences(op.sourceOperation, originalOp); - } - } - - navigateProgram(program, { - operation: (op: Operation) => { - checkOperationReferences(op.sourceOperation, op); - }, - }); -} +import { isSharedRoute } from "./route.js"; export function $onValidate(program: Program) { // Pass along any diagnostics that might be returned from the HTTP library @@ -84,8 +25,4 @@ export function $onValidate(program: Program) { } } } - - // Check for referenced (`op is`) operations which have a @route on one of - // their containers - checkOperationReferenceContainerRoutes(program); } diff --git a/packages/http/test/routes.test.ts b/packages/http/test/routes.test.ts index 6fa508c84a9..5f12ade3370 100644 --- a/packages/http/test/routes.test.ts +++ b/packages/http/test/routes.test.ts @@ -600,84 +600,4 @@ describe("http: routes", () => { strictEqual(getRoutePath(runner.program, get2)?.shared, false); }); }); - - describe("operation references", () => { - it("emit a diagnostic when referenced op has a route prefix on its parent container", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/foo") - namespace Foo { - @route("/test") - op get(): string; - } - - @route("/ifoo") - interface IFoo { - op get(): string; - } - - @route("/bar") - namespace Bar { - interface IBar { - op get(): string; - } - } - - namespace Test { - @route("/get1") op get1 is Foo.get; - @route("/get2") op get2 is IFoo.get; - @route("/get3") op get3 is Bar.IBar.get; - @route("/get4") op get4 is get3; // Follow reference chain to find parent container - } - `); - - expectDiagnostics(diagnostics, [ - { - code: "@typespec/http/operation-reference-container-route", - message: - 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', - }, - { - code: "@typespec/http/operation-reference-container-route", - message: - 'Operation get2 references an operation which has a @route prefix on its namespace or interface: "/ifoo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', - }, - { - code: "@typespec/http/operation-reference-container-route", - message: - 'Operation get3 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', - }, - { - code: "@typespec/http/operation-reference-container-route", - message: - 'Operation get4 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', - }, - ]); - }); - - it("does not emit for references inside of the same container", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/foo") - namespace Foo { - @route("/test") - op get(): string; - } - - namespace Bar { - @route("/get1") op get1 is Foo.get; - } - - namespace Foo { - @route("/get2") op get2 is Foo.get; - } - `); - - expectDiagnostics(diagnostics, [ - { - code: "@typespec/http/operation-reference-container-route", - message: - 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', - }, - ]); - }); - }); }); diff --git a/packages/http/test/rules/operation-reference-container-route.test.ts b/packages/http/test/rules/operation-reference-container-route.test.ts new file mode 100644 index 00000000000..eb28598fd33 --- /dev/null +++ b/packages/http/test/rules/operation-reference-container-route.test.ts @@ -0,0 +1,97 @@ +import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing"; +import { operationReferenceContainerRouteRule } from "../../src/rules/operation-reference-container-route.js"; +import { createHttpTestRunner } from "../test-host.js"; + +describe("operation reference route container rule", () => { + let ruleTester: LinterRuleTester; + beforeEach(async () => { + const runner = await createHttpTestRunner(); + ruleTester = createLinterRuleTester( + runner, + operationReferenceContainerRouteRule, + "@typespec/http" + ); + }); + + it("emit a diagnostic when referenced op has a route prefix on its parent container", async () => { + await ruleTester + .expect( + ` + @route("/foo") + namespace Foo { + @route("/test") + op get(): string; + } + + @route("/ifoo") + interface IFoo { + op get(): string; + } + + @route("/bar") + namespace Bar { + interface IBar { + op get(): string; + } + } + + namespace Test { + @route("/get1") op get1 is Foo.get; + @route("/get2") op get2 is IFoo.get; + @route("/get3") op get3 is Bar.IBar.get; + @route("/get4") op get4 is get3; // Follow reference chain to find parent container + } + ` + ) + .toEmitDiagnostics([ + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get2 references an operation which has a @route prefix on its namespace or interface: "/ifoo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get3 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get4 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + ]); + }); + + it("does not emit for references inside of the same container", async () => { + await ruleTester + .expect( + ` + @route("/foo") + namespace Foo { + @route("/test") + op get(): string; + } + + namespace Bar { + @route("/get1") op get1 is Foo.get; + } + + namespace Foo { + @route("/get2") op get2 is Foo.get; + } + ` + ) + .toEmitDiagnostics([ + { + code: "@typespec/http/operation-reference-container-route", + message: + 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', + }, + ]); + }); +}); diff --git a/packages/rest/src/rest.ts b/packages/rest/src/rest.ts index a416c064419..1521d494e42 100644 --- a/packages/rest/src/rest.ts +++ b/packages/rest/src/rest.ts @@ -1,3 +1,5 @@ +import { createStateSymbol, reportDiagnostic } from "./lib.js"; + import { createDiagnosticCollector, DecoratorContext, @@ -25,7 +27,6 @@ import { RouteProducerResult, setRouteProducer, } from "@typespec/http"; -import { createStateSymbol, reportDiagnostic } from "./lib.js"; import { getResourceTypeKey } from "./resource.js"; // ----------------- @autoRoute ----------------- From d8d0598af35cc8547b7763e1f4e9c3214f984cac Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 5 Sep 2023 19:10:07 +0300 Subject: [PATCH 3/6] Format code --- packages/http/src/decorators.ts | 4 ++-- packages/http/src/route.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index 7a998ae3a78..7db4a7048c4 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -1,6 +1,5 @@ -import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; +import { createDiagnostic, reportDiagnostic } from "./lib.js"; -import { HttpStateKeys } from "./state.js"; import { DecoratorContext, Diagnostic, @@ -24,6 +23,7 @@ import { validateDecoratorUniqueOnNode, } from "@typespec/compiler"; import { setRoute, setSharedRoute } from "./route.js"; +import { HttpStateKeys } from "./state.js"; import { AuthenticationOption, HeaderFieldOptions, diff --git a/packages/http/src/route.ts b/packages/http/src/route.ts index 63bd399b2ae..ffe1d6dca3f 100644 --- a/packages/http/src/route.ts +++ b/packages/http/src/route.ts @@ -1,6 +1,5 @@ -import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; +import { createDiagnostic, reportDiagnostic } from "./lib.js"; -import { HttpStateKeys } from "./state.js"; import { createDiagnosticCollector, DecoratorContext, @@ -13,6 +12,7 @@ import { validateDecoratorTarget, } from "@typespec/compiler"; import { getOperationParameters } from "./parameters.js"; +import { HttpStateKeys } from "./state.js"; import { HttpOperation, HttpOperationParameters, From 0ab93d02f4f2794d300b89e12f4486c1856d64e1 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 5 Sep 2023 19:55:23 +0300 Subject: [PATCH 4/6] Ignore cspell --- .../http/test/rules/operation-reference-container-route.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/http/test/rules/operation-reference-container-route.test.ts b/packages/http/test/rules/operation-reference-container-route.test.ts index eb28598fd33..1dff731be0b 100644 --- a/packages/http/test/rules/operation-reference-container-route.test.ts +++ b/packages/http/test/rules/operation-reference-container-route.test.ts @@ -14,6 +14,7 @@ describe("operation reference route container rule", () => { }); it("emit a diagnostic when referenced op has a route prefix on its parent container", async () => { + // cspell:ignore IFoo await ruleTester .expect( ` From 5879a99300dd36a4cf4ff28513c3d8eb541bfe12 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 6 Sep 2023 13:14:17 +0300 Subject: [PATCH 5/6] Remove unneeded rest library change --- packages/rest/src/rest.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/rest/src/rest.ts b/packages/rest/src/rest.ts index 1521d494e42..a416c064419 100644 --- a/packages/rest/src/rest.ts +++ b/packages/rest/src/rest.ts @@ -1,5 +1,3 @@ -import { createStateSymbol, reportDiagnostic } from "./lib.js"; - import { createDiagnosticCollector, DecoratorContext, @@ -27,6 +25,7 @@ import { RouteProducerResult, setRouteProducer, } from "@typespec/http"; +import { createStateSymbol, reportDiagnostic } from "./lib.js"; import { getResourceTypeKey } from "./resource.js"; // ----------------- @autoRoute ----------------- From 73fd95ac0c6561ee39fedfcd539abf7ca09da838 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 11 Sep 2023 17:26:24 +0300 Subject: [PATCH 6/6] Rename rule to `op-reference-container-route` --- packages/http/src/lib.ts | 6 +++--- ...oute.ts => op-reference-container-route.ts} | 4 ++-- ...ts => op-reference-container-route.test.ts} | 18 +++++++----------- 3 files changed, 12 insertions(+), 16 deletions(-) rename packages/http/src/rules/{operation-reference-container-route.ts => op-reference-container-route.ts} (95%) rename packages/http/test/rules/{operation-reference-container-route.test.ts => op-reference-container-route.test.ts} (83%) diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index 2fb3171654e..ac547b45193 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -1,5 +1,5 @@ import { createTypeSpecLibrary, paramMessage } from "@typespec/compiler"; -import { operationReferenceContainerRouteRule } from "./rules/operation-reference-container-route.js"; +import { opReferenceContainerRouteRule } from "./rules/op-reference-container-route.js"; export const $lib = createTypeSpecLibrary({ name: "@typespec/http", @@ -127,11 +127,11 @@ export const $lib = createTypeSpecLibrary({ }, }, linter: { - rules: [operationReferenceContainerRouteRule], + rules: [opReferenceContainerRouteRule], ruleSets: { all: { enable: { - [`@typespec/http/${operationReferenceContainerRouteRule.name}`]: true, + [`@typespec/http/${opReferenceContainerRouteRule.name}`]: true, }, }, }, diff --git a/packages/http/src/rules/operation-reference-container-route.ts b/packages/http/src/rules/op-reference-container-route.ts similarity index 95% rename from packages/http/src/rules/operation-reference-container-route.ts rename to packages/http/src/rules/op-reference-container-route.ts index 1f5a6b009c9..589a563b5e5 100644 --- a/packages/http/src/rules/operation-reference-container-route.ts +++ b/packages/http/src/rules/op-reference-container-route.ts @@ -2,8 +2,8 @@ import { Operation, createRule, paramMessage } from "@typespec/compiler"; import { getRoutePath } from "../route.js"; import { OperationContainer } from "../types.js"; -export const operationReferenceContainerRouteRule = createRule({ - name: "operation-reference-container-route", +export const opReferenceContainerRouteRule = createRule({ + name: "op-reference-container-route", description: "Check for referenced (`op is`) operations which have a @route on one of their containers.", severity: "warning", diff --git a/packages/http/test/rules/operation-reference-container-route.test.ts b/packages/http/test/rules/op-reference-container-route.test.ts similarity index 83% rename from packages/http/test/rules/operation-reference-container-route.test.ts rename to packages/http/test/rules/op-reference-container-route.test.ts index 1dff731be0b..166c5fee759 100644 --- a/packages/http/test/rules/operation-reference-container-route.test.ts +++ b/packages/http/test/rules/op-reference-container-route.test.ts @@ -1,16 +1,12 @@ import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing"; -import { operationReferenceContainerRouteRule } from "../../src/rules/operation-reference-container-route.js"; +import { opReferenceContainerRouteRule } from "../../src/rules/op-reference-container-route.js"; import { createHttpTestRunner } from "../test-host.js"; describe("operation reference route container rule", () => { let ruleTester: LinterRuleTester; beforeEach(async () => { const runner = await createHttpTestRunner(); - ruleTester = createLinterRuleTester( - runner, - operationReferenceContainerRouteRule, - "@typespec/http" - ); + ruleTester = createLinterRuleTester(runner, opReferenceContainerRouteRule, "@typespec/http"); }); it("emit a diagnostic when referenced op has a route prefix on its parent container", async () => { @@ -46,22 +42,22 @@ describe("operation reference route container rule", () => { ) .toEmitDiagnostics([ { - code: "@typespec/http/operation-reference-container-route", + code: "@typespec/http/op-reference-container-route", message: 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', }, { - code: "@typespec/http/operation-reference-container-route", + code: "@typespec/http/op-reference-container-route", message: 'Operation get2 references an operation which has a @route prefix on its namespace or interface: "/ifoo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', }, { - code: "@typespec/http/operation-reference-container-route", + code: "@typespec/http/op-reference-container-route", message: 'Operation get3 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', }, { - code: "@typespec/http/operation-reference-container-route", + code: "@typespec/http/op-reference-container-route", message: 'Operation get4 references an operation which has a @route prefix on its namespace or interface: "/bar". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', }, @@ -89,7 +85,7 @@ describe("operation reference route container rule", () => { ) .toEmitDiagnostics([ { - code: "@typespec/http/operation-reference-container-route", + code: "@typespec/http/op-reference-container-route", message: 'Operation get1 references an operation which has a @route prefix on its namespace or interface: "/foo". This operation will not carry forward the route prefix so the final route may be different than the referenced operation.', },