Skip to content

Commit

Permalink
Refactor StringManipulator tool so it works just with string and not …
Browse files Browse the repository at this point in the history 
…work item field (#2548)

This should fix #2538

As mention id that bug report, custom user mapping is not applied during
migration. @LudekStipal pointed to the code it was responsible for this.
The problem is not the `StringManipulatorTool` itself, but the value
which is used in that branch of code. The user mapping and all the other
field mapping is done using `oldWorkItem` object, which is of type
`Field`. But that specific branch for strings was processed using
`oldWorkItemData` object which is of type `FieldItem`. So **it is the
different object** and for user fields, it is not working with mapped
user names, because the are mapped in the other place.

This is at least a bug for user fields which are mapped as @LudekStipal
found. But I think, that is is also a potential source of bugs in the
future, if something will be changing here. Because the one who will be
changing something here may not notice, that populating work item fields
it is done in different way for the strings.

`StringManipulatorTool` was working with `FieldItem` type. At first, I
wanted to change it to work with `Field` type, so it would work the same
way as user mapping. But this is not possible, because `Field` type is
from TFS client library, but `StringManipulatorTool` in in different
(lower level) assembly where this is not accessible. So I changes
`StringManipulatorTool` that it does not take a whole field as argument,
but just a string and returns new string. Basically, it does what it
name suggests – takes a plain string, manipulates it and returns a new
string. It was used only in this one place.

This may be a breaking change. The potential problem may be, than
`FieldItem` was modified by `StringManipulatorTool` until now and now it
remains the same. I am not sure if this can be a real issue.
  • Loading branch information
@MrHinsh
MrHinsh authored Dec 4, 2024
2 parents 2107bb5 + bac4352 commit 5451700
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 98 deletions.
5 changes: 3 additions & 2 deletions src/MigrationTools.Clients.TfsObjectModel/Processors/TfsWorkItemMigrationProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,9 @@ private void PopulateWorkItem(WorkItemData oldWorkItemData, WorkItemData newWork
switch (f.FieldDefinition.FieldType)
{
case FieldType.String:
CommonTools.StringManipulator.ProcessorExecutionWithFieldItem(null, oldWorkItemData.Fields[f.ReferenceName]);
newWorkItem.Fields[f.ReferenceName].Value = oldWorkItemData.Fields[f.ReferenceName].Value;
string oldValue = oldWorkItem.Fields[f.ReferenceName].Value.ToString();
string newValue = CommonTools.StringManipulator.ProcessString(oldValue);
newWorkItem.Fields[f.ReferenceName].Value = newValue;
break;
default:
newWorkItem.Fields[f.ReferenceName].Value = oldWorkItem.Fields[f.ReferenceName].Value;
Expand Down
11 changes: 2 additions & 9 deletions src/MigrationTools.Shadows/Tools/MockStringManipulatorTool.cs
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,10 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using MigrationTools.DataContracts;
using MigrationTools.Processors.Infrastructure;
using MigrationTools.Tools.Interfaces;
using MigrationTools.Tools.Interfaces;

namespace MigrationTools.Tools.Shadows
{
public class MockStringManipulatorTool : IStringManipulatorTool
{
public void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fieldItem)
public string? ProcessString(string? value)
{
throw new NotImplementedException();
}
Expand Down
141 changes: 95 additions & 46 deletions src/MigrationTools.Tests/ProcessorEnrichers/StringManipulatorEnricherTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using MigrationTools.DataContracts;
using MigrationTools.Endpoints;
using MigrationTools.Processors;
using MigrationTools.Tests;
using Microsoft.Extensions.Options;
using MigrationTools.Tools;
using MigrationTools.Shadows;
using MigrationTools.Tools;

namespace MigrationTools.ProcessorEnrichers.Tests
{
Expand All @@ -21,7 +17,7 @@ public void StringManipulatorTool_ConfigureTest()
var options = new StringManipulatorToolOptions();
options.Enabled = true;
options.MaxStringLength = 10;
options.Manipulators = new List<RegexStringManipulator>
options.Manipulators = new List<RegexStringManipulator>
{
new RegexStringManipulator
{
Expand All @@ -40,10 +36,10 @@ public void StringManipulatorTool_ConfigureTest()
[TestMethod(), TestCategory("L1")]
public void StringManipulatorTool_RegexTest()
{
var options = new StringManipulatorToolOptions();
var options = new StringManipulatorToolOptions();
options.Enabled = true;
options.MaxStringLength = 10;
options.Manipulators = new List<RegexStringManipulator>
options.MaxStringLength = 10;
options.Manipulators = new List<RegexStringManipulator>
{
new RegexStringManipulator
{
Expand All @@ -56,40 +52,24 @@ public void StringManipulatorTool_RegexTest()

var x = GetStringManipulatorTool(options);

var fieldItem = new FieldItem
{
FieldType = "String",
internalObject = null,
ReferenceName = "Custom.Test",
Name = "Test",
Value = "Test"
};

x.ProcessorExecutionWithFieldItem(null, fieldItem);
string value = "Test";
string? newValue = x.ProcessString(value);

Assert.AreEqual("Test 2", fieldItem.Value);
Assert.AreEqual("Test 2", newValue);
}

[TestMethod(), TestCategory("L1")]
public void StringManipulatorTool_LengthShorterThanMaxTest()
{
var options = new StringManipulatorToolOptions();
options.Enabled = true;
options.MaxStringLength = 10;
options.MaxStringLength = 10;
var x = GetStringManipulatorTool(options);

var fieldItem = new FieldItem
{
FieldType = "String",
internalObject = null,
ReferenceName = "Custom.Test",
Name = "Test",
Value = "Test"
};

x.ProcessorExecutionWithFieldItem(null, fieldItem);
string value = "Test";
string? newValue = x.ProcessString(value);

Assert.AreEqual(4, fieldItem.Value.ToString().Length);
Assert.AreEqual(4, newValue.Length);
}

[TestMethod(), TestCategory("L1")]
Expand All @@ -100,24 +80,93 @@ public void StringManipulatorTool_LengthLongerThanMaxTest()
options.MaxStringLength = 10;
var x = GetStringManipulatorTool(options);

var fieldItem = new FieldItem
{
FieldType = "String",
internalObject = null,
ReferenceName = "Custom.Test",
Name = "Test",
Value = "Test Test Test Test Test Test Test Test Test Test Test Test Test"
};
string value = "Test Test Test Test Test Test Test Test Test Test Test Test Test";
string? newValue = x.ProcessString(value);

x.ProcessorExecutionWithFieldItem(null, fieldItem);
Assert.AreEqual(10, newValue.Length);
}

Assert.AreEqual(10, fieldItem.Value.ToString().Length);
[DataTestMethod(), TestCategory("L1")]
[DataRow(null, null)]
[DataRow("", "")]
[DataRow("lorem", "lorem")]
public void StringManipulatorTool_Disabled(string? value, string? expected)
{
var options = new StringManipulatorToolOptions();
options.Enabled = false;
options.MaxStringLength = 15;
options.Manipulators = new List<RegexStringManipulator>
{
new RegexStringManipulator
{
Enabled = true,
Pattern = "(^.*$)",
Replacement = "$1 $1",
Description = "Test"
}
};
var x = GetStringManipulatorTool(options);

string? newValue = x.ProcessString(value);
Assert.AreEqual(expected, newValue);
}

private static StringManipulatorTool GetStringManipulatorTool()
[DataTestMethod(), TestCategory("L1")]
[DataRow(null, null)]
[DataRow("", " ")]
[DataRow("lorem", "lorem lorem")]
[DataRow("lorem ipsum", "lorem ipsum lor")]
public void StringManipulatorTool_Process(string? value, string? expected)
{
var options = new StringManipulatorToolOptions();
return GetStringManipulatorTool(options);
var options = new StringManipulatorToolOptions();
options.Enabled = true;
options.MaxStringLength = 15;
options.Manipulators = new List<RegexStringManipulator>
{
new RegexStringManipulator
{
Enabled = true,
Pattern = "(^.*$)",
Replacement = "$1 $1",
Description = "Test"
}
};
var x = GetStringManipulatorTool(options);

string? newValue = x.ProcessString(value);
Assert.AreEqual(expected, newValue);
}

[DataTestMethod(), TestCategory("L1")]
[DataRow(null, null)]
[DataRow("", " 1 2")]
[DataRow("lorem", "lorem 1 2")]
public void StringManipulatorTool_MultipleManipulators(string? value, string? expected)
{
var options = new StringManipulatorToolOptions();
options.Enabled = true;
options.MaxStringLength = 15;
options.Manipulators = new List<RegexStringManipulator>
{
new RegexStringManipulator
{
Enabled = true,
Pattern = "(^.*$)",
Replacement = "$1 1",
Description = "Add 1"
},
new RegexStringManipulator
{
Enabled = true,
Pattern = "(^.*$)",
Replacement = "$1 2",
Description = "Add 2"
}
};
var x = GetStringManipulatorTool(options);

string? newValue = x.ProcessString(value);
Assert.AreEqual(expected, newValue);
}

private static StringManipulatorTool GetStringManipulatorTool(StringManipulatorToolOptions options)
Expand All @@ -134,4 +183,4 @@ private static StringManipulatorTool GetStringManipulatorTool(StringManipulatorT
return services.BuildServiceProvider().GetService<StringManipulatorTool>();
}
}
}
}
10 changes: 2 additions & 8 deletions src/MigrationTools/Tools/Interfaces/IStringManipulatorTool.cs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
using System;
using System.Collections.Generic;
using System.Text;
using MigrationTools.DataContracts;
using MigrationTools.Processors.Infrastructure;

namespace MigrationTools.Tools.Interfaces
namespace MigrationTools.Tools.Interfaces
{
public interface IStringManipulatorTool
{
void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fieldItem);
string? ProcessString(string? value);
}
}
66 changes: 33 additions & 33 deletions src/MigrationTools/Tools/StringManipulatorTool.cs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
using System;
using System.Collections.Generic;
using System.Text;
using System.Text.RegularExpressions;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using MigrationTools.DataContracts;
using MigrationTools.Enrichers;
using MigrationTools.Processors;
using MigrationTools.Processors.Infrastructure;
using MigrationTools.Tools.Infrastructure;
using MigrationTools.Tools.Interfaces;

Expand All @@ -19,20 +14,27 @@ namespace MigrationTools.Tools
public class StringManipulatorTool : Tool<StringManipulatorToolOptions>, IStringManipulatorTool
{

public StringManipulatorTool(IOptions<StringManipulatorToolOptions> options, IServiceProvider services, ILogger<StringManipulatorTool> logger, ITelemetryLogger telemetryLogger)
: base(options, services, logger, telemetryLogger)
public StringManipulatorTool(
IOptions<StringManipulatorToolOptions> options,
IServiceProvider services,
ILogger<StringManipulatorTool> logger,
ITelemetryLogger telemetryLogger)
: base(options, services, logger, telemetryLogger)
{
}

public void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fieldItem)
public string? ProcessString(string? value)
{
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem", GetType().Name);
const string logPrefix = nameof(StringManipulatorTool) + "::" + nameof(ProcessString);
Log.LogDebug(logPrefix);

string result = value;
if (!Options.Enabled)
{
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Disabled", GetType().Name);
return;
Log.LogDebug(logPrefix + "::Disabled");
return result;
}
if (fieldItem.FieldType == "String" && fieldItem.Value != null)
if (value is not null)
{
if (!HasManipulators())
{
Expand All @@ -42,45 +44,43 @@ public void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fiel
{
if (manipulator.Enabled)
{
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Running::{Description} with {pattern}", GetType().Name, manipulator.Description, manipulator.Pattern);
var originalValue = fieldItem.Value;
fieldItem.Value = Regex.Replace((string)fieldItem.Value, manipulator.Pattern, manipulator.Replacement);
Log.LogTrace("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Running::{Description}::Original::{@OriginalValue}", GetType().Name, manipulator.Description, originalValue);
Log.LogTrace("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Running::{Description}::New::{@fieldItemValue}", GetType().Name, manipulator.Description, fieldItem.Value);
Log.LogDebug(logPrefix + "::Running::{Description} with {pattern}", manipulator.Description, manipulator.Pattern);
string oldValue = result;
result = Regex.Replace(result, manipulator.Pattern, manipulator.Replacement);
Log.LogTrace(logPrefix + "::Running::{Description}::Original::{@oldValue}", manipulator.Description, oldValue);
Log.LogTrace(logPrefix + "::Running::{Description}::New::{@newValue}", manipulator.Description, result);
}
else
{
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Disabled::{Description}", GetType().Name, manipulator.Description);
Log.LogDebug(logPrefix + "::Disabled::{Description}", manipulator.Description);
}
}

if (HasStringTooLong(fieldItem))
if (IsStringTooLong(result))
{
fieldItem.Value = fieldItem.Value.ToString().Substring(0, Math.Min(fieldItem.Value.ToString().Length, Options.MaxStringLength));
result = result.Substring(0, Options.MaxStringLength);
}
}

return result;
}

private void AddDefaultManipulator()
{
if (Options.Manipulators == null)
if (Options.Manipulators is null)
{
Options.Manipulators = new List<RegexStringManipulator>();
}
Options.Manipulators.Add(new RegexStringManipulator() { Enabled = true, Description = "Default: Removes invalid chars!", Pattern = "[^( -~)\n\r\t]+", Replacement = "" });
Options.Manipulators.Add(new RegexStringManipulator()
{
Enabled = true,
Description = "Default: Removes invalid chars!",
Pattern = "[^( -~)\n\r\t]+",
Replacement = ""
});
}

private bool HasStringTooLong(FieldItem fieldItem)
{
return fieldItem.Value.ToString().Length > 0 && fieldItem.Value.ToString().Length > Options.MaxStringLength;
}
private bool IsStringTooLong(string value) => (value.Length > 0) && (value.Length > Options.MaxStringLength);

private bool HasManipulators()
{
return Options.Manipulators != null && Options.Manipulators.Count > 0;
}
private bool HasManipulators() => Options.Manipulators?.Count > 0;
}

}

0 comments on commit 5451700

Please sign in to comment.