2.1.0 can't do Value Conversions on complex IEnumerable types like JObject (newtonsoft)

[NinoFloris]

It seems any complex property type still goes through all the discover relationships machinery even though you explicitly call the new 2.1 value converter api HasConversion(converter).

For example try to set up a value conversion for Newtonsofts JObject as this one implements many IEnumerable derived interfaces that will trigger this DiscoverRelationships codepath.

Leading to beautifully vague stacktraces and completely blocking us from using this new api

 System.Reflection.AmbiguousMatchException : Ambiguous match found.
Stack Trace:
   at System.RuntimeType.GetPropertyImpl(String name, BindingFlags bindingAttr, Binder binder, Type returnType, Type[] types, ParameterModifier[] modifiers)
   at System.Type.GetProperty(String name, BindingFlags bindingAttr)
   at System.SharedTypeExtensions.GetPropertiesInHierarchy(Type type, String name)+MoveNext()
   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
   at System.Reflection.PropertyInfoExtensions.FindSetterProperty(PropertyInfo propertyInfo)
   at System.Reflection.PropertyInfoExtensions.IsCandidateProperty(PropertyInfo propertyInfo, Boolean needsWrite, Boolean publicOnly)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.PropertyDiscoveryConvention.IsCandidatePrimitiveProperty(PropertyInfo propertyInfo)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.PropertyDiscoveryConvention.Apply(InternalEntityTypeBuilder entityTypeBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ImmediateConventionScope.OnEntityTypeAdded(InternalEntityTypeBuilder entityTypeBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.RunVisitor.VisitOnEntityTypeAdded(OnEntityTypeAddedNode node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionVisitor.VisitConventionScope(ConventionScope node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionVisitor.VisitConventionScope(ConventionScope node)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ConventionBatch.Run()
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.RelationshipDiscoveryConvention.DiscoverRelationships(InternalEntityTypeBuilder entityTypeBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ImmediateConventionScope.OnEntityTypeAdded(InternalEntityTypeBuilder entityTypeBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.Model.AddEntityType(EntityType entityType)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalModelBuilder.Entity(TypeIdentity& type, ConfigurationSource configurationSource, Boolean throwOnQuery)

[ajcvickers]

See also #12229

[ajcvickers]

Depending on the complexity and risk of the fix, we may consider this for a patch release since converting JObject seems like it will be a common request. Putting in 2.1.3 at least until we have the PR. /cc @AndriySvyryd

[AndriySvyryd]

@NinoFloris As a workaround you can put a [NotMapped] attribute on the JObject property. This will prevent it from being discovered until the value converter is set.

[NinoFloris]

Thanks for the quick responses!

@AndriySvyryd So NotMapped is specifically not applicable for ValueConversions? Sounds like a thing to have in the docs as I would intuitively think NotMapped == never touched by EF.

EDIT:
Or is it because I have an explicit configuration for HasConversion on that property that NotMapped won't hold in that codepath? (i.e. the properties aren't being filtered on NotMapped early on, but granularly by each EF feature)

[ajcvickers]

@NinoFloris Yes, the NotMapped is just a workaround that relies on the principal that fluent API overrides data annotations. So the property is mapped ultimately, but NotMapped stops it from being treated as an entity type in discovery before the mapping happens.

[ajcvickers]

This is approved for 2.1.3. Do not merge yet; branch is expected to open Monday.

Given these issues have been parked for some time, please be careful to ensure the correct commits get into the correct branches.

[ajcvickers]

@AndriySvyryd This issue is approved for patch and the release\2.1 branch is now open for merging. Please ensure:

• The appropriate changes get into both the release and dev branches
• Quirking is included for the release branch only