Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync to EF Core 9.0.0-rc.2.24460.3 #3275

Merged
merged 1 commit into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Directory.Packages.props
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project>
<PropertyGroup>
<EFCoreVersion>[9.0.0-rc.1.24451.1]</EFCoreVersion>
<MicrosoftExtensionsVersion>9.0.0-rc.1.24431.7</MicrosoftExtensionsVersion>
<EFCoreVersion>[9.0.0-rc.2.24460.3]</EFCoreVersion>
<MicrosoftExtensionsVersion>9.0.0-rc.2.24456.9</MicrosoftExtensionsVersion>
<NpgsqlVersion>8.0.4</NpgsqlVersion>
</PropertyGroup>

Expand Down
124 changes: 74 additions & 50 deletions src/EFCore.PG/Migrations/Internal/NpgsqlHistoryRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
/// 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.
/// </summary>
public class NpgsqlHistoryRepository : HistoryRepository
public class NpgsqlHistoryRepository : HistoryRepository, IHistoryRepository
{
/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -19,60 +19,29 @@ public NpgsqlHistoryRepository(HistoryRepositoryDependencies dependencies)
{
}

// TODO: We override Exists() as a workaround for https://github.com/dotnet/efcore/issues/34569; this should be fixed on the EF side
// before EF 9.0 is released

/// <summary>
/// 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.
/// </summary>
public override bool Exists()
=> Dependencies.DatabaseCreator.Exists()
&& InterpretExistsResult(
Dependencies.RawSqlCommandBuilder.Build(ExistsSql).ExecuteScalar(
new RelationalCommandParameterObject(
Dependencies.Connection,
null,
null,
Dependencies.CurrentContext.Context,
Dependencies.CommandLogger, CommandSource.Migrations)));

/// <summary>
/// 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.
/// </summary>
public override async Task<bool> ExistsAsync(CancellationToken cancellationToken = default)
=> await Dependencies.DatabaseCreator.ExistsAsync(cancellationToken).ConfigureAwait(false)
&& InterpretExistsResult(
await Dependencies.RawSqlCommandBuilder.Build(ExistsSql).ExecuteScalarAsync(
new RelationalCommandParameterObject(
Dependencies.Connection,
null,
null,
Dependencies.CurrentContext.Context,
Dependencies.CommandLogger, CommandSource.Migrations),
cancellationToken).ConfigureAwait(false));
public override LockReleaseBehavior LockReleaseBehavior => LockReleaseBehavior.Transaction;

/// <summary>
/// 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.
/// </summary>
public override IDisposable GetDatabaseLock()
public override IMigrationsDatabaseLock AcquireDatabaseLock()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd here's the PG implementation of the final implementation of the migration lock... Everything seems OK (at least the MigrationsInfrastructureTestBase.Can_apply_*_in_parallel are passing).

Note that NpgsqlMigrationDatabaseLock is a no-op, as it generally will always be when LockReleaseBehavior is Transaction (since the lock is implicitly released when the transaction terminates, and not before). So it effectively functions only as a wrapper for MigrationHistory - feels a bit weird, but fine.

{
// TODO: There are issues with the current lock implementation in EF - most importantly, the lock isn't acquired within a
// transaction so we can't use e.g. LOCK TABLE. This should be fixed for rc.1, see #34439.
Dependencies.MigrationsLogger.AcquiringMigrationLock();

// Dependencies.RawSqlCommandBuilder
// .Build($"LOCK TABLE {Dependencies.SqlGenerationHelper.DelimitIdentifier(TableName, TableSchema)} IN ACCESS EXCLUSIVE MODE")
// .ExecuteNonQuery(CreateRelationalCommandParameters());
Dependencies.RawSqlCommandBuilder
.Build($"LOCK TABLE {Dependencies.SqlGenerationHelper.DelimitIdentifier(TableName, TableSchema)} IN ACCESS EXCLUSIVE MODE")
.ExecuteNonQuery(CreateRelationalCommandParameters());

return new DummyDisposable();
return new NpgsqlMigrationDatabaseLock(this);
}

/// <summary>
Expand All @@ -81,19 +50,24 @@ public override IDisposable GetDatabaseLock()
/// 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.
/// </summary>
public override Task<IAsyncDisposable> GetDatabaseLockAsync(CancellationToken cancellationToken = default)
public override async Task<IMigrationsDatabaseLock> AcquireDatabaseLockAsync(CancellationToken cancellationToken = default)
{
// TODO: There are issues with the current lock implementation in EF - most importantly, the lock isn't acquired within a
// transaction so we can't use e.g. LOCK TABLE. This should be fixed for rc.1, see #34439.

// await Dependencies.RawSqlCommandBuilder
// .Build($"LOCK TABLE {Dependencies.SqlGenerationHelper.DelimitIdentifier(TableName, TableSchema)} IN ACCESS EXCLUSIVE MODE")
// .ExecuteNonQueryAsync(CreateRelationalCommandParameters(), cancellationToken)
// .ConfigureAwait(false);
await Dependencies.RawSqlCommandBuilder
.Build($"LOCK TABLE {Dependencies.SqlGenerationHelper.DelimitIdentifier(TableName, TableSchema)} IN ACCESS EXCLUSIVE MODE")
.ExecuteNonQueryAsync(CreateRelationalCommandParameters(), cancellationToken)
.ConfigureAwait(false);

return Task.FromResult<IAsyncDisposable>(new DummyDisposable());
return new NpgsqlMigrationDatabaseLock(this);
}

private RelationalCommandParameterObject CreateRelationalCommandParameters()
=> new(
Dependencies.Connection,
null,
null,
Dependencies.CurrentContext.Context,
Dependencies.CommandLogger, CommandSource.Migrations);

/// <summary>
/// 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
Expand Down Expand Up @@ -127,6 +101,48 @@ SELECT 1 FROM pg_catalog.pg_class c
protected override bool InterpretExistsResult(object? value)
=> (bool?)value == true;

bool IHistoryRepository.CreateIfNotExists()
{
// In PG, doing CREATE TABLE IF NOT EXISTS concurrently can result in a unique constraint violation
// (duplicate key value violates unique constraint "pg_type_typname_nsp_index"). We catch this and report that the table wasn't
// created.
try
{
return Dependencies.MigrationCommandExecutor.ExecuteNonQuery(
GetCreateIfNotExistsCommands(), Dependencies.Connection, new MigrationExecutionState(), commitTransaction: true)
!= 0;
}
catch (PostgresException e) when (e.SqlState == "23505")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd note this annoying thing to deal with the PG problem of doing CREATE TABLE IF NOT EXISTS in parallel :/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only a real issue when running test code that exercises these convoluted scenarios. I'd say that the correct thing here is to let it throw and handle it in the execution strategy used in tests

{
return false;
}
}

async Task<bool> IHistoryRepository.CreateIfNotExistsAsync(CancellationToken cancellationToken)
{
// In PG, doing CREATE TABLE IF NOT EXISTS concurrently can result in a unique constraint violation
// (duplicate key value violates unique constraint "pg_type_typname_nsp_index"). We catch this and report that the table wasn't
// created.
try
{
return (await Dependencies.MigrationCommandExecutor.ExecuteNonQueryAsync(
GetCreateIfNotExistsCommands(), Dependencies.Connection, new MigrationExecutionState(), commitTransaction: true,
cancellationToken: cancellationToken).ConfigureAwait(false))
!= 0;
}
catch (PostgresException e) when (e.SqlState == "23505")
{
return false;
}
}

private IReadOnlyList<MigrationCommand> GetCreateIfNotExistsCommands()
=> Dependencies.MigrationsSqlGenerator.Generate([new SqlOperation
{
Sql = GetCreateIfNotExistsScript(),
SuppressTransaction = true
}]);

/// <summary>
/// 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
Expand All @@ -136,7 +152,7 @@ protected override bool InterpretExistsResult(object? value)
public override string GetCreateIfNotExistsScript()
{
var script = GetCreateScript();
return script.Insert(script.IndexOf("CREATE TABLE", StringComparison.Ordinal) + 12, " IF NOT EXISTS");
return script.Replace("CREATE TABLE", "CREATE TABLE IF NOT EXISTS");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing like some string-based code reuse 😆

}

/// <summary>
Expand Down Expand Up @@ -178,8 +194,16 @@ public override string GetEndIfScript()
END $EF$;
""";

private sealed class DummyDisposable : IDisposable, IAsyncDisposable
private sealed class NpgsqlMigrationDatabaseLock(IHistoryRepository historyRepository) : IMigrationsDatabaseLock
{
/// <summary>
/// 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.
/// </summary>
public IHistoryRepository HistoryRepository => historyRepository;

public void Dispose()
{
}
Expand Down
5 changes: 3 additions & 2 deletions src/EFCore.PG/Migrations/Internal/NpgsqlMigrator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ public NpgsqlMigrator(
IDatabaseProvider databaseProvider,
IMigrationsModelDiffer migrationsModelDiffer,
IDesignTimeModel designTimeModel,
IDbContextOptions contextOptions)
IDbContextOptions contextOptions,
IExecutionStrategy executionStrategy)
: base(migrationsAssembly, historyRepository, databaseCreator, migrationsSqlGenerator, rawSqlCommandBuilder,
migrationCommandExecutor, connection, sqlGenerationHelper, currentContext, modelRuntimeInitializer, logger,
commandLogger, databaseProvider, migrationsModelDiffer, designTimeModel, contextOptions)
commandLogger, databaseProvider, migrationsModelDiffer, designTimeModel, contextOptions, executionStrategy)
{
_historyRepository = historyRepository;
_connection = connection;
Expand Down
7 changes: 2 additions & 5 deletions test/EFCore.PG.FunctionalTests/ComputedColumnTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ public void Can_use_computed_columns_with_nullable_enum()
public async Task InitializeAsync()
=> TestStore = await NpgsqlTestStore.CreateInitializedAsync("ComputedColumnTest");

public Task DisposeAsync()
{
TestStore.Dispose();
return Task.CompletedTask;
}
public async Task DisposeAsync()
=> await TestStore.DisposeAsync();
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,6 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Migrations
public class MigrationsInfrastructureNpgsqlTest(MigrationsInfrastructureNpgsqlTest.MigrationsInfrastructureNpgsqlFixture fixture)
: MigrationsInfrastructureTestBase<MigrationsInfrastructureNpgsqlTest.MigrationsInfrastructureNpgsqlFixture>(fixture)
{
// TODO: The following test the migration lock, which isn't yet implemented - waiting for EF-side fixes in rc.2
#region Unskip for 9.0.0-rc.2

public override void Can_apply_one_migration_in_parallel()
{
}

public override Task Can_apply_one_migration_in_parallel_async()
=> Task.CompletedTask;

public override void Can_apply_second_migration_in_parallel()
{
}

public override Task Can_apply_second_migration_in_parallel_async()
=> Task.CompletedTask;

#endregion Unskip for 9.0.0-rc.2

public override void Can_get_active_provider()
{
base.Can_get_active_provider();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public class NpgsqlValueGenerationScenariosTest
[Fact]
public async Task Insert_with_sequence_id()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextSequence(testStore.Name))
{
Expand All @@ -35,7 +35,7 @@ public class BlogContextSequence(string databaseName) : ContextBase(databaseName
[Fact]
public async Task Insert_with_sequence_HiLo()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextHiLo(testStore.Name))
{
Expand Down Expand Up @@ -68,7 +68,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_default_value_from_sequence()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextDefaultValue(testStore.Name))
{
Expand Down Expand Up @@ -146,7 +146,7 @@ public class BlogWithStringKey
[Fact]
public async Task Insert_with_key_default_value_from_sequence()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextKeyColumnWithDefaultValue(testStore.Name))
{
Expand Down Expand Up @@ -187,7 +187,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[ConditionalFact]
public async Task Insert_uint_to_Identity_column_using_value_converter()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
using (var context = new BlogContextUIntToIdentityUsingValueConverter(testStore.Name))
{
context.Database.EnsureCreatedResiliently();
Expand Down Expand Up @@ -231,7 +231,7 @@ public class BlogWithUIntKey
[ConditionalFact]
public async Task Insert_string_to_Identity_column_using_value_converter()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
using (var context = new BlogContextStringToIdentityUsingValueConverter(testStore.Name))
{
context.Database.EnsureCreatedResiliently();
Expand Down Expand Up @@ -276,7 +276,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_explicit_non_default_keys()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextNoKeyGeneration(testStore.Name))
{
Expand Down Expand Up @@ -312,7 +312,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_explicit_with_default_keys()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextNoKeyGenerationNullableKey(testStore.Name))
{
Expand Down Expand Up @@ -348,7 +348,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_non_key_default_value()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextNonKeyDefaultValue(testStore.Name))
{
Expand Down Expand Up @@ -401,7 +401,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_non_key_default_value_readonly()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

using (var context = new BlogContextNonKeyReadOnlyDefaultValue(testStore.Name))
{
Expand Down Expand Up @@ -455,7 +455,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_serial_non_id()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

int afterSave;

Expand Down Expand Up @@ -493,7 +493,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
[Fact]
public async Task Insert_with_client_generated_GUID_key()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

Guid afterSave;
using (var context = new BlogContext(testStore.Name))
Expand All @@ -518,7 +518,7 @@ public class BlogContext(string databaseName) : ContextBase(databaseName);
[Fact]
public async Task Insert_with_server_generated_GUID_key()
{
using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);
await using var testStore = await NpgsqlTestStore.CreateInitializedAsync(DatabaseName);

Guid afterSave;
using (var context = new BlogContextServerGuidKey(testStore.Name))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ public virtual void Dispose()
{
}

public virtual Task DisposeAsync()
=> _testStore.DisposeAsync();
public virtual async Task DisposeAsync()
=> await _testStore.DisposeAsync();
}

public class CompatibilityTestEntity
Expand Down
Loading
Loading