Fix potential crash in SQL Server column retrieval code

[0xced]

Before this commit, scaffolding could crash with an ArgumentNullException.

System.ArgumentNullException: Value cannot be null. (Parameter 'column')
   at Microsoft.EntityFrameworkCore.Utilities.Check.NotNull[T](T value, String parameterName) in Microsoft.EntityFrameworkCore.Design.dll:token 0x6000111+0x1a
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.GetPropertyName(DatabaseColumn column) in Microsoft.EntityFrameworkCore.Design.dll:token 0x6000221+0x0
   at System.Linq.Enumerable.SelectListIterator`2.ToArray() in System.Linq.dll:token 0x60001dc+0x20
   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source) in System.Linq.dll:token 0x600011a+0x1b
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitIndex(EntityTypeBuilder builder, DatabaseIndex index) in Microsoft.EntityFrameworkCore.Design.dll:token 0x600022d+0xa2
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitIndexes(EntityTypeBuilder builder, ICollection`1 indexes) in Microsoft.EntityFrameworkCore.Design.dll:token 0x600022c+0x28
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitTable(ModelBuilder modelBuilder, DatabaseTable table) in Microsoft.EntityFrameworkCore.Design.dll:token 0x6000226+0xea
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitTables(ModelBuilder modelBuilder, ICollection`1 tables) in Microsoft.EntityFrameworkCore.Design.dll:token 0x6000225+0x28
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitDatabaseModel(ModelBuilder modelBuilder, DatabaseModel databaseModel) in Microsoft.EntityFrameworkCore.Design.dll:token 0x6000222+0x78
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.Create(DatabaseModel databaseModel, ModelReverseEngineerOptions options) in Microsoft.EntityFrameworkCore.Design.dll:token 0x600021e+0xf6
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.ReverseEngineerScaffolder.ScaffoldModel(String connectionString, DatabaseModelFactoryOptions databaseOptions, ModelReverseEngineerOptions modelOptions, ModelCodeGenerationOptions codeOptions) in Microsoft.EntityFrameworkCore.Design.dll:token 0x600023d+0xfe
   at Microsoft.EntityFrameworkCore.Design.Internal.DatabaseOperations.ScaffoldContext(String provider, String connectionString, String outputDir, String outputContextDir, String dbContextClassName, IEnumerable`1 schemas, IEnumerable`1 tables, String modelNamespace, String contextNamespace, Boolean useDataAnnotations, Boolean overwriteFiles, Boolean useDatabaseNames, Boolean suppressOnConfiguring, Boolean noPluralize) in Microsoft.EntityFrameworkCore.Design.dll:token 0x600043e+0x11c
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.ScaffoldContextImpl(String provider, String connectionString, String outputDir, String outputDbContextDir, String dbContextClassName, IEnumerable`1 schemaFilters, IEnumerable`1 tableFilters, String modelNamespace, String contextNamespace, Boolean useDataAnnotations, Boolean overwriteFiles, Boolean useDatabaseNames, Boolean suppressOnConfiguring, Boolean noPluarlize) in Microsoft.EntityFrameworkCore.Design.dll:token 0x60003ed+0x32
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.ScaffoldContext.<>c__DisplayClass0_0.<.ctor>b__0() in Microsoft.EntityFrameworkCore.Design.dll:token 0x60006ed+0x0
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.<>c__DisplayClass3_0`1.<Execute>b__0() in Microsoft.EntityFrameworkCore.Design.dll:token 0x60006f3+0x0
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action) in Microsoft.EntityFrameworkCore.Design.dll:token 0x600066a+0xc

After this commit, scaffolding produces warnings for columns with missing type information instead of crashing.

Could not find type mapping for column 'SalesLT.Address.StateProvince' with data type '???'. Skipping column.
Could not find type mapping for column 'SalesLT.Address.CountryRegion' with data type '???'. Skipping column.
Unable to scaffold the index 'IX_Address_AddressLine1_AddressLine2_City_StateProvince_PostalCode_CountryRegion'. The following columns could not be scaffolded: StateProvince,CountryRegion.
Unable to scaffold the index 'IX_Address_StateProvince'. The following columns could not be scaffolded: StateProvince.

Fixes #25729

[roji]

@0xced I'm not totally clear on the above... The current column loading query does an INNER JOIN on [sys].[types], so it filters out any columns where the type is unknown. Your change in #25745 instead loads these problematic columns, but there's no way they can be scaffolded correctly, since their type isn't known (your ??? type name will never correspond to type mapping later on). It's also unclear to me why you originally got a ArgumentNullException before this change - shouldn't the column have been filtered by the INNER JOIN?

Also, any idea how you managed to get to a database state where there's a column which references a non-existent type?

[0xced]

but there's no way they can be scaffolded correctly, since their type isn't known

Absolutely and that's the point, you get warnings with the names of the problematic columns/properties instead of a crash with zero information on what's going wrong:

Could not find type mapping for column 'SalesLT.Address.StateProvince' with data type '???'. Skipping column.
Could not find type mapping for column 'SalesLT.Address.CountryRegion' with data type '???'. Skipping column.
Unable to scaffold the index 'IX_Address_AddressLine1_AddressLine2_City_StateProvince_PostalCode_CountryRegion'. The following columns could not be scaffolded: StateProvince,CountryRegion.
Unable to scaffold the index 'IX_Address_StateProvince'. The following columns could not be scaffolded: StateProvince.

