From 1adf282c6b91e3fd480f0a102b9b398cd50d5b7f Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:03:54 -0700 Subject: [PATCH] refactor(advanced-search): reuse thread-safe Server.Types for general property parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server.Types is now thread-safe (ConcurrentDictionary for the IsParsable and Parse-method caches; per-call args array instead of a shared static one), so it can be used off the main thread. AdvancedSearch's CompareValues now delegates its general/reference/IParsable parsing branch to Types instead of comparing against a raw string — so reference types (Poison, entity-by-serial) and unlisted IParsable value types (Guid, ...) now parse into their real type and compare by value. The hot typed paths (numeric/string/bool/TimeSpan/DateTime/ enum) stay span-based and zero-alloc; only this uncommon branch allocates. Fixes: 'Poison = Lethal' (and any reference-type/IParsable property search) previously always returned no-match. Adds Poison + Guid tests. Full suite 534/534. --- .../AdvancedSearchTypesTests.cs | 29 +++++++++++++ .../AdvancedSearchUtilitiesTests.cs | 10 +++++ .../AdvancedSearchUtilities.cs | 14 ++++--- Projects/UOContent/Utilities/Types.cs | 42 ++++++++----------- 4 files changed, 65 insertions(+), 30 deletions(-) create mode 100644 Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs new file mode 100644 index 000000000..60951c7c0 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs @@ -0,0 +1,29 @@ +using Server; +using Server.Engines.AdvancedSearch; +using Xunit; + +namespace UOContent.Tests; + +// Fixture-backed: parsing a reference type through Server.Types (Poison) needs the poison registry +// populated, which requires Core.Expansion (AOS+, set by the fixture) and PoisonKinds.Configure(). +[Collection("Sequential UOContent Tests")] +public class AdvancedSearchTypesTests +{ + [Fact] + public void CompareValues_Poison_ReferenceTypeParsedViaTypes() + { + PoisonKinds.Configure(); // idempotent; registers Lesser..Lethal now that Core.Expansion is set + + // Poison is a reference type implementing ISpanParsable; it can't use the compile-time span + // path and routes through the shared Server.Types converter. Poison.Parse returns the + // registered singleton, so "= Lethal" is a reference-equality match — this is the case that + // previously compared a Poison against the raw string and always failed. + var prop = Poison.Lethal; + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(Poison), prop, "Lethal", "=")); + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(Poison), prop, "Lesser", "=")); + + var ex = Record.Exception(() => + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(Poison), prop, "notapoison", "="))); + Assert.Null(ex); + } +} diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs index f3db9b6f7..4c0650e48 100644 --- a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs @@ -86,4 +86,14 @@ public class AdvancedSearchUtilitiesTests Assert.False(AdvancedSearchUtilities.CompareValues(typeof(TimeSpan), TimeSpan.Zero, "notaspan", "="))); Assert.Null(ex); } + + [Fact] + public void CompareValues_Guid_ValueTypeParsedViaTypes() + { + // Guid is a value type not named by the hot paths; it's parsed via Types (IParsable) and + // compared by value. + var g = Guid.Parse("00000000-0000-0000-0000-000000000001"); + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(Guid), g, "00000000-0000-0000-0000-000000000001", "=")); + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(Guid), g, "00000000-0000-0000-0000-000000000002", "=")); + } } diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs index c9a36b59b..bdd5314f4 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs @@ -127,9 +127,14 @@ public static class AdvancedSearchUtilities _ => false }; } - if (!propertyType.IsValueType) + // Anything the hot typed paths above didn't handle — reference types (Poison, Map, entity + // properties resolved by serial, ...) and value types exposing IParsable (Guid, + // DateTimeOffset, ...). Delegate parsing to the shared, thread-safe Types converter so the + // target is parsed into the property's real type, then compare by value. A string is + // allocated here, but this is the uncommon path; the common types never reach it. + if (!propertyType.IsValueType || Types.IsParsable(propertyType)) { - return TryParseValue(valuePart, out var parsedValue) && + return Types.TryParse(propertyType, valuePart.ToString(), out var parsedValue) == null && CompareReference(propertyValue!, parsedValue, operatorSpan); } @@ -345,9 +350,8 @@ public static class AdvancedSearchUtilities return TryParseNumericValue(valuePart, out value); } - // string/object need no parsing — the span itself is the value (object is the reference-type - // comparison path, which compares against the raw text). - if (typeof(T) == typeof(string) || typeof(T) == typeof(object)) + // string needs no parsing — the span itself is the value. + if (typeof(T) == typeof(string)) { value = (T)(object)valuePart.ToString(); return true; diff --git a/Projects/UOContent/Utilities/Types.cs b/Projects/UOContent/Utilities/Types.cs index 810ff0310..5edac64e6 100644 --- a/Projects/UOContent/Utilities/Types.cs +++ b/Projects/UOContent/Utilities/Types.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.Reflection; @@ -10,7 +11,6 @@ namespace Server { public static readonly Type[] ParseStringParamTypes = { typeof(string), typeof(IFormatProvider) }; public static readonly Type[] ParseStringNumericParamTypes = { typeof(string), typeof(NumberStyles) }; - private static object[] _parseParams = { null, null }; public static readonly Type OfByte = typeof(byte); public static readonly Type OfSByte = typeof(sbyte); @@ -75,7 +75,9 @@ namespace Server OfULong }; - private static Dictionary _isParsable; + // Thread-safe: parse metadata is read from parallel callers (e.g. the Advanced Search workers), + // not just the single-threaded command path. + private static readonly ConcurrentDictionary _isParsable = new(); [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsType(Type type, Type check) => check.IsAssignableFrom(type); @@ -89,25 +91,19 @@ namespace Server [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsText(Type t) => IsType(t, OfText); - public static bool IsParsable(Type t) - { - _isParsable ??= new(); - if (_isParsable.TryGetValue(t, out var isParsable)) + public static bool IsParsable(Type t) => + _isParsable.GetOrAdd(t, static type => { - return isParsable; - } - - foreach (var x in t.GetInterfaces()) - { - if (x.IsGenericType && x.GetGenericTypeDefinition() == typeof(IParsable<>)) + foreach (var x in type.GetInterfaces()) { - isParsable = true; - break; + if (x.IsGenericType && x.GetGenericTypeDefinition() == typeof(IParsable<>)) + { + return true; + } } - } - return _isParsable[t] = isParsable; - } + return false; + }); [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsDecimal(Type t) => Array.IndexOf(DecimalTypes, t) >= 0; @@ -118,18 +114,14 @@ namespace Server [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsEntity(Type t) => OfEntity.IsAssignableFrom(t); - private static Dictionary _parseMethods; + private static readonly ConcurrentDictionary _parseMethods = new(); public static object Parse(Type t, string value) { - _parseMethods ??= new(); - if (!_parseMethods.TryGetValue(t, out var method)) - { - _parseMethods[t] = method = t.GetMethod("Parse", ParseStringParamTypes); - } + var method = _parseMethods.GetOrAdd(t, static type => type.GetMethod("Parse", ParseStringParamTypes)); - _parseParams[0] = value; - return method?.Invoke(null, _parseParams); + // Fresh args array per call — a shared static array would race across concurrent callers. + return method?.Invoke(null, new object[] { value, null }); } // Do not use this in "Parse" methods, it may cause a stack overflow