Skip to content

fix: generate parameterized accessor for subclients with subclient-specific parameters #9919

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.

Sign up for GitHub

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

Merged
JoshLove-msft merged 13 commits into from
Mar 6, 2026
Merged

fix: generate parameterized accessor for subclients with subclient-specific parameters #9919

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1389cb6
Initial plan
Copilot Mar 4, 2026
624a489
fix(http-client-js): include subclient parameters in accessor methods
Copilot Mar 4, 2026
533c12b
fix(http-client-csharp): include subclient parameters in accessor met…
Copilot Mar 4, 2026
8c982c7
revert: restore http-client-js/CHANGELOG.md to original state
Copilot Mar 4, 2026
08eb7b2
chore: delete stale http-client-csharp changeset file
Copilot Mar 4, 2026
afdc1da
fix: parameterized accessor for IndividuallyParent subclients with ac…
Copilot Mar 4, 2026
d716f43
revert: remove Spector azure test project per review feedback
Copilot Mar 4, 2026
dd06669
style: run prettier on SampleService/main.tsp
Copilot Mar 4, 2026
cb0f232
chore: regenerate Sample-TypeSpec after Generate.ps1
Copilot Mar 5, 2026
5166912
Changes before error encountered
Copilot Mar 5, 2026
8d161f2
refactor: simplify subclient accessor params - remove propagation det…
Copilot Mar 5, 2026
4cbd977
fix: add MetricsClientParams back to main.tsp and regenerate samples
JoshLove-msft Mar 6, 2026
4a4e7a8
refactor: simplify subclient parameter comparison logic in accessor g…
JoshLove-msft Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 83 additions & 21 deletions ...csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,24 @@ private IReadOnlyList<ParameterProvider> GetSubClientInternalConstructorParamete
return subClientParameters;
}

/// <summary>
/// Determines whether this subclient has non-infrastructure parameters that need to be
/// included as parameters in the parent's accessor method rather than stored on the parent.
/// A subclient has accessor-only parameters if it has non-infrastructure parameters
/// (not API versions, not endpoint) that are not present on the parent's InputClient.Parameters.
/// Uses the raw <see cref="InputClient.Parameters"/> to avoid circular lazy-initialization dependencies.
/// </summary>
internal bool HasAccessorOnlyParameters(InputClient parentInputClient)
Comment thread
JoshLove-msft marked this conversation as resolved.
{
var parentParamNames = parentInputClient.Parameters
.Select(p => p.Name)
.ToHashSet(StringComparer.OrdinalIgnoreCase);

return _inputClient.Parameters
.Where(p => !p.IsApiVersion && !(p is InputEndpointParameter ep && ep.IsEndpoint))
.Any(p => !parentParamNames.Contains(p.Name));
}

private Lazy<IReadOnlyList<ParameterProvider>> _clientParameters;
internal IReadOnlyList<ParameterProvider> ClientParameters => _clientParameters.Value;
private IReadOnlyList<ParameterProvider> GetClientParameters()
Expand Down Expand Up @@ -397,7 +415,10 @@ protected override FieldProvider[] BuildFields()
// add sub-client caching fields
foreach (var subClient in _subClients.Value)
{
if (subClient._clientCachingField != null)
// Only add caching field when the accessor does not require additional parameters.
// If the subclient has parameters that are not on the parent, each accessor call may
// produce a different client instance, so caching is not appropriate.
if (subClient._clientCachingField != null && !subClient.HasAccessorOnlyParameters(_inputClient))
{
fields.Add(subClient._clientCachingField);
}
Expand Down Expand Up @@ -899,8 +920,20 @@ protected override ScmMethodProvider[] BuildMethods()

var cachedClientFieldVar = new VariableExpression(subClient.Type, subClient._clientCachingField.Declaration, IsRef: true);
List<ValueExpression> subClientConstructorArgs = new(3);

// Populate constructor arguments
List<ParameterProvider> accessorMethodParams = [];

// Determine which subclient parameters should be on the accessor method.
// Subclient-specific parameters (not on the parent) need to be passed via the accessor.
var parentEffectiveParamNames = _allClientParameters
.Select(p => p.Name)
.ToHashSet(StringComparer.OrdinalIgnoreCase);
var subClientExtraInputParamNames = subClient._inputClient.Parameters
Comment thread
JoshLove-msft marked this conversation as resolved.
Outdated
.Where(p => !p.IsApiVersion && !(p is InputEndpointParameter ep && ep.IsEndpoint))
.Where(p => !parentEffectiveParamNames.Contains(p.Name))
.Select(p => p.Name)
.ToHashSet(StringComparer.OrdinalIgnoreCase);

// Populate constructor arguments, collecting extra params for the accessor method signature
foreach (var param in subClient._subClientInternalConstructorParams.Value)
{
if (parentClientProperties.TryGetValue(param.Name, out var parentProperty))
Expand All @@ -920,30 +953,59 @@ protected override ScmMethodProvider[] BuildMethods()
subClientConstructorArgs.Add(correspondingApiVersionField.Field);
}
}
else if (subClientExtraInputParamNames.Contains(param.Name))
{
// This parameter is subclient-specific and not available on the parent --
// expose it as an accessor method parameter.
accessorMethodParams.Add(param);
subClientConstructorArgs.Add(param);
}
// else: infra param (pipeline, auth, endpoint) not found in parent mock — silently skip
}

