Skip to content

Commit ba787fc

Browse files
CopilotJonathanCrdJoshLove-msftCopilot
authored
fix: required client init parameters should not trigger separate ClientOptions type (#10375)
`UseSingletonInstance` in `ClientOptionsProvider` treated all non-standard client parameters as requiring a separate client-specific `ClientOptions` class. But required parameters (no `DefaultValue`) never become properties on the options class—they're inlined as constructor parameters on the client. This caused unnecessary per-client options types when using `@@clientInitialization` with required parameters in multi-client scenarios. ### Changes - **`ClientOptionsProvider.cs`**: Added `parameter.DefaultValue != null` check in `UseSingletonInstance` so only optional parameters (which actually become options properties) trigger a separate client-specific options type. This aligns the singleton decision logic with `BuildProperties`, which already filters on `DefaultValue != null`. - **`ClientOptionsProviderTests.cs`**: Added `MultipleClientsWithRequiredCustomParametersShareSingletonOptions` test covering the multi-client scenario where a required custom parameter should still share the singleton options. ### Before/After ```tsp // Multi-client scenario with required init parameter @@clientInitialization(KnowledgeBaseRetrievalClient, { parameters: KnowledgeBaseRetrievalClientOptions, // knowledgeBaseName: string (required) }); ``` **Before:** Generates `KnowledgeBaseRetrievalClientOptions` (separate, unnecessary options type) **After:** Shares singleton `<ServiceName>ClientOptions` with other clients in the library --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: JonathanCrd <17486462+JonathanCrd@users.noreply.github.com> Co-authored-by: jolov <jolov@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3bad3dc commit ba787fc

3 files changed

Lines changed: 58 additions & 4 deletions

File tree

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,13 @@ public static ClientOptionsProvider CreateClientOptionsProvider(InputClient inpu
8585
}
8686

8787
/// <summary>
88-
/// Determines if a client has only standard parameters (ApiVersion and Endpoint).
88+
/// Determines if a client can share the singleton options instance.
89+
/// Multi-service clients always need their own options type for service-specific API version properties.
90+
/// Only optional parameters with default values that become properties on the options class
91+
/// should trigger a separate client-specific options type.
8992
/// </summary>
9093
/// <param name="inputClient">The input client to check.</param>
91-
/// <returns>True if the client has only standard parameters, false otherwise.</returns>
94+
/// <returns>True if the client can share the singleton options instance, false otherwise.</returns>
9295
private static bool UseSingletonInstance(InputClient inputClient)
9396
{
9497
var rootClients = ScmCodeModelGenerator.Instance.InputLibrary.InputNamespace.RootClients;
@@ -98,6 +101,12 @@ private static bool UseSingletonInstance(InputClient inputClient)
98101
return false;
99102
}
100103

104+
// Multi-service clients need their own options type for service-specific API version properties
105+
if (inputClient.IsMultiServiceClient)
106+
{
107+
return false;
108+
}
109+
101110
foreach (var parameter in inputClient.Parameters)
102111
{
103112
// Check if parameter is NOT an ApiVersion or Endpoint parameter
@@ -111,11 +120,15 @@ private static bool UseSingletonInstance(InputClient inputClient)
111120
return false; // Found a non-standard endpoint parameter
112121
}
113122
}
114-
else
123+
else if (parameter.DefaultValue != null)
115124
{
116-
// Found a non-ApiVersion, non-Endpoint parameter
125+
// Found a non-ApiVersion, non-Endpoint optional parameter that will become
126+
// a property on the options class — requires a separate client-specific options type.
117127
return false;
118128
}
129+
// Required parameters (DefaultValue == null) are inlined as constructor parameters
130+
// on the client and do not become properties on the options class,
131+
// so they should not trigger a separate client-specific options type.
119132
}
120133
}
121134
return true;

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,6 +466,42 @@ public void MultipleClientsWithCustomParametersCreateSeparateOptions()
466466
Assert.AreEqual("SampleClientOptions", options2!.Name);
467467
}
468468

469+
[Test]
470+
public void MultipleClientsWithRequiredCustomParametersShareSingletonOptions()
471+
{
472+
// Required parameters (no DefaultValue) should NOT trigger a separate client-specific options type.
473+
// They are inlined as constructor parameters on the client, not as properties on ClientOptions.
474+
var requiredParam = InputFactory.MethodParameter(
475+
"knowledgeBaseName",
476+
InputPrimitiveType.String,
477+
isRequired: true,
478+
scope: InputParameterScope.Client);
479+
480+
var client1 = InputFactory.Client("KnowledgeBaseRetrievalClient", clientNamespace: "TestNamespace", parameters: [requiredParam]);
481+
var client2 = InputFactory.Client("SearchClient", clientNamespace: "TestNamespace");
482+
483+
MockHelpers.LoadMockGenerator(clients: () => [client1, client2]);
484+
485+
var clientProvider1 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client1);
486+
var clientProvider2 = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(client2);
487+
488+
Assert.IsNotNull(clientProvider1);
489+
Assert.IsNotNull(clientProvider2);
490+
491+
var options1 = clientProvider1!.ClientOptions;
492+
var options2 = clientProvider2!.ClientOptions;
493+
494+
Assert.IsNotNull(options1);
495+
Assert.IsNotNull(options2);
496+
497+
// Both clients should share the same singleton ClientOptions instance
498+
// because the required parameter does not become a property on the options class
499+
Assert.AreSame(options1, options2);
500+
501+
// The name should be based on the InputNamespace (singleton naming)
502+
Assert.AreEqual("SampleClientOptions", options1!.Name);
503+
}
504+
469505
[Test]
470506
public void NamespaceLastSegmentIsUsedForSingletonName()
471507
{

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.ClientModel.Primitives;
77
using System.Collections.Generic;
88
using System.Linq;
9+
using System.Reflection;
910
using System.Runtime.CompilerServices;
1011
using System.Text.RegularExpressions;
1112
using System.Threading.Tasks;
@@ -69,6 +70,10 @@ public bool ValidateIsLastNamespaceSegmentTheSame(string left, string right)
6970
[SetUp]
7071
public void SetUp()
7172
{
73+
// Reset the singleton instance before each test to prevent state leaking
74+
var singletonField = typeof(ClientOptionsProvider).GetField("_singletonInstance", BindingFlags.Static | BindingFlags.NonPublic);
75+
singletonField?.SetValue(null, null);
76+
7277
var categories = TestContext.CurrentContext.Test?.Properties["Category"];
7378
_containsSubClients = categories?.Contains(SubClientsCategory) ?? false;
7479
_hasKeyAuth = categories?.Contains(KeyAuthCategory) ?? false;

0 commit comments

Comments
 (0)