Fix exceptions in Slider and Tags property value converters (#13782)

* Fix IndexOutOfRangeException when converting single value to range in SliderValueConverter * Fix NullReferenceException while deserializing empty value in TagsValueConverter * Use invariant decimal parsing * Handle converting from slider to single value * Fix parsing range as single value * Make Handle methods autonomous --------- Co-authored-by: nikolajlauridsen <nikolajlauridsen@protonmail.ch>
2023-08-23 10:09:43 +02:00
parent 9fd0b1986c
commit 3c37653012
3 changed files with 125 additions and 84 deletions
--- a/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs
+++ b/src/Umbraco.Core/Cache/DataTypeCacheRefresher.cs
@@ -88,10 +88,6 @@ public sealed class DataTypeCacheRefresher : PayloadCacheRefresherBase<DataTypeC
            }
        }

-        // TODO: not sure I like these?
-        TagsValueConverter.ClearCaches();
-        SliderValueConverter.ClearCaches();
-
        // refresh the models and cache
        _publishedModelFactory.WithSafeLiveFactoryReset(() =>
            _publishedSnapshotService.Notify(payloads));
--- a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs
+++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs
@@ -1,4 +1,4 @@
-using System.Collections.Concurrent;
+using System.Globalization;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.PublishedContent;
 using Umbraco.Cms.Core.Services;
@@ -6,74 +6,123 @@ using Umbraco.Extensions;

 namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

+/// <summary>
+/// The slider property value converter.
+/// </summary>
+/// <seealso cref="Umbraco.Cms.Core.PropertyEditors.PropertyValueConverterBase" />
 [DefaultPropertyValueConverter]
 public class SliderValueConverter : PropertyValueConverterBase
 {
-    private static readonly ConcurrentDictionary<int, bool> Storages = new();
-    private readonly IDataTypeService _dataTypeService;
+    /// <summary>
+    /// Initializes a new instance of the <see cref="SliderValueConverter" /> class.
+    /// </summary>
+    public SliderValueConverter()
+    { }

-    public SliderValueConverter(IDataTypeService dataTypeService) => _dataTypeService =
-        dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService));
+    /// <summary>
+    /// Initializes a new instance of the <see cref="SliderValueConverter" /> class.
+    /// </summary>
+    /// <param name="dataTypeService">The data type service.</param>
+    [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")]
+    public SliderValueConverter(IDataTypeService dataTypeService)
+    { }

-    public static void ClearCaches() => Storages.Clear();
+    /// <summary>
+    /// Clears the data type configuration caches.
+    /// </summary>
+    [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")]
+    public static void ClearCaches()
+    { }

+    /// <inheritdoc />
    public override bool IsConverter(IPublishedPropertyType propertyType)
        => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Slider);

+    /// <inheritdoc />
    public override Type GetPropertyValueType(IPublishedPropertyType propertyType)
-        => IsRangeDataType(propertyType.DataType.Id) ? typeof(Range<decimal>) : typeof(decimal);
+        => IsRange(propertyType) ? typeof(Range<decimal>) : typeof(decimal);

+    /// <inheritdoc />
    public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType)
        => PropertyCacheLevel.Element;

+    /// <inheritdoc />
    public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview)
    {
-        if (source == null)
+        bool isRange = IsRange(propertyType);
+
+        var sourceString = source?.ToString();
+
+        return isRange
+            ? HandleRange(sourceString)
+            : HandleDecimal(sourceString);
+    }
+
+    private static Range<decimal> HandleRange(string? sourceString)
+    {
+        if (sourceString is null)
        {
-            return null;
+            return new Range<decimal>();
        }

-        if (IsRangeDataType(propertyType.DataType.Id))
-        {
-            var rangeRawValues = source.ToString()!.Split(Constants.CharArrays.Comma);
-            Attempt<decimal> minimumAttempt = rangeRawValues[0].TryConvertTo<decimal>();
-            Attempt<decimal> maximumAttempt = rangeRawValues[1].TryConvertTo<decimal>();
+        string[] rangeRawValues = sourceString.Split(Constants.CharArrays.Comma);

-            if (minimumAttempt.Success && maximumAttempt.Success)
+        if (TryParseDecimal(rangeRawValues[0], out var minimum))
+        {
+            if (rangeRawValues.Length == 1)
            {
-                return new Range<decimal> { Maximum = maximumAttempt.Result, Minimum = minimumAttempt.Result };
+                // Configuration is probably changed from single to range, return range with same min/max
+                return new Range<decimal>
+                {
+                    Minimum = minimum,
+                    Maximum = minimum
+                };
+            }
+
+            if (rangeRawValues.Length == 2 && TryParseDecimal(rangeRawValues[1], out var maximum))
+            {
+                return new Range<decimal>
+                {
+                    Minimum = minimum,
+                    Maximum = maximum
+                };
            }
        }

-        Attempt<decimal> valueAttempt = source.ToString().TryConvertTo<decimal>();
-        if (valueAttempt.Success)
+        return new Range<decimal>();
+    }
+
+    private static decimal HandleDecimal(string? sourceString)
+    {
+        if (string.IsNullOrEmpty(sourceString))
        {
-            return valueAttempt.Result;
+            return default;
        }

-        // Something failed in the conversion of the strings to decimals
-        return null;
+        // This used to be a range slider, so we'll assign the minimum value as the new value
+        if (sourceString.Contains(','))
+        {
+            var minimumValueRepresentation = sourceString.Split(Constants.CharArrays.Comma)[0];
+
+            if (TryParseDecimal(minimumValueRepresentation, out var minimum))
+            {
+                return minimum;
+            }
+        }
+        else if (TryParseDecimal(sourceString, out var value))
+        {
+            return value;
+        }
+
+        return default;
    }

    /// <summary>
-    ///     Discovers if the slider is set to range mode.
+    /// Helper method for parsing a double consistently
    /// </summary>
-    /// <param name="dataTypeId">
-    ///     The data type id.
-    /// </param>
-    /// <returns>
-    ///     The <see cref="bool" />.
-    /// </returns>
-    private bool IsRangeDataType(int dataTypeId) =>
+    private static bool TryParseDecimal(string? representation, out decimal value)
+        => decimal.TryParse(representation, NumberStyles.Number, CultureInfo.InvariantCulture, out value);

-        // GetPreValuesCollectionByDataTypeId is cached at repository level;
-        // still, the collection is deep-cloned so this is kinda expensive,
-        // better to cache here + trigger refresh in DataTypeCacheRefresher
-        // TODO: this is cheap now, remove the caching
-        Storages.GetOrAdd(dataTypeId, id =>
-        {
-            IDataType? dataType = _dataTypeService.GetDataType(id);
-            SliderConfiguration? configuration = dataType?.ConfigurationAs<SliderConfiguration>();
-            return configuration?.EnableRange ?? false;
-        });
+    private static bool IsRange(IPublishedPropertyType propertyType)
+        => propertyType.DataType.ConfigurationAs<SliderConfiguration>()?.EnableRange == true;
 }
