Fix duplicate client builder extension methods when both credential and non-credential constructors share effective parameters#56408
Conversation
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
jorgerangel-msft
left a comment
There was a problem hiding this comment.
@copilot Ensure all tests are passing and rerun the ./eng/scripts/Generate.ps1 script
...p-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs
Outdated
Show resolved
Hide resolved
...p-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs
Outdated
Show resolved
Hide resolved
...p-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs
Outdated
Show resolved
Hide resolved
....Generator/test/Providers/ClientBuilderExtensionsDefinitions/ClientBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
…arer, check AzureKeyCredential, rename test Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
...p-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs
Outdated
Show resolved
Hide resolved
…TypeIgnoreNullableComparer Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the generator emits duplicate client builder extension methods with identical signatures but different generic constraints when a client has both credential (TokenCredential or AzureKeyCredential) and non-credential constructors with the same effective parameters.
Changes:
- Pre-collects parameter types from all credential constructors before generating extension methods
- Skips non-credential constructors whose parameters would create duplicate extension method signatures
- Adds test coverage for the TokenCredential duplicate scenario with golden file validation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ClientBuilderExtensionsDefinition.cs | Implements deduplication logic by pre-collecting credential constructor parameter types into a HashSet and skipping conflicting non-credential constructors |
| ClientBuilderExtensionsTests.cs | Adds SkipsDuplicateNonCredentialExtension test and TestUriEndpointCustomCodeView helper class to verify the fix |
| SkipsDuplicateNonCredentialExtension.cs | Golden file showing expected output with only the credential version of the extension method |
Comments suppressed due to low confidence (3)
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs:114
- The comment is misleading. It says "Skip non-credential constructors that would produce the same extension method signature as an existing TokenCredential constructor" but the code (line 86) collects parameters from BOTH TokenCredential AND AzureKeyCredential constructors. The comment should be updated to say "TokenCredential or AzureKeyCredential constructor" to accurately reflect the implementation.
// Skip non-credential constructors that would produce the same extension method signature
// as an existing TokenCredential constructor. Prefer the credential version.
eng/packages/http-client-csharp/generator/Azure.Generator/test/Providers/ClientBuilderExtensionsDefinitions/ClientBuilderExtensionsTests.cs:137
- The test only covers the TokenCredential duplicate scenario but not the AzureKeyCredential scenario. Since the fix (line 86 in ClientBuilderExtensionsDefinition.cs) handles both TokenCredential AND AzureKeyCredential constructors identically, there should be a test case to verify that AzureKeyCredential duplicates are also handled correctly. Consider adding a test similar to SkipsDuplicateNonCredentialExtension but using apiKeyAuth instead of oauth2Auth to ensure the fix works for both credential types.
[Test]
public void SkipsDuplicateNonCredentialExtension()
{
var inputClient = InputFactory.Client("TestClient", "Samples", "");
var plugin = MockHelpers.LoadMockGenerator(
oauth2Auth: () => new InputOAuth2Auth([new InputOAuth2Flow(["mock"], null, null, null)]),
clients: () => [inputClient]);
var client = plugin.Object.OutputLibrary.TypeProviders
.OfType<ClientProvider>().Single();
Assert.IsNotNull(client);
// Add a custom constructor (Uri endpoint, Options) that has the same effective params
// as the generated TokenCredential constructor (Uri endpoint, TokenCredential, Options).
// The non-credential extension method should be skipped in favor of the credential version.
MockHelpers.SetCustomCodeView(client, new TestUriEndpointCustomCodeView(client));
var builderExtensions = plugin.Object.OutputLibrary.TypeProviders
.OfType<ClientBuilderExtensionsDefinition>().SingleOrDefault();
Assert.IsNotNull(builderExtensions);
var writer = new TypeProviderWriter(builderExtensions!);
var file = writer.Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs:122
- The deduplication logic uses a flat set of parameter types from all credential constructors, which could lead to unexpected behavior in edge cases. If there are multiple credential constructors with different parameters (e.g., one with Uri endpoint, another with string connectionString), a custom non-credential constructor combining both parameters (Uri endpoint, string connectionString) would be skipped even though it represents a different parameter combination than any single credential constructor.
While this scenario is unlikely in practice, it means that developers cannot create custom non-credential constructors with parameter combinations that happen to be a superset of types used across multiple credential constructors. Consider documenting this behavior or tracking parameter sequences (ordered lists) per credential constructor rather than merging all types into a single set, to only skip exact matches.
// Pre-collect the effective (non-credential, non-options) parameter types for credential constructors
// to avoid generating duplicate extension methods that favor non-credential versions.
var comparer = new CSharpType.CSharpTypeIgnoreNullableComparer();
var credentialParamSets = new HashSet<CSharpType>(comparer);
foreach (var ctor in client.CanonicalView.Constructors)
{
if (!ctor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public))
continue;
if (ctor.Signature.Parameters.LastOrDefault()?.Type.Name.Equals(client.ClientOptionsParameter.Type.Name) != true)
continue;
if (ctor.Signature.Parameters.Count >= 2)
{
var credType = ctor.Signature.Parameters[^2].Type;
if (comparer.Equals(credType, typeof(TokenCredential)) || comparer.Equals(credType, typeof(AzureKeyCredential)))
{
foreach (var param in ctor.Signature.Parameters.SkipLast(2))
credentialParamSets.Add(param.Type);
}
}
}
foreach (var constructor in client.CanonicalView.Constructors)
{
if (!constructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public))
{
continue;
}
// Only add overloads for the full constructors that include the client options parameter
// Check that the name of the last parameter matches the client options parameter as the namespace will not be resolved for
// customized constructors. This is safe as we don't allow multiple types types with the same name in an Azure library.
if (constructor.Signature.Parameters.LastOrDefault()?.Type.Name.Equals(client.ClientOptionsParameter.Type.Name) != true)
{
continue;
}
// get the second to last parameter, which is the location of the auth credential parameter if there is one
var authParameter = constructor.Signature.Parameters[^2];
var isTokenCredential = authParameter?.Type.Equals(typeof(TokenCredential)) == true;
// Skip non-credential constructors that would produce the same extension method signature
// as an existing TokenCredential constructor. Prefer the credential version.
if (!isTokenCredential)
{
var nonCredParams = constructor.Signature.Parameters.SkipLast(1).ToArray();
if (nonCredParams.Length > 0 && nonCredParams.All(p => credentialParamSets.Contains(p.Type)))
{
continue;
}
}
...p-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs
Show resolved
Hide resolved
...p-client-csharp/generator/Azure.Generator/src/Providers/ClientBuilderExtensionsDefinition.cs
Show resolved
Hide resolved
…loop Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
|
/check-enforcer reset |
|
/check-enforcer evaluate |
When a client has a
TokenCredentialorAzureKeyCredentialconstructor and a non-credential constructor whose remaining parameters (after stripping auth/options) are identical, the generator emits twoAddXxxClient<TBuilder>overloads with the same signature but different generic constraints — which is a compile error.Root cause:
ClientBuilderExtensionsDefinition.BuildMethods()iterated over all public constructors unconditionally, generating an extension method for each without checking for signature collisions.Fix: Before the constructor loop, pre-compute the effective parameter types (params minus credential + options) for every public
TokenCredentialorAzureKeyCredentialconstructor into aHashSet<CSharpType>usingCSharpType.CSharpTypeIgnoreNullableComparer. Then skip any non-credential constructor whose effective parameters are all present in that set — preferring the credential version.Changes:
ClientBuilderExtensionsDefinition.cs— pre-collect effective parameter types from credential constructors (bothTokenCredentialandAzureKeyCredential) into aHashSet<CSharpType>(new CSharpType.CSharpTypeIgnoreNullableComparer()); skip conflicting non-credential constructors using a simpleAll(p => set.Contains(p.Type))check; allifandforeachstatements use braces consistentlySkipsDuplicateNonCredentialExtensionwith golden fileThis checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.