Skip to content

Commit 5451700

Browse files
MrHinshMrHinsh
authored
Refactor StringManipulator tool so it works just with string and not 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.
2 parents 2107bb5 + bac4352 commit 5451700

File tree

5 files changed

+135
-98
lines changed

5 files changed

+135
-98
lines changed

‎src/MigrationTools.Clients.TfsObjectModel/Processors/TfsWorkItemMigrationProcessor.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -449,8 +449,9 @@ private void PopulateWorkItem(WorkItemData oldWorkItemData, WorkItemData newWork
449449
switch (f.FieldDefinition.FieldType)
450450
{
451451
case FieldType.String:
452-
CommonTools.StringManipulator.ProcessorExecutionWithFieldItem(null, oldWorkItemData.Fields[f.ReferenceName]);
453-
newWorkItem.Fields[f.ReferenceName].Value = oldWorkItemData.Fields[f.ReferenceName].Value;
452+
string oldValue = oldWorkItem.Fields[f.ReferenceName].Value.ToString();
453+
string newValue = CommonTools.StringManipulator.ProcessString(oldValue);
454+
newWorkItem.Fields[f.ReferenceName].Value = newValue;
454455
break;
455456
default:
456457
newWorkItem.Fields[f.ReferenceName].Value = oldWorkItem.Fields[f.ReferenceName].Value;

‎src/MigrationTools.Shadows/Tools/MockStringManipulatorTool.cs‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,10 @@
1-
using System;
2-
using System.Collections.Generic;
3-
using System.Linq;
4-
using System.Text;
5-
using System.Threading.Tasks;
6-
using MigrationTools.DataContracts;
7-
using MigrationTools.Processors.Infrastructure;
8-
using MigrationTools.Tools.Interfaces;
1+
using MigrationTools.Tools.Interfaces;
92

103
namespace MigrationTools.Tools.Shadows
114
{
125
public class MockStringManipulatorTool : IStringManipulatorTool
136
{
14-
public void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fieldItem)
7+
public string? ProcessString(string? value)
158
{
169
throw new NotImplementedException();
1710
}

‎src/MigrationTools.Tests/ProcessorEnrichers/StringManipulatorEnricherTests.cs‎

Lines changed: 95 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,8 @@
22
using Microsoft.Extensions.DependencyInjection;
33
using Microsoft.VisualStudio.TestTools.UnitTesting;
44
using MigrationTools.DataContracts;
5-
using MigrationTools.Endpoints;
6-
using MigrationTools.Processors;
7-
using MigrationTools.Tests;
8-
using Microsoft.Extensions.Options;
9-
using MigrationTools.Tools;
105
using MigrationTools.Shadows;
6+
using MigrationTools.Tools;
117

128
namespace MigrationTools.ProcessorEnrichers.Tests
139
{
@@ -21,7 +17,7 @@ public void StringManipulatorTool_ConfigureTest()
2117
var options = new StringManipulatorToolOptions();
2218
options.Enabled = true;
2319
options.MaxStringLength = 10;
24-
options.Manipulators = new List<RegexStringManipulator>
20+
options.Manipulators = new List<RegexStringManipulator>
2521
{
2622
new RegexStringManipulator
2723
{
@@ -40,10 +36,10 @@ public void StringManipulatorTool_ConfigureTest()
4036
[TestMethod(), TestCategory("L1")]
4137
public void StringManipulatorTool_RegexTest()
4238
{
43-
var options = new StringManipulatorToolOptions();
39+
var options = new StringManipulatorToolOptions();
4440
options.Enabled = true;
45-
options.MaxStringLength = 10;
46-
options.Manipulators = new List<RegexStringManipulator>
41+
options.MaxStringLength = 10;
42+
options.Manipulators = new List<RegexStringManipulator>
4743
{
4844
new RegexStringManipulator
4945
{
@@ -56,40 +52,24 @@ public void StringManipulatorTool_RegexTest()
5652

5753
var x = GetStringManipulatorTool(options);
5854

59-
var fieldItem = new FieldItem
60-
{
61-
FieldType = "String",
62-
internalObject = null,
63-
ReferenceName = "Custom.Test",
64-
Name = "Test",
65-
Value = "Test"
66-
};
67-
68-
x.ProcessorExecutionWithFieldItem(null, fieldItem);
55+
string value = "Test";
56+
string? newValue = x.ProcessString(value);
6957

70-
Assert.AreEqual("Test 2", fieldItem.Value);
58+
Assert.AreEqual("Test 2", newValue);
7159
}
7260

7361
[TestMethod(), TestCategory("L1")]
7462
public void StringManipulatorTool_LengthShorterThanMaxTest()
7563
{
7664
var options = new StringManipulatorToolOptions();
7765
options.Enabled = true;
78-
options.MaxStringLength = 10;
66+
options.MaxStringLength = 10;
7967
var x = GetStringManipulatorTool(options);
8068

81-
var fieldItem = new FieldItem
82-
{
83-
FieldType = "String",
84-
internalObject = null,
85-
ReferenceName = "Custom.Test",
86-
Name = "Test",
87-
Value = "Test"
88-
};
89-
90-
x.ProcessorExecutionWithFieldItem(null, fieldItem);
69+
string value = "Test";
70+
string? newValue = x.ProcessString(value);
9171

92-
Assert.AreEqual(4, fieldItem.Value.ToString().Length);
72+
Assert.AreEqual(4, newValue.Length);
9373
}
9474

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

103-
var fieldItem = new FieldItem
104-
{
105-
FieldType = "String",
106-
internalObject = null,
107-
ReferenceName = "Custom.Test",
108-
Name = "Test",
109-
Value = "Test Test Test Test Test Test Test Test Test Test Test Test Test"
110-
};
83+
string value = "Test Test Test Test Test Test Test Test Test Test Test Test Test";
84+
string? newValue = x.ProcessString(value);
11185

112-
x.ProcessorExecutionWithFieldItem(null, fieldItem);
86+
Assert.AreEqual(10, newValue.Length);
87+
}
11388

114-
Assert.AreEqual(10, fieldItem.Value.ToString().Length);
89+
[DataTestMethod(), TestCategory("L1")]
90+
[DataRow(null, null)]
91+
[DataRow("", "")]
92+
[DataRow("lorem", "lorem")]
93+
public void StringManipulatorTool_Disabled(string? value, string? expected)
94+
{
95+
var options = new StringManipulatorToolOptions();
96+
options.Enabled = false;
97+
options.MaxStringLength = 15;
98+
options.Manipulators = new List<RegexStringManipulator>
99+
{
100+
new RegexStringManipulator
101+
{
102+
Enabled = true,
103+
Pattern = "(^.*$)",
104+
Replacement = "$1 $1",
105+
Description = "Test"
106+
}
107+
};
108+
var x = GetStringManipulatorTool(options);
109+
110+
string? newValue = x.ProcessString(value);
111+
Assert.AreEqual(expected, newValue);
115112
}
116113

117-
private static StringManipulatorTool GetStringManipulatorTool()
114+
[DataTestMethod(), TestCategory("L1")]
115+
[DataRow(null, null)]
116+
[DataRow("", " ")]
117+
[DataRow("lorem", "lorem lorem")]
118+
[DataRow("lorem ipsum", "lorem ipsum lor")]
119+
public void StringManipulatorTool_Process(string? value, string? expected)
118120
{
119-
var options = new StringManipulatorToolOptions();
120-
return GetStringManipulatorTool(options);
121+
var options = new StringManipulatorToolOptions();
122+
options.Enabled = true;
123+
options.MaxStringLength = 15;
124+
options.Manipulators = new List<RegexStringManipulator>
125+
{
126+
new RegexStringManipulator
127+
{
128+
Enabled = true,
129+
Pattern = "(^.*$)",
130+
Replacement = "$1 $1",
131+
Description = "Test"
132+
}
133+
};
134+
var x = GetStringManipulatorTool(options);
135+
136+
string? newValue = x.ProcessString(value);
137+
Assert.AreEqual(expected, newValue);
138+
}
139+
140+
[DataTestMethod(), TestCategory("L1")]
141+
[DataRow(null, null)]
142+
[DataRow("", " 1 2")]
143+
[DataRow("lorem", "lorem 1 2")]
144+
public void StringManipulatorTool_MultipleManipulators(string? value, string? expected)
145+
{
146+
var options = new StringManipulatorToolOptions();
147+
options.Enabled = true;
148+
options.MaxStringLength = 15;
149+
options.Manipulators = new List<RegexStringManipulator>
150+
{
151+
new RegexStringManipulator
152+
{
153+
Enabled = true,
154+
Pattern = "(^.*$)",
155+
Replacement = "$1 1",
156+
Description = "Add 1"
157+
},
158+
new RegexStringManipulator
159+
{
160+
Enabled = true,
161+
Pattern = "(^.*$)",
162+
Replacement = "$1 2",
163+
Description = "Add 2"
164+
}
165+
};
166+
var x = GetStringManipulatorTool(options);
167+
168+
string? newValue = x.ProcessString(value);
169+
Assert.AreEqual(expected, newValue);
121170
}
122171

123172
private static StringManipulatorTool GetStringManipulatorTool(StringManipulatorToolOptions options)
@@ -134,4 +183,4 @@ private static StringManipulatorTool GetStringManipulatorTool(StringManipulatorT
134183
return services.BuildServiceProvider().GetService<StringManipulatorTool>();
135184
}
136185
}
137-
}
186+
}

‎src/MigrationTools/Tools/Interfaces/IStringManipulatorTool.cs‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,7 @@
1-
using System;
2-
using System.Collections.Generic;
3-
using System.Text;
4-
using MigrationTools.DataContracts;
5-
using MigrationTools.Processors.Infrastructure;
6-
7-
namespace MigrationTools.Tools.Interfaces
1+
namespace MigrationTools.Tools.Interfaces
82
{
93
public interface IStringManipulatorTool
104
{
11-
void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fieldItem);
5+
string? ProcessString(string? value);
126
}
137
}

‎src/MigrationTools/Tools/StringManipulatorTool.cs‎

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,8 @@
11
using System;
22
using System.Collections.Generic;
3-
using System.Text;
43
using System.Text.RegularExpressions;
54
using Microsoft.Extensions.Logging;
65
using Microsoft.Extensions.Options;
7-
using MigrationTools.DataContracts;
8-
using MigrationTools.Enrichers;
9-
using MigrationTools.Processors;
10-
using MigrationTools.Processors.Infrastructure;
116
using MigrationTools.Tools.Infrastructure;
127
using MigrationTools.Tools.Interfaces;
138

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

22-
public StringManipulatorTool(IOptions<StringManipulatorToolOptions> options, IServiceProvider services, ILogger<StringManipulatorTool> logger, ITelemetryLogger telemetryLogger)
23-
: base(options, services, logger, telemetryLogger)
17+
public StringManipulatorTool(
18+
IOptions<StringManipulatorToolOptions> options,
19+
IServiceProvider services,
20+
ILogger<StringManipulatorTool> logger,
21+
ITelemetryLogger telemetryLogger)
22+
: base(options, services, logger, telemetryLogger)
2423
{
2524
}
2625

27-
public void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fieldItem)
26+
public string? ProcessString(string? value)
2827
{
29-
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem", GetType().Name);
28+
const string logPrefix = nameof(StringManipulatorTool) + "::" + nameof(ProcessString);
29+
Log.LogDebug(logPrefix);
30+
31+
string result = value;
3032
if (!Options.Enabled)
3133
{
32-
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Disabled", GetType().Name);
33-
return;
34+
Log.LogDebug(logPrefix + "::Disabled");
35+
return result;
3436
}
35-
if (fieldItem.FieldType == "String" && fieldItem.Value != null)
37+
if (value is not null)
3638
{
3739
if (!HasManipulators())
3840
{
@@ -42,45 +44,43 @@ public void ProcessorExecutionWithFieldItem(IProcessor processor, FieldItem fiel
4244
{
4345
if (manipulator.Enabled)
4446
{
45-
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Running::{Description} with {pattern}", GetType().Name, manipulator.Description, manipulator.Pattern);
46-
var originalValue = fieldItem.Value;
47-
fieldItem.Value = Regex.Replace((string)fieldItem.Value, manipulator.Pattern, manipulator.Replacement);
48-
Log.LogTrace("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Running::{Description}::Original::{@OriginalValue}", GetType().Name, manipulator.Description, originalValue);
49-
Log.LogTrace("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Running::{Description}::New::{@fieldItemValue}", GetType().Name, manipulator.Description, fieldItem.Value);
47+
Log.LogDebug(logPrefix + "::Running::{Description} with {pattern}", manipulator.Description, manipulator.Pattern);
48+
string oldValue = result;
49+
result = Regex.Replace(result, manipulator.Pattern, manipulator.Replacement);
50+
Log.LogTrace(logPrefix + "::Running::{Description}::Original::{@oldValue}", manipulator.Description, oldValue);
51+
Log.LogTrace(logPrefix + "::Running::{Description}::New::{@newValue}", manipulator.Description, result);
5052
}
5153
else
5254
{
53-
Log.LogDebug("{WorkItemProcessorEnricher}::ProcessorExecutionWithFieldItem::Disabled::{Description}", GetType().Name, manipulator.Description);
55+
Log.LogDebug(logPrefix + "::Disabled::{Description}", manipulator.Description);
5456
}
5557
}
5658

57-
if (HasStringTooLong(fieldItem))
59+
if (IsStringTooLong(result))
5860
{
59-
fieldItem.Value = fieldItem.Value.ToString().Substring(0, Math.Min(fieldItem.Value.ToString().Length, Options.MaxStringLength));
61+
result = result.Substring(0, Options.MaxStringLength);
6062
}
6163
}
62-
64+
return result;
6365
}
6466

6567
private void AddDefaultManipulator()
6668
{
67-
if (Options.Manipulators == null)
69+
if (Options.Manipulators is null)
6870
{
6971
Options.Manipulators = new List<RegexStringManipulator>();
7072
}
71-
Options.Manipulators.Add(new RegexStringManipulator() { Enabled = true, Description = "Default: Removes invalid chars!", Pattern = "[^( -~)\n\r\t]+", Replacement = "" });
73+
Options.Manipulators.Add(new RegexStringManipulator()
74+
{
75+
Enabled = true,
76+
Description = "Default: Removes invalid chars!",
77+
Pattern = "[^( -~)\n\r\t]+",
78+
Replacement = ""
79+
});
7280
}
7381

74-
private bool HasStringTooLong(FieldItem fieldItem)
75-
{
76-
return fieldItem.Value.ToString().Length > 0 && fieldItem.Value.ToString().Length > Options.MaxStringLength;
77-
}
82+
private bool IsStringTooLong(string value) => (value.Length > 0) && (value.Length > Options.MaxStringLength);
7883

79-
private bool HasManipulators()
80-
{
81-
return Options.Manipulators != null && Options.Manipulators.Count > 0;
82-
}
84+
private bool HasManipulators() => Options.Manipulators?.Count > 0;
8385
}
84-
8586
}
86-

0 commit comments

Comments
 (0)