dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.78k stars 3.19k forks source link

Migrations: Order columns of abstract base class properties last in CreateTable #11314

Closed TanvirArjel closed 4 years ago

TanvirArjel commented 6 years ago

Before the release of Entity Framework Core 2.1, Entity Framework Core team announced that,

Column ordering in migrations: Based on customer feedback, we have updated migrations to initially generate columns for tables in the same order as properties are declared in classes.

but unfortunately, it is not working as expected in case of the following scenarios:

public abstract class AuditModel
{
    public int? CreatedBy { get; set; }
    public DateTime CreatedAt { get; set; } = DateTime.UtcNow;
    public int? ModifiedBy { get; set; }
    public DateTime? ModifiedAt { get; set; }
    public bool IsActive { get; set; } = true;
}

public class Employee : AuditModel
{
   [Key]
   [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
   public long EmployeeId { get; set; }

   public string EmployeeNo { get; set; }

   [ForeignKey("Company")]
   public long CompanyId { get; set; }

   [ForeignKey("CompanyOffice")]
   public long OfficeId { get; set; }

   [ForeignKey("Section")]
   public long SectionId { get; set; }
    ..........................
}

Entity Framework Core Migration is generating the Employee Table columns as the following order:

  [CreatedBy] <-----
  ,[CreatedAt] <-----
  ,[ModifiedBy] <-----
  ,[ModifiedAt] <-----
  ,[IsActive] <-----
  ,[EmployeeId]
  ,[EmployeeNo]
  ,[CompanyId]
  ,[OfficeId]
  ,[SectionId]
  ............

Actually It should have been:

