A DataListItemAttribute with custom value can sometime not deserialize correctly

[benjaminc]

Which Contentment version are you using?

3.0.1

Which Umbraco version are you using? For example: 8.14.1 - don't just write v8

9.1.2

Bug summary

When I have an enum with DataListItem attributes, if one of the attributes is a number it will use the enum value whose zero-based position in the enum matches that number rather than whose DataListItem attribute matches that number, when I access it as IPublishedContent.

Steps to reproduce

1. Create an Enum, such as this:

   public enum CustomEnum
   {
   [DataListItem(Value = "1")]
   First,
   [DataListItem(Value = "2")]
   Second
   }

2. Create a datatype with a Contentment dropdown using the CustomEnum as the source
3. Add that datatype to a document type
4. Display the selected enum value in the template for the doc type
5. Create a document using that document type, and select First as the value from the dropdown list
6. Navigate to the page for that document, and see that the selected value is displayed as Second

Expected result / actual result

I would expect that when I select First from the Umbraco back-office, it would display First when I show the value on the front-end.

This appears to result from the IPublishedContent.Value call executing the property value converter, which ends up at https://github.com/leekelleher/umbraco-contentment/blob/master/src/Umbraco.Community.Contentment/DataEditors/DataList/DataSources/EnumDataListSource.cs#L188. The Enum.Parse call at that line won't just check the name of the value, but will also check the integer value of the enum against the contents of the string. To use this as a workaround, you have to manually assign integer values to all values in your enum, and as some of my enums have over 2,000 values that can become quite onerous. I believe the code fix would be to simply move line 188 referenced above down below the foreach block that ends at line 200.

Do you have Umbraco ModelsBuilder enabled?

• [X] Yes, it is enabled.

What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

[leekelleher]

@benjaminc Thanks for raising this. I hadn't replied sooner, but I have been thinking about this over the past few days.

I agree that it isn't ideal that the call to Enum.Parse comes first. My hesitation with moving that code (line 188) to lower down is that I wanted to delay the use of Reflection until it was needed. e.g. do a quick check/return with Enum.Parse, then fallback on Reflection to check the field attributes.

I'll have a think about if there are other workarounds to this. 🤔 If not, then I'll look to reorder the code. 👍

Gotta say, not sure if I'm impressed or shocked to hear an enum having over 2000 fields?! 😨😆 I'm curious which list-editor you are using to display them?

[benjaminc]

You will hopefully be appropriately appalled that they want to see that many in a dropdown... And I figured that was the reason for having Enum.Parse before the reflection. I also try to delay using reflection whenever possible. I've never actually done any performance metrics on it though, so maybe I'll try that out today and get back to you. For now, I've solved it by just going through and assigning int values to all the enum values that match their attribute value. It was a pain, but now that it is done it works fine.

And that may be a valid option, but it would be nice if it was a little clearer what was going on, or what I needed to do. I'll run some comparison tests today, and if Enum.Parse performs better, maybe just a docs change is all that's needed.

[benjaminc]

I got lots of good data, at the bottom, but the TLDR is that Enum.Parse is MUCH faster than the current loop, especially when you start working with large enums with lots of values. There is a better way though, as it seems that a dictionary lookup is 5-20x faster than Enum.Parse! With that in mind, I would propose changing the ConvertValue method as follows:

private static readonly ConcurrentDictionary<string, Dictionary<string, object>> _enumValues = new ConcurrentDictionary<string, Dictionary<string, object>>();
public object ConvertValue(Type type, string value)
{
   if (string.IsNullOrWhiteSpace(value) == false && type?.IsEnum == true)
   {
      var valueMap = _enumValues.GetOrAdd(type, x => {
         var fields = type.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static);
         var map = new Dictionary<string, object>(fields.Length);

         foreach (var field in fields)
         {
            var attr = field.GetCustomAttribute<DataListItemAttribute>(false);
            var attr2 = field.GetCustomAttribute<EnumMemberAttribute>(false);
            map[attr2?.Value ?? attr?.Value ?? field.Name] = Enum.Parse(type, field.Name);
         }

         return map;
      });

