Sync dependencies and update SQL generation improvements (#2314)

EF Core -> 7.0.0-preview.3.22175.1

Directory.Packages.props

 <Project>
   <PropertyGroup>
-    <EFCoreVersion>7.0.0-preview.3.22159.9</EFCoreVersion>
-    <MicrosoftExtensionsVersion>7.0.0-preview.3.22157.1</MicrosoftExtensionsVersion>
+    <EFCoreVersion>7.0.0-preview.3.22175.1</EFCoreVersion>
+    <MicrosoftExtensionsVersion>7.0.0-preview.3.22175.4</MicrosoftExtensionsVersion>
     <NpgsqlVersion>7.0.0-preview.2</NpgsqlVersion>
   </PropertyGroup>
 

src/EFCore.PG/Update/Internal/NpgsqlModificationCommandBatch.cs

@@ -19,7 +19,6 @@ public class NpgsqlModificationCommandBatch : ReaderModificationCommandBatch
 {
     private const int DefaultBatchSize = 1000;
     private readonly int _maxBatchSize;
-    private int _parameterCount;
 
     /// <summary>
     /// Constructs an instance of the <see cref="NpgsqlModificationCommandBatch"/> class.
@@ -29,7 +28,7 @@ public class NpgsqlModificationCommandBatch : ReaderModificationCommandBatch
         int? maxBatchSize)
         : base(dependencies)
     {
-        if (maxBatchSize.HasValue && maxBatchSize.Value <= 0)
+        if (maxBatchSize is <= 0)
         {
             throw new ArgumentOutOfRangeException(nameof(maxBatchSize), RelationalStrings.InvalidMaxBatchSize(maxBatchSize));
         }
@@ -37,27 +36,9 @@ public class NpgsqlModificationCommandBatch : ReaderModificationCommandBatch
         _maxBatchSize = maxBatchSize ?? DefaultBatchSize;
     }
 
-    protected override int GetParameterCount() => _parameterCount;
-
-    protected override bool CanAddCommand(IReadOnlyModificationCommand modificationCommand)
-    {
-        if (ModificationCommands.Count >= _maxBatchSize)
-        {
-            return false;
-        }
-
-        var newParamCount = (long)_parameterCount + modificationCommand.ColumnModifications.Count;
-        if (newParamCount > int.MaxValue)
-        {
-            return false;
-        }
-
-        _parameterCount = (int)newParamCount;
-        return true;
-    }
-
-    protected override bool IsCommandTextValid()
-        => true;
+    /// <inheritdoc />
+    protected override bool IsValid()
+        => ModificationCommands.Count <= _maxBatchSize && ParameterValues.Count <= ushort.MaxValue;
 
     protected override void Consume(RelationalDataReader reader)
     {
@@ -209,4 +190,4 @@ protected override void Consume(RelationalDataReader reader)
                 ModificationCommands[commandIndex].Entries);
         }
     }
-}
\ No newline at end of file
+}

src/EFCore.PG/Update/Internal/NpgsqlUpdateSqlGenerator.cs

@@ -23,17 +23,29 @@ public NpgsqlUpdateSqlGenerator(UpdateSqlGeneratorDependencies dependencies)
         bool overridingSystemValue,
         out bool requiresTransaction)
     {
-        Check.NotNull(commandStringBuilder, nameof(commandStringBuilder));
-        Check.NotNull(command, nameof(command));
-
         var name = command.TableName;
         var schema = command.Schema;
         var operations = command.ColumnModifications;
 
-        var writeOperations = operations.Where(o => o.IsWrite).ToArray();
-        var readOperations = operations.Where(o => o.IsRead).ToArray();
+        var writeOperations = operations.Where(o => o.IsWrite).ToList();
+        var readOperations = operations.Where(o => o.IsRead).ToList();
+
+        AppendInsertCommand(commandStringBuilder, name, schema, writeOperations, readOperations, overridingSystemValue);
+
+        requiresTransaction = false;
+
+        return readOperations.Count > 0 ? ResultSetMapping.LastInResultSet : ResultSetMapping.NoResultSet;
+    }
 
