Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCGC: @methodLocation decorator #914

Closed
l0lawrence opened this issue May 23, 2024 · 11 comments · Fixed by #1398
Closed

TCGC: @methodLocation decorator #914

l0lawrence opened this issue May 23, 2024 · 11 comments · Fixed by #1398
Assignees
Labels
feature New feature or request lib:tcgc Issues for @azure-tools/typespec-client-generator-core library

Comments

@l0lawrence
Copy link
Member

Is it possible to move parameters that persist on each operation to the client instead with a decorator of some sort? In EventGrid this shows up in the case of

client()
publish(topic_name, events)
receive(topic_name, subscription_name, max_events)
ack(topic_name, subscription_name, lock_tokens)

to

client(topic_name, subscription_name)
publish(events)
receive(max_events)
ack(lock_tokens)

typespec spec: https://github.com/Azure/azure-rest-api-specs/blob/9df71d5a717e4ed5e6728e7e6ba2fead60f62243/specification/eventgrid/Azure.Messaging.EventGrid/main.tsp#L261

@iscai-msft
Copy link
Contributor

I see the need for this decorator, what about something like @parameterScope. Going to post this in the dpg sync chat and have people respond

@tadelesh tadelesh added the lib:tcgc Issues for @azure-tools/typespec-client-generator-core library label May 29, 2024
@tadelesh
Copy link
Member

similar with #493?

@m-nash
Copy link
Member

m-nash commented May 29, 2024

main.tsp

interface foo {
op myOp(x: int, y: int) : void
}

client.tsp

@clientName("FooClientOptions", "csharp")
model myClientInitModel {
x: int;
}

@@client(foo, { additionalInitialization: myClientInitModel })

//typegraph from tcgc

SdkOperation {
parameters: [ {x: int, location: client}, { y: int, location: operation } ]
}

SdkMethod {
parameters: [ { y: int, location: operation } ]
}

@lmazuel
Copy link
Member

lmazuel commented Jun 10, 2024

similar with #493?

Yes, I agree it's a duplicate, I'm closing #493

@iscai-msft
Copy link
Contributor

iscai-msft commented Aug 21, 2024

To determine whether to map a method parameter onto the client parameter, we will

  1. By default, check the name and the type of the parameter. If the name and the type match up, we will elevate that param up to the client
  2. Add decorator @paramMap, where you can explicitly map a parameter from being on the method to the client, if the default name-matching doesn't elevate your parameter
// main.tsp
namespace My.Service;

op upload(blobName: string): void;
op download(blobName: string): void;
op theName(bName: string): void;

// client.tsp
import "@azure-tools/typespec-client-generator-core";
using Azure.ClientGenerator.Core;

namespace My.Customizations;

model ClientInitOptions {
  blobClientName: string;
}
@@clientInitialization(MyService, ClientInitOptions)
@@paramMap(download.blobName, ClientInitOptions.blobClientName);
@@paramMap(theName.bName, ClientInitOptions.blobClientName)

@msyyc
Copy link
Member

msyyc commented Aug 26, 2024

How about the following scenario?

// main.tsp
namespace My.Service;

op upload(blobName: string): void;
op download(blobName: string): void;

// client.tsp
import "@azure-tools/typespec-client-generator-core";
using Azure.ClientGenerator.Core;

namespace My.Customizations;

model ClientInitOptions {
  blobClientName: string;
}
@@clientInitialization(MyService, ClientInitOptions)
  • Users have to set specific blobName when initialize client and create a new client if they want to access other resources with different blobName.

In view of up cons, I think it is better to adopt @clientInitialization to additional convenient instead of original client. For example:

  • Still generate API as showed in main.tsp
  • For most common used API, define convenient client which could help users reduce number of parameters to pass, like:
// for users which want to access resources across different blobName
client = ServiceClient(...)
client.upload(blob_name="1", ...)
client.upload(blob_name="2", ...)

// for users who only want to access specific blob
convenientClient = ServiceConvenientClient(blob_name="1")
convenientClient.upload()

I think up API could meet all kinds of scenario usage. For convenient client, it is easy to implement with base client like:

class ServiceClient():
   ...

class ServiceConvenientClient:
   def __init__(blob_name, ...):
       self._client = ServiceClient(...)
       self.blob_name = blob_name

  def upload(...)
       return self._client.upload(blob_name=self.blob_name, ...)

TCGC may need additional design for language emitters to map wrapped API with original generated API.
nit: the inspiration is from azure-storage-blob which has similar convenient client for SDK users.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Aug 26, 2024

General design in Java is to avoid having 2 (functionally same) API in different client.

e.g. upload only exists in BlobClient (which is initialized with a blobName).

It does not appear in its parent e.g. BlobContainerClient.

PS: list (in Java it is listBlobs only exists in BlobContainerClient)

@tadelesh
Copy link
Member

i also not prefer to have two same api in different client. i think only when all operations under a sub-client all have the same common parameter (kind of resource and sub resource) that we could make this parameter to be client level, and we should add lint for the new decorator to avoid other cases.

@qiaozha
Copy link
Member

qiaozha commented Aug 26, 2024

I feel like I don't want to provide two methods with the same functionality in Modular either.

In JS RLC, we will not take client.tsp method location customization, but we have a proposal for path parameter specifically in a more general way, because it's not very easy or have a good way to support this in a regular RLC experience Azure/autorest.typescript#2148 (comment), though it was just an initial thought and we haven't figured out any implementation details yet.

// for path "/path1/{path1Param}/path2/{path2Param}", we have the following two ways to call the apis.

client.path("/path1/{path1Param}", path1ParamValue)
        .path("/path2/{path2Param}", path2ParamValue)
 
client.path("/path1/{path1Param}/path2/{path2Param}", path1ParamValue, path2ParamValue).get();

// If there's another path "/path1/{path1Param}/path3/{path3Param}"

client.path("/path1/{path1Param}", path1ParamValue)
        .path("/path3/{path3Param}", path3ParamValue)
 
client.path("/path1/{path1Param}/path3/{path3Param}", path1ParamValue, path3ParamValue).get();

In this way, if customer wants to save their effort of input path1Param in each method, they could take this approach.

const subClient = client.path("/path1/{path1Param}", path1ParamValue);

subClient.path("/path2/{path2Param}", path2ParamValue)
subClient.path("/path3/{path3Param}", path3ParamValue)

Another point of this approach is, mgmt plane path is very long path, which could benefit from this new user experience.

/cc @joheredi @xirzec

@iscai-msft
Copy link
Contributor

@msyyc I think the decision to have the @clientInitialization is more to do with archboard than something we should deal with in tcgc. If archboard and everyone is already good with the downsides of having client params and what that means for individual method calls, we should honor that and generate what we're being asked to generate. With @clientInitialization we would just generate the kind of client that the spec author has explicitly written, so I think here we have to trust them that it's the right move. If not, it becomes "garbage in garbage out", but at that point, it's not our fault because they explicitly asked for that.

@chunyu3
Copy link
Member

chunyu3 commented Aug 28, 2024

i also not prefer to have two same api in different client. i think only when all operations under a sub-client all have the same common parameter (kind of resource and sub resource) that we could make this parameter to be client level, and we should add lint for the new decorator to avoid other cases.

.NET in general will not two clients with the same api, too. But I think it is not required that all the operations in the client have the parameter then this parameter can be bumped to client level. Whether bump a parameter to client level is decided by archboard and service team according to the SDK usage/API definition. It may be good that tcgc don't add a constraint to stop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants