Fluent mapping with MaxLength giving type with no length

[nicolaspierre1990]

I'm trying to map a database from database first to code first. While I'm using IEntityTypeConfiguration classes to specify my mappings one problem occurs, the mapping of a char field to the database with a maxlength gives a RawSql of char with no specification of length.

Same for NCHAR and VARBINARY

Steps to reproduce

builder.Property(e => e.NotificationsLot).HasColumnType("char").HasMaxLength(5).IsFixedLength();

which compiles to

NotificationsLot = table.Column<string>(type: "char", fixedLength: true, maxLength: 5, nullable: true),

which in turn turns into

[NotificationsLot] char NULL,

Further technical details

EF Core version: 2.2.3 Database Provider: Microsoft.EntityFrameworkCore.SqlServer Operating system: Windows 10 Pro IDE: Visual Studio 2019 Preview 16.0.0 Preview 4.3

[ajcvickers]

Note for triage: we changed the behavior around this somewhat in the 2.x timeframe. Previously if you specified a store type it had to be the entire store type. We relaxed that such that you could combine, for example, "char" and HasMaxLength, but this doesn't seem to be working correctly.

[roji]

This is probably related to my work for 3.0 in #15048, I can investigate.

[nicolaspierre1990]

Any temporary workaround for now then?

[ajcvickers]

@nicolaspierre1990 Yes; use the fully specified type name:

builder.Property(e => e.NotificationsLot).HasColumnType("char(5)").HasMaxLength(5).IsFixedLength();

[nicolaspierre1990]

@nicolaspierre1990 Yes; use the fully specified type name:

builder.Property(e => e.NotificationsLot).HasColumnType("char(5)").HasMaxLength(5).IsFixedLength();

Same for NCHAR and VARBINARY ?

[roji]

@nicolaspierre1990 yes, that should work too.

[roji]

What's happening here seems rather simple:

• The user's HasColumnType("char") sets the property's configured column type
• The MigrationsModelDiffer constructs an AddColumnOperation, taking the property's configured column type
• The MigrationsSqlGenerator sees that the operation has a non-null ColumnType, and does not call GetColumnType() is where the call into the TypeMappingSource happens.

In other words, the TypeMappingSource is consulted for the store type only when it isn't configured by the user. If we want to support the scenario where HasColumnType("char").HasMaxLength(5) results in char(5), it seems we need to always call into the TypeMappingSource.

One possible thing to do, is to remove the TypeMappingSource lookup from MigrationsSqlGenerator altogether. The MigrationsModelDiffer would take the column type from the mapping instead of directly from the property (as it already takes the ClrType). Apart from fixing this issue, it seems like it would make things cleaner, removing any need to deal with mappings from MigrationsSqlGenerator and always providing a non-null ColumnType in AddColumnOperation.

But I'm a bit of a newcomer :) @bricelam you may have some light to shed on all the above...

[roji]

Note that I'm thinking about all this in general #15048. For example, as things are we seem to want to support HasColumnType("char").HasMaxLength(5), but not HasColumnType("char").IsUnicode(true) (which would presumably return nchar) - it seems a bit odd to support this for one facet but not for another. We may want to consider dropping this composition over a user-provided column type - either the user specifies a full store type including all facets, or they do not and we generate the store type for them (this would definitely simplify things).

But the refactor above, where the mapping lookup would happen in TypeMappingSource instead of MigrationsSqlGenerator, still makes sense to me.

[ajcvickers]

@roji Yes, the behavior until 3.0 was exactly that: if you specified a store type then it would be used verbatim. That was originally so we didn't need to parse the type and try to reconstruct with facets changed/added. We're a long way beyond that now. However, even though I relaxed the requirement in some places it clearly still needs work to be actually useful. In other words, I'm not surprised this is not working.

The main thing that we want to support--the thing people asked for--was to be able to specify a base string type and a length. Probably because you could do this in EF6. I'm fine with saying that a facet will only be applied to a type name if the type name does not already specify that facet. So, for example, a type name of char means non-unicode and fixed length on SQL Server. But most people interpret it to have no length (even though this is actually technically incorrect; it has a length of 1) so in that case adding the length do it is what people expect.

I'm also fine with going back to the original behavior and requiring type names to be specified fully if we think doing otherwise will be too confusing.

Also, remember that the store type name cannot be in the migrations snapshot unless it was explicitly set by the application. Otherwise it makes all migrations very provider-specific such that it would no longer be reasonable to have small amounts of conditional code for provider differences.