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.72k stars 3.17k forks source link

Allow defining column order via [ColumnAttribute.Order] when creating tables #10059

Closed bricelam closed 3 years ago

bricelam commented 7 years ago

2272 matched the CLR reflection order for columns within a table.

During design, we decided to wait for more feedback before honoring the value of ColumnAttibute.Order on the properties.

If the implementation of #2272 is insufficient for you and specifying something like [Column(Order = 1)] would help, please vote for this issue and add details about your scenario (if not already listed) below.

yitzchokneuhaus1 commented 6 years ago

@bricelam Is this enhancement scheduled for the near future to be implemented in either EF Core or EF6?

bricelam commented 6 years ago

It works in EF6. Still on the Backlog of EF Core.

samconn commented 6 years ago

+1 for implementing this feature, and sooner rather than later.

We're porting an SAAS application from EF6x to EFCore and were a wee bit taken aback to find that the Order designation on the ColumnAttribute is being ignored.

But kudos to the whole team (and community!) for at least going open-kimono on the design discussions to at least help us understand the reasoning.

bricelam commented 6 years ago

@andytaw mentioned another good case for this:

Column order is of no great significance if all data operations go through EF. I need to use SQL Bulk Copy as well as EF. I could (will...) write a whole bunch of tedious code to manage the two, however I'd much prefer to use .HasColumnOrder().

MelGrubb commented 6 years ago

Those who work in the database all day tend like their columns in decreasing order of "importance", with the most significant or often-used coming at the start, and picky details way over on the right out of the way, or sometimes they like them clustered in neighborhoods where related fields are next to each other. Either way, this means that the column order can be significant to some team members. Meanwhile, over in the land of code, I like to use tools like ReSharper or CodeMaid to automatically sort my C# code, which drastically reduces merge collisions. Really, I can't recommend it enough for team projects, it's a huge gain in productivity.

As it stands now, I can't make both sets of developers happy. I really need a way to specify the order of the database columns, and it seems to be that using the existing DataAnnotations' ColumnAttribute would be an ideal way to do this.

As for strategy, I'd recommend putting the properties with the attribute first, in increasing order according to the Order property of the attribute, and then the remaining properties in their existing "natural" order, although that order is non-deterministic and shouldn't be relied upon to remain stable in the future.

pwen090 commented 6 years ago

@bricelam you mentioned it on #2272 but to reiterate and upvote here if we have something like:

public class Blog
{
    public int BlogId { get; set; }
    public string Url { get; set; }
}

public class RepoBlog : Blog
{
    [Key]
    public int Id { get; set; }
}

This will cause the primary key Id to be output as the last column. I know we can edit migration files but surely somewhere along the lines someone will forget to do that and make it a pain to clean up. It would be great if we can either support Order attribute or at least have the generators try harder to always put key columns first.

bricelam commented 6 years ago

@pwen090 Tracked by https://github.com/aspnet/EntityFrameworkCore/issues/11314#issuecomment-375159209

smitpatel commented 6 years ago

Afaik the model posted by @pwen090 wouldn't work if Blog is mapped since derived types cannot have keys.

davisnw commented 5 years ago

Is there any workaround right now before this gets implemented? HasColumnOrder doesn't seem to be supported either, and it makes it a real pain to check that the mappings are in sync with a database that is not primarily under the control of EntityFramework. E.g, generate database with e.g. EnsureCreated and doing a schema comparison with SSDT - but if I can't control EF column order, the comparisons don't line up. I was thinking there might be something in the metadata api, but I haven't found it yet if it exists.

ajcvickers commented 5 years ago

@davisnw I'm not aware of any workaround for using EnsureCreated directly. The normal workaround is to scaffold a migration and arrange the column order in the migration.

kayakingcoder commented 5 years ago

As for strategy, I'd recommend putting the properties with the attribute first, in increasing order according to the Order property of the attribute, and then the remaining properties in their existing "natural" order, although that order is non-deterministic and shouldn't be relied upon to remain stable in the future.

This would not work in our scenario where we have LastModifiedDate and LastModifiedUserId as properties in a base class. I'd want a way to put those two fields at the end of my tables (as per the "importance" that @MelGrubb mentions) without being forced to put an order attribute on every property of descendant classes, instead they currently appear straight after the id.

