From 667d01601d5e98ead57fc4f83e0def4bb6398ad8 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sat, 28 Aug 2021 22:27:43 +0200 Subject: [PATCH] Resiliency to columns with missing types in SQL Server scaffolding (#25748) * Load columns even if they have no type, warn and skip. * Skip indexes/constraints which reference unknown columns. Fixes #25729 --- .../Internal/SqlServerLoggingDefinitions.cs | 8 + .../Diagnostics/SqlServerEventId.cs | 7 + .../Internal/SqlServerLoggerExtensions.cs | 21 +++ .../Properties/SqlServerStrings.Designer.cs | 25 +++ .../Properties/SqlServerStrings.resx | 4 + .../Internal/SqlServerDatabaseModelFactory.cs | 145 ++++++++++++------ 6 files changed, 160 insertions(+), 50 deletions(-) diff --git a/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs b/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs index 12bc41e89e9..150aee42191 100644 --- a/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs +++ b/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs @@ -37,6 +37,14 @@ public class SqlServerLoggingDefinitions : RelationalLoggingDefinitions /// public EventDefinitionBase? LogByteIdentityColumn; + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public EventDefinitionBase? LogColumnWithoutType; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs b/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs index a15bcaae125..833d790f3df 100644 --- a/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs +++ b/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs @@ -64,6 +64,7 @@ private enum Id ForeignKeyPrincipalColumnMissingWarning, ReflexiveConstraintIgnored, DuplicateForeignKeyConstraintIgnored, + ColumnWithoutTypeWarning } private static readonly string _validationPrefix = DbLoggerCategory.Model.Validation.Name + "."; @@ -237,5 +238,11 @@ private static EventId MakeScaffoldingId(Id id) /// This event is in the category. /// public static readonly EventId DuplicateForeignKeyConstraintIgnored = MakeScaffoldingId(Id.DuplicateForeignKeyConstraintIgnored); + + /// + /// A column was skipped because its database type could not be found. + /// This event is in the category. + /// + public static readonly EventId ColumnWithoutTypeWarning = MakeScaffoldingId(Id.ColumnWithoutTypeWarning); } } diff --git a/src/EFCore.SqlServer/Extensions/Internal/SqlServerLoggerExtensions.cs b/src/EFCore.SqlServer/Extensions/Internal/SqlServerLoggerExtensions.cs index 3793ab6d8a4..aab255bb3ab 100644 --- a/src/EFCore.SqlServer/Extensions/Internal/SqlServerLoggerExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/Internal/SqlServerLoggerExtensions.cs @@ -433,6 +433,27 @@ public static void MissingTableWarning( // No DiagnosticsSource events because these are purely design-time messages } + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public static void ColumnWithoutTypeWarning( + this IDiagnosticsLogger diagnostics, + string tableName, + string columnName) + { + var definition = SqlServerResources.LogColumnWithoutType(diagnostics); + + if (diagnostics.ShouldLog(definition)) + { + definition.Log(diagnostics, tableName, columnName); + } + + // No DiagnosticsSource events because these are purely design-time messages + } + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs index afd28b83c94..4ccab03a1c4 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs @@ -385,6 +385,31 @@ public static EventDefinition LogByteIdentityColumn(IDiagnostics return (EventDefinition)definition; } + /// + /// A database type for column '{columnName}' on table '{tableName}' could not be found, the column will be skipped. + /// + public static EventDefinition LogColumnWithoutType(IDiagnosticsLogger logger) + { + var definition = ((Diagnostics.Internal.SqlServerLoggingDefinitions)logger.Definitions).LogColumnWithoutType; + if (definition == null) + { + definition = NonCapturingLazyInitializer.EnsureInitialized( + ref ((Diagnostics.Internal.SqlServerLoggingDefinitions)logger.Definitions).LogColumnWithoutType, + logger, + static logger => new EventDefinition( + logger.Options, + SqlServerEventId.ColumnWithoutTypeWarning, + LogLevel.Warning, + "SqlServerEventId.ColumnWithoutTypeWarning", + level => LoggerMessage.Define( + level, + SqlServerEventId.ColumnWithoutTypeWarning, + _resourceManager.GetString("LogColumnWithoutType")!))); + } + + return (EventDefinition)definition; + } + /// /// Both the SqlServerValueGenerationStrategy '{generationStrategy}' and '{otherGenerationStrategy}' have been set on property '{propertyName}' on entity type '{entityName}'. Configuring two strategies is usually unintentional and will likely result in a database error. /// diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx index 1ed0c6f040c..e626bbedc50 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx @@ -184,6 +184,10 @@ The property '{property}' on entity type '{entityType}' is of type 'byte', but is set up to use a SQL Server identity column; this requires that values starting at 255 and counting down will be used for temporary key values. A temporary key value is needed for every entity inserted in a single call to 'SaveChanges'. Care must be taken that these values do not collide with real key values. Warning SqlServerEventId.ByteIdentityColumnWarning string string + + A database type for column '{columnName}' on table '{tableName}' could not be found, the column will be skipped. + Warning SqlServerEventId.ColumnWithoutTypeWarning string string + Both the SqlServerValueGenerationStrategy '{generationStrategy}' and '{otherGenerationStrategy}' have been set on property '{propertyName}' on entity type '{entityName}'. Configuring two strategies is usually unintentional and will likely result in a database error. Warning SqlServerEventId.ConflictingValueGenerationStrategiesWarning string string string string diff --git a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs index ae69fa1208a..20b1db2a489 100644 --- a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs +++ b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Data; using System.Data.Common; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Linq; using System.Text; @@ -19,6 +20,7 @@ using Microsoft.EntityFrameworkCore.SqlServer.Internal; using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal; using Microsoft.EntityFrameworkCore.Utilities; +using Microsoft.Extensions.Logging; namespace Microsoft.EntityFrameworkCore.SqlServer.Scaffolding.Internal { @@ -711,7 +713,7 @@ UNION ALL commandText += @" ) o JOIN [sys].[columns] AS [c] ON [o].[object_id] = [c].[object_id] -JOIN [sys].[types] AS [tp] ON [c].[user_type_id] = [tp].[user_type_id] +LEFT JOIN [sys].[types] AS [tp] ON [c].[user_type_id] = [tp].[user_type_id] LEFT JOIN [sys].[extended_properties] AS [e] ON [e].[major_id] = [o].[object_id] AND [e].[minor_id] = [c].[column_id] AND [e].[class] = 1 AND [e].[name] = 'MS_Description' LEFT JOIN [sys].[computed_columns] AS [cc] ON [c].[object_id] = [cc].[object_id] AND [c].[column_id] = [cc].[column_id] LEFT JOIN [sys].[default_constraints] AS [dc] ON [c].[object_id] = [dc].[parent_object_id] AND [c].[column_id] = [dc].[parent_column_id]"; @@ -744,7 +746,7 @@ UNION ALL var columnName = dataRecord.GetFieldValue("column_name"); var ordinal = dataRecord.GetFieldValue("ordinal"); var dataTypeSchemaName = dataRecord.GetValueOrDefault("type_schema"); - var dataTypeName = dataRecord.GetFieldValue("type_name"); + var dataTypeName = dataRecord.GetValueOrDefault("type_name"); var maxLength = dataRecord.GetValueOrDefault("max_length"); var precision = dataRecord.GetValueOrDefault("precision"); var scale = dataRecord.GetValueOrDefault("scale"); @@ -758,6 +760,12 @@ UNION ALL var isSparse = dataRecord.GetValueOrDefault("is_sparse"); var generatedAlwaysType = SupportsTemporalTable() ? dataRecord.GetValueOrDefault("generated_always_type") : 0; + if (dataTypeName is null) + { + _logger.ColumnWithoutTypeWarning(DisplayName(tableSchema, tableName), columnName); + continue; + } + _logger.ColumnFound( DisplayName(tableSchema, tableName), columnName, @@ -991,13 +999,62 @@ FROM [sys].[indexes] i TypeDesc: ddr.GetValueOrDefault("type_desc"))) .ToArray(); + Check.DebugAssert(primaryKeyGroups.Length == 0 || primaryKeyGroups.Length == 1, "Multiple primary keys found"); + if (primaryKeyGroups.Length == 1) { - var primaryKeyGroup = primaryKeyGroups[0]; + if (TryGetPrimaryKey(primaryKeyGroups[0], out var primaryKey)) + { + _logger.PrimaryKeyFound(primaryKey.Name!, DisplayName(tableSchema, tableName)); + table.PrimaryKey = primaryKey; + } + } - _logger.PrimaryKeyFound(primaryKeyGroup.Key.Name, DisplayName(tableSchema, tableName)); + var uniqueConstraintGroups = tableIndexGroup + .Where(ddr => ddr.GetValueOrDefault("is_unique_constraint")) + .GroupBy( + ddr => + (Name: ddr.GetValueOrDefault("index_name"), + TypeDesc: ddr.GetValueOrDefault("type_desc"))) + .ToArray(); - var primaryKey = new DatabasePrimaryKey { Table = table, Name = primaryKeyGroup.Key.Name }; + foreach (var uniqueConstraintGroup in uniqueConstraintGroups) + { + if (TryGetUniqueConstraint(uniqueConstraintGroup, out var uniqueConstraint)) + { + _logger.UniqueConstraintFound(uniqueConstraintGroup.Key.Name!, DisplayName(tableSchema, tableName)); + table.UniqueConstraints.Add(uniqueConstraint); + } + } + + var indexGroups = tableIndexGroup + .Where( + ddr => !ddr.GetValueOrDefault("is_primary_key") + && !ddr.GetValueOrDefault("is_unique_constraint")) + .GroupBy( + ddr => + (Name: ddr.GetValueOrDefault("index_name"), + TypeDesc: ddr.GetValueOrDefault("type_desc"), + IsUnique: ddr.GetValueOrDefault("is_unique"), + HasFilter: ddr.GetValueOrDefault("has_filter"), + FilterDefinition: ddr.GetValueOrDefault("filter_definition"), + FillFactor: ddr.GetValueOrDefault("fill_factor"))) + .ToArray(); + + foreach (var indexGroup in indexGroups) + { + if (TryGetIndex(indexGroup, out var index)) + { + _logger.IndexFound(indexGroup.Key.Name!, DisplayName(tableSchema, tableName), indexGroup.Key.IsUnique); + table.Indexes.Add(index); + } + } + + bool TryGetPrimaryKey( + IGrouping<(string Name, string? TypeDesc), DbDataRecord> primaryKeyGroup, + [NotNullWhen(true)] out DatabasePrimaryKey? primaryKey) + { + primaryKey = new DatabasePrimaryKey { Table = table, Name = primaryKeyGroup.Key.Name }; if (primaryKeyGroup.Key.TypeDesc == "NONCLUSTERED") { @@ -1008,29 +1065,25 @@ FROM [sys].[indexes] i { var columnName = dataRecord.GetValueOrDefault("column_name"); var column = table.Columns.FirstOrDefault(c => c.Name == columnName) - ?? table.Columns.FirstOrDefault( - c => c.Name!.Equals(columnName, StringComparison.OrdinalIgnoreCase)); - Check.DebugAssert(column != null, "column is null."); + ?? table.Columns.FirstOrDefault( + c => c.Name!.Equals(columnName, StringComparison.OrdinalIgnoreCase)); + + if (column is null) + { + return false; + } primaryKey.Columns.Add(column); } - table.PrimaryKey = primaryKey; + return true; } - var uniqueConstraintGroups = tableIndexGroup - .Where(ddr => ddr.GetValueOrDefault("is_unique_constraint")) - .GroupBy( - ddr => - (Name: ddr.GetValueOrDefault("index_name"), - TypeDesc: ddr.GetValueOrDefault("type_desc"))) - .ToArray(); - - foreach (var uniqueConstraintGroup in uniqueConstraintGroups) + bool TryGetUniqueConstraint( + IGrouping<(string? Name, string? TypeDesc), DbDataRecord> uniqueConstraintGroup, + [NotNullWhen(true)] out DatabaseUniqueConstraint? uniqueConstraint) { - _logger.UniqueConstraintFound(uniqueConstraintGroup.Key.Name!, DisplayName(tableSchema, tableName)); - - var uniqueConstraint = new DatabaseUniqueConstraint { Table = table, Name = uniqueConstraintGroup.Key.Name }; + uniqueConstraint = new DatabaseUniqueConstraint { Table = table, Name = uniqueConstraintGroup.Key.Name }; if (uniqueConstraintGroup.Key.TypeDesc == "CLUSTERED") { @@ -1041,35 +1094,26 @@ FROM [sys].[indexes] i { var columnName = dataRecord.GetValueOrDefault("column_name"); var column = table.Columns.FirstOrDefault(c => c.Name == columnName) - ?? table.Columns.FirstOrDefault( - c => c.Name!.Equals(columnName, StringComparison.OrdinalIgnoreCase)); - Check.DebugAssert(column != null, "column is null."); + ?? table.Columns.FirstOrDefault( + c => c.Name!.Equals(columnName, StringComparison.OrdinalIgnoreCase)); + + if (column is null) + { + return false; + } uniqueConstraint.Columns.Add(column); } - table.UniqueConstraints.Add(uniqueConstraint); + return true; } - var indexGroups = tableIndexGroup - .Where( - ddr => !ddr.GetValueOrDefault("is_primary_key") - && !ddr.GetValueOrDefault("is_unique_constraint")) - .GroupBy( - ddr => - (Name: ddr.GetValueOrDefault("index_name"), - TypeDesc: ddr.GetValueOrDefault("type_desc"), - IsUnique: ddr.GetValueOrDefault("is_unique"), - HasFilter: ddr.GetValueOrDefault("has_filter"), - FilterDefinition: ddr.GetValueOrDefault("filter_definition"), - FillFactor: ddr.GetValueOrDefault("fill_factor"))) - .ToArray(); - - foreach (var indexGroup in indexGroups) + bool TryGetIndex( + IGrouping<(string? Name, string? TypeDesc, bool IsUnique, bool HasFilter, string? FilterDefinition, byte FillFactor), + DbDataRecord> indexGroup, + [NotNullWhen(true)] out DatabaseIndex? index) { - _logger.IndexFound(indexGroup.Key.Name!, DisplayName(tableSchema, tableName), indexGroup.Key.IsUnique); - - var index = new DatabaseIndex + index = new DatabaseIndex { Table = table, Name = indexGroup.Key.Name, @@ -1098,17 +1142,18 @@ FROM [sys].[indexes] i } var column = table.Columns.FirstOrDefault(c => c.Name == columnName) - ?? table.Columns.FirstOrDefault( - c => c.Name!.Equals(columnName, StringComparison.OrdinalIgnoreCase)); - Check.DebugAssert(column != null, "column is null."); + ?? table.Columns.FirstOrDefault( + c => c.Name!.Equals(columnName, StringComparison.OrdinalIgnoreCase)); + + if (column is null) + { + return false; + } index.Columns.Add(column); } - if (index.Columns.Count > 0) - { - table.Indexes.Add(index); - } + return index.Columns.Count > 0; } } }