-        AppendInsertCommandHeader(commandStringBuilder, command.TableName, command.Schema, writeOperations);
+    protected virtual void AppendInsertCommand(
+        StringBuilder commandStringBuilder,
+        string name,
+        string? schema,
+        IReadOnlyList<IColumnModification> writeOperations,
+        IReadOnlyList<IColumnModification> readOperations,
+        bool overridingSystemValue)
+    {
+        AppendInsertCommandHeader(commandStringBuilder, name, schema, writeOperations);
 
         if (overridingSystemValue)
         {
@@ -42,17 +54,8 @@ public NpgsqlUpdateSqlGenerator(UpdateSqlGeneratorDependencies dependencies)
 
         AppendValuesHeader(commandStringBuilder, writeOperations);
         AppendValues(commandStringBuilder, name, schema, writeOperations);
-
-        if (readOperations.Length > 0)
-        {
-            AppendReturningClause(commandStringBuilder, readOperations);
-        }
-
-        commandStringBuilder.Append(SqlGenerationHelper.StatementTerminator).AppendLine();
-
-        requiresTransaction = false;
-
-        return ResultSetMapping.NoResultSet;
+        AppendReturningClause(commandStringBuilder, readOperations);
+        commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator);
     }
 
     public override ResultSetMapping AppendUpdateOperation(
@@ -61,41 +64,40 @@ public NpgsqlUpdateSqlGenerator(UpdateSqlGeneratorDependencies dependencies)
         int commandPosition,
         out bool requiresTransaction)
     {
-        Check.NotNull(commandStringBuilder, nameof(commandStringBuilder));
-        Check.NotNull(command, nameof(command));
-
-        var tableName = command.TableName;
-        var schemaName = command.Schema;
+        // The default implementation adds RETURNING 1 to do concurrency check (was the row actually updated), but in PostgreSQL we check
+        // the per-statement row-affected value exposed by Npgsql in the batch; so no need for RETURNING 1.
+        var name = command.TableName;
+        var schema = command.Schema;
         var operations = command.ColumnModifications;
 
-        var writeOperations = operations.Where(o => o.IsWrite).ToArray();
-        var conditionOperations = operations.Where(o => o.IsCondition).ToArray();
-        var readOperations = operations.Where(o => o.IsRead).ToArray();
-
-        AppendUpdateCommandHeader(commandStringBuilder, tableName, schemaName, writeOperations);
-        AppendWhereClause(commandStringBuilder, conditionOperations);
-
-        if (readOperations.Length > 0)
-        {
-            AppendReturningClause(commandStringBuilder, readOperations);
-        }
-
-        commandStringBuilder.Append(SqlGenerationHelper.StatementTerminator).AppendLine();
+        var writeOperations = operations.Where(o => o.IsWrite).ToList();
+        var conditionOperations = operations.Where(o => o.IsCondition).ToList();
+        var readOperations = operations.Where(o => o.IsRead).ToList();
 
         requiresTransaction = false;
 
-        return ResultSetMapping.NoResultSet;
+        AppendUpdateCommand(commandStringBuilder, name, schema, writeOperations, readOperations, conditionOperations);
+
+        return ResultSetMapping.LastInResultSet;
     }
 
-    // ReSharper disable once ParameterTypeCanBeEnumerable.Local
-    private void AppendReturningClause(
+    public override ResultSetMapping AppendDeleteOperation(
         StringBuilder commandStringBuilder,
-        IReadOnlyList<IColumnModification> operations)
+        IReadOnlyModificationCommand command,
+        int commandPosition,
+        out bool requiresTransaction)
     {
-        commandStringBuilder
-            .AppendLine()
-            .Append("RETURNING ")
-            .AppendJoin(operations.Select(c => SqlGenerationHelper.DelimitIdentifier(c.ColumnName)));
+        // The default implementation adds RETURNING 1 to do concurrency check (was the row actually deleted), but in PostgreSQL we check
+        // the per-statement row-affected value exposed by Npgsql in the batch; so no need for RETURNING 1.
+        var name = command.TableName;
+        var schema = command.Schema;
+        var conditionOperations = command.ColumnModifications.Where(o => o.IsCondition).ToList();
+
+        requiresTransaction = false;
+
+        AppendDeleteCommand(commandStringBuilder, name, schema, Array.Empty<IColumnModification>(), conditionOperations);
+
+        return ResultSetMapping.NoResultSet;
     }
 
     public override void AppendNextSequenceValueOperation(StringBuilder commandStringBuilder, string name, string? schema)
@@ -105,23 +107,9 @@ public override void AppendNextSequenceValueOperation(StringBuilder commandStrin
         commandStringBuilder.Append("')");
     }
 
-    public override void AppendBatchHeader(StringBuilder commandStringBuilder)
-    {
-    }
-
     protected override void AppendIdentityWhereCondition(StringBuilder commandStringBuilder, IColumnModification columnModification)
