RC2 Breaking - Ensure migrations persist the executed key, when executed.

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<TMigration>()
         where TMigration : MigrationBase;
+
+    bool IsCompleted { get; }
+
+    void Complete();
 }

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;
 /// </summary>
 internal class MigrationContext : IMigrationContext
 {
+    private readonly Action? _onCompleteAction;
     private readonly List<Type> _postMigrations = new();
 
     /// <summary>
     ///     Initializes a new instance of the <see cref="MigrationContext" /> class.
     /// </summary>
-    public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger<MigrationContext> logger)
+    public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger<MigrationContext> logger, Action? onCompleteAction = null)
     {
+        _onCompleteAction = onCompleteAction;
         Plan = plan;
         Database = database ?? throw new ArgumentNullException(nameof(database));
         Logger = logger ?? throw new ArgumentNullException(nameof(logger));
@@ -41,6 +45,19 @@ public MigrationContext(MigrationPlan plan, IUmbracoDatabase? database, ILogger<
     /// <inheritdoc />
     public bool BuildingExpression { get; set; }
 
+    public bool IsCompleted { get; private set; } = false;
+
+    public void Complete()
+    {
+        if (IsCompleted)
+        {
+            return;
+        }
+
+        _onCompleteAction?.Invoke();
+
+        IsCompleted = true;
+    }
 
     /// <inheritdoc />
     [Obsolete("This will be removed in the V13, and replaced with a RebuildCache flag on the MigrationBase, and a UmbracoPlanExecutedNotification.")]

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 @@
     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,19 +54,42 @@
         IMigrationBuilder migrationBuilder,
         IUmbracoDatabaseFactory databaseFactory,
         IPublishedSnapshotService publishedSnapshotService,
-        DistributedCache distributedCache)
+        DistributedCache distributedCache,
+        IKeyValueService keyValueService)
     {
         _scopeProvider = scopeProvider;
         _scopeAccessor = scopeAccessor;
         _loggerFactory = loggerFactory;
         _migrationBuilder = migrationBuilder;
         _databaseFactory = databaseFactory;
         _publishedSnapshotService = publishedSnapshotService;
+        _keyValueService = keyValueService;
         _distributedCache = distributedCache;
         _logger = _loggerFactory.CreateLogger<MigrationPlanExecutor>();
     }
 
-    [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<IUmbracoDatabaseFactory>(),
+            StaticServiceProvider.Instance.GetRequiredService<IPublishedSnapshotService>(),
+            StaticServiceProvider.Instance.GetRequiredService<DistributedCache>(),
+            StaticServiceProvider.Instance.GetRequiredService<IKeyValueService>())
+    {
+    }
+
+    [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 15.")]
     public MigrationPlanExecutor(
         ICoreScopeProvider scopeProvider,
         IScopeAccessor scopeAccessor,
@@ -76,7 +102,9 @@
             migrationBuilder,
             StaticServiceProvider.Instance.GetRequiredService<IUmbracoDatabaseFactory>(),
             StaticServiceProvider.Instance.GetRequiredService<IPublishedSnapshotService>(),
-            StaticServiceProvider.Instance.GetRequiredService<DistributedCache>())
+            StaticServiceProvider.Instance.GetRequiredService<DistributedCache>(),
+            StaticServiceProvider.Instance.GetRequiredService<IKeyValueService>()
+            )
     {
     }
 
@@ -92,7 +120,6 @@
     /// <para>Each migration in the plan, may or may not run in a scope depending on the type of plan.</para>
     /// <para>A plan can complete partially, the changes of each completed migration will be saved.</para>
     /// </remarks>
-    [Obsolete("This will return an ExecutedMigrationPlan in V13")]
     public ExecutedMigrationPlan ExecutePlan(MigrationPlan plan, string fromState)
     {
         plan.Validate();
@@ -104,6 +131,7 @@
         // 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,30 +188,37 @@
             {
                 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)
             {
                 _logger.LogError(exception, "Plan {PlanName} failed at step {TargetState}", plan.Name, transition.TargetState);
 
                 // We have to always return something, so whatever running this has a chance to save the state we got to.
                 return new ExecutedMigrationPlan
                 {
                     Successful = false,
                     Exception = exception,
                     InitialState = fromState,
                     FinalState = transition.SourceState,
                     CompletedTransitions = completedTransitions,
                     Plan = plan,
                     ExecutedMigrationContexts = executedMigrationContexts
                 };
             }
 
+
+            IEnumerable<IMigrationContext> 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.");
+            }
+
             // 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 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<MigrationContext>());
+        var context = new MigrationContext(plan, database, _loggerFactory.CreateLogger<MigrationContext>(), () => OnComplete(plan, transition.TargetState));
 
-        RunMigration(migrationType, context);
+        RunMigration(transition.MigrationType, context);
 
         return context;
     }
 
-    private MigrationContext RunScopedMigration(Type migrationType, MigrationPlan plan)
+    private void OnComplete(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 @@
             var context = new MigrationContext(
                 plan,
                 _scopeAccessor.AmbientScope?.Database,
-                _loggerFactory.CreateLogger<MigrationContext>());
+                _loggerFactory.CreateLogger<MigrationContext>(),
+                () => OnComplete(plan, transition.TargetState));
+
+            RunMigration(transition.MigrationType, context);
 
-            RunMigration(migrationType, context);
+            // Ensure we mark the context as complete before the scope completes
+            context.Complete();
 
             scope.Complete();
 

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;
     }
 

src/Umbraco.Infrastructure/Migrations/Upgrade/V_14_0_0/AddGuidsToUserGroups.cs

@@ -57,6 +57,8 @@ private void MigrateSqlServer()
             Database.Update(userGroup);
         }
 
+        Context.Complete();
+
         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.Complete();
         scope.Complete();
     }
 

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.Complete();
             scope.Complete();
             return;
         }
 
         MigrateSqlite();
+        Context.Complete();
         scope.Complete();
     }
 

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.Complete();
             scope.Complete();
             return;
         }
 
         MigrateSqlite();
+        Context.Complete();
         scope.Complete();
     }
 

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.Complete();
             scope.Complete();
             return;
         }
 
         MigrateUserTableSqlite();
+        Context.Complete();
         scope.Complete();
     }
 

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<IUmbracoVersion>();
     private IEventAggregator EventAggregator => GetRequiredService<IEventAggregator>();
-    private IMigrationPlanExecutor MigrationPlanExecutor => GetRequiredService<IMigrationPlanExecutor>();
+    private ICoreScopeProvider CoreScopeProvider => GetRequiredService<ICoreScopeProvider>();
+    private IScopeAccessor ScopeAccessor => GetRequiredService<IScopeAccessor>();
+    private ILoggerFactory LoggerFactory => GetRequiredService<ILoggerFactory>();
+    private IMigrationBuilder MigrationBuilder => GetRequiredService<IMigrationBuilder>();
+    private IUmbracoDatabaseFactory UmbracoDatabaseFactory => GetRequiredService<IUmbracoDatabaseFactory>();
+    private IPublishedSnapshotService PublishedSnapshotService => GetRequiredService<IPublishedSnapshotService>();
+    private DistributedCache DistributedCache => GetRequiredService<DistributedCache>();
+    private IMigrationPlanExecutor MigrationPlanExecutor => new MigrationPlanExecutor(
+        CoreScopeProvider,
+        ScopeAccessor,
+        LoggerFactory,
+        MigrationBuilder,
+        UmbracoDatabaseFactory,
+        PublishedSnapshotService,
+        DistributedCache,
+        Mock.Of<IKeyValueService>());
 
     [Test]
     public void CreateTableOfTDto()

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");

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.Complete();
     }
 }
 
@@ -354,4 +356,4 @@ public SimpleMigrationStep(
         : base(context) => _logger = logger;
 
     protected override void Migrate() => _logger.LogDebug("Here be migration");
-}
+}

tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs

@@ -76,7 +76,7 @@ public void CanExecute()
             loggerFactory,
             migrationBuilder,
             databaseFactory,
-            Mock.Of<IPublishedSnapshotService>(), distributedCache);
+            Mock.Of<IPublishedSnapshotService>(), distributedCache, Mock.Of<IKeyValueService>());
 
         var plan = new MigrationPlan("default")
             .From(string.Empty)