Closed maliming closed 1 week ago
@maliming you can workaround the problem by slightly modifying the converter:
public class MyDateTimeValueConverter : ValueConverter<DateTime, DateTime>
{
public MyDateTimeValueConverter(ConverterMappingHints? mappingHints = null)
: base(
x => new MyDatetimeConverter().Normalize(x),
x => new MyDatetimeConverter().Normalize(x),
mappingHints)
{
}
}
and the model configuration part:
modelBuilder.Entity<Book>().Property(e => e.CreationDate).HasConversion(new MyDateTimeValueConverter());
Basically, rather than passing MyDatetimeConverter
as closure variable in the MyDateTimeValueConverter
ctor expressions, create them within the expression itself.
shaper we generate:
(queryContext, dataReader, resultContext, resultCoordinator) =>
{
DateTime? value1;
value1 = dataReader.IsDBNull(0) ? default(DateTime?) : (DateTime?)Invoke(((MyDateTimeValueConverter)[LIFTABLE Constant: SqlServerDateTimeTypeMapping | Resolver: c => (RelationalTypeMapping)c.Dependencies.TypeMappingSource.FindMapping(
type: System.Nullable`1[System.DateTime],
model: c.Dependencies.Model,
elementMapping: null)].Converter).ConvertFromProviderTyped, dataReader.GetDateTime(0));
return new { Day = (DateTime)value1 };
}
We use generic CLR DateTime to find mapping for the value, and that obviously doesn't have the converter. We can't use converter ctor expression because it contains an unliftable constant, so we "fall back" to the dummy way of doing things.
Here is a relevant piece of code documentation from CreateGetValueExpression
:
// if IProperty is available, we can reliably get the converter from the model and then incorporate FromProvider(Typed) delegate
// into the expression. This way we have consistent behavior between precompiled and normal queries (same code path)
// however, if IProperty is not available, we could try to get TypeMapping from TypeMappingSource based on ClrType, but that may
// return incorrect mapping. So for that case we would prefer to incorporate the FromProvider lambda, like we used to do before AOT
// and only resort to unreliable TypeMappingSource lookup, if the converter expression captures "forbidden" constant
// see issue #33517 for more details
This essentially is dupe of #33517
this is a breaking change, so we should document this, as the workaround is not intuitive at all @roji
this is a breaking change
@maumar are you suggest we document this as a by-design regression? Isn't it a bug that we need to fix (and probably service)?
it's a bug/limitation of the current liftable constant approach. I'd service this but i'm worried how risky the fix is going to be.
OK. It in any case seems like a bug (and a regression at that) rather than an intended change... Ideally we'd make sure this scenario works in regular mode even if it doesn't in precompiled/AOT mode (which is experimental anyway, etc.); I think it's OK for now if we'd throw an exception eagerly if a value converter with a captured variable is used with precompilation, as long as things work in normal mode... Let me know what you think.
BTW note that the workaround instantiates a converter instance each and every time a value is converted, which is a bit problematic perf-wise.
yeah, I thought about it over night and I do agree. I don't think we have a way to fix it right now in a way that would work for AOT (need https://github.com/dotnet/efcore/issues/33517 for that), but we could revert to pre-AOT pattern in these specific scenarios.
Yeah, that sounds right.
fixed in 8561f4ef72e557fcb1f0b937b7c0052bd994ab62
Also reported via https://github.com/dotnet/efcore/issues/34934
The
csproj
and code to reproduce:It works if I create the
MyDatetimeConverter()
object in the expression parameter. And the all code works in EF Core 8.0.