dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.78k stars 3.19k forks source link

Mechanism/API to specify a default conversion for any property of a given type in the model #10784

Closed ajcvickers closed 3 years ago

ajcvickers commented 6 years ago

Value conversions were introduced by #242. Currently conversions are only set per-property, although bulk configuration can be used at the end of OnModelCreating in the normal way. This issue is about adding some kind of mechanism to set the conversion for all properties of a given type in the model. This could be through configuration on the model, or it could be via improved bulk config APIs.

ajcvickers commented 6 years ago

Note that the converter isn't really for "every property" even though it will also have that effect. It should also be for any time the CLR type is used in the query--that is, it is really a custom type mapping setup by the application.

bugproof commented 6 years ago

As a temporary workaround you can use this extension method(I think it works):

public static class ModelBuilderExtensions
{
    public static ModelBuilder UseValueConverterForType<T>(this ModelBuilder modelBuilder, ValueConverter converter)
    {
        return modelBuilder.UseValueConverterForType(typeof(T), converter);
    }

    public static ModelBuilder UseValueConverterForType(this ModelBuilder modelBuilder, Type type, ValueConverter converter)
    {
        foreach (var entityType in modelBuilder.Model.GetEntityTypes())
        {
            // note that entityType.GetProperties() will throw an exception, so we have to use reflection 
            var properties = entityType.ClrType.GetProperties().Where(p => p.PropertyType == type);
            foreach (var property in properties)
            {
                modelBuilder.Entity(entityType.Name).Property(property.Name)
                    .HasConversion(converter);
            }
        }

        return modelBuilder;
    }
}
ajcvickers commented 6 years ago

@ZeroNightzz What exception is thrown by entityType.GetProperties()?

bugproof commented 6 years ago

@ajcvickers

