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] design for Java emitter to influence the default access property of SdkInitializationType and SdkClientAccessor #1702

Open
weidongxu-microsoft opened this issue Oct 17, 2024 · 3 comments
Assignees
Labels
lib:tcgc Issues for @azure-tools/typespec-client-generator-core library

Comments

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Oct 17, 2024

Upon #1696

Context

In TCGC, I take

  • SdkInitializationType.access means whether the client/subclient is directly initializable (from builder or client class ctor).
  • SdkClientAccessor.access means whether the subclient can be accessed by this client access method (from parent client of this subclient).
  • SdkInitializationType.model.access means whether the model of the SdkInitializationType is public or not.

In Java, there is currently 2 pattern when accessing a subclient.

  1. "client builder pattern" as
// client
Client client = new ClientBuilder().buildClient();

// subclient via builder (equivalent to client initialization in other languages)
SubClient subclient = new ClientBuilder()
  .parameter(<param>)
  .buildSubClient();

I assume it maps to TCGC

// subclient initialization type
subclient.initialization.access=public;

// client access method from (parent) client
client.<clientaccessor_method>.access=internal;
  1. "client accessor pattern" as
// client
Client client = new ClientBuilder().buildClient();

// subclient via client accessor
SubClient subclient = client.getSubClient(<param>);

I assume it maps to TCGC

// subclient initialization type
subclient.initialization.access=internal;

// client access method from (parent) client
client.<clientaccessor_method>.access=public;

At present, DPG and unbranded by default uses "client builder pattern". And we also need certain configure to switch to "client accessor pattern" for some modules (e.g. Azure Face API, unbranded OpenAI)

And currently the TCGC value of SdkInitializationType.access and SdkClientAccessor.access is not what Java expected. Therefore, Java emitter does not use the TCGC value for now.
It would infer the client/subclient from whether the client has parent client.

PS: Java emitter would still treat the SdkInitializationType.model.access as "internal" or "none", as Java uses its properties directly and not the model (meaning there is currently no ##ClientOptions class for Java).

With "access" in @clientInitialization

Afterward, Java emitter must always use the TCGC value, as it cannot tell whether service has overridden the default @clientInitialization's "access" value.

Problem to solve

Java emitter would still want to affect TCGC to provide the expect default value (when service did not set the "access"). And this default is different from other languages.

The intention is to avoid existing DPG from TypeSpec always adding mulitple lines of @clientInitialization(<interface>, access=public, scope="java"), to make subclient.initialization.access=public for Java.

Possible solutions

Java emitter may provide a e.g. defaultClientInitializationAccess as TCGC context.
Default internal for all languages, so that without client.tsp override, subclient is not directly initializable (without its parent client).
And Java may provide public to TCGC context, for the required situations. <-- I assume it also switch the SdkClientAccessor.access to internal on its parent client accessor method

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

i think currently, client accessor method is always set with internal cause we do not support public client initialization before @clientInitialization. i agree with the access value you mentioned in two situations, and it should be fixed in tcgc. also, we may need a way to set client accessor's accessibility. cc: @iscai-msft

@iscai-msft
Copy link
Contributor

  1. Client-Builder-Pattern: Yes, this is a correct mapping to me
  2. Client-Accessor-Pattern: Currently, we don't handle the client accessor pattern, our typing system is just designed so it will eventually handle it. The thinking is if you want the client accessor pattern, you need to explicitly define it in your client.tsp. I think this issue was lost in our repo reordering, so I recreated it here. In this case, the subclient's initialization.access can be either internal or public. For example, in storage, the subclient ContainerClient would have initilization.access === "public" and <client-accessor-method>.access === "public", since you can either use a client accessor to create it, or you can directly create it yourself.
  3. SdkInitializationType.model.access: I'm adding a ClientInitialization usage to usage flags, so languages like Java and Python can filter out the generation of it

I'm not fully understanding how TCGC isn't setting the correct access: are you talking about in the client-builder-pattern, or the client-accessor-pattern? We don't have full support for client-accessor-pattern so that is definitely true that we should add support for it. My question is: is Java not getting the correct values for client-builder-pattern? Because that would be a bug since we do support the client-builder-pattern.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Oct 22, 2024

What I mean is that today (I am aware that TCGC didn't give full information at this time, so emitter of different language kind of handling this client-subclient in their own best practice), .NET and Python is actually generate the code according to the Client-Accessor-Pattern.

E.g. .NET Face API
FaceAdministrationClient.GetLargeFaceListClient()
https://github.com/Azure/azure-sdk-for-net/blob/d68cd74453a6eec91135f72fefaf2bc851be4238/sdk/face/Azure.AI.Vision.Face/src/Generated/FaceAdministrationClient.cs#L101-L110
meaning <client-accessor-method>.access === "public"

Python Face API
FaceAdministrationClient.large_face_list
https://github.com/Azure/azure-sdk-for-python/blob/fc3fb85f5ce4a8cf7b915e626a52c232b350f69a/sdk/face/azure-ai-vision-face/generated_tests/test_face_administration_large_face_list_operations.py#L19C44-L19C45
I take this .large_face_list in python also means <client-accessor-method>.access === "public" (though it may not be a method)?

All of above is done without service ever add op getLargeFaceList() in client.tsp for client accessor support.
Emitter just generate code according to their best practice for the client-subclient.

It is only Java generating code as Client-Builder-Pattern (we can say this be same as Client-Constructor-Pattern for Python/.NET, that the subclient can be initialized directly without the need to first initialize its parent client).


This is before we allow service to explicitly set the access on @clientInitialization.

After support of optional access in @clientInitialization, service would be able to explicitly set the access to some client, and language emitter can no longer ignore TCGC and do their own default behavior (as emitter won't know in which case that default get overridden by this access).

Hence now we need TCGC always gives correct information.
And the problem is that this information for default (when service didn't add access) would be almost opossite in Java and Python/.NET.

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

No branches or pull requests

3 participants