From 451f82610587fa09449a62109dd0b51978e74b17 Mon Sep 17 00:00:00 2001 From: Kevin Jump Date: Sat, 11 Jul 2026 08:59:08 +0100 Subject: [PATCH] Pre-empt TryConvertTo InvalidCastExceptions (issue #304) Two sources of swallowed first-chance InvalidCastExceptions during push, ~100 each per run, that slow the app when a debugger is attached. 1. SyncEntityCache.GetName read from the entity cache (`cache`) while AddName wrote to `nameCache`. The entity cache holds IEntitySlim objects under the same id key, so every name lookup threw IEntitySlim -> CachedName and returned null - the name cache never actually worked. GetName now reads from nameCache. 2. Config/setting values arrive as JsonElement; Umbraco's TryConvertTo throws (and swallows) InvalidCastException turning a JsonElement into a value type e.g. bool. Added JsonTextExtensions.TryConvertPreChecked which does the JsonElement conversion with System.Text.Json first and only falls back to TryConvertTo. Routed the value/config wrappers (ConversionExtensions, SyncValueMapperBase, SyncSerializerOptions, HandlerSettingsExtensions) through it. Adds tests for both fixes. Co-Authored-By: Claude Opus 4.8 --- .../Configuration/uSyncHandlerSettings.cs | 8 +- uSync.Core/Cache/SyncEntityCache.cs | 5 +- uSync.Core/Extensions/ConversionExtensions.cs | 8 +- uSync.Core/Extensions/JsonTextExtensions.cs | 37 ++++++ uSync.Core/Mapping/SyncValueMapperBase.cs | 5 +- .../Serialization/SyncSerializerOptions.cs | 9 +- uSync.Tests/Cache/SyncEntityCacheTests.cs | 75 +++++++++++ .../Extensions/TryConvertPreCheckedTests.cs | 118 ++++++++++++++++++ 8 files changed, 247 insertions(+), 18 deletions(-) create mode 100644 uSync.Tests/Cache/SyncEntityCacheTests.cs create mode 100644 uSync.Tests/Extensions/TryConvertPreCheckedTests.cs diff --git a/uSync.BackOffice/Configuration/uSyncHandlerSettings.cs b/uSync.BackOffice/Configuration/uSyncHandlerSettings.cs index 3f3916481..4c6811d9e 100644 --- a/uSync.BackOffice/Configuration/uSyncHandlerSettings.cs +++ b/uSync.BackOffice/Configuration/uSyncHandlerSettings.cs @@ -4,6 +4,8 @@ using Umbraco.Extensions; +using uSync.Core.Extensions; + namespace uSync.BackOffice.Configuration; /// @@ -89,10 +91,10 @@ public static class HandlerSettingsExtensions /// public static TResult GetSetting(this HandlerSettings settings, string key, TResult defaultValue) { - if (settings.Settings != null && settings.Settings.TryGetValue(key, out var value)) + if (settings.Settings != null && settings.Settings.TryGetValue(key, out var value) && value is not null) { - var attempt = value.TryConvertTo(); - if (attempt) return attempt.Result ?? defaultValue; + if (value.TryConvertPreChecked(out var result) && result is not null) + return result; } return defaultValue; diff --git a/uSync.Core/Cache/SyncEntityCache.cs b/uSync.Core/Cache/SyncEntityCache.cs index e537b582c..cf1621b77 100644 --- a/uSync.Core/Cache/SyncEntityCache.cs +++ b/uSync.Core/Cache/SyncEntityCache.cs @@ -39,7 +39,10 @@ public SyncEntityCache( public CachedName? GetName(int id) { if (!_cacheEnabled) return default; - return cache.GetCacheItem(id.ToString()); + // read from nameCache - this is where AddName stores CachedName values. + // (reading from `cache` returned IEntitySlim entries under the same key, + // which threw a swallowed InvalidCastException and never actually cached). + return nameCache.GetCacheItem(id.ToString()); } public void AddName(int id, Guid guid, string name) diff --git a/uSync.Core/Extensions/ConversionExtensions.cs b/uSync.Core/Extensions/ConversionExtensions.cs index a4ba55ed7..d04d2bd70 100644 --- a/uSync.Core/Extensions/ConversionExtensions.cs +++ b/uSync.Core/Extensions/ConversionExtensions.cs @@ -1,14 +1,10 @@ -using Umbraco.Extensions; - -namespace uSync.Core.Extensions; +namespace uSync.Core.Extensions; internal static class ConversionExtensions { public static TObject? GetValueAs(this object value) { if (value == null) return default; - var attempt = value.TryConvertTo(); - if (!attempt) return default; - return attempt.Result; + return value.TryConvertPreChecked(out var result) ? result : default; } public static Guid ConvertToGuid(this int value) diff --git a/uSync.Core/Extensions/JsonTextExtensions.cs b/uSync.Core/Extensions/JsonTextExtensions.cs index ecaca55b2..86d8424e8 100644 --- a/uSync.Core/Extensions/JsonTextExtensions.cs +++ b/uSync.Core/Extensions/JsonTextExtensions.cs @@ -400,6 +400,43 @@ private static bool TryGetValueAs(this object value, [MaybeNullWhen(fal return true; } + /// + /// Convert a value to the requested type, pre-empting the first-chance + /// InvalidCastException that Umbraco's TryConvertTo throws when converting + /// a JsonElement to a value type (see uSync.Complete issue #304). + /// + /// + /// Settings/config values often arrive as JsonElement (bound from appsettings.json). + /// Asking Umbraco's TryConvertTo to turn one into e.g. a bool throws (and swallows) + /// an InvalidCastException every call - harmless, but noisy and slow when a debugger + /// is attached. Doing the JsonElement conversion with System.Text.Json first means the + /// common path never throws; anything STJ can't handle still falls back to TryConvertTo. + /// + public static bool TryConvertPreChecked(this object? value, [MaybeNullWhen(false)] out TObject result) + { + result = default; + if (value is null) return false; + + if (value is JsonElement element) + { + try + { + result = element.Deserialize(_defaultOptions); + if (result is not null) return true; + } + catch + { + // not something STJ could convert directly - fall back to TryConvertTo below. + } + } + + var attempt = value.TryConvertTo(); + if (attempt.Success is false || attempt.Result is null) return false; + + result = attempt.Result; + return true; + } + #endregion #region property getters diff --git a/uSync.Core/Mapping/SyncValueMapperBase.cs b/uSync.Core/Mapping/SyncValueMapperBase.cs index 97fa70256..5d07c36c4 100644 --- a/uSync.Core/Mapping/SyncValueMapperBase.cs +++ b/uSync.Core/Mapping/SyncValueMapperBase.cs @@ -115,10 +115,7 @@ protected IEnumerable CreateDependencies(IEnumerable ud protected static TObject? GetValueAs(object value) { if (value == null) return default; - var attempt = value.TryConvertTo(); - if (!attempt) return default; - - return attempt.Result; + return value.TryConvertPreChecked(out var result) ? result : default; } } diff --git a/uSync.Core/Serialization/SyncSerializerOptions.cs b/uSync.Core/Serialization/SyncSerializerOptions.cs index f34b0bdbf..6426ac885 100644 --- a/uSync.Core/Serialization/SyncSerializerOptions.cs +++ b/uSync.Core/Serialization/SyncSerializerOptions.cs @@ -2,6 +2,8 @@ using Umbraco.Extensions; +using uSync.Core.Extensions; + namespace uSync.Core.Serialization; /// @@ -70,11 +72,10 @@ public SyncSerializerOptions(SerializerFlags flags, Dictionary public TResult GetSetting(string key, TResult defaultValue) { - if (this.Settings?.TryGetValue(key, out var value) is true) + if (this.Settings?.TryGetValue(key, out var value) is true && value is not null) { - var attempt = value.TryConvertTo(); - if (attempt.Success && attempt.Result is not null) - return attempt.Result; + if (value.TryConvertPreChecked(out var result) && result is not null) + return result; } return defaultValue; diff --git a/uSync.Tests/Cache/SyncEntityCacheTests.cs b/uSync.Tests/Cache/SyncEntityCacheTests.cs new file mode 100644 index 000000000..62faf16f6 --- /dev/null +++ b/uSync.Tests/Cache/SyncEntityCacheTests.cs @@ -0,0 +1,75 @@ +using System; + +using Moq; + +using NUnit.Framework; + +using Umbraco.Cms.Core.Models.Entities; +using Umbraco.Cms.Core.Services; + +using uSync.Core.Cache; + +namespace uSync.Tests.Cache; + +[TestFixture] +internal class SyncEntityCacheTests +{ + private Mock _entityServiceMock; + private Mock _contentTypeServiceMock; + private SyncEntityCache _cache; + + [SetUp] + public void Setup() + { + _entityServiceMock = new Mock(); + _contentTypeServiceMock = new Mock(); + _cache = new SyncEntityCache(_entityServiceMock.Object, _contentTypeServiceMock.Object); + } + + [Test] + public void AddName_ThenGetName_RoundTrips() + { + var id = 1234; + var key = Guid.NewGuid(); + + _cache.AddName(id, key, "Test Name"); + + var result = _cache.GetName(id); + + Assert.That(result, Is.Not.Null); + Assert.Multiple(() => + { + Assert.That(result.Key, Is.EqualTo(key)); + Assert.That(result.Name, Is.EqualTo("Test Name")); + }); + } + + // regression for uSync.Complete issue #304 - GetName used to read from the + // entity cache, which holds IEntitySlim objects under the same id key. That + // threw a swallowed InvalidCastException (IEntitySlim -> CachedName) and + // never returned the name. GetName must read from the name cache instead. + [Test] + public void GetName_WhenEntityCachedUnderSameId_StillReturnsName() + { + var id = 4321; + var key = Guid.NewGuid(); + + var entityMock = new Mock(); + entityMock.SetupGet(x => x.Id).Returns(id); + _entityServiceMock.Setup(x => x.Get(id)).Returns(entityMock.Object); + + // populate the entity cache for this id (as GetFriendlyPath does). + _ = _cache.GetEntity(id); + + _cache.AddName(id, key, "Real Name"); + + var result = _cache.GetName(id); + + Assert.That(result, Is.Not.Null); + Assert.Multiple(() => + { + Assert.That(result.Key, Is.EqualTo(key)); + Assert.That(result.Name, Is.EqualTo("Real Name")); + }); + } +} diff --git a/uSync.Tests/Extensions/TryConvertPreCheckedTests.cs b/uSync.Tests/Extensions/TryConvertPreCheckedTests.cs new file mode 100644 index 000000000..048c31dd7 --- /dev/null +++ b/uSync.Tests/Extensions/TryConvertPreCheckedTests.cs @@ -0,0 +1,118 @@ +using System; +using System.Text.Json; + +using NUnit.Framework; + +using uSync.Core.Extensions; + +namespace uSync.Tests.Extensions; + +/// +/// tests for the JsonElement pre-check that avoids the swallowed +/// InvalidCastException Umbraco's TryConvertTo throws on JsonElement values +/// (uSync.Complete issue #304). +/// +[TestFixture] +internal class TryConvertPreCheckedTests +{ + [Test] + public void JsonElementTrue_ConvertsToBool() + { + object value = JsonSerializer.SerializeToElement(true); + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.True); + Assert.That(result, Is.True); + }); + } + + [Test] + public void JsonElementFalse_ConvertsToBool() + { + object value = JsonSerializer.SerializeToElement(false); + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.True); + Assert.That(result, Is.False); + }); + } + + [Test] + public void JsonElementNumber_ConvertsToInt() + { + object value = JsonSerializer.SerializeToElement(42); + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.True); + Assert.That(result, Is.EqualTo(42)); + }); + } + + [Test] + public void JsonElementString_ConvertsToGuid() + { + var guid = Guid.NewGuid(); + object value = JsonSerializer.SerializeToElement(guid.ToString()); + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.True); + Assert.That(result, Is.EqualTo(guid)); + }); + } + + [Test] + public void JsonElementString_ConvertsToString() + { + object value = JsonSerializer.SerializeToElement("hello"); + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.True); + Assert.That(result, Is.EqualTo("hello")); + }); + } + + // a plain CLR value skips the JsonElement branch and still converts via + // the TryConvertTo fallback - behaviour must be unchanged for these. + [Test] + public void PlainString_ConvertsToInt_ViaFallback() + { + object value = "42"; + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.True); + Assert.That(result, Is.EqualTo(42)); + }); + } + + [Test] + public void Null_ReturnsFalse() + { + object? value = null; + + var success = value.TryConvertPreChecked(out var result); + + Assert.Multiple(() => + { + Assert.That(success, Is.False); + Assert.That(result, Is.False); + }); + } +}