diff --git a/README.md b/README.md index 5a67fef..0cdb049 100644 --- a/README.md +++ b/README.md @@ -1200,6 +1200,11 @@ var dto = mapper.MapWithConvention( NamingConvention.PascalCase); ``` +The convention pass skips a member the mapper binds, or the configuration ignores or resolves with +`MapFrom`. Any other member is filled while it still holds the value a new `UserDto` starts with, so +what an `AfterMap` set is kept. A member initialised to a new instance, such as `Tags = new()`, is +always filled, because every destination starts with a different instance. + ### Check Name Matching ```csharp @@ -1210,6 +1215,10 @@ bool match = NamingConvention.NamesMatch( // Result: true ``` +Matching compares the letters and digits and ignores case and where the words break, so `user_id` +matches `UserID` and `address_line_1` matches `AddressLine1`. When a name is converted, an acronym +stays one word: `HTTPServerID` becomes `http_server_id`. + ### Real-World Example: External API Integration ```csharp diff --git a/changelog.d/naming-conventions-word-splitting.fixed.md b/changelog.d/naming-conventions-word-splitting.fixed.md new file mode 100644 index 0000000..6b69462 --- /dev/null +++ b/changelog.d/naming-conventions-word-splitting.fixed.md @@ -0,0 +1,11 @@ +- `Mapsicle.NamingConventions`: an acronym is one word, so `HTTPServerID` converts to + `http_server_id` instead of `h_t_t_p_server_i_d`, and `user_id` fills `UserID`. +- `Mapsicle.NamingConventions`: names match on their letters and digits wherever the words break, + so `address_line_1` fills `AddressLine1`. Non-ASCII letters are part of a word, so `straße_name` + fills `StraßeName`. +- `Mapsicle.NamingConventions`: the `IMapper` overload of `MapWithConvention` fills a member that + still holds its initial value. It used to test for `default(T)`, so a `string` initialised to + `""`, a `bool` initialised to `true` or a list initialised to `new()` was never filled. +- `Mapsicle.NamingConventions`: the same overload leaves a member the mapper binds or a `MapFrom` + resolves. A `MapFrom` that returned the default value used to be overwritten by the convention + match. diff --git a/src/Mapsicle.NamingConventions/NamingConvention.cs b/src/Mapsicle.NamingConventions/NamingConvention.cs index d693afd..dcc61cb 100644 --- a/src/Mapsicle.NamingConventions/NamingConvention.cs +++ b/src/Mapsicle.NamingConventions/NamingConvention.cs @@ -9,11 +9,14 @@ namespace Mapsicle.NamingConventions /// public abstract class NamingConvention { - // Compiled regex for better performance - splits on word boundaries - // Matches sequences like "User", "Name", "ID", "XMLParser", etc. + // The capitalised-word alternative used to come first and accept a lone capital, so the + // acronym alternative never ran and "HTTPServerID" split into H, T, T, P, Server, I, D. The + // classes were ASCII only, so "StraßeName" lost its ß and split around the gap. /// Splits an identifier into words at case changes and separators. protected static readonly Regex WordBoundaryRegex = new( - @"([A-Z][a-z0-9]*|[A-Z]+(?=[A-Z][a-z]|$)|[a-z0-9]+)", + @"[\p{Lu}\p{Lt}]+(?>\p{Nd}*)(?![\p{Ll}\p{Lo}\p{Lm}\p{M}])" + + @"|[\p{Lu}\p{Lt}][\p{Ll}\p{Lo}\p{Lm}\p{M}\p{Nd}]*" + + @"|[\p{Ll}\p{Lo}\p{Lm}\p{M}\p{Nd}]+", RegexOptions.Compiled); /// @@ -57,6 +60,17 @@ public abstract class NamingConvention /// public abstract string FromWords(string[] words); + internal static string[] SplitAtCaseChanges(string name) + { + var matches = WordBoundaryRegex.Matches(name); + var words = new string[matches.Count]; + for (int i = 0; i < matches.Count; i++) + { + words[i] = matches[i].Value; + } + return words; + } + /// /// Converts a name from one convention to another. /// @@ -76,17 +90,13 @@ public static bool NamesMatch(string sourceName, NamingConvention sourceConventi if (string.IsNullOrEmpty(sourceName) || string.IsNullOrEmpty(destName)) return false; - var sourceWords = sourceConvention.ToWords(sourceName); - var destWords = destConvention.ToWords(destName); - - if (sourceWords.Length != destWords.Length) return false; - - for (int i = 0; i < sourceWords.Length; i++) - { - if (!string.Equals(sourceWords[i], destWords[i], StringComparison.OrdinalIgnoreCase)) - return false; - } - return true; + // Compared word by word, this refused address_line_1 against AddressLine1, because one + // side has three words and the other two. Where a word ends is a property of the + // convention, not of the name, so only the letters are compared. + return string.Equals( + string.Concat(sourceConvention.ToWords(sourceName)), + string.Concat(destConvention.ToWords(destName)), + StringComparison.OrdinalIgnoreCase); } } @@ -96,14 +106,7 @@ internal class PascalCaseConvention : NamingConvention public override string[] ToWords(string name) { - // Use compiled regex for better performance - var matches = WordBoundaryRegex.Matches(name); - var words = new string[matches.Count]; - for (int i = 0; i < matches.Count; i++) - { - words[i] = matches[i].Value; - } - return words; + return SplitAtCaseChanges(name); } public override string FromWords(string[] words) @@ -126,14 +129,7 @@ internal class CamelCaseConvention : NamingConvention public override string[] ToWords(string name) { - // Use compiled regex for better performance - var matches = WordBoundaryRegex.Matches(name); - var words = new string[matches.Count]; - for (int i = 0; i < matches.Count; i++) - { - words[i] = matches[i].Value; - } - return words; + return SplitAtCaseChanges(name); } public override string FromWords(string[] words) diff --git a/src/Mapsicle.NamingConventions/NamingConventionExtensions.cs b/src/Mapsicle.NamingConventions/NamingConventionExtensions.cs index 1fe38bd..d96ef0c 100644 --- a/src/Mapsicle.NamingConventions/NamingConventionExtensions.cs +++ b/src/Mapsicle.NamingConventions/NamingConventionExtensions.cs @@ -13,6 +13,8 @@ namespace Mapsicle.NamingConventions public static class NamingConventionExtensions { private static readonly ConcurrentDictionary<(Type, Type, string, string), Dictionary> _propertyMappingCache = new(); + private static readonly ConcurrentDictionary<(Type, Type), HashSet> _boundMemberCache = new(); + private static readonly ConcurrentDictionary> _initialValueCache = new(); /// /// Creates a mapper that applies naming conventions when matching properties. @@ -100,26 +102,29 @@ public static class NamingConventionExtensions var sourceType = typeof(TSource); var destType = typeof(TDest); - // A member the configuration ignores is left at its default by the mapper, which is - // exactly what this pass reads as "not mapped yet", so it used to fill it anyway. + // "Not mapped yet" used to mean equal to default(T) and nothing else. A string initialised + // to "" or a list initialised to a new instance was therefore never filled, and a member + // the configuration ignored or resolved to its default was filled over. The mapper and + // its configuration are asked first, and the value only decides what neither accounts for. var typeMap = (mapper as FluentMapper)?.Configuration.GetTypeMap(sourceType, destType); + var bound = _boundMemberCache.GetOrAdd( + (source.GetType(), dest.GetType()), + pair => new HashSet(Mapper.GetBoundMembers(pair.Item1, pair.Item2).Keys, StringComparer.OrdinalIgnoreCase)); + var initialValues = _initialValueCache.GetOrAdd(destType, _ => ReadInitialValues(new TDest())); foreach (var mapping in propertyMappings) { - if (typeMap?.IsIgnored(mapping.Value) == true) continue; + if (bound.Contains(mapping.Value)) continue; + if (typeMap?.IsIgnored(mapping.Value) == true || typeMap?.HasCustomMapping(mapping.Value) == true) continue; var sourceProp = sourceType.GetProperty(mapping.Key); var destProp = destType.GetProperty(mapping.Value); if (sourceProp?.GetGetMethod() != null && destProp?.CanWrite == true) { - // Only set if dest property is default/null (wasn't mapped by standard mapper) - var currentValue = destProp.GetValue(dest); - var defaultValue = destProp.PropertyType.IsValueType - ? Activator.CreateInstance(destProp.PropertyType) - : null; + initialValues.TryGetValue(destProp.Name, out var initialValue); - if (Equals(currentValue, defaultValue)) + if (StillUnset(destProp.GetValue(dest), initialValue)) { try { @@ -204,7 +209,37 @@ public static string ConvertName(this string name, NamingConvention from, Naming /// /// Clears the property mapping cache. Useful for testing scenarios. /// - public static void ClearMappingCache() => _propertyMappingCache.Clear(); + public static void ClearMappingCache() + { + _propertyMappingCache.Clear(); + _boundMemberCache.Clear(); + _initialValueCache.Clear(); + } + + // A hook such as AfterMap can set a member nothing else accounts for, and a value that moved + // off its initial one is the only sign of it. An initializer that builds a new instance + // gives every destination a different reference, so there the comparison says nothing and + // the member is filled. + private static bool StillUnset(object? current, object? initial) => + Equals(current, initial) || initial is not (null or string or ValueType); + + private static Dictionary ReadInitialValues(object fresh) + { + var values = new Dictionary(); + foreach (var prop in fresh.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance)) + { + if (prop.GetGetMethod() == null || prop.GetIndexParameters().Length > 0) continue; + try + { + values[prop.Name] = prop.GetValue(fresh); + } + catch (Exception) + { + values[prop.Name] = null; + } + } + return values; + } private static object? ConvertValue(object value, Type targetType) { diff --git a/src/Mapsicle/Mapsicle.csproj b/src/Mapsicle/Mapsicle.csproj index ecfaa08..236f35f 100644 --- a/src/Mapsicle/Mapsicle.csproj +++ b/src/Mapsicle/Mapsicle.csproj @@ -13,6 +13,7 @@ + diff --git a/tests/Mapsicle.NamingConventions.Tests/NamingConventionTests.cs b/tests/Mapsicle.NamingConventions.Tests/NamingConventionTests.cs index 8d49a42..1235f3c 100644 --- a/tests/Mapsicle.NamingConventions.Tests/NamingConventionTests.cs +++ b/tests/Mapsicle.NamingConventions.Tests/NamingConventionTests.cs @@ -48,8 +48,8 @@ public class NamingConventionTests [Theory] [InlineData("UserName", new[] { "User", "Name" })] [InlineData("FirstName", new[] { "First", "Name" })] - [InlineData("ID", new[] { "I", "D" })] - [InlineData("XMLParser", new[] { "X", "M", "L", "Parser" })] + [InlineData("ID", new[] { "ID" })] + [InlineData("XMLParser", new[] { "XML", "Parser" })] [InlineData("userId", new[] { "user", "Id" })] public void PascalCase_ToWords_SplitsCorrectly(string input, string[] expected) { diff --git a/tests/Mapsicle.NamingConventions.Tests/WordSplittingTests.cs b/tests/Mapsicle.NamingConventions.Tests/WordSplittingTests.cs new file mode 100644 index 0000000..c41e33b --- /dev/null +++ b/tests/Mapsicle.NamingConventions.Tests/WordSplittingTests.cs @@ -0,0 +1,207 @@ +using Mapsicle; +using Mapsicle.Fluent; +using Mapsicle.NamingConventions; +using Xunit; + +namespace Mapsicle.NamingConventions.Tests +{ +#pragma warning disable IDE1006 + public class WsSnake + { + public int user_id { get; set; } + public int http_status { get; set; } + public string address_line_1 { get; set; } = ""; + public string address_line2 { get; set; } = ""; + public string straße_name { get; set; } = ""; + public string display_name { get; set; } = ""; + public bool is_active { get; set; } + } +#pragma warning restore IDE1006 + + public class WsPascal + { + public int UserID { get; set; } + public int HTTPStatus { get; set; } + public string AddressLine1 { get; set; } = ""; + public string AddressLine2 { get; set; } = ""; + public string StraßeName { get; set; } = ""; + } + + public class WsInitialised + { + public string DisplayName { get; set; } = ""; + public bool IsActive { get; set; } = true; + } + +#pragma warning disable IDE1006 + public class WsSnakeTags + { + public System.Collections.Generic.List tag_names { get; set; } = new(); + } +#pragma warning restore IDE1006 + + public class WsPascalTags + { + public System.Collections.Generic.List TagNames { get; set; } = new(); + } + + public class WsUnrelated + { + public int UserKey { get; set; } + public string Street { get; set; } = ""; + } + + [Collection("StaticMapperTests")] + public class WordSplittingTests + { + public WordSplittingTests() + { + Mapper.ClearCache(); + NamingConventionExtensions.ClearMappingCache(); + } + + private static WsSnake Snake() => new WsSnake + { + user_id = 7, + http_status = 200, + address_line_1 = "1 Main St", + address_line2 = "Unit 4", + straße_name = "Hauptstraße", + display_name = "ann", + is_active = false + }; + + [Theory] + [InlineData("ID", new[] { "ID" })] + [InlineData("XMLParser", new[] { "XML", "Parser" })] + [InlineData("HTTPServerID", new[] { "HTTP", "Server", "ID" })] + [InlineData("UserID", new[] { "User", "ID" })] + [InlineData("HTTP2Server", new[] { "HTTP2", "Server" })] + [InlineData("AddressLine1", new[] { "Address", "Line1" })] + [InlineData("StraßeName", new[] { "Straße", "Name" })] + [InlineData("ÉcoleNom", new[] { "École", "Nom" })] + public void PascalCase_ToWords_KeepsAcronymsAndNonAsciiLettersWhole(string input, string[] expected) + { + Assert.Equal(expected, NamingConvention.PascalCase.ToWords(input)); + } + + [Theory] + [InlineData("userID", new[] { "user", "ID" })] + [InlineData("straßeName", new[] { "straße", "Name" })] + public void CamelCase_ToWords_KeepsAcronymsAndNonAsciiLettersWhole(string input, string[] expected) + { + Assert.Equal(expected, NamingConvention.CamelCase.ToWords(input)); + } + + [Theory] + [InlineData("HTTPServerID", "http_server_id")] + [InlineData("UserID", "user_id")] + [InlineData("AddressLine1", "address_line1")] + [InlineData("StraßeName", "straße_name")] + public void Convert_PascalToSnake_KeepsAcronymsWhole(string input, string expected) + { + Assert.Equal(expected, input.ConvertName(NamingConvention.PascalCase, NamingConvention.SnakeCase)); + } + + [Theory] + [InlineData("user_id", "UserID")] + [InlineData("http_status", "HTTPStatus")] + [InlineData("address_line_1", "AddressLine1")] + [InlineData("address_line1", "AddressLine1")] + [InlineData("straße_name", "StraßeName")] + public void NamesMatch_SnakeAndPascal_IgnoresWhereTheWordsBreak(string snake, string pascal) + { + Assert.True(NamingConvention.NamesMatch(snake, NamingConvention.SnakeCase, pascal, NamingConvention.PascalCase)); + } + + [Theory] + [InlineData("user_id", "UserKey")] + [InlineData("address_line_1", "AddressLine2")] + [InlineData("straße_name", "StrasseName")] + public void Control_NamesMatch_StillRefusesDifferentNames(string snake, string pascal) + { + Assert.False(NamingConvention.NamesMatch(snake, NamingConvention.SnakeCase, pascal, NamingConvention.PascalCase)); + } + + [Fact] + public void MapWithConvention_FillsAcronymDigitAndNonAsciiMembers() + { + var dto = Snake().MapWithConvention(NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal(7, dto.UserID); + Assert.Equal(200, dto.HTTPStatus); + Assert.Equal("1 Main St", dto.AddressLine1); + Assert.Equal("Unit 4", dto.AddressLine2); + Assert.Equal("Hauptstraße", dto.StraßeName); + } + + [Fact] + public void Control_MapWithConvention_LeavesUnrelatedMembersAlone() + { + var dto = Snake().MapWithConvention(NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal(0, dto.UserKey); + Assert.Equal("", dto.Street); + } + + [Fact] + public void MapperOverload_FillsAMemberStillAtItsInitialValue() + { + var mapper = new MapperConfiguration(cfg => cfg.CreateMap()).CreateMapper(); + + var dto = mapper.MapWithConvention(Snake(), NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal("ann", dto.DisplayName); + Assert.False(dto.IsActive); + } + + [Fact] + public void Control_MapperOverload_KeepsWhatTheMapperSet() + { + var mapper = new MapperConfiguration(cfg => + cfg.CreateMap() + .ForMember(d => d.DisplayName, o => o.MapFrom(s => "from config"))).CreateMapper(); + + var dto = mapper.MapWithConvention(Snake(), NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal("from config", dto.DisplayName); + } + + [Fact] + public void MapperOverload_KeepsAConfiguredValueThatEqualsTheInitialValue() + { + var mapper = new MapperConfiguration(cfg => + cfg.CreateMap() + .ForMember(d => d.DisplayName, o => o.MapFrom(s => ""))).CreateMapper(); + + var dto = mapper.MapWithConvention(Snake(), NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal("", dto.DisplayName); + Assert.False(dto.IsActive); + } + + [Fact] + public void Control_MapperOverload_KeepsWhatAnAfterMapSet() + { + var mapper = new MapperConfiguration(cfg => + cfg.CreateMap() + .AfterMap((s, d) => d.DisplayName = "from hook")).CreateMapper(); + + var dto = mapper.MapWithConvention(Snake(), NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal("from hook", dto.DisplayName); + Assert.False(dto.IsActive); + } + + [Fact] + public void MapperOverload_FillsAMemberInitialisedToANewInstance() + { + var mapper = new MapperConfiguration(cfg => cfg.CreateMap()).CreateMapper(); + var source = new WsSnakeTags { tag_names = { "a", "b" } }; + + var dto = mapper.MapWithConvention(source, NamingConvention.SnakeCase, NamingConvention.PascalCase)!; + + Assert.Equal(new[] { "a", "b" }, dto.TagNames); + } + } +}