Efcore: RevEng: Fixed Length facet incorrect in ScaffoldingTypeMapper

[smitpatel]

https://github.com/aspnet/EntityFrameworkCore/blob/8e7402701ea1416b7fe55e1863ae65b3147c02c7/src/EFCore.Design/Scaffolding/Internal/ScaffoldingTypeMapper.cs#L117-L125

Above code is supposed to pass in default value for Fixed Length and compare type mapping to figure out if the value is same as default (hence no need to scaffold). Default for Fixed length is false and _not_ true.

[ajcvickers]

Presumably we are missing tests for this. Is this because of the general test debt for rev-eng, or was this just something we missed? /cc @bricelam

[smitpatel]

We do have tests (I am trying to understand how it is working in SqlServer)
https://github.com/aspnet/EntityFrameworkCore/blob/5d889997ca6496d4aa3e4030bdb02ba351856d47/test/EFCore.Design.Tests/Scaffolding/Internal/ScaffoldingTypeMapperSqlServerTest.cs#L277-L308

[smitpatel]

@ajcvickers - In the following piece of code, if the size is set on store type, then it is setting fixedLength as true. Is that correct? (e.g. varchar(300))
https://github.com/aspnet/EntityFrameworkCore/blob/5d889997ca6496d4aa3e4030bdb02ba351856d47/src/EFCore.Relational/Storage/RelationalTypeMappingInfo.cs#L95-L104

[ajcvickers]

@smitpatel Looks wrong to me.

[smitpatel]

@roji - Sorry, I am poaching this. Seems like double whammy. 2 bugs neglecting each other's effect.

[roji]

No problem - go ahead and take it, was happy to help uncover it.

[smitpatel]

Tests were there, they just did not verify if IsFixedLength facet was set correctly.

[roji]

One question - are you planning to release this in a patch for 2.2? If so I'll backport the Npgsql-side fix (https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/pull/750) on my side.

[roji]

@smitpatel one comment... in https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/issues/745 you wrote:

Basically, we are supposed to pass in the default value for the facet and see if the typemapping sets the facet as what we got from server. Hence we don't need to scaffold. For Unicode default is true, for size default is null, for fixed length default is ... false.

However, it still seems to me that these calls should pass in null rather than false, i.e. by omitting the fixedLength parameter altogether when calling FindMapping(). If false is passed, then the type mapping source will presumably always return a mapping with fixedLength false - just like it always returns true when true is specified. Unless I'm misunderstanding things, the idea is to see what the default fixedLength would be when not specified (i.e. null).

[smitpatel]

@roji - This is going to 3.0 since it's design time change only. User can modify the generated code to fix the issue. (unless bosses decide otherwise)
As for passed value, default is false. The function takes null argument but when you pass in null, it will go back to default values. Following lines in SqlServer shows that. So unless you pass false for unicode, it will be set as unicode.
https://github.com/aspnet/EntityFrameworkCore/blob/af13b52bfc390346551926ca56d5b0703c1eb0e7/src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs#L296-L297

I will update PR to pass in null since it would default to whatever provider default is eventually. I suppose the reason for passing true/false rather null in past may have been parameter not being nullable at some point.