EnumerableConverter fails with inherited class

[leekelleher]

An issue has been flagged for when attempting to map a property type that inherits from a generic enumerable.

Background on the issue can be found here: https://our.umbraco.org/projects/developer-tools/ditto/ditto-feedback/79244-error-on-using-relatedlink-with-ditto-and-jeavon-property-converter#comment-257744

The culprit is here: EnumerableConverterAttribute.cs, line 76 As the object-type is detected as an enumerable type (line 40), but when the GenericTypeArguments are accessed (line 76), the top-level type is accessed, not the base type.

These unit-tests attempt to test the processor directly, (as opposed to going via the .As<T> method. The first test is successful, as it is working with a generic enumerable type directly. The second test fails, as it is working with a class inherited from a generic enumerable type.

[JimBobSquarePants]

@leekelleher Interesting... Reading through the issue it does seem like the PropertyValueConverter library should not be returning null. That would fix the issue yeah?

If so I don't really want to mess around with our code as returning null instead of an empty enumerable is considered poor practise and I don't want to be hacking around to deal with that.

[leekelleher]

@JimBobSquarePants I agree that the PropertyValueConverter shouldn't return a null. 👍

As for the isolated unit-test itself, would we expect that to work or not?

If an object inherits from a generic enumerable type, should line 40 detect that it is an IEnumerable? and then should line 76 be able to find the first GenericTypeArguments type?

If the answer is yes, then I guess there's another question about how deep down the inheritance should we check?

[JimBobSquarePants]

Ok @leekelleher... So I've finally had the chance to put some thought to this.

The returned value should be null but we should be able to correctly detect and return that value without throwing.

I've updated the PR with my updated fix.

Basically the logic is as follows.

1. If the return type is IEnumerable<T> we can cast and return.
2. If the return type implements IEnumerable<T> and has an empty constructor we can create an instance and return.
3. If the return type implements IEnumerable<T> and does not have an empty constructor we return null.

BTW Having done this digging I finally fixed #176 also.

We now need to do a PR in Umbraco-Core-Property-Value-Converters to add an empty constructor and return an new empty instance instead of null.

[JimBobSquarePants]

@leekelleher I added a test that initially failed in the PR 😄

https://github.com/leekelleher/umbraco-ditto/pull/195/files#diff-dcacc48ee99c56708eba88c3efecd08bR90

[leekelleher]

@JimBobSquarePants Bad news, it turns out that our AppVeyor hasn't been running our unit-tests for the past 11 months, (because we renamed the assembly and didn't update the appveyor.yml), anyhooo ... it turns out that there's a few failing unit-tests...

• EnumerableConverter_EnumerableModelEmptyDictionary_Test
• EnumerablesCast
• GenericDictionaryPropertyIsNotDetectedAsCastableEnumerable

I was going to start taking a look at them, but it's getting pretty late tonight (UK time) ... and you'll probably a better idea of how to tackle the Dictionary<K, V> stuff 😼

[JimBobSquarePants]

Really? I ran them all locally and everything was passing. Will have a look.

EDIT I'm blaming resharpers test runner, I think it missed something.

[JimBobSquarePants]

EnumerableDetectionTests.GenericDictionaryPropertyIsNotDetectedAsCastableEnumerable

By the name of the method directly contradicts with

TypeInferenceTests.TestIsCastableEnumerableType

We're expecting not null with the first but false in the second.

The source for the latter deliberately excludes dictionary since we'd have to go further down the rabbit hole to discover what the subtypes are. My mantra was, if you are expecting a dictionary then you are using a PropertyValueConverterwhich would return a dictionary type which then will automatically cast to IEnumerable.

So the test doesn't really describe the actual running functionality (Our tests badly need comments, it's really hard to see what's being tested)

public static bool IsCastableEnumerableType(this Type type)
{
    // String, though enumerable have no generic arguments.
    // Types with more than one generic argument cannot be cast. 
    // Dictionary, though enumerable, requires linq to convert and shouldn't be attempted anyway.
    return type.IsEnumerableType() && type.GenericTypeArguments.Any()
            && type.GenericTypeArguments.Length == 1
            && type.TryGetElementType(typeof(IDictionary<,>)) == null;
}

I originally wrote it nearly 450 days ago so something has definitely got lost in translation when we migrated from the type converters as it must not have been an issue before.

Sooooo.... Do we try to figure it out?

Update

Ok... So I've fixed the Dictionary stuff so we can now cast that to our heart's content.

Interestingly I get a failing test locally for Factory_Resolves with the following stacktrace. I must have something missing on my machine.

Also the generic invocation of the plugin manager needs caching. We could use the CachedInvocations tools to do that but I can't run or test that with my failing test.

Test Name:  Factory_Resolves
Test FullName:  Our.Umbraco.Ditto.Tests.DittoFactoryTests.Factory_Resolves
Test Source:    C:\github\umbraco-ditto\tests\Our.Umbraco.Ditto.Tests\DittoFactoryTests.cs : line 82
Test Outcome:   Failed
Test Duration:  0:00:00.168

