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 after field rename with Datetime type doesn't work #9344

Closed
vRITHNER opened this issue Aug 6, 2017 · 16 comments
Closed

Migrations after field rename with Datetime type doesn't work #9344

vRITHNER opened this issue Aug 6, 2017 · 16 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression type-bug
Milestone

Comments

@vRITHNER
Copy link

vRITHNER commented Aug 6, 2017

I just renamed field in many tables with type DateTime
Then I add a migration but the Up() method is empty
but it works well with string fields

Steps to reproduce

Include a complete code listing (or project/solution) that we can run to reproduce the issue.
Rename a field Datetime
Add a migration

Partial code listings, or multiple fragments of code, will slow down our response or cause us to push the issue back to you to provide code to reproduce the issue.

//before modification:
  public DateTime CreatedDateFrm { get; set; }
//after modification:
  public DateTime CreatedFrm { get; set; }

In command shell:
dotnet ef migrations add "data_5" -c Db -v -p ..\GrrDev2.Data\GrrDev2.Data.csproj

Migration class:

 public partial class data_5 : Migration
    {
        protected override void Up(MigrationBuilder migrationBuilder)
        {

        }

        protected override void Down(MigrationBuilder migrationBuilder)
        {

        }
    }

Further technical details

EF Core version: (found in project.json or packages.config)
Microsoft.EntityFrameworkCore (2.0.0)
Database Provider:
Microsoft.EntityFrameworkCore.SqlServer (2.0.0)
Operating system:

Microsoft Visual Studio Enterprise 2017 Preview
Version 15.3.0 Preview 7.0
VisualStudio.15.Preview/15.3.0-pre.7.0+26730.0
Microsoft .NET Framework
Version 4.7.02542

.NET Command Line Tools (2.1.0-preview1-007012)

Product Information:
 Version:            2.1.0-preview1-007012
 Commit SHA-1 hash:  b60272d64d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.16257
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.1.0-preview1-007012\

Microsoft .NET Core Shared Framework Host

  Version  : 2.0.0
  Build    : e8b8861ac7faf042c87a5c2f9f2d04c98b69f28d
@vRITHNER vRITHNER changed the title Migrations Rename field doesn't work with Datetime field Migrations after Rename field with Datetime type doesn't work Aug 6, 2017
@bricelam
Copy link
Contributor

bricelam commented Aug 7, 2017

Confirmed. Something is wrong. I get the following on the latest 2.0.0 bits.

migrationBuilder.DropColumn(
    name: "DateTime",
    table: "Entities");

migrationBuilder.AddColumn<DateTime>(
    name: "Updated",
    table: "Entities",
    type: "datetime2",
    nullable: false,
    defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified));

For some reason, the Migrations model differ isn't pairing up the old an new column.

@bricelam bricelam self-assigned this Aug 7, 2017
@bricelam
Copy link
Contributor

bricelam commented Aug 7, 2017

Hmm, I'm getting this for all types, not just DateTime.

@bricelam
Copy link
Contributor

Looks like the model snapshot returns null for IProperty.Relational().ColumnType if it's not configured, but the model always returns a type.

@ajcvickers @AndriySvyryd Should we change the way ColumnType works on the snapshot or just compensate in the model differ?

@bricelam
Copy link
Contributor

If we want to make it consistent, we could either:

  1. Always serialize the type into the snapshot, or
  2. Lazily compute it using the current provider after the snapshot is loaded.

@bricelam
Copy link
Contributor

var oldType = new MyContextModelSnapshot().Model
    .FindEntityType("MyProject.MyEntity")
    .FindProperty("MyProperty")
    .Relational().ColumnType;

var newType = new MyContext().Model
    .FindEntityType("MyProject.MyEntity")
    .FindProperty("MyProperty")
    .Relational().ColumnType;

Assert.Equal(oldType, newType);

@AndriySvyryd
Copy link
Member

@bricelam Probably need a design meeting, as we've already discussed this at least twice

@bricelam
Copy link
Contributor

Will cue it up. Although this is mostly orthogonal to #8624.

@bricelam
Copy link
Contributor

I'm leaning towards writing it into the model snapshot and compensating for older snapshots.

@ajcvickers
Copy link
Member

Agree with @bricelam.

@bricelam
Copy link
Contributor

I changed my approach (see the PR). There were a few other places we only want the value if it was explicitly configured by the user. Also, while writing them in the snapshot is fine, that would make it more difficult to keep them out of the migrations where they definitely shouldn't be.

@bricelam bricelam added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 18, 2017
@bricelam bricelam reopened this Aug 23, 2017
@fc1943s
Copy link

fc1943s commented Sep 4, 2017

@bricelam
Hello, after 2.0 my migrations are being generated with explicit types.

Example: my GUID columns are being generated with this:

migrationBuilder.AddColumn<Guid>(... type: "BLOB" ... );
migrationBuilder.CreateTable(name: "X", columns: table => new {
                                 Id = table.Column<Guid>(type: "BLOB", nullable: false)
} ...

Since i generate migrations with a mode=memory sqlite database, and my application runs on postgres (that dont understand BLOB), its breaking now.

Your pull request already fixes this issue, right?
Thanks!

@bricelam
Copy link
Contributor

bricelam commented Sep 5, 2017

Yes, that is another symptom of the same underlying bug that was fixed.

@Eilon
Copy link
Member

Eilon commented Sep 15, 2017

This patch bug is approved for the 2.0.x patch. Please send a PR to the feature/2.0.1 branch and get it reviewed and merged. When we have the rel/2.0.1 branches ready please port the commit to that branch.

@smitpatel
Copy link
Member

@bricelam - Are we sure, that the cause of issue in first post is the same? I am not able to repro empty migration.

@bricelam
Copy link
Contributor

I wasn't able to repro it either, but I did find the bug that we ended up fixing while trying.

@Eilon
Copy link
Member

Eilon commented Oct 23, 2017

Hi, we have a public test feed that you can use to try out the ASP.NET/EF Core 2.0.3 patch!

To try out the pre-release patch, please refer to the following guide:

We are looking for feedback on this patch. We'd like to know if you have any issues with this patch by updating your apps and libraries to the latest packages and seeing if it fixes the issues you've had, or if it introduces any new issues. If you have any issues or questions, please reply on this issue to let us know as soon as possible.

Thanks,
Eilon

@divega divega changed the title Migrations after Rename field with Datetime type doesn't work Migrations after field rename with Datetime type doesn't work Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression type-bug
Projects
None yet
Development

No branches or pull requests

8 participants