Add `@sharedRoute` decorator and decouple it from `@route` by daviwil · Pull Request #1798 · microsoft/typespec

daviwil · 2023-04-06T12:46:16Z

This PR decouples the concept of a shared route from the @route decorator by adding new @sharedRoute decorator, setSharedRoute and getSharedRoute accessors. I'm also storing the state of shared routes separately so that @autoRoute operations can also have the shared route status set independently.

Note that the old {shared: true} pattern still continues to work and we still return aggregate RoutePath information from getRoutePath so that no breaking change is introduced.

Fixes #1747.

Tasks

Deprecate getAction and getCollectionAction
Ensure that autoRoute shared route support doesn't break openapi shared route logic
Update autorest emitter samples

github-actions · 2023-04-06T12:46:29Z

Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/1798/

Website: https://cadlwebsite.z1.web.core.windows.net/prs/1798/

tjprescott

We need documentation for this, but I also am not clear why we are adding this decorator. Presumably you can set shared state in autoroute using the {shared: bool} object? What would this look like in the context of autoroute? If we are adding this, why are we not deprecating the old way?

I would prefer to defer merging this until next sprint.

daviwil · 2023-04-06T15:11:47Z

This is a P0 for client generation for this sprint according to @johanste because we need a better option than the current overloads design.

We decided to break this out as a separate decorator because an Azure.Core user needs to be able to add @sharedRoute to a standard resource operation that already has @autoRoute on it.

Here's an example adapted from a sample that Johan wants to get working:

import "@azure-tools/typespec-azure-core";

alias ContentNegotiate<TMimeType extends string> = 
  Azure.Core.Traits.RequestHeadersTrait< { @TypeSpec.Http.header("Accept") accept: TMimeType; }> & 
  Azure.Core.Traits.ResponseHeadersTrait<{ @TypeSpec.Http.header("Content-Type") contentType: TMimeType;}
>;

@TypeSpec.Rest.resource("things")
model Thing {
  @key
  @visibility("read")
  thingId: string;
}

@sharedRoute
op readThingAsString is Azure.Core.ResourceRead<Thing, ContentNegotiate<"application/xml">>;

@sharedRoute
op readThingAsXml is Azure.Core.ResourceRead<Thing, ContentNegotiate<"application/json">>;

ResourceRead already has @autoRoute applied. I think it makes sense to use a separate decorator so that we don't need to worry about the semantics of re-applying @autoRoute with a different options object for enabling the shared routes.

tjprescott · 2023-04-10T15:42:06Z

+ * If the namespace or interface that contains the operation is also marked with a `@route` decorator,
+ * 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
+ * @route("/widgets")
+ * op getWidget(@path id: string): Widget;
+ * ```
+ */
+extern dec route(target: Namespace | Interface | Operation, path: string, options?: object);


If we are preserving the options object, we should include that in the documentation. The docstring should also include @param so that it appears in the generated docs.

also shouldn't the options be a model expression alias instead of just object?

Probably, I'm not that good at writing decorator signatures yet. Do you mean this:

extern dec route(target: Namespace | Interface | Operation, path: string, options?: { shared?: boolean});

yep, or use an alias alias RouteOptions = {...} for the model expression

the alias might be better for when we have the valueof so we can do: options?: valueof(RouteOptions)

I added the signature for this but the error reporting on an invalid check isn't so clear. Filed issue #1832 with an example.

tjprescott · 2023-04-10T15:43:47Z

+      @autoRoute
+      @sharedRoute
+      op get1(@path name: string): string;
+
+      @test
+      @autoRoute
+      op get2(@path name: string): string;
+    `)) as { get1: Operation; get2: Operation };


Is it common to actually put @autoRoute on individual operations? Why could we not just have @autoRoute take the same options object as @route? Or just have both make use of this decorator?

Yes, this is the pattern used in Azure.Core. The @autoRoute decorator is placed on ResourceOperation<TResource> which causes it to be applied individually to every operation which uses standard operation signatures via is.

In my earlier comment, I explained that we shouldn't conflate sharedness of routes with @route or @autoRoute because changing an operation signature to go from non-shared to shared means re-applying the original decorator instead of just adding @sharedRoute to the operation, no matter what kind of route it uses. This is the key element of how this PR is intended to affect the design.

tjprescott · 2023-04-10T15:48:06Z

+    if (entity.kind === "Operation" && details.shared) {
+      setSharedRoute(context.program, entity as Operation);
+    }


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

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.

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

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

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

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

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

Added deprecation for the old shared option pattern.

tjprescott · 2023-04-10T15:50:58Z

+  return path
+    ? {
+        path,
+        shared: entity.kind === "Operation" && isSharedRoute(program, entity as Operation),


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.

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.

tjprescott

My overall thoughts are that we should either:

Deprecate the {shared: true} pattern on @route and make use of @sharedRoute the standard or:
We should not introduce a new decorator and just make @autoRoute accept an options object like @route does.

Here is the design issue where we said that the options object was the way to go. #984.

I don't know why this did not also go through design. It seems like we are treating it as a bug fix when it's really not and the current implementation would leave us with multiple ways of doing the same thing, which we try to avoid.

tjprescott · 2023-04-10T16:17:39Z

    ]);
  });