  ,[EmployeeId]
  ,[EmployeeNo]
  ,[CompanyId]
  ,[OfficeId]
  ,[SectionId]
   ............
  ,[CreatedBy] <-----
  ,[CreatedAt] <-----
  ,[ModifiedBy] <-----
  ,[ModifiedAt] <-----
  ,[IsActive] <-----

Note that, Entity Framework 6.x Migration generates the columns as expected order even in case of inheritance.

ajcvickers commented 6 years ago

@TanvirArjel Can you be more specific about what is "wrong" with the order generated? For example, is it just that the primary key is not the first column? Or is it also that the unmapped base type should go at the end? If the latter, then what is the logic that determines this and would that also be true if it was a mapped base type?

TanvirArjel commented 6 years ago

@ajcvickers Yes! Primary key should be the first column and the unmapped based type columns should go at the end..

ajcvickers commented 6 years ago

@TanvirArjel Can you provide an explanation as to why the unmapped base type should go at the end and whether or not this should also be the case for a mapped base type? (I'm not saying you're wrong, I'm just trying to understand what the reasoning is behind this ordering.)

TanvirArjel commented 6 years ago

Dear @ajcvickers , Thanks for your persistent reply.

Does it make sense that the primary key of the table at column no. 5 or so other than the first column of the table? Look at the two orders of the columns in the question. Which makes sense? Which is more meaningful? Obviously not the first order..Hope,you will also be agree with me.

In case of unmapped based type, yes! it also be more readable and rational that unmapped based type columns should go at the end of the table because normally in the common unmapped base type we put the columns which are common to the every table like CreatorName, CreationTime, ModifierName, ModificationTime etc. These columns at the beginning of the table does not make sense.

Point to be noted that, In Entity Framework 6.2.0, Primary key of the table is always the first column and both unmapped and mapped based type goes at the end. This really makes sense!

I know Entity Framework Core is a breaking change from Entity Framework but what's wrong to bring these already accustomed,well tested and more sensible behaviors from Entity Framework to Entity Framework Core?

ajcvickers commented 6 years ago

@TanvirArjel Thanks for the additional information. It will help us decide our course of action here.

ajcvickers commented 6 years ago

Triage decisions:

TanvirArjel commented 6 years ago

Dear @ajcvickers, Thanks a lot! for taking speedy action..That's why we love and rely on Microsoft :)

shargon commented 6 years ago

Any update about this?

ajcvickers commented 6 years ago

@TanvirArjel This issue is in the 3.0 milestone. This means it is currently planned to be fixed for the 3.0 release.

nunorelvao commented 6 years ago

I also would love to have the possibility to work with order annotations eg:


 public class BaseModel
    {
        [Key, Column(Order = 0)]
        [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
        public int Id { get; set; }
        [Column(Order = 100, TypeName = "datetime")]
        public DateTime Base_Addeddate { get; set; }
        [Column(Order = 101, TypeName = "datetime")]
        public DateTime Base_Modifieddate { get; set; }
        [Column(Order = 102, TypeName = "nvarchar(50)")]
        public string Base_Username { get; set; }
        [Column(Order = 103)]
        [MaxLength(255)]
        public string Base_Ipaddress { get; set; }
        [Column(Order = 104)]
        public bool? Base_Enabled { get; set; }
    }

public class Language : BaseModel
    {
        [Column(Order = 2)]
        public string Code { get; set; }
        [Column(Order = 3)]
        public string Name { get; set; }
        [InverseProperty("Language")]
        public virtual IList Countries { get; set; }
    }

Will this be possible? AS currently the order is created is by Name! and ignores totally this Annotation!

So the idea is to have the order as described even by inherited classes!

TanvirArjel commented 6 years ago

@nunorelvao Think! you have hundred of model classes with lots of columns. Then imagine how many times you have to write order annotations.

bricelam commented 6 years ago

Please upvote #10059 for [Column(Order = 1)] support.

smitpatel commented 6 years ago

Primary Keys first fixed in #13678

TrevorFradsham commented 5 years ago

Is there any further work being doing to allow the order of the fields to be set? I have an SoftDeleted class with fields to handle logical deletion and it is inherited by any any class that would like this feature. The problem: the fields from the inherited class are always shown immediately after the primary key field. I understand that I can control the order of the fields within a class, but I cannot across an inherited class.

    public class SoftDeleted : ISoftDeleted
    {
        public DateTime? DeletedOn { get; set; }
        public String DeletedBy { get; set; }
    }

Just a thought: is it possible to have an annotation [PriorityOrder(n)] so you can control the order of the column generation? ex: to

     [PriorityOrder(300)]
    public class SoftDeleted : ISoftDeleted
    {
        public DateTime? DeletedOn { get; set; }
        public String DeletedBy { get; set; }
    }
    [PriorityOrder(100)]
    public class MyClass: SoftDeleted
    {
        [Key]
        public int Id { get; set; }
        public String Comment{ get; set; }
    }

so the migration output would look like

       protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.CreateTable(
                name: "MyClass",
                columns: table => new
                {
                    Id = table.Column<int>(nullable: false)
                        .Annotation("Sqlite:Autoincrement", true),
                    Comment= table.Column<string>(nullable: true),
                    DeletedOn = table.Column<DateTime>(nullable: true),
                    DeletedBy = table.Column<string>(nullable: true),

instead of:

        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.CreateTable(
                name: "MyClass",
                columns: table => new
                {
                    Id = table.Column<int>(nullable: false)
                        .Annotation("Sqlite:Autoincrement", true),
                    DeletedOn = table.Column<DateTime>(nullable: true),
                    DeletedBy = table.Column<string>(nullable: true),
                    Comment= table.Column<string>(nullable: true),
ajcvickers commented 5 years ago

@TrevorFradsham You would likely be able to do what you want with the Column attribute. Issue #10059 is tracking this.

TrevorFradsham commented 5 years ago

thanks for the link. I am not seeing that there is any progress for Column order for EF Core. Will keep monitoring though.

biliktamas79 commented 5 years ago

+1 for the explicit orderable properties/columns.

Olbrasoft commented 5 years ago

++ for the explicit orderable properties/columns.

bricelam commented 5 years ago

You're doing it wrong. We only count ๐Ÿ‘ reactions on #10059. (joking, but kinda true)

TanvirArjel commented 5 years ago

@ajcvickers as you said:

Triage decisions:

Primary keys first Un-mapped base classes at end

Primary keys first has been fixed! What about the Un-mapped base classes at end? Any update please?

ajcvickers commented 5 years ago

@TanvirArjel The issue is currently in the 3.0 milestone, but 3.0 is still overbooked with work, so unfortunately this issue may have to be pushed back to a later release.

ekuryla commented 5 years ago

This problem is creating messy code. What is the status of this issue (putting unmapped base classes at the end of a table)? In 3.0 Preview 4, I'm seeing naming convention changes- but nothing about the unmapped classes issue, which to me is more significant. No one in their right mind wants to see auditable table properties at the beginning of the table -- totally against industry standard. Does anyone have a workaround for this?

ekuryla commented 5 years ago

Not using EF Core Migrations as the workaround is the wrong answer...lol

ekuryla commented 5 years ago

Found this as workaround - https://stackoverflow.com/questions/55757687/order-columns-in-migrations-ef-core. This works...but not perfect. Alot of decoration...

TanvirArjel commented 5 years ago

@ajcvickers EF Core 3.0 has already been released! This issue has not been fixed yet! This issue is persisting from EF Core 1.0 and submitted in EF Core 2.0 but still in backlog and being delayed by milestones. Is there any plan to fix this issue in this century please! ๐Ÿ˜‚๐Ÿ˜‚

Thank you.

smitpatel commented 5 years ago

There are 81 more years to go before century ends. ๐Ÿ˜ฎ

bricelam commented 5 years ago

There were some improvements to this area in 2.2.

With our limited team of five engineers, we strive really hard to prioritize our work appropriately.

Just for perspective, this issue: image; many-to-many: image

jdanl89 commented 5 years ago

The easiest work around is to do a migration without extending the base class. Then add the extension, and do another migration. It's not a good solution.

Another possible option might be to do something like [Column(Order = 1000)] in the base class... I haven't tried it, but it might work.

My first thought was that you should be able to do [Column(Order = -1)]... but this doesn't solve the issue of multiple levels of inheritance.

Really, there should be a class-level attribute for specifying whether the base class's properties come before or after those of the class in question... then you would add that attribute at each level of inheritance.

administersoftware commented 5 years ago

Just tried this on one of my entities, I set the first property an annotation of [Column(Order = 0)], and then in the abstract class I set the only property an annotation of [Column(Order = 1000)] and generated my database. Sadly the column ordering was ignored and the property in the abstract class was generated as the 2nd property rather than the last :(

Now that NetCore3 is out I would think a lot more Dev's are going to run into this, I'm already thinking of taking the pain of removing my abstract classes to get the entity layout I require - and only because I'm fussy ;) But hopeful for a fix...

bricelam commented 5 years ago

You're all aware you can just reorder the columns in the CreateTable call of the generated migration, right? No need to refactor your entire model or contort your workflow...

administersoftware commented 5 years ago

No I did not. I will begin googlebinging, but if there is anything you can point me in the direction of I would be most grateful indeed.

Thank you.

bricelam commented 5 years ago
migrationBuilder.CreateTable(
    name: "Posts",
    columns: table => new
    {
        // TODO: Reorder columns here
        PostId = table.Column<int>(nullable: false),
        Title = table.Column<string>(nullable: true),
        Content = table.Column<string>(nullable: true),
        BlogId = table.Column<int>(nullable: false)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_Posts", x => x.PostId);
    });
administersoftware commented 5 years ago

Thanks, I don't do migrations yet, I only build the database as application is still in development, so dropping and rebuilding locally is fine, schema changes are then done to online database used by devs, but will look into this right now.

PatrickOliveros commented 5 years ago
migrationBuilder.CreateTable(
    name: "Posts",
    columns: table => new
    {
        // TODO: Reorder columns here
        PostId = table.Column<int>(nullable: false),
        Title = table.Column<string>(nullable: true),
        Content = table.Column<string>(nullable: true),
        BlogId = table.Column<int>(nullable: false)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_Posts", x => x.PostId);
    });

I am quite aware of this workaround although it defeats the purpose of doing migrations as there's another step that has to be done to get the order correctly. I thought the release of .NET Core 3.0 would have this one fixed and is it safe to assume that this won't even be available by .NET 5 release?

administersoftware commented 5 years ago
migrationBuilder.CreateTable(
    name: "Posts",
    columns: table => new
    {
        // TODO: Reorder columns here
        PostId = table.Column<int>(nullable: false),
        Title = table.Column<string>(nullable: true),
        Content = table.Column<string>(nullable: true),
        BlogId = table.Column<int>(nullable: false)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_Posts", x => x.PostId);
    });

Thanks for this, a workaround it is, but a lot of pain :( Hopefully a better automated resolution will be available in due course.

BernardBoakye commented 5 years ago
migrationBuilder.CreateTable(
    name: "Posts",
    columns: table => new
    {
        // TODO: Reorder columns here
        PostId = table.Column<int>(nullable: false),
        Title = table.Column<string>(nullable: true),
        Content = table.Column<string>(nullable: true),
        BlogId = table.Column<int>(nullable: false)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_Posts", x => x.PostId);
    });

Imagine you are working on large data entities with a lot of base entities. That will be really tedious.

administersoftware commented 5 years ago

It sure was for me, took a couple of hours to do and check, sure hope it gets fixed soon.

fradsham commented 5 years ago

I have been following this thread for a while. I was hoping that it would have been addressed with net core 3. I have a project with 57 objects and some are inherited, so reordering the builder files is not feasible. I have tested different approaches. For example to handle logical deletion as opposed to physical deletion of records, I have a root class that all are inherited from (code below). The DeletedBy field should appear as the second last field given its high column number (600) but it does not. It is always the third field any any generated migration files. The columns in the child class all have lower numbers.

public class SoftDeleted : ISoftDeleted
{
    [Column(Order = 601)]
    public DateTime? DeletedOn { get; set; }
    [Column(Order = 600)]
    public String DeletedBy { get; set; }
}
no3Ldev commented 5 years ago

Also having the same problem that I have been working on for a while now. DataAnnotation "Column -> Order" parameter does not work. Regardless whether you place column order annotation on the base class with higher index, or on the inheriting class starting from 0, order still follows its own style - primary key first followed by the columns from the base class, then the remaining columns from the inheriting class.

[NewClass : BaseClass]
- pk_column
- column2
- column3

[BaseClass]
- base_userid
- base_timestamp

[DB Result]
- pk_column
- base_userid
- base_timestamp
- column2
- column3

Still looking for workarounds, anyone?

Luis-Palacios commented 5 years ago

@no3Ldev sadly the only workarounds at the time are not using inheritance or sorting manually the CreateTable methods generated on the migrations ๐Ÿ˜ข

no3Ldev commented 4 years ago

well that's what i thought. this concern has been quite around for some time already. thank you

TanvirArjel commented 4 years ago

This is a very weird bug which should have been fixed in version 1.1. But it seems it would not be fixed even in EF Core 8.0 in 2023. Frustrated! very frustrated.

pklejnowski commented 4 years ago

Is there any workaround for this?

tehZeno commented 4 years ago

Is there any workaround for this?

I manually edited the files and changed the order; but it's a royal pain in the behind. Don't think there is a real workable workaround yet.

administersoftware commented 4 years ago

Is there any workaround for this?

I'm still manually edit the migration file, and as @tehZeno says, it's a royal pain in the behind :(

drewpayment commented 4 years ago

Is there any workaround for this?

This is wild. There are a multitude of issues that have been opened over the course of the last five years, closed for various reasons and ultimately hundreds, or possibly thousands, of hours wasted because no one has just fixed this issue.

I was thinking about using EF Core, but I mean......... this is painful after the last 30 minutes of reading through these issues.

administersoftware commented 4 years ago

Is there any workaround for this?

This is wild. There are a multitude of issues that have been opened over the course of the last five years, closed for various reasons and ultimately hundreds, or possibly thousands, of hours wasted because no one has just fixed this issue.

I was thinking about using EF Core, but I mean......... this is painful after the last 30 minutes of reading through these issues.

@drewpayment I definitely agree to the number of hours wasted on this one issue, but I also understand that there are more important issues for the team to be focussed on, what we need is more people to upvote the priority of this issue. Atleast the issue remains open :)

Luis-Palacios commented 4 years ago

In any case for those of you that like me are manually ordering the migrations, have you found a way to order columns when adding a new column to an existing table?

TanvirArjel commented 4 years ago

@Luis-Palacios No way! You have to do it manually.

premchandrasingh commented 4 years ago

I managed ordering by customizing SqlServerMigrationsAnnotationProvider and SqlServerMigrationsSqlGenerator. Even though it says internal API, I think it is much cleaner until available out of the box. Here is git repo https://github.com/premchandrasingh/EFCoreColumnOrder