Skip to content

Refactor GetFieldMappings for missing mappings #3060

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
MrHinsh merged 1 commit into from
Nov 12, 2025
Merged

Refactor GetFieldMappings for missing mappings #3060

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 6 additions & 5 deletions src/MigrationTools/Tools/FieldMappingTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
/// <param name="services">Service provider for dependency injection</param>
/// <param name="logger">Logger for the tool operations</param>
/// <param name="telemetry">Telemetry logger for tracking operations</param>
public FieldMappingTool(IOptions<FieldMappingToolOptions> options, IServiceProvider services, ILogger<ITool> logger, ITelemetryLogger telemetry) : base(options, services, logger, telemetry)

Check warning on line 28 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

Update this logger to use its enclosing type. (https://rules.sonarsource.com/csharp/RSPEC-6672)

Check warning on line 28 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

Update this logger to use its enclosing type. (https://rules.sonarsource.com/csharp/RSPEC-6672)

Check warning on line 28 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

Update this logger to use its enclosing type. (https://rules.sonarsource.com/csharp/RSPEC-6672)
{
if (Options.FieldMaps != null)
{
Expand All @@ -41,8 +41,8 @@

if (type == null)
{
Log.LogError("Type " + typePattern + " not found.", typePattern);

Check warning on line 44 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

Don't use string concatenation in logging message templates. (https://rules.sonarsource.com/csharp/RSPEC-2629)

Check warning on line 44 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

Don't use string concatenation in logging message templates. (https://rules.sonarsource.com/csharp/RSPEC-2629)
throw new Exception("Type " + typePattern + " not found.");

Check warning on line 45 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

'System.Exception' should not be thrown by user code. (https://rules.sonarsource.com/csharp/RSPEC-112)

Check warning on line 45 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

'System.Exception' should not be thrown by user code. (https://rules.sonarsource.com/csharp/RSPEC-112)
}
IFieldMap fm = (IFieldMap)Services.GetRequiredService(type);
fm.Configure(fieldmapConfig);
Expand All @@ -62,7 +62,7 @@
{
if (string.IsNullOrEmpty(workItemTypeName))
{
throw new IndexOutOfRangeException("workItemTypeName on all fieldmaps must be set to at least '*'.");

Check warning on line 65 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

'System.IndexOutOfRangeException' should not be thrown by user code. (https://rules.sonarsource.com/csharp/RSPEC-112)

Check warning on line 65 in src/MigrationTools/Tools/FieldMappingTool.cs

View workflow job for this annotation

GitHub Actions / Build, Test, Sonar Cloud Analysis, & Package 

'System.IndexOutOfRangeException' should not be thrown by user code. (https://rules.sonarsource.com/csharp/RSPEC-112)
}
if (!_fieldMaps.ContainsKey(workItemTypeName))
{
Expand All @@ -73,15 +73,16 @@

public List<IFieldMap> GetFieldMappings(string witName)
{
if (_fieldMaps.TryGetValue("*", out List<IFieldMap> fieldMaps))
var result = new List<IFieldMap>();
if (_fieldMaps.TryGetValue("*", out var globalMaps))
{
return fieldMaps;
result.AddRange(globalMaps);
}
else if (_fieldMaps.TryGetValue(witName, out fieldMaps))
if (_fieldMaps.TryGetValue(witName, out var specificMaps))
{
return fieldMaps;
result.AddRange(specificMaps);
}
return [];
return result;
}
Comment on lines 74 to 86
Copy link

@coderabbitai coderabbitai bot Nov 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify that combining global and specific mappings won't cause duplicate application issues.

The refactoring is clean and the intent is clear, but this is a significant behavioral change. Previously, the method returned either global or specific mappings; now it returns both concatenated (global first, then specific).

Consider these scenarios:

  1. Duplicate application: If the same field mapping is registered both globally ("*") and for a specific work item type, it will now execute twice via ProcessFieldMapList. Verify that field maps are idempotent or that duplicate application is the intended behavior.
  2. Ordering dependency: Global mappings execute before specific ones. If later mappings override earlier ones, specific mappings will win—confirm this is correct for your use case.
  3. Breaking change: Existing behavior expected one or the other; now callers get both. Ensure test coverage validates this new combined behavior.

Run the following script to check if the same field map instance can appear in multiple lists:

🏁 Script executed:

#!/bin/bash
# Description: Search for usage patterns of AddFieldMap to understand if the same 
# IFieldMap instance can be registered for both "*" and specific work item types.

# Search for AddFieldMap calls and field map registration patterns
rg -n -C5 'AddFieldMap\(' --type=cs

# Look for field map configuration and ApplyTo patterns
rg -n -C3 'ApplyTo' --type=cs -g '!*.Designer.cs'

# Search for tests covering GetFieldMappings to verify expected behavior
ast-grep --pattern 'GetFieldMappings($$$)'

Length of output: 21055

Add deduplication logic or verify field map idempotency—the refactoring creates duplicate execution risk.

The concern is valid and specific to your architecture. When a field map is configured with ApplyTo: ["*", "Bug"], the same IFieldMap instance is stored in _fieldMaps["*"] and _fieldMaps["Bug"] (lines 49–51). Your refactored GetFieldMappings("Bug") will now return that instance twice—once from the wildcard lookup and once from the specific type lookup. The instance executes twice when passed to ProcessFieldMapList.

This is straightforward: either deduplicate the result list by instance identity before returning, or verify that every field map implementation is idempotent. The examples in the codebase show ApplyTo containing mixed wildcards and specific types, so this isn't a contrived edge case.

Additionally, add a null or empty check for the witName parameter (line 74) to match the defensive validation in AddFieldMap (line 63).

🤖 Prompt for AI Agents 
In src/MigrationTools/Tools/FieldMappingTool.cs around lines 74 to 86,
GetFieldMappings currently returns duplicates when a field map is registered for
both "*" and a specific wit (e.g., "Bug") and also lacks defensive validation of
the witName parameter; fix it by first validating witName is not null or empty
(throw ArgumentNullException/ArgumentException to match AddFieldMap), then
collect mappings into a collection that deduplicates by instance identity (e.g.,
use a HashSet with reference equality or track seen instances) while preserving
the intended order (global first then specific), and finally return the
deduplicated List<IFieldMap>.


public void ApplyFieldMappings(WorkItemData source, WorkItemData target)
Expand Down
Loading