Skip to content

Fix #672: MultipleInterfaces ByTag method naming scoped per-interface #922

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
christianhelle merged 2 commits into from
Mar 3, 2026
Merged

Fix #672: MultipleInterfaces ByTag method naming scoped per-interface #922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24eb22b
Fix #580: Auto-enable GenerateOptionalPropertiesAsNullable for nullab…
christianhelle Mar 3, 2026
3cc5f2b
Fix #672: Scope method naming per-interface instead of globally
christianhelle Mar 3, 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
6 changes: 6 additions & 0 deletions src/Refitter.Core/CSharpClientGeneratorFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ public CustomCSharpClientGenerator Create()
settings.CodeGeneratorSettings,
generator.Settings.CSharpGeneratorSettings);

// Auto-enable optional properties as nullable when nullable reference types enabled
if (generator.Settings.CSharpGeneratorSettings.GenerateNullableReferenceTypes)
{
generator.Settings.CSharpGeneratorSettings.GenerateOptionalPropertiesAsNullable = true;
}
Comment on lines +65 to +69

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change silently overrides the user's GenerateOptionalPropertiesAsNullable setting whenever GenerateNullableReferenceTypes is enabled, with no way for the user to opt out. Since MapCSharpGeneratorSettings skips properties that match the default value (false), a user who explicitly wants GenerateOptionalPropertiesAsNullable = false while using GenerateNullableReferenceTypes = true cannot prevent this override. This is a breaking behavioral change for existing users who have GenerateNullableReferenceTypes = true. The PR description does not mention this change at all — it only describes the ByTag method naming fix — making it even harder for users to anticipate this new behavior. This change should either be reverted if unintentional, or at minimum, it should only apply when the user has not explicitly set GenerateOptionalPropertiesAsNullable in their settings.

Copilot uses AI. Check for mistakes.

return generator;
}

Expand Down
22 changes: 20 additions & 2 deletions src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace Refitter.Core;
internal class RefitMultipleInterfaceByTagGenerator : RefitInterfaceGenerator
{
private readonly HashSet<string> knownIdentifiers = new();
private Dictionary<string, HashSet<string>>? _methodIdentifiersByInterface;

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing private field knownIdentifiers uses camelCase without an underscore prefix, but the new field _methodIdentifiersByInterface uses the _camelCase convention with a leading underscore. These two private fields in the same class follow inconsistent naming conventions. The new field should be renamed to methodIdentifiersByInterface to be consistent with the existing codebase convention in this file.

Copilot uses AI. Check for mistakes.

internal RefitMultipleInterfaceByTagGenerator(
RefitGeneratorSettings settings,
Expand All @@ -29,6 +30,8 @@ public override IEnumerable<GeneratedCode> GenerateCode()

Dictionary<string, StringBuilder> interfacesByGroup = new();
Dictionary<string, string> interfacesNamesByGroup = new();

_methodIdentifiersByInterface = new();

foreach (var kv in byGroup)
{
Expand Down Expand Up @@ -59,6 +62,10 @@ public override IEnumerable<GeneratedCode> GenerateCode()

interfacesNamesByGroup[kv.Key] = interfaceName;
}
else
{
interfaceName = interfacesNamesByGroup[kv.Key];
}

var operationName = GetOperationName(interfaceName, op.PathItem.Key, operations.Key, operation);
var dynamicQuerystringParameterType = operationName + "QueryParams";
Expand Down Expand Up @@ -155,8 +162,19 @@ private string GetOperationName(
string verb,
OpenApiOperation operation)
{
var generatedName = IdentifierUtils.Counted(knownIdentifiers, GenerateOperationName(name, verb, operation, capitalizeFirstCharacter: true), parent: interfaceName);
knownIdentifiers.Add($"{interfaceName}.{generatedName}");
// Initialize per-interface tracking if needed
if (!_methodIdentifiersByInterface!.ContainsKey(interfaceName))
{
_methodIdentifiersByInterface[interfaceName] = new HashSet<string>();
}

var interfaceIdentifiers = _methodIdentifiersByInterface[interfaceName];
var generatedName = IdentifierUtils.Counted(
interfaceIdentifiers,
GenerateOperationName(name, verb, operation, capitalizeFirstCharacter: true),
parent: interfaceName);

interfaceIdentifiers.Add(generatedName);
Comment on lines +165 to +177

@coderabbitai coderabbitai Bot Mar 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Method-collision detection is broken by mismatched identifier keys.

At Line 172, IdentifierUtils.Counted(...) is called with parent: interfaceName, so it checks keys like IUsersApi.GetAll.
But at Line 177, the set stores only GetAll. This means duplicates in the same interface won’t be detected.

🐛 Proposed fix 
-        var generatedName = IdentifierUtils.Counted(
-            interfaceIdentifiers,
-            GenerateOperationName(name, verb, operation, capitalizeFirstCharacter: true),
-            parent: interfaceName);
+        var generatedName = IdentifierUtils.Counted(
+            interfaceIdentifiers,
+            GenerateOperationName(name, verb, operation, capitalizeFirstCharacter: true));

This keeps collision checks and stored identifiers in the same format.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Initialize per-interface tracking if needed
if (!_methodIdentifiersByInterface!.ContainsKey(interfaceName))
{
_methodIdentifiersByInterface[interfaceName] = new HashSet<string>();
}
var interfaceIdentifiers = _methodIdentifiersByInterface[interfaceName];
var generatedName = IdentifierUtils.Counted(
interfaceIdentifiers,
GenerateOperationName(name, verb, operation, capitalizeFirstCharacter: true),
parent: interfaceName);
interfaceIdentifiers.Add(generatedName);
// Initialize per-interface tracking if needed
if (!_methodIdentifiersByInterface!.ContainsKey(interfaceName))
{
_methodIdentifiersByInterface[interfaceName] = new HashSet<string>();
}
var interfaceIdentifiers = _methodIdentifiersByInterface[interfaceName];
var generatedName = IdentifierUtils.Counted(
interfaceIdentifiers,
GenerateOperationName(name, verb, operation, capitalizeFirstCharacter: true));
interfaceIdentifiers.Add(generatedName);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs` around lines 165 -
177, The collision check uses IdentifierUtils.Counted(..., parent:
interfaceName) which expects keys formatted with the interface prefix (e.g.,
"IUsersApi.GetAll"), but the code then stores only the short name into the set;
update the storage to match the same keyed format used by the collision check
(i.e., when calling interfaceIdentifiers.Add(...) append or compose the
interfaceName prefix the same way Counted expects) so the generated name key
stored in _methodIdentifiersByInterface[interfaceName] matches the format used
by IdentifierUtils.Counted (refer to IdentifierUtils.Counted,
GenerateOperationName, and the
_methodIdentifiersByInterface/interfaceIdentifiers.Add call to locate the
change).

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interfaceIdentifiers.Add(generatedName) call stores only the bare method name (e.g., "GetAll"), but IdentifierUtils.Counted checks for the qualified form $"{parent}.{name}" (e.g., "IFooApi.GetAll") when parent is non-empty. Because the stored key never matches what the lookup expects, intra-interface duplicate detection is broken: if two operations within the same interface resolve to the same name, Counted will return the same name twice rather than appending a counter. The fix is to store the qualified form $"{interfaceName}.{generatedName}" to match the original pattern, as was done before this change.

Suggested change
interfaceIdentifiers.Add(generatedName);
interfaceIdentifiers.Add($"{interfaceName}.{generatedName}");

Copilot uses AI. Check for mistakes.
return generatedName;
}

Expand Down
Loading