[RevEng] Output the HasColumnType() and HasMaxLength() APIs correctly

[lajones]

Fix for #4312.

For SQL Server this means for nvarchar columns (or synonyms) we use HasMaxLength() and not HasColumnType(), for other string types we use HasColumnType() and not HasMaxLength(). Similarly for varbinary (or synonyms) use HasMaxLength() and not HasColumnType(), but for other binary column types use HasColumnType() and not HasMaxLength().

[natemcmaster]

Anyway we can capture this in the SqlServer type mapper? I thought it was already designed to handle lengths.

[natemcmaster]

Seems like we could make improvements to reuse type mapping API. Perhaps we need new API, such as IRelationalTypeMapper.FindMapping(string typeName, int length). We already have some API for handling sized type mappings (RelationalSizedTypeMapping)

Thoughts on this @ajcvickers ?

[lajones]

@natemcmaster @ajcvickers Nit: It would definitely have to be int? length.

The problem is that different data types interpret having a null max length differently. For image, text etc it is legal (in fact expected) and means "max length doesn't apply to this data type", for nvarchar, varbinary etc it means "equivalent to nvarchar(max), varbinary(max)" etc. For other types it would make no sense (e.g. nchar, char) and would be an error.

[natemcmaster]

It would definitely have to be int? length.

Or an overload.

The problem is that different data types interpret having a null max length differently....

To clarify, my point was that this logic could be placed in SqlServerTypeMapper instead of SqlServerScaffoldModelFactory.

[lajones]

@natemcmaster Sort of. This is not just about type mapping. It would be perfectly legal for us to output e.g. HasColumnType("nvarchar(100)") but we have chosen instead to write out HasMaxLength(100) and rely on the fact that the code we produce will define the CLR type for that column as a string and the default mapping for a string is back to nvarchar.

So in the end we need 2 pieces of information out: 1) should we use HasColumnType() or HasMaxLength() and 2) if HasColumnType(), what is the full definition of the column type we want to use, including any length qualifications.

I'll try to put all of that in SqlServerTypeMapper and see what it looks like.

[lajones]

@ajcvickers Have updated as suggested above - could you give the final sign-off since Nate is away?

[lajones]

Removing "Needs Review" while I rebase this on the renamed codebase and get it working again. Will put it back on when ready for review again.

[jdlundquist]

What about rowversion and uniqueidentifier - would these map to byte[] and string CLR types, respectively, and need to be excluded?

Also, I don't think date, datetime, or smalldatetime allow max length (at least per SQL Server, not sure about HasMaxLength()), but those would be excluded implicitly by CLR type in MaxLengthQualifiedDataType

[lajones]

Please don't review just yet - I'm in the process of updating this and will add the "Needs Review" tag back when it's ready. Nonetheless I take your point and will check those scenarios.

[lajones]

I tried to put the code in SqlServerTypeMapper but what we are doing in scaffolding right there is overriding already assigned column types and max lengths. And sometimes it's important not to override the existing values - so it's not really a generic "output column type and max length for this column" method which would be more generally useful throughout the stack - it's quite specific to what scaffolding needs. I agree such a method would be useful and I'll raise another issue to suggest looking into SqlServerTypeMapper and redesigning so that there is such a method. But for this issue, after discussion with @natemcmaster, I've moved most of the code back into SqlServerScaffoldingModelFactory. I did have to leave in one change to SqlServerTypeMapper as a binary data type should not be mapped to _varbinary otherwise it thinks its DefaultTypeName is varbinary and we end up outputting HasMaxLength() instead of HasColumnType().

That said there are real synonyms for some types e.g. varbinary has the synonym binary varying. So I've updated the tests so we now test DDL using all data types including synonyms and check that we get the correct HasColumnType() and HasMaxLength() fluent API as a result of that.

[natemcmaster]

LGTM.

[lajones]

Checked in with commit 443c52b.

[lajones]

Note: opened issue #4425 to track trying to make the SqlServerTypeMapper APIs more useful as discussed with @natemcmaster.