Skip to content

Commit

Permalink
Fix don't emit shared route error when verb don't match (#2948)
Browse files Browse the repository at this point in the history
fix [#2925](#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 <brian.terlson@microsoft.com>
  • Loading branch information
timotheeguerin and bterlson authored Feb 28, 2024
1 parent c9c1f3e commit 9d8cfb0
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 15 deletions.
8 changes: 8 additions & 0 deletions .chronus/changes/fix-shared-route-verb-2024-1-23-18-18-26.md
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion packages/http/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
59 changes: 47 additions & 12 deletions packages/http/src/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,61 @@ 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
const [services, diagnostics] = getAllHttpServices(program);
if (diagnostics.length > 0) {
program.reportDiagnostics(diagnostics);
}
validateSharedRouteConsistency(program, services);
}

function groupHttpOperations(
operations: HttpOperation[]
): Map<string, Map<string, HttpOperation[]>> {
const paths = new Map<string, Map<string, HttpOperation[]>>();

for (const operation of operations) {
const { verb, path } = operation;
let pathOps = paths.get(path);
if (pathOps === undefined) {
pathOps = new Map<string, HttpOperation[]>();
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<string, boolean>();
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 },
});
}
}
}
}
}
Expand Down
21 changes: 19 additions & 2 deletions packages/http/test/http-decorators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.`,
},
]);
});
Expand All @@ -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;
Expand Down

0 comments on commit 9d8cfb0

Please sign in to comment.