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

Value generation on add failing for GUID #10070

Closed
serifine opened this issue Oct 13, 2017 · 9 comments
Closed

Value generation on add failing for GUID #10070

serifine opened this issue Oct 13, 2017 · 9 comments

Comments

@serifine
Copy link

I believe I have stumbled across a bug. Using EntityFrameworkCore 2.0, I am trying to set up a table that has a primary key of type guid. However, when I add the migration, the new primary key is not set to auto-generate. According to the generated values documentation this should work.

Conventions

By convention, primary keys that are of an integer or GUID data type will be setup to have values generated on add. All other properties will be setup with no value generation.

Steps to reproduce

Entity

public class OrganizationInvitation
{
  public Guid OrganizationInvitationId { get; set; }
  public string EmailAddress { get; set; }
}

DbContext

public class DataContext : DbContext
{
  public DataContext(DbContextOptions<DataContext> options) : base(options) { }

  // Other db sets removed for brevity
  public DbSet<OrganizationInvitation> OrganizationInvitations { get; set; }
}

EF Setup in Startup.cs

services.AddDbContext<DataContext>(options => options.UseSqlServer(connection));

I run

dotnet ef --startup-project ../Api migrations add "Add OrganizationInvitation"

Which results in the following migration that is missing the code to auto-generate the guid.

public partial class AddOrganizationInvitation : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.CreateTable(
            name: "OrganizationInvitations",
            columns: table => new
            {
                OrganizationInvitationId = table.Column<Guid>(type: "uniqueidentifier", nullable: false),
                EmailAddress = table.Column<string>(type: "nvarchar(max)", nullable: false)
            },
            constraints: table =>
            {
                table.PrimaryKey("PK_OrganizationInvitations", x => x.OrganizationInvitationId);
            });
    }

    protected override void Down(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.DropTable(
            name: "OrganizationInvitations");
    }
}

I have tried my initial migration with all of the following, and I believe valid, combinations for the Id field.

public Guid Id { get; set; }
public Guid OrganizationInvitationId { get; set; }
// should not be required because of conventions, but just to check
[Key]
[DatabaseGenerated(DatabaseGeneratedOption.Identity)]
public Id { get; set; }
// again should not be required because of conventions, but just to check
[Key]
[DatabaseGenerated(DatabaseGeneratedOption.Identity)]
public Guid OrganizationInvitationId { get; set; }
[Key]
[DatabaseGenerated(DatabaseGeneratedOption.Identity)]
public Guid InvitationId { get; set; }

In each case, the migration produced the incorrect migration code. However, upon renaming the property to a valid Id (examples 1 and 2) or with a non-conventional name and explicitly defining it as a key (example 5), the second migration will produce migration code that would look as though it will work, specifically it adds the following line to the column properties.

defaultValue: new Guid("00000000-0000-0000-0000-000000000000")

That seems to be incorrect as well and is causing the database to simply generate a GUID of 00000000-0000-0000-0000-000000000000.

Further technical details

EF Core version: 2.0
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Database: Microsoft Azure SQL Server
Operating system: Windows 10 Professional (15063)
IDE: VS Code and Visual Studio 2017, using the dotnet-cli tools.

@ajcvickers
Copy link
Member

@ZHollingshead By convention values for GUID keys are generated on the client rather than in the database. (For SQL Server, this uses a sequential GUID algorithm, similar to "newsequentialid" on SQL Server.) Client-side key generation has the advantage that new keys are available immediately rather than only after a round-trip to the database.

To make EF Core use values generated on the database you just need to tell EF which database function to use to do this. Something like:

modelBuilder.Entity<OrganizationInvitation>()
    .Property(e => e.OrganizationInvitationId)
    .HasDefaultValueSql("newsequentialid()");

@serifine
Copy link
Author

Ah so it is. Thanks for the quick reply. I was tripped up by this line from the docs.

For example, when using SQL Server, values will be automatically generated for GUID properties (using the SQL Server sequential GUID algorithm)

@TehWardy
Copy link

Completely agree ... Here's what i think is a simpler explanation though ...
dotnet/ef6#762

@TehWardy
Copy link

@ajcvickers I don't think that answer solves the issue.
It's both inconsistent in behaviour and implies that a key will be generated when it is not generated as the migration information generated results in a runtime error during inserts.

You're basically saying that "by convention all guid keys require something outside the db" then why does EF "sometimes" generate a migration that results in the guid keys being created by SQL and sometimes not?

dotnet/ef6#762

^ there is definitely a bug here, if nothing else it's that the behaviour is not consistent but IMO when a property on an entity is marked with a database generated option then it should always mean that and it's EF's problem to figure out what that means regardless of "best practice".

@ajcvickers
Copy link
Member

@TehWardy

It's both inconsistent in behaviour and implies that a key will be generated when it is not generated as the migration information generated results in a runtime error during inserts.

Can you file an issue for this including a small, runnable project/solution or complete code listing that demonstrates the behavior.

You're basically saying that "by convention all guid keys require something outside the db" then why does EF "sometimes" generate a migration that results in the guid keys being created by SQL and sometimes not?

I don't fully follow this. EF Core will use different value generation strategies for primary keys depending on the type being used and the capabilities of the database provider. For GUIDs, most providers default to client-side sequential generation. This can of course be changed by configuring something else.

With regards to the EF6 behavior it is by-design different from the EF Core behavior. This is because EF6 doesn't support client-generated keys, or indeed many of the value generation strategies that can be used with EF Core. So if by "not consistent" you mean between EF Core and EF6, then that is very much expected. If there is some inconsistent behavior in EF6, then please file a bug clearly describing it and we will take a look. However, we won't be making any breaking changes on EF6, and the bar for new features is very high.

@TehWardy
Copy link

TehWardy commented May 1, 2019

The EF6 Issue

The link i provided above is describing the inconsistency I noticed that EF6 suffers in Guid Key generation.
The short version is "the migration doesn't include the defaultsql part as I would expect" and I don't know why, and in "some" situations it generates the keys in SQL and in others it doesn't, by my entity code appeared to be like for like so I got confused.

I Suspect that the core fluent API call of .HasDefaultValueSql("newid()"); being added to EF6's API layer would resolve my issue. Happy to accept that as the fix for that ticket, asusming it works as you described above if you can make that happen. Currently the only solution seems to be for me to manually hack the migration.

Generating Keys, Best Practice

Could you guys (someone in EF dev) explain how this (preferring client side key gen) came about as a "best practice" for EF core as I felt EF6 worked perfectly fine preferring the server to gen keys?
Is this a side effect of EF being compared to "ORM like products" and being called slow or something?

I've always felt the lower perf came at the advantage of having the framework do more for me and supporting bulk operations was always a goal of EF Core from the outset from what I understand which could easily be handled by generating scripts that do things like use SQLBulkCopy (in the event the server was MSSQL.

Honestly though, I don't mind where the keys are generated by I would just like to be able to just build an entity and not populate the primary key then ask EF (any version) to insert that in to the DB with the result being "it now has a valid saved primary key value" ... how that actually happens regarding DB scripts is essentially not my concern as a consumer of EF.

For the most part, (unless the key is a string i guess) Is it not reasonable for the default to be "EF handles the key gen in some way unless user tells me otherwise" for all key properties?

@ajcvickers
Copy link
Member

@TehWardy Your last two paragraphs describe in a general sense how EF Core works.

@TehWardy
Copy link

TehWardy commented May 2, 2019

Ah perfect then :)
I have to admit i've not used EF core in anger yet. Some of the breaking changes between EF6 and EF Core have made that too much work. The most obvious being many to many relationships join tables, I recall there is no way to have the system behave like EF6 and manage them transparently, unless that's changed now?

It's sounding like i may have to migrate though if Core is going to start having tooling in it that 6 doesn't ... I found .Net core to be temperamental at best though due to referencing issues.

Thanks for the feedback though :)
What are my chances of having .HasDefaultValueSql("newsequentialid()"); added to EF6 at this point as it's all pretty much sorted now but the lack of this is resulting in m forgetting / regularly breaking models when I build new tables and the recent surge of Core focused docs are giving the impression this is how it should be solved despite the option not being in EF6?

@TehWardy
Copy link

I totally put this ticket out of my mind after working around the problem @ajcvickers
I had an email today saying it was reopened but looks like it already got closed.

Just in case this helps in some small way here's a sort of retrospective on it ...

The bottom line is that for entity keys I think the default should be that they are generated with common rules followed for types like integers and guid's with us having an ability to specify a generation expression or something if need be.

I have examples like say ... a cultures table where the key is a string, requires no generation and the system should just accept what I give it but that's more of an edge case than the norm from what I understand.

In my case I was finding that I couldn't get to the bottom of why in some cases the auto generated migrations would generate the auto generation calls for the key and in other cases it wouldn't.

I gelt like it was a bug but never could get to the bottom as to why EF would do it some cases and not in others.
Here's the weirdness I found on a stack overflow question ...
https://stackoverflow.com/questions/50930786/ef-code-first-migrations-db-generated-guid-keys

I had to manually alter the migration code to do what I felt should have been automatic but the feedback I got at the time was that the Application should generate the keys not the db.
EF is in the application .. so I couldn't understand why either way I had to alter the migration and EF couldn't just manage it.

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

No branches or pull requests

3 participants