Pre-empt TryConvertTo InvalidCastExceptions (issue #304) (#986)

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 <noreply@anthropic.com>

Author: KevinJump

uSync.BackOffice/Configuration/uSyncHandlerSettings.cs

@@ -4,6 +4,8 @@
 
 using Umbraco.Extensions;
 
+using uSync.Core.Extensions;
+
 namespace uSync.BackOffice.Configuration;
 
 /// <summary>
@@ -89,10 +91,10 @@ public static class HandlerSettingsExtensions
     /// <returns></returns>
     public static TResult GetSetting<TResult>(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<TResult>();
-            if (attempt) return attempt.Result ?? defaultValue;
+            if (value.TryConvertPreChecked<TResult>(out var result) && result is not null)
+                return result;
         }
 
         return defaultValue;

uSync.Core/Cache/SyncEntityCache.cs

@@ -39,7 +39,10 @@ public SyncEntityCache(
     public CachedName? GetName(int id)
     {
         if (!_cacheEnabled) return default;
-        return cache.GetCacheItem<CachedName>(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<CachedName>(id.ToString());
     }
 
     public void AddName(int id, Guid guid, string name)

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<TObject>(this object value)
     {
         if (value == null) return default;
-        var attempt = value.TryConvertTo<TObject>();
-        if (!attempt) return default;
-        return attempt.Result;
+        return value.TryConvertPreChecked<TObject>(out var result) ? result : default;
     }
 
     public static Guid ConvertToGuid(this int value)

uSync.Core/Extensions/JsonTextExtensions.cs

@@ -400,6 +400,43 @@ private static bool TryGetValueAs<TObject>(this object value, [MaybeNullWhen(fal
         return true;
     }
 
+    /// <summary>
+    ///  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).
+    /// </summary>
+    /// <remarks>
+    ///  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.
+    /// </remarks>
+    public static bool TryConvertPreChecked<TObject>(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<TObject>(_defaultOptions);
+                if (result is not null) return true;
+            }
+            catch
+            {
+                // not something STJ could convert directly - fall back to TryConvertTo below.
+            }
+        }
+
+        var attempt = value.TryConvertTo<TObject>();
+        if (attempt.Success is false || attempt.Result is null) return false;
+
+        result = attempt.Result;
+        return true;
+    }
+
     #endregion
 
     #region property getters 

uSync.Core/Mapping/SyncValueMapperBase.cs

@@ -115,10 +115,7 @@ protected IEnumerable<uSyncDependency> CreateDependencies(IEnumerable<string> ud
     protected static TObject? GetValueAs<TObject>(object value)
     {
         if (value == null) return default;
-        var attempt = value.TryConvertTo<TObject>();
-        if (!attempt) return default;
-
-        return attempt.Result;
+        return value.TryConvertPreChecked<TObject>(out var result) ? result : default;
     }
 }
 

uSync.Core/Serialization/SyncSerializerOptions.cs

@@ -2,6 +2,8 @@
 
 using Umbraco.Extensions;
 
+using uSync.Core.Extensions;
+
 namespace uSync.Core.Serialization;
 
 /// <summary>
@@ -70,11 +72,10 @@ public SyncSerializerOptions(SerializerFlags flags, Dictionary<string, object?>
 
     public TResult GetSetting<TResult>(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<TResult>();
-            if (attempt.Success && attempt.Result is not null)
-                return attempt.Result;
+            if (value.TryConvertPreChecked<TResult>(out var result) && result is not null)
+                return result;
         }
 
         return defaultValue;

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<IEntityService> _entityServiceMock;
+    private Mock<IContentTypeService> _contentTypeServiceMock;
+    private SyncEntityCache _cache;
+
+    [SetUp]
+    public void Setup()
+    {
+        _entityServiceMock = new Mock<IEntityService>();
+        _contentTypeServiceMock = new Mock<IContentTypeService>();
+        _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<IEntitySlim>();
+        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"));
+        });
+    }
+}

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;
+
+/// <summary>
+///  tests for the JsonElement pre-check that avoids the swallowed
+///  InvalidCastException Umbraco's TryConvertTo throws on JsonElement values
+///  (uSync.Complete issue #304).
+/// </summary>
+[TestFixture]
+internal class TryConvertPreCheckedTests
+{
+    [Test]
+    public void JsonElementTrue_ConvertsToBool()
+    {
+        object value = JsonSerializer.SerializeToElement(true);
+
+        var success = value.TryConvertPreChecked<bool>(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<bool>(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<int>(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<Guid>(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<string>(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<int>(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<bool>(out var result);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(success, Is.False);
+            Assert.That(result, Is.False);
+        });
+    }
+}