-    {
-        throw new NotSupportedException();
-    }
+        => throw new NotSupportedException();
 
     protected override void AppendRowsAffectedWhereCondition(StringBuilder commandStringBuilder, int expectedRowsAffected)
-    {
-        throw new NotSupportedException();
-    }
-
-    public enum ResultsGrouping
-    {
-        OneResultSet,
-        OneCommandPerResultSet
-    }
-}
\ No newline at end of file
+        => throw new NotSupportedException();
+}

test/EFCore.PG.FunctionalTests/Query/ComplexNavigationsSharedTypeQueryNpgsqlTest.cs

@@ -41,4 +41,12 @@ public override Task GroupJoin_client_method_in_OrderBy(bool async)
     public override Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
         => Assert.ThrowsAsync<EqualException>(
             async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
+
+    [ConditionalTheory(Skip = "https://github.com/dotnet/efcore/issues/26104")]
+    public override Task GroupBy_aggregate_where_required_relationship(bool async)
+        => base.GroupBy_aggregate_where_required_relationship(async);
+
+    [ConditionalTheory(Skip = "https://github.com/dotnet/efcore/issues/26104")]
+    public override Task GroupBy_aggregate_where_required_relationship_2(bool async)
+        => base.GroupBy_aggregate_where_required_relationship_2(async);
 }

test/EFCore.PG.FunctionalTests/Query/Ef6GroupByNpgsqlTest.cs

@@ -11,17 +11,79 @@ public Ef6GroupByNpgsqlTest(Ef6GroupByNpgsqlFixture fixture, ITestOutputHelper t
         : base(fixture)
     {
         Fixture.TestSqlLoggerFactory.Clear();
-        //Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
+        // Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
     }
 
     [ConditionalTheory(Skip = "https://github.com/dotnet/efcore/issues/27155")]
     public override Task Average_Grouped_from_LINQ_101(bool async)
         => base.Average_Grouped_from_LINQ_101(async);
 
+    public override async Task Whats_new_2021_sample_3(bool async)
+    {
+        await base.Whats_new_2021_sample_3(async);
+
+        AssertSql(
+            @"SELECT (
+    SELECT p1.""LastName""
+    FROM ""Person"" AS p1
+    WHERE p1.""MiddleInitial"" = 'Q' AND p1.""Age"" = 20 AND (p.""LastName"" = p1.""LastName"" OR ((p.""LastName"" IS NULL) AND (p1.""LastName"" IS NULL)))
+    LIMIT 1)
+FROM ""Person"" AS p
+WHERE p.""MiddleInitial"" = 'Q' AND p.""Age"" = 20
+GROUP BY p.""LastName""
+ORDER BY length((
+    SELECT p1.""LastName""
+    FROM ""Person"" AS p1
+    WHERE p1.""MiddleInitial"" = 'Q' AND p1.""Age"" = 20 AND (p.""LastName"" = p1.""LastName"" OR ((p.""LastName"" IS NULL) AND (p1.""LastName"" IS NULL)))
+    LIMIT 1))::INT NULLS FIRST");
+    }
+
+    public override async Task Whats_new_2021_sample_5(bool async)
+    {
+        await base.Whats_new_2021_sample_5(async);
+
+        AssertSql(
+            @"SELECT (
+    SELECT p1.""LastName""
+    FROM ""Person"" AS p1
+    WHERE p.""FirstName"" = p1.""FirstName"" OR ((p.""FirstName"" IS NULL) AND (p1.""FirstName"" IS NULL))
+    LIMIT 1)
+FROM ""Person"" AS p
+GROUP BY p.""FirstName""
+ORDER BY (
+    SELECT p1.""LastName""
+    FROM ""Person"" AS p1
+    WHERE p.""FirstName"" = p1.""FirstName"" OR ((p.""FirstName"" IS NULL) AND (p1.""FirstName"" IS NULL))
+    LIMIT 1) NULLS FIRST");
+    }
+
+    public override async Task Whats_new_2021_sample_6(bool async)
+    {
+        await base.Whats_new_2021_sample_6(async);
+
+        AssertSql(
+            @"SELECT (
+    SELECT p1.""MiddleInitial""
+    FROM ""Person"" AS p1
+    WHERE p1.""Age"" = 20 AND p.""Id"" = p1.""Id""
+    LIMIT 1)
+FROM ""Person"" AS p
+WHERE p.""Age"" = 20
+GROUP BY p.""Id""
+ORDER BY (
+    SELECT p1.""MiddleInitial""
+    FROM ""Person"" AS p1
+    WHERE p1.""Age"" = 20 AND p.""Id"" = p1.""Id""
+    LIMIT 1) NULLS FIRST");
+    }
+
     [ConditionalTheory(Skip = "https://github.com/dotnet/efcore/issues/27155")]
     public override Task Whats_new_2021_sample_10(bool async)
         => base.Whats_new_2021_sample_10(async);
 
+    private void AssertSql(params string[] expected)
+        => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
+
     public class Ef6GroupByNpgsqlFixture : Ef6GroupByFixtureBase
     {
         public TestSqlLoggerFactory TestSqlLoggerFactory

test/EFCore.PG.FunctionalTests/StoreGeneratedNpgsqlTest.cs

@@ -111,7 +111,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
 
             modelBuilder.Entity<NonStoreGenDependent>().Property(e => e.HasTemp).HasDefaultValue(777);
 
+            modelBuilder.Entity<CompositePrincipal>().Property(e => e.Id).UseIdentityColumn();
+
             base.OnModelCreating(modelBuilder, context);
         }
     }
