search

dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.72k stars 3.17k forks source link

RevEng: Fixed Length facet incorrect in ScaffoldingTypeMapper #14160

Closed smitpatel closed 5 years ago

smitpatel commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@smitpatel Looks wrong to me.

smitpatel commented 5 years ago

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

roji commented 5 years ago

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

smitpatel commented 5 years ago

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

roji commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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.