+
+  describe("shared routes", () => {


@markcowl also left a comment on the original post to make sure that this works for actions, so we should probably add a test that covers that as well.

I will add a test for that and some extra validation logic.

Added the validation logic and test to verify.

daviwil · 2023-04-10T17:34:17Z

I don't know why this did not also go through design. It seems like we are treating it as a bug fix when it's really not and the current implementation would leave us with multiple ways of doing the same thing, which we try to avoid.

I brought it up in Thursday's design meeting and there wasn't any objection to it at that point. I asked folks to come comment on the PR if they had other feedback but only Mark has left any so far.

github-actions · 2023-04-18T14:34:57Z

Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/1798/

Website: https://cadlwebsite.z1.web.core.windows.net/prs/1798/

daviwil · 2023-04-18T14:36:41Z

This PR is ready for another look, I've addressed all PR feedback and added tests where needed.

tjprescott

Looks good but can we also update the shared routes tests in OpenAPI3 (and Autorest on the Azure side) to use @sharedRoute instead of {shared: true}?

daviwil · 2023-04-18T17:10:55Z

I've updated the openapi3 tests already, but I'll go check on the autorest emitter tests too.

daviwil · 2023-04-19T11:36:00Z

Sent out the typespec-azure PR to uptake changes here: Azure/typespec-azure#2863

Add @sharedRoute decorator and decouple it from @route

0b9c7b8

daviwil requested review from allenjzhang, bterlson, markcowl, nguerrera and timotheeguerin as code owners April 6, 2023 12:46

daviwil requested review from johanste and tjprescott April 6, 2023 12:52

tjprescott requested changes Apr 6, 2023

View reviewed changes

markcowl requested changes Apr 6, 2023

View reviewed changes

Comment thread packages/http/src/decorators.ts

Comment thread packages/http/test/routes.test.ts

markcowl and others added 6 commits April 6, 2023 09:53

Merge branch 'main' into split-shared-route

83f4677

Merge remote-tracking branch 'upstream/main' into split-shared-route

7ab8010

Add decorator definitions for @route and @SharedRoute

ebc926c

Add autoRoute shared route tests

aa639c6

Merge branch 'main' into split-shared-route

69c784d

Update generated documentation

d2564b3

markcowl approved these changes Apr 7, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into split-shared-route

08da601

tjprescott reviewed Apr 10, 2023

View reviewed changes

Comment thread docs/standard-library/http/reference/decorators.md Outdated

tjprescott reviewed Apr 10, 2023

View reviewed changes

tjprescott requested changes Apr 10, 2023

View reviewed changes

tjprescott reviewed Apr 10, 2023

View reviewed changes

daviwil added 2 commits April 18, 2023 09:47

Merge remote-tracking branch 'upstream/main' into split-shared-route

a9b23f3

Deprecate shared option for @route parameter

311fabb

daviwil added 6 commits April 18, 2023 09:55

Format source

8cf9bd2

Add options signature for route decorator

edcc87d

Ensure @action operations have explicit name when using @SharedRoute

665dcf9

Update Rush change files

d2e7e25

Update documentation

279feea

Update openapi3 shared route tests to use @SharedRoute

60d7fbb

daviwil closed this Apr 18, 2023

daviwil reopened this Apr 18, 2023

daviwil force-pushed the split-shared-route branch from 41589bd to 60d7fbb Compare April 18, 2023 14:44

tjprescott requested changes Apr 18, 2023

View reviewed changes

Comment thread packages/http/test/http-decorators.test.ts

Comment thread packages/rest/test/routes.test.ts Outdated

Comment thread packages/rest/test/routes.test.ts

Comment thread packages/rest/src/rest.ts

daviwil added 3 commits April 19, 2023 14:15

PR feedback

1c35071

Merge remote-tracking branch 'upstream/main' into split-shared-route

994e5d2

Format

54a0d04

tjprescott approved these changes Apr 19, 2023

View reviewed changes

Merge branch 'main' into split-shared-route

7a29f78

daviwil merged commit df55792 into microsoft:main Apr 19, 2023

daviwil deleted the split-shared-route branch April 19, 2023 17:10

Uh oh!

Conversation

daviwil commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 6, 2023

Uh oh!

tjprescott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheeguerin Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Apr 10, 2023

Uh oh!

github-actions Bot commented Apr 18, 2023

Uh oh!

daviwil commented Apr 18, 2023

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daviwil commented Apr 6, 2023 •

edited

Loading

tjprescott left a comment •

edited

Loading

daviwil commented Apr 6, 2023 •

edited

Loading

timotheeguerin Apr 10, 2023 •

edited

Loading

tjprescott left a comment •

edited

Loading