-
Notifications
You must be signed in to change notification settings - Fork 68
Extend multiple-services design doc with nested @client pattern for client hierarchy customization #3847
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
base: main
Are you sure you want to change the base?
Conversation
… customization Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
autoMerge option for client hierarchy customization
tadelesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Refine with my comment. I don't want to add the new autoMerge flag. You need to check the current two design doc and current cache.ts implementation code to see this new design could meet all current cases and the new scenarios.
packages/typespec-client-generator-core/design-docs/multiple-services.md
Outdated
Show resolved
Hide resolved
packages/typespec-client-generator-core/design-docs/multiple-services.md
Outdated
Show resolved
Hide resolved
packages/typespec-client-generator-core/design-docs/multiple-services.md
Outdated
Show resolved
Hide resolved
…stead Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
Refactored the design to remove the
Also fixed |
Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
tadelesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Resolve the remaining comments.
packages/typespec-client-generator-core/design-docs/multiple-services.md
Show resolved
Hide resolved
packages/typespec-client-generator-core/design-docs/multiple-services.md
Show resolved
Hide resolved
packages/typespec-client-generator-core/design-docs/multiple-services.md
Outdated
Show resolved
Hide resolved
packages/typespec-client-generator-core/design-docs/multiple-services.md
Outdated
Show resolved
Hide resolved
…tion to all YAML, remove comparison code Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
Resolved all remaining comments in commit 83e9199:
|
autoMerge option for client hierarchy customization
tadelesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I also want to make a change for client with multiple services: to make the apiVersions array could be 2-dimension to store all possible API versions for multiple services.
packages/typespec-client-generator-core/design-docs/multiple-services.md
Show resolved
Hide resolved
Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
Updated |
commit: |
|
@copilot Format the doc. |
Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
| kind: client | ||
| name: CombineClient | ||
| apiVersions: [] | ||
| apiVersions: [[av1, av2], [bv1, bv2]] # 2-dimensional array for multiple services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be beneficial to have a more explicit type (or a dict of namespace -> [<versions>]is also better).
A 2-dimensional array is not easy to understand (at least, to me...) -- which direction is for which catalog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Change the apiVersions to be a map, the key is a service namespace's full qualified name, the value if the api versions array. Let's make it to be a non-breaking change, add a new property apiVersionMap, keep previous apiVersions property as a 1-dimensinal array and keep it empty for cross service client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use apiVersionsMap (a map of service namespace full qualified name to API versions array) instead of a 2D array. The apiVersions property is kept as an empty array for cross-service clients to maintain backward compatibility. See commit c0517f2.
|
|
||
| ### Scenario 2: Services as Direct Children (No Deep Auto-Merge) | ||
|
|
||
| In the first step design, when combining multiple services with an empty client namespace, all nested namespaces/interfaces from all services are auto-merged into the root client as children. Some users prefer to keep each service's namespace as a direct child of the root client without deep merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I feel it more natural to behave according to the standard Namespace arrangement. If there is 2 root Namespace, it would be 2 root client.
Or, if we don't want this behavior, we can let TCGC error out, if no explicit client is specified on multiple-service case.
Automatically "merge" the 2 independent Namespace into a single root client, seems not match the single service behavior.
(I think if 2 root Namespace belong to a single service, TCGC will produce 2 root client, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer if there are two root @service namespace, then TCGC auto make them to be two root clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Change previous design in client.tsp of just detect the first @service to be one client. If there are multiple services defined, each @service should be a separate client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the design so that multiple @service namespaces without explicit @client will automatically create separate root clients (one per service). Also updated client.md to clarify this behavior. See commit 448799a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot You need also to add another scenario into multiple-services.md to show this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added "Scenario 0: Multiple Services Without Explicit @client (Default Behavior)" with full TCGC behavior details, YAML output, and Python SDK example. See commit 59382d3.
| When TCGC detects multiple services in one client, it will: | ||
|
|
||
| 1. Create the root client for the combined client. If any service is versioned, the root client's initialization method will have an `apiVersion` parameter with no default value. The `apiVersions` property and the `apiVersion` parameter for the root client will be empty (since multiple services' API versions cannot be combined). The root client's endpoint and credential parameters will be created based on the first sub-service, which means all sub-services must share the same endpoint and credential. | ||
| 1. Create the root client for the combined client. If any service is versioned, the root client's initialization method will have an `apiVersion` parameter with no default value. The `apiVersions` property for the root client will be a 2-dimensional array to store all possible API versions for each service (e.g., `[[av1, av2], [bv1, bv2]]`). The root client's endpoint and credential parameters will be created based on the first sub-service, which means all sub-services must share the same endpoint and credential. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if different services use different endpoint and credential, will TCGC report error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, TCGC will report a diagnostic error if services have different @server or @useAuth definitions. This is documented in the TCGC Behavior section and the "Endpoint and Credential Limitations" sections for Scenarios 2 and 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There will be diagnostics. TCGC does not support mix operations with different endpoint/credential into one client. This is mentioned in the following scenarios.
| The key design principle is: | ||
|
|
||
| - **If the client namespace is empty**: TCGC auto-merges all services' nested namespaces/interfaces into the current client as children (first step design behavior). | ||
| - **If the client namespace contains nested `@client` decorators**: TCGC uses the explicitly defined client hierarchy instead of auto-merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this will not happen in ARM, but do we support the mix, e.g. some sub clients could be merged while others has explicitly client hierachy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current design doesn't support a mix of merged and explicitly defined sub-clients at the same level. When the namespace has any nested @client decorators, TCGC uses only the explicitly defined hierarchy. To achieve partial merging, you would need to explicitly define all child clients you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a recursive behavior. For each @client namespace, TCGC will decide the merge behavior based on if there is any nested @client. So, you could mix all kinds of clients as you wish.
|
|
||
| @client({ | ||
| name: "CombineClient", | ||
| service: [ServiceA, ServiceB], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we define service: [ServiceA, ServiceB, ServiceC],
but in namespace CombineClient, there is only serviceA and serviceB, will serviceC be ignored in client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a service is listed in service: [ServiceA, ServiceB, ServiceC] but not used in any nested @client decorator, its operations would be omitted from the SDK. The design doc mentions in Validation Rules: "Operations not referenced by any explicit client are omitted from the SDK."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If there are explicit nested @client then the service here is not so important.
…ients Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
…nts by default Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
…and Python examples Co-authored-by: tadelesh <1726438+tadelesh@users.noreply.github.com>
Extends the
multiple-services.mddesign document to support advanced client hierarchy customization using nested@clientdecorators.Key Design Changes
Default Behavior for Multiple Services
@client: Each@servicenamespace becomes its own root client automatically (consistent with single-service behavior)@client: Users can customize client names or merge multiple services into one clientKey Design Principle (for explicit
@client)@clientnamespace is empty: TCGC auto-merges all services' nested namespaces/interfaces into the current client as children@clientnamespace contains nested@clientdecorators: TCGC uses the explicitly defined client hierarchy instead of auto-mergingScenarios
Scenario 0: Multiple Services Without Explicit
@client(Default Behavior)Multiple
@servicenamespaces automatically become multiple root clients (e.g.,ServiceAClientandServiceBClient) - NEW: now includes full YAML output and Python SDK exampleScenario 1: Explicit Client Names for Multiple Services
Use
@clientto customize client names (e.g.,MyServiceAClientinstead ofServiceAClient)Scenario 2: Services as Direct Children (No Deep Auto-Merge)
Use nested
@clientdecorators to explicitly define each service as a child client, preventing deep auto-mergingScenario 3: Fully Customized Client Hierarchy
Use nested
@clientdecorators with explicit operation mapping viaiskeywordDocumentation Updates
client.mdto clarify that each@servicenamespace becomes a separate root clientchildrenproperty (matchingSdkClientTypeinterface)clientInitializationwithinitializedByproperty to all YAML examples followingclient.mddefaultsapiVersionsMapfor multi-service clients (non-breaking change)Design doc only—no code changes included.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.