RevEng: generated props .HasMaxLength(x).HasColumnType("varchar") create varchar(1) columns in DB

[umu24013]

The dnx command with the -a option
dnx ef dbcontext scaffold <connection string> EntityFramework.MicrosoftSqlServer -a -c <context name> -o <output dir>

generates incomplete entity properties for the varchar data type in the OnModelCreating method:

for nvarchar(x) it generates .HasMaxLength(x)
but for varchar(x) it generates .HasMaxLength(x).HasColumnType("varchar")
where x is data length

Then, when I call Database.EnsureCreated() from the generated dbcontext, it creates varchar(1) columns in the DB rather then varchar(x).

As a result, the SQL Server throws en exception "String or binary data would be truncated" when trying to write to those castrated columns.

[lajones]

Thanks for reporting this @umu24013. I'll look into it.

[rowanmiller]

Once we have IsUnicode API (#3420) the code should be .HasMaxLength(x).IsUnicode(false). In the meantime it should just be HasColumnType("varchar(x)"). The HasMaxLength call isn't needed because the length is specified in the datatype... but if it's less work just to leave it in, then that is fine (we'll want it when we swap to IsUnicode anyway).

[lajones]

Fix is checked in with #4348, commit 443c52b.

Details: using both HasColumnType() and HasMaxLength() at the same time doesn't work. Because the column type contains (either implicitly or explicitly) a max length we just ignore the HasMaxLength() call and use the column type defined by HasColumnType(). When scaffolding generated e.g. .HasColumnType("varchar").HasMaxLength(100) it was expecting that migrations would treat that as an underlying data type of varchar(100). What actually happened, as mentioned at the top of this issue, was that the HasMaxLength() call was ignored and migrations assumed a data type of varchar which SQL Server treats as varchar(1).

In addition to that for properties with type string the default SQL Server data type assumed by migrations is nvarchar(max). So if that is the type you intend you can skip defining HasColumnType() altogether. If you skip HasColumnType() then you can and should use HasMaxLength(). But if e.g. you have a string property but the data type you want it to be on SQL Server is e.g. varchar(max) (note: not nvarchar(max)) you cannot rely on the default mapping, so you must use HasColumnType() which means you must declare the length of the type within that call i.e. HasColumnType("varchar(max)") rather than just HasColumnType("varchar"). Similarly for byte[] properties the default SQL Server type is varbinary(max) and you get similar concerns.

So I updated the scaffolding code to look again at any string and byte[] properties it is generating and make sure that, as listed in the above rules, either HasColumnType() or HasMaxLength() is called, or neither of them, but definitely not both. (And of course that it you specify using annotations then the [MaxLength] annotation will be output instead of a call to HasMaxLength() - however this happens less often now as we often have to call HasColumnType() specifying a length qualification within it - and there is no matching annotation for that).

Lastly, since the big change to use dotnet, dnx commands at the command-line don't work any more. The work to fix that is tracked in #3925. In the meantime the only way to test this is to use Scaffold-DbContext from the Package Manager within VS.

[divega]

(And of course that it you specify using annotations then the [MaxLength] annotation will be output instead of a call to HasMaxLength() - however this happens less often now as we often have to call HasColumnType() specifying a length qualification within it - and there is no matching annotation for that).

@lajones just to make sure I understand, do we still generate properties of a specific CLR type and either [MaxLenght] or .HasMaxLenght() for the most common store types? Ideally we shouldn't generate .HasColumnType() calls unless it is needed.

[lajones]

@divega Depends what you mean. If the underlying store type is nvarchar(x) or varbinary(x) or any non-string, non-byte[] property then yes. But e.g. varchar(x) is pretty common and we need to generate HasColumnType("varchar(x)") for that.

[lajones]

BTW @divega The fluent API tests and the annotations tests have the results for an AllDataTypes table which now has every conceivable data type including synonyms.

[divega]

Thanks for the links. I see that for something like a column Name of type nvarchar(100) we are already just generating .Property(e => e.Name).HasMaxLenght(100).

I think once we have #3420 fixed it should be possible to just use .Property(e => e.Name).HasMaxLenght(100).IsUnicode(false) for varchar(100) as well.

[lajones]

@divega Agreed. See comment from @rowanmiller above.

[divega]

Got it. Thanks for clarifying.

[lajones]

@divega No worries. BTW are we planning to provide a [Unicode(false)] annotation to go with that?

[divega]

We could. As a general principle I would be conservative about adding new annotations on our own initiative and wait for customer feedback or PRs instead 😄