From 629fd43e3811653b8d48ce9e083c9fe714e2c794 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 4 Nov 2022 13:48:30 +0100 Subject: [PATCH] Fixes duplicate tag names. --- .../Tags/TagService.cs | 43 ++++++++--- .../StringExtensions.cs | 32 +++++++++ .../Tags/TagServiceTests.cs | 72 +++++++++++++++++++ .../StringExtensionsTests.cs | 17 +++++ 4 files changed, 153 insertions(+), 11 deletions(-) diff --git a/backend/src/Squidex.Domain.Apps.Entities/Tags/TagService.cs b/backend/src/Squidex.Domain.Apps.Entities/Tags/TagService.cs index c62c97eb4..b33ae7e9d 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Tags/TagService.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Tags/TagService.cs @@ -73,6 +73,7 @@ namespace Squidex.Domain.Apps.Entities.Tags return false; } + // Avoid the normalization of the new name, if the old name does not exist. newName = NormalizeName(newName); if (string.Equals(name, newName, StringComparison.OrdinalIgnoreCase)) @@ -80,15 +81,26 @@ namespace Squidex.Domain.Apps.Entities.Tags return false; } - tag.Value.Name = newName; + if (TryGetTag(newName, out var newTag)) + { + // Merge both tags by adding up the count. + newTag.Info.Count += tag.Info.Count; + + // Remove one of the tags. + Tags.Remove(tag.Id); + } + else + { + tag.Info.Name = newName; + } foreach (var alias in Alias.Where(x => x.Value == name).ToList()) { Alias.Remove(alias.Key); - if (alias.Key != tag.Value.Name) + if (alias.Key != name) { - Alias[alias.Key] = tag.Value.Name; + Alias[alias.Key] = name; } } @@ -127,7 +139,7 @@ namespace Squidex.Domain.Apps.Entities.Tags { if (TryGetTag(name, out var tag)) { - tagIds[name] = tag.Key; + tagIds[name] = tag.Id; } else { @@ -149,9 +161,9 @@ namespace Squidex.Domain.Apps.Entities.Tags foreach (var id in ids) { - if (Tags.TryGetValue(id, out var tagInfo)) + if (Tags.TryGetValue(id, out var tag)) { - tagNames[id] = tagInfo.Name; + tagNames[id] = tag.Name; } } @@ -160,19 +172,28 @@ namespace Squidex.Domain.Apps.Entities.Tags public TagsSet GetTags(long version) { - var clone = Tags.Values.ToDictionary(x => x.Name, x => x.Count); + var clone = new Dictionary(); + + foreach (var tag in Tags.Values) + { + // We have changed the normalization logic, therefore some names are not up to date. + var name = NormalizeName(tag.Name); + + // An old bug could have produced duplicate names. + clone[name] = clone.GetValueOrDefault(name) + tag.Count; + } return new TagsSet(clone, version); } private static string NormalizeName(string name) { - return name.Trim().ToLowerInvariant(); + return name.TrimNonLetterOrDigit().ToLowerInvariant(); } - private bool TryGetTag(string name, out KeyValuePair result) + private bool TryGetTag(string name, out (string Id, Tag Info)result) { - result = default; + result = default!; if (Alias.TryGetValue(name, out var newName)) { @@ -183,7 +204,7 @@ namespace Squidex.Domain.Apps.Entities.Tags if (found.Value != null) { - result = new KeyValuePair(found.Key, found.Value); + result = (found.Key, found.Value); return true; } diff --git a/backend/src/Squidex.Infrastructure/StringExtensions.cs b/backend/src/Squidex.Infrastructure/StringExtensions.cs index a974d4ea9..97ab3ce12 100644 --- a/backend/src/Squidex.Infrastructure/StringExtensions.cs +++ b/backend/src/Squidex.Infrastructure/StringExtensions.cs @@ -82,5 +82,37 @@ namespace Squidex.Infrastructure { return value.ToString("yyyy-MM-ddTHH:mm:ssK", CultureInfo.InvariantCulture); } + + public static string TrimNonLetterOrDigit(this string value) + { + var span = value.AsSpan(); + + while (span.Length > 0) + { + if (char.IsLetterOrDigit(span[0])) + { + break; + } + + span = span[1..]; + } + + while (span.Length > 0) + { + if (char.IsLetterOrDigit(span[^1])) + { + break; + } + + span = span[0..^1]; + } + + if (span.Length == value.Length) + { + return value; + } + + return new string(span); + } } } diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Tags/TagServiceTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Tags/TagServiceTests.cs index 3e1979390..6db90c0a7 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Tags/TagServiceTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Tags/TagServiceTests.cs @@ -125,6 +125,78 @@ namespace Squidex.Domain.Apps.Entities.Tags Assert.Equal(ids_0.Values, ids_1.Values); } + [Fact] + public async Task Should_merge_tags_on_rename() + { + var ids = await sut.GetTagIdsAsync(appId, group, HashSet.Of("tag1", "tag2"), ct); + + await sut.UpdateAsync(appId, group, new Dictionary + { + [ids["tag1"]] = 1, + [ids["tag2"]] = 2 + }, ct); + + await sut.RenameTagAsync(appId, group, "tag2", "tag1", ct); + + var allTags = await sut.GetTagsAsync(appId, group, ct); + + Assert.Equal(new Dictionary + { + ["tag1"] = 3 + }, allTags); + } + + [Fact] + public async Task Should_merge_tags_when_stored_with_duplicate_names() + { + var tags = new TagsExport + { + Tags = new Dictionary + { + ["id1"] = new Tag { Name = "tag1", Count = 10 }, + ["id2"] = new Tag { Name = "tag1", Count = 20 } + }, + Alias = null! + }; + + await sut.RebuildTagsAsync(appId, group, tags, ct); + + var allTags = await sut.GetTagsAsync(appId, group, ct); + + Assert.Equal(new Dictionary + { + ["tag1"] = 30 + }, allTags); + } + + [Fact] + public async Task Should_fix_names_when_stored_with_wrong_names() + { + var tags = new TagsExport + { + Tags = new Dictionary + { + ["id1"] = new Tag { Name = "tag1 ", Count = 10 }, + ["id2"] = new Tag { Name = "tag2,", Count = 20 }, + ["id3"] = new Tag { Name = " tag3,", Count = 30 }, + ["id4"] = new Tag { Name = ",tag4,", Count = 40 } + }, + Alias = null! + }; + + await sut.RebuildTagsAsync(appId, group, tags, ct); + + var allTags = await sut.GetTagsAsync(appId, group, ct); + + Assert.Equal(new Dictionary + { + ["tag1"] = 10, + ["tag2"] = 20, + ["tag3"] = 30, + ["tag4"] = 40 + }, allTags); + } + [Fact] public async Task Should_rebuild_tags() { diff --git a/backend/tests/Squidex.Infrastructure.Tests/StringExtensionsTests.cs b/backend/tests/Squidex.Infrastructure.Tests/StringExtensionsTests.cs index ce042dbcd..17dc38a2a 100644 --- a/backend/tests/Squidex.Infrastructure.Tests/StringExtensionsTests.cs +++ b/backend/tests/Squidex.Infrastructure.Tests/StringExtensionsTests.cs @@ -95,5 +95,22 @@ namespace Squidex.Infrastructure Assert.Equal("0001FF1A2B3C", actual); } + + [Theory] + [InlineData("", "")] + [InlineData(" ", "")] + [InlineData("-", "")] + [InlineData("--", "")] + [InlineData("text1 ", "text1")] + [InlineData("texts-", "texts")] + [InlineData(" 1text", "1text")] + [InlineData("-atext", "atext")] + [InlineData("a-text", "a-text")] + public void Should_trim_non_digits_or_letters(string input, string output) + { + var result = input.TrimNonLetterOrDigit(); + + Assert.Equal(output, result); + } } }