I would prefer this strategy from https://linq2db.github.io/api/LinqToDB.Mapping.ColumnAttribute.html "Specifies the order of the field in table creation. Positive values first (ascending), then unspecified (arbitrary), then negative values (ascending)."

MelGrubb commented 5 years ago

Couldn't you just put attributes with ridiculously high values on the base classes?

kayakingcoder commented 5 years ago

Couldn't you just put attributes with ridiculously high values on the base classes?

It doesn't matter how high you make those values, if the rule is to put the columns WITH the attribute first, then we'd be forced to put the attribute on every other column. The way they've done it in Linq to DB per that link I gave is just more flexible, why not go with that?

liamwang commented 5 years ago

It would be more perfect to make below conventions as default:

  1. Primary keys first
  2. Base class properties last

That way, I don't even need to set ColumnAttibute.

For example:

public class AuditEntity
{
    public int Id { get; set; }

    public int CreatedBy { get; set; }
    public DateTime CreatedAt { get; set; } = DateTime.Now;
    public int UpdatedBy { get; set; }
    public DateTime UpdatedAt { get; set; } = DateTime.Now;
}

public class Member : AuditEntity
{
    public string Name { get; set; }
    public string Avatar { get; set; }
}

The ordered columns of Member should be:

Id,
Name,
Avatar,
CreatedBy,
CreatedAt,
UpdatedBy, 
UpdatedAt
zgonzales commented 4 years ago

Would love to see this implemented, really wish that when adding a new column to a table it would be ordered before the less important existing inherited fields instead of at the end.

davisnw commented 4 years ago

Can someone explain the resistance to allowing us to simply specify the desired order somewhere in our EntityFramework mappings rather than having to scaffold and manually edit migration files?

  1. Order attribute is not supported
  2. Specifying order via fluent api is not supported
  3. Order doesn't doesn't seem to be exposed via the metadata api in any way that would let us write our own convention (this would provide the greatest flexibility for projects to order as they please)

I suggest that Entity Framework Core should not put so much effort into guessing how we like our column orders, and just let us specify them easily in our mappings. Different projects have different rules and different interactions between the application and database administrators.

roji commented 4 years ago

Note that databases in general do not support adding columns to a table in a specific position, or altering the position of an existing column (SQL Server, PostgreSQL) - any change in order requires a table rebuild, which can be quite complex and seems problematic for something as cosmetic as a column ordering change. So while something could be implemented here for initial table creation, as the schema evolves things would get out of sync.

@davisnw if you look at the comments above, you'll see that people are asking for different things - automatic ordering by EF Core when using inheritance, or just manually via the attribute, or both. This isn't to say nothing should be implemented - just that some careful thought into the precise design is needed.

davisnw commented 4 years ago

@roji Thanks for the response.

With regards to "adding columns in a specific position ... requires a table rebuild" for many cases, that is not a big deal (tables are relatively small). Though I agree that if you are using entity framework for migrations, that the need for a table rebuild should be surfaced to the programmer.

With regards to column order being "cosmetic" - column order is for the humans, who often have strong preferences for a specific column order, because they are often running queries directly in the database, bypassing Entity Framework entirely.

With regards to "as the schema evolves things would get out of sync.", Entity Framework Core is not always the primary driver of database changes. I somewhat frequently work on projects where Entity Framework is just the "application glue", and "keeping things in sync" is typically going from the database changes (sometimes managed by Sql Server Data Tools) back to Entity Framework, and thus have little use for Entity Framework Core migrations (other than hacking column order to make schema comparison easier). Being able to specify column order more simply would make that job easier.

With regard to people wanting different things, that is why I suggest that it should be easier for us to specify our own column order (understanding that ColumnAttribute.Order may not be the clearest way to do that).