System.InvalidOperationException: The property 'CurrencyExchangeRate.Currency' could not be mapped, because it is of type 'Currency' which is not a supported primitive type or a valid entity type. Either explicitly map this property, or ignore it using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating'.
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.PropertyMappingValidationConvention.Apply(InternalModelBuilder modelBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ImmediateConventionScope.OnModelBuilt(InternalModelBuilder modelBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.OnModelBuilt(InternalModelBuilder modelBuilder)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.Model.Validate()
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.CreateModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.<>c__DisplayClass5_0.<GetModel>b__1()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.GetModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.CreateModel()
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.get_Model()
   at Microsoft.EntityFrameworkCore.Infrastructure.EntityFrameworkServicesBuilder.<>c.<TryAddCoreServices>b__7_1(IServiceProvider p)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitFactory(FactoryCallSite factoryCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine.<>c__DisplayClass1_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.GetService(Type serviceType, ServiceProviderEngineScope serviceProviderEngineScope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at Microsoft.EntityFrameworkCore.DbContext.get_DbContextDependencies()
   at Microsoft.EntityFrameworkCore.DbContext.get_InternalServiceProvider()
   at Microsoft.EntityFrameworkCore.DbContext.Microsoft.EntityFrameworkCore.Infrastructure.IInfrastructure<System.IServiceProvider>.get_Instance()
   at Microsoft.EntityFrameworkCore.Internal.InternalAccessorExtensions.GetService[TService](IInfrastructure`1 accessor)
   at Microsoft.EntityFrameworkCore.Infrastructure.AccessorExtensions.GetService[TService](IInfrastructure`1 accessor)
   at Microsoft.EntityFrameworkCore.Design.Internal.DbContextOperations.CreateContext(Func`1 factory)
   at Microsoft.EntityFrameworkCore.Design.Internal.DbContextOperations.CreateContext(String contextType)
   at Microsoft.EntityFrameworkCore.Design.Internal.MigrationsOperations.AddMigration(String name, String outputDir, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.AddMigrationImpl(String name, String outputDir, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.AddMigration.<>c__DisplayClass0_1.<.ctor>b__0()
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.<>c__DisplayClass3_0`1.<Execute>b__0()
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action)
The property 'CurrencyExchangeRate.Currency' could not be mapped, because it is of type 'Currency' which is not a supported primitive type or a valid entity type. Either explicitly map this property, or ignore it using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating'.

this is caused, so I have to use GetProperties() by accessing ClrType. I think the conversion must be known before using GetProperties() defined in EF.

ajcvickers commented 6 years ago

@Necronux Thanks. I thought you were indicating an exception when calling the GetProperties method itself.

AndriySvyryd commented 5 years ago

The bulk configuration should be performed before set discovery as it could affect whether a property would be considered a navigation. See https://github.com/dotnet/efcore/issues/12229

ajcvickers commented 5 years ago

Consider allowing converters to be registered only for a given provider, or only for cases where the provider doesn't have built-in support for the type being converted. See #14319

kskalski commented 5 years ago

Is there a way to use converters (or other mechanism) to map property (or in general its type) to a navigational property (basically what using ICollection<> would provide)?

My use-case is, I would like to have a property that is an IDictionary, which acts as navigational property containing related entities accessible by their primary key. The simple approach like below doesn't seem to work, I guess it would require re-discovering nagivational properties after their type was mapped by conversions:

    class Garden {
      public IDictionary<long, Flower> Flowers { get; set; }
    }
...
    protected override void OnModelCreating(ModelBuilder modelBuilder) {
      modelBuilder.Entity<Garden>()
        .Property(m => m.Flowers)
        .HasConversion(v => v.Values, v => v.ToDictionary(flower => flower.Id, flower => flower));
    }

A dedicated API to support IDictionary or other bags would probably allow for more efficient implementation, I guess this issue https://github.com/aspnet/EntityFrameworkCore/issues/2919 was created with that in mind. But I'm curious if there is any way available right now.

ajcvickers commented 5 years ago

@kskalski I'm not aware of any way to do this now; as you said, #2919 is tracking this.

BjarkeMeier commented 5 years ago

@backdoormanUC It works like a charm. Thx for sharing! I made a few small extensions to target my personal needs:

public static class ModelBuilderExtensions
  {
    /// <summary>
    ///   Based on BackDoorManUC's suggestion: https://github.com/aspnet/EntityFrameworkCore/issues/10784#issuecomment-415769754.
    /// </summary>
    public static ModelBuilder UseValueConverterForType<T>(this ModelBuilder modelBuilder, ValueConverter converter, string sqlType = null, bool useTypeNameSuffix = false)
    {
      return modelBuilder.UseValueConverterForType(typeof(T), converter, sqlType, useTypeNameSuffix);
    }

    public static ModelBuilder UseValueConverterForType(this ModelBuilder modelBuilder, Type type, ValueConverter converter, string sqlType = null, bool useTypeNameSuffix = false)
    {
      foreach (var entityType in modelBuilder.Model.GetEntityTypes())
      {
        // note that entityType.GetProperties() will throw an exception, so we have to use reflection 
        var properties = entityType.ClrType.GetProperties().Where(p => p.PropertyType == type);
        foreach (var property in properties)
        {
          var prop = modelBuilder.Entity(entityType.Name).Property(property.Name);
          prop.HasConversion(converter);
          if (sqlType != null)
            prop.HasColumnType(sqlType);
          if (useTypeNameSuffix)
            // Apparantly property.Name doesn't provide the current property name
            prop.HasColumnName($"{prop.Metadata.Relational().ColumnName}_{type.Name}");
        }
      }

      return modelBuilder;
    }
  }
ghost commented 4 years ago

@bugproof Solution works very well! But, It still needed a check for properties without setter and I changed it a bit to allow a even simpler syntax for decimal to double conversions. Besides note that it should be called twice one for the normal types and another for the nullable types:

This is the modified extension method if someone finds useful:

public static ModelBuilder UseGlobalConverter<T, B>(this ModelBuilder modelBuilder, ValueConverter? converter = null) {
            foreach (var entityType in modelBuilder.Model.GetEntityTypes()) {
                var propertiesInfo = entityType.ClrType.GetProperties().Where(p => p.PropertyType == typeof(T) && p.GetSetMethod() != null);
                foreach (var propertyInfo in propertiesInfo) {
                    if (converter == null) {
                    modelBuilder.Entity(entityType.Name).Property(propertyInfo.Name).HasConversion<B>();
                    } else {
                    modelBuilder.Entity(entityType.Name).Property(propertyInfo.Name).HasConversion(converter);
                    }
                }
            }
       return modelBuilder;
}

An you use it in OnModelCreating() like this:

modelBuilder.UseGlobalConverter<decimal?, double?>();
modelBuilder.UseGlobalConverter<decimal, double>();
ajcvickers commented 4 years ago

@dotnet/efcore and @AndriySvyryd in particular

API Proposal

The idea here is to create a new top-level method on ModelBuilder that will allow configuration of defaults for all properties of a given type. (As mentioned above, this will also apply to naked instances of the type in a query, so it's really about changing the type mapping, but conceptually it's easier to think about as setting defaults for a property type.)

The API mirrors the Property builder API since it is about setting defaults for properties. I went through all the property builder methods that we currently have and these seem to make sense for defaults:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.PropertyType<string>(b =>
    {
        b.HasAnnotation("", "");
        b.HasConversion<byte[]>(); // etc.
        b.IsRequired();
        b.IsUnicode();
        b.HasMaxLength(42);
        b.HasValueGenerator<GuidValueGenerator>();
        b.IsConcurrencyToken();
        b.IsRowVersion();
        b.ValueGeneratedNever(); // etc.
        b.UsePropertyAccessMode(PropertyAccessMode.Field);

        // Relational
        b.HasColumnType("");
        b.IsFixedLength();
        b.HasDefaultValueSql("");
        b.HasComputedColumnSql("");
        b.HasDefaultValue(0);

        // Cosmos
        b.IsEtagConcurrency();

        // Sqlite
        b.HasSrid(0);
        b.HasGeometricDimension("z");

        // SQL Server
        b.UseHiLo();
        b.HasHiLoSequence();
        b.UseIdentityColumn();
        b.HasIdentityColumnSeed();
        b.HasIdentityColumnIncrement();
    });
}

I have not included

        b.ToJsonProperty("");
        b.HasField("");
        b.HasColumnName("");
        b.HasViewColumnName("");
        b.HasComment("");

because these seem very specific to individual properties.

Thoughts?

AndriySvyryd commented 4 years ago

This configuration can potentially impact a lot of conventions, so they would need to be rerun after this is applied. Also it itself doesn't depend on anything being in the model already, so this would be much better suited to pre-convention configration

vslee commented 4 years ago

If it's not too much trouble, maybe the property builders on the bottom can also be included. They might be used in some rare cases. Such as if someone wanted to put a comment on all DateTime columns, saying that all DateTimes are stored as UTC on the server. Or if they used views to combine columns for all instances of a particular type.

ajcvickers commented 4 years ago

@AndriySvyryd I was thinking the same thing. I was thinking we can document and enforce that these must be specified at the beginning of OnModelCreating.

AndriySvyryd commented 4 years ago

@ajcvickers That's too late, OnModelInitialized would've run already. We need a separate method.

ajcvickers commented 4 years ago

@vslee I think that's a valid option. You don't think it would be confusing to see, for example, HasField here?

ajcvickers commented 4 years ago

@AndriySvyryd Hmm. Good point.

AndriySvyryd commented 4 years ago

@ajcvickers Re: less-general configuration I agree with @vslee that there can be valid cases for all of them, but we don't need to implement everything in one go. We can start with just the most common ones.

vslee commented 4 years ago

I wouldn't personally be using HasField, but playing devil's advocate I could imagine, say, a large code generated set of classes which need a little bit of tweaking later. This might be done more easily with this than trying to go through to fix every little thing. Deferring some implementations to later is fine!

smitpatel commented 4 years ago

Can put something hanging off OnConfiguring method? or OnModelInitializing?

ajcvickers commented 4 years ago

@smitpatel OnConfiguring runs too often.

AndriySvyryd commented 4 years ago

There's nothing bad about having an extra protected method on DbContext

smitpatel commented 4 years ago

OnModelInitializing

sjb-sjb commented 4 years ago

One problem with the workarounds specified above is that the ClrType may have properties that are not in the model. I just put in the following and so far it seems to be working for me (at least for the 10 minutes since I first compiled it).

    static public void SetValueConverters<TProperty>(this ModelBuilder modelBuilder, ValueConverter converter) where TProperty: struct
    {
        foreach (var entityType in modelBuilder.Model.GetEntityTypes()) {
            foreach (IMutableProperty property in entityType.GetProperties()) {
                if (property.ClrType == typeof(TProperty) || property.ClrType == typeof(Nullable<TProperty>)) {
                    property.SetValueConverter(converter);
                }
            }
        }
    }

I am calling this after calling base.OnModelCreating( modelBuilder).

vslee commented 4 years ago

A future enhancement might be to add overloads for even more configurability. For example:

IsRequired(Func<IPropertyBase,bool> predicate)
HasComment(Func<IPropertyBase,string> selector)

Used as:

b.IsRequired(p => p.Name.StartsWith('a')) // only make properties starting with 'a' required
b.HasComment(p => $"{p.Name} is a special column") // ability to use the property name in the comment
ajcvickers commented 4 years ago

@vslee That's an interesting idea. Probably deserves its own issue as I doubt it will make it into the initial implementation.

ajcvickers commented 4 years ago

Spent some more time investigation this. I believe at this point we should punt this for 5.0 for two reasons:

AndriySvyryd commented 4 years ago

Ideally, TypeMappingSource would return the correct mapping for a given type after this change.

There's of course another option, where the default conversion doesn't affect what TypeMappingSource returns, instead it's just a default for the properties in the model. There aren't many places where we need the TypeMapping of a type without having an associated property and for those scenarios we can consider adding alternative APIs to specify it (#4978. #21386).

vanillajonathan commented 3 years ago

@AndriySvyryd Could you explain how to configure EF Core to use a default conversion for any property of a given type in the model?

roji commented 3 years ago

@vanillajonathan this new feature was just merged, and will only be available in 6.0 previews. For 5.0, there's a good chance you can do something similar to https://github.com/dotnet/efcore/issues/10784#issuecomment-415769754 as a workaround.

AndriySvyryd commented 3 years ago

For EF Core 6.0.0-preview6:

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<string>().HaveConversion<byte[]>()
}
julielerman commented 3 years ago

Hi @AndriySvyryd , I'm tring to use the HaveConversion method to pass in the two transofmration lambdas ala

configurationBuilder.Properties().HaveConversion(c=>c.ToString(),s=>Color.FromName(s));

but the compiler is complaining. I looked at source and don't see an overload that matches the signature I use to do this with HasConversion:

(Expression<Func<TProperty, TProvider>> convertToProviderExpression, Expression<Func<TProvider, TProperty>> convertFromProviderExpression);

Is this coming in EF Core 6? Thanks

NOTE: I can achieve this by creating my own ValueConverter class. it's just the handy shortcut that's missing for now. :)

ajcvickers commented 3 years ago

@julielerman You need to create an actual ValueConverter class when using it for all properties like this. This isn't something that is planned to change.

julielerman commented 3 years ago

Thanks. Yeah I figured that out (I think I wrote my edit at the same time ou wrote this response. LOL. The expression overload would certainly be handy though....someday.

AndriySvyryd commented 3 years ago

@julielerman That overload wouldn't be compatible with compiled models, so we discourage using it (by not providing it in the new API)

julielerman commented 3 years ago

ohhh! Right. Now have to keep compiled models in mind for all the configs! Thanks @AndriySvyryd

vanillajonathan commented 2 years ago

The value conversions article needs to be updated.

Dynalon commented 2 years ago
protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<string>().HaveConversion<byte[]>()
}

As @vanillajonathan outlined, this really should go into the value conversions article for documentation purposes. The only mention to that addition I could find was the 6.0.0-preview6 release blog article, and it took me a while to find it - even so it is perfectly what I needed.

roji commented 2 years ago

@Dynalon @vanillajonathan I've added a bulk configuration section to the value conversions page in https://github.com/dotnet/EntityFramework.Docs/pull/3716.

thomaslevesque commented 2 years ago

@Dynalon @vanillajonathan I've added a bulk configuration section to the value conversions page in #3716.

Wrong link? #3716 doesn't seem to be related to this..

roji commented 2 years ago

Sorry, wrong repo - corrected above.

ronnyek commented 2 years ago

Is there a decent workaround for efcore versions < 6? We dont have the ability to upgrade to .net 6 just yet, and have a domain model of thousands of tables that have used the old oracle convention of using bool as char(1) with constraints to Y/N. Something a value converter exists for, but is gonna make mapping this entities a royal nightmare.

roji commented 2 years ago

@ronnyek take a look at Bulk configuration in OnModelCreating, that may work for you. Note also that EF Core 5.0 goes out of support in May, and 3.1 in December - so I'd recommend upgrading soon instead.

igbenic commented 1 year ago

Hi, I don't know if this is the right place to ask. We use conversion on our models and it works perfectly, but is it possible to have this kind of conversion for SQL procedure parameters as well? We are working with multiple DBs from a single server, and we have a specific requirement to store DateTime in each DBs local timezone but internally we are working with UTC for all DateTimes. For models it works globally, for procedures we are still manually adding code for converting to local.

igbenic commented 1 year ago

Hi, I don't know if this is the right place to ask. We use conversion on our models and it works perfectly, but is it possible to have this kind of conversion for SQL procedure parameters as well? We are working with multiple DBs from a single server, and we have a specific requirement to store DateTime in each DBs local timezone but internally we are working with UTC for all DateTimes. For models it works globally, for procedures we are still manually adding code for converting to local.

Sorry, solved with interceptors using the same ValueConverter for IN/OUT parameters.