Value Converters: Enable passing in some context

[HaoK]

Needed to enable: https://github.com/aspnet/Identity/issues/1753

Today value converters are only given the value, it would be nice to be able to flow context, so like in the identity case, we would be able to flow the entire user entity.

cc @blowdart this potentially would help for your non stable encryption as well right?

[blowdart]

It'd make it better to make decisions on yes, at the core (pun intended) I'd like to know what the field is that I'm about to play with.

[HaoK]

So maybe Convert(value, user, "property")?

[ajcvickers]

Consider also here the case where conversion needs to call some async method.

[divega]

I wonder if enabling async conversions would open a pit of failure. These conversions tend to be very granular, and this would introduce latency for each of them.

[roji]

@divega there's a possible scenario where you'd only occasionally need to do I/O, i.e. to fetch the new key or whatever, so 99% of invocations would return synchronously. That's a bit like how my mental model currently looks like for SQL Server access tokens.

But it's true there's a danger of a pit of failure where hapless users start doing networking on each and every value read/write.

[ajcvickers]

@divega @roji Agreed that allowing value converters to do async is likely a pit of failure. My intention was not to imply we should do this, but rather that we should consider the scenario and see if we can come up with something that makes sense in this kind of case. At the moment I don't have any great ideas, so really for now this is just something to think about when I get to working on this.

[StevenRasmussen]

@divega @roji @ajcvickers - as a user of EF Core and and a heavy believer in async I would love to have this option. For sure there is a potential to kill an app if you are doing any sort of heavy lifting or IO operations but I do still think that there is value in providing this as an option. The potential for doing IO and heavy operations still exist in the synchronous version, it's maybe less in-your-face in an async operation and thus maybe some of the hesitation here. Maybe just adding a warning would suffice when using the async version? Anyway, just my 2 cents ;)

[roji]

@StevenRasmussen do you have a specific scenario in mind that this would enable?

[StevenRasmussen]

@roji - I could potentially see a scenario where a setting or some other type of configuration would need to be retrieved in order to determine how something might be converted. This information might be in a file, DB or retrieved from an endpoint. This would ideally use an async API method call but then cache the result so that further calls to retrieve this information would not take the IO hit. So the responsibility to make the async call performant (by caching or other methods) is on the developer but it would be nice not to have to use the synchronous versions of existing API calls, or in some instances supply a custom synchronous version just for this purpose.

[twsouthwick]

For the async version, what about having ways to hook into the .ToListAsync() resolution. Not sure how well that would work since it's a static method, but thought I'd throw it out there.

The scenario I've seen (which triggered @ajcvickers to add async to this thread) is a user who needs to decrypt certain fields via a separate service. They're currently doing this via a ValueConverter and calling .Result in there which is of course problematic for performance reasons. If there was a way to intercept the result that .ToListAsync returns and potentially modify values there would be ideal to allow easy batching.

[roji]

@StevenRasmussen unless I'm mistaken, if the information/configuration in question doesn't change during the lifetime of the application, it could probably just be loaded and passed to the constructor of a custom ValueConverter. An actual async API would be needed only if a value needs to be fetched during value conversion, i.e. if the value changes from time to time (or every time).

[divega]

there's a possible scenario where you'd only occasionally need to do I/O...

The potential for doing IO and heavy operations still exist in the synchronous version..

Fair enough. Maybe it is not even a concern because the effects are obvious.

[ajcvickers]

I've been converging on the same conclusion. However, I do think we need to consider this carefully because:

• Async is viral; we need to understand how deep the async will have to go
• Once there is an async method, people often use it when they don't need to--for example, AddAsync. So we should make sure that we either effectively guide people away from this, or conclude that using when not needed is okay--i.e. doesn't cause unreasonable perf changes or other issues.

[marchy]

Could we split this out and at least have the non-async version adopted?

We have a lot of scenarios where we need to access an object mapper from DI in order to do the conversions.

Our current / EF6-age implementations for a single property is horrendous:

In model object:

private string? _whenRaw; // NOTE: Field name must be different than property for the materialization code to hit the property not directly the field (at least as of EF6)
internal string? DALWhen {
	get => _whenRaw;
	set {
		_whenRaw = value;

		// deserialize
		WhenDBO whereDBO = value.NullOr( JsonConvert.DeserializeObject<WhenDBO> );
		_when = whereDBO.NullOr( _ => DomainServiceLocator.Mapper.Map<When>( _ ));
	}
}
private When? _when;
public When? When {
	get => _when;
	set {
		_when = value;

		// serialize
		WhenDBO whenDBO = DomainServiceLocator.Mapper.Map<When, WhenDBO>( When );
		_whenRaw = JsonConvert.SerializeObject( whenDBO );
	}
}