I did find (off of issue https://github.com/dotnet/efcore/issues/11314#issuecomment-586265217) that someone managed to do HasColumnOrder(...) (not ColumnAttribute.Order) by using "internal" apis (so could be broken in the future).

So perhaps I should file a separate issue regarding the metadata api - have to do a little more digging on that.

roji commented 4 years ago

@davisnw thanks for the additional comments - just to be clear, I wasn't saying we shouldn't do anything, just voicing some observations.

With regards to column order being "cosmetic" - column order is for the humans, who often have strong preferences for a specific column order, because they are often running queries directly in the database, bypassing Entity Framework entirely.

I don't doubt that - maybe the word "cosmetic" was too strong; I only meant that it has no effect on EF Core and on typical programmatic access methods to the table. The 63 votes on this issue do show that people care about this for sure.

With regards to "as the schema evolves things would get out of sync.", Entity Framework Core is not always the primary driver of database changes

That's absolutely true, but in that case the problem of adding columns and enforcing column order is out of EF Core's scope, isn't it? It becomes the concern of whatever external tool you use to change your database schema.

MelGrubb commented 4 years ago

I'd say it's more than cosmetic. Whether you try to pretend the database is there or not, eventually you end up opening SSMS and "SELECT * FROM WHERE"ing a table to track down a problem. At that point, the order of the columns becomes important. On a wide table with several fields that are inherently more "interesting" than others, I want to see the interesting ones first. Audit fields like created and modified date/by are pretty boring and yet "created" alphabetizes right up there at the front, taking screen space away from the things I wanted to see. Yes, I can hand-craft a query to get to the good parts, but do you have any idea how many times a day I right-click and select the top 1000 rows? It's ridiculous.

As for having to rebuild the table. I'd say EF should just do its best with what it has. The first time the table builds, it should obey my ordering attributes absolutely. After that, when I add columns, it should use the metadata to add the new columns properly ordered in relation to each other, but not necessarily in relation to the existing columns.

Perhaps an optional parameter or configuration option could say that you are okay with the full table rebuild, and to just go ahead and do it, but by default, I'd favor the simpler faster option. New stuff goes at the end, sorry.

videokojot commented 4 years ago

As for strategy, I'd recommend putting the properties with the attribute first, in increasing order according to the Order property of the attribute, and then the remaining properties in their existing "natural" order, although that order is non-deterministic and shouldn't be relied upon to remain stable in the future.

This would not work in our scenario where we have LastModifiedDate and LastModifiedUserId as properties in a base class. I'd want a way to put those two fields at the end of my tables (as per the "importance" that @MelGrubb mentions) without being forced to put an order attribute on every property of descendant classes, instead they currently appear straight after the id.

I would prefer this strategy from https://linq2db.github.io/api/LinqToDB.Mapping.ColumnAttribute.html "Specifies the order of the field in table creation. Positive values first (ascending), then unspecified (arbitrary), then negative values (ascending)."

This is also our desired scenario, we want to have some columns from abstract base class in the beginning and some columns last. Thanks.

mjavad007 commented 4 years ago

Implementing this would also help in adding composite key using data annotations. if ef6 we could use column to denote the order of composite key. Since this is not implemented in ef core we get error "entity type has composite type defined with data annotations, use fluent api" in ef core. So implement this or add parameter to key to denote order of composite key.

ajcvickers commented 4 years ago

@mjavad007 Conflating column order with key order was a mistake in EF6 since the two can be different. #11003 is tracking composite key mapping by attribute.

Matt11 commented 3 years ago

I can see so many discussions, but nothing on the current/final state. Is it solved?

I was trying column ordering for colums when a table is being created via code first and it does not work for me. ef core 5.0 6 latest stable version.

roji commented 3 years ago

@Matt11 the issue is open and in the 6.0.0 milestone, so it wasn't done for 5.0.

Fosol commented 3 years ago

Column ordering is important to some clients. So much so they have schema standards that must be followed to pass quality/approval gates. Results in client rejecting EF Core ORM features that makes development more efficient, consistent, stable.

bricelam commented 3 years ago

@Fosol Now I'm curious, do they actually go to the extent of rebuilding the entire table when adding a column? Like, they actually wait for all the indexes to be rebuilt just to add a new column--just so it doesn't appear after ModifiedOn in their tools?

Fosol commented 3 years ago

They're a government agency used to waterfall projects. Schema drawn and created before hand. Changes for them wouldn't occur often... However we're on an Agile contract with constant change. So there are more issues at work here, however they're pushing for hand-bombing sql migration scripts and we're pushing using tools to automate. They are allowing the tools if they can maintain their standards...

So we're running into all the deltas now and having the back and forth fight to address what really matters and what doesn't... Of course it's hard to convince someone who has a rule book they have lived by for over a decade...

As for your specific question. I think if we asked them straight out, they might ask for rebuilding indexes and resorting columns if new ones are added. However reality is they may not notice until it would become too much effort to do it... Lesson in life, sometimes it's best to not share all information...

TanvirArjel commented 3 years ago

@bricelam, Will there be any Fluent API version of this [ColumnAttribute.Order]? It would be great if there is a Fluent API version of this so that we can decide the order of the shadow property too.

bricelam commented 3 years ago

@TanvirArjel I'll need to discuss with the team. Our current approach is to not and metadata to the EF model for order, but instead, read the CLR annotations directly inside of Migrations. This approach will not work for shadow properties.

One crazy idea that could work: (but may lead to a whole new set of problems)

[MetadataType(typeof(MyEntityMetadata))]
class MyEntity
{
    // ...
}

class MyEntityMetadata
{
    [Column(Order = 66)]
    public string MyShadowProperty { get; set; }
}
bricelam commented 3 years ago

Design Meeting Notes


For reference, here is the default order.

  1. Primary key properties (recursive)
  2. Properties
    1. Declared properties. Reflection order
      1. Table-split properties grouped by navigation property (recursive)
    2. Derived type properties. Grouped topologically (recursive)
    3. Shadow and indexer properties. Alphabetical
  3. Unmapped base type properties. Grouped topologically (recursive)
maulik-modi commented 3 years ago

@Fosol , I agree with you. This becomes important for reporting tables where there are 30-40 columns. @Fosol Have you checked Nhibernate? @roji , Is there any possibility of doing in EF Core 7?

roji commented 3 years ago

@maulik-modi at the moment, the plan is still to get it in for 6.0.

Fosol commented 3 years ago

@maulik-modi I have looked at Nhibernate. It's a possible option for some work, but not in the current projects I'm engaged on.

Dzivo commented 3 years ago

Ordering doesnt work if you have following structure:

  public class EntityBase<TEntity> : IEntityBase<TEntity>
    where TEntity : class, IEntityBase<TEntity>
  {
    [Key, Column(Order = 0), DatabaseGenerated(DatabaseGeneratedOption.Identity)]
    public virtual int Id { get; set; }

    public Func<TEntity, object[]> PrimaryKey()
    {
      return m => new object[] { m.Id };
    }
  }
  public class TenantEntityBase<TEntity> : EntityBase<TEntity>, ITenantEntity
    where TEntity : class, IEntityBase<TEntity>
  {
    [Column(Order = 1), DatabaseGenerated(DatabaseGeneratedOption.Identity)]
    public virtual Guid TenantId { get; set; }
  }
public class MyClass: TenantEntityBase<MyClass>
  {
    public string Name { get; set; }
 }

Generated Migration:

migrationBuilder.CreateTable(
    name: "MyClass",
    columns: table => new
    {
        Id = table.Column<int>(type: "int", nullable: false)
            .Annotation("SqlServer:Identity", "1, 1"),
        Name = table.Column<string>(type: "nvarchar(max)", nullable: true),
        ZLastProp = table.Column<string>(type: "nvarchar(max)", nullable: true),
        TenantId = table.Column<Guid>(type: "uniqueidentifier", nullable: false)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_MyClass", x => x.Id);
    });
TanvirArjel commented 3 years ago

@Dzivo Please add the migration output here.

roji commented 3 years ago

@Dzivo better yet, please open a new issue with the full details.

Dzivo commented 3 years ago

Just a sec i will make new project fast to see if it happens there

Dzivo commented 3 years ago

Here is whole example:

https://we.tl/t-cQLQ3E0pOA

it uses local docker sql server version docker pull mcr.microsoft.com/mssql/server

But you can change connection string in startup.cs

All classes are defined in Context.cs

Dzivo commented 3 years ago

Did you take a look at it ? Thoughts ?

roji commented 3 years ago

@Dzivo I'll take a look, but can you please open a new issue and post the repro there?

ajcvickers commented 3 years ago

@Dzivo Your project uses EF Core 5.0.9. This issue is fixed in EF Core 6.0 RC2 as indicated by its milestone, which will be released soon, or you can get it from a daily build.

anshuldubey commented 2 years ago

The column ordering issue is still there for UPDATE statements. When we try to perform UPDATE operation from EFCore, it changes the order of primary keys alphabetically. That's strange because SELECT query executed from EFCore retains the correct order.

Using EF Version: 5.0.2

roji commented 2 years ago

@anshuldubey this issue is about the order of columns in the table, when they're created via migrations - not about the order in which columns are specified in SELECT or UPDATE statements. I'm not aware of any importance to column order in SELECT/UPDATE; if you feel there's a problem, please open a new issue, with an explanation of why you think the any change is needed.