7.22.4 has undocumented breaking change in temporal table queries

[jzabroski]

Description

When upgrading from 7.18.4 to 7.100.0.5, we observed one of our unit tests failing. Below is the basic structure of the test. I gradually changed Nuget package versions from 7.18.4 upwards until I found which release caused the breaking change, and it seems to be 7.22.4 is when things broke.

[Fact]
public void TestGetByKey()
{
    var testInstances = GetTestInstances();
    foreach (var testInstance in testInstances)
    {
        var instance = Repository
            .GetByKey(testInstance.Id, DateTime.MaxValue)
            .SingleOrDefault();
        Assert.NotNull(instance); // THIS FAILS ON 7.22.4
        Assert.Equal(v, instance);
    }
    var testInstanceId = Fixture.Create<string>();
    var testDate = DateTime.MinValue;
    var result = Repository
        .GetByKey(testInstanceId , testDate)
        .SingleOrDefault();
    Assert.Null(result);
}

Repository.GetByKey implementation

public List<T> GetByKey(TKey key, DateTime asOfDate)
{
  return Set<T>()
    .Where(x => x.Id == key)
    .TemporalTableAsOf(asOfDate.ToUniversalTime(), GetTemporalTypes())
    .ToList();
}

protected Type[] GetTemporalTypes()
{
    return Context.EntityTypes
        .Where(x => typeof(IHasSystemVersion).IsAssignableFrom(x))
        .ToArray();
}

Additional Implementation Notes

• IHasSystemVersion is just a basic interface for the system periods exposed by SQL Server temporal tables.
• GetTestInstances returns instances created by an Effort-like test wrapper that sets up a database integration test/unit test. After the test is complete, the transaction gets rolled back and nothing is saved - the only visible side-effect is table auto-identity would get incremented for primary keys using identity insert.

Exception

If you are seeing an exception, include the full exceptions details (message and stack trace).

Xunit.Sdk.NotNullException
Assert.NotNull() Failure: Value is null
   at FundRepositoryTests.TestGetByKey() in C:\Users\John.Zabroski\source\repos\erp\Repositories\Funds\FundRepositoryTests.cs:line 121

Fiddle or Project (Optional)

n/a

Further technical details

• EF version: 6.4.4
• EF Extensions version: testInstanceId
• Database Provider: SQL Server with .NET Framework 4.8

[JonathanMagnan]

Hello @jzabroski ,

Thank you for reporting. We will look at what could have happened.

Best Regards,

Jon

[JonathanMagnan]

Hello @jzabroski ,

Unfortunately, my developer was not able to reproduce it with the current information.

Do you think you could create a runnable project with the issue? It doesn’t need to be your project, just a new solution with the minimum code to reproduce the issue.

Best Regards,

Jon

[JonathanMagnan]

Hello @jzabroski

Unfortunately, since we didn't hear from you I will close this issue.

As previously mentioned, we need a runnable project to be able to assist you.

We will reopen the issue if a project is received.

Feel free to contact us for questions, issues or feedback.

Best Regards,

Jon

[jzabroski]

@JonathanMagnan The bug is that, prior to 7.22.4, queries were formatted with in-line DateTime values and the precision was limited to whole seconds, e.g.

EXEC sp_executesql N'SELECT 
    [Extent1].[Id] AS [Id], 
    [Extent1].[Version] AS [Version], 
    [Extent1].[ValidFromInUtc] AS [ValidFromInUtc], 
    [Extent1].[ValidToInUtc] AS [ValidToInUtc]
    FROM [dbo].[Person] FOR SYSTEM_TIME AS OF ''4/15/2024 4:07:00 PM'' AS [Extent1]
    WHERE ( NOT ((N''ZZZQueryHook;'' = @p__linq__0) AND (@p__linq__0 IS NOT NULL))) AND (1 = [Extent1].[Id])',N'@p__linq__0 nvarchar(4000)',@p__linq__0=N''

From 7.22.4 onwards, it seems the ''4/15/2024 4:07:00 PM'' is now a variable passed-in as an argument to sp_executesql, and the argument value's DateTime precision is not truncated. Below is the new, breaking behvior:

exec sp_executesql N'SELECT 
    [Extent1].[Id] AS [Id], 
    [Extent1].[Version] AS [Version], 
    [Extent1].[ValidFromInUtc] AS [ValidFromInUtc], 
    [Extent1].[ValidToInUtc] AS [ValidToInUtc]
    FROM [dbo].[Person] FOR SYSTEM_TIME AS OF @94088a4e_a832_4d60_97d0_4b7ae7caebe4 AS [Extent1]
    WHERE ( NOT ((N''ZZZQueryHook;'' = @p__linq__0) AND (@p__linq__0 IS NOT NULL))) AND (1 = [Extent1].[Id])',N'@p__linq__0 nvarchar(4000),@94088a4e_a832_4d60_97d0_4b7ae7caebe4 datetime2(7)',@p__linq__0=N'',@94088a4e_a832_4d60_97d0_4b7ae7caebe4='2024-04-16 15:02:56.0005610'

I believe the reason 7.18 functioned the way it did is because we were the initial customer who requested this feature, and you asked us how we wanted it to behave, and we wanted it to the nearest second to avoid weird idiosyncrasies with DateTime.Now and DateTime vs. DateTime2(7) interactions. - My team member Mike is on vacation, so will have to wait until he gets back to confirm.

Also, I can submit a project, but it's honestly messy code given I struggled to get EF6 code-first migrations working so that you could run a test over and over again. I use my own project, FluentMigrator, for our internal migrations. I forgot how bad EF6 code-first migrations were (EF6 code-first migration snapshot validation feature is super difficult to deal with).

[JonathanMagnan]

Hello @jzabroski ,

You are 100% right; we made some changes to the DateTime for temporal table and forgot to document it.

The change was to fix the following reported issue: https://github.com/zzzprojects/EntityFramework-Plus/issues/766

We currently cannot undo the fix as I believe it should be the default behavior. However, we might not be against adding a global option to change this default behavior to work as prior this change.

Let me know if a global options is something you would want or you prefer Mike come back from vacation to choose what will be the best solution for your side.

Best Regards,

Jon

[jzabroski]

Would the global option effect everywhere a Datetime comparison happens, e.g. Not just temporal table but future queries, bulk insert/update/merge/contains? Just trying to assess what the best path forward is. It could honestly be we just need to look at every query in our system where this could be a problem, but we only caught it in a single unit test out of 6,000 repository tests, so first identifying the surface area would help me estimate how much regression risk we have using the "right behavior".

My other thought would be, we unfortunately mix Datetime and datetime2 data types in our database. Datetime has a 3.33ms accuracy issue that makes comparing records to DateTime.Now a pita, so that might be why we may want a global option, depending again on where else this change regressed to, if that makes sense. It is likely a huge project to standardize on Datetime2(7).

[JonathanMagnan]

Hello @jzabroski ,

I confirmed with my employee that we can add an option, and it will only affect the temporal table's behavior as before.

Let me know if that's the path you would like to try.

Best Regards,

Jon

[jzabroski]

Yes

[JonathanMagnan]

Hello @jzabroski ,

A new version has been released today.

Could you try the following option: TemporalTableManager.DisableDateTimeParameter = true. When enabled, you should get the old behavior with temporal table methods.

Let us know if that worked.

Best Regards,

Jon

[jzabroski]

Hi, forgot to say that this fix did in fact work.