From f7caff8ad3f7dabaab1d76884db8396e06a79755 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:14:09 -0700 Subject: [PATCH] fix: Advanced Search results came back in a different order every search (#2626) ## Summary Advanced Search results came back in a different order every search, whether or not a sort was chosen. Follow-up to #2625, where it was noticed while testing the property test. ## Why Results arrive from the search workers in whatever order they finished. Each sort the gump offers compares a single key and `Array.Sort` is not stable, so equal keys, which is most rows under type or map, landed in a different order every run; with no sort chosen the list was arrival order outright. The range sort also answered 0 for any two results off the viewer's map, so those shuffled as well. ## What changed - The five comparers share one base class whose `Compare` applies the chosen key and then a fixed tie-break, serial ascending regardless of direction. The direction is about the key; a fixed order among equal keys is what keeps two searches identical. - The collected result list is put in serial order before the gump sees it, so an unsorted search is deterministic too. That sort runs on the thread-pool work item that already assembles the list and reads only the result records. - No change to which key each sort uses or to what ascending and descending mean. ## Test plan - [x] New `AdvancedSearchResultOrderTests`: six equal-keyed results offered in two arrival orders to every comparer in both directions, asserting the same sequence and serial order; the range comparer on off-map results; reverse flips the key but not the tie-break. - [x] `UOContent.Tests`: 873 passed, 0 failed - [x] `Server.Tests`: 869 passed, 0 failed - [ ] In game: run the same search twice with each sort and with none; the lists match. --- .../AdvancedSearchResultOrderTests.cs | 132 +++++++++++++ .../Advanced Search/AdvancedSearchGump.cs | 2 + .../AdvancedSearchResultComparers.cs | 185 ++++++++++-------- 3 files changed, 240 insertions(+), 79 deletions(-) create mode 100644 Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchResultOrderTests.cs diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchResultOrderTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchResultOrderTests.cs new file mode 100644 index 000000000..f6921de9e --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchResultOrderTests.cs @@ -0,0 +1,132 @@ +using System; +using System.Collections.Generic; +using Server; +using Server.Engines.AdvancedSearch; +using Server.Items; +using Xunit; + +namespace UOContent.Tests; + +// Every sort must give the same sequence from any arrival order: key first, then serial. +[Collection("Sequential UOContent Tests")] +public class AdvancedSearchResultOrderTests : IDisposable +{ + private readonly List _items = []; + + public void Dispose() + { + for (var i = 0; i < _items.Count; i++) + { + _items[i].Delete(); + } + + _items.Clear(); + } + + private AdvancedSearchResult Result(Item item, string name = "same", Map map = null) + { + _items.Add(item); + return new AdvancedSearchResult(name, item.GetType(), item.Location, map ?? Map.Felucca, null) { Entity = item }; + } + + private AdvancedSearchResult[] EqualKeyed() + { + var results = new AdvancedSearchResult[6]; + + for (var i = 0; i < results.Length; i++) + { + results[i] = Result(new Item(0x1)); + } + + return results; + } + + private static AdvancedSearchResult[] Shuffled(AdvancedSearchResult[] inOrder, params int[] permutation) + { + var shuffled = new AdvancedSearchResult[inOrder.Length]; + + for (var i = 0; i < permutation.Length; i++) + { + shuffled[i] = inOrder[permutation[i]]; + } + + return shuffled; + } + + private static void AssertSameSequenceFromAnyArrival( + AdvancedSearchResult[] inOrder, + IComparer comparer + ) + { + var first = Shuffled(inOrder, 3, 0, 5, 1, 4, 2); + var second = Shuffled(inOrder, 5, 4, 3, 2, 1, 0); + + Array.Sort(first, comparer); + Array.Sort(second, comparer); + + Assert.Equal(first, second); + } + + public static IEnumerable Comparers() + { + yield return [AdvancedSearchResultSerialComparer.Instance]; + yield return [AdvancedSearchResultTypeComparer.Instance]; + yield return [AdvancedSearchResultTypeComparer.InstanceReverse]; + yield return [AdvancedSearchResultNameComparer.Instance]; + yield return [AdvancedSearchResultNameComparer.InstanceReverse]; + yield return [AdvancedSearchResultMapComparer.Instance]; + yield return [AdvancedSearchResultMapComparer.InstanceReverse]; + yield return [AdvancedSearchResultSelectedComparer.Instance]; + yield return [AdvancedSearchResultSelectedComparer.InstanceReverse]; + } + + [Theory] + [MemberData(nameof(Comparers))] + public void EqualKeysOrderBySerialFromAnyArrivalOrder(IComparer comparer) + { + var inOrder = EqualKeyed(); + + AssertSameSequenceFromAnyArrival(inOrder, comparer); + + var sorted = Shuffled(inOrder, 3, 0, 5, 1, 4, 2); + Array.Sort(sorted, comparer); + + Assert.Equal(inOrder, sorted); + } + + // Off-map results compare equal on range. + [Fact] + public void RangeComparerOrdersOffMapResultsBySerial() + { + var from = new Mobile(); + from.MoveToWorld(new Point3D(1000, 1000, 0), Map.Trammel); + + try + { + var inOrder = EqualKeyed(); + + AssertSameSequenceFromAnyArrival(inOrder, new AdvancedSearchRangeComparer(from)); + AssertSameSequenceFromAnyArrival(inOrder, new AdvancedSearchRangeComparer(from, true)); + } + finally + { + from.Delete(); + } + } + + [Fact] + public void ReverseFlipsTheKeyNotTheTieBreak() + { + var a1 = Result(new Item(0x1), "alpha"); + var a2 = Result(new Item(0x1), "alpha"); + var b1 = Result(new Item(0x1), "beta"); + + var results = new[] { b1, a2, a1 }; + + Array.Sort(results, AdvancedSearchResultNameComparer.Instance); + Assert.Equal([a1, a2, b1], results); + + Array.Sort(results, AdvancedSearchResultNameComparer.InstanceReverse); + Assert.Equal([b1, a1, a2], results); + } +} diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs index 96c357fd0..e788128f2 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs @@ -819,6 +819,8 @@ public class AdvancedSearchGump : Gump } } + resultsList.Sort(AdvancedSearchResultSerialComparer.Instance); + SearchResults = resultsList.ToArray(); // Force the GC to collect the results diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchResultComparers.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchResultComparers.cs index 8842b8f80..1a455064d 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchResultComparers.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchResultComparers.cs @@ -2,86 +2,118 @@ using System.Collections.Generic; namespace Server.Engines.AdvancedSearch; -public class AdvancedSearchResultTypeComparer : IComparer +// Results arrive in worker-finish order, so every sort ends in the same serial tie-break. +public abstract class AdvancedSearchResultComparer : IComparer { - public static readonly AdvancedSearchResultTypeComparer Instance = new(); - public static readonly AdvancedSearchResultTypeComparer InstanceReverse = new(true); + protected readonly bool Reverse; - private readonly bool _reverse; - - public AdvancedSearchResultTypeComparer(bool reverse = false) => _reverse = reverse; + protected AdvancedSearchResultComparer(bool reverse) => Reverse = reverse; public int Compare(AdvancedSearchResult x, AdvancedSearchResult y) { - var a = x?.Entity?.GetType().Name; - var b = y?.Entity?.GetType().Name; - - return _reverse ? b.InsensitiveCompare(a) : a.InsensitiveCompare(b); - } -} - -public class AdvancedSearchResultNameComparer : IComparer -{ - public static readonly AdvancedSearchResultNameComparer Instance = new(); - public static readonly AdvancedSearchResultNameComparer InstanceReverse = new(true); - - private readonly bool _reverse; - - public AdvancedSearchResultNameComparer(bool reverse = false) => _reverse = reverse; - - public int Compare(AdvancedSearchResult x, AdvancedSearchResult y) - { - var a = x?.Name; - var b = y?.Name; - - return _reverse ? b.InsensitiveCompare(a) : a.InsensitiveCompare(b); - } -} - -public class AdvancedSearchResultMapComparer : IComparer -{ - public static readonly AdvancedSearchResultMapComparer Instance = new(); - public static readonly AdvancedSearchResultMapComparer InstanceReverse = new(true); - - private readonly bool _reverse; - - public AdvancedSearchResultMapComparer(bool reverse = false) => _reverse = reverse; - - public int Compare(AdvancedSearchResult x, AdvancedSearchResult y) - { - var a = x?.Map?.MapID ?? -1; - var b = y?.Map?.MapID ?? -1; - - return _reverse ? b.CompareTo(a) : a.CompareTo(b); - } -} - -public class AdvancedSearchRangeComparer : IComparer -{ - private readonly bool _reverse; - private readonly Mobile _from; - - public AdvancedSearchRangeComparer(Mobile from, bool reverse = false) - { - _from = from; - _reverse = reverse; - } - - public int Compare(AdvancedSearchResult x, AdvancedSearchResult y) - { - if (_from == null || x == null && y == null) + if (ReferenceEquals(x, y)) { return 0; } if (x == null) { - return _reverse ? 1 : -1; + return -1; } if (y == null) { - return _reverse ? -1 : 1; + return 1; + } + + var c = CompareKey(x, y); + + return c != 0 ? c : CompareSerial(x, y); + } + + protected abstract int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y); + + // Always ascending; the direction applies to the key only. + public static int CompareSerial(AdvancedSearchResult x, AdvancedSearchResult y) + { + var a = x.Entity?.Serial ?? Serial.Zero; + var b = y.Entity?.Serial ?? Serial.Zero; + + return a.CompareTo(b); + } +} + +public sealed class AdvancedSearchResultSerialComparer : AdvancedSearchResultComparer +{ + public static readonly AdvancedSearchResultSerialComparer Instance = new(); + + private AdvancedSearchResultSerialComparer() : base(false) + { + } + + protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) => 0; +} + +public sealed class AdvancedSearchResultTypeComparer : AdvancedSearchResultComparer +{ + public static readonly AdvancedSearchResultTypeComparer Instance = new(); + public static readonly AdvancedSearchResultTypeComparer InstanceReverse = new(true); + + public AdvancedSearchResultTypeComparer(bool reverse = false) : base(reverse) + { + } + + protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) + { + var a = x.Entity?.GetType().Name; + var b = y.Entity?.GetType().Name; + + return Reverse ? b.InsensitiveCompare(a) : a.InsensitiveCompare(b); + } +} + +public sealed class AdvancedSearchResultNameComparer : AdvancedSearchResultComparer +{ + public static readonly AdvancedSearchResultNameComparer Instance = new(); + public static readonly AdvancedSearchResultNameComparer InstanceReverse = new(true); + + public AdvancedSearchResultNameComparer(bool reverse = false) : base(reverse) + { + } + + protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) => + Reverse ? y.Name.InsensitiveCompare(x.Name) : x.Name.InsensitiveCompare(y.Name); +} + +public sealed class AdvancedSearchResultMapComparer : AdvancedSearchResultComparer +{ + public static readonly AdvancedSearchResultMapComparer Instance = new(); + public static readonly AdvancedSearchResultMapComparer InstanceReverse = new(true); + + public AdvancedSearchResultMapComparer(bool reverse = false) : base(reverse) + { + } + + protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) + { + var a = x.Map?.MapID ?? -1; + var b = y.Map?.MapID ?? -1; + + return Reverse ? b.CompareTo(a) : a.CompareTo(b); + } +} + +public sealed class AdvancedSearchRangeComparer : AdvancedSearchResultComparer +{ + private readonly Mobile _from; + + public AdvancedSearchRangeComparer(Mobile from, bool reverse = false) : base(reverse) => _from = from; + + protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) + { + if (_from == null) + { + return 0; } var fromMap = _from.Map; @@ -93,36 +125,31 @@ public class AdvancedSearchRangeComparer : IComparer if (x.Map == fromMap && y.Map != fromMap) { - return _reverse ? 1 : -1; + return Reverse ? 1 : -1; } if (x.Map != fromMap && y.Map == fromMap) { - return _reverse ? -1 : 1; + return Reverse ? -1 : 1; } var xDist = _from.GetDistanceToSqrt(x.Location); var yDist = _from.GetDistanceToSqrt(y.Location); - return _reverse ? yDist.CompareTo(xDist) : xDist.CompareTo(yDist); + return Reverse ? yDist.CompareTo(xDist) : xDist.CompareTo(yDist); } } -public class AdvancedSearchResultSelectedComparer : IComparer +public sealed class AdvancedSearchResultSelectedComparer : AdvancedSearchResultComparer { public static readonly AdvancedSearchResultSelectedComparer Instance = new(); public static readonly AdvancedSearchResultSelectedComparer InstanceReverse = new(true); - private readonly bool _reverse; - - public AdvancedSearchResultSelectedComparer(bool reverse = false) => _reverse = reverse; - - public int Compare(AdvancedSearchResult x, AdvancedSearchResult y) + public AdvancedSearchResultSelectedComparer(bool reverse = false) : base(reverse) { - var a = x?.Selected ?? false; - var b = y?.Selected ?? false; - - // True then false, which is 1 then 0, so the comparison is reverse of integers - return _reverse ? a.CompareTo(b) : b.CompareTo(a); } + + // True then false, which is 1 then 0, so the comparison is reverse of integers + protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) => + Reverse ? x.Selected.CompareTo(y.Selected) : y.Selected.CompareTo(x.Selected); }