Efcore: 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.