From 169a9578022f74af098c694fd342eabbce2c1351 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sat, 7 Aug 2021 23:52:10 +0200 Subject: [PATCH] Various fixes to computed columns Fixes #1942 Fixes #1946 Fixes #1947 --- .../NpgsqlMigrationsSqlGenerator.cs | 58 ++++++---- .../Migrations/MigrationsNpgsqlTest.cs | 102 ++++++++++++++++++ 2 files changed, 139 insertions(+), 21 deletions(-) diff --git a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs index 4c2f906bb5..790c7146d8 100644 --- a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs +++ b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs @@ -382,7 +382,10 @@ protected override void Generate(AlterColumnOperation operation, IModel? model, var column = model?.GetRelationalModel().FindTable(operation.Table, operation.Schema) ?.Columns.FirstOrDefault(c => c.Name == operation.Name); - if (operation.ComputedColumnSql != null) + ApplyTsVectorColumnSql(operation, model, operation.Schema, operation.Table); + ApplyTsVectorColumnSql(operation.OldColumn, model, operation.OldColumn.Schema, operation.OldColumn.Table); + + if (operation.ComputedColumnSql != operation.OldColumn.ComputedColumnSql) { // 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 @@ -414,7 +417,9 @@ protected override void Generate(AlterColumnOperation operation, IModel? model, DefaultValueSql = operation.DefaultValueSql, ComputedColumnSql = operation.ComputedColumnSql, IsFixedLength = operation.IsFixedLength, - IsStored = operation.IsStored + IsStored = operation.IsStored, + Comment = operation.Comment, + Collation = operation.Collation }; addColumnOperation.AddAnnotations(operation.GetAnnotations()); Generate(addColumnOperation, model, builder); @@ -1384,17 +1389,7 @@ protected override void ColumnDefinition( } } - if (operation[NpgsqlAnnotationNames.TsVectorConfig] is string tsVectorConfig) - { - var tsVectorIncludedColumns = operation[NpgsqlAnnotationNames.TsVectorProperties] as string[]; - if (tsVectorIncludedColumns == null) - throw new InvalidOperationException( - $"{nameof(NpgsqlAnnotationNames.TsVectorConfig)} is present in a migration but " + - $"{nameof(NpgsqlAnnotationNames.TsVectorProperties)} is absent or empty"); - - operation.ComputedColumnSql = ColumnsToTsVector(tsVectorIncludedColumns, tsVectorConfig, model, schema, table); - operation.IsStored = true; - } + ApplyTsVectorColumnSql(operation, model, schema, table); if (operation.ComputedColumnSql != null) { @@ -1431,6 +1426,27 @@ protected override void ColumnDefinition( } } + /// + /// Checks for a annotation on the given column, and if found, assigns + /// the appropriate SQL to . + /// + protected virtual void ApplyTsVectorColumnSql(ColumnOperation column, IModel? model, string? schema, string table) + { + if (column[NpgsqlAnnotationNames.TsVectorConfig] is string tsVectorConfig) + { + var tsVectorIncludedColumns = column[NpgsqlAnnotationNames.TsVectorProperties] as string[]; + if (tsVectorIncludedColumns == null) + throw new InvalidOperationException( + $"{nameof(NpgsqlAnnotationNames.TsVectorConfig)} is present in a migration but " + + $"{nameof(NpgsqlAnnotationNames.TsVectorProperties)} is absent or empty"); + + column.ComputedColumnSql = ColumnsToTsVector(tsVectorIncludedColumns, tsVectorConfig, model, schema, table); + column.IsStored = true; + + column.RemoveAnnotation(NpgsqlAnnotationNames.TsVectorConfig); + } + } + // Note: this definition is only used for creating new identity columns, not for alterations. protected virtual void IdentityDefinition( ColumnOperation operation, @@ -1743,6 +1759,14 @@ private string IndexColumnList(IndexColumn[] columns, string? method) private string ColumnsToTsVector(IEnumerable columns, string tsVectorConfig, IModel? model, string? schema, string table) { + return new StringBuilder() + .Append("to_tsvector(") + .Append(_stringTypeMapping.GenerateSqlLiteral(tsVectorConfig)) + .Append(", ") + .Append(string.Join(" || ' ' || ", columns.Select(GetTsVectorColumnExpression))) + .Append(")") + .ToString(); + string GetTsVectorColumnExpression(string columnName) { var delimitedColumnName = DelimitIdentifier(columnName); @@ -1752,14 +1776,6 @@ string GetTsVectorColumnExpression(string columnName) ? $"coalesce({delimitedColumnName}, '')" : delimitedColumnName; } - - return new StringBuilder() - .Append("to_tsvector(") - .Append(_stringTypeMapping.GenerateSqlLiteral(tsVectorConfig)) - .Append(", ") - .Append(string.Join(" || ' ' || ", columns.Select(GetTsVectorColumnExpression))) - .Append(")") - .ToString(); } private static bool TryParseSchema(string identifier, out string name, out string? schema) diff --git a/test/EFCore.PG.FunctionalTests/Migrations/MigrationsNpgsqlTest.cs b/test/EFCore.PG.FunctionalTests/Migrations/MigrationsNpgsqlTest.cs index fb03dcec58..ec0d60a21e 100644 --- a/test/EFCore.PG.FunctionalTests/Migrations/MigrationsNpgsqlTest.cs +++ b/test/EFCore.PG.FunctionalTests/Migrations/MigrationsNpgsqlTest.cs @@ -1004,6 +1004,51 @@ await Test( public override Task Alter_column_change_computed_type() => Assert.ThrowsAsync(() => base.Alter_column_change_computed()); + [ConditionalTheory] + [InlineData(true)] + [InlineData(false)] + [InlineData(null)] + public virtual async Task Alter_column_make_non_computed(bool? stored) + { + if (TestEnvironment.PostgresVersion.IsUnder(12)) + { + // await Assert.ThrowsAsync(() => base.Alter_column_make_non_computed()); + return; + } + + if (stored != true) + { + // Non-stored generated columns aren't yet supported (PG12) + // await Assert.ThrowsAsync(() => base.Alter_column_make_non_computed(stored)); + return; + } + + await Test( + builder => builder.Entity( + "People", e => + { + e.Property("Id"); + e.Property("X"); + e.Property("Y"); + }), + builder => builder.Entity("People").Property("Sum") + .HasComputedColumnSql($"{DelimitIdentifier("X")} + {DelimitIdentifier("Y")}"), + builder => builder.Entity("People").Property("Sum"), + model => + { + var table = Assert.Single(model.Tables); + var sumColumn = Assert.Single(table.Columns, c => c.Name == "Sum"); + Assert.Null(sumColumn.ComputedColumnSql); + Assert.NotEqual(true, sumColumn.IsStored); + }) + + AssertSql( + @"ALTER TABLE ""People"" DROP COLUMN ""Sum"";", + // + @"ALTER TABLE ""People"" ADD ""Sum"" integer NOT NULL;"); + + } + public override async Task Alter_column_add_comment() { await base.Alter_column_add_comment(); @@ -1473,6 +1518,63 @@ await Test( @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""C"";"); } + [Fact] + public virtual async Task Alter_column_generated_tsvector_change_config() + { + if (TestEnvironment.PostgresVersion.IsUnder(12)) + return; + + await Test( + builder => builder.Entity( + "Blogs", e => + { + e.Property("Title").IsRequired(); + e.Property("Description"); + }), + builder => builder.Entity("Blogs").Property("TsVector") + .IsGeneratedTsVectorColumn("german", "Title", "Description"), + builder => builder.Entity("Blogs").Property("TsVector") + .IsGeneratedTsVectorColumn("english", "Title", "Description"), + model => + { + var table = Assert.Single(model.Tables); + var column = Assert.Single(table.Columns, c => c.Name == "TsVector"); + Assert.Equal("tsvector", column.StoreType); + Assert.Equal(@"to_tsvector('english'::regconfig, ((""Title"" || ' '::text) || COALESCE(""Description"", ''::text)))", column.ComputedColumnSql); + }); + + AssertSql( + @"ALTER TABLE ""Blogs"" DROP COLUMN ""TsVector"";", + // + @"ALTER TABLE ""Blogs"" ADD ""TsVector"" tsvector GENERATED ALWAYS AS (to_tsvector('english', ""Title"" || ' ' || coalesce(""Description"", ''))) STORED;"); + } + + [Fact] + public virtual async Task Alter_column_computed_set_collation() + { + if (TestEnvironment.PostgresVersion.IsUnder(12)) + return; + + await Test( + builder => builder.Entity("People", b => + { + b.Property("Name"); + b.Property("Name2").HasComputedColumnSql(@"""Name""", stored: true); + }), + builder => { }, + builder => builder.Entity("People").Property("Name2") + .UseCollation(NonDefaultCollation), + model => + { + var computedColumn = Assert.Single(Assert.Single(model.Tables).Columns, c => c.Name == "Name2"); + Assert.Equal(@"""Name""", computedColumn.ComputedColumnSql); + Assert.Equal(NonDefaultCollation, computedColumn.Collation); + }); + + AssertSql( + @"ALTER TABLE ""People"" ALTER COLUMN ""Name2"" TYPE text COLLATE ""POSIX"";"); + } + public override async Task Drop_column() { await base.Drop_column();