From 4c9fea7e7054a98d8f132db48b3deaabeaea11fe Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sat, 28 Mar 2020 14:26:35 +0100 Subject: [PATCH] Changes for new relational model Following https://github.com/dotnet/efcore/pull/20305 --- .../NpgsqlServiceCollectionExtensions.cs | 4 +- .../Internal/NpgsqlAnnotationProvider.cs} | 66 +++++++++++-------- .../NpgsqlMigrationsSqlGenerator.cs | 29 ++++---- .../NpgsqlSqlTranslatingExpressionVisitor.cs | 8 +-- ...lSqlTranslatingExpressionVisitorFactory.cs | 4 +- .../Storage/Internal/NpgsqlDatabaseCreator.cs | 4 +- .../MigrationSqlGeneratorTestBase.cs | 49 ++++++++++++-- 7 files changed, 107 insertions(+), 57 deletions(-) rename src/EFCore.PG/{Migrations/Internal/NpgsqlMigrationsAnnotationProvider.cs => Metadata/Internal/NpgsqlAnnotationProvider.cs} (55%) diff --git a/src/EFCore.PG/Extensions/NpgsqlServiceCollectionExtensions.cs b/src/EFCore.PG/Extensions/NpgsqlServiceCollectionExtensions.cs index cbc5acfa4..9fa74e9ae 100644 --- a/src/EFCore.PG/Extensions/NpgsqlServiceCollectionExtensions.cs +++ b/src/EFCore.PG/Extensions/NpgsqlServiceCollectionExtensions.cs @@ -2,6 +2,7 @@ using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata; using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure; using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.EntityFrameworkCore.Query; @@ -14,6 +15,7 @@ using Npgsql.EntityFrameworkCore.PostgreSQL.Infrastructure.Internal; using Npgsql.EntityFrameworkCore.PostgreSQL.Internal; using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata.Conventions; +using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata.Internal; using Npgsql.EntityFrameworkCore.PostgreSQL.Migrations; using Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.Internal; using Npgsql.EntityFrameworkCore.PostgreSQL.Query.ExpressionTranslators.Internal; @@ -61,7 +63,7 @@ public static IServiceCollection AddEntityFrameworkNpgsql([NotNull] this IServic .TryAdd(p => p.GetService()) .TryAdd() .TryAdd() - .TryAdd() + .TryAdd() .TryAdd() .TryAdd() .TryAdd() diff --git a/src/EFCore.PG/Migrations/Internal/NpgsqlMigrationsAnnotationProvider.cs b/src/EFCore.PG/Metadata/Internal/NpgsqlAnnotationProvider.cs similarity index 55% rename from src/EFCore.PG/Migrations/Internal/NpgsqlMigrationsAnnotationProvider.cs rename to src/EFCore.PG/Metadata/Internal/NpgsqlAnnotationProvider.cs index 32263e8ee..090efbed3 100644 --- a/src/EFCore.PG/Migrations/Internal/NpgsqlMigrationsAnnotationProvider.cs +++ b/src/EFCore.PG/Metadata/Internal/NpgsqlAnnotationProvider.cs @@ -5,19 +5,21 @@ using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Metadata; -using Microsoft.EntityFrameworkCore.Migrations; -using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata; -using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata.Internal; -namespace Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.Internal +namespace Npgsql.EntityFrameworkCore.PostgreSQL.Metadata.Internal { - public class NpgsqlMigrationsAnnotationProvider : MigrationsAnnotationProvider + public class NpgsqlAnnotationProvider : RelationalAnnotationProvider { - public NpgsqlMigrationsAnnotationProvider([NotNull] MigrationsAnnotationProviderDependencies dependencies) - : base(dependencies) {} + public NpgsqlAnnotationProvider([NotNull] RelationalAnnotationProviderDependencies dependencies) + : base(dependencies) + { + } - public override IEnumerable For(IEntityType entityType) + public override IEnumerable For(ITable table) { + // Model validation ensures that these facets are the same on all mapped entity types + var entityType = table.EntityTypeMappings.First().EntityType; + if (entityType.GetIsUnlogged()) yield return new Annotation(NpgsqlAnnotationNames.UnloggedTable, entityType.GetIsUnlogged()); if (entityType[CockroachDbAnnotationNames.InterleaveInParent] != null) @@ -29,11 +31,13 @@ public override IEnumerable For(IEntityType entityType) } } - public override IEnumerable For(IProperty property) + public override IEnumerable For(IColumn column) { - var valueGenerationStrategy = property.GetValueGenerationStrategy(); - if (valueGenerationStrategy != NpgsqlValueGenerationStrategy.None) + var property = column.PropertyMappings.Select(m => m.Property) + .FirstOrDefault(p => p.GetValueGenerationStrategy() != NpgsqlValueGenerationStrategy.None); + if (property != null) { + var valueGenerationStrategy = property.GetValueGenerationStrategy(); yield return new Annotation(NpgsqlAnnotationNames.ValueGenerationStrategy, valueGenerationStrategy); if (valueGenerationStrategy == NpgsqlValueGenerationStrategy.IdentityByDefaultColumn || @@ -46,43 +50,51 @@ public override IEnumerable For(IProperty property) } } - if (property.GetTsVectorConfig() is string tsVectorConfig) + if (column.PropertyMappings.Select(m => m.Property.GetTsVectorConfig()) + .FirstOrDefault(c => c != null) is string tsVectorConfig) + { yield return new Annotation(NpgsqlAnnotationNames.TsVectorConfig, tsVectorConfig); + } - if (property.GetTsVectorProperties() is IReadOnlyList tsVectorProperties) + property = column.PropertyMappings.Select(m => m.Property) + .FirstOrDefault(p => p.GetTsVectorProperties() != null); + if (property != null) { yield return new Annotation( NpgsqlAnnotationNames.TsVectorProperties, - tsVectorProperties - .Select(p => property.DeclaringEntityType.FindProperty(p).GetColumnName()) + property.GetTsVectorProperties() + .Select(p2 => property.DeclaringEntityType.FindProperty(p2).GetColumnName()) .ToArray()); } } - public override IEnumerable For(IIndex index) + public override IEnumerable For(ITableIndex index) { - if (index.GetMethod() is string method) + // Model validation ensures that these facets are the same on all mapped indexes + var modelIndex = index.MappedIndexes.First(); + + if (modelIndex.GetMethod() is string method) yield return new Annotation(NpgsqlAnnotationNames.IndexMethod, method); - if (index.GetOperators() is IReadOnlyList operators) + if (modelIndex.GetOperators() is IReadOnlyList operators) yield return new Annotation(NpgsqlAnnotationNames.IndexOperators, operators); - if (index.GetCollation() is IReadOnlyList collation) + if (modelIndex.GetCollation() is IReadOnlyList collation) yield return new Annotation(NpgsqlAnnotationNames.IndexCollation, collation); - if (index.GetSortOrder() is IReadOnlyList sortOrder) + if (modelIndex.GetSortOrder() is IReadOnlyList sortOrder) yield return new Annotation(NpgsqlAnnotationNames.IndexSortOrder, sortOrder); - if (index.GetNullSortOrder() is IReadOnlyList nullSortOrder) + if (modelIndex.GetNullSortOrder() is IReadOnlyList nullSortOrder) yield return new Annotation(NpgsqlAnnotationNames.IndexNullSortOrder, nullSortOrder); - if (index.GetTsVectorConfig() is string configName) + if (modelIndex.GetTsVectorConfig() is string configName) yield return new Annotation(NpgsqlAnnotationNames.TsVectorConfig, configName); - if (index.GetIncludeProperties() is IReadOnlyList includeProperties) + if (modelIndex.GetIncludeProperties() is IReadOnlyList includeProperties) { yield return new Annotation( NpgsqlAnnotationNames.IndexInclude, includeProperties - .Select(p => index.DeclaringEntityType.FindProperty(p).GetColumnName()) + .Select(p => modelIndex.DeclaringEntityType.FindProperty(p).GetColumnName()) .ToArray()); } - var isCreatedConcurrently = index.IsCreatedConcurrently(); + var isCreatedConcurrently = modelIndex.IsCreatedConcurrently(); if (isCreatedConcurrently.HasValue) { yield return new Annotation( @@ -91,8 +103,8 @@ public override IEnumerable For(IIndex index) } } - public override IEnumerable For(IModel model) - => model.GetAnnotations().Where(a => + public override IEnumerable For(IRelationalModel model) + => model.Model.GetAnnotations().Where(a => a.Name.StartsWith(NpgsqlAnnotationNames.PostgresExtensionPrefix, StringComparison.Ordinal) || a.Name.StartsWith(NpgsqlAnnotationNames.EnumPrefix, StringComparison.Ordinal) || a.Name.StartsWith(NpgsqlAnnotationNames.RangePrefix, StringComparison.Ordinal)); diff --git a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs index 35537696e..cf92c6d72 100644 --- a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs +++ b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs @@ -20,7 +20,6 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Migrations { public class NpgsqlMigrationsSqlGenerator : MigrationsSqlGenerator { - readonly IMigrationsAnnotationProvider _migrationsAnnotations; readonly RelationalTypeMapping _stringTypeMapping; /// @@ -30,12 +29,10 @@ public class NpgsqlMigrationsSqlGenerator : MigrationsSqlGenerator public NpgsqlMigrationsSqlGenerator( [NotNull] MigrationsSqlGeneratorDependencies dependencies, - [NotNull] IMigrationsAnnotationProvider migrationsAnnotations, [NotNull] INpgsqlOptions npgsqlOptions) : base(dependencies) { _postgresVersion = npgsqlOptions.PostgresVersion; - _migrationsAnnotations = migrationsAnnotations; _stringTypeMapping = dependencies.TypeMappingSource.GetMapping(typeof(string)) ?? throw new InvalidOperationException("No string type mapping found"); } @@ -305,12 +302,12 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M if (IsSystemColumn(operation.Name)) return; + var column = model?.GetRelationalModel().FindTable(operation.Table, operation.Schema) + ?.Columns.FirstOrDefault(c => c.Name == operation.Name); var type = operation.ColumnType ?? GetColumnType(operation.Schema, operation.Table, operation.Name, operation, model); if (operation.ComputedColumnSql != null) { - var property = FindProperty(model, operation.Schema, operation.Table, operation.Name); - // TODO: The following will fail if the column being altered is part of an index. // SqlServer recreates indexes, but wait to see if PostgreSQL will introduce a proper ALTER TABLE ALTER COLUMN // that allows us to do this cleanly. @@ -321,8 +318,8 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M Name = operation.Name }; - if (property != null) - dropColumnOperation.AddAnnotations(_migrationsAnnotations.ForRemove(property)); + if (column != null) + dropColumnOperation.AddAnnotations(column.GetAnnotations()); Generate(dropColumnOperation, model, builder); @@ -351,9 +348,8 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M string newSequenceName = null; var defaultValueSql = operation.DefaultValueSql; - var table = Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema); - var column = Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name); - var alterBase = $"ALTER TABLE {table} ALTER COLUMN {column} "; + var alterBase = $"ALTER TABLE {Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema)} " + + $"ALTER COLUMN {Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name)} "; // TYPE builder.Append(alterBase) @@ -396,8 +392,8 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M var oldSequenceWithoutSchema = Dependencies.SqlGenerationHelper.DelimitIdentifier($"{operation.Table}_{operation.Name}_old_seq"); builder .AppendLine($"ALTER SEQUENCE {sequence} RENAME TO {oldSequenceWithoutSchema};") - .AppendLine($"ALTER TABLE {table} ALTER COLUMN {column} DROP DEFAULT;") - .AppendLine($"ALTER TABLE {table} ALTER COLUMN {column} ADD GENERATED {identityTypeClause} AS IDENTITY;") + .AppendLine($"{alterBase}DROP DEFAULT;") + .AppendLine($"{alterBase}ADD GENERATED {identityTypeClause} AS IDENTITY;") .AppendLine($"SELECT * FROM setval('{sequence}', nextval('{oldSequence}'), false);") .AppendLine($"DROP SEQUENCE {oldSequence};"); break; @@ -1477,11 +1473,12 @@ string IndexColumnList(IndexColumn[] columns, string method) string ColumnsToTsVector(IEnumerable columns, string tsVectorConfig, IModel model, string schema, string table) { - string GetTsVectorColumnExpression(string column) + string GetTsVectorColumnExpression(string columnName) { - var delimitedColumnName = Dependencies.SqlGenerationHelper.DelimitIdentifier(column); - var property = FindProperty(model, schema, table, column); - return property?.IsColumnNullable() == true + var delimitedColumnName = Dependencies.SqlGenerationHelper.DelimitIdentifier(columnName); + var column = model?.GetRelationalModel() + .FindTable(table, schema)?.Columns.FirstOrDefault(c => c.Name == columnName); + return column?.IsNullable != false ? $"coalesce({delimitedColumnName}, '')" : delimitedColumnName; } diff --git a/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs b/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs index 5bfda3832..dc224f160 100644 --- a/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs @@ -55,10 +55,10 @@ public class NpgsqlSqlTranslatingExpressionVisitor : RelationalSqlTranslatingExp readonly RelationalTypeMapping _boolMapping; public NpgsqlSqlTranslatingExpressionVisitor( - RelationalSqlTranslatingExpressionVisitorDependencies dependencies, - IModel model, - QueryableMethodTranslatingExpressionVisitor queryableMethodTranslatingExpressionVisitor) - : base(dependencies, model, queryableMethodTranslatingExpressionVisitor) + [NotNull] RelationalSqlTranslatingExpressionVisitorDependencies dependencies, + [NotNull] QueryCompilationContext queryCompilationContext, + [NotNull] QueryableMethodTranslatingExpressionVisitor queryableMethodTranslatingExpressionVisitor) + : base(dependencies, queryCompilationContext, queryableMethodTranslatingExpressionVisitor) { _sqlExpressionFactory = (NpgsqlSqlExpressionFactory)dependencies.SqlExpressionFactory; _jsonPocoTranslator = ((NpgsqlMemberTranslatorProvider)Dependencies.MemberTranslatorProvider).JsonPocoTranslator; diff --git a/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitorFactory.cs b/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitorFactory.cs index 1643634e1..e637e02e0 100644 --- a/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitorFactory.cs +++ b/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitorFactory.cs @@ -13,11 +13,11 @@ public class NpgsqlSqlTranslatingExpressionVisitorFactory : IRelationalSqlTransl => _dependencies = dependencies; public virtual RelationalSqlTranslatingExpressionVisitor Create( - IModel model, + QueryCompilationContext queryCompilationContext, QueryableMethodTranslatingExpressionVisitor queryableMethodTranslatingExpressionVisitor) => new NpgsqlSqlTranslatingExpressionVisitor( _dependencies, - model, + queryCompilationContext, queryableMethodTranslatingExpressionVisitor); } } diff --git a/src/EFCore.PG/Storage/Internal/NpgsqlDatabaseCreator.cs b/src/EFCore.PG/Storage/Internal/NpgsqlDatabaseCreator.cs index 213618ecd..52b842c84 100644 --- a/src/EFCore.PG/Storage/Internal/NpgsqlDatabaseCreator.cs +++ b/src/EFCore.PG/Storage/Internal/NpgsqlDatabaseCreator.cs @@ -217,7 +217,7 @@ await Dependencies.MigrationCommandExecutor public override void CreateTables() { - var operations = Dependencies.ModelDiffer.GetDifferences(null, Dependencies.Model); + var operations = Dependencies.ModelDiffer.GetDifferences(null, Dependencies.Model.GetRelationalModel()); var commands = Dependencies.MigrationsSqlGenerator.Generate(operations, Dependencies.Model); // If a PostgreSQL extension, enum or range was added, we want Npgsql to reload all types at the ADO.NET level. @@ -256,7 +256,7 @@ public override void CreateTables() public override async Task CreateTablesAsync(CancellationToken cancellationToken = default) { - var operations = Dependencies.ModelDiffer.GetDifferences(null, Dependencies.Model); + var operations = Dependencies.ModelDiffer.GetDifferences(null, Dependencies.Model.GetRelationalModel()); var commands = Dependencies.MigrationsSqlGenerator.Generate(operations, Dependencies.Model); // If a PostgreSQL extension, enum or range was added, we want Npgsql to reload all types at the ADO.NET level. diff --git a/test/EFCore.PG.Tests/Migrations/MigrationSqlGeneratorTestBase.cs b/test/EFCore.PG.Tests/Migrations/MigrationSqlGeneratorTestBase.cs index ec0bd9543..94feb0e93 100644 --- a/test/EFCore.PG.Tests/Migrations/MigrationSqlGeneratorTestBase.cs +++ b/test/EFCore.PG.Tests/Migrations/MigrationSqlGeneratorTestBase.cs @@ -6,6 +6,9 @@ using System; using System.Linq; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Metadata.Conventions; +using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure; using Microsoft.EntityFrameworkCore.Metadata.Internal; using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.EntityFrameworkCore.Migrations.Operations; @@ -34,7 +37,7 @@ public virtual void AddColumnOperation_without_column_type() [ConditionalFact] public virtual void AddColumnOperation_with_unicode_overridden() => Generate( - modelBuilder => modelBuilder.Entity("Person").Property("Name").IsUnicode(false), + modelBuilder => modelBuilder.Entity().Property("Name").IsUnicode(false), new AddColumnOperation { Table = "Person", @@ -73,7 +76,7 @@ public virtual void AddColumnOperation_with_fixed_length_no_model() [ConditionalFact] public virtual void AddColumnOperation_with_maxLength_overridden() => Generate( - modelBuilder => modelBuilder.Entity("Person").Property("Name").HasMaxLength(30), + modelBuilder => modelBuilder.Entity().Property("Name").HasMaxLength(30), new AddColumnOperation { Table = "Person", @@ -142,6 +145,30 @@ public virtual void SqlOperation() => Generate( new SqlOperation { Sql = "-- I <3 DDL" }); + [ConditionalTheory] + [InlineData(false)] + [InlineData(true)] + public virtual void DefaultValue_with_line_breaks(bool isUnicode) + { + Generate( + new CreateTableOperation + { + Name = "TestLineBreaks", + Schema = "dbo", + Columns = + { + new AddColumnOperation + { + Name = "TestDefaultValue", + Table = "Test", + ClrType = typeof(string), + DefaultValue = "\r\nVarious Line\rBreaks\n", + IsUnicode = isUnicode + } + } + }); + } + protected TestHelpers TestHelpers { get; } protected MigrationSqlGeneratorTestBase(TestHelpers testHelpers) @@ -158,10 +185,16 @@ protected virtual void Generate(Action buildAction, params Migrati modelBuilder.Model.RemoveAnnotation(CoreAnnotationNames.ProductVersion); buildAction(modelBuilder); - var batch = TestHelpers.CreateContextServices().GetRequiredService() - .Generate(operation, modelBuilder.Model); + var services = TestHelpers.CreateContextServices(); + + IModel model = modelBuilder.Model; + var conventionSet = services.GetRequiredService().CreateConventionSet(); + var relationalModelConvention = conventionSet.ModelFinalizedConventions.OfType().First(); + model = relationalModelConvention.ProcessModelFinalized((IConventionModel)model); + model = ((IMutableModel)model).FinalizeModel(); + + var batch = services.GetRequiredService().Generate(operation, modelBuilder.Model); - // Note that GO here is just a delimiter introduced in the tests to indicate a batch boundary Sql = string.Join( "GO" + EOL + EOL, batch.Select(b => b.CommandText)); @@ -169,5 +202,11 @@ protected virtual void Generate(Action buildAction, params Migrati protected void AssertSql(string expected) => Assert.Equal(expected, Sql, ignoreLineEndingDifferences: true); + + protected class Person + { + public int Id { get; set; } + public string Name { get; set; } + } } }