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

Sugar for sequence-based key value generation on SQL Server #28477

Merged
merged 4 commits into from
Jul 26, 2022

Conversation

ajcvickers
Copy link
Member

Part of #28096

This PR adds the new key generation strategy. A second PR will change what happens by convention on SQL Server.

@ajcvickers ajcvickers requested a review from a team July 19, 2022 13:49
@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 19, 2022

Assume default will remain IDENTITY - and then Sequence when TPC?

@ajcvickers
Copy link
Member Author

@ErikEJ Yes.

@ajcvickers
Copy link
Member Author

/cc @roji @lauxjpn Might be good to have a similar option for PostgreSQL and MySQL.

@ajcvickers
Copy link
Member Author

@roji @AndriySvyryd Updated with changes, except to use a different sequence for each hierarchy, which I will do in a new commit. This new commit will also likely change some of the annotation name and values, so I left them all as KeySequenceXxx for now.

Part of #28096

This PR adds the new key generation strategy. A second PR will change what happens by convention on SQL Server.
@ajcvickers ajcvickers merged commit 4801ae9 into main Jul 26, 2022
@ajcvickers ajcvickers deleted the Sequencer0715 branch July 26, 2022 18:30
@@ -284,11 +381,20 @@ public static class SqlServerModelBuilderExtensions
{
modelBuilder.HasIdentityColumnSeed(null, fromDataAnnotation);
modelBuilder.HasIdentityColumnIncrement(null, fromDataAnnotation);
modelBuilder.HasKeySequences(null, null, fromDataAnnotation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajcvickers implementing this for PG - is this correct? Shouldn't we do HasKeySequences(null) only below, in valueGenerationStrategy != SqlServerValueGenerationStrategy.Sequence?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajcvickers looked at this again in the latest main, and unless I'm misunderstanding, the logic is inversed (see docs). AFAICT each condition block checks if the strategy is not a specific value, and if so, turns off the configuration for that value. So removing the key sequence annotation should only happen in the block which checks that the strategy isn't Sequence.

(also in SqlServerPropertyBuilderExtensions)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah. This is weird.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd @roji I removed all this code and ran the tests. Nothing failed. Thoughts? File a bug? Check-in with the code removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests

roji added a commit to roji/efcore.pg that referenced this pull request Jul 29, 2022
roji added a commit to roji/efcore.pg that referenced this pull request Jul 29, 2022
@roji
Copy link
Member

roji commented Jul 29, 2022

PG implementation: npgsql/efcore.pg#2456

@MoamenMohamed
Copy link

MoamenMohamed commented Mar 13, 2023

Assume default will remain IDENTITY - and then Sequence when TPC?

@ajcvickers @roji I am using PostgresSQL with TPC and I use UseIdentityColumn to configure the base entity ID but the generated sql creates only a column of type integer without any definition for the Identity or Sequence. Should I use some other extension method to configure Sequence for TPC? Thanks in advance.

@roji
Copy link
Member

roji commented Mar 13, 2023

@MoamenMohamed UseIdentityColumn is a very specific thing which indeed doesn't create a sequence in the database: the identity column is rather driven from an implicit, behind-the-scenes sequence which can't be reused for the child tables.

If you simply remove UseIdentityColumn, everything should just happen by convention: EF automatically sets up a sequence for TPC hierarchies; see code below.

Npgsql TPC sample
await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseNpgsql(@"Host=localhost;Username=test;Password=test")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>().UseTpcMappingStrategy();
        modelBuilder.Entity<SpecialBlog1>();
        modelBuilder.Entity<SpecialBlog2>();
    }
}

public abstract class Blog
{
    public int Id { get; set; }
    public string? Name { get; set; }
}

public class SpecialBlog1 : Blog
{
    public int SpecialProp1 { get; set; }
}

public class SpecialBlog2 : Blog
{
    public int SpecialProp2 { get; set; }
}

@MoamenMohamed
Copy link

@roji I removed it and it worked, Thank a lot for the quick response.

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

Successfully merging this pull request may close these issues.

5 participants