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

Consider throwing/warning by default if a migration is used against a different provider than it was generated for #15174

Open
Tracked by #19587 ...
gpresland opened this issue Mar 27, 2019 · 4 comments
Labels
area-migrations consider-for-current-release customer-reported punted-for-3.0 punted-for-5.0 punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-enhancement
Milestone

Comments

@gpresland
Copy link

gpresland commented Mar 27, 2019

ValueGeneratedOnAdd and UseSqlServerIdentityColumn do not create an Identity Specification in MSSQL Server. That is to say, Identity Specification will have a value of No on the column. This is following the documentation at https://docs.microsoft.com/en-us/ef/core/modeling/generated-properties.

Oddly enough, when the database already exists but is empty and you create the table(s) with EnsureCreated(), values are inserted and incremented automatically without any issues. HOWEVER, if you create the table(s) with Migrate(), you will received the error:

An error occurred while updating the entries. See the inner exception for details. Cannot insert the value NULL into column 'xId', table 'db.dbo.tablename'; column does not allow nulls. INSERT fails.
The statement has been terminated.

The value of xId is not NULL. It is set with:

x.Id = default;

which is a value of 0 and works just fine if the tables were created with EnsureCreated().

Both EnsureCreated() and Migrate() will create the column with Identity Specification with a value of No, but if you used EnsureCreated() to create the table(s), INSERTs will work as expected, auto incrementing.

I would use EnsureCreated() if I could since it still works in inserting automatically incrementing IDs, but it turns out to have a quirk of its own that makes it unusable in certain contexts. EnsureCreated() will not create (or even attempt to create) any table(s) if the database contains one (1) or more existing tables, even if the existing table(s) are not found in the context. i.e. you cannot have any tables at all in your database if you want to call EnsureCreated() on an existing database.

Entity is as simple as:

    ...
    public int Id { get; set; }
    ...

Example of trying every combination on a column:

public void Configure(EntityTypeBuilder<ExampleEntity> builder)
{
	builder
		.HasKey(e => e.Id)
                .HasName("PK_xId"); // <-- Adding this line Identify Specification is `Yes` when tables are created with EnsureCreate(), but `No` when created with Migrate(). If omitted, Identify Specification is always `No`.
	builder
		.Property(e => e.Id)
		.HasColumnName("xId")
		.ValueGeneratedOnAdd()
		.UseSqlServerIdentityColumn()
		.Metadata.AfterSaveBehavior = PropertySaveBehavior.Throw;
		
		...
}

Migrations script generates with dotnet ef migrations add

migrationBuilder.CreateTable(
	name: "ExampleTable",
	columns: table => new
	{
		xId = table.Column<int>(nullable: false)
			.Annotation("Sqlite:Autoincrement", true)
	},
	constraints: table =>
	{
		table.PrimaryKey("PK_ExampleTable", x => x.xId);
	});

Designer:

modelBuilder.Entity("ExampleTable", b =>
	{
		b.Property<int>("Id")
			.ValueGeneratedOnAdd()
			.HasColumnName("xId")
			.HasAnnotation("SqlServer:ValueGenerationStrategy", SqlServerValueGenerationStrategy.IdentityColumn);

		b.HasKey("Id");

		b.ToTable("ExampleTable");
	});

Note: I've changed the name of my column to xId and table to ExampleTable manually for this post.

@ajcvickers
Copy link
Member

@gpresland That migration was scaffolded for SQLite. If you want it to work for SQL Server, then it needs to be scaffolded for SQL Server,

@gpresland
Copy link
Author

Thanks @ajcvickers.

For anyone that runs into this in the future, my context supported both SQLite and MSSQL, but used SQLite if you instantiated a new context without passing anything into the constructor. I didn't think enough to notice that that would affect the migration script generation. This has been resolved by reworking the context script.

@ajcvickers ajcvickers changed the title ValueGeneratedOnAdd/UseSqlServerIdentityColumn Don't Create Identity, EnsureCreated Works Regardless, Migrations Bugged Consider throwing/warning by default if a migration is used against a different provider than it was generated for Mar 28, 2019
@ajcvickers
Copy link
Member

Reopening to consider making this more obvious. The idea is that usually if a migration for one provider is run against another provider this is a mistake. Of course, it might not be, because a common set of migrations for use with multiple providers (often with small amounts of conditional code) is entirely valid and a good approach for targeting multiple providers. So there needs to be an easy way to opt out of the warning/error.

@ajcvickers ajcvickers reopened this Mar 28, 2019
@gpresland
Copy link
Author

As an example, in some situations I want to use the same context (if possible) but be able to choose between local (SQLite) and remote (MSSQL). Local perhaps for testing, running the application as standalone or portable, etc. and remote for full install deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations consider-for-current-release customer-reported punted-for-3.0 punted-for-5.0 punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-enhancement
Projects
None yet
Development

No branches or pull requests

4 participants