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 <[email protected]>

Author: bergmania

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 @@ 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,19 +54,42 @@ public MigrationPlanExecutor(
         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 @@ public MigrationPlanExecutor(
             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 MigrationPlanExecutor(
     /// <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 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<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 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<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 @@ private MigrationContext RunScopedMigration(Type migrationType, MigrationPlan pl
             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");