RC2 Breaking - Ensure migrations persist the executed key, when executed. (#16113)

* 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

* Updated tests

* Renamed after review suggestion

* Rename in test

* More renaming after review

* Remove public modifier from interface

* Add missing space in exception message

---------

Co-authored-by: nikolajlauridsen <nikolajlauridsen@protonmail.ch>

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 @@ internal class MigrationContext : IMigrationContext
     /// <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 @@ 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 class MigrationPlanExecutor : IMigrationPlanExecutor
         IMigrationBuilder migrationBuilder,
         IUmbracoDatabaseFactory databaseFactory,
         IPublishedSnapshotService publishedSnapshotService,
-        DistributedCache distributedCache)
+        DistributedCache distributedCache,
+        IKeyValueService keyValueService)
     {
         _scopeProvider = scopeProvider;
         _scopeAccessor = scopeAccessor;
@@ -59,11 +63,33 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
         _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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
             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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
     /// <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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
         // 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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
             {
                 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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
                 };
             }
 
+
+            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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
         };
     }
 
-    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 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
             var context = new MigrationContext(
                 plan,
                 _scopeAccessor.AmbientScope?.Database,
-                _loggerFactory.CreateLogger<MigrationContext>());
+                _loggerFactory.CreateLogger<MigrationContext>(),
+                () => OnComplete(plan, transition.TargetState));
 
-            RunMigration(migrationType, context);
+            RunMigration(transition.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 class Upgrader
             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 @@ public class AddGuidsToUserGroups : UnscopedMigrationBase
             Database.Update(userGroup);
         }
 
+        Context.Complete();
+
         scope.Complete();
     }
 
@@ -87,6 +89,8 @@ public class AddGuidsToUserGroups : UnscopedMigrationBase
 
         // 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 @@ internal class AddGuidsToUsers : UnscopedMigrationBase
         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 @@ public class AddListViewKeysToDocumentTypes : UnscopedMigrationBase
         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 @@ internal class MigrateTours : UnscopedMigrationBase
         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 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Migrations.Expres
                 .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 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Migrations.Expres
                 .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 @@ internal class AssertScopeUnscopedTestMigration : UnscopedMigrationBase
 
         using var scope = _scopeProvider.CreateScope();
         Assert.IsNull(((Scope)scope).ParentScope);
+
+        Context.Complete();
     }
 }
 
@@ -354,4 +356,4 @@ internal class SimpleMigrationStep : MigrationBase
         : 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 class MigrationPlanTests
             loggerFactory,
             migrationBuilder,
             databaseFactory,
-            Mock.Of<IPublishedSnapshotService>(), distributedCache);
+            Mock.Of<IPublishedSnapshotService>(), distributedCache, Mock.Of<IKeyValueService>());
 
         var plan = new MigrationPlan("default")
             .From(string.Empty)