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

EF Core 6 - IsTemporal adds an unnecessary migration when DefaultSchema is configured at the DbContext Level #26676

Closed
jaliyaudagedara opened this issue Nov 14, 2021 · 4 comments · Fixed by #27198
Assignees
Labels
area-model-building area-sqlserver 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 type-bug
Milestone

Comments

@jaliyaudagedara
Copy link
Contributor

jaliyaudagedara commented Nov 14, 2021

I am seeing this incorrect behavior with EF Core 6 in relation to TemporalTables.

Consider the below code.

public class MyDbContext : DbContext
{
    private const string DEFAULT_SCHEMA = "app";

    public DbSet<Student> Students { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder
            .UseSqlServer("<ConnectionString>")
            .EnableSensitiveDataLogging()
            .LogTo(Console.WriteLine, LogLevel.Information);
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.HasDefaultSchema(DEFAULT_SCHEMA);

        modelBuilder.Entity<Student>(builder =>
        {
            builder.ToTable(nameof(Student), x => x.IsTemporal());
        });
    }
}

public class Student
{
    public int Id { get; set; }

    public string Name { get; set; }
}

Now to reproduce the issue,

  1. Add a migration
  2. Add just another migration
    image

Look at the highlighted line. It drops the annotation for the schema in History table.

I can get around this issue by removing the DefaultSchema configuration from the DbCotext level and introducing it into the table level. But then that has to be done for each table :(

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Student>(builder =>
    {
        builder.ToTable(nameof(Student), DEFAULT_SCHEMA, x => x.IsTemporal());
    });
}

The sample executable code is attached herewith.
EfCore6-TemporalTables.zip

Environment Info:

EF Core version: 6.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6.0
@jaliyaudagedara jaliyaudagedara changed the title EF Core 6 - IsTemporal adds a unnecessary migration when DefaultSchema is configured at the DbContext Level EF Core 6 - IsTemporal adds an unnecessary migration when DefaultSchema is configured at the DbContext Level Nov 14, 2021
@maumar maumar self-assigned this Nov 15, 2021
@ajcvickers
Copy link
Member

@maumar Repros on 7.0.0-alpha.1.21566.3 daily build. Servicing consider.

@maumar
Copy link
Contributor

maumar commented Dec 17, 2021

Problem here is that when we build RelationalModel in design time, when we create annotations for table we assign HistoryTableSchema the same as the table schema, if the history table schema wasn't set explicitly (so that table schema and history table schema are in sync). This becomes a problem, when the model differ looks at that RelationalModel and compares it with current model (where history table is not set) and hence generates the migration. When building actual migration sql we compensate for the difference and not generate actual sql code, but the problem persists.

We can fix this by leaving historytableschema as null if it wasn't explicitly set, so that the RelationalModel is the same as current model. However this complicates the migration generation step - now we need to sometimes generate extra migrations, because differ won't detect it. e.g.:

myEntity.ToTable("MyTable", "mySchema", tb => tb.IsTemporal())

changed to:

myEntity.ToTable("MyTable", "myModifiedSchema", tb => tb.IsTemporal())

model differ will only detect change in the table schema but we also need to change schema of the history table. This gets worse if you, say, also change the history table name - the differ doesn't know that the history table schema would be implicitly changed (when the temporal table schema changed), so it generates migration with potentially incorrect schema and we need to correct this.

There are probably more issues around this so we need to add a lot of tests to make sure we don't regress - too risky to patch especially given that there is a simple (but annoying) workaround.

Alternatively we can try a different approach - maybe make it so that in current model, history table schema also follows the table schema, So we avoid the differ discrepancy between relational model and current model, but we don't get the complications mentioned above. But we get into business of managing conventions - when schema changes on entity or global schema is added, we need to propagate the change to history tables.

@gtteamamxx
Copy link

gtteamamxx commented Dec 31, 2021

Temp fix for that as mentioned above, just explictly set scheme in OnModelConfiguring:

from

.ToTable("Table", b => b.IsTemporal());

to

.ToTable("Table", b => b.IsTemporal(ttb => ttb.UseHistoryTable("TableHistory", "Scheme")));

also you should edit snapshot of adding temporal migration (.designer.cs) and also context snapshot.
and add line:
image

After that every migration will be empty.

maumar added a commit that referenced this issue Jan 17, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to improve the temporal convention so that history table schema in current model (more) closely resembles the temporal table schema and therefore is similar to RelationalModel generated in runtime.

Fixes #26676
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 17, 2022
maumar added a commit that referenced this issue Jan 17, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to improve the temporal convention so that history table schema in current model (more) closely resembles the temporal table schema and therefore is similar to RelationalModel generated in runtime.

Fixes #26676
maumar added a commit that referenced this issue Jan 17, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to improve the temporal convention so that history table schema in current model (more) closely resembles the temporal table schema and therefore is similar to RelationalModel generated in runtime.

Fixes #26676
maumar added a commit that referenced this issue Jan 18, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to improve the temporal convention so that history table schema in current model (more) closely resembles the temporal table schema and therefore is similar to RelationalModel generated in runtime.

Fixes #26676
maumar added a commit that referenced this issue Jan 18, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to improve the temporal convention so that history table schema in current model (more) closely resembles the temporal table schema and therefore is similar to RelationalModel generated in runtime.

Fixes #26676
maumar added a commit that referenced this issue Jan 20, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to add missing default annotations in the model finalization step. This is a temporary measure until we implement default value conventions (#9329)

Fixes #26676
maumar added a commit that referenced this issue Jan 20, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to add missing default annotations in the model finalization step. This is a temporary measure until we implement default value conventions (#9329)

Fixes #26676
maumar added a commit that referenced this issue Jan 20, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to add missing default annotations in the model finalization step. This is a temporary measure until we implement default value conventions (#9329)

Fixes #26676
maumar added a commit that referenced this issue Jan 20, 2022
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to add missing default annotations in the model finalization step. This is a temporary measure until we implement default value conventions (#9329)

Fixes #26676
@maumar maumar closed this as completed in cf46ae0 Jan 21, 2022
@ajcvickers ajcvickers added this to the 7.0.0 milestone Jan 27, 2022
@ajcvickers ajcvickers removed this from the 7.0.0 milestone Feb 14, 2022
@moander
Copy link

moander commented Nov 11, 2023

This did the trick for me

.ToTable(b => b.IsTemporal(tb => tb.UseHistoryTable(b.Name!, b.Schema!)));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment