From ede675824e183725d589c9f4d592c327d75c8cb1 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 19 Apr 2024 19:16:41 +0200 Subject: [PATCH 1/7] Adds new functionality to the migrations. This requires a migration to call Context.SetDone() on the migration context. This happens automatically on scoped migrations before the scope is completed. But migrations inheriting the UnScopedMigrationBase needs to call this manually, inside the scopes or when it is considered done. Thereby, we minimize the risk (and eliminate it for SqlServer) that a migration is executed but the state is not saved. If a migration is executed without the SetDone is called, the migration upgrader throws an error, so we do not start executing the next migration --- .../Migrations/IMigrationContext.cs | 4 ++ .../Migrations/MigrationContext.cs | 18 ++++- .../Migrations/MigrationPlanExecutor.cs | 68 +++++++++++++++---- .../Migrations/Upgrade/Upgrader.cs | 4 -- .../Upgrade/V_14_0_0/AddGuidsToUserGroups.cs | 4 ++ .../Upgrade/V_14_0_0/AddGuidsToUsers.cs | 2 + .../AddListViewKeysToDocumentTypes.cs | 2 + .../Upgrade/V_14_0_0/MigrateTours.cs | 2 + 8 files changed, 87 insertions(+), 17 deletions(-) diff --git a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs index 5e0766755aa9..9034ef29f8f9 100644 --- a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs +++ b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs @@ -44,4 +44,8 @@ public interface IMigrationContext [Obsolete("This will be removed in the V13, and replaced with a RebuildCache flag on the MigrationBase")] void AddPostMigration() where TMigration : MigrationBase; + + public bool IsDone { get; } + + public void SetDone(); } diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs index f75370777083..9b33c22924b6 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs @@ -1,4 +1,6 @@ using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Persistence; namespace Umbraco.Cms.Infrastructure.Migrations; @@ -8,13 +10,15 @@ namespace Umbraco.Cms.Infrastructure.Migrations; /// internal class MigrationContext : IMigrationContext { + private readonly Action _onDoneAction; private readonly List _postMigrations = new(); /// /// Initializes a new instance of the class. /// - public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger logger) + public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger logger, Action onDoneAction) { + _onDoneAction = onDoneAction; Plan = plan; Database = database ?? throw new ArgumentNullException(nameof(database)); Logger = logger ?? throw new ArgumentNullException(nameof(logger)); @@ -41,6 +45,18 @@ public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger< /// public bool BuildingExpression { get; set; } + public bool IsDone { get; private set; } = false; + + public void SetDone() + { + if (IsDone) + { + return; + } + + _onDoneAction(); + IsDone = true; + } /// [Obsolete("This will be removed in the V13, and replaced with a RebuildCache flag on the MigrationBase, and a UmbracoPlanExecutedNotification.")] diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs index a3646ed5c913..29f89966ef1c 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs @@ -1,10 +1,12 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Migrations; using Umbraco.Cms.Core.PublishedCache; using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Persistence; using Umbraco.Cms.Infrastructure.Scoping; using Umbraco.Extensions; @@ -39,6 +41,7 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor private readonly IMigrationBuilder _migrationBuilder; private readonly IUmbracoDatabaseFactory _databaseFactory; private readonly IPublishedSnapshotService _publishedSnapshotService; + private readonly IKeyValueService _keyValueService; private readonly DistributedCache _distributedCache; private readonly IScopeAccessor _scopeAccessor; private readonly ICoreScopeProvider _scopeProvider; @@ -51,7 +54,8 @@ public MigrationPlanExecutor( IMigrationBuilder migrationBuilder, IUmbracoDatabaseFactory databaseFactory, IPublishedSnapshotService publishedSnapshotService, - DistributedCache distributedCache) + DistributedCache distributedCache, + IKeyValueService keyValueService) { _scopeProvider = scopeProvider; _scopeAccessor = scopeAccessor; @@ -59,11 +63,33 @@ public MigrationPlanExecutor( _migrationBuilder = migrationBuilder; _databaseFactory = databaseFactory; _publishedSnapshotService = publishedSnapshotService; + _keyValueService = keyValueService; _distributedCache = distributedCache; _logger = _loggerFactory.CreateLogger(); } - [Obsolete("Use constructor with 7 parameters")] + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 15.")] + public MigrationPlanExecutor( + ICoreScopeProvider scopeProvider, + IScopeAccessor scopeAccessor, + ILoggerFactory loggerFactory, + IMigrationBuilder migrationBuilder, + IUmbracoDatabaseFactory databaseFactory, + IPublishedSnapshotService publishedSnapshotService, + DistributedCache distributedCache) + : this( + scopeProvider, + scopeAccessor, + loggerFactory, + migrationBuilder, + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService()) + { + } + + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 15.")] public MigrationPlanExecutor( ICoreScopeProvider scopeProvider, IScopeAccessor scopeAccessor, @@ -76,7 +102,9 @@ public MigrationPlanExecutor( migrationBuilder, StaticServiceProvider.Instance.GetRequiredService(), StaticServiceProvider.Instance.GetRequiredService(), - StaticServiceProvider.Instance.GetRequiredService()) + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService() + ) { } @@ -92,7 +120,6 @@ public MigrationPlanExecutor( /// Each migration in the plan, may or may not run in a scope depending on the type of plan. /// A plan can complete partially, the changes of each completed migration will be saved. /// - [Obsolete("This will return an ExecutedMigrationPlan in V13")] public ExecutedMigrationPlan ExecutePlan(MigrationPlan plan, string fromState) { plan.Validate(); @@ -104,6 +131,7 @@ public ExecutedMigrationPlan ExecutePlan(MigrationPlan plan, string fromState) // If any completed migration requires us to rebuild cache we'll do that. if (_rebuildCache) { + _logger.LogInformation("Starts rebuilding the cache. This can be a long running operation"); RebuildCache(); } @@ -160,11 +188,11 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt { if (transition.MigrationType.IsAssignableTo(typeof(UnscopedMigrationBase))) { - executedMigrationContexts.Add(RunUnscopedMigration(transition.MigrationType, plan)); + executedMigrationContexts.Add(RunUnscopedMigration(transition, plan)); } else { - executedMigrationContexts.Add(RunScopedMigration(transition.MigrationType, plan)); + executedMigrationContexts.Add(RunScopedMigration(transition, plan)); } } catch (Exception exception) @@ -184,6 +212,13 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt }; } + + IEnumerable nonDoneMigrationsContexts = executedMigrationContexts.Where(x => x.IsDone is false); + if (nonDoneMigrationsContexts.Any()) + { + throw new InvalidOperationException($"Migration ({transition.MigrationType.FullName})has been executed without indicated it was done correctly."); + } + // The plan migration (transition), completed, so we'll add this to our list so we can return this at some point. completedTransitions.Add(transition); nextState = transition.TargetState; @@ -233,17 +268,22 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt }; } - private MigrationContext RunUnscopedMigration(Type migrationType, MigrationPlan plan) + private MigrationContext RunUnscopedMigration(MigrationPlan.Transition transition, MigrationPlan plan) { using IUmbracoDatabase database = _databaseFactory.CreateDatabase(); - var context = new MigrationContext(plan, database, _loggerFactory.CreateLogger()); + var context = new MigrationContext(plan, database, _loggerFactory.CreateLogger(), () => OnMigrationDone(plan, transition.TargetState)); - RunMigration(migrationType, context); + RunMigration(transition.MigrationType, context); return context; } - private MigrationContext RunScopedMigration(Type migrationType, MigrationPlan plan) + private void OnMigrationDone(MigrationPlan plan, string targetState) + { + _keyValueService.SetValue(Constants.Conventions.Migrations.KeyValuePrefix + plan.Name, targetState); + } + + private MigrationContext RunScopedMigration(MigrationPlan.Transition transition, MigrationPlan plan) { // We want to suppress scope (service, etc...) notifications during a migration plan // execution. This is because if a package that doesn't have their migration plan @@ -255,9 +295,13 @@ private MigrationContext RunScopedMigration(Type migrationType, MigrationPlan pl var context = new MigrationContext( plan, _scopeAccessor.AmbientScope?.Database, - _loggerFactory.CreateLogger()); + _loggerFactory.CreateLogger(), + () => OnMigrationDone(plan, transition.TargetState)); + + RunMigration(transition.MigrationType, context); - RunMigration(migrationType, context); + // Ensure we call SetDone before the scope completes + context.SetDone(); scope.Complete(); diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs index 35c04e08e07e..b73967f400b0 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs @@ -70,10 +70,6 @@ public ExecutedMigrationPlan Execute( return result; } - // We always save the final state of the migration plan, this is because a partial success is possible - // So we still want to save the place we got to in the database- - SetState(result.FinalState, scopeProvider, keyValueService); - return result; } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs index bc4b287eae60..e40b4566d0c3 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs @@ -57,6 +57,8 @@ private void MigrateSqlServer() Database.Update(userGroup); } + Context.SetDone(); + scope.Complete(); } @@ -87,6 +89,8 @@ private void MigrateSqlite() // Now that keys are disabled and we have a transaction, we'll do our migration. MigrateColumnSqlite(); + + Context.SetDone(); scope.Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs index 0995acfeeb42..a0f1c4f27731 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs @@ -33,11 +33,13 @@ protected override void Migrate() if (DatabaseType != DatabaseType.SQLite) { MigrateSqlServer(); + Context.SetDone(); scope.Complete(); return; } MigrateSqlite(); + Context.SetDone(); scope.Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs index de54432c737c..92df59c262ff 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs @@ -31,11 +31,13 @@ protected override void Migrate() if (DatabaseType != DatabaseType.SQLite) { MigrateSqlServer(); + Context.SetDone(); scope.Complete(); return; } MigrateSqlite(); + Context.SetDone(); scope.Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs index fedfd43c517f..b84f8fd1c529 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs @@ -79,11 +79,13 @@ protected override void Migrate() if (DatabaseType != DatabaseType.SQLite) { MigrateUserTableSqlServer(); + Context.SetDone(); scope.Complete(); return; } MigrateUserTableSqlite(); + Context.SetDone(); scope.Complete(); } From 6d3a416a24a908a3193c982768d1f191563dcb2c Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 19 Apr 2024 20:08:29 +0200 Subject: [PATCH 2/7] Updated tests --- .../Migrations/AdvancedMigrationTests.cs | 23 ++++++++++++++++++- .../Expressions/CreateTableExpressionTests.cs | 6 ++--- .../Migrations/PartialMigrationsTests.cs | 4 +++- .../SqlServerSyntaxProviderTests.cs | 2 +- .../Migrations/AlterMigrationTests.cs | 2 +- .../Migrations/MigrationPlanTests.cs | 2 +- .../Migrations/MigrationTests.cs | 2 +- 7 files changed, 32 insertions(+), 9 deletions(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/AdvancedMigrationTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/AdvancedMigrationTests.cs index 792f40ac2e0d..084efb2371bf 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/AdvancedMigrationTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/AdvancedMigrationTests.cs @@ -2,16 +2,22 @@ // See LICENSE for more details. using System.Linq; +using Microsoft.Extensions.Logging; using Moq; using NUnit.Framework; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Migrations; +using Umbraco.Cms.Core.PublishedCache; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Migrations; using Umbraco.Cms.Infrastructure.Migrations.Install; using Umbraco.Cms.Infrastructure.Migrations.Upgrade; +using Umbraco.Cms.Infrastructure.Persistence; using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Cms.Infrastructure.Scoping; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; @@ -23,7 +29,22 @@ public class AdvancedMigrationTests : UmbracoIntegrationTest { private IUmbracoVersion UmbracoVersion => GetRequiredService(); private IEventAggregator EventAggregator => GetRequiredService(); - private IMigrationPlanExecutor MigrationPlanExecutor => GetRequiredService(); + private ICoreScopeProvider CoreScopeProvider => GetRequiredService(); + private IScopeAccessor ScopeAccessor => GetRequiredService(); + private ILoggerFactory LoggerFactory => GetRequiredService(); + private IMigrationBuilder MigrationBuilder => GetRequiredService(); + private IUmbracoDatabaseFactory UmbracoDatabaseFactory => GetRequiredService(); + private IPublishedSnapshotService PublishedSnapshotService => GetRequiredService(); + private DistributedCache DistributedCache => GetRequiredService(); + private IMigrationPlanExecutor MigrationPlanExecutor => new MigrationPlanExecutor( + CoreScopeProvider, + ScopeAccessor, + LoggerFactory, + MigrationBuilder, + UmbracoDatabaseFactory, + PublishedSnapshotService, + DistributedCache, + Mock.Of()); [Test] public void CreateTableOfTDto() diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs index ff508c8f32de..b4345da32f63 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs @@ -40,7 +40,7 @@ public void Do_ForDefinitionWithPrimaryKey_PrimaryKeyConstraintIsAdded() .WithColumn("bar").AsInt32().PrimaryKey("PK_foo") .Do(); - // (TableName, ColumnName, ConstraintName) + // (TableName, ColumnName, ConstraintName) var constraint = database.SqlContext.SqlSyntax.GetConstraintsPerColumn(database).Single(); Assert.Multiple(() => @@ -92,7 +92,7 @@ public void Do_ForDefinitionWithForeignKeys_ForeignKeysAreCreated() .ForeignKey("MY_SUPER_COOL_FK", "foo", "bar") .Do(); - // (TableName, ColumnName, ConstraintName) + // (TableName, ColumnName, ConstraintName) var constraint = database.SqlContext.SqlSyntax .GetConstraintsPerColumn(database) .Single(x => x.Item3 == "MY_SUPER_COOL_FK"); @@ -108,7 +108,7 @@ private CreateBuilder GetBuilder(IUmbracoDatabase db) { var logger = GetRequiredService>(); - var ctx = new MigrationContext(new TestPlan(), db, logger); + var ctx = new MigrationContext(new TestPlan(), db, logger, () => { }); return new CreateBuilder(ctx); } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs index cb857befda42..ac81c83159e3 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs @@ -263,6 +263,8 @@ protected override void Migrate() using var scope = _scopeProvider.CreateScope(); Assert.IsNull(((Scope)scope).ParentScope); + + Context.SetDone(); } } @@ -354,4 +356,4 @@ public SimpleMigrationStep( : base(context) => _logger = logger; protected override void Migrate() => _logger.LogDebug("Here be migration"); -} \ No newline at end of file +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs index be90d8695bbf..68324f85b286 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs @@ -40,7 +40,7 @@ private MigrationContext GetMigrationContext(out TestDatabase db) var logger = Mock.Of>(); var sqlSyntax = GetSqlSyntax(); db = new TestDatabase(DatabaseType.SqlServer2005, sqlSyntax); - return new MigrationContext(new TestPlan(), db, logger); + return new MigrationContext(new TestPlan(), db, logger, () => { }); } [UmbracoTest(Database = UmbracoTestOptions.Database.NewEmptyPerTest)] diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs index 516e7f80c146..6714631d47d0 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs @@ -28,7 +28,7 @@ public TestPlan() private MigrationContext GetMigrationContext(out TestDatabase db) { db = new TestDatabase(); - return new MigrationContext(new TestPlan(), db, _logger); + return new MigrationContext(new TestPlan(), db, _logger, () => { }); } [Test] diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs index 8956fe4315ce..fd279b5b3a88 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs @@ -76,7 +76,7 @@ public void CanExecute() loggerFactory, migrationBuilder, databaseFactory, - Mock.Of(), distributedCache); + Mock.Of(), distributedCache, Mock.Of()); var plan = new MigrationPlan("default") .From(string.Empty) diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs index 1e4ddd6f348d..6ce122840332 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs @@ -74,7 +74,7 @@ private MigrationContext GetMigrationContext() => new( new TestPlan(), Mock.Of(), - Mock.Of>()); + Mock.Of>(), () => { }); [Test] public void RunGoodMigration() From 61dc8afe6181afc133ca6abf42bdd8de57a8efd9 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 19 Apr 2024 20:15:41 +0200 Subject: [PATCH 3/7] Renamed after review suggestion --- .../Migrations/IMigrationContext.cs | 4 ++-- src/Umbraco.Infrastructure/Migrations/MigrationContext.cs | 8 ++++---- .../Migrations/MigrationPlanExecutor.cs | 4 ++-- .../Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs | 4 ++-- .../Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs | 4 ++-- .../Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs | 4 ++-- .../Migrations/Upgrade/V_14_0_0/MigrateTours.cs | 4 ++-- 7 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs index 9034ef29f8f9..5519f960a218 100644 --- a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs +++ b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs @@ -45,7 +45,7 @@ public interface IMigrationContext void AddPostMigration() where TMigration : MigrationBase; - public bool IsDone { get; } + public bool IsCompleted { get; } - public void SetDone(); + public void Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs index 9b33c22924b6..0c42eab760fa 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs @@ -45,17 +45,17 @@ public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger< /// public bool BuildingExpression { get; set; } - public bool IsDone { get; private set; } = false; + public bool IsCompleted { get; private set; } = false; - public void SetDone() + public void Complete() { - if (IsDone) + if (IsCompleted) { return; } _onDoneAction(); - IsDone = true; + IsCompleted = true; } /// diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs index 29f89966ef1c..b9b5a820a746 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs @@ -213,7 +213,7 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt } - IEnumerable nonDoneMigrationsContexts = executedMigrationContexts.Where(x => x.IsDone is false); + IEnumerable nonDoneMigrationsContexts = executedMigrationContexts.Where(x => x.IsCompleted is false); if (nonDoneMigrationsContexts.Any()) { throw new InvalidOperationException($"Migration ({transition.MigrationType.FullName})has been executed without indicated it was done correctly."); @@ -301,7 +301,7 @@ private MigrationContext RunScopedMigration(MigrationPlan.Transition transition, RunMigration(transition.MigrationType, context); // Ensure we call SetDone before the scope completes - context.SetDone(); + context.Complete(); scope.Complete(); diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs index e40b4566d0c3..ad8e5091ea67 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs @@ -57,7 +57,7 @@ private void MigrateSqlServer() Database.Update(userGroup); } - Context.SetDone(); + Context.Complete(); scope.Complete(); } @@ -90,7 +90,7 @@ private void MigrateSqlite() // Now that keys are disabled and we have a transaction, we'll do our migration. MigrateColumnSqlite(); - Context.SetDone(); + Context.Complete(); scope.Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs index a0f1c4f27731..e5edce8e870c 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUsers.cs @@ -33,13 +33,13 @@ protected override void Migrate() if (DatabaseType != DatabaseType.SQLite) { MigrateSqlServer(); - Context.SetDone(); + Context.Complete(); scope.Complete(); return; } MigrateSqlite(); - Context.SetDone(); + Context.Complete(); scope.Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs index 92df59c262ff..452160d25bbc 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddListViewKeysToDocumentTypes.cs @@ -31,13 +31,13 @@ protected override void Migrate() if (DatabaseType != DatabaseType.SQLite) { MigrateSqlServer(); - Context.SetDone(); + Context.Complete(); scope.Complete(); return; } MigrateSqlite(); - Context.SetDone(); + Context.Complete(); scope.Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs index b84f8fd1c529..302d49e763a8 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/MigrateTours.cs @@ -79,13 +79,13 @@ protected override void Migrate() if (DatabaseType != DatabaseType.SQLite) { MigrateUserTableSqlServer(); - Context.SetDone(); + Context.Complete(); scope.Complete(); return; } MigrateUserTableSqlite(); - Context.SetDone(); + Context.Complete(); scope.Complete(); } From c010d6108b4f4b7989162a480d2f88f4b1da28fd Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 19 Apr 2024 20:28:36 +0200 Subject: [PATCH 4/7] Rename in test --- .../Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs index ac81c83159e3..1fb0e784176c 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs @@ -264,7 +264,7 @@ protected override void Migrate() using var scope = _scopeProvider.CreateScope(); Assert.IsNull(((Scope)scope).ParentScope); - Context.SetDone(); + Context.Complete(); } } From c846299b9c5d1505285b9d94ae0ffac24945cb64 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 22 Apr 2024 08:03:11 +0200 Subject: [PATCH 5/7] More renaming after review --- .../Migrations/MigrationContext.cs | 9 +++++---- .../Migrations/MigrationPlanExecutor.cs | 14 +++++++------- .../Expressions/CreateTableExpressionTests.cs | 2 +- .../SyntaxProvider/SqlServerSyntaxProviderTests.cs | 2 +- .../Migrations/AlterMigrationTests.cs | 2 +- .../Migrations/MigrationTests.cs | 2 +- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs index 0c42eab760fa..4af60ece4255 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs @@ -10,15 +10,15 @@ namespace Umbraco.Cms.Infrastructure.Migrations; /// internal class MigrationContext : IMigrationContext { - private readonly Action _onDoneAction; + private readonly Action? _onCompleteAction; private readonly List _postMigrations = new(); /// /// Initializes a new instance of the class. /// - public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger logger, Action onDoneAction) + public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger logger, Action? onCompleteAction = null) { - _onDoneAction = onDoneAction; + _onCompleteAction = onCompleteAction; Plan = plan; Database = database ?? throw new ArgumentNullException(nameof(database)); Logger = logger ?? throw new ArgumentNullException(nameof(logger)); @@ -54,7 +54,8 @@ public void Complete() return; } - _onDoneAction(); + _onCompleteAction?.Invoke(); + IsCompleted = true; } diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs index b9b5a820a746..91b1b6c3f205 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs @@ -213,10 +213,10 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt } - IEnumerable nonDoneMigrationsContexts = executedMigrationContexts.Where(x => x.IsCompleted is false); - if (nonDoneMigrationsContexts.Any()) + IEnumerable nonCompletedMigrationsContexts = executedMigrationContexts.Where(x => x.IsCompleted is false); + if (nonCompletedMigrationsContexts.Any()) { - throw new InvalidOperationException($"Migration ({transition.MigrationType.FullName})has been executed without indicated it was done correctly."); + throw new InvalidOperationException($"Migration ({transition.MigrationType.FullName})has been executed without indicated it was completed correctly."); } // The plan migration (transition), completed, so we'll add this to our list so we can return this at some point. @@ -271,14 +271,14 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt private MigrationContext RunUnscopedMigration(MigrationPlan.Transition transition, MigrationPlan plan) { using IUmbracoDatabase database = _databaseFactory.CreateDatabase(); - var context = new MigrationContext(plan, database, _loggerFactory.CreateLogger(), () => OnMigrationDone(plan, transition.TargetState)); + var context = new MigrationContext(plan, database, _loggerFactory.CreateLogger(), () => OnComplete(plan, transition.TargetState)); RunMigration(transition.MigrationType, context); return context; } - private void OnMigrationDone(MigrationPlan plan, string targetState) + private void OnComplete(MigrationPlan plan, string targetState) { _keyValueService.SetValue(Constants.Conventions.Migrations.KeyValuePrefix + plan.Name, targetState); } @@ -296,11 +296,11 @@ private MigrationContext RunScopedMigration(MigrationPlan.Transition transition, plan, _scopeAccessor.AmbientScope?.Database, _loggerFactory.CreateLogger(), - () => OnMigrationDone(plan, transition.TargetState)); + () => OnComplete(plan, transition.TargetState)); RunMigration(transition.MigrationType, context); - // Ensure we call SetDone before the scope completes + // Ensure we mark the context as complete before the scope completes context.Complete(); scope.Complete(); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs index b4345da32f63..9f44122f20b7 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Expressions/Create/Expressions/CreateTableExpressionTests.cs @@ -108,7 +108,7 @@ private CreateBuilder GetBuilder(IUmbracoDatabase db) { var logger = GetRequiredService>(); - var ctx = new MigrationContext(new TestPlan(), db, logger, () => { }); + var ctx = new MigrationContext(new TestPlan(), db, logger); return new CreateBuilder(ctx); } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs index 68324f85b286..be90d8695bbf 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs @@ -40,7 +40,7 @@ private MigrationContext GetMigrationContext(out TestDatabase db) var logger = Mock.Of>(); var sqlSyntax = GetSqlSyntax(); db = new TestDatabase(DatabaseType.SqlServer2005, sqlSyntax); - return new MigrationContext(new TestPlan(), db, logger, () => { }); + return new MigrationContext(new TestPlan(), db, logger); } [UmbracoTest(Database = UmbracoTestOptions.Database.NewEmptyPerTest)] diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs index 6714631d47d0..516e7f80c146 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/AlterMigrationTests.cs @@ -28,7 +28,7 @@ public TestPlan() private MigrationContext GetMigrationContext(out TestDatabase db) { db = new TestDatabase(); - return new MigrationContext(new TestPlan(), db, _logger, () => { }); + return new MigrationContext(new TestPlan(), db, _logger); } [Test] diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs index 6ce122840332..1e4ddd6f348d 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationTests.cs @@ -74,7 +74,7 @@ private MigrationContext GetMigrationContext() => new( new TestPlan(), Mock.Of(), - Mock.Of>(), () => { }); + Mock.Of>()); [Test] public void RunGoodMigration() From ebd383ed92583f4ea7725fca104edd7b445be1a9 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Mon, 22 Apr 2024 10:53:35 +0200 Subject: [PATCH 6/7] Remove public modifier from interface --- src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs index 5519f960a218..cf68617315ba 100644 --- a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs +++ b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs @@ -45,7 +45,7 @@ public interface IMigrationContext void AddPostMigration() where TMigration : MigrationBase; - public bool IsCompleted { get; } + bool IsCompleted { get; } - public void Complete(); + void Complete(); } From f12417fb87aa19984ae20de1b1d1e84dc909ce7f Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Mon, 22 Apr 2024 11:08:36 +0200 Subject: [PATCH 7/7] Add missing space in exception message --- src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs index 91b1b6c3f205..31a9a3fa5f5c 100644 --- a/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs +++ b/src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs @@ -216,7 +216,7 @@ private ExecutedMigrationPlan RunMigrationPlan(MigrationPlan plan, string fromSt IEnumerable nonCompletedMigrationsContexts = executedMigrationContexts.Where(x => x.IsCompleted is false); if (nonCompletedMigrationsContexts.Any()) { - throw new InvalidOperationException($"Migration ({transition.MigrationType.FullName})has been executed without indicated it was completed correctly."); + throw new InvalidOperationException($"Migration ({transition.MigrationType.FullName}) has been executed without indicated it was completed correctly."); } // The plan migration (transition), completed, so we'll add this to our list so we can return this at some point.