diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index 69897e0742a..d03408d16fe 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -1367,7 +1367,7 @@ protected virtual void ColumnDefinition( { builder .Append(" COLLATE ") - .Append(operation.Collation); + .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Collation)); } builder.Append(operation.IsNullable ? " NULL" : " NOT NULL"); diff --git a/src/EFCore.Relational/Query/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/QuerySqlGenerator.cs index 34d79d6bf49..a1bbe219f0d 100644 --- a/src/EFCore.Relational/Query/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/QuerySqlGenerator.cs @@ -733,9 +733,13 @@ protected override Expression VisitCollate(CollateExpression collateExpression) { Visit(collateExpression.Operand); + // In some databases (e.g. SQL Server), collations aren't regular identifiers (like column or table names), and so shouldn't be + // quoted. However, we still default to quoting to prevent cases where arbitrary collation name inputs get integrated into the SQL. + // Providers which don't support quoting of collation names should override this method to provide sanitization logic, + // e.g. throwing if the name contains any but a restricted set of characters. _relationalCommandBuilder .Append(" COLLATE ") - .Append(collateExpression.Collation); + .Append(_sqlGenerationHelper.DelimitIdentifier(collateExpression.Collation)); return collateExpression; } diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index c1d3e2b8a46..3a3f7f6b73b 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1646,6 +1646,18 @@ protected override void ColumnDefinition( if (operation.Collation != null) { + // SQL Server collation docs: https://learn.microsoft.com/sql/relational-databases/collations/collation-and-unicode-support + + // The default behavior in MigrationsSqlGenerator is to quote collation names, but SQL Server does not support that. + // Instead, make sure the collation name only contains a restricted set of characters. + foreach (var c in operation.Collation) + { + if (!char.IsLetterOrDigit(c) && c != '_') + { + throw new InvalidOperationException(SqlServerStrings.InvalidCollationName(operation.Collation)); + } + } + builder .Append(" COLLATE ") .Append(operation.Collation); diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs index 9851615076b..21e51b8f76a 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs @@ -215,6 +215,14 @@ public static string IncompatibleTableMemoryOptimizedMismatch(object? table, obj public static string IndexTableRequired => GetString("IndexTableRequired"); + /// + /// Collation name '{collation}' is invalid; collation names may only contain alphanumeric characters and underscores. + /// + public static string InvalidCollationName(object? collation) + => string.Format( + GetString("InvalidCollationName", nameof(collation)), + collation); + /// /// The expression passed to the 'propertyReference' parameter of the 'FreeText' method is not a valid reference to a property. The expression must represent a reference to a full-text indexed property on the object referenced in the from clause: 'from e in context.Entities where EF.Functions.FreeText(e.SomeProperty, textToSearchFor) select e' /// diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx index 6a92672c5c0..d4650a81f97 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx @@ -192,6 +192,9 @@ SQL Server requires the table name to be specified for index operations. Specify table name in calls to 'MigrationBuilder.RenameIndex' and 'DropIndex'. + + Collation name '{collation}' is invalid; collation names may only contain alphanumeric characters and underscores. + The expression passed to the 'propertyReference' parameter of the 'FreeText' method is not a valid reference to a property. The expression must represent a reference to a full-text indexed property on the object referenced in the from clause: 'from e in context.Entities where EF.Functions.FreeText(e.SomeProperty, textToSearchFor) select e' diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs index baf5c10b1df..c7d228f5ff6 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs @@ -55,6 +55,35 @@ protected override bool TryGenerateWithoutWrappingSelect(SelectExpression select => selectExpression.Tables is not [ValuesExpression] && base.TryGenerateWithoutWrappingSelect(selectExpression); + /// + /// 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. + /// + protected override Expression VisitCollate(CollateExpression collateExpression) + { + Visit(collateExpression.Operand); + + // SQL Server collation docs: https://learn.microsoft.com/sql/relational-databases/collations/collation-and-unicode-support + + // The default behavior in QuerySqlGenerator is to quote collation names, but SQL Server does not support that. + // Instead, make sure the collation name only contains a restricted set of characters. + foreach (var c in collateExpression.Collation) + { + if (!char.IsLetterOrDigit(c) && c != '_') + { + throw new InvalidOperationException(SqlServerStrings.InvalidCollationName(collateExpression.Collation)); + } + } + + Sql + .Append(" COLLATE ") + .Append(collateExpression.Collation); + + return collateExpression; + } + /// /// 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.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs b/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs index 8267ff11987..10e6c5bfca7 100644 --- a/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs @@ -377,13 +377,13 @@ private IReadOnlyList RewriteOperations( // Skip autoincrement primary key columns that are being added in this migration var isAutoincrement = column.FindAnnotation(SqliteAnnotationNames.Autoincrement)?.Value as bool? == true; var isPrimaryKey = column.Table.PrimaryKey?.Columns.Contains(column) == true; - + if (isAutoincrement && isPrimaryKey) { // Check if this column is being added in the current migration var isNewColumn = migrationOperations.OfType() .Any(op => op.Table == key.Table && op.Schema == key.Schema && op.Name == column.Name); - + if (isNewColumn) { continue; // Skip newly added autoincrement columns @@ -976,7 +976,7 @@ protected override void ComputedColumnDefinition( { builder .Append(" COLLATE ") - .Append(operation.Collation); + .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Collation)); } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs index 39fc06a897f..51f37ee2e49 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.Data.SqlClient; +using Microsoft.EntityFrameworkCore.SqlServer.Internal; using Microsoft.EntityFrameworkCore.TestModels.Northwind; // ReSharper disable InconsistentNaming @@ -133,6 +134,17 @@ WHERE [c].[Region] IS NULL """); } + [ConditionalTheory, MemberData(nameof(IsAsyncData))] + public virtual async Task Collate_with_invalid_chars_throws(bool async) + { + using var context = CreateContext(); + + var exception = await Assert.ThrowsAsync( + () => context.Customers.CountAsync(c => EF.Functions.Collate(c.ContactName, "Invalid]Collation") == "test")); + + Assert.Equal(SqlServerStrings.InvalidCollationName("Invalid]Collation"), exception.Message); + } + public override Task Least(bool async) => AssertTranslationFailed(() => base.Least(async)); diff --git a/test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs index b4a86845151..e63e3b77cdc 100644 --- a/test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs @@ -1476,7 +1476,7 @@ public virtual void Can_query_OrderBy_of_converted_types() SELECT "b"."Id" FROM "BuiltInDataTypes" AS "b" WHERE "b"."PartitionId" = 207 -ORDER BY "b"."TestDecimal" COLLATE EF_DECIMAL +ORDER BY "b"."TestDecimal" COLLATE "EF_DECIMAL" LIMIT 1 """); @@ -1532,7 +1532,7 @@ public virtual void Can_query_ThenBy_of_converted_types() SELECT "b"."Id" FROM "BuiltInDataTypes" AS "b" WHERE "b"."PartitionId" = 208 -ORDER BY "b"."PartitionId", "b"."TestDecimal" COLLATE EF_DECIMAL +ORDER BY "b"."PartitionId", "b"."TestDecimal" COLLATE "EF_DECIMAL" LIMIT 1 """); @@ -1591,7 +1591,7 @@ public virtual void Can_query_OrderBy_decimal_with_Turkish_culture() """ SELECT "b"."Id", "b"."TestDecimal" FROM "BuiltInDataTypes" AS "b" -ORDER BY "b"."TestDecimal" COLLATE EF_DECIMAL +ORDER BY "b"."TestDecimal" COLLATE "EF_DECIMAL" """, // """ diff --git a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs index e851d998649..486fe39f437 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs @@ -42,7 +42,7 @@ public override async Task Create_table_all_settings() -- Employer ID comment "EmployerId" INTEGER NOT NULL, - "SSN" TEXT COLLATE NOCASE NOT NULL, + "SSN" TEXT COLLATE "NOCASE" NOT NULL, CONSTRAINT "AK_People_SSN" UNIQUE ("SSN"), CONSTRAINT "CK_People_EmployerId" CHECK ("EmployerId" > 0), CONSTRAINT "FK_People_Employers_EmployerId" FOREIGN KEY ("EmployerId") REFERENCES "Employers" ("Id") ON DELETE CASCADE @@ -498,7 +498,7 @@ public override async Task Add_column_with_collation() AssertSql( """ -ALTER TABLE "People" ADD "Name" TEXT COLLATE NOCASE NULL; +ALTER TABLE "People" ADD "Name" TEXT COLLATE "NOCASE" NULL; """); } @@ -508,8 +508,8 @@ public override async Task Add_column_computed_with_collation(bool stored) AssertSql( stored - ? """ALTER TABLE "People" ADD "Name" AS ('hello') STORED COLLATE NOCASE;""" - : """ALTER TABLE "People" ADD "Name" AS ('hello') COLLATE NOCASE;"""); + ? """ALTER TABLE "People" ADD "Name" AS ('hello') STORED COLLATE "NOCASE";""" + : """ALTER TABLE "People" ADD "Name" AS ('hello') COLLATE "NOCASE";"""); } public override async Task Add_column_with_check_constraint() @@ -953,7 +953,7 @@ public override async Task Alter_column_set_collation() AssertSql( """ CREATE TABLE "ef_temp_People" ( - "Name" TEXT COLLATE NOCASE NULL + "Name" TEXT COLLATE "NOCASE" NULL ); """, // @@ -2123,7 +2123,7 @@ await Test( var table = Assert.Single(model.Tables); Assert.Equal("Person", table.Name); Assert.Equal(2, table.Columns.Count()); - + var idColumn = Assert.Single(table.Columns, c => c.Name == "Id"); Assert.False(idColumn.IsNullable); }); @@ -2187,7 +2187,7 @@ await Test( var table = Assert.Single(model.Tables); Assert.Equal("ProductWithStrongId", table.Name); Assert.Equal(2, table.Columns.Count()); - + var idColumn = Assert.Single(table.Columns, c => c.Name == "Id"); Assert.False(idColumn.IsNullable); }); @@ -2220,7 +2220,7 @@ await Test( var table = Assert.Single(model.Tables); Assert.Equal("ProductWithStrongId", table.Name); Assert.Equal(2, table.Columns.Count()); - + var idColumn = Assert.Single(table.Columns, c => c.Name == "Id"); Assert.False(idColumn.IsNullable); }); @@ -2252,7 +2252,7 @@ await Test( var table = Assert.Single(model.Tables); Assert.Equal("CompositeEntity", table.Name); Assert.Equal(2, table.Columns.Count()); - + var id1Column = Assert.Single(table.Columns, c => c.Name == "Id1"); Assert.False(id1Column.IsNullable); var id2Column = Assert.Single(table.Columns, c => c.Name == "Id2"); @@ -2294,7 +2294,7 @@ await Test( var table = Assert.Single(model.Tables); Assert.Equal("Product", table.Name); Assert.Equal(2, table.Columns.Count()); - + var idColumn = Assert.Single(table.Columns, c => c.Name == "Id"); Assert.False(idColumn.IsNullable); });