Improve error handling for duplicate display names#3091
Conversation
Wrap SingleOrDefault by display name in try-catch to handle cases where multiple target users share the same display name. Log detailed error and warning messages, including a suggestion to enable MatchUsersByEmail and a list of matching users, before rethrowing the exception for better diagnostics. Fix for #3090
WalkthroughThe change adds defensive error handling to the user mapping tool when looking up users by display name. It wraps the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-river-093197403-3091.westeurope.5.azurestaticapps.net |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs:
- Around line 199-201: The Log.LogError call in
TfsUserMappingTool::GetUsersInSourceMappedToTarget has a format placeholder
mismatch: the message contains only "{displayName}" but passes two values
(sourceUser.DisplayName, sourceUser.AccountName). Fix by aligning format and
parameters—either add a "{accountName}" placeholder to the format string to
include sourceUser.AccountName, or remove the unused sourceUser.AccountName
argument so only sourceUser.DisplayName is passed to Log.LogError.
🧹 Nitpick comments (1)
src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs (1)
199-201: Avoid string concatenation in log messages.Using
+to concatenate log message strings is dated. Use either string interpolation or a single format string with multiple placeholders. The logging framework handles multi-line messages just fine.♻️ Refactor to use a single format string
-Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. " - + "Consider enabling MatchUsersByEmail option to avoid this issue. ", - sourceUser.DisplayName, sourceUser.AccountName); +Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}' for source user '{accountName}'. Consider enabling MatchUsersByEmail option to avoid this issue.", + sourceUser.DisplayName, sourceUser.AccountName);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs
🧰 Additional context used
📓 Path-based instructions (1)
src/**
📄 CodeRabbit inference engine (.github/instructions/src.instructions.md)
src/**: Comments are for things that cannot be expressed as code
Comments which restate code must be deleted
If the comment says what the code could say, then the code must change to make the comment redundant
Files:
src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs
🧠 Learnings (2)
📚 Learning: 2025-12-03T16:20:01.189Z
Learnt from: CR
Repo: nkdAgility/azure-devops-migration-tools PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-03T16:20:01.189Z
Learning: Applies to src/MigrationTools.ConsoleFull/Program.cs : In MigrationTools.ConsoleFull (Program.cs), support both legacy TFS Object Model and modern REST API migrations. Include assembly loading for dynamically loading client assemblies (TfsObjectModel, FileSystem, AzureDevops.Rest)
Applied to files:
src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs
📚 Learning: 2025-12-03T16:20:01.189Z
Learnt from: CR
Repo: nkdAgility/azure-devops-migration-tools PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-03T16:20:01.189Z
Learning: Applies to src/MigrationTools.ConsoleFull/Program.cs : In MigrationTools.ConsoleFull, dynamically load client assemblies at startup: MigrationTools.Clients.TfsObjectModel.dll, MigrationTools.Clients.FileSystem.dll, and MigrationTools.Clients.AzureDevops.Rest.dll
Applied to files:
src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test, Sonar Cloud Analysis, & Package
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs (2)
202-204: Good diagnostic logging.Listing all matching users with their mail addresses is excellent for troubleshooting. This will save significant support time when users hit this scenario.
193-207: Remove this verification request—the framework gracefully handles the rethrow.The exception is caught by the
Processor.Execute()wrapper (which callsInternalExecute()), setting the processor status toFailed, logging it as critical, and tracking telemetry. The processor fails fast on duplicate display names, but doesn't halt the entire migration job. This is reasonable behaviour—failing fast on a data integrity issue is preferable to silently choosing an arbitrary user.
| Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. " | ||
| + "Consider enabling MatchUsersByEmail option to avoid this issue. ", | ||
| sourceUser.DisplayName, sourceUser.AccountName); |
There was a problem hiding this comment.
Fix the format string parameter mismatch.
Your log message has one placeholder ({displayName}) but you're passing two parameters (sourceUser.DisplayName, sourceUser.AccountName). The second parameter will be ignored by the logging framework—or worse, cause unexpected behaviour depending on the logger implementation. Either add a {accountName} placeholder to the format string or remove the unused parameter.
🔧 Proposed fix
If you want to include the account name:
-Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. "
- + "Consider enabling MatchUsersByEmail option to avoid this issue. ",
- sourceUser.DisplayName, sourceUser.AccountName);
+Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}' for source user '{accountName}'. "
+ + "Consider enabling MatchUsersByEmail option to avoid this issue.",
+ sourceUser.DisplayName, sourceUser.AccountName);Or remove the unused parameter:
-Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. "
- + "Consider enabling MatchUsersByEmail option to avoid this issue. ",
- sourceUser.DisplayName, sourceUser.AccountName);
+Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. "
+ + "Consider enabling MatchUsersByEmail option to avoid this issue.",
+ sourceUser.DisplayName);📝 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.
| Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. " | |
| + "Consider enabling MatchUsersByEmail option to avoid this issue. ", | |
| sourceUser.DisplayName, sourceUser.AccountName); | |
| Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}' for source user '{accountName}'. " | |
| "Consider enabling MatchUsersByEmail option to avoid this issue.", | |
| sourceUser.DisplayName, sourceUser.AccountName); |
| Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. " | |
| + "Consider enabling MatchUsersByEmail option to avoid this issue. ", | |
| sourceUser.DisplayName, sourceUser.AccountName); | |
| Log.LogError("TfsUserMappingTool::GetUsersInSourceMappedToTarget:: Multiple target users found with the same display name '{displayName}'. " | |
| "Consider enabling MatchUsersByEmail option to avoid this issue.", | |
| sourceUser.DisplayName); |
🤖 Prompt for AI Agents
In @src/MigrationTools.Clients.TfsObjectModel/Tools/TfsUserMappingTool.cs around
lines 199 - 201, The Log.LogError call in
TfsUserMappingTool::GetUsersInSourceMappedToTarget has a format placeholder
mismatch: the message contains only "{displayName}" but passes two values
(sourceUser.DisplayName, sourceUser.AccountName). Fix by aligning format and
parameters—either add a "{accountName}" placeholder to the format string to
include sourceUser.AccountName, or remove the unused sourceUser.AccountName
argument so only sourceUser.DisplayName is passed to Log.LogError.
There was a problem hiding this comment.
@copilot, can you add the second placeholder to the message?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
MrHinsh
deleted the
3090-You-have-two-users-with-identical-display-names
branch
Wrap SingleOrDefault by display name in try-catch to handle cases where multiple target users share the same display name. Log detailed error and warning messages, including a suggestion to enable MatchUsersByEmail and a list of matching users, before rethrowing the exception for better diagnostics.
Fix for #3090
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.