      if (valueMap.TryGetValue(value, out var enumValue)) return enumValue;

      // NOTE: Can't use `Enum.TryParse` here, as it's only available with generic types in .NET 4.8.
      try { return Enum.Parse(type, value, true); } catch { /* ¯\_(ツ)_/¯ */ }

#if NET472
      _logger.Debug<EnumDataListSource>($"Unable to find value '{value}' in enum '{type.FullName}'.");
#else
      _logger.LogDebug($"Unable to find value '{value}' in enum '{type.FullName}'.");
#endif
   }

   return type.GetDefaultValue();
}

Here are the test results for reference. Let me know if you are interested in the code I used, and I can share that as well.

.NET Framework 4.8
Enum with 10 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.4597142 for an average runtime of 0.000459ms or 4.597142ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:01.0632018 for an average runtime of 0.1063ms or 1063.2018ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0831061 for an average runtime of 8.3E-05ms or 0.831061ticks

Enum with 100 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.6160590 for an average runtime of 0.000616ms or 6.16059ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:05.3377078 for an average runtime of 0.5337ms or 5337.7078ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0392129 for an average runtime of 3.9E-05ms or 0.392129ticks

Enum with 1000 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.3734050 for an average runtime of 0.000373ms or 3.73405ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:49.1137151 for an average runtime of 4.9113ms or 49113.7151ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0561070 for an average runtime of 5.6E-05ms or 0.56107ticks

.NET 5.0
Enum with 10 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.1720516 for an average runtime of 0.000172ms or 1.720516ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:00.3967951 for an average runtime of 0.0396ms or 396.7951ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0410241 for an average runtime of 4.1E-05ms or 0.410241ticks

Enum with 100 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.1496173 for an average runtime of 0.000149ms or 1.496173ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:03.1624293 for an average runtime of 0.3162ms or 3162.4293ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0428021 for an average runtime of 4.2E-05ms or 0.428021ticks

Enum with 1000 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.1367134 for an average runtime of 0.000136ms or 1.367134ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:37.9425542 for an average runtime of 3.7942ms or 37942.5542ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0385279 for an average runtime of 3.8E-05ms or 0.385279ticks

.NET 6.0
Enum with 10 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.6987096 for an average runtime of 0.000698ms or 6.987096ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:00.4145160 for an average runtime of 0.0414ms or 414.516ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0333034 for an average runtime of 3.3E-05ms or 0.333034ticks

Enum with 100 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.1666437 for an average runtime of 0.000166ms or 1.666437ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:03.5440884 for an average runtime of 0.3544ms or 3544.0884ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0371926 for an average runtime of 3.7E-05ms or 0.371926ticks

Enum with 1000 values
    Starting Enum.Parse test
    Ran 1000000 iterations in 00:00:00.1517192 for an average runtime of 0.000151ms or 1.517192ticks
    Starting Attribute test
    Ran 10000 iterations in 00:00:29.8459201 for an average runtime of 2.9845ms or 29845.9201ticks
    Starting Dictionary test
    Ran 1000000 iterations in 00:00:00.0369712 for an average runtime of 3.6E-05ms or 0.369712ticks

[leekelleher]

@benjaminc Thank you for this! 🙏 (I'm taking some time away from my laptop at the moment, but wanted to acknowledge your response - and I'll look to refactor the Enum data-source code once I'm back).

[leekelleher]

@benjaminc Just to let you know that I've implemented a dictionary/cache lookup for the Enum data-source, (see commit 361ff9a28a39795e263a0bcd75d78bb1ba07f96c). The code is different to what you offered as I've incorporated the GetItems in there too, since it was already iterating over the enum fields.

This will be part of the next release, should be v3.2.0 - no timeframe on that just yet, soon-ish though.