Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/http",
"comment": "Add `@sharedRoute` decorator for marking operations as sharing a route with other operations",
"type": "none"
}
],
"packageName": "@typespec/http"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/http",
"comment": "Deprecate the `shared` option in the `@route` decorator. `@sharedRoute` is the new way to accomplish the same behavior.",
"type": "none"
}
],
"packageName": "@typespec/http"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/openapi3",
"comment": "",
"type": "none"
}
],
"packageName": "@typespec/openapi3"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/rest",
"comment": "",
"type": "none"
}
],
"packageName": "@typespec/rest"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/rest",
"comment": "Add validation to ensure that @action or @collectionAction operations have a specified name when used with @sharedRoute",
"type": "none"
}
],
"packageName": "@typespec/rest"
}
41 changes: 34 additions & 7 deletions docs/standard-library/http/reference/decorators.md
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ dec TypeSpec.Http.query(target: ModelProperty, queryNameOrOptions?: string | Typ

### `@route` {#@TypeSpec.Http.route}

`@route` defines the relative route URI for the target operation
Defines the relative route URI for the target operation

The first argument should be a URI fragment that may contain one or more path parameter fields.
If the namespace or interface that contains the operation is also marked with a `@route` decorator,
Expand All @@ -246,19 +246,19 @@ it will be used as a prefix to the route URI of the operation.
`@route` can only be applied to operations, namespaces, and interfaces.

```typespec
dec TypeSpec.Http.route(entity: unknown, path: string, parameters?: object)
dec TypeSpec.Http.route(target: Namespace | Interface | Operation, path: string, options?: (anonymous model))
```

#### Target

`(intrinsic) unknown`
`union Namespace | Interface | Operation`

#### Parameters

| Name | Type | Description |
| ---------- | --------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- |
| path | `scalar string` | Relative route path. Cannot include query parameters. |
| parameters | `model object` | Optional set of parameters used to configure the route. Supports `{shared: true}` which indicates that the route may be shared by several operations. |
| Name | Type | Description |
| ------- | ------------------------- | ----------------------------------------------------- |
| path | `scalar string` | Relative route path. Cannot include query parameters. |
| options | `model (anonymous model)` | |

### `@server` {#@TypeSpec.Http.server}

Expand All @@ -280,6 +280,33 @@ dec TypeSpec.Http.server(target: Namespace, url: string, description: string, pa
| description | `scalar string` | Description of the endpoint |
| parameters | `model object` | Optional set of parameters used to interpolate the url. |

### `@sharedRoute` {#@TypeSpec.Http.sharedRoute}

`@sharedRoute` marks the operation as sharing a route path with other operations.

When an operation is marked with `@sharedRoute`, it enables other operations to share the same
route path as long as those operations are also marked with `@sharedRoute`.

`@sharedRoute` can only be applied directly to operations.

```typespec
@sharedRoute
@route("/widgets")
op getWidget(@path id: string): Widget;
```

```typespec
dec TypeSpec.Http.sharedRoute(target: Operation)
```

#### Target

`Operation`

#### Parameters

None

### `@statusCode` {#@TypeSpec.Http.statusCode}

Specify the status code for this response. Property type must be a status code integer or a union of status code integer.
Expand Down
1 change: 1 addition & 0 deletions docs/standard-library/http/reference/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ toc_max_heading_level: 3
- [`@query`](./decorators.md#@TypeSpec.Http.query)
- [`@route`](./decorators.md#@TypeSpec.Http.route)
- [`@server`](./decorators.md#@TypeSpec.Http.server)
- [`@sharedRoute`](./decorators.md#@TypeSpec.Http.sharedRoute)
- [`@statusCode`](./decorators.md#@TypeSpec.Http.statusCode)
- [`@useAuth`](./decorators.md#@TypeSpec.Http.useAuth)

Expand Down
33 changes: 31 additions & 2 deletions packages/http/lib/http-decorators.tsp
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ extern dec useAuth(target: Namespace, auth: object | Union | object[]);
extern dec includeInapplicableMetadataInPayload(target: unknown, value: boolean);

/**
* `@route` defines the relative route URI for the target operation
* Defines the relative route URI for the target operation
*
* The first argument should be a URI fragment that may contain one or more path parameter fields.
* If the namespace or interface that contains the operation is also marked with a `@route` decorator,
Expand All @@ -216,5 +216,34 @@ extern dec includeInapplicableMetadataInPayload(target: unknown, value: boolean)
*
* @param path Relative route path. Cannot include query parameters.
* @param parameters Optional set of parameters used to configure the route. Supports `{shared: true}` which indicates that the route may be shared by several operations.
*
* @example
*
* ```typespec
* @route("/widgets")
* op getWidget(@path id: string): Widget;
* ```
*/
extern dec route(
target: Namespace | Interface | Operation,
path: string,
options?: {
shared?: boolean,
}
);

/**
* `@sharedRoute` marks the operation as sharing a route path with other operations.
*
* When an operation is marked with `@sharedRoute`, it enables other operations to share the same
* route path as long as those operations are also marked with `@sharedRoute`.
*
* `@sharedRoute` can only be applied directly to operations.
*
* ```typespec
* @sharedRoute
* @route("/widgets")
* op getWidget(@path id: string): Widget;
* ```
*/
extern dec route(entity: unknown, path: string, parameters?: object);
extern dec sharedRoute(target: Operation);
60 changes: 36 additions & 24 deletions packages/http/src/decorators.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
import {
createDiagnosticCollector,
DecoratorContext,
Diagnostic,
DiagnosticTarget,
getDoc,
isArrayModelType,
Model,
ModelProperty,
Namespace,
Operation,
Program,
setTypeSpecNamespace,
Tuple,
Type,
typespecTypeToJson,
Union,
createDiagnosticCollector,
getDoc,
isArrayModelType,
reportDeprecated,
setTypeSpecNamespace,
typespecTypeToJson,
validateDecoratorTarget,
validateDecoratorUniqueOnNode,
} from "@typespec/compiler";
import { createDiagnostic, createStateSymbol, reportDiagnostic } from "./lib.js";
import { setRoute } from "./route.js";
import { setRoute, setSharedRoute } from "./route.js";
import {
AuthenticationOption,
HeaderFieldOptions,
Expand Down Expand Up @@ -559,23 +560,6 @@ export function getAuthentication(
return program.stateMap(authenticationKey).get(namespace);
}

function extractSharedValue(context: DecoratorContext, parameters?: Model): boolean {
const sharedType = parameters?.properties.get("shared")?.type;
if (sharedType === undefined) {
return false;
}
switch (sharedType.kind) {
case "Boolean":
return sharedType.value;
default:
reportDiagnostic(context.program, {
code: "shared-boolean",
target: sharedType,
});
return false;
}
}

/**
* `@route` defines the relative route URI for the target operation
*
Expand All @@ -588,12 +572,40 @@ function extractSharedValue(context: DecoratorContext, parameters?: Model): bool
export function $route(context: DecoratorContext, entity: Type, path: string, parameters?: Model) {
validateDecoratorUniqueOnNode(context, entity, $route);

// Handle the deprecated `shared` option
let shared = false;
const sharedValue = parameters?.properties.get("shared")?.type;
if (sharedValue !== undefined) {
reportDeprecated(
context.program,
"The `shared` option is deprecated, use the `@sharedRoute` decorator instead.",
entity
);

// The type checker should have raised a diagnostic if the value isn't boolean
if (sharedValue.kind === "Boolean") {
shared = sharedValue.value;
}
}

setRoute(context, entity, {
path,
shared: extractSharedValue(context, parameters),
shared,
});
}

/**
* `@sharedRoute` marks the operation as sharing a route path with other operations.
*
* When an operation is marked with `@sharedRoute`, it enables other operations to share the same
* route path as long as those operations are also marked with `@sharedRoute`.
*
* `@sharedRoute` can only be applied directly to operations.
*/
export function $sharedRoute(context: DecoratorContext, entity: Operation) {
Comment thread
daviwil marked this conversation as resolved.
setSharedRoute(context.program, entity);
}

const includeInapplicableMetadataInPayloadKey = createStateSymbol(
"includeInapplicableMetadataInPayload"
);
Expand Down
6 changes: 0 additions & 6 deletions packages/http/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,6 @@ const libDefinition = {
default: paramMessage`@useAuth ${"kind"} only accept Auth model, Tuple of auth model or union of auth model.`,
},
},
"shared-boolean": {
severity: "error",
messages: {
default: "shared parameter must be a boolean.",
},
},
"shared-inconsistency": {
severity: "error",
messages: {
Expand Down
25 changes: 20 additions & 5 deletions packages/http/src/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,28 +216,43 @@ export function setRoute(context: DecoratorContext, entity: Type, details: Route
const state = context.program.stateMap(routesKey);

if (state.has(entity) && entity.kind === "Namespace") {
const existingValue: RoutePath = state.get(entity);
if (existingValue.path !== details.path) {
const existingPath: string | undefined = state.get(entity);
if (existingPath !== details.path) {
reportDiagnostic(context.program, {
code: "duplicate-route-decorator",
messageId: "namespace",
target: entity,
});
}
} else {
state.set(entity, details);
state.set(entity, details.path);
if (entity.kind === "Operation" && details.shared) {
setSharedRoute(context.program, entity as Operation);
}
Comment on lines +229 to +231

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should deprecate this pattern if we are going with an @sharedRoute decorator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also like to deprecate it if we proceed with @sharedRoute, but I'm not sure how to deprecate a parameter to a decorator at this moment. A simple diagnostic could work, I suppose.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That, and you should be able to mark it with @deprecated in the docstring, I think. @timotheeguerin would know for sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That, and you should be able to mark it with @deprecated in the docstring, I think. @timotheeguerin would know for sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just call reportDeprecated but with a condition on the parameter being used instead of just every time for the decorator

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also if you add the type for the options

alias RouteOptions = {
  @deprecated
  shared?: boolean;
}

I don't think it will emit a diagnostic automatically here but we could add that support

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible that just calling reportDeprecated directly is better for now. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better, we should add a migration script to do this for people

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added deprecation for the old shared option pattern.

}
}

const sharedRoutesKey = createStateSymbol("sharedRoutes");

export function setSharedRoute(program: Program, operation: Operation) {
program.stateMap(sharedRoutesKey).set(operation, true);
}

export function isSharedRoute(program: Program, operation: Operation): boolean {
return program.stateMap(routesKey).get(operation)?.shared;
return program.stateMap(sharedRoutesKey).get(operation) === true;
}

export function getRoutePath(
program: Program,
entity: Namespace | Interface | Operation
): RoutePath | undefined {
return program.stateMap(routesKey).get(entity);
const path = program.stateMap(routesKey).get(entity);
return path
? {
path,
shared: entity.kind === "Operation" && isSharedRoute(program, entity as Operation),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original logic simply plumbed details through without regard to whether it was an operation, interface, etc. so this change would introduce a behavioral change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original design, applying @route on a namespace or interface does not propagate the sharedness status down to the contained operations, so it never worked in practice. The new decorator just makes this explicit in that @sharedRoute can't be applied anywhere but to an operation.

}
: undefined;
}

const routeOptionsKey = createStateSymbol("routeOptions");
Expand Down
25 changes: 19 additions & 6 deletions packages/http/test/http-decorators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,23 @@ describe("http: decorators", () => {
]);
});

it("emit diagnostics when not all duplicated routes are declared shared", async () => {
it("emits diagnostic when deprecated `shared` option is used", async () => {
const diagnostics = await runner.diagnose(`
@route("/test", { shared: true }) op test(): string;
@route("/test", { shared: true }) op test2(): string;
`);
expectDiagnostics(diagnostics, [
{
code: "deprecated",
message:
"Deprecated: The `shared` option is deprecated, use the `@sharedRoute` decorator instead.",
},
]);
});

it("emit diagnostics when not all duplicated routes are declared shared", async () => {
const diagnostics = await runner.diagnose(`
@route("/test") @sharedRoute op test(): string;
@route("/test") @sharedRoute op test2(): string;
@route("/test") op test3(): string;
`);
expectDiagnostics(diagnostics, [
Expand All @@ -261,8 +274,8 @@ describe("http: decorators", () => {

it("do not emit diagnostics when duplicated shared routes are applied", async () => {
const diagnostics = await runner.diagnose(`
@route("/test", {shared: true}) op test(): string;
@route("/test", {shared: true}) op test2(): string;
@route("/test") @sharedRoute op test(): string;
@route("/test") @sharedRoute op test2(): string;
`);

expectDiagnosticEmpty(diagnostics);
Expand All @@ -274,8 +287,8 @@ describe("http: decorators", () => {
`);
expectDiagnostics(diagnostics, [
{
code: "@typespec/http/shared-boolean",
message: `shared parameter must be a boolean.`,
code: "invalid-argument",
message: `Argument '(anonymous model)' is not assignable to parameter of type '(anonymous model)'`,
Comment thread
tjprescott marked this conversation as resolved.
},
]);
});
Expand Down
Loading