It's also unclear to me why you originally got a ArgumentNullException before this change - shouldn't the column have been filtered by the INNER JOIN?

The column is part of on index and it fails while visiting that index (see RelationalScaffoldingModelFactory.VisitIndex in the stack trace).

Note that when running the code in the Debug configuration I got the error earlier on Check.DebugAssert(column != null, "column is null.")

Also, any idea how you managed to get to a database state where there's a column which references a non-existent type?

I have no idea, it's a publicly available SQL Server database used as a demo db for the SLQPro Studio app.

[roji]

The column is part of on index and it fails while visiting that index (see RelationalScaffoldingModelFactory.VisitIndex in the stack trace).

OK, thanks for the info.

Setting a "non-existing" type name such as ??? is probably not the right way to deal with this - we could instead make the index code resilient to this case (i.e. simply skip the index). It may still be valuable to issue warnings for these weird columns.

In any case, can you provide the exact details for the publicly-available database? I've installed the application and can see various sample databases (Northwind, SQL Server Sample DB, Booktown...), though I'm not seeing the full connection details. If this is cloud database, could you simply provide the full connection string you're connecting to (you can email it to me if you want, my address is on my github profile)?

[ErikEJ]

Where can we get a copy of the database with the corrupted schema?

[0xced]

Setting a "non-existing" type name such as ??? is probably not the right way to deal with this - we could instead make the index code resilient to this case (i.e. simply skip the index).

I agree it's probably not right right way but the pragmatic in me says it's already a much better way than the ArgumentNullException. I already have spent an inordinate amount of time on this issue. I spent literally hours figuring out how to run the scaffolder code under a debugger (I had to write my own project and create an instance of DatabaseOperations then call ScaffoldContext because it was impossible to debug the dotnet-ef code which is full of dotnet exec spawning new processes). Then I spent way too much time figuring out why the columns query did not retrieve all the columns so I'd prefer if a member of the EF Core team takes it from there to improve this fix if it's deemed desirable.

In any case, can you provide the exact details for the publicly-available database?

Here's the connection string (it was already in the description of #25729) and can be easily retrieved from a demo copy of SQLPro Studio. 😉

Server=sqlprosample.database.windows.net;Database=sqlprosample;User=sqlproro;Password=nh{Zd?*8ZU@Y}Bb#

[roji]

Thanks @0xced and sorry for missing the connection string. I can see the error happening and will investigate to fix for 6.0.

[0xced]

Awesome, thanks!

[ErikEJ]

@roji Try to run

SELECT * FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_NAME = N'Address'

This returns the data type! And notice the DOMAIN_CATALOG value for the 2 columns...

[ErikEJ]

Ah! User defined data type, that is no longer present :-(

[0xced]

By the way, here's the tool I had to write in order to debug this issue: 0xced@ffbf6fb

I'm left wondering how do the EF Core team members debug the dotnet ef commands with breakpoints?

[roji]

Ah! User defined data type, that is no longer present :-(

But does that mean SQL Server allows dropping a UDT while there are columns of that type still in tables? How does anything work? :)

I'm left wondering how do the EF Core team members debug the dotnet ef commands with breakpoints?

I've got some code snippets for running scaffolding/migration creation programmatically. For scaffolding, I use this:

Programmatic scaffolding

const string outputDir = "/tmp/scaffolded";

if (Directory.Exists(outputDir))
    Directory.Delete(outputDir, recursive: true);

using var ctx = new BlogContext();

var services = new ServiceCollection()
    .AddEntityFrameworkDesignTimeServices()
    .AddDbContextDesignTimeServices(ctx);
new NpgsqlDesignTimeServices().ConfigureDesignTimeServices(services);

var serviceProvider = services.BuildServiceProvider();

var scaffolder = serviceProvider.GetRequiredService<IReverseEngineerScaffolder>();
var scaffoldedModel = scaffolder.ScaffoldModel(
    @"Host=localhost;Username=test;Password=test",
    new DatabaseModelFactoryOptions(tables: null, schemas: null),
    new ModelReverseEngineerOptions { UseDatabaseNames = true, NoPluralize = true },
    new ModelCodeGenerationOptions
    {
        UseDataAnnotations = false,
        RootNamespace = "Scaffolded",
        ModelNamespace = "Scaffolded",
        ContextNamespace = "Scaffolded",
        Language = null,
        // ContextDir = MakeDirRelative(outputDir, outputContextDir),
        // ContextName = dbContextClassName,
        SuppressOnConfiguring = false
    });

scaffolder.Save(
    scaffoldedModel,
    outputDir: outputDir,
    overwriteFiles: true);

[smitpatel]

Superseded by #25748
We should not put a dummy value in. We should skip the column from database model itself.

[ErikEJ]

@roji Under "normal" circumstances, you will get this message when a used UDT is being dropped if in use:

 Cannot drop type 'dbo.Test' because it is being referenced by object 'tester'. There may be other objects that reference this type. (Microsoft SQL Server, Error: 3732)

[ErikEJ]

It is most likely because the user accessing the database is db_datareader only!

https://www.mssqltips.com/sqlservertip/2983/security-for-sql-server-user-defined-data-types/

[roji]

@ErikEJ thanks, that makes sense!