Skip to content

Commit c9fe3a8

Browse files
Refactor ClientOptionsProvider to support singleton pattern for multiple clients (#8864)
- [x] Understand the current ClientOptionsProvider implementation - [x] Add logic to determine if singleton ClientOptions should be used (multiple root clients + only standard parameters) - [x] Make ClientOptionsProvider constructor internal (for testing) - [x] Create CreateClientOptionsProvider factory method - [x] Update ClientProvider to use the factory method - [x] Update ScmOutputLibrary to use HashSet for ClientOptions - [x] Add tests to verify the new behavior - [x] Build and test the changes (899 tests passing) - [x] Verify implementation against existing test projects - [x] Simplify BuildName and BuildDescription by checking `this == _singletonInstance` instead of recalculating conditions - [x] Remove ResetSingleton API and use reflection in tests instead - [x] Rename clients variable to types in ScmOutputLibrary for clarity - [x] Simplify BuildClient by using HashSet<TypeProvider> to automatically deduplicate client options <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Refactor ClientOptionsProvider behavior</issue_title> > <issue_description>We need to support libraries having multiple clients and a single client options. The criteria for using this singleton clientOptionsProvider should be if the following > - there is more than one top level client InputClient > - a client has no Client parameters other than ApiVersion and Endpoint parameters. > The name of the singleton client options should be based off of the PrimaryNamespace. Specifically, it should use the last segment of the namespace and be suffixed with "ClientOptions". > If the client has parameters or there is only a single client, the name and namespace should follow the client, i.e. FooClientOptions. > > The singleton can be a private static on ClientOptionsProvider. > In order to ensure correct usage, we should make ClientOptionsProvider's constructor private and introduce a factory method CreateClientOptionsProvider which will either create a new ClientOptionsProvider instance (if there is only one client or there are client parameters, or delegate to the static singleton). When iterating through the client types in ScmOutputLibrary we will need to ensure that we use a HashSet as multiple clients can use the same instance now.</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> - Fixes #8863 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: JoshLove-msft <[email protected]>
1 parent 6727e90 commit c9fe3a8

File tree

4 files changed

+218
-13
lines changed

4 files changed

+218
-13
lines changed

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientOptionsProvider.cs

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@ public class ClientOptionsProvider : TypeProvider
2424
private readonly TypeProvider? _serviceVersionEnum;
2525
private readonly PropertyProvider? _versionProperty;
2626
private FieldProvider? _latestVersionField;
27+
private static ClientOptionsProvider? _singletonInstance;
2728

28-
public ClientOptionsProvider(InputClient inputClient, ClientProvider clientProvider)
29+
// Internal constructor for testing purposes
30+
internal ClientOptionsProvider(InputClient inputClient, ClientProvider clientProvider)
2931
{
3032
_inputClient = inputClient;
3133
_clientProvider = clientProvider;
@@ -46,6 +48,65 @@ public ClientOptionsProvider(InputClient inputClient, ClientProvider clientProvi
4648
}
4749
}
4850

51+
/// <summary>
52+
/// Factory method to create a ClientOptionsProvider instance.
53+
/// Returns a singleton instance when there are multiple root clients and the client has no custom parameters.
54+
/// Otherwise, creates a new instance specific to the client.
55+
/// </summary>
56+
/// <param name="inputClient">The input client.</param>
57+
/// <param name="clientProvider">The client provider.</param>
58+
/// <returns>A ClientOptionsProvider instance.</returns>
59+
public static ClientOptionsProvider CreateClientOptionsProvider(InputClient inputClient, ClientProvider clientProvider)
60+
{
61+
var rootClients = ScmCodeModelGenerator.Instance.InputLibrary.InputNamespace.RootClients;
62+
var hasMultipleRootClients = rootClients.Count > 1;
63+
var hasOnlyStandardParameters = HasOnlyStandardParameters(inputClient);
64+
65+
if (hasMultipleRootClients && hasOnlyStandardParameters)
66+
{
67+
// Use singleton instance
68+
if (_singletonInstance == null)
69+
{
70+
// Create singleton with namespace-based naming
71+
_singletonInstance = new ClientOptionsProvider(inputClient, clientProvider);
72+
}
73+
return _singletonInstance;
74+
}
75+
76+
// Create client-specific instance
77+
return new ClientOptionsProvider(inputClient, clientProvider);
78+
}
79+
80+
/// <summary>
81+
/// Determines if a client has only standard parameters (ApiVersion and Endpoint).
82+
/// </summary>
83+
/// <param name="inputClient">The input client to check.</param>
84+
/// <returns>True if the client has only standard parameters, false otherwise.</returns>
85+
private static bool HasOnlyStandardParameters(InputClient inputClient)
86+
{
87+
foreach (var parameter in inputClient.Parameters)
88+
{
89+
// Check if parameter is NOT an ApiVersion or Endpoint parameter
90+
if (!parameter.IsApiVersion)
91+
{
92+
if (parameter is InputEndpointParameter endpointParam)
93+
{
94+
// Endpoint parameters are standard
95+
if (!endpointParam.IsEndpoint)
96+
{
97+
return false; // Found a non-standard endpoint parameter
98+
}
99+
}
100+
else
101+
{
102+
// Found a non-ApiVersion, non-Endpoint parameter
103+
return false;
104+
}
105+
}
106+
}
107+
return true;
108+
}
109+
49110
internal PropertyProvider? VersionProperty => _versionProperty;
50111
private FieldProvider? LatestVersionField => _latestVersionField ??= BuildLatestVersionField();
51112

@@ -63,9 +124,43 @@ public ClientOptionsProvider(InputClient inputClient, ClientProvider clientProvi
63124
}
64125

65126
protected override string BuildRelativeFilePath() => Path.Combine("src", "Generated", $"{Name}.cs");
66-
protected override string BuildName() => $"{_clientProvider.Name}Options";
127+
128+
protected override string BuildName()
129+
{
130+
if (this == _singletonInstance)
131+
{
132+
// Use namespace-based naming for singleton
133+
var primaryNamespace = ScmCodeModelGenerator.Instance.InputLibrary.InputNamespace.Name;
134+
var lastSegment = GetLastNamespaceSegment(primaryNamespace);
135+
return $"{lastSegment}ClientOptions";
136+
}
137+
138+
// Use client-specific naming
139+
return $"{_clientProvider.Name}Options";
140+
}
141+
67142
protected override string BuildNamespace() => _clientProvider.Type.Namespace;
68-
protected override FormattableString BuildDescription() => $"Client options for {_clientProvider.Type:C}.";
143+
144+
protected override FormattableString BuildDescription()
145+
{
146+
if (this == _singletonInstance)
147+
{
148+
return $"Client options for clients in this library.";
149+
}
150+
151+
return $"Client options for {_clientProvider.Type:C}.";
152+
}
153+
154+
/// <summary>
155+
/// Gets the last segment of a namespace.
156+
/// </summary>
157+
/// <param name="namespace">The namespace string.</param>
158+
/// <returns>The last segment of the namespace.</returns>
159+
private static string GetLastNamespaceSegment(string @namespace)
160+
{
161+
var lastDotIndex = @namespace.LastIndexOf('.');
162+
return lastDotIndex >= 0 ? @namespace.Substring(lastDotIndex + 1) : @namespace;
163+
}
69164

70165
protected override CSharpType BuildBaseType()
71166
{

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public ClientProvider(InputClient inputClient)
9292
_inputAuth = ScmCodeModelGenerator.Instance.InputLibrary.InputNamespace.Auth;
9393
_endpointParameter = BuildClientEndpointParameter();
9494
_publicCtorDescription = $"Initializes a new instance of {Name}.";
95-
ClientOptions = _inputClient.Parent is null ? new ClientOptionsProvider(_inputClient, this) : null;
95+
ClientOptions = _inputClient.Parent is null ? ClientOptionsProvider.CreateClientOptionsProvider(_inputClient, this) : null;
9696
ClientOptionsParameter = ClientOptions != null ? ScmKnownParameters.ClientOptions(ClientOptions.Type) : null;
9797

9898
var apiKey = _inputAuth?.ApiKey;

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/ScmOutputLibrary.cs

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,40 +13,41 @@ public class ScmOutputLibrary : OutputLibrary
1313
private static TypeProvider[] BuildClientTypes()
1414
{
1515
var inputClients = ScmCodeModelGenerator.Instance.InputLibrary.InputNamespace.RootClients;
16-
var clients = new List<TypeProvider>();
16+
var types = new HashSet<TypeProvider>();
17+
1718
foreach (var inputClient in inputClients)
1819
{
19-
BuildClient(inputClient, clients);
20+
BuildClient(inputClient, types);
2021
}
2122

22-
return [.. clients];
23+
return [.. types];
2324
}
2425

25-
private static void BuildClient(InputClient inputClient, IList<TypeProvider> clients)
26+
private static void BuildClient(InputClient inputClient, HashSet<TypeProvider> types)
2627
{
2728
foreach (var child in inputClient.Children)
2829
{
29-
BuildClient(child, clients);
30+
BuildClient(child, types);
3031
}
3132

3233
var client = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(inputClient);
3334
if (client == null)
3435
{
3536
return;
3637
}
37-
clients.Add(client);
38-
clients.Add(client.RestClient);
38+
types.Add(client);
39+
types.Add(client.RestClient);
3940
var clientOptions = client.ClientOptions;
4041
if (clientOptions != null)
4142
{
42-
clients.Add(clientOptions);
43+
types.Add(clientOptions);
4344
}
4445

4546
foreach (var method in client.Methods)
4647
{
4748
if (method is ScmMethodProvider scmMethod && scmMethod.CollectionDefinition != null)
4849
{
49-
clients.Add(scmMethod.CollectionDefinition);
50+
types.Add(scmMethod.CollectionDefinition);
5051
}
5152
}
5253
}

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ClientOptionsProviderTests.cs

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.ClientModel.Primitives;
66
using System.Collections.Generic;
77
using System.Linq;
8+
using System.Reflection;
89
using System.Threading.Tasks;
910
using Microsoft.TypeSpec.Generator.ClientModel.Providers;
1011
using Microsoft.TypeSpec.Generator.Expressions;
@@ -25,6 +26,10 @@ public class ClientOptionsProviderTests
2526
[SetUp]
2627
public void SetUp()
2728
{
29+
// Reset the singleton instance before each test using reflection
30+
var singletonField = typeof(ClientOptionsProvider).GetField("_singletonInstance", BindingFlags.Static | BindingFlags.NonPublic);
31+
singletonField?.SetValue(null, null);
32+
2833
var categories = TestContext.CurrentContext.Test?.Properties["Category"];
2934
bool containsApiVersions = categories?.Contains(ApiVersionsCategory) ?? false;
3035

@@ -335,5 +340,109 @@ await MockHelpers.LoadMockGeneratorAsync(
335340
Assert.IsTrue(body?.Contains("ServiceVersion.V2023_11_01 => \"2023-11-01\""));
336341
Assert.IsTrue(body?.Contains("ServiceVersion.V2024_01_01 => \"2024-01-01\""));
337342
}
343+
344+
[Test]
345+
public void SingletonCreatedForMultipleClientsWithStandardParameters()
346+
{
347+
var client1 = InputFactory.Client("ClientA", clientNamespace: "TestNamespace");
348+
var client2 = InputFactory.Client("ClientB", clientNamespace: "TestNamespace");
349+
350+
MockHelpers.LoadMockGenerator(clients: () => [client1, client2]);
351+
352+
var clientProvider1 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client1);
353+
var clientProvider2 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client2);
354+
355+
Assert.IsNotNull(clientProvider1);
356+
Assert.IsNotNull(clientProvider2);
357+
358+
var options1 = clientProvider1!.ClientOptions;
359+
var options2 = clientProvider2!.ClientOptions;
360+
361+
Assert.IsNotNull(options1);
362+
Assert.IsNotNull(options2);
363+
364+
// Both clients should share the same ClientOptions instance
365+
Assert.AreSame(options1, options2);
366+
367+
// The name should be based on the InputNamespace's last segment (which is "Sample" in MockHelpers)
368+
Assert.AreEqual("SampleClientOptions", options1!.Name);
369+
}
370+
371+
[Test]
372+
public void SingleClientCreatesClientSpecificOptions()
373+
{
374+
var client = InputFactory.Client("TestClient", clientNamespace: "TestNamespace");
375+
376+
MockHelpers.LoadMockGenerator(clients: () => [client]);
377+
378+
var clientProvider = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client);
379+
380+
Assert.IsNotNull(clientProvider);
381+
382+
var options = clientProvider!.ClientOptions;
383+
384+
Assert.IsNotNull(options);
385+
386+
// The name should be based on the client name
387+
Assert.AreEqual("TestClientOptions", options!.Name);
388+
}
389+
390+
[Test]
391+
public void MultipleClientsWithCustomParametersCreateSeparateOptions()
392+
{
393+
var customParam = InputFactory.MethodParameter(
394+
"customParam",
395+
InputPrimitiveType.String,
396+
isRequired: false,
397+
defaultValue: InputFactory.Constant.String("default"),
398+
scope: InputParameterScope.Client);
399+
400+
var client1 = InputFactory.Client("ClientA", clientNamespace: "TestNamespace", parameters: [customParam]);
401+
var client2 = InputFactory.Client("ClientB", clientNamespace: "TestNamespace");
402+
403+
MockHelpers.LoadMockGenerator(clients: () => [client1, client2]);
404+
405+
var clientProvider1 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client1);
406+
var clientProvider2 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client2);
407+
408+
Assert.IsNotNull(clientProvider1);
409+
Assert.IsNotNull(clientProvider2);
410+
411+
var options1 = clientProvider1!.ClientOptions;
412+
var options2 = clientProvider2!.ClientOptions;
413+
414+
Assert.IsNotNull(options1);
415+
Assert.IsNotNull(options2);
416+
417+
// ClientA has custom parameters, so it should NOT share options
418+
Assert.AreNotSame(options1, options2);
419+
420+
// ClientA should have client-specific options
421+
Assert.AreEqual("ClientAOptions", options1!.Name);
422+
// ClientB should have namespace-based options (since it has only standard parameters)
423+
// Note: InputNamespace in MockHelpers is set to "Sample" by default
424+
Assert.AreEqual("SampleClientOptions", options2!.Name);
425+
}
426+
427+
[Test]
428+
public void NamespaceLastSegmentIsUsedForSingletonName()
429+
{
430+
var client1 = InputFactory.Client("ClientA", clientNamespace: "Company.Service.Api");
431+
var client2 = InputFactory.Client("ClientB", clientNamespace: "Company.Service.Api");
432+
433+
MockHelpers.LoadMockGenerator(clients: () => [client1, client2]);
434+
435+
var clientProvider1 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client1);
436+
437+
Assert.IsNotNull(clientProvider1);
438+
439+
var options = clientProvider1!.ClientOptions;
440+
441+
Assert.IsNotNull(options);
442+
443+
// The name should be based on the InputNamespace's last segment
444+
// Note: InputNamespace in MockHelpers is set to "Sample" by default, not based on client namespace
445+
Assert.AreEqual("SampleClientOptions", options!.Name);
446+
}
338447
}
339448
}

0 commit comments

Comments
 (0)