Migration needs to check for completed transitions before accessing (#14174)

* check for completed transitions before accessing target state

* upgrader should be happy with initial and final state being the same value - only an empty final state should throw

* refactor

* reduce complexity

* Don't check for successfull

* Add test that runs migration twice

---------

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

src/Umbraco.Infrastructure/Migrations/MigrationPlanExecutor.cs

@@ -175,11 +175,24 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor
 
         _logger.LogInformation("Done");
 
+        // final state is set to either the transition target state
+        // or the final completed transition target state if transition is null
+        // or the original migration state, if no transitions completed
+        string finalState = fromState;
+        if (transition is not null)
+        {
+            finalState = transition.TargetState;
+        }
+        else if (completedTransitions.Any())
+        {
+            finalState = completedTransitions.Last().TargetState;
+        }
+
         return new ExecutedMigrationPlan
         {
             Successful = true,
             InitialState = fromState,
-            FinalState = transition?.TargetState ?? completedTransitions.Last().TargetState,
+            FinalState = finalState,
             CompletedTransitions = completedTransitions,
             Plan = plan,
         };

src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs

@@ -36,37 +36,37 @@ public class Upgrader
     /// <param name="migrationPlanExecutor"></param>
     /// <param name="scopeProvider">A scope provider.</param>
     /// <param name="keyValueService">A key-value service.</param>
-    /// <param name="aggregator"></param>
     public ExecutedMigrationPlan Execute(
         IMigrationPlanExecutor migrationPlanExecutor,
         ICoreScopeProvider scopeProvider,
         IKeyValueService keyValueService)
     {
-        if (scopeProvider == null)
+        if (scopeProvider is null)
         {
             throw new ArgumentNullException(nameof(scopeProvider));
         }
 
-        if (keyValueService == null)
+        if (keyValueService is null)
         {
             throw new ArgumentNullException(nameof(keyValueService));
         }
 
-        var initialState = GetInitialState(scopeProvider, keyValueService);
+        string initialState = GetInitialState(scopeProvider, keyValueService);
 
         ExecutedMigrationPlan result = migrationPlanExecutor.ExecutePlan(Plan, initialState);
 
-        if (string.IsNullOrWhiteSpace(result.FinalState) || result.FinalState == result.InitialState)
+        // This should never happen, if the final state comes back as null or equal to the initial state
+        // it means that no transitions was successful, which means it cannot be a successful migration
+        if (result.Successful && string.IsNullOrWhiteSpace(result.FinalState))
         {
-            // This should never happen, if the final state comes back as null or equal to the initial state
-            // it means that no transitions was successful, which means it cannot be a successful migration
-            if (result.Successful)
-            {
-                throw new InvalidOperationException("Plan execution returned an invalid null or empty state.");
-            }
+            throw new InvalidOperationException("Plan execution returned an invalid null or empty state.");
+        }
 
-            // Otherwise it just means that our migration failed on the first step, which is fine.
-            // We will skip saving the state since we it's still the same
+        // Otherwise it just means that our migration failed on the first step, which is fine,
+        // or there were no pending transitions so nothing changed.
+        // We will skip saving the state since we it's still the same
+        if (result.FinalState == result.InitialState)
+        {
             return result;
         }
 

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs

@@ -1,4 +1,5 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
 using NPoco;
 using NUnit.Framework;
 using Umbraco.Cms.Core.Configuration;
@@ -91,6 +92,26 @@ public class PartialMigrationsTests : UmbracoIntegrationTest
         });
     }
 
+    [Test]
+    public void CanRunMigrationTwice()
+    {
+        Upgrader? upgrader = new(new SimpleMigrationPlan());
+        Upgrader? upgrader2 = new(new SimpleMigrationPlan());
+        var result = upgrader.Execute(MigrationPlanExecutor, ScopeProvider, KeyValueService);
+        var result2 = upgrader2.Execute(MigrationPlanExecutor, ScopeProvider, KeyValueService);
+
+        Assert.Multiple(() =>
+        {
+            Assert.True(result.Successful);
+            Assert.AreEqual("SimpleMigrationPlan_InitialState", result.InitialState);
+            Assert.AreEqual("SimpleMigrationStep", result.FinalState);
+            Assert.AreEqual(1, result.CompletedTransitions.Count);
+            Assert.IsNull(result.Exception);
+            Assert.True(result2.Successful);
+            Assert.IsNull(result2.Exception);
+        });
+    }
+
     [Test]
     public void StateIsOnlySavedIfAMigrationSucceeds()
     {
@@ -307,4 +328,30 @@ internal class TestUmbracoPlan : UmbracoPlan
         To<ErrorMigration>("b");
         To<AddColumnMigration>("c");
     }
+}
+
+internal class SimpleMigrationPlan : MigrationPlan
+{
+    public SimpleMigrationPlan()
+        : base("SimpleMigrationPlan") => DefinePlan();
+
+    public override string InitialState => "SimpleMigrationPlan_InitialState";
+
+    private void DefinePlan()
+    {
+        MigrationPlan plan = From(InitialState)
+            .To<SimpleMigrationStep>(nameof(SimpleMigrationStep));
+    }
+}
+
+internal class SimpleMigrationStep : MigrationBase
+{
+    private readonly ILogger<SimpleMigrationStep> _logger;
+
+    public SimpleMigrationStep(
+        IMigrationContext context,
+        ILogger<SimpleMigrationStep> logger)
+        : base(context) => _logger = logger;
+
+    protected override void Migrate() => _logger.LogDebug("Here be migration");
 }