From 7b46d44eeb08732d8257a29cffa38f3b570c5884 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 22 Apr 2024 12:21:16 +0200 Subject: [PATCH] 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 --- .../Migrations/IMigrationContext.cs | 4 ++ .../Migrations/MigrationContext.cs | 19 +++++- .../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 + .../Migrations/AdvancedMigrationTests.cs | 23 ++++++- .../Expressions/CreateTableExpressionTests.cs | 4 +- .../Migrations/PartialMigrationsTests.cs | 4 +- .../Migrations/MigrationPlanTests.cs | 2 +- 12 files changed, 116 insertions(+), 22 deletions(-) diff --git a/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/IMigrationContext.cs index 5e0766755a..cf68617315 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; + + bool IsCompleted { get; } + + void Complete(); } diff --git a/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs b/src/Umbraco.Infrastructure/Migrations/MigrationContext.cs index f753707770..4af60ece42 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? _onCompleteAction; 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? 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 /// public bool BuildingExpression { get; set; } + public bool IsCompleted { get; private set; } = false; + + public void Complete() + { + if (IsCompleted) + { + return; + } + + _onCompleteAction?.Invoke(); + + IsCompleted = 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 a3646ed5c9..31a9a3fa5f 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 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(); } - [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 class MigrationPlanExecutor : IMigrationPlanExecutor migrationBuilder, StaticServiceProvider.Instance.GetRequiredService(), StaticServiceProvider.Instance.GetRequiredService(), - StaticServiceProvider.Instance.GetRequiredService()) + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService() + ) { } @@ -92,7 +120,6 @@ public class MigrationPlanExecutor : IMigrationPlanExecutor /// 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 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 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()); + var context = new MigrationContext(plan, database, _loggerFactory.CreateLogger(), () => 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()); + _loggerFactory.CreateLogger(), + () => 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(); diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs index 35c04e08e0..b73967f400 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/Upgrader.cs +++ b/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; } 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 bc4b287eae..ad8e5091ea 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 @@ 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(); } 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 0995acfeeb..e5edce8e87 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 @@ internal class AddGuidsToUsers : UnscopedMigrationBase if (DatabaseType != DatabaseType.SQLite) { MigrateSqlServer(); + Context.Complete(); scope.Complete(); return; } MigrateSqlite(); + 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 de54432c73..452160d25b 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 @@ public class AddListViewKeysToDocumentTypes : UnscopedMigrationBase if (DatabaseType != DatabaseType.SQLite) { MigrateSqlServer(); + Context.Complete(); scope.Complete(); return; } MigrateSqlite(); + 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 fedfd43c51..302d49e763 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 @@ internal class MigrateTours : UnscopedMigrationBase if (DatabaseType != DatabaseType.SQLite) { MigrateUserTableSqlServer(); + Context.Complete(); scope.Complete(); return; } MigrateUserTableSqlite(); + Context.Complete(); scope.Complete(); } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/AdvancedMigrationTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/AdvancedMigrationTests.cs index 792f40ac2e..084efb2371 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 ff508c8f32..9f44122f20 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 @@ 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"); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/PartialMigrationsTests.cs index cb857befda..1fb0e78417 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 @@ 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"); -} \ No newline at end of file +} diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/MigrationPlanTests.cs index 8956fe4315..fd279b5b3a 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 class MigrationPlanTests loggerFactory, migrationBuilder, databaseFactory, - Mock.Of(), distributedCache); + Mock.Of(), distributedCache, Mock.Of()); var plan = new MigrationPlan("default") .From(string.Empty)