--- a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs
+++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs
@@ -1,4 +1,3 @@
-using System.Collections.Concurrent;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.PublishedContent;
 using Umbraco.Cms.Core.Serialization;
@@ -7,69 +6,66 @@ using Umbraco.Extensions;

 namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

+/// <summary>
+/// The tags property value converter.
+/// </summary>
+/// <seealso cref="Umbraco.Cms.Core.PropertyEditors.PropertyValueConverterBase" />
 [DefaultPropertyValueConverter]
 public class TagsValueConverter : PropertyValueConverterBase
 {
-    private static readonly ConcurrentDictionary<int, bool> Storages = new();
-    private readonly IDataTypeService _dataTypeService;
    private readonly IJsonSerializer _jsonSerializer;

+    /// <summary>
+    /// Initializes a new instance of the <see cref="TagsValueConverter" /> class.
+    /// </summary>
+    /// <param name="jsonSerializer">The JSON serializer.</param>
+    /// <exception cref="System.ArgumentNullException">jsonSerializer</exception>
+    public TagsValueConverter(IJsonSerializer jsonSerializer)
+        => _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer));
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="TagsValueConverter" /> class.
+    /// </summary>
+    /// <param name="dataTypeService">The data type service.</param>
+    /// <param name="jsonSerializer">The JSON serializer.</param>
+    [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")]
    public TagsValueConverter(IDataTypeService dataTypeService, IJsonSerializer jsonSerializer)
-    {
-        _dataTypeService = dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService));
-        _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer));
-    }
+        : this(jsonSerializer)
+    { }

-    public static void ClearCaches() => Storages.Clear();
+    /// <summary>
+    /// Clears the data type configuration caches.
+    /// </summary>
+    [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")]
+    public static void ClearCaches()
+    { }

+    /// <inheritdoc />
    public override bool IsConverter(IPublishedPropertyType propertyType)
        => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Tags);

+    /// <inheritdoc />
    public override Type GetPropertyValueType(IPublishedPropertyType propertyType)
        => typeof(IEnumerable<string>);

+    /// <inheritdoc />
    public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType)
        => PropertyCacheLevel.Element;

+    /// <inheritdoc />
    public override object? ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object? source, bool preview)
    {
-        if (source == null)
+        string? sourceString = source?.ToString();
+        if (string.IsNullOrEmpty(sourceString))
        {
            return Array.Empty<string>();
        }

-        // if Json storage type deserialize and return as string array
-        if (JsonStorageType(propertyType.DataType.Id))
-        {
-            var array = source.ToString() is not null
-                ? _jsonSerializer.Deserialize<string[]>(source.ToString()!)
-                : null;
-            return array ?? Array.Empty<string>();
-        }
-
-        // Otherwise assume CSV storage type and return as string array
-        return source.ToString()?.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries);
+        return IsJson(propertyType)
+            ? _jsonSerializer.Deserialize<string[]>(sourceString) ?? Array.Empty<string>()
+            : sourceString.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries);
    }

-    public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) => (string[]?)source;
-
-    /// <summary>
-    ///     Discovers if the tags data type is storing its data in a Json format
-    /// </summary>
-    /// <param name="dataTypeId">
-    ///     The data type id.
-    /// </param>
-    /// <returns>
-    ///     The <see cref="bool" />.
-    /// </returns>
-    private bool JsonStorageType(int dataTypeId) =>
-
-        // GetDataType(id) is cached at repository level; still, there is some
-        // deep-cloning involved (expensive) - better cache here + trigger
-        // refresh in DataTypeCacheRefresher
-        Storages.GetOrAdd(dataTypeId, id =>
-        {
-            TagConfiguration? configuration = _dataTypeService.GetDataType(id)?.ConfigurationAs<TagConfiguration>();
-            return configuration?.StorageType == TagsStorageType.Json;
-        });
+    private static bool IsJson(IPublishedPropertyType propertyType)
+        => propertyType.DataType.ConfigurationAs<TagConfiguration>()?.StorageType == TagsStorageType.Json;
 }