Align property discovery convention with type mapping

[divega]

When a TEntity CLR type is passed to ModelBuilder.Entity<TEntity>() we need a mechanism to identify scalar properties and navigation properties (plus complex type/value object properties in the future) on it.

Currently PropertyDiscoveryConvention uses SharedTypeExtensions.IsPrimitive() to identify "primitive" properties on a given entity's CLR type.

Unfortunately the heuristic perpetuates the idea of a canonical set of primitive types supported by EF, while in reality type support should vary on a provider basis (e.g. while SQL Server does not support many unsigned int types, other providers do not support DateTimeOffset and some providers may have native support for types that EF does not know about, etc.) and in the long term even become extensible via user-provided custom type mappers.

One way to improve this would be to have an API that the convention can use to find out if a specific property type can be handled by the "current" type mapping (i.e. the provider's type mapping and in the future, any registered custom type mappers).

• If the type can be handled then we know it can be treated as a scalar type by EF.
• If the type cannot be handled as a scalar, we'll try to shred it into its individual components (e.g. the property could be an entity or collection of entities, etc. so we would need to reflect on it to further discover properties).
• If none of the above succeeds we would need to fail the mapping of the property.

Note that this approach assumes that at model building time we would have access to type mapping (which I am not sure is currently possible).

[ajcvickers]

One thing to keep in mind here is that even though type support varies by provider, the current fashion is to build one model to rule them all. We all know that fashions change rapidly, but assuming we continue with one model, then it means every provider should get a chance to decide where a property can be mapped or not.

We also need to decide what to do when a property can't be mapped. The behavior in the old stack and the current behavior in EF7 is to ignore properties of types that can't be mapped. I think we should probably change this because it is usually not what people want--especially for primitive/simple types. I'll file an issue for this. (Note we can still choose to ignore properties for other reasons, such as read-only properties.)

Bringing these two things together, should we fail if a given property type cannot be mapped by any of the providers for which we are building the model? Based on the one model logic (that it represents your domain and should be structurally equivalent across providers) I think we probably should.

[divega]

The behavior in the old stack and the current behavior in EF7 is to ignore properties of types that can't be mapped. I think we should probably change this because it is usually not what people want--especially for primitive/simple types.

Agreed!

should we fail if a given property type cannot be mapped by any of the providers for which we are building the model?

I like that option. I believe when we add custom type mapping customers that hit a scenario like this will be able to tweak the mapping for that specific provider so as long as they can come up with a mapping that works this will become totally ok.

[roji]

+1 for this, any chance you'll get this into the RTM?

If not, users can still use the fluent API (or annotations) to include a property with a non-standard type in the model, as long as the type mapper supports it, right?

[divega]

This issue describes what @smitpatel has done in #3387 (throwing when the property cannot be mapped as described in #2612 is only one aspect of it) so we moved it back from the backlog to rc1.

[divega]

@smitpatel there a few issues @ajcvickers raised in the PR that we need to follow up on. Can you file a new bug for those?

[smitpatel]

@divega - filed #3443
I am re-opening issue, I will write details for provider writers since this change would break them and close the issue afterwards.

[smitpatel]

Details of changes for provider writers
PropertyDiscoveryConvention adds properties from CLR type to model. InverseProperty, ForeignKeyAttribute & RelationshipDiscovery conventions need to identify navigations. Since IRelationalTypeMapper is relational concept, core still uses the same canonical set of properties to identify property/navigations. To align conventions with typemapper in relational and providers deriving from it, all those conventions have been derived in Relational namespace with appropriate function overridden so that conventions use type mapper. Since TypeMapper comes from DI, in order to avoid adding all conventions to DI, the RelationalConventionSetBuilder takes IRelationalTypeMapper from DI and initialize all above conventions with type mapper and replaces above core conventions with relational ones.
This means that provider specific ConventionSetBuilder (deriving from RelationalConventionSetBuilder) will need constructor with IRelationalTypeMapper.

To throw exception for unmapped property, new convention PropertyMappingValidationConvention has been added. It also has same set up as above and derived class in Relational.

IRelationalTypeMapper contract has been changed to facilitate above. Boolean functions - IsTypeMapped(Type clrType) & IsPropertyMapped(IProperty property) has been added. Internally in RelationalTypeMapper, FindMapping functions have been added alongside GetMapping.

For Provider writers, they need to update provider specific type mapper and convention set builder, everything else should wire up by itself.

To answer @roji 's question - User can still use Fluent API to add property with non-standard type and specify which column type to use for it. PropertyMappingValidationConvention checks that every added property is mapped by type mapper or not. As long as type mapper can support the property through clr type or user specified column type, it would work. Last PR missed adding property through data-annotations with column type specified. Sending out PR for it shortly.

[roji]

Thanks for the details @smitpatel!

[smitpatel]

@roji - Regarding your question - We decided today that CLR types which are not supported by type mapper cannot be added to the model. (even if there is column type specified since conversion from CLR type to specified column type and vice-versa is not known to EF). So for any non-standard type, as long as type mapper says that such CLR type can be mapped to the database, it can be added.

[roji]

OK, sounds good. I'll be re-implementing the Npgsql type mapper very soon and will post if there are any difficulties.