efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Allow temporal Period properties to be mapped to CLR properties (i.e. not shadow properties)

Open FransBouma opened this issue 3 years ago • 20 comments

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

System.InvalidOperationException : Period property 'Employee.ValidFrom' must be a shadow property.
   at Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal.SqlServerModelValidator

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

FransBouma avatar Oct 26 '21 09:10 FransBouma

@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 avatar Oct 26 '21 18:10 ajcvickers

@ajcvickers 👍 I have it working now, having the fields in the entity definition but not mapped in the modelbuilder. The main reason I did keep them in my own framework (instead of hiding them) is also because when you fetch a set of these entities, you can use the values for the period e.g. in a UI or other means. But I admit, it's a subjective thing: do they belong to the entity or are they strictly there to help the DB do its bookkeeping.

FransBouma avatar Oct 26 '21 21:10 FransBouma

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. Code might not work on an upgrade.

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 NotMapped to the interface members.

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 EntityMaterializerSource and implementation of IEntityMaterializerSource

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 DbContext:

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 EF.Property:

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.

dragos-durlut avatar Jan 07 '22 19:01 dragos-durlut

I've added a demo for this implementation on a temporal tables demo solution. It can be found here (branch period-properties).

I have renamed PeriodStart -> FromSysDate & PeriodEnd -> ToSysDate for easier debugging. (I had some issues with following the correct columns)

A note is that you have to be careful with migrations. In some instances the [NotMapped] attribute does not help and has to be temporarily commented out, generate the migration and then put it back in.

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)

dragos-durlut avatar Jan 25 '22 13:01 dragos-durlut

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: ex: $"Last edited by {row.ModifiedBy} on {row.PeriodStart}"

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. and while we're at it, why not take that last statement to its logical end and make the fluent API for mapping temporal tables and period columns match with mapping any other table/property:

modelBuilder.Entity<MyTemporalTable>(entity => {
  entity.ToTable(nameof(MyTemporalTable)).IsTemporal("MyTemporalTableHistory");
  entity.Property(e => e.SysStartTime).IsPeriodStart();
});

burtonrodman avatar Feb 02 '22 06:02 burtonrodman

@burtonrodman

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).

That's what I argued indeed in the startpost :)

FransBouma avatar Feb 02 '22 08:02 FransBouma

I appear to have a working solution.

In my database I have defined the temporal columns as PeriodStart and PeriodEnd. In the DbContext, the entity is set with IsTemporal and UseHistoryTable.

