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

Migrations: Adding properties to owned types causes strange renames #9873

Closed
kykc opened this issue Sep 21, 2017 · 4 comments
Closed

Migrations: Adding properties to owned types causes strange renames #9873

kykc opened this issue Sep 21, 2017 · 4 comments
Assignees
Milestone

Comments

@kykc
Copy link

@kykc kykc commented Sep 21, 2017

Observed behavior

When I'm trying to add another owned type property to already existing entity, instead of just adding new fields, migration renames one of the existing properties fields to the new name and creates new fields for already existing property. The resulting Up method looks like this:

        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.RenameColumn(
                name: "AmountCharged_Currency",
                table: "BusinessTransactions",
                newName: "ChargeFee_Currency");

            migrationBuilder.RenameColumn(
                name: "AmountCharged_Amount",
                table: "BusinessTransactions",
                newName: "ChargeFee_Amount");

            migrationBuilder.AddColumn<decimal>(
                name: "AmountCharged_Amount",
                table: "BusinessTransactions",
                type: "decimal(18, 2)",
                nullable: false,
                defaultValue: 0m);

            migrationBuilder.AddColumn<int>(
                name: "AmountCharged_Currency",
                table: "BusinessTransactions",
                type: "int",
                nullable: false,
                defaultValue: 0);
        }

Expected behavior

        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.AddColumn<decimal>(
                name: "ChargeFee_Amount",
                table: "BusinessTransactions",
                type: "decimal(18, 2)",
                nullable: false,
                defaultValue: 0m);

            migrationBuilder.AddColumn<int>(
                name: "ChargeFee_Currency",
                table: "BusinessTransactions",
                type: "int",
                nullable: false,
                defaultValue: 0);
        }

Steps to reproduce

Minimal testcase is better than a thousand words:

  1. Clone testcase repo
  2. Uncomment two lines in Program.cs
  3. Add-Migration TestNewField in nuget console

Note: this repo also contains borken branch which contains all migrations and snapshots as it happened on my side, so master is before, borken is after.

Further technical details

EF Core version: 2.0.0
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 1703
IDE: Visual Studio 2017 15.3.5

@kykc
Copy link
Author

@kykc kykc commented Sep 22, 2017

Futher experiments:

  1. Setting field names explicitly like this achieves nothing:
var x = modelBuilder.Entity<BusinessTransaction>().OwnsOne(p => p.Profit);
x.Property(m => m.Amount).HasColumnName("Profit_Amount");
x.Property(m => m.Currency).HasColumnName("Profit_Currency");
  1. Setting owned type automatically for each property of desired type (MoneyAmount in this example) achieves nothing:
foreach (var entity in modelBuilder.Model.GetEntityTypes().ToList())
{
	foreach (var property in entity.ClrType.GetProperties().ToList())
	{
		if (property.PropertyType == typeof(MoneyAmount))
		{
			modelBuilder.Entity(entity.ClrType).OwnsOne(typeof(MoneyAmount), property.Name);
		}
	}
}
  1. If I correct generated Up and Down methods by hand further migrations (without owned types) and subsequent Update-Database calls seem to work as expected since generated snapshots are correct, the problem is only in the migration.
        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.AddColumn<decimal>(
                name: "ChargeFee_Amount",
                table: "BusinessTransactions",
                type: "decimal(18, 2)",
                nullable: false,
                defaultValue: 0m);

            migrationBuilder.AddColumn<int>(
                name: "ChargeFee_Currency",
                table: "BusinessTransactions",
                type: "int",
                nullable: false,
                defaultValue: 0);
        }

        protected override void Down(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.DropColumn(
                name: "ChargeFee_Amount",
                table: "BusinessTransactions");

            migrationBuilder.DropColumn(
                name: "ChargeFee_Currency",
                table: "BusinessTransactions");
        }
    }

Is it save thing to do? I've tried to never touch code-generated migrations before.

  1. There's a strange thing in the snapshot that I've spotted: for one field BusinessTransactionId isn't nullable, and for others it is:
                        b.OwnsOne("efc_minimal_testcase.MoneyAmount", "AmountCharged", b1 =>
                        {
                            b1.Property<int>("BusinessTransactionId");

and

                        b.OwnsOne("efc_minimal_testcase.MoneyAmount", "ChargeFee", b1 =>
                        {
                            b1.Property<int?>("BusinessTransactionId");
@ajcvickers ajcvickers added this to the 2.1.0 milestone Sep 22, 2017
@ajcvickers ajcvickers added the type-bug label Sep 22, 2017
@bricelam
Copy link
Member

@bricelam bricelam commented Sep 25, 2017

I suspect this was fixed by #9469. I'll confirm before closing.

@bricelam
Copy link
Member

@bricelam bricelam commented Sep 26, 2017

Negative. This was not fixed by that change.

@bricelam
Copy link
Member

@bricelam bricelam commented Sep 26, 2017

For properties on owned types we probably just need to include the navigation property when pairing up old and new properties.

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

Successfully merging a pull request may close this issue.

None yet
3 participants