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/decorators.ts b/packages/http/src/decorators.ts index 09efb1ab307..7db4a7048c4 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -1,3 +1,5 @@ +import { createDiagnostic, reportDiagnostic } from "./lib.js"; + import { DecoratorContext, Diagnostic, @@ -20,8 +22,8 @@ import { validateDecoratorTarget, validateDecoratorUniqueOnNode, } from "@typespec/compiler"; -import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; import { setRoute, setSharedRoute } from "./route.js"; +import { HttpStateKeys } from "./state.js"; import { AuthenticationOption, HeaderFieldOptions, @@ -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 ace2ee5814e..ac547b45193 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -1,6 +1,7 @@ import { createTypeSpecLibrary, paramMessage } from "@typespec/compiler"; +import { opReferenceContainerRouteRule } from "./rules/op-reference-container-route.js"; -const libDefinition = { +export const $lib = createTypeSpecLibrary({ name: "@typespec/http", diagnostics: { "http-verb-duplicate": { @@ -125,9 +126,16 @@ const libDefinition = { }, }, }, -} as const; - -const httpLib = createTypeSpecLibrary(libDefinition); -const { reportDiagnostic, createDiagnostic, createStateSymbol } = httpLib; + linter: { + rules: [opReferenceContainerRouteRule], + ruleSets: { + all: { + enable: { + [`@typespec/http/${opReferenceContainerRouteRule.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..ffe1d6dca3f 100644 --- a/packages/http/src/route.ts +++ b/packages/http/src/route.ts @@ -1,3 +1,5 @@ +import { createDiagnostic, reportDiagnostic } from "./lib.js"; + import { createDiagnosticCollector, DecoratorContext, @@ -9,8 +11,8 @@ import { Type, validateDecoratorTarget, } from "@typespec/compiler"; -import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js"; import { getOperationParameters } from "./parameters.js"; +import { HttpStateKeys } from "./state.js"; import { HttpOperation, HttpOperationParameters, @@ -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/op-reference-container-route.ts b/packages/http/src/rules/op-reference-container-route.ts new file mode 100644 index 00000000000..589a563b5e5 --- /dev/null +++ b/packages/http/src/rules/op-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 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", + 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/test/rules/op-reference-container-route.test.ts b/packages/http/test/rules/op-reference-container-route.test.ts new file mode 100644 index 00000000000..166c5fee759 --- /dev/null +++ b/packages/http/test/rules/op-reference-container-route.test.ts @@ -0,0 +1,94 @@ +import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing"; +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, opReferenceContainerRouteRule, "@typespec/http"); + }); + + it("emit a diagnostic when referenced op has a route prefix on its parent container", async () => { + // cspell:ignore IFoo + 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/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/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/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/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.', + }, + ]); + }); + + 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/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.', + }, + ]); + }); +});