// Create the interlocked compare exchange expression for the body
var interlockedCompareExchange = Static(typeof(Interlocked)).Invoke(
nameof(Interlocked.CompareExchange),
[cachedClientFieldVar, New.Instance(subClient.Type, subClientConstructorArgs), Null]);
var factoryMethodName = subClient.Name.EndsWith(ClientSuffix, StringComparison.OrdinalIgnoreCase)
? $"Get{subClient.Name}"
: $"Get{subClient.Name}{ClientSuffix}";

var factoryMethod = new ScmMethodProvider(
new(
factoryMethodName,
$"Initializes a new instance of {subClient.Type.Name}",
MethodSignatureModifiers.Public | MethodSignatureModifiers.Virtual,
subClient.Type,
null,
[]),
// return Volatile.Read(ref _cachedClient) ?? Interlocked.CompareExchange(ref _cachedClient, new Client(_pipeline, _keyCredential, _endpoint), null) ?? _cachedClient;
Return(
Static(typeof(Volatile)).Invoke(nameof(Volatile.Read), cachedClientFieldVar)
.NullCoalesce(interlockedCompareExchange.NullCoalesce(subClient._clientCachingField))),
this,
ScmMethodKind.Convenience);
ScmMethodProvider factoryMethod;
if (accessorMethodParams.Count > 0)
{
// When the accessor requires extra parameters, caching is not appropriate
// (different parameter values may produce different client instances).
// Return a new instance directly.
factoryMethod = new ScmMethodProvider(
new(
factoryMethodName,
$"Initializes a new instance of {subClient.Type.Name}",
MethodSignatureModifiers.Public | MethodSignatureModifiers.Virtual,
subClient.Type,
null,
[.. accessorMethodParams]),
Return(New.Instance(subClient.Type, subClientConstructorArgs)),
this,
ScmMethodKind.Convenience);
}
else
{
// No extra params - use the existing caching pattern
var interlockedCompareExchange = Static(typeof(Interlocked)).Invoke(
nameof(Interlocked.CompareExchange),
[cachedClientFieldVar, New.Instance(subClient.Type, subClientConstructorArgs), Null]);
factoryMethod = new ScmMethodProvider(
new(
factoryMethodName,
$"Initializes a new instance of {subClient.Type.Name}",
MethodSignatureModifiers.Public | MethodSignatureModifiers.Virtual,
subClient.Type,
null,
[]),
// return Volatile.Read(ref _cachedClient) ?? Interlocked.CompareExchange(ref _cachedClient, new Client(_pipeline, _keyCredential, _endpoint), null) ?? _cachedClient;
Return(
Static(typeof(Volatile)).Invoke(nameof(Volatile.Read), cachedClientFieldVar)
.NullCoalesce(interlockedCompareExchange.NullCoalesce(subClient._clientCachingField))),
this,
ScmMethodKind.Convenience);
}
methods.Add(factoryMethod);
}

Expand Down
110 changes: 110 additions & 0 deletions ...soft.TypeSpec.Generator.ClientModel/test/Providers/ClientProviders/ClientProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,116 @@ public void TestBuildMethods_ForParent_InitializedByBoth_HasSubClientAccessor()
Assert.IsNotNull(cachingField, "Parent should have caching field for subclient with InitializedBy.Individually | Parent");
}

