From 9d8cfb016e70accb73df928313a45b21cd081597 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 28 Feb 2024 14:17:46 -0800 Subject: [PATCH] Fix don't emit shared route error when verb don't match (#2948) fix [#2925](https://github.com/microsoft/typespec/issues/2925) Stop emitting the error if there is a shared route and a non shared route on a different verb. Also improve the error: - change message to be a little more clear - emit the error on every offending operation not just the first one we find an duplicate --------- Co-authored-by: Brian Terlson --- ...ix-shared-route-verb-2024-1-23-18-18-26.md | 8 +++ packages/http/src/lib.ts | 2 +- packages/http/src/validate.ts | 59 +++++++++++++++---- packages/http/test/http-decorators.test.ts | 21 ++++++- 4 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 .chronus/changes/fix-shared-route-verb-2024-1-23-18-18-26.md diff --git a/.chronus/changes/fix-shared-route-verb-2024-1-23-18-18-26.md b/.chronus/changes/fix-shared-route-verb-2024-1-23-18-18-26.md new file mode 100644 index 0000000000..ad0169835a --- /dev/null +++ b/.chronus/changes/fix-shared-route-verb-2024-1-23-18-18-26.md @@ -0,0 +1,8 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: fix +packages: + - "@typespec/http" +--- + +Fix don't emit shared route error when verb don't match diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index 0c5dbc6f41..3e1be350ab 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -102,7 +102,7 @@ export const $lib = createTypeSpecLibrary({ "shared-inconsistency": { severity: "error", messages: { - default: "All shared routes must agree on the value of the shared parameter.", + default: paramMessage`Each operation routed at "${"verb"} ${"path"}" needs to have the @sharedRoute decorator.`, }, }, "write-visibility-not-supported": { diff --git a/packages/http/src/validate.ts b/packages/http/src/validate.ts index 7bebe88938..2439f185a1 100644 --- a/packages/http/src/validate.ts +++ b/packages/http/src/validate.ts @@ -2,6 +2,7 @@ import { Program } from "@typespec/compiler"; import { reportDiagnostic } from "./lib.js"; import { getAllHttpServices } from "./operations.js"; import { isSharedRoute } from "./route.js"; +import { HttpOperation, HttpService } from "./types.js"; export function $onValidate(program: Program) { // Pass along any diagnostics that might be returned from the HTTP library @@ -9,19 +10,53 @@ export function $onValidate(program: Program) { if (diagnostics.length > 0) { program.reportDiagnostics(diagnostics); } + validateSharedRouteConsistency(program, services); +} + +function groupHttpOperations( + operations: HttpOperation[] +): Map> { + const paths = new Map>(); + + for (const operation of operations) { + const { verb, path } = operation; + let pathOps = paths.get(path); + if (pathOps === undefined) { + pathOps = new Map(); + paths.set(path, pathOps); + } + const ops = pathOps.get(verb); + if (ops === undefined) { + pathOps.set(verb, [operation]); + } else { + ops.push(operation); + } + } + return paths; +} +function validateSharedRouteConsistency(program: Program, services: HttpService[]) { for (const service of services) { - const paths = new Map(); - for (const operation of service.operations) { - const path = operation.path; - const shared = isSharedRoute(program, operation.operation); - const val = paths.get(path); - if (shared && val === undefined) { - paths.set(path, shared); - } else if (val && val !== shared) { - reportDiagnostic(program, { - code: "shared-inconsistency", - target: operation.operation, - }); + const paths = groupHttpOperations(service.operations); + for (const pathOps of paths.values()) { + for (const ops of pathOps.values()) { + let hasShared = false; + let hasNonShared = false; + for (const op of ops) { + if (isSharedRoute(program, op.operation)) { + hasShared = true; + } else { + hasNonShared = true; + } + } + if (hasShared && hasNonShared) { + for (const op of ops) { + reportDiagnostic(program, { + code: "shared-inconsistency", + target: op.operation, + format: { verb: op.verb, path: op.path }, + }); + } + } } } } diff --git a/packages/http/test/http-decorators.test.ts b/packages/http/test/http-decorators.test.ts index ac3a002c7f..c95a696c31 100644 --- a/packages/http/test/http-decorators.test.ts +++ b/packages/http/test/http-decorators.test.ts @@ -260,7 +260,7 @@ describe("http: decorators", () => { ]); }); - it("emit diagnostics when not all duplicated routes are declared shared", async () => { + it("emit diagnostics when not all duplicated routes are declared shared on each op conflicting", async () => { const diagnostics = await runner.diagnose(` @route("/test") @sharedRoute op test(): string; @route("/test") @sharedRoute op test2(): string; @@ -269,7 +269,15 @@ describe("http: decorators", () => { expectDiagnostics(diagnostics, [ { code: "@typespec/http/shared-inconsistency", - message: `All shared routes must agree on the value of the shared parameter.`, + message: `Each operation routed at "get /test" needs to have the @sharedRoute decorator.`, + }, + { + code: "@typespec/http/shared-inconsistency", + message: `Each operation routed at "get /test" needs to have the @sharedRoute decorator.`, + }, + { + code: "@typespec/http/shared-inconsistency", + message: `Each operation routed at "get /test" needs to have the @sharedRoute decorator.`, }, ]); }); @@ -283,6 +291,15 @@ describe("http: decorators", () => { expectDiagnosticEmpty(diagnostics); }); + it("do not emit diagnostics routes sharing path but not same verb", async () => { + const diagnostics = await runner.diagnose(` + @route("/test") @sharedRoute op test(): string; + @route("/test") @sharedRoute op test2(): string; + @route("/test") @post op test3(): string; + `); + 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;