From 46a154d911e7d9420c50a80b7036a93da2431409 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sun, 28 Aug 2022 11:09:09 +0200 Subject: [PATCH] Fall back to non-RETURNING updates with old Sqlite Fixes #28776 --- .../SqliteServiceCollectionExtensions.cs | 13 +- .../SqliteLegacyUpdateSqlGenerator.cs | 84 ++++ .../StoreValueGenerationLegacySqliteTest.cs | 381 ++++++++++++++++++ .../StoreValueGenerationSqliteFixture.cs | 34 ++ .../Update/StoreValueGenerationSqliteTest.cs | 33 +- 5 files changed, 512 insertions(+), 33 deletions(-) create mode 100644 src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs create mode 100644 test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationLegacySqliteTest.cs create mode 100644 test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteFixture.cs diff --git a/src/EFCore.Sqlite.Core/Extensions/SqliteServiceCollectionExtensions.cs b/src/EFCore.Sqlite.Core/Extensions/SqliteServiceCollectionExtensions.cs index 05cc828579d..1a0dafed2e6 100644 --- a/src/EFCore.Sqlite.Core/Extensions/SqliteServiceCollectionExtensions.cs +++ b/src/EFCore.Sqlite.Core/Extensions/SqliteServiceCollectionExtensions.cs @@ -98,15 +98,12 @@ public static IServiceCollection AddEntityFrameworkSqlite(this IServiceCollectio .TryAdd() .TryAdd() .TryAdd() - .TryAdd() .TryAdd() .TryAdd(p => p.GetRequiredService()) .TryAdd() .TryAdd() .TryAdd() .TryAdd() - - // New Query Pipeline .TryAdd() .TryAdd() .TryAdd() @@ -114,6 +111,16 @@ public static IServiceCollection AddEntityFrameworkSqlite(this IServiceCollectio .TryAdd() .TryAdd() .TryAdd() + .TryAdd(sp => + { + // Support for the RETURNING clause on INSERT/UPDATE/DELETE was added in Sqlite 3.35. + // Detect which version we're using, and fall back to the older INSERT/UPDATE+SELECT behavior on legacy versions. + var dependencies = sp.GetRequiredService(); + + return SQLitePCL.raw.sqlite3_libversion_number() < 3035000 + ? new SqliteLegacyUpdateSqlGenerator(dependencies) + : new SqliteUpdateSqlGenerator(dependencies); + }) .TryAddProviderSpecificServices( b => b.TryAddScoped()); diff --git a/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs b/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs new file mode 100644 index 00000000000..eecf2cfb4dc --- /dev/null +++ b/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs @@ -0,0 +1,84 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Text; +using Microsoft.EntityFrameworkCore.Sqlite.Internal; + +namespace Microsoft.EntityFrameworkCore.Sqlite.Update.Internal; + +/// +/// 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 class SqliteLegacyUpdateSqlGenerator : UpdateAndSelectSqlGenerator +{ + /// + /// 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 SqliteLegacyUpdateSqlGenerator(UpdateSqlGeneratorDependencies dependencies) + : base(dependencies) + { + } + + /// + /// Appends a WHERE condition for the identity (i.e. key value) of the given column. + /// + /// The builder to which the SQL should be appended. + /// The column for which the condition is being generated. + protected override void AppendIdentityWhereCondition(StringBuilder commandStringBuilder, IColumnModification columnModification) + { + Check.NotNull(commandStringBuilder, nameof(commandStringBuilder)); + Check.NotNull(columnModification, nameof(columnModification)); + + SqlGenerationHelper.DelimitIdentifier(commandStringBuilder, "rowid"); + commandStringBuilder.Append(" = ") + .Append("last_insert_rowid()"); + } + + /// + /// Appends a SQL command for selecting the number of rows affected. + /// + /// The builder to which the SQL should be appended. + /// The name of the table. + /// The table schema, or to use the default schema. + /// The ordinal of the command for which rows affected it being returned. + /// The for this command. + protected override ResultSetMapping AppendSelectAffectedCountCommand(StringBuilder commandStringBuilder, string name, string? schema, int commandPosition) + { + Check.NotNull(commandStringBuilder, nameof(commandStringBuilder)); + Check.NotEmpty(name, nameof(name)); + + commandStringBuilder + .Append("SELECT changes()") + .AppendLine(SqlGenerationHelper.StatementTerminator) + .AppendLine(); + + return ResultSetMapping.LastInResultSet; + } + + /// + /// Appends a WHERE condition checking rows affected. + /// + /// The builder to which the SQL should be appended. + /// The expected number of rows affected. + protected override void AppendRowsAffectedWhereCondition(StringBuilder commandStringBuilder, int expectedRowsAffected) + { + Check.NotNull(commandStringBuilder, nameof(commandStringBuilder)); + + commandStringBuilder.Append("changes() = ").Append(expectedRowsAffected); + } + + /// + /// 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 override string GenerateNextSequenceValueOperation(string name, string? schema) + => throw new NotSupportedException(SqliteStrings.SequencesNotSupported); +} diff --git a/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationLegacySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationLegacySqliteTest.cs new file mode 100644 index 00000000000..4901eef619f --- /dev/null +++ b/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationLegacySqliteTest.cs @@ -0,0 +1,381 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore.Update; + +#nullable enable + +[SqliteVersionCondition(Max = "3.34.999")] +public class StoreValueGenerationLegacySqliteTest : StoreValueGenerationTestBase +{ + public StoreValueGenerationLegacySqliteTest(StoreValueGenerationSqliteFixture fixture, ITestOutputHelper testOutputHelper) + : base(fixture) + { + fixture.TestSqlLoggerFactory.Clear(); + // fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); + } + + // We don't currently batch in Sqlite (the perf impact is likely to be minimal, no networking) + protected override int ShouldExecuteInNumberOfCommands( + EntityState firstOperationType, + EntityState? secondOperationType, + GeneratedValues generatedValues, + bool withDatabaseGenerated) + => secondOperationType is null ? 1 : 2; + + // Old Sqlite versions don't support the RETURNING clause, so we use the INSERT/UPDATE+SELECT behavior for fetching back database- + // generated values and rows affected. Multiple statements should be wrapped in a transaction. + protected override bool ShouldCreateImplicitTransaction( + EntityState firstOperationType, + EntityState? secondOperationType, + GeneratedValues generatedValues, + bool withSameEntityType) + => secondOperationType is not null + || (generatedValues is GeneratedValues.Some or GeneratedValues.All + && firstOperationType is EntityState.Added or EntityState.Modified); + + #region Single operation + + public override async Task Add_with_generated_values(bool async) + { + await base.Add_with_generated_values(async); + + AssertSql( + @"@p0='1000' + +INSERT INTO ""WithSomeDatabaseGenerated"" (""Data2"") +VALUES (@p0); +SELECT ""Id"", ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();"); + } + + public override async Task Add_with_no_generated_values(bool async) + { + await base.Add_with_no_generated_values(async); + + AssertSql( + @"@p0='100' +@p1='1000' +@p2='1000' + +INSERT INTO ""WithNoDatabaseGenerated"" (""Id"", ""Data1"", ""Data2"") +VALUES (@p0, @p1, @p2); +SELECT changes();"); + } + + public override async Task Add_with_all_generated_values(bool async) + { + await base.Add_with_all_generated_values(async); + + AssertSql( + @"INSERT INTO ""WithAllDatabaseGenerated"" +DEFAULT VALUES; +SELECT ""Id"", ""Data1"", ""Data2"" +FROM ""WithAllDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();"); + } + + public override async Task Modify_with_generated_values(bool async) + { + await base.Modify_with_generated_values(async); + + AssertSql( + @"@p1='1' +@p0='1000' + +UPDATE ""WithSomeDatabaseGenerated"" SET ""Data2"" = @p0 +WHERE ""Id"" = @p1; +SELECT ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""Id"" = @p1;"); + } + + public override async Task Modify_with_no_generated_values(bool async) + { + await base.Modify_with_no_generated_values(async); + + AssertSql( + @"@p2='1' +@p0='1000' +@p1='1000' + +UPDATE ""WithNoDatabaseGenerated"" SET ""Data1"" = @p0, ""Data2"" = @p1 +WHERE ""Id"" = @p2; +SELECT changes();"); + } + + public override async Task Delete(bool async) + { + await base.Delete(async); + + AssertSql( + @"@p0='1' + +DELETE FROM ""WithSomeDatabaseGenerated"" +WHERE ""Id"" = @p0; +SELECT changes();"); + } + + #endregion Single operation + + #region Same two operations with same entity type + + public override async Task Add_Add_with_same_entity_type_and_generated_values(bool async) + { + await base.Add_Add_with_same_entity_type_and_generated_values(async); + + AssertSql( + @"@p0='1000' + +INSERT INTO ""WithSomeDatabaseGenerated"" (""Data2"") +VALUES (@p0); +SELECT ""Id"", ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();", + // + @"@p0='1001' + +INSERT INTO ""WithSomeDatabaseGenerated"" (""Data2"") +VALUES (@p0); +SELECT ""Id"", ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();"); + } + + public override async Task Add_Add_with_same_entity_type_and_no_generated_values(bool async) + { + await base.Add_Add_with_same_entity_type_and_no_generated_values(async); + + AssertSql( + @"@p0='100' +@p1='1000' +@p2='1000' + +INSERT INTO ""WithNoDatabaseGenerated"" (""Id"", ""Data1"", ""Data2"") +VALUES (@p0, @p1, @p2); +SELECT changes();", + // + @"@p0='101' +@p1='1001' +@p2='1001' + +INSERT INTO ""WithNoDatabaseGenerated"" (""Id"", ""Data1"", ""Data2"") +VALUES (@p0, @p1, @p2); +SELECT changes();"); + } + + public override async Task Add_Add_with_same_entity_type_and_all_generated_values(bool async) + { + await base.Add_Add_with_same_entity_type_and_all_generated_values(async); + + AssertSql( + @"INSERT INTO ""WithAllDatabaseGenerated"" +DEFAULT VALUES; +SELECT ""Id"", ""Data1"", ""Data2"" +FROM ""WithAllDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();", + // + @"INSERT INTO ""WithAllDatabaseGenerated"" +DEFAULT VALUES; +SELECT ""Id"", ""Data1"", ""Data2"" +FROM ""WithAllDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();"); + } + + public override async Task Modify_Modify_with_same_entity_type_and_generated_values(bool async) + { + await base.Modify_Modify_with_same_entity_type_and_generated_values(async); + + AssertSql( + @"@p1='1' +@p0='1000' + +UPDATE ""WithSomeDatabaseGenerated"" SET ""Data2"" = @p0 +WHERE ""Id"" = @p1; +SELECT ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""Id"" = @p1;", + // + @"@p1='2' +@p0='1001' + +UPDATE ""WithSomeDatabaseGenerated"" SET ""Data2"" = @p0 +WHERE ""Id"" = @p1; +SELECT ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""Id"" = @p1;"); + } + + public override async Task Modify_Modify_with_same_entity_type_and_no_generated_values(bool async) + { + await base.Modify_Modify_with_same_entity_type_and_no_generated_values(async); + + AssertSql( + @"@p2='1' +@p0='1000' +@p1='1000' + +UPDATE ""WithNoDatabaseGenerated"" SET ""Data1"" = @p0, ""Data2"" = @p1 +WHERE ""Id"" = @p2; +SELECT changes();", + // + @"@p2='2' +@p0='1001' +@p1='1001' + +UPDATE ""WithNoDatabaseGenerated"" SET ""Data1"" = @p0, ""Data2"" = @p1 +WHERE ""Id"" = @p2; +SELECT changes();"); + } + + public override async Task Delete_Delete_with_same_entity_type(bool async) + { + await base.Delete_Delete_with_same_entity_type(async); + + AssertSql( + @"@p0='1' + +DELETE FROM ""WithSomeDatabaseGenerated"" +WHERE ""Id"" = @p0; +SELECT changes();", + // + @"@p0='2' + +DELETE FROM ""WithSomeDatabaseGenerated"" +WHERE ""Id"" = @p0; +SELECT changes();"); + } + + #endregion Same two operations with same entity type + + #region Same two operations with different entity types + + public override async Task Add_Add_with_different_entity_types_and_generated_values(bool async) + { + await base.Add_Add_with_different_entity_types_and_generated_values(async); + + AssertSql( + @"@p0='1000' + +INSERT INTO ""WithSomeDatabaseGenerated"" (""Data2"") +VALUES (@p0); +SELECT ""Id"", ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();", + // + @"@p0='1001' + +INSERT INTO ""WithSomeDatabaseGenerated2"" (""Data2"") +VALUES (@p0); +SELECT ""Id"", ""Data1"" +FROM ""WithSomeDatabaseGenerated2"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();"); + } + + public override async Task Add_Add_with_different_entity_types_and_no_generated_values(bool async) + { + await base.Add_Add_with_different_entity_types_and_no_generated_values(async); + + AssertSql( + @"@p0='100' +@p1='1000' +@p2='1000' + +INSERT INTO ""WithNoDatabaseGenerated"" (""Id"", ""Data1"", ""Data2"") +VALUES (@p0, @p1, @p2); +SELECT changes();", + // + @"@p0='101' +@p1='1001' +@p2='1001' + +INSERT INTO ""WithNoDatabaseGenerated2"" (""Id"", ""Data1"", ""Data2"") +VALUES (@p0, @p1, @p2); +SELECT changes();"); + } + + public override async Task Add_Add_with_different_entity_types_and_all_generated_values(bool async) + { + await base.Add_Add_with_different_entity_types_and_all_generated_values(async); + + AssertSql( + @"INSERT INTO ""WithAllDatabaseGenerated"" +DEFAULT VALUES; +SELECT ""Id"", ""Data1"", ""Data2"" +FROM ""WithAllDatabaseGenerated"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();", + // + @"INSERT INTO ""WithAllDatabaseGenerated2"" +DEFAULT VALUES; +SELECT ""Id"", ""Data1"", ""Data2"" +FROM ""WithAllDatabaseGenerated2"" +WHERE changes() = 1 AND ""rowid"" = last_insert_rowid();"); + } + + public override async Task Modify_Modify_with_different_entity_types_and_generated_values(bool async) + { + await base.Modify_Modify_with_different_entity_types_and_generated_values(async); + + AssertSql( + @"@p1='1' +@p0='1000' + +UPDATE ""WithSomeDatabaseGenerated"" SET ""Data2"" = @p0 +WHERE ""Id"" = @p1; +SELECT ""Data1"" +FROM ""WithSomeDatabaseGenerated"" +WHERE changes() = 1 AND ""Id"" = @p1;", + // + @"@p1='2' +@p0='1001' + +UPDATE ""WithSomeDatabaseGenerated2"" SET ""Data2"" = @p0 +WHERE ""Id"" = @p1; +SELECT ""Data1"" +FROM ""WithSomeDatabaseGenerated2"" +WHERE changes() = 1 AND ""Id"" = @p1;"); + } + + public override async Task Modify_Modify_with_different_entity_types_and_no_generated_values(bool async) + { + await base.Modify_Modify_with_different_entity_types_and_no_generated_values(async); + + AssertSql( + @"@p2='1' +@p0='1000' +@p1='1000' + +UPDATE ""WithNoDatabaseGenerated"" SET ""Data1"" = @p0, ""Data2"" = @p1 +WHERE ""Id"" = @p2; +SELECT changes();", + // + @"@p2='2' +@p0='1001' +@p1='1001' + +UPDATE ""WithNoDatabaseGenerated2"" SET ""Data1"" = @p0, ""Data2"" = @p1 +WHERE ""Id"" = @p2; +SELECT changes();"); + } + + public override async Task Delete_Delete_with_different_entity_types(bool async) + { + await base.Delete_Delete_with_different_entity_types(async); + + AssertSql( + @"@p0='1' + +DELETE FROM ""WithSomeDatabaseGenerated"" +WHERE ""Id"" = @p0; +SELECT changes();", + // + @"@p0='2' + +DELETE FROM ""WithSomeDatabaseGenerated2"" +WHERE ""Id"" = @p0; +SELECT changes();"); + } + + #endregion Same two operations with different entity types +} diff --git a/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteFixture.cs b/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteFixture.cs new file mode 100644 index 00000000000..6b58edda5a3 --- /dev/null +++ b/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteFixture.cs @@ -0,0 +1,34 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore.Update; + +#nullable enable + +public class StoreValueGenerationSqliteFixture : StoreValueGenerationFixtureBase +{ + private string? _cleanDataSql; + + protected override ITestStoreFactory TestStoreFactory + => SqliteTestStoreFactory.Instance; + + public override void CleanData() + { + using var context = CreateContext(); + context.Database.ExecuteSqlRaw(_cleanDataSql ??= GenerateCleanDataSql()); + } + + private string GenerateCleanDataSql() + { + var context = CreateContext(); + var builder = new StringBuilder(); + + foreach (var table in context.Model.GetEntityTypes().SelectMany(e => e.GetTableMappings().Select(m => m.Table.Name))) + { + builder.AppendLine($"DELETE FROM {table};"); + builder.AppendLine($"DELETE FROM sqlite_sequence WHERE name='{table}';"); + } + + return builder.ToString(); + } +} diff --git a/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteTest.cs index 1796adcc8a0..0b48b99d8ae 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Update/StoreValueGenerationSqliteTest.cs @@ -5,8 +5,8 @@ namespace Microsoft.EntityFrameworkCore.Update; #nullable enable -public class StoreValueGenerationSqliteTest : StoreValueGenerationTestBase< - StoreValueGenerationSqliteTest.StoreValueGenerationSqliteFixture> +[SqliteVersionCondition(Min = "3.35.0")] +public class StoreValueGenerationSqliteTest : StoreValueGenerationTestBase { public StoreValueGenerationSqliteTest(StoreValueGenerationSqliteFixture fixture, ITestOutputHelper testOutputHelper) : base(fixture) @@ -15,6 +15,7 @@ public StoreValueGenerationSqliteTest(StoreValueGenerationSqliteFixture fixture, // fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } + // We don't currently batch in Sqlite (the perf impact is likely to be minimal, no networking) protected override int ShouldExecuteInNumberOfCommands( EntityState firstOperationType, EntityState? secondOperationType, @@ -331,32 +332,4 @@ DELETE FROM ""WithSomeDatabaseGenerated2"" } #endregion Same two operations with different entity types - - public class StoreValueGenerationSqliteFixture : StoreValueGenerationFixtureBase - { - private string? _cleanDataSql; - - protected override ITestStoreFactory TestStoreFactory - => SqliteTestStoreFactory.Instance; - - public override void CleanData() - { - using var context = CreateContext(); - context.Database.ExecuteSqlRaw(_cleanDataSql ??= GenerateCleanDataSql()); - } - - private string GenerateCleanDataSql() - { - var context = CreateContext(); - var builder = new StringBuilder(); - - foreach (var table in context.Model.GetEntityTypes().SelectMany(e => e.GetTableMappings().Select(m => m.Table.Name))) - { - builder.AppendLine($"DELETE FROM {table};"); - builder.AppendLine($"DELETE FROM sqlite_sequence WHERE name='{table}';"); - } - - return builder.ToString(); - } - } }