Result StackTrace:  
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
at Our.Umbraco.Ditto.DittoFactoryAttribute.ProcessValue() in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\ComponentModel\Processors\DittoFactoryAttribute.cs:line 99
at Our.Umbraco.Ditto.DittoCacheableAttribute.GetCacheItem[TOuputType](DittoCacheContext cacheContext, Func`1 refresher) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\ComponentModel\Attributes\DittoCacheableAttribute.cs:line 58
at Our.Umbraco.Ditto.DittoProcessorAttribute.ProcessValue(Object value, DittoProcessorContext context) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\ComponentModel\Attributes\DittoProcessorAttribute.cs:line 110
at Our.Umbraco.Ditto.PublishedContentExtensions.DoGetProcessedValue(IPublishedContent content, CultureInfo culture, Type targetType, PropertyInfo propertyInfo, PropertyDescriptor propertyDescriptor, Type defaultProcessorType, IEnumerable`1 processorContexts) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\Extensions\PublishedContentExtensions.cs:line 503
at Our.Umbraco.Ditto.PublishedContentExtensions.GetProcessedValue(IPublishedContent content, CultureInfo culture, Type targetType, PropertyInfo propertyInfo, Object instance, Type defaultProcessorType, IEnumerable`1 processorContexts) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\Extensions\PublishedContentExtensions.cs:line 421
at Our.Umbraco.Ditto.PublishedContentExtensions.ConvertContent(IPublishedContent content, Type type, CultureInfo culture, Object instance, IEnumerable`1 processorContexts, Action`1 onConverting, Action`1 onConverted) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\Extensions\PublishedContentExtensions.cs:line 370
at Our.Umbraco.Ditto.PublishedContentExtensions.As(IPublishedContent content, Type type, CultureInfo culture, Object instance, IEnumerable`1 processorContexts, Action`1 onConverting, Action`1 onConverted) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\Extensions\PublishedContentExtensions.cs:line 220
at Our.Umbraco.Ditto.PublishedContentExtensions.As[T](IPublishedContent content, CultureInfo culture, T instance, IEnumerable`1 processorContexts, Action`1 onConverting, Action`1 onConverted) in C:\github\umbraco-ditto\src\Our.Umbraco.Ditto\Extensions\PublishedContentExtensions.cs:line 69
at Our.Umbraco.Ditto.Tests.DittoFactoryTests.Factory_Resolves() in C:\github\umbraco-ditto\tests\Our.Umbraco.Ditto.Tests\DittoFactoryTests.cs:line 101
--FileNotFoundException
at System.Reflection.RuntimeAssembly._nLoad(AssemblyName fileName, String codeBase, Evidence assemblySecurity, RuntimeAssembly locationHint, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean throwOnFileNotFound, Boolean forIntrospection, Boolean suppressSecurityChecks)
at System.Reflection.RuntimeAssembly.nLoad(AssemblyName fileName, String codeBase, Evidence assemblySecurity, RuntimeAssembly locationHint, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean throwOnFileNotFound, Boolean forIntrospection, Boolean suppressSecurityChecks)
at System.Reflection.RuntimeAssembly.InternalLoadAssemblyName(AssemblyName assemblyRef, Evidence assemblySecurity, RuntimeAssembly reqAssembly, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean throwOnFileNotFound, Boolean forIntrospection, Boolean suppressSecurityChecks)
at System.Reflection.Assembly.Load(AssemblyName assemblyRef)
at Umbraco.Core.TypeFinder.<.cctor>b__21()
at System.Lazy`1.CreateValue()
at System.Lazy`1.LazyInitValue()
at System.Lazy`1.get_Value()
at Umbraco.Core.TypeFinder.GetFilteredAssemblies(IEnumerable`1 excludeFromResults, String[] exclusionFilter)
at Umbraco.Core.TypeFinder.GetAssembliesWithKnownExclusions(IEnumerable`1 excludeFromResults)
at Umbraco.Core.PluginManager.get_AssembliesToScan()
at Umbraco.Core.PluginManager.<>c__DisplayClass3d`1.<ResolveTypes>b__3c()
at Umbraco.Core.PluginManager.LoadViaScanningAndUpdateCacheFile[T](TypeList typeList, TypeResolutionKind resolutionKind, Func`1 finder)
at Umbraco.Core.PluginManager.ResolveTypes[T](Func`1 finder, TypeResolutionKind resolutionType, Boolean cacheResult)
at Umbraco.Core.PluginManager.ResolveTypes[T](Boolean cacheResult, IEnumerable`1 specificAssemblies)
--FileNotFoundException
Result Message: 
System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
  ----> System.IO.FileNotFoundException : Could not load file or assembly 'System.Net.Http.Formatting, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.
  ----> System.IO.FileNotFoundException : Could not load file or assembly 'System.Net.Http.Formatting, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.

[leekelleher]

@JimBobSquarePants Are you happy for me to merge this in? or did you need help figuring out why the Factory_Resolves test fails? (we could open as a separate issue)

💯 I agree about commenting our unit-tests... same old story, at the time of writing the test I think it makes sense, then later forget what it does. I also think we have a lot of duplication in our tests - but I'm hesitant to refactor/remove any at the moment.

[JimBobSquarePants]

Yeah, very happy to merge. The other issue was thrown by umbraco not us so I'll put that aside for now.

Let's definitely not remove tests just do a little tidy up with comments/formatting.

[JimBobSquarePants]

Yay!