[Test]
public void TestBuildMethods_ForParent_InitializedByBoth_WithSubClientParams_HasParameterizedAccessor()
{
var parentClient = InputFactory.Client("ParentClient");
var subClientParam = InputFactory.PathParameter("resourceId", InputPrimitiveType.String, scope: InputParameterScope.Client);
var subClient = InputFactory.Client(
"SubClient",
parent: parentClient,
parameters: [subClientParam],
initializedBy: InputClientInitializedBy.Individually | InputClientInitializedBy.Parent);

MockHelpers.LoadMockGenerator(
clients: () => [parentClient]);

var parentProvider = new ClientProvider(parentClient);

Assert.IsNotNull(parentProvider);

// The parent should have a factory method for the subclient
var factoryMethod = parentProvider.Methods.FirstOrDefault(
m => m.Signature?.Name == "GetSubClient" || m.Signature?.Name == "GetSubClientClient");
Assert.IsNotNull(factoryMethod, "Parent should have factory method for subclient with parameters");

// The accessor method should include the subclient's extra parameters
Assert.IsNotNull(factoryMethod!.Signature, "Factory method should have a signature");
Assert.AreEqual(1, factoryMethod.Signature!.Parameters.Count,
"Accessor method should include subclient parameters not present on parent");
Assert.AreEqual("resourceId", factoryMethod.Signature.Parameters[0].Name,
"Accessor method parameter should be the subclient's extra parameter");
}

[Test]
public void TestBuildFields_ForParent_InitializedByBoth_WithSubClientParams_NoCachingField()
{
var parentClient = InputFactory.Client("ParentClient");
var subClientParam = InputFactory.PathParameter("resourceId", InputPrimitiveType.String, scope: InputParameterScope.Client);
var subClient = InputFactory.Client(
"SubClient",
parent: parentClient,
parameters: [subClientParam],
initializedBy: InputClientInitializedBy.Individually | InputClientInitializedBy.Parent);

MockHelpers.LoadMockGenerator(
clients: () => [parentClient]);

var parentProvider = new ClientProvider(parentClient);

Assert.IsNotNull(parentProvider);

// The parent should NOT have a caching field for the subclient when the accessor requires parameters,
// since caching is not appropriate when different parameter values produce different client instances.
var cachingField = parentProvider.Fields.FirstOrDefault(f => f.Name == "_cachedSubClient");
Assert.IsNull(cachingField, "Parent should not have caching field for subclient that has subclient-specific parameters in its accessor");
}

[Test]
public void TestBuildMethods_ForParent_InitializedByParentOnly_WithSubClientParams_HasParameterizedAccessor()
{
var parentClient = InputFactory.Client("ParentClient");
var subClientParam = InputFactory.PathParameter("resourceId", InputPrimitiveType.String, scope: InputParameterScope.Client);
var subClient = InputFactory.Client(
"SubClient",
parent: parentClient,
parameters: [subClientParam],
initializedBy: InputClientInitializedBy.Parent);

MockHelpers.LoadMockGenerator(
clients: () => [parentClient]);

var parentProvider = new ClientProvider(parentClient);

Assert.IsNotNull(parentProvider);

// The parent should have a factory method for the subclient
var factoryMethod = parentProvider.Methods.FirstOrDefault(
m => m.Signature?.Name == "GetSubClient" || m.Signature?.Name == "GetSubClientClient");
Assert.IsNotNull(factoryMethod, "Parent should have factory method for subclient with parameters");

// The accessor method should include the subclient's extra parameters
Assert.IsNotNull(factoryMethod!.Signature, "Factory method should have a signature");
Assert.AreEqual(1, factoryMethod.Signature!.Parameters.Count,
"Accessor method should include subclient parameters not present on parent");
Assert.AreEqual("resourceId", factoryMethod.Signature.Parameters[0].Name,
"Accessor method parameter should be the subclient's extra parameter");
}

[Test]
public void TestBuildFields_ForParent_InitializedByParentOnly_WithSubClientParams_NoCachingField()
{
var parentClient = InputFactory.Client("ParentClient");
var subClientParam = InputFactory.PathParameter("resourceId", InputPrimitiveType.String, scope: InputParameterScope.Client);
var subClient = InputFactory.Client(
"SubClient",
parent: parentClient,
parameters: [subClientParam],
initializedBy: InputClientInitializedBy.Parent);

MockHelpers.LoadMockGenerator(
clients: () => [parentClient]);

var parentProvider = new ClientProvider(parentClient);

Assert.IsNotNull(parentProvider);

// The parent should NOT have a caching field for the subclient when the accessor requires parameters,
// since caching is not appropriate when different parameter values produce different client instances.
var cachingField = parentProvider.Fields.FirstOrDefault(f => f.Name == "_cachedSubClient");
Assert.IsNull(cachingField, "Parent should not have caching field for subclient that has subclient-specific parameters in its accessor");
}

private void ValidatePrimaryConstructor(
ConstructorProvider primaryPublicConstructor,
List<InputParameter> inputParameters,
Expand Down
Loading