Skip to content

Commit

Permalink
rest - filter string constant path http properties when using `@autoR…
Browse files Browse the repository at this point in the history
…oute` (microsoft#6326)

microsoft#6311 switched openapi3 to use `HttpProperty` instead of
`HttpOperationParameter`. The `@typespec/rest` package did some
filtering of parameters under a couple conditions (including path
constants) that wasn't applied to properties. This caused an extra path
parameter to be emitted in OpenAPI3 (and typespec-autorest when
eventually applying the same changes there).

This PR applies the same `@autoRoute` parameter filtering to properties.

---------

Co-authored-by: Christopher Radek <[email protected]>
  • Loading branch information
2 people authored and Mingzhe Huang (from Dev Box) committed Mar 10, 2025
1 parent 304bc57 commit d733c75
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 8 deletions.
7 changes: 7 additions & 0 deletions .chronus/changes/rest-prop-filtering-2025-2-7-9-50-32.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: fix
packages:
- "@typespec/rest"
---

Updates `@autoRoute` behavior to apply same HttpOperationParameter filtering to HttpProperty
12 changes: 12 additions & 0 deletions packages/rest/src/rest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ function autoRouteProducer(
const routePath = getRoutePath(program, operation)?.path;
const segments = [...parentSegments, ...(routePath ? [routePath] : [])];
const filteredParameters: HttpOperationParameter[] = [];
const filteredParamProperties = new Set<ModelProperty>();
const paramOptions = {
...(options?.paramOptions ?? {}),
verbSelector: getResourceOperationHttpVerb,
Expand Down Expand Up @@ -146,10 +147,21 @@ function autoRouteProducer(

// Push all usable parameters to the filtered list
filteredParameters.push(httpParam);
filteredParamProperties.add(httpParam.param);
}

// Replace the original parameters with filtered set
parameters.parameters = filteredParameters;
// Remove any header/query/path/cookie properties that aren't in the filtered list
for (let i = parameters.properties.length - 1; i >= 0; i--) {
const httpProp = parameters.properties[i];
if (!["header", "query", "path", "cookie"].includes(httpProp.kind)) {
continue;
}
if (!filteredParamProperties.has(httpProp.property)) {
parameters.properties.splice(i, 1);
}
}

// Add the operation's own segment if present
addSegmentFragment(program, operation, segments);
Expand Down
60 changes: 52 additions & 8 deletions packages/rest/test/routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ describe("rest: routes", () => {
});

it("autoRoute operations filter out path parameters with a string literal type", async () => {
const routes = await getRoutesFor(
const operations = await getOperations(
`
model ThingId {
@path
Expand All @@ -167,16 +167,60 @@ describe("rest: routes", () => {
}
`,
);

deepStrictEqual(routes, [
{
verb: "get",
path: "/subscriptions/{subscriptionId}/providers/Microsoft.Things/things/{thingId}",
params: ["subscriptionId", "thingId"],
},
expect(operations.length).toBe(1);
const operation = operations[0];
expect(operation.verb).toBe("get");
expect(operation.path).toBe(
"/subscriptions/{subscriptionId}/providers/Microsoft.Things/things/{thingId}",
);
const operationParams = operation.parameters.parameters;
const operationProps = operation.parameters.properties;
expect(operationParams.map((p) => p.name)).toEqual(["subscriptionId", "thingId"]);
expect(operationProps.map((p) => (p as any).options.name)).toEqual([
"subscriptionId",
"thingId",
]);
});

it("autoRoute operations does not filter out non-param http properties", async () => {
const operations = await getOperations(
`
model ThingId {
@path
@segment("things")
thingId: string;
}
@autoRoute
interface Things {
@put
op WithFilteredParam(
@path
@segment("subscriptions")
subscriptionId: string,
@path
@segment("providers")
provider: "Microsoft.Things",
...ThingId,
@header contentType: "text/plain",
@body body: "Test Content"
): string;
}
`,
);
expect(operations.length).toBe(1);
const operation = operations[0];
expect(operation.verb).toBe("put");
expect(operation.path).toBe(
"/subscriptions/{subscriptionId}/providers/Microsoft.Things/things/{thingId}",
);
const operationProps = operation.parameters.properties;
expect(operationProps.map((p) => p.kind)).toEqual(["path", "path", "contentType", "body"]);
});

it("emit diagnostic if passing arguments to autoroute decorators", async () => {
const [_, diagnostics] = await compileOperations(`
@autoRoute("/test") op test(): string;
Expand Down

0 comments on commit d733c75

Please sign in to comment.