Indexes specified by attribute do not change type mappings appropriately

[ajcvickers]

When I have this entity type:

public class User
{
    public int Id { get; set; }
    public string Name { get; set; }
}

And configure it like this:

modelBuilder.Entity<User>().HasIndex(e => e.Name);

Then EF uses the "key or index" mapping for the string type, which results in this:

CREATE TABLE [Users] (
    [Id] int NOT NULL IDENTITY,
    [Name] nvarchar(450) NULL,
    CONSTRAINT [PK_Users] PRIMARY KEY ([Id])
);

CREATE INDEX [IX_Users_Name] ON [Users] ([Name]);

Notice that nvarchar(450) is used.

However, if I do this:

[Index(nameof(User.Name))]
public class User
{
    public int Id { get; set; }
    public string Name { get; set; }
}

Then creating the database throws because Name is still using nvarchar(max).

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (5ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE TABLE [Users] (
          [Id] int NOT NULL IDENTITY,
          [Name] nvarchar(max) NULL,
          CONSTRAINT [PK_Users] PRIMARY KEY ([Id])
      );
fail: Microsoft.EntityFrameworkCore.Database.Command[20102]
      Failed executing DbCommand (2ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE INDEX [IX_Users_Name] ON [Users] ([Name]);
Unhandled exception. Microsoft.Data.SqlClient.SqlException (0x80131904): Column 'Name' in table 'Users' is of a type that is invalid for use as a key column in an index.
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean isAsync, Int32 timeout, Boolean asyncWrite)
   at Microsoft.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String methodName)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IEnumerable`1 migrationCommands, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Storage.RelationalDatabaseCreator.CreateTables()
   at Microsoft.EntityFrameworkCore.Storage.RelationalDatabaseCreator.EnsureCreated()
   at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.EnsureCreated()
   at Program.Main() in /home/ajcvickers/AllTogetherNow/Daily/Daily.cs:line 22
ClientConnectionId:ac59a862-d8f7-44f8-95be-c95a41b4b94d
Error Number:1919,State:1,Class:16

[smitpatel]

IndexAttributeConvention runs after TypeMappingConvention causing issue.

[AndriySvyryd]

IndexAttributeConvention should be converted to an interactive convention by implementing IEntityTypeAddedConvention and IPropertyAddedConvention instead of IModelFinalizingConvention

[lajones]

IndexAttributeConvention should be converted to an interactive convention by implementing IEntityTypeAddedConvention and IPropertyAddedConvention instead of IModelFinalizingConvention

@AndriySvyryd Can you explain more why this is necessary and what would go in these methods?

1. I have it apparently working having moved the convention before TypeMappingConvention. What test scenario am I missing?
2. As you remember, the original design had IndexAttributeConvention as an EntityTypeAttributeConventionBase which had a ProcessEntityTypeAdded() method (see this commit). But we moved to an IModelFinalizing convention because we had to check whether the strings passed in to the IndexAttribute corresponded to real properties and if they were not throwing was inappropriate before finalization. Why would that problem not come up again?

[AndriySvyryd]

@lajones There are two problems with IndexAttributeConvention or any convention running as IModelFinalizing:

1. Order is important and fragile as we see with this issue. At some point there will be a convention A that need to run after B, but B depends on changes made by A.
2. Users can't act on the changes made by non-interactive convention. We've recommended users to use Metadata API in lue of light-weight conventions, but it won't pickup indexes created by the convention:

foreach (var entityType in modelBuilder.Model.GetEntityTypes())
{
    foreach (var index in entityType.GetDeclaredIndexes())
    {
        index.SetDatabaseName("My" + index.Name);
    }
}

In ProcessEntityTypeAdded the convention will have to do nothing if there are missing properties. If the properties are added explicitly later then the index would be created in ProcessPropertyAdded. If the properties are still missing by when ProcessModelFinalizing runs then it will throw, but it won't mutate the model in any scenario.

[lajones]

So would the following be correct?

1. Add ProcessEntityTypeAdded and ProcessPropertyAdded which will both do what ProcessModelFinalizing does now except if the properties are in any way incorrect they will both just do nothing (no error, and no index created).
2. ProcessModelFinalizing needs to remain - but only to do error checking - i.e. it does what it does now except will no longer create the index.

[AndriySvyryd]

@lajones Yes