-}
\ No newline at end of file
+}

test/EFCore.PG.FunctionalTests/TestUtilities/TestRelationalCommandBuilderFactory.cs

@@ -38,6 +38,13 @@ public IRelationalCommandBuilder AddParameter(IRelationalParameter parameter)
             return this;
         }
 
+        public IRelationalCommandBuilder RemoveParameterAt(int index)
+        {
+            _parameters.RemoveAt(index);
+
+            return this;
+        }
+
         [Obsolete("Code trying to add parameter should add type mapped parameter using TypeMappingSource directly.")]
         public IRelationalTypeMappingSource TypeMappingSource
             => Dependencies.TypeMappingSource;

test/EFCore.PG.FunctionalTests/Update/StoreValueGenerationNpgsqlTest.cs

@@ -4,7 +4,7 @@
 using Microsoft.EntityFrameworkCore.TestModels.StoreValueGenerationModel;
 using Npgsql.EntityFrameworkCore.PostgreSQL.TestUtilities;
 
-namespace Microsoft.EntityFrameworkCore.Update;
+namespace Npgsql.EntityFrameworkCore.PostgreSQL.Update;
 
 #nullable enable
 
@@ -15,7 +15,7 @@ public StoreValueGenerationNpgsqlTest(StoreValueGenerationNpgsqlFixture fixture,
         : base(fixture)
     {
         fixture.TestSqlLoggerFactory.Clear();
-        fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
+        // fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
     }
 
     protected override bool ShouldCreateImplicitTransaction(

test/EFCore.PG.Tests/Update/NpgsqlModificationCommandBatchFactoryTest.cs

@@ -46,8 +46,8 @@ public void Uses_MaxBatchSize_specified_in_NpgsqlOptionsExtension()
 
         var batch = factory.Create();
 
-        Assert.True(batch.AddCommand(CreateModificationCommand("T1", null, false)));
-        Assert.False(batch.AddCommand(CreateModificationCommand("T1", null, false)));
+        Assert.True(batch.TryAddCommand(CreateModificationCommand("T1", null, false)));
+        Assert.False(batch.TryAddCommand(CreateModificationCommand("T1", null, false)));
     }
 
     [Fact]
@@ -86,8 +86,8 @@ public void MaxBatchSize_is_optional()
 
         var batch = factory.Create();
 
-        Assert.True(batch.AddCommand(CreateModificationCommand("T1", null, false)));
-        Assert.True(batch.AddCommand(CreateModificationCommand("T1", null, false)));
+        Assert.True(batch.TryAddCommand(CreateModificationCommand("T1", null, false)));
+        Assert.True(batch.TryAddCommand(CreateModificationCommand("T1", null, false)));
     }
 
     private class FakeDbContext : DbContext

test/EFCore.PG.Tests/Update/NpgsqlModificationCommandBatchTest.cs

@@ -40,13 +40,13 @@ public void AddCommand_returns_false_when_max_batch_size_is_reached()
                         typeMapper, new CoreSingletonOptions())),
                 new CurrentDbContext(new FakeDbContext()),
                 logger),
-            1);
+            maxBatchSize: 1);
 
         Assert.True(
-            batch.AddCommand(
+            batch.TryAddCommand(
                 CreateModificationCommand("T1", null, false)));
         Assert.False(
-            batch.AddCommand(
+            batch.TryAddCommand(
                 CreateModificationCommand("T1", null, false)));
     }