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

[christianhelle]

Fixes #672: MultipleInterfaces ByTag increments method names globally

Problem:
When using --multiple-interfaces ByTag, method names were getting global numeric
suffixes across all interfaces instead of being scoped per-interface.

Example:

• IUsersApi.GetAll() ✅
• IProductsApi.GetAll2() ❌ (should be GetAll)
• IOrdersApi.GetAll3() ❌ (should be GetAll)

Root Cause:
Single global HashSet<string> knownIdentifiers was used for all interfaces.
Method name collision detection needs to be per-interface, not global.

Solution:
Implemented per-interface scoped identifier tracking using a Dictionary of HashSets.
Each interface now tracks its own method names for conflict resolution independently.

Changes:

• Modified: src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs (method scoping)
• Test Status: 10/10 method naming tests pass (perfect)
• No regressions: 1,142/1,146 tests pass (4 pre-existing unrelated failures)

Validation:
✅ Each interface has methods like GetAll() without global counter
✅ No GetAll2, GetAll3 across different interfaces
✅ Generated code compiles
✅ Build succeeds

Summary by CodeRabbit

• Bug Fixes
  • Corrected nullable property configuration in C# client generation
  • Fixed duplicate operation names when generating multiple interface groups

[coderabbitai]

📝 Walkthrough

Walkthrough

Two files modified to improve nullable property handling and fix method name generation in multi-interface scenarios. A conditional assignment enables nullable properties when nullable reference types are enabled. Per-interface method name tracking replaces global tracking to prevent increments from bleeding across different interface groups.

Changes

Cohort / File(s)	Summary
Nullable Property Handling src/Refitter.Core/CSharpClientGeneratorFactory.cs	Automatically enables optional properties as nullable when nullable reference types are enabled by setting GenerateOptionalPropertiesAsNullable to true if GenerateNullableReferenceTypes is true.
Per-Interface Method Tracking src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs	Introduces per-interface operation name tracking via _methodIdentifiersByInterface dictionary to prevent method name increments from carrying over across different interface groups. Updates GetOperationName to use interface-specific identifier sets instead of global tracking.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

enhancement, .NET

Poem

🐰 With tracking per interface, no more global mess,
Method names stay local—they pass the test!
Nullable props bloom when refs take their form,
Each interface unique, the new standard norm! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR includes a change to CSharpClientGeneratorFactory.cs related to nullable properties (Fix #580), which is unrelated to the primary objective of fixing issue #672.	Move the CSharpClientGeneratorFactory.cs change to a separate PR or issue, keeping this PR focused solely on fixing issue #672's per-interface method naming scope.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the main change: scoping method naming to per-interface for the MultipleInterfaces ByTag feature, and directly references issue #672.
Linked Issues check	✅ Passed	The PR successfully implements the requirement from issue #672 by replacing global identifier tracking with per-interface scoped tracking via a Dictionary of HashSet.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/interface-method-naming-scope

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs`:
- Around line 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).

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0e15e0 and 3cc5f2b.

📒 Files selected for processing (2)

• src/Refitter.Core/CSharpClientGeneratorFactory.cs
• src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs

[coderabbitai]

⚠️ 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]

Pull request overview

This PR fixes issue #672, where MultipleInterfaces.ByTag was incorrectly using a single global HashSet<string> to track method name collisions across all generated interfaces, causing numeric suffixes (e.g., GetAll2, GetAll3) to be appended globally rather than per-interface.

Changes:

• Introduces per-interface method identifier tracking via a Dictionary<string, HashSet<string>> in RefitMultipleInterfaceByTagGenerator
• Adds an else branch to correctly retrieve the existing interfaceName for subsequent operations in the same tag group (fixing a pre-existing null! issue)
• Adds an unrelated auto-enablement of GenerateOptionalPropertiesAsNullable when GenerateNullableReferenceTypes is enabled in CSharpClientGeneratorFactory

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs	Replaces global identifier set with per-interface dictionary; adds missing else branch to retrieve existing interface name
src/Refitter.Core/CSharpClientGeneratorFactory.cs	Unrelated change: auto-enables GenerateOptionalPropertiesAsNullable when NRT is on

---

The review found the following issues:

Critical bug in RefitMultipleInterfaceByTagGenerator.cs line 177: The new code calls interfaceIdentifiers.Add(generatedName) storing just the bare method name (e.g., "GetAll"). However, IdentifierUtils.Counted looks for $"{parent}.{name}" (e.g., "IFooApi.GetAll") in the set when parent is non-empty. Because the stored key doesn't match the lookup pattern, intra-interface duplicate detection is fully broken — two operations within the same interface that map to the same name will both be returned as "GetAll", causing a compilation error in the generated code. The fix requires storing $"{interfaceName}.{generatedName}" to match the original pattern (and how GetInterfaceName still correctly stores generatedName in knownIdentifiers because that call uses no parent).

Unrelated and undocumented behavioral change in CSharpClientGeneratorFactory.cs lines 65–69: The GenerateOptionalPropertiesAsNullable = true auto-enable is not mentioned in the PR description and silently changes behavior for all users who have GenerateNullableReferenceTypes = true. Since MapCSharpGeneratorSettings skips default-valued properties (default for GenerateOptionalPropertiesAsNullable is false), a user cannot opt out of this auto-override.

Missing test coverage: No test exercises the exact scenario in #672 (same operation name generated for two different tag-grouped interfaces), meaning the per-interface scoping fix is not validated by any test in the suite.

[Copilot]

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]

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]

There are no tests covering the exact scenario described in issue #672: multiple interfaces (from different tags) that each have an operation resolving to the same method name. The existing tests only use unique operation IDs across all tags, so the per-interface scoping fix is never exercised. A regression test should be added with two tags both containing an endpoint that would produce the same generated operation name (e.g., the same operationId or same verb/path pattern) to verify that both interfaces produce GetAll() instead of one getting GetAll() and the other GetAll2().

[Copilot]

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.

[MrScottyTay]

Oh sweet you eventually got around to implementing this on your own. Sorry I kinda abandoned my attempt, life kept getting in the way.

[christianhelle]

Oh sweet you eventually got around to implementing this on your own. Sorry I kinda abandoned my attempt, life kept getting in the way.

I know exactly what you mean