diff --git a/src/SmartFormat.Tests/Extensions/DictionarySourceTests.cs b/src/SmartFormat.Tests/Extensions/DictionarySourceTests.cs index fd2ecc07..050a6bdd 100644 --- a/src/SmartFormat.Tests/Extensions/DictionarySourceTests.cs +++ b/src/SmartFormat.Tests/Extensions/DictionarySourceTests.cs @@ -2,8 +2,8 @@ using System.Collections; using System.Collections.Generic; using System.Dynamic; -using System.Linq; using NUnit.Framework; +using SmartFormat.Core.Formatting; using SmartFormat.Core.Settings; using SmartFormat.Extensions; using SmartFormat.Tests.TestUtils; @@ -159,6 +159,98 @@ public void Dictionary_Dot_Notation_Nullable() Assert.That(result, Is.EqualTo(expected)); } + [Test] + public void Dictionary_NullableOperator_MissingKey_ReturnsEmpty() + { + // Test similar to GitHub issue #522, but using string keys + var sf = Smart.CreateDefaultSmartFormat(); + + var obj = new { + Changes = new Dictionary + { + { "Count", 100 }, + // Volume is intentionally missing + // although it is accessed in the format string + { "Name", "ABCD" }, + } + }; + + // Use nullable syntax to avoid exceptions for selectors + // that cannot be resolved with the DictionarySource: + // 'Volume' may be missing, but 'Changes' is expected to be present + var resultSyntax1 = + sf.Format("{Changes:{Count};{?.Volume};{Name}}", obj); + var resultSyntax2 = + sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj); + + Assert.Multiple(() => + { + Assert.That(resultSyntax1, Is.EqualTo("100;;ABCD")); + Assert.That(resultSyntax2, Is.EqualTo("100;;ABCD")); + }); + + // Now add the missing key and verify that it is properly resolved + obj.Changes.Add("Volume", 200); + Assert.That(sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj), + Is.EqualTo("100;200;ABCD")); + } + + private enum ChangeKey { Count, Volume, Name } + + [Test] + public void Dictionary_NullableOperator_MissingEnumKey_ReturnsEmpty() + { + // Test for GitHub issue #522 with non-string dictionary keys + var sf = Smart.CreateDefaultSmartFormat(); + var obj = new { + Changes = new Dictionary + { + { ChangeKey.Count, 100 }, + // Volume is intentionally missing + // although it is accessed in the format string + { ChangeKey.Name, "ABCD" }, + } + }; + // Use nullable syntax to avoid exceptions for selectors + // that cannot be resolved with the DictionarySource: + // 'Volume' may be missing, but 'Changes' is expected to be present + var result = + sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj); + Assert.Multiple(() => + { + Assert.That(result, Is.EqualTo("100;;ABCD")); + }); + + // Now add the missing key and verify that it is properly resolved + obj.Changes.Add(ChangeKey.Volume, 200); + Assert.That(sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj), + Is.EqualTo("100;200;ABCD")); + } + + [Test] + public void Dictionary_NullableOperator_MissingKey_OnWrongSelector() + { + // Test for GitHub issue #522 + var sf = Smart.CreateDefaultSmartFormat(); + + var obj = new { Changes = new Dictionary + { + { "Count", 100 }, + // Volume is intentionally missing + // although it is accessed in the format string + { "Name", "ABCD" }, + } + }; + + // The nullable operator must only be applied + // to the selector that is expected to be missing or + // a preceding selector in the chain that is expected to be missing + // Here: 'Changes' is expected to be present, but 'Volume' is missing, + // so the nullable operator must be applied to 'Volume' instead of after 'Volume' + Assert.Throws(() => + sf.Format("{Changes.Count};{Changes.Volume?.Anything};{Changes.Name}", obj)); + } + [Test] public void Generic_Dictionary_String_String() { @@ -195,28 +287,6 @@ public void IReadOnlyDictionary_With_String_Key() Assert.That(result, Is.EqualTo("12three")); } - [Test] - public void IReadOnlyDictionary_Cache_Should_Store_Types_It_Cannot_Handle() - { - var dictSource = new DictionarySource { IsIReadOnlyDictionarySupported = true }; - var kvp = new KeyValuePair("One", 1); - var smart = new SmartFormatter() - .AddExtensions(new DefaultSource(), dictSource, new KeyValuePairSource()) - .AddExtensions(new DefaultFormatter()); - var result = smart.Format("{One}", kvp); - - Assert.Multiple(() => - { - Assert.That(result, Is.EqualTo("1")); - Assert.That(dictSource.RoDictionaryTypeCache.Keys, Has.Count.EqualTo(1)); - }); - Assert.Multiple(() => - { - Assert.That(dictSource.RoDictionaryTypeCache.Keys.First(), Is.EqualTo(typeof(KeyValuePair))); - Assert.That(dictSource.RoDictionaryTypeCache.Values.First(), Is.Null); - }); - } - public class CustomReadOnlyDictionary : IReadOnlyDictionary { private readonly IDictionary _dictionary; diff --git a/src/SmartFormat/Core/Extensions/Source.cs b/src/SmartFormat/Core/Extensions/Source.cs index d73f4682..a46c2e73 100644 --- a/src/SmartFormat/Core/Extensions/Source.cs +++ b/src/SmartFormat/Core/Extensions/Source.cs @@ -33,23 +33,31 @@ public virtual void Initialize(SmartFormatter smartFormatter) } /// - /// Checks if any of the 's has nullable ? as their first operator. + /// Checks whether any of the 's , + /// up to and including the selector at , + /// has the nullable ? as the first character of its operator. /// - /// + /// The for the selector currently being evaluated. /// - /// , any of the 's has nullable ? as their first operator. + /// if any with a + /// less than or equal to + /// has the nullable ? as the first character of its operator; otherwise . /// /// - /// The nullable operator '?' can be followed by a dot (like '?.') or a square brace (like '.[') + /// Only selectors up to the current one are considered, so a nullable operator on a later selector + /// in the same does not influence the evaluation of earlier selectors. + /// The nullable operator ? must be followed by a dot (e.g. ?.) or a square bracket (e.g. ?[). /// - private bool HasNullableOperator(ISelectorInfo selectorInfo) + protected virtual bool HasNullableOperator(ISelectorInfo selectorInfo) { if (_smartSettings != null && selectorInfo.Placeholder != null) { #pragma warning disable S3267 // Don't use LINQ in favor of less GC foreach (var s in selectorInfo.Placeholder.Selectors) { - if (s.OperatorLength > 1 && s.BaseString[s.OperatorStartIndex] == ParserSettings.NullableOperator) + if (s.SelectorIndex <= selectorInfo.SelectorIndex + && s.OperatorLength > 1 + && s.BaseString[s.OperatorStartIndex] == ParserSettings.NullableOperator) return true; } #pragma warning restore S3267 // Restore: Loops should be simplified with "LINQ" expressions diff --git a/src/SmartFormat/Extensions/DictionarySource.cs b/src/SmartFormat/Extensions/DictionarySource.cs index 66506b77..58423bf3 100644 --- a/src/SmartFormat/Extensions/DictionarySource.cs +++ b/src/SmartFormat/Extensions/DictionarySource.cs @@ -4,6 +4,7 @@ using System; using System.Collections; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Reflection; using SmartFormat.Core.Extensions; @@ -27,33 +28,46 @@ public override bool TryEvaluateSelector(ISelectorInfo selectorInfo) { var current = selectorInfo.CurrentValue; if (TrySetResultForNullableOperator(selectorInfo)) return true; - - if (current is null) return false; + + if (current == null) return false; + var dictionaryType = GetDictionaryType(current); + if (dictionaryType == DictionaryType.None) return false; var selector = selectorInfo.SelectorText; var comparison = selectorInfo.FormatDetails.Settings.GetCaseSensitivityComparison(); // Try to get the selector value for IDictionary - if (TryGetIDictionaryValue(current, selector, comparison, out var value)) + if (dictionaryType == DictionaryType.NonGeneric + && TryGetIDictionaryValue(current, selector, comparison, out var value)) { selectorInfo.Result = value; return true; } // Try to get the selector value for Dictionary<,> and dynamics (ExpandoObject) - if (TryGetGenericDictionaryValue(current, selector, comparison, out value)) + if (dictionaryType == DictionaryType.Generic + && TryGetGenericDictionaryValue(current, selector, comparison, out value)) { selectorInfo.Result = value; return true; } // Try to get the selector value for IReadOnlyDictionary<,> - if (IsIReadOnlyDictionarySupported && TryGetReadOnlyDictionaryValue(current, selector, - comparison, out value)) + if (dictionaryType == DictionaryType.ReadOnly && IsIReadOnlyDictionarySupported + && TryGetReadOnlyDictionaryValue(current, selector, comparison, out value)) { selectorInfo.Result = value; return true; } + + // No matching key found in a dictionary, but if the selector + // has a nullable operator, we set the result to null + // instead of leaving it as not found. + if (HasNullableOperator(selectorInfo)) + { + selectorInfo.Result = null; + return true; + } return false; } @@ -144,13 +158,6 @@ private bool TryGetDictionaryProperties(Type type, out (PropertyInfo KeyProperty if (RoDictionaryTypeCache.TryGetValue(type, out propertyTuple)) return propertyTuple != null; - if (!IsIReadOnlyDictionary(type)) - { - // don't check the type again, although it's not a IReadOnlyDictionary - RoDictionaryTypeCache[type] = null; - return false; - } - // get Key and Item properties of the dictionary propertyTuple = (type.GetProperty(nameof(IDictionary.Keys)), type.GetProperty("Item"))!; @@ -160,19 +167,45 @@ private bool TryGetDictionaryProperties(Type type, out (PropertyInfo KeyProperty return true; } + private static readonly ConcurrentDictionary RoDictionaryTypeBoolCache = new(); + private static bool IsIReadOnlyDictionary(Type type) { + if (RoDictionaryTypeBoolCache.TryGetValue(type, out var cached)) + return cached; + // No Linq for less garbage + var result = false; foreach (var typeInterface in type.GetInterfaces()) { if (typeInterface == typeof(IReadOnlyDictionary<,>) || (typeInterface.IsGenericType && typeInterface.GetGenericTypeDefinition() == typeof(IReadOnlyDictionary<,>))) - return true; + { + result = true; + break; + } } - return false; + RoDictionaryTypeBoolCache[type] = result; + return result; } #endregion + + internal enum DictionaryType + { + None, + NonGeneric, + Generic, + ReadOnly + } + + private DictionaryType GetDictionaryType(object current) + { + if (current is IDictionary) return DictionaryType.NonGeneric; + if (current is IDictionary) return DictionaryType.Generic; + if (IsIReadOnlyDictionarySupported && IsIReadOnlyDictionary(current.GetType())) return DictionaryType.ReadOnly; + return DictionaryType.None; + } }