In the model class, I have added my LastModifiedTimestamp property which maps to PeriodStart:

        [Column("PeriodStart")]
        [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
        public DateTime LastModifiedTimestamp { get; init; }

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 avatar Mar 14 '22 13:03 channeladam

@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:

Invalid column name 'SysEndTime1'
Invalid column name 'SysStartTime1'

For historical reasons, my period columns are named SysStartTime and SysEndTime, which I have specified like this:

      builder.IsTemporal(ttb => {
        ttb.HasPeriodStart(AuditableEntityBase.SysStartTime);
        ttb.HasPeriodEnd(AuditableEntityBase.SysEndTime);
        ttb.UseHistoryTable($"{tableName}History");
      });

I have added properties like yours to my entity class:

    [Column(SysStartTime)]
    [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    public DateTime PeriodStart { get; init; }

    [Column(SysEndTime)]
    [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    public DateTime PeriodEnd { get; init; }

Any suggestions would be appreciated.

btw, SysStartTime and SysEndTime in the above examples are string constants.

burtonrodman avatar Mar 15 '22 05:03 burtonrodman

@burtonrodman that looks about right and the same, except our database column names differ.

  • Are you absolutely sure that you don't already have a property named SysStartTime and SysEndTime in your model? Or that you haven't already mapped another property to your SysStartTime and SysEndTime column names? I have seen that error with a 1 suffix before if there are two mappings to the same column.

  • One other thing to try out of desperation would be to change your property names to anything but 'PeriodStart' and 'PeriodEnd' (in case EF is internally making them special)

channeladam avatar Mar 16 '22 10:03 channeladam

@channeladam Thanks for those suggestions. Unfortunately, I'm not making any progress.

I have confirmed that there is no other column mapped called SysStartTime or SysEndTime. I recreated my temporal table with the default PeriodStart and PeriodEnd column names and renamed the property on the entity to something completely different and now I just get the same error but with the PeriodStart1 column name.

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 ;(

    [Column("PeriodStart")]
    [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    public DateTime BobsBurgerBarn { get; init; }
  , PeriodStart DATETIME2 GENERATED ALWAYS AS ROW START NOT NULL
  , PeriodEnd DATETIME2 GENERATED ALWAYS AS ROW END NOT NULL
  , PERIOD FOR SYSTEM_TIME (PeriodStart,PeriodEnd)

burtonrodman avatar Mar 28 '22 05:03 burtonrodman

Any suggestions would be greatly appreciated.

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":

    public override ValueTask<InterceptionResult<DbDataReader>> ReaderExecutingAsync(
        DbCommand command,
        CommandEventData eventData,
        InterceptionResult<DbDataReader> result,
        CancellationToken cancellationToken = default)
    {
        command.CommandText = command.CommandText
            .Replace(".[PeriodStart1] AS", "~~~")
            .Replace(".[PeriodStart1]", ".[PeriodStart] AS [PeriodStart1]")
            .Replace("~~~", ".[PeriodStart] AS");

        return new ValueTask<InterceptionResult<DbDataReader>>(result);
    }

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.

foreach (var entityType in modelBuilder.Model.GetEntityTypes())
{
    if (entityType.IsTemporal() && entityType.ClrType.IsAssignableTo(typeof(ILogged)))
    {
        var modified = entityType.GetProperty(nameof(ILogged.Modified));
        var start = entityType.GetPeriodStartPropertyName();

        modified.SetComputedColumnSql(start);
    }
}

fimiki avatar Sep 02 '22 18:09 fimiki

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):

builder.Property(o => o.LastUpdateTimeUtc).HasComputedColumnSql("period_start");

Haven't tested this extensively but did not need to use AsNoTracking on the query that used it.

mh-logiten avatar Dec 07 '22 09:12 mh-logiten

Will this be resolved in EF Core 7 and (hopefully) a maintenance release of EF Core 6?

MikeOtown avatar Jan 18 '23 14:01 MikeOtown

@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.

ajcvickers avatar Jan 25 '23 21:01 ajcvickers

@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:

We are more likely to patch an issue if:

  • It is impacting multiple customers
  • => It is a regression from a previous release <=
  • The failure causes data corruption

We are less likely to patch an issue if:

  • => There are reasonable workarounds <=
  • The fix has high risk of breaking something else
  • The bug is in a corner-case

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. Like @burtonrodman said, I also have complex temporal queries to implement, because the temporal tables in my project are used to keep many variations on rows and the users need to be able to consult all the changes made over time and not only on a single table but on various tables related each other. Temporal queries on my application are many and the order of the day.

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). @ajcvickers May we ask at least for an "official", straightforward, solid workaround made by EF Team?

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 avatar Feb 02 '23 12:02 OculiViridi

@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:

  • Don't use this EF Core temporal table mappings. Note this doesn't mean you can't use temporal tables with EF Core, just as you always have been able to do. It just means EF won't do anything special with them.
  • Use shadow properties.
  • Use one of the mapping tricks various people have suggested above.

ajcvickers avatar Feb 02 '23 16:02 ajcvickers

I have the following setup:

public interface IHistory
{
    DateTime ValFrom { get; init; }
    DateTime ValUntil { get; init; }
}

and an implementation of the properties like

public class History : IHistory
{
    [Column("SysStartTime")]
    [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    public DateTime ValFrom { get; init; }

    [Column("SysEndTime")]
    [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    public DateTime ValUntil { get; init; }
}

and a mapping like

builder.ToTable(e => e.IsTemporal(t =>
{
    t.HasPeriodStart("SysStartTime");
    t.HasPeriodEnd("SysEndTime");
    t.UseHistoryTable("HistoryAudit", "audit");
}));
  1. If I execute a simple query like await dbCtx.History.TemporalAll().ToListAsync() I get the same "Invalid column name 'SysStartTime1'" error message as stated above by @burtonrodman.
  2. Mapping to normal fields allows to define interfaces that we can pass around more easily in the application.
  3. If we had period fields mapped to normal fields, we can more easily write extension methods.

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.

Taci42 avatar Mar 16 '23 09:03 Taci42

I've encountered this same issue when trying to upgrade Microsoft.EntityFrameworkCore.SqlServer from 6.0.1 to the latest 7.0.5.

With this entity

public abstract class TemporalEntity : BaseEntity
{
    public DateTime VersionValidityStart { get; set; }
    public DateTime VersionValidityEnd { get; set; }
}

And having DB columns SysStartTime and SysEndTime .

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] @ajcvickers

[...] This is not a regression. A regression is when something stops working that was previously working. This has never been implemented. [...]

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

InvalidOperationException: 'GlnEntity.PeriodEnd' and 'GlnEntity.VersionValidityEnd' are both mapped to column 'SysEndTime' in 'AdresseToGlnMapping', but the properties are contained within the same hierarchy. All properties on an entity type must be mapped to unique different columns.

whereas it was possible to use a CLR and shadow property before.

taconaut avatar May 11 '23 06:05 taconaut

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)?

  1. Ignore the CLR (non-shadow) properties, when setting up the DbContext
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");
                    });
}
  1. Create the interceptor, which sets the CLR properties from the shadow properties:
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;
    }
}
  1. Register the interceptor when adding the DbContext:
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 Select, the shadow properties would have to be used. If using a lib like automapper, which can ProjectTo, I don't see a nice solution to configure the mappings.

[edit2] I've updated the example code to register the interceptor using a single static instance, as using a new instance lead to Microsoft.EntityFrameworkCore.Infrastructure.ManyServiceProvidersCreatedWarning exception after some time. See https://stackoverflow.com/questions/75601346/adding-interceptor-in-dbcontext-gives-microsoft-entityframeworkcore-infrastructu

taconaut avatar May 23 '23 06:05 taconaut

@OculiViridi This is not a regression. A regression is when something stops working that was previously working. This has never been implemented.

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.

IanKemp avatar Jul 13 '23 12:07 IanKemp

@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:

  • Don't use this EF Core temporal table mappings. Note this doesn't mean you can't use temporal tables with EF Core, just as you always have been able to do. It just means EF won't do anything special with them.
  • Use shadow properties.
  • Use one of the mapping tricks various people have suggested above.

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 TemporalAll, TemporalFromTo, TemporalBetween, and TemporalContainedIn since these all can return multiple versions of the same record and we need to be able to distinguish between them easily. I'm even more surprised that this was punted for as long as it was given this is probably an easy fix of just removing the validation check and verifying some user scenarios with unit tests. I'm sure validating all the other scenarios is time consuming and the EF Core team has been pumping out features pretty nicely, so can't complain.

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-classingSqlServerModelValidator, copying exactly and overriding the ValidateTemporalTables method, and copying and editing the ValidateTemporalPeriodProperty method to remove the if (!periodProperty.IsShadowProperty() && ...) clause with the thrown exception and then using the ReplaceService<IModelValidator, TemporalSqlServerModelValidator>() to replace SqlServerModelValidator with my hotfix sub-classed TemporalSqlServerModelValidator code and that's it.

hotfix sub-subclass example:

using System.Linq;
using System;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.SqlServer.Internal;

namespace Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;

#pragma warning disable EF1001 // Internal EF Core API usage.
public class TemporalSqlServerModelValidator : SqlServerModelValidator
{
    public TemporalSqlServerModelValidator(
        ModelValidatorDependencies dependencies,
        RelationalModelValidatorDependencies relationalDependencies)
        : base(dependencies, relationalDependencies)
    {
    }

    /// <summary>
    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
    ///     any release. You should only use it directly in your code with extreme caution and knowing that
    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
    /// </summary>
    protected override void ValidateTemporalTables(
        IModel model,
        IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger)
    {
        var temporalEntityTypes = model.GetEntityTypes().Where(t => t.IsTemporal()).ToList();
        foreach (var temporalEntityType in temporalEntityTypes)
        {
            if (temporalEntityType.BaseType != null)
            {
                throw new InvalidOperationException(SqlServerStrings.TemporalOnlyOnRoot(temporalEntityType.DisplayName()));
            }

            ValidateTemporalPeriodProperty(temporalEntityType, periodStart: true);
            ValidateTemporalPeriodProperty(temporalEntityType, periodStart: false);

            var derivedTableMappings = temporalEntityType.GetDerivedTypes().Select(t => t.GetTableName()).Distinct().ToList();
            if (derivedTableMappings.Count > 0
                && (derivedTableMappings.Count != 1 || derivedTableMappings.First() != temporalEntityType.GetTableName()))
            {
                throw new InvalidOperationException(SqlServerStrings.TemporalOnlySupportedForTPH(temporalEntityType.DisplayName()));
            }
        }
    }

    private static void ValidateTemporalPeriodProperty(IEntityType temporalEntityType, bool periodStart)
    {
        var annotationPropertyName = periodStart
            ? temporalEntityType.GetPeriodStartPropertyName()
            : temporalEntityType.GetPeriodEndPropertyName();

        if (annotationPropertyName == null)
        {
            throw new InvalidOperationException(
                SqlServerStrings.TemporalMustDefinePeriodProperties(
                    temporalEntityType.DisplayName()));
        }

        var periodProperty = temporalEntityType.FindProperty(annotationPropertyName);
        if (periodProperty == null)
        {
            throw new InvalidOperationException(
                SqlServerStrings.TemporalExpectedPeriodPropertyNotFound(
                    temporalEntityType.DisplayName(), annotationPropertyName));
        }

        // Remove or comment out this validation
        //if (!periodProperty.IsShadowProperty() && !temporalEntityType.IsPropertyBag)
        //{
        //    throw new InvalidOperationException(
        //        SqlServerStrings.TemporalPeriodPropertyMustBeInShadowState(
        //            temporalEntityType.DisplayName(), periodProperty.Name));
        //}

        if (periodProperty.IsNullable
            || periodProperty.ClrType != typeof(DateTime))
        {
            throw new InvalidOperationException(
                SqlServerStrings.TemporalPeriodPropertyMustBeNonNullableDateTime(
                    temporalEntityType.DisplayName(), periodProperty.Name, nameof(DateTime)));
        }


... <Copied code removed for brevity>


        if (periodProperty.ValueGenerated != ValueGenerated.OnAddOrUpdate)
        {
            throw new InvalidOperationException(
                SqlServerStrings.TemporalPropertyMappedToPeriodColumnMustBeValueGeneratedOnAddOrUpdate(
                    temporalEntityType.DisplayName(), periodProperty.Name, nameof(ValueGenerated.OnAddOrUpdate)));
        }

        // TODO: check that period property is excluded from query (once the annotation is added)
    }
}
#pragma warning restore EF1001 // Internal EF Core API usage.

replace service example:

services.AddDbContext<MyContext>(builder => builder
    .UseSqlServer(dataConfiguration.ConnectionString)
    .ReplaceService<Microsoft.EntityFrameworkCore.Infrastructure.IModelValidator, Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal.TemporalSqlServerModelValidator>()

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. TemporalSqlServerModelValidator.zip

crowet avatar Jan 24 '24 13:01 crowet

I've tried to implement the TemporalEntityMaterializationInterceptor in PRODUCTION but I've found that going the interceptor route, the EntityMaterializerSurce does a lot more work that without.

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: image image

What you're seeing is the following:

  • duration for EFCore8 with TemporalEntityMaterializerSource re-implemented.
  • duration for EFCore8 WITHOUT any temporal materialization
  • duration for EFCore8 with interceptors
  • duration for EFCore8 with interceptors and 120 compatibility mode
  • duration for EFCore6 with TemporalEntityMaterializerSource used

I had to copy the existing EntityMaterializerSource in EFCore8 and modify it to assign to the temporal properties directly.

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 avatar Mar 07 '24 15:03 dragos-durlut

I've tried to implement the TemporalEntityMaterializationInterceptor in PRODUCTION but I've found that going the interceptor route, the EntityMaterializerSurce does a lot more work that without.

@dragos-durlut Have you tried my suggestion above? From my experience it's less overhead and less restrictions. https://github.com/dotnet/efcore/issues/26463#issuecomment-1908155130

crowet avatar Mar 07 '24 16:03 crowet

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:

  • SQL Server / Azure SQL Database
  • Ability to change the Database schema
  • Database first - generating the entity model with dotnet ef dbcontext scaffold... it 'just works' with the computed column as if it was a normal column/property. No shadow properties or interceptors to deal with...
  • I want to query LastModifiedTimestamp (being PeriodStart)
CREATE TABLE Blah
(
...
    [LastModifiedTimestamp] AS [PeriodStart],

    [PeriodStart]  DATETIME2  GENERATED ALWAYS AS ROW START HIDDEN NOT NULL,
    [PeriodEnd]  DATETIME2  GENERATED ALWAYS AS ROW END HIDDEN NOT NULL,
    PERIOD FOR SYSTEM_TIME ([PeriodStart], [PeriodEnd]),
)

channeladam avatar Mar 07 '24 17:03 channeladam

@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.

crowet avatar Mar 07 '24 17:03 crowet

@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.

        [TemporalValidFrom] as [ValidFrom],
	[TemporalValidTo] as [ValidTo],
	[ValidFrom] DATETIME2 (0) GENERATED ALWAYS AS ROW START,
	[ValidTo] DATETIME2 (0) GENERATED ALWAYS AS ROW END, 
        PERIOD FOR SYSTEM_TIME (ValidFrom, ValidTo),
	INDEX Action_Code_Index NONCLUSTERED([Code]),
	CONSTRAINT [FK_Action_ActionType] FOREIGN KEY ([ActionTypeId]) REFERENCES [dc].[ActionTypeLookup]([Id])
)
WITH (SYSTEM_VERSIONING = ON(HISTORY_TABLE=[dc].[Action_HISTORY], DATA_CONSISTENCY_CHECK=ON))

szavoda avatar Mar 27 '24 22:03 szavoda

@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 https://github.com/microsoft/DacFx/issues/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!

channeladam avatar Mar 28 '24 02:03 channeladam