-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Allow temporal Period properties to be mapped to CLR properties (i.e. not shadow properties) #26463
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
@FransBouma We think they make more sense as shadow properties since most uses of the entity do not need the period properties. (Essentially, the same reason they are hidden by default in the table.) That being said, we will enable mapping them to non-shadow properties in 7.0. |
@ajcvickers 👍 |
Based on a solution from user @StevenRasmussen (from this thread ) I figured out a hack to map shadow properties to fields in the entity. Part of the inspiration came from user @FransBouma as well. The solution is not perfect but it does the job without having to alter too much of the code. This is done over efcore 6.0.1 version. The interface I used to keep track of things public interface ITemporalEntity
{
DateTime FromSysDate { get; set; }
DateTime ToSysDate { get; set; }
} An example of the implemented entity, do not forget to add public class SomeTemporalEntity : ITemporalEntity
{
public int Id {get; set;}
public string Name {get; set;}
[NotMapped]
DateTime FromSysDate { get; set; }
[NotMapped]
DateTime ToSysDate { get; set; }
} The constants class so we do not have to hardcode everything: public static class TemporalEntityConstants
{
public static readonly string FromSysDate = "FromSysDate";
public static readonly string ToSysDate = "ToSysDate";
public static readonly string FromSysDateShadow = "FromSysDateShadow";
public static readonly string ToSysDateShadow = "ToSysDateShadow";
} The overload of using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Query.Internal;
using Microsoft.EntityFrameworkCore.Storage;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
#pragma warning disable EF1001 // Internal EF Core API usage.
public class TemporalEntityMaterializerSource : EntityMaterializerSource, Microsoft.EntityFrameworkCore.Query.IEntityMaterializerSource
{
public TemporalEntityMaterializerSource(EntityMaterializerSourceDependencies dependencies) : base(dependencies)
{
}
public override Expression CreateMaterializeExpression(IEntityType entityType, string entityInstanceName, Expression materializationContextExpression)
{
var baseExpression = base.CreateMaterializeExpression(entityType, entityInstanceName, materializationContextExpression);
if (entityType.ClrType.GetInterfaces().FirstOrDefault(i => i == typeof(ITemporalEntity)) != null)
{
var fromSysDatePropertyInfo = entityType.ClrType.GetProperty(nameof(ITemporalEntity.FromSysDate));
var toSysDatePropertyInfo = entityType.ClrType.GetProperty(nameof(ITemporalEntity.ToSysDate));
var shadowPropertiesHashSet = new HashSet<IPropertyBase>(
entityType.GetServiceProperties().Cast<IPropertyBase>()
.Concat(
entityType
.GetProperties()
.Where(p => p.IsShadowProperty()))
);
var blockExpressions = new List<Expression>(((BlockExpression)baseExpression).Expressions);
var instanceVariable = blockExpressions.Last() as ParameterExpression;
var valueBufferExpression = Expression.Call(materializationContextExpression, MaterializationContext.GetValueBufferMethod);
var bindingInfo = new ParameterBindingInfo(
entityType,
materializationContextExpression);
var temporalBlockExpressions = new List<Expression>();
foreach (var shadowPropertyBase in shadowPropertiesHashSet)
{
var shadowPropertyMemberType = typeof(DateTime);
var readValueExpression =
valueBufferExpression.CreateValueBufferReadValueExpression(
shadowPropertyMemberType,
shadowPropertyBase.GetIndex(),
shadowPropertyBase);
if (shadowPropertyBase.Name == TemporalEntityConstants.FromSysDateShadow)
temporalBlockExpressions.Add(CreateMemberAssignment(instanceVariable, fromSysDatePropertyInfo, shadowPropertyBase, readValueExpression));
if (shadowPropertyBase.Name == TemporalEntityConstants.ToSysDateShadow)
temporalBlockExpressions.Add(CreateMemberAssignment(instanceVariable, toSysDatePropertyInfo, shadowPropertyBase, readValueExpression));
}
blockExpressions.InsertRange(blockExpressions.Count - 1, temporalBlockExpressions);
return Expression.Block(new[] { instanceVariable }, blockExpressions);
}
return baseExpression;
static Expression CreateMemberAssignment(Expression parameter, MemberInfo memberInfo, IPropertyBase property, Expression value)
{
if (property.IsIndexerProperty())
return Expression.Assign(
Expression.MakeIndex(
parameter, (PropertyInfo)memberInfo, new List<Expression> { Expression.Constant(property.Name) }),
value);
return Expression.MakeMemberAccess(parameter, memberInfo).Assign(value);
}
}
}
#pragma warning restore EF1001 // Internal EF Core API usage. The setup in protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
{
optionsBuilder.ReplaceService<IEntityMaterializerSource, TemporalEntityMaterializerSource>();
base.OnConfiguring(optionsBuilder);
} Not sure if necessary in Startup: services.AddScoped<IEntityMaterializerSource, TemporalEntityMaterializerSource>(); How to setup the entity tables: TemporalTableBuilder<TEntity> temporalTableBuilder
temporalTableBuilder.HasPeriodStart(TemporalEntityConstants.FromSysDateShadow).HasColumnName(TemporalEntityConstants.FromSysDate)
temporalTableBuilder.HasPeriodEnd(TemporalEntityConstants.ToSysDateShadow).HasColumnName(TemporalEntityConstants.ToSysDate) On querying the data, you will have to still use public Task<List<TEntity>> GetAllAsync(Expression<Func<TEntity, bool>> criteria)
=> DbSet.Where(criteria)
.OrderBy(x => EF.Property<DateTime>(x, TemporalEntityConstants.FromSysDateShadow))
.ThenBy(x => EF.Property<DateTime>(x, TemporalEntityConstants.ToSysDateShadow))
.ToListAsync(); That is about it. I will add more if I find any bugs. |
I've added a demo for this implementation on a temporal tables demo solution. I have renamed A note is that you have to be careful with migrations. Then column names have to be swapped: /*b.Property<DateTime>("FromSysDate")
.HasColumnType("datetime2")
.HasColumnName("FromSysDate");*/
b.Property<DateTime>("FromSysDateShadow")
.ValueGeneratedOnAddOrUpdate()
.HasColumnType("datetime2")
.HasColumnName("FromSysDate"); /*b.Property<DateTime>("ToSysDate")
.HasColumnType("datetime2")
.HasColumnName("ToSysDate");*/
b.Property<DateTime>("ToSysDateShadow")
.ValueGeneratedOnAddOrUpdate()
.HasColumnType("datetime2")
.HasColumnName("ToSysDate"); You can see the list of changes here (commit 1597590) |
Maybe I'm using temporal tables wrong ;) It makes perfect sense to me that I would present status messages throughout my UI based on the Period column values: While accessing shadow properties is workable for simple scenarios, if I'm doing a more complex history query where for example, I need to find the last edit date of the most recently edited row from a navigation property, there is no practical way to state this query in EF/LINQ based on shadow properties of a navigation target. I don't know if this needs to be said, but to be crystal of what I'm expecting, please allow mapping Period columns EXACTLY like all other mapped properties on an entity (from a query perspective). P.S.
|
That's what I argued indeed in the startpost :) |
I appear to have a working solution. In my database I have defined the temporal columns as PeriodStart and PeriodEnd. In the model class, I have added my LastModifiedTimestamp property which maps to PeriodStart:
The value for LastModifiedTimestamp (PeriodStart) is returned in both tracked and untracked queries, and I can filter on and select LastModifiedTimestamp in my LINQ. |
@channeladam would you be able to provide a full example? When I try your solution, I receive the following error doing a simple select from the table:
For historical reasons, my period columns are named SysStartTime and SysEndTime, which I have specified like this:
I have added properties like yours to my entity class:
Any suggestions would be appreciated. btw, SysStartTime and SysEndTime in the above examples are string constants. |
@burtonrodman that looks about right and the same, except our database column names differ.
|
@channeladam I have confirmed that there is no other column mapped called SysStartTime or SysEndTime. Any suggestions would be greatly appreciated. Having to manually select the shadow property out during queries is seriously complicating what should be some fairly mundane queries -- especially when AutoMapper is involved ;(
|
I tried @channeladam's suggestion and it worked just fine for non-tracking queries, but I got the "Invalid column name" error on tracking queries. To work around that, I created a read interceptor to replace the erroneous column (I'm only mapping "StartPeriod":
Ugly for sure, and there's still the issue with newly generated migrations wanting to rename the temporal columns. Perhaps the cleanest workaround maintenance-wise is to just map the temporal column name as a computed column for your CLR property. The migration for existing temporal tables is ugly, but at least it's a one-time hit and not a constant pain. The chunk of code below is in the override of OnModelCreating in a custom DbContext class. ILogged is an interface applied to temporal tables whose PeriodStart value I want mapped to a CLR property.
|
I've been able to get this working by adding a computed column into the entity type configuration class (but this should equally apply to the OnModelCreating method of the DbContext):
Haven't tested this extensively but did not need to use AsNoTracking on the query that used it. |
Will this be resolved in EF Core 7 and (hopefully) a maintenance release of EF Core 6? |
@MikeOtown This will not be included in a patch of EF Core 6 or 7. See Release Planning for details on what we do/do not patch. |
Based on what I read on the page you indicated, in my humble opinion, we are in a kind of limbo between these 2 cases:
The lack of temporal period properties doesn't allow to do the temporal queries. So this is clearly an obstacle and so a regression in terms of functionalities. It's also true that we can use a workaround, but it seems that those here are far away from being straightforward and fully functional (so, maybe not so reasonable). In the end, while I may (but not quite, since it's a LTS version!) understand the willingness not to apply a patch on EF Core 6, I disagree with not doing that for EF Core 7 either! |
@OculiViridi This is not a regression. A regression is when something stops working that was previously working. This has never been implemented. As such, the options are:
|
I have the following setup:
and an implementation of the properties like
and a mapping like
IMHO providing the possibility to map period fields to normal fields would integrate much better with many essentials aspects of C#. I would very much like to see this feature available in EF 7. |
I've encountered this same issue when trying to upgrade With this entity public abstract class TemporalEntity : BaseEntity
{
public DateTime VersionValidityStart { get; set; }
public DateTime VersionValidityEnd { get; set; }
} And having DB columns We configure EF like this (which works with 6.0.1) private static void ConfigureTemporalEntity<TEntity>(EntityTypeBuilder<TEntity> builder)
where TEntity : TemporalEntity
{
ConfigureBaseEntity(builder);
builder.ToTable(tableBuilder =>
{
tableBuilder.IsTemporal()
.HasPeriodStart("PeriodStart")
.HasColumnName("SysStartTime");
tableBuilder.IsTemporal()
.HasPeriodEnd("PeriodEnd")
.HasColumnName("SysEndTime");
});
builder.Property(temporalEntity => temporalEntity.VersionValidityStart)
.HasColumnName("SysStartTime")
.ValueGeneratedOnAddOrUpdate();
builder.Property(temporalEntity => temporalEntity.VersionValidityEnd)
.HasColumnName("SysEndTime")
.ValueGeneratedOnAddOrUpdate();
} I'd also like to map these fields to non-shadow properties, ideally with a syntax similar to this private static void ConfigureTemporalEntity<TEntity>(EntityTypeBuilder<TEntity> builder)
where TEntity : TemporalEntity
{
ConfigureBaseEntity(builder);
builder.ToTable(tableBuilder =>
{
tableBuilder.IsTemporal()
.HasPeriodStart(temporalEntity => temporalEntity.VersionValidityStart)
.HasColumnName("SysStartTime");
tableBuilder.IsTemporal()
.HasPeriodEnd(temporalEntity => temporalEntity.VersionValidityEnd)
.HasColumnName("SysEndTime");
});
} I'm adding this comment because we don't annotate our entities but want to configure them fluently. [edit]
From our point of view there is a regression. The above code works fine with EF 6.0.1 (I haven't tried to pinpoint the version which added this validation but it doesn't work with 7.0.5 and not with the latest 6 being 6.0.16). It now throws
whereas it was possible to use a CLR and shadow property before. |
With the IMaterializationInterceptor Interface added with EF7 I've found a solution I like, which doesn't feel like a workaround. What had to be done (compared to my above post)?
private static void ConfigureTemporalEntity<TEntity>(EntityTypeBuilder<TEntity> builder)
where TEntity : TemporalEntity
{
ConfigureBaseEntity(builder);
builder.Ignore(entity => entity.VersionValidityStart);
builder.Ignore(entity => entity.VersionValidityEnd);
builder.ToTable(tableBuilder =>
{
tableBuilder.IsTemporal()
.HasPeriodStart(Constants.ShadowPropertyNames.PeriodStart)
.HasColumnName("SysStartTime");
tableBuilder.IsTemporal()
.HasPeriodEnd(Constants.ShadowPropertyNames.PeriodEnd)
.HasColumnName("SysEndTime");
});
}
internal class TemporalPropertiesSetterInterceptor : IMaterializationInterceptor
{
public static TemporalPropertiesSetterInterceptor Instance { get; } = new();
private TemporalPropertiesSetterInterceptor() { }
public object InitializedInstance(MaterializationInterceptionData materializationData, object entity)
{
if (entity is not TemporalEntity temporalEntity)
return entity;
temporalEntity.VersionValidityStart = materializationData.GetPropertyValue<DateTime>(Constants.ShadowPropertyNames.PeriodStart);
temporalEntity.VersionValidityEnd = materializationData.GetPropertyValue<DateTime>(Constants.ShadowPropertyNames.PeriodEnd);
return temporalEntity;
}
}
services.AddDbContext<MyContext>(builder => builder.UseSqlServer(dataConfiguration.ConnectionString)
.AddInterceptors(TemporalPropertiesSetterInterceptor.Instance)); The only pitfall I see with this approach is that one might want to use the CLR properties to execute a where query; this won't work, but the extension methods provided by SqlServerDbSetExtensions are intended for this usage. [edit] I don't work with projections on temporal tables but these would probably also cause issue. When creating them with EF using a [edit2] I've updated the example code to register the interceptor using a single static instance, as using a new instance lead to |
There was an implicit existing behaviour, you explicitly changed that behaviour, and those depending on that behaviour are now broken. Claiming that the old behaviour was "never implemented" doesn't magically absolve you from the fact that it existed and rightly or wrongly, there is now a dependency on it. Therefore, this behavioural change absolutely represents a regression. |
I'm a little surprised shadow properties only were the initial direction but I'm glad the team is open to change direction. There has to be a lot of reasons people would like to see the validity dates, especially when using I haven't tried in previous versions but in EF Core 8.0.1 I wasn't able to get it to work by using any of the mapping tricks suggested without maintenance issues like migrations, etc. To me the least problematic fix would to simply be to remove the validation itself by replacing the validation service with a sub-classed Temporal version that I would maintain and will re-do upon updates to newer version of EF Core until this is fixed. This so far seems to be working for both runtime as well as migrations, although I haven't tried creating new/removing existing temporal tables with migrations just yet, but it doesn't try adding computed fields and all the other workaround mess. I've tested projections as well as sorting so far on the runtime aspect with zero issues. If that's the case, the bug fix is simply removing the overly restrictive but unneeded validation and adding some unit tests to verify some user scenarios including ones I haven't tested yet like filtering. My only issue with using shadow properties is that it makes integrations with abstractions like Hotchocolate GraphQL more difficult to use. If EF Core is where you write your queries, it makes perfect sense to just use shadow properties as they can pretty much do everything the same with the exception of those pesky strings lacking proper type checking, but when you try to layer on top of that and use the POCO classes as your automatic mapping layer, well now you have problems because now your POCO needs to be a smart object to translate shadow properties into an actual property. Having a POCO property makes that discovery and automapping process much simpler and doesn't require special configuration. Below is the file I created by sub-classing hotfix sub-subclass example:
replace service example:
I've included the whole file as well. Hope this helps anyone that might be having this issue while avoiding workaround pittfalls until a proper fix comes out. |
I've tried to implement the I can see that there is no more direct attribution and it initializes the interceptor for EACH and every entity. This causes increases in duration from 50% to 100%. I am attaching some data from my benchmarks: What you're seeing is the following:
I had to copy the existing You can see the changes here The main important piece of code is this one: private List<Expression> CreateTemporalPropertiesBlockExpressions(ITypeBase structuralType, ParameterExpression instanceVariable, ParameterBindingInfo bindingInfo)
{
if (structuralType.ImplementsTemporalEntityInterface() && structuralType.HasShadowProperties(out var shadowPropertiesHashSet))
{
var fromSysDatePropertyInfo = structuralType.ClrType.GetProperty(nameof(ITemporalEntity.FromSysDate));
var toSysDatePropertyInfo = structuralType.ClrType.GetProperty(nameof(ITemporalEntity.ToSysDate));
var valueBufferExpression = Expression.Call(bindingInfo.MaterializationContextExpression, MaterializationContext.GetValueBufferMethod);
var temporalBlockExpressions = new List<Expression>();
foreach (var shadowPropertyBase in shadowPropertiesHashSet)
{
var shadowPropertyMemberType = typeof(DateTime);
var readValueExpression =
valueBufferExpression.CreateValueBufferReadValueExpression(
shadowPropertyMemberType,
shadowPropertyBase.GetIndex(),
shadowPropertyBase);
if (shadowPropertyBase.Name == TemporalEntityConstants.FromSysDateShadow)
temporalBlockExpressions.Add(CreateMemberAssignment(instanceVariable, fromSysDatePropertyInfo, shadowPropertyBase, readValueExpression));
if (shadowPropertyBase.Name == TemporalEntityConstants.ToSysDateShadow)
temporalBlockExpressions.Add(CreateMemberAssignment(instanceVariable, toSysDatePropertyInfo, shadowPropertyBase, readValueExpression));
}
return temporalBlockExpressions;
}
else
{
return null;
}
} |
@dragos-durlut Have you tried my suggestion above? From my experience it's less overhead and less restrictions. #26463 (comment) |
So this issue is still alive... FYI, my previous approach in 2022 stopped working a while ago. Since then, the easiest solution I have found is: define a computed column in the database table. This has worked well in my typical scenario:
|
@channeladam computed columns are probably the most stable workaround that will survive EF Core versions upgrades. It puts a bad taste in my mouth that I have to change the schema of my database to work around the issue though. If you have control over the schema, it's not a bad way to go. |
@channeladam - do you know if there is any way to avoid getting an error "SQL71610 - System-versioned tables with computed columns are not supported" when using System_Versioning? This is in a database project that gets compiled and the dacpac published.
|
@szavoda the internet tells me that appears to be a known Dac issue... SQL Server supports it just fine, just not Dac... see microsoft/DacFx#207 I use DbUp.Reboot, so I haven't run into that problem - sorry! If it isn't one thing, it's another... Good luck to us all! |
bump... sorry if I've missed news elsewhere, but is there any progress on this, or even an "official" workaround from MS? |
Is there any progress on this topic? |
Has this been abandoned? Would be great to have this for a data migrator I'm working on (Not EF core migrations, a hand-written migration to a different project.) |
Any updates on this? |
(.NET 6, EF Core 6 RC2/latest)
I generate the fields in an entity which are mapped to the period fields in a temporal table as readonly properties and map them as normal fields, however when using them in a query I get:
Why do the period properties have the requirement to be shadow properties? I could generate them as such but IMHO it sucks for the user as to use them they have to fall back to string based names for the fields...
The text was updated successfully, but these errors were encountered: