ForSqlServerInclude does not respect column mapping

[jirikanda]

Steps to reproduce

For this model and mapping:

	public class SomeTable
	{
		public int Id { get; set; }
		public int Value1 { get; set; }		
		public int Value2 { get; set; }
	}

	public class ApplicationDbContext : DbContext
	{
		protected override void OnModelCreating(ModelBuilder modelBuilder)
		{
			base.OnModelCreating(modelBuilder);

			modelBuilder.Entity<SomeTable>().ForSqlServerHasIndex(st => st.Value1).ForSqlServerInclude(st => st.Value2);

			modelBuilder.Entity<SomeTable>().Property(st => st.Id).HasColumnName("SomeTableId");
			modelBuilder.Entity<SomeTable>().Property(st => st.Value1).HasColumnName("SomeTableValue1");
			modelBuilder.Entity<SomeTable>().Property(st => st.Value2).HasColumnName("SomeTableValue2");
		}

		protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
		{
			base.OnConfiguring(optionsBuilder);

			optionsBuilder.UseSqlServer(@"Data Source=(localdb)\mssqllocaldb;Initial Catalog=EFCoreTest");
		}
	}

is created a migration

        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.CreateTable(
                name: "SomeTable",
                columns: table => new
                {
                    SomeTableId = table.Column<int>(nullable: false)
                        .Annotation("SqlServer:ValueGenerationStrategy", SqlServerValueGenerationStrategy.IdentityColumn),
                    SomeTableValue1 = table.Column<int>(nullable: false),
                    SomeTableValue2 = table.Column<int>(nullable: false)
                },
                constraints: table =>
                {
                    table.PrimaryKey("PK_SomeTable", x => x.SomeTableId);
                });

            migrationBuilder.CreateIndex(
                name: "IX_SomeTable_SomeTableValue1",
                table: "SomeTable",
                column: "SomeTableValue1")
                .Annotation("SqlServer:Include", new[] { "Value2" });
        }

See the difference: column: "SomeTableValue1" vs Annotation(..."Value2" }).

When scripted, a broken SQL Script is generated (column Value2 used in the index does not exists in the table).

CREATE TABLE [SomeTable] (
    [SomeTableId] int NOT NULL IDENTITY,
    [SomeTableValue1] int NOT NULL,
    [SomeTableValue2] int NOT NULL,
    CONSTRAINT [PK_SomeTable] PRIMARY KEY ([SomeTableId])
);

GO

CREATE INDEX [IX_SomeTable_SomeTableValue1] ON [SomeTable] ([SomeTableValue1]) INCLUDE ([Value2]);

GO

Further technical details

EF Core version: 2.2.0
Database Provider: Microsoft.EntityFrameworkCore.SqlServer

[NArnott]

Are there plans to fix this in 2.2? or only for 3.0?

I ran into the same issue. I even tried passing a string array (as opposed to the lambda expression), and it failed as well.

System.InvalidOperationException: Include property 'SomeTable.SomeColumn' not found at Microsoft.EntityFrameworkCore.Internal.SqlServerModelValidator.ValidateIndexIncludeProperties(IModel model) at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ValidatingConvention.Apply(InternalModelBuilder modelBuilder) at Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal.ConventionDispatcher.ImmediateConventionScope.OnModelBuilt(InternalModelBuilder modelBuilder) at Microsoft.EntityFrameworkCore.ModelBuilder.FinalizeModel()

[ajcvickers]

@NArnott We don't currently have any plans to port this back to 2.2. The workaround is to edit your migration to correct the error.

[NArnott]

@ajcvickers That works. I tried entering the column names manually on the ModelBuilder side, but that broke due to some "validation" failure. I didn't think to fix the migration itself after generation, but it appears to works. Thx.

[mirol-h]

What's the current situation regarding this bug? It has 3.0.0 milestone - is this being worked on? If not, I'd like to volunteer to work on this.

From what I'm seeing in SqlServerMigrationsSqlGenerator, this could be fixed in IndexOptions method by looking up the property in the model and using column name instead of property name to generate INCLUDE clause. So something like this (only the first part of IndexOptions method):

if (operation[SqlServerAnnotationNames.Include] is IReadOnlyList<string> includeProperties
    && includeProperties.Count > 0)
{
    builder.Append(" INCLUDE (");
    for (var i = 0; i < includeProperties.Count; i++)
    {
        var property = FindEntityTypes(model, operation.Schema, operation.Table)?
            .Select(e => e.FindDeclaredProperty(includeProperties[i]))
            .FirstOrDefault(p => p != null);
        builder.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(property?.GetColumnName() ?? includeProperties[i]));

        if (i != includeProperties.Count - 1)
        {
            builder.Append(", ");
        }
    }

    builder.Append(")");
}

Any thoughts on this fix?

[bricelam]

We need to resolve the column names sooner than that. This code needs to be updated from property names to column names:

https://github.com/aspnet/EntityFrameworkCore/blob/f0c8b019e94381215dd193b4b1c9575822d74574/src/EFCore.SqlServer/Migrations/Internal/SqlServerMigrationsAnnotationProvider.cs#L88-L94

[bricelam]

@mirol-h Feel free to send a PR

[mirol-h]

Done.

#16960

It does seem weird to have one annotation have different meaning in context of model (SqlServerIndexExtensions) and migrations (MigrationsModelDiffer and MigrationsSqlGenerator).

[bricelam]

In hindsight, I would separate the names for model and migrations annotations, but 🤷‍♂️ probably not worth it at this point