where what we would ideally like is:

public When? When { get; private set; }

and then be able to move the object mapping into the converter:

@event.Property( _ => _.When )
	.HasConversion(
		model => {
			WhenDBO whenDBO = DomainServiceLocator.Mapper.Map<When, WhenDBO>( When );
			return = JsonConvert.SerializeObject( whenDBO );
		}
		raw => {
			WhereDBO whereDBO = value.NullOr( JsonConvert.DeserializeObject<WhereDBO> );
			return whereDBO.NullOr( _ => DomainServiceLocator.Mapper.Map<Where>( _ ));
		}));

In this case we need to access the DI container to resolve our object mapper (DomainServiceLocator.Mapper).

This problem/pattern applies to all our JSON-mapped fields which contain any sort of complex objects – so it's quite far-reaching impact-wise.

NOTE: We are aware of the owned entity types pattern however that approach is only well-suited for really simple object clusters (ie: flat fields only, not hierarchical and not full Swift-like 'associated enums' / ie: mini class hierarchies mapped in JSON via a discriminator). These limitations push towards JSON mapping unless we need to specifically query for such fields, which in most cases we don't, and can still be achieved (perhaps slightly less performantly) with a CONTAINS("%Field='Value'%") query against a string JSON field.

[KKamal87]

I also need value conversions based on another database column value. I need to encrypt/decrypt column data based on a flag another column (bool column in the same record). Issue explained below -

https://forums.asp.net/p/2176350/6338967.aspx?p=True&t=637571897875400177

[roji]

@KKamal87 that is probably covered by #13947

[cherchyk]

access to context can help with multilanguage support.
conversion method can access current culture and translate string values

[papa-pep]

If I were able to access the DbContext instance, that would prevent me from needing to implement a custom IModelCacheKeyFactory when using different encryption keys for different tenant databases. That ends up eating a lot of memory :(. Making copies of the model cache is completely unnecessary in my case if I can get some additional context into the value converter.

[EmperorArthur]

I have hacked around this problem by creating a custom ValueConverter and then doing new, attach during model creation.

However, this makes using EntityTypeBuilder more difficult, and I am pretty sure results in the context that built the model living forever.

In my case, I need the DB server's TimeZone (Not the Servers) to convert between DateTime and DateTimeOffset.

[soroshsabz]

ITNOA

Hi,

How many votes needed to start implementation of this feature?

[ajcvickers]

@soroshsabz There isn't a fixed number. The more votes there are, the higher the priority going into the planning process. Currently, about 80 votes are needed to get into the top 25 issues.

[Misiu]

As of today, there are 103 votes.

[Li7ye]

Because of this feature is not coming. I share to my workaround for everyone. You can download Demo code for you reference. Only can support non-async scenario and it just a demo not a fully verified solution.

The challenge is that value converter expression tree only can reference a constant object.
e.g. Expression<Fun<string,string>> exp = value=> dbContext.Conveter(value). Only a dbContext instance is cached in expression tree by closure. So, the dbContext can be as a singleton and can't represent the current dbContext in later.

To solve this problem, using AsycLocal<DbContext> to save dbContext instance in the current thread. Despite, it is a singleton but it can represent the current dbContext as long as can set AsycLocal<DbContext> value before any operation. e.g. in DbContext constructor.

AsyncLocalCurrentDbContext is DbContext wrapper class.

internal class AsyncLocalCurrentDbContext
{
    private readonly AsyncLocal<DbContext> asyncLocal= new();

    public DbContext Current
    {
        get => asyncLocal.Value!;
        set => asyncLocal.Value = value;
    }
}

//register in EF Core DI container.
svcs.AddSingleton<AsyncLocalCurrentDbContext>()

In addition, must set AsyncLocalCurrentDbContext value at IDbSetInitializer.InitializeSets() timing. Because IDbSetInitializer.InitializeSets() is executed in DbContext constructor.

internal class InitializeAsycLocalCurrentDbContext(AsyncLocalCurrentDbContext currentDbContext, IDbSetInitializer innerlDbSetInitializer) : IDbSetInitializer
{
    public void InitializeSets(DbContext context)
    {
        innerlDbSetInitializer.InitializeSets(context);
        currentDbContext.Current = context;
    }
}

//register in EF Core DI container.
svcs.DecorateService<IDbSetInitializer, InitializeAsycLocalCurrentDbContext>();

Introducing ContextValueConveter<TModel, TProvider> abstract class to force to include AsyncLocalCurrentDbContext parameter for the implementations.

internal abstract class ContextValueConveter<TModel, TProvider> : ValueConverter<TModel, TProvider>, IContextValueConveter
{
    protected ContextValueConveter(AsyncLocalCurrentDbContext asyncLocalCurrentDb,
                                   Expression<Func<TModel, TProvider>> convertToProviderExpression,
                                   Expression<Func<TProvider, TModel>> convertFromProviderExpression,
                                   ConverterMappingHints? mappingHints = null) : base(convertToProviderExpression, convertFromProviderExpression, mappingHints)
    {

    }
}

internal interface IContextValueConveter { }

Implement ContextValueConveter<TModel, TProvider> by PasswordContextValueConvter in demo.
reference asyncLocalCurrentDb instance in expression tree.
PS : EncryptPassword and DecryptPassword are extension methods for DbContext.

internal class PasswordContextValueConvter : ContextValueConveter<string, string>
{
    public PasswordContextValueConvter(AsyncLocalCurrentDbContext asyncLocalCurrentDb)
                                      : base(asyncLocalCurrentDb,
                                             password => asyncLocalCurrentDb.Current.EncryptPassword(password),
                                             encryptPassword => asyncLocalCurrentDb.Current.DecryptPassword(encryptPassword))
    {

    }
}

EncryptPassword and DecryptPassword method use application provider to get IDataProtect to encrypt or decrypt data.

public static string EncryptPassword(this DbContext dbcontext, string password)
{
    Console.WriteLine($"EncryptPassword : current dbContext id : {dbcontext.ContextId}");
    var appProvider = dbcontext.GetService<IDbContextOptions>()?.Extensions.OfType<CoreOptionsExtension>().FirstOrDefault()?.ApplicationServiceProvider!;

    var dataProtector = appProvider.GetRequiredService<IDataProtectionProvider>()
                                   .CreateProtector("password");

    var value = dataProtector.Protect(password);

    Console.WriteLine($"Password ({password})  is encrypted to {value}");
    Console.WriteLine();

    return value;
}


public static string DecryptPassword(this DbContext dbcontext, string decrypted)
{
    Console.WriteLine($"DecryptPassword : current dbContext id : {dbcontext.ContextId}");

    var appProvider = dbcontext.GetService<IDbContextOptions>()?.Extensions.OfType<CoreOptionsExtension>().FirstOrDefault()?.ApplicationServiceProvider!;

    var dataProtector = appProvider.GetRequiredService<IDataProtectionProvider>()
                                   .CreateProtector("password");

    var password = dataProtector.Unprotect(decrypted);

    Console.WriteLine($"Decrypted password ({decrypted}) is decrypted to {password}");
    Console.WriteLine();

    return password;
}

Register PasswordContextValueConvter for Password property in OnModelCreating method.

b.Property(x => x.Password)
 .HasContextValueConversion<PasswordContextValueConvter>();

Program.cs

var svcs = new ServiceCollection();
svcs.AddDataProtection().SetApplicationName("test");
svcs.AddDbContext<MyDbContext>(b =>
{
    b.UseInMemoryDatabase("db");

    // add context value conversion services in EF Core DI container.
    b.UseContextValueConversion();
});

var rootProvider = svcs.BuildServiceProvider();

using (var scope = rootProvider.CreateScope())
{
    var dbContext = scope.ServiceProvider.GetRequiredService<MyDbContext>();
    dbContext.Users.Add(new User()
    {
        Name = "test",
        Password ="pwd",
    });
    await dbContext.SaveChangesAsync();

    var user = await dbContext.Users.FirstAsync();
    Console.WriteLine($"Password : {user.Password}");
    Console.WriteLine();
}


using (var scope = rootProvider.CreateScope())
{
    var dbContext = scope.ServiceProvider.GetRequiredService<MyDbContext>();
    var user = await dbContext.Users.FirstAsync();
    Console.WriteLine($"Password : {user.Password}");
}

Console.ReadKey();

Console output: the different dbContext ids are displayed on the screen, means can get current dbcontext in value converter.

[TheConstructor]

@Li7ye does your workaround work with EF Core 9 rc2? We recently had trouble using non-static fields and methods in conversions, when the field is projected during a query. This worked fine in EF Core 8.

[roji]

@TheConstructor if you have a scenario that worked in EF Core 8 but no longer works in 9, please submit a new issue with a minimal repro - we'd want to take a look.

[TheConstructor]

@TheConstructor if you have a scenario that worked in EF Core 8 but no longer works in 9, please submit a new issue with a minimal repro - we'd want to take a look.

I hope #34956 can be understood; I am a bit tired right now. Perfect solution mid-term would likely be, if this issue would be implemented.

[roji]

@TheConstructor as of now #34956 has been closed as already fixed in 9.0 - it would be good if you could check and confirm whether that's the case.