Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renaming PeriodStart and PeriodEnd columns of a temporal table causes them to be swapped #29902

Closed
orty opened this issue Dec 20, 2022 · 5 comments · Fixed by #32328
Closed
Assignees
Labels
area-migrations area-temporal-tables closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-8.0 Originally planned for the EF Core 8.0 (EF8) release, but moved out due to resource constraints. type-bug

Comments

@orty
Copy link

orty commented Dec 20, 2022

Hi,

I just discovered that, in my database, on 3 temporal tables, the meaning of the PeriodStart and PeriodEnd column has been kind of swapped after renaming them (to be compliant with our naming convention).
As a result, dates in the new PeriodStart-like column are greater than the dates in the PeriodEnd one.

Repro steps:

Having this entity model:

public class Contact : IEntity
{
    /// <inheritdoc />
    public Guid Id { get; set; }
}

Having this in DbContext:

modelBuilder.Entity<Contact>(
    entity =>
    {
        entity.ToTable(
            builder =>
            {
                builder.IsTemporal();
            });

        entity.HasKey(c => c.Id).HasName("ConId");
    });

Generates the following migration Up():

migrationBuilder.AlterTable(
    name: "Contacts")
    .Annotation("SqlServer:IsTemporal", true)
    .Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
    .Annotation("SqlServer:TemporalHistoryTableSchema", null)
    .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
    .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");
    
migrationBuilder.AddColumn<DateTime>(
    name: "PeriodEnd",
    table: "Contacts",
    type: "datetime2",
    nullable: false,
    defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified))
    .Annotation("SqlServer:IsTemporal", true)
    .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
    .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");
    
migrationBuilder.AddColumn<DateTime>(
    name: "PeriodStart",
    table: "Contacts",
    type: "datetime2",
    nullable: false,
    defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified))
    .Annotation("SqlServer:IsTemporal", true)
    .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
    .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

Then, if we are to rename the columns to be compliant with the Con prefix trigram, we add this to the DbContext:

modelBuilder.Entity<Contact>(
    entity =>
    {
        entity.ToTable(
            builder =>
            {
                builder.IsTemporal(
                    tableBuilder =>
                    {
                        tableBuilder.HasPeriodStart("ConValidFrom");
                        tableBuilder.HasPeriodEnd("ConValidTo");
                    });
            });
  
        entity.HasKey(c => c.Id).HasName("ConId");
    });

And this is the resulting migration Up() generated method:

migrationBuilder.RenameColumn(
    name: "PeriodStart",
    table: "Contacts",
    newName: "ConValidTo");

migrationBuilder.RenameColumn(
    name: "PeriodEnd",
    table: "Contacts",
    newName: "ConValidFrom");

migrationBuilder.AlterTable(
    name: "Contacts")
    .Annotation("SqlServer:IsTemporal", true)
    .Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
    .Annotation("SqlServer:TemporalHistoryTableSchema", null)
    .Annotation("SqlServer:TemporalPeriodEndColumnName", "ConValidTo")
    .Annotation("SqlServer:TemporalPeriodStartColumnName", "ConValidFrom")
    .OldAnnotation("SqlServer:IsTemporal", true)
    .OldAnnotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
    .OldAnnotation("SqlServer:TemporalHistoryTableSchema", null)
    .OldAnnotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
    .OldAnnotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

Here, it seems to me that the renaming of the columns kind of "swaps" them, isn't it ?

Thanks.

Include provider and version information

EF Core version: 6.0.5
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6.0
Operating system: Win 11
IDE: Visual Studio 2022 17.3.6

@orty
Copy link
Author

orty commented Dec 20, 2022

I just double checked using the following SQL query

select  OBJECT_NAME(object_id) as table_name,* from sys.periods

that the columns for start and end are swapped in the database.

@maumar
Copy link
Contributor

maumar commented Dec 21, 2022

migrations model differ is not aware of the period start/end annotations, and when figuring out renames it just looks at the names (and property mappings, but those are the same). It so happens that old names were in different order ("End", "Start") than the new ones ("From", "To").

Workaround is to rename one column at a time.

@orty
Copy link
Author

orty commented Dec 21, 2022

Thanks for having looked into it. This is definitely not a satisfying workaround for such a task, but at the very least, it does the trick.
Naming the columns the right way in the first place does not create any issue, I just hope that a fix for this strange behavior can find its way to v.7.0

@ajcvickers
Copy link
Member

Note for triage: root cause is tracked by #15178. We should consider raising the priority of this.

@ajcvickers ajcvickers added this to the 8.0.0 milestone Jan 12, 2023
@ajcvickers
Copy link
Member

Note from triage: at least try to fix this issue in 8.0, even if we don't get to all of #15178.

@ajcvickers ajcvickers added the punted-for-8.0 Originally planned for the EF Core 8.0 (EF8) release, but moved out due to resource constraints. label Sep 6, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0, Backlog Sep 6, 2023
maumar added a commit that referenced this issue Nov 17, 2023
…ral table causes them to be swapped

Problem was that migration model differ was too lax with pairing up columns. Fix is to add more predicates - one that matches annotations+values and one that just matches annotations, ignoring the values, before we fallback to simple column definition.
Now that we reworked temporal annotations, this actually gives clean match for the period start and period end column.
Moreover, this change could improve matching in other, non-temporal scenarios.

Fixes #29902
@maumar maumar added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Nov 17, 2023
@maumar maumar modified the milestones: Backlog, 9.0.0 Nov 17, 2023
maumar added a commit that referenced this issue Nov 17, 2023
…ral table causes them to be swapped (#32328)

Problem was that migration model differ was too lax with pairing up columns. Fix is to add more predicates - one that matches annotations+values and one that just matches annotations, ignoring the values, before we fallback to simple column definition.
Now that we reworked temporal annotations, this actually gives clean match for the period start and period end column.
Moreover, this change could improve matching in other, non-temporal scenarios.

Fixes #29902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations area-temporal-tables closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-8.0 Originally planned for the EF Core 8.0 (EF8) release, but moved out due to resource constraints. type-bug
Projects
None yet
4 participants