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

Useless AlterColumn operations are generated for TimeStampAttribute-marked properties on every migration #792

Closed
msvprogs opened this issue Jun 20, 2019 · 10 comments
Assignees
Labels
Milestone

Comments

@msvprogs
Copy link

Steps to reproduce

Consider the following model:

public class TestContext : DbContext
{
   public DbSet<TestEntity> TestEntities { get; set; }
}

public class TestEntity
{
   [Key]
   public int Id { get; set; }

   public string Name { get; set; }
 
   [Required, TimeStamp, ConcurrencyCheck]
   public byte[] RowVersion { get; set; }
}

The issue

When I make a migration, useless AlterColumn operations are issued, even if there are no changes in the model.

public partial class TestMigration : Migration
{
   protected override void Up(MigrationBuilder migrationBuilder)
   {
      migrationBuilder.AlterColumn<DateTime>(
         name: "RowVersion",
         table: "TestEntities",
         rowVersion: true,
         nullable: false,
         oldClrType: typeof(DateTime))
         .OldAnnotation("MySql:ValueGenerationStrategy", MySqlValueGenerationStrategy.ComputedColumn);
   }

   protected override void Down(MigrationBuilder migrationBuilder)
   {
      migrationBuilder.AlterColumn<DateTime>(
         name: "RowVersion",
         table: "TestEntities",
         nullable: false,
         oldClrType: typeof(DateTime),
         oldRowVersion: true)
         .Annotation("MySql:ValueGenerationStrategy", MySqlValueGenerationStrategy.ComputedColumn);
   }
}

Further technical details

MySQL version: 8.0
Operating system: Windows 10 Pro
Pomelo.EntityFrameworkCore.MySql version: 2.2.0

@Mvii7
Copy link

Mvii7 commented Jul 12, 2019

I have the same issue - I can generate the migration and apply it, but all other migrations I generate have this..

@lauxjpn lauxjpn self-assigned this Oct 25, 2019
@lauxjpn
Copy link
Collaborator

lauxjpn commented Oct 25, 2019

A simple workaround is to use System.DateTime instead of System.Byte[] because RowVersion/Timestamp fields in MySQL use the timestamp or timestamp(6) database data type, which is mapped to the CLR data type System.DateTime:

[Required, TimeStamp, ConcurrencyCheck]
public DateTime RowVersion { get; set; } // <-- changed from byte[] to DateTime

The support for byte[] timestamps is implemented internally by applying value converters to convert between byte[] and DateTime and seems to be a bit buggy, but should be fixable.

@caleblloyd Do you remember what the reason for removing RelationalTypeMapping _rowversion from MySqlTypeMappingSource (and therefore the official mapping between System.DateTime and a RowVersion) was?

@msvprogs
Copy link
Author

msvprogs commented Oct 25, 2019 via email

lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Oct 25, 2019
@lauxjpn
Copy link
Collaborator

lauxjpn commented Oct 25, 2019

This might be an EF Core bug.
I opened an issue on their repo.

@msvprogs
Copy link
Author

Okay, that might be so. ValueConverters handling is still far from perfect (

lauxjpn added a commit that referenced this issue Oct 27, 2019
* Add support to reverse engineer views.
Add comments for tables and columns.
Improve handling of default values.

* Handle the `CURRENT_TIMESTAMP` default value for `timestamp` columns correctly.
Fixes #703

* Correctly implement `CURRENT_TIMESTAMP` with `ON UPDATE` clauses.
Introduce a workaround for the missing EF Core handling of `ValueGenerated.OnUpdate`.
Fixes #877

* Remove unnecessary code

Can probably remove this code as it only applies to Release Candidate not General Availability versions.

https://bugs.mysql.com/bug.php?id=89793

>The NON_UNIQUE column in the INFORMATION_SCHEMA.STATISTICS table had
type BIGINT prior to MySQL 8.0, but became VARCHAR in MySQL 8.0 with
the introduction of the data dictionary. The NON_UNIQUE column now
has an integer type again (INT because the column need not be as
large as BIGINT).

* Correctly map `unsigned` database types with precision, scale, size or display width to CLR types.

* Fix table/view determination.

* Support views in `MySqlDatabaseCleaner`.

* Fix some Timestamp/RowVersion issues.
Still depends on dotnet/efcore#18592
Addresses #792
@lauxjpn lauxjpn removed the blocked label Oct 29, 2019
@lauxjpn
Copy link
Collaborator

lauxjpn commented Oct 29, 2019

This specific problem is fixed by #896 and dotnet/efcore#18599.
There is another issues that came up while fixing this one, that is blocked by dotnet/efcore#13850.

@lauxjpn lauxjpn closed this as completed Oct 29, 2019
@lauxjpn lauxjpn added this to the 3.1.0 milestone Oct 29, 2019
@RickFrankel
Copy link

I am running v3.1.0-rc1.final and am still having this issue. I even changed all my byte[] columns to DateTime and still have the issue. Am I missing something here?

@lauxjpn
Copy link
Collaborator

lauxjpn commented Dec 15, 2019

This specific problem is fixed by #896 [...]

The intonation here is on specific. The issue @msvprogs posted was about a byte[] property with the [Required, TimeStamp] attributes, which should be fixed.

While fixing the issue, others came up and we opened an issue on the EF Core repo upstream, that also contained a sub issue about the same property setup described here, but without the [Required] attribute. It would look like this:

[Timestamp] // <-- without [Required] attribute
public byte[] RowVersion { get; set; }

This one has not been fixed yet, because it is blocked by another EF Core issue:

On a second look we can't yet mark them as non-nullable before dotnet/efcore#13850 is fixed as we can still send nulls.

You can read up on this one here: dotnet/efcore#18592


If you think your issue is not related to the one described above, than please post your model class, its definition and the generated migrations.

If your issue is related and you need a workaround, the following line should work:

[Timestamp, Required] // <-- use [Required] attribute
public byte[] RowVersion { get; set; }

@RickFrankel
Copy link

Perfect many thanks for the detailed explanation. Your workaround worked perfectly.

@tuanbs
Copy link

tuanbs commented Jan 6, 2020

@msvprogs Thanks for creating this issue. I had this issue for almost 2 years and now it's solved. Many thanks guys. :)

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

No branches or pull requests

5 participants