From 889e48ea4a2b01e5a8548e2f5315fe04a1545552 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 15 Nov 2018 19:34:32 +0100 Subject: [PATCH] Detect incomplete migrations --- src/Umbraco.Core/Migrations/IMigration.cs | 3 + .../Migrations/IMigrationContext.cs | 7 +- .../IncompleteMigrationExpressionException.cs | 28 ++++++++ src/Umbraco.Core/Migrations/MigrationBase.cs | 39 ++++++++--- .../Migrations/MigrationContext.cs | 13 ++++ .../Migrations/MigrationExpressionBase.cs | 1 + .../Migrations/Upgrade/UmbracoPlan.cs | 3 +- .../Upgrade/V_8_0_0/TagsMigration.cs | 4 +- .../Upgrade/V_8_0_0/TagsMigrationFix.cs | 16 +++++ src/Umbraco.Core/Umbraco.Core.csproj | 2 + .../Migrations/MigrationTests.cs | 68 ++++++++++++++++++- 11 files changed, 172 insertions(+), 12 deletions(-) create mode 100644 src/Umbraco.Core/Migrations/IncompleteMigrationExpressionException.cs create mode 100644 src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigrationFix.cs diff --git a/src/Umbraco.Core/Migrations/IMigration.cs b/src/Umbraco.Core/Migrations/IMigration.cs index 53b7874b3a..c929234f77 100644 --- a/src/Umbraco.Core/Migrations/IMigration.cs +++ b/src/Umbraco.Core/Migrations/IMigration.cs @@ -7,6 +7,9 @@ namespace Umbraco.Core.Migrations /// public interface IMigration : IDiscoverable { + /// + /// Executes the migration. + /// void Migrate(); } } diff --git a/src/Umbraco.Core/Migrations/IMigrationContext.cs b/src/Umbraco.Core/Migrations/IMigrationContext.cs index 4db1b07b63..80ba78b6de 100644 --- a/src/Umbraco.Core/Migrations/IMigrationContext.cs +++ b/src/Umbraco.Core/Migrations/IMigrationContext.cs @@ -24,8 +24,13 @@ namespace Umbraco.Core.Migrations ISqlContext SqlContext { get; } /// - /// Gets the expression index. + /// Gets or sets the expression index. /// int Index { get; set; } + + /// + /// Gets or sets a value indicating whether an expression is being built. + /// + bool BuildingExpression { get; set; } } } diff --git a/src/Umbraco.Core/Migrations/IncompleteMigrationExpressionException.cs b/src/Umbraco.Core/Migrations/IncompleteMigrationExpressionException.cs new file mode 100644 index 0000000000..91d1838d6f --- /dev/null +++ b/src/Umbraco.Core/Migrations/IncompleteMigrationExpressionException.cs @@ -0,0 +1,28 @@ +using System; + +namespace Umbraco.Core.Migrations +{ + /// + /// Represents errors that occurs when a migration exception is not executed. + /// + /// + /// Migration expression such as Alter.Table(...).Do() *must* end with Do() else they are + /// not executed. When a non-executed expression is detected, an IncompleteMigrationExpressionException + /// is thrown. + /// + public class IncompleteMigrationExpressionException : Exception + { + /// + /// Initializes a new instance of the class. + /// + public IncompleteMigrationExpressionException() + { } + + /// + /// Initializes a new instance of the class with a message. + /// + public IncompleteMigrationExpressionException(string message) + : base(message) + { } + } +} diff --git a/src/Umbraco.Core/Migrations/MigrationBase.cs b/src/Umbraco.Core/Migrations/MigrationBase.cs index 9fbee0ed92..58edcae80a 100644 --- a/src/Umbraco.Core/Migrations/MigrationBase.cs +++ b/src/Umbraco.Core/Migrations/MigrationBase.cs @@ -62,42 +62,65 @@ namespace Umbraco.Core.Migrations /// protected Sql Sql(string sql, params object[] args) => Context.SqlContext.Sql(sql, args); - /// + /// + /// Executes the migration. + /// public abstract void Migrate(); + /// + void IMigration.Migrate() + { + Migrate(); + + // ensure there is no building expression + // ie we did not forget to .Do() an expression + if (Context.BuildingExpression) + throw new IncompleteMigrationExpressionException("The migration has run, but leaves an expression that has not run."); + } + + // ensures we are not already building, + // ie we did not forget to .Do() an expression + private T BeginBuild(T builder) + { + if (Context.BuildingExpression) + throw new IncompleteMigrationExpressionException("Cannot create a new expression: the previous expression has not run."); + Context.BuildingExpression = true; + return builder; + } + /// /// Builds an Alter expression. /// - public IAlterBuilder Alter => new AlterBuilder(Context); + public IAlterBuilder Alter => BeginBuild(new AlterBuilder(Context)); /// /// Builds a Create expression. /// - public ICreateBuilder Create => new CreateBuilder(Context); + public ICreateBuilder Create => BeginBuild(new CreateBuilder(Context)); /// /// Builds a Delete expression. /// - public IDeleteBuilder Delete => new DeleteBuilder(Context); + public IDeleteBuilder Delete => BeginBuild(new DeleteBuilder(Context)); /// /// Builds an Execute expression. /// - public IExecuteBuilder Execute => new ExecuteBuilder(Context); + public IExecuteBuilder Execute => BeginBuild(new ExecuteBuilder(Context)); /// /// Builds an Insert expression. /// - public IInsertBuilder Insert => new InsertBuilder(Context); + public IInsertBuilder Insert => BeginBuild(new InsertBuilder(Context)); /// /// Builds a Rename expression. /// - public IRenameBuilder Rename => new RenameBuilder(Context); + public IRenameBuilder Rename => BeginBuild(new RenameBuilder(Context)); /// /// Builds an Update expression. /// - public IUpdateBuilder Update => new UpdateBuilder(Context); + public IUpdateBuilder Update => BeginBuild(new UpdateBuilder(Context)); } } diff --git a/src/Umbraco.Core/Migrations/MigrationContext.cs b/src/Umbraco.Core/Migrations/MigrationContext.cs index d0802c813d..da454fab03 100644 --- a/src/Umbraco.Core/Migrations/MigrationContext.cs +++ b/src/Umbraco.Core/Migrations/MigrationContext.cs @@ -4,20 +4,33 @@ using Umbraco.Core.Persistence; namespace Umbraco.Core.Migrations { + /// + /// Represents a migration context. + /// internal class MigrationContext : IMigrationContext { + /// + /// Initializes a new instance of the class. + /// public MigrationContext(IUmbracoDatabase database, ILogger logger) { Database = database ?? throw new ArgumentNullException(nameof(database)); Logger = logger ?? throw new ArgumentNullException(nameof(logger)); } + /// public ILogger Logger { get; } + /// public IUmbracoDatabase Database { get; } + /// public ISqlContext SqlContext => Database.SqlContext; + /// public int Index { get; set; } + + /// + public bool BuildingExpression { get; set; } } } diff --git a/src/Umbraco.Core/Migrations/MigrationExpressionBase.cs b/src/Umbraco.Core/Migrations/MigrationExpressionBase.cs index 4c5a1f1aa7..6ac92a07aa 100644 --- a/src/Umbraco.Core/Migrations/MigrationExpressionBase.cs +++ b/src/Umbraco.Core/Migrations/MigrationExpressionBase.cs @@ -50,6 +50,7 @@ namespace Umbraco.Core.Migrations if (_executed) throw new InvalidOperationException("This expression has already been executed."); _executed = true; + Context.BuildingExpression = false; var sql = GetSql(); diff --git a/src/Umbraco.Core/Migrations/Upgrade/UmbracoPlan.cs b/src/Umbraco.Core/Migrations/Upgrade/UmbracoPlan.cs index fb0f18700c..1edd54a510 100644 --- a/src/Umbraco.Core/Migrations/Upgrade/UmbracoPlan.cs +++ b/src/Umbraco.Core/Migrations/Upgrade/UmbracoPlan.cs @@ -119,7 +119,7 @@ namespace Umbraco.Core.Migrations.Upgrade Chain("{517CE9EA-36D7-472A-BF4B-A0D6FB1B8F89}"); // from 7.12.0 Chain("{BBD99901-1545-40E4-8A5A-D7A675C7D2F2}"); // from 7.12.0 //Chain("{2C87AA47-D1BC-4ECB-8A73-2D8D1046C27F}"); // stephan added that one = merge conflict, remove - + Chain("{8B14CEBD-EE47-4AAD-A841-93551D917F11}"); // add andy's after others, with a new target state From("{CF51B39B-9B9A-4740-BB7C-EAF606A7BFBF}") // and provide a path out of andy's .CopyChain("{39E5B1F7-A50B-437E-B768-1723AEC45B65}", "{BBD99901-1545-40E4-8A5A-D7A675C7D2F2}", "{8B14CEBD-EE47-4AAD-A841-93551D917F11}"); // to next @@ -141,6 +141,7 @@ namespace Umbraco.Core.Migrations.Upgrade Chain("{8804D8E8-FE62-4E3A-B8A2-C047C2118C38}"); Chain("{23275462-446E-44C7-8C2C-3B8C1127B07D}"); Chain("{6B251841-3069-4AD5-8AE9-861F9523E8DA}"); + Chain("{EE429F1B-9B26-43CA-89F8-A86017C809A3}"); //FINAL diff --git a/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigration.cs b/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigration.cs index 89cb7e74ac..5dc5e0b6fe 100644 --- a/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigration.cs +++ b/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigration.cs @@ -18,7 +18,9 @@ namespace Umbraco.Core.Migrations.Upgrade.V_8_0_0 // kill unused parentId column Delete.ForeignKey("FK_cmsTags_cmsTags").OnTable(Constants.DatabaseSchema.Tables.Tag).Do(); - Delete.Column("ParentId").FromTable(Constants.DatabaseSchema.Tables.Tag); + Delete.Column("ParentId").FromTable(Constants.DatabaseSchema.Tables.Tag).Do(); } } + + // fixes TagsMigration that... originally failed to properly drop the ParentId column } diff --git a/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigrationFix.cs b/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigrationFix.cs new file mode 100644 index 0000000000..4ee95c9f58 --- /dev/null +++ b/src/Umbraco.Core/Migrations/Upgrade/V_8_0_0/TagsMigrationFix.cs @@ -0,0 +1,16 @@ +namespace Umbraco.Core.Migrations.Upgrade.V_8_0_0 +{ + public class TagsMigrationFix : MigrationBase + { + public TagsMigrationFix(IMigrationContext context) + : base(context) + { } + + public override void Migrate() + { + // kill unused parentId column, if it still exists + if (ColumnExists(Constants.DatabaseSchema.Tables.Tag, "ParentId")) + Delete.Column("ParentId").FromTable(Constants.DatabaseSchema.Tables.Tag).Do(); + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 6762dc508c..2bfa52a80a 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -335,6 +335,7 @@ + @@ -367,6 +368,7 @@ + diff --git a/src/Umbraco.Tests/Migrations/MigrationTests.cs b/src/Umbraco.Tests/Migrations/MigrationTests.cs index b408d351d7..d06cf2244e 100644 --- a/src/Umbraco.Tests/Migrations/MigrationTests.cs +++ b/src/Umbraco.Tests/Migrations/MigrationTests.cs @@ -1,16 +1,19 @@ using System; using System.Data; +using Moq; +using NUnit.Framework; using Semver; using Umbraco.Core.Events; -using Umbraco.Core.Logging; using Umbraco.Core.Migrations; using Umbraco.Core.Migrations.Upgrade; using Umbraco.Core.Persistence; using Umbraco.Core.Scoping; using Umbraco.Core.Services; +using ILogger = Umbraco.Core.Logging.ILogger; namespace Umbraco.Tests.Migrations { + [TestFixture] public class MigrationTests { public class TestUpgrader : Upgrader @@ -67,5 +70,68 @@ namespace Umbraco.Tests.Migrations public IScopeContext Context { get; set; } public ISqlContext SqlContext { get; set; } } + + [Test] + public void RunGoodMigration() + { + var migrationContext = new MigrationContext(Mock.Of(), Mock.Of()); + IMigration migration = new GoodMigration(migrationContext); + migration.Migrate(); + } + + [Test] + public void DetectBadMigration1() + { + var migrationContext = new MigrationContext(Mock.Of(), Mock.Of()); + IMigration migration = new BadMigration1(migrationContext); + Assert.Throws(() => migration.Migrate()); + } + + [Test] + public void DetectBadMigration2() + { + var migrationContext = new MigrationContext(Mock.Of(), Mock.Of()); + IMigration migration = new BadMigration2(migrationContext); + Assert.Throws(() => migration.Migrate()); + } + + public class GoodMigration : MigrationBase + { + public GoodMigration(IMigrationContext context) + : base(context) + { } + + public override void Migrate() + { + Execute.Sql("").Do(); + } + } + + public class BadMigration1 : MigrationBase + { + public BadMigration1(IMigrationContext context) + : base(context) + { } + + public override void Migrate() + { + Alter.Table("foo"); // stop here, don't Do it + } + } + + public class BadMigration2 : MigrationBase + { + public BadMigration2(IMigrationContext context) + : base(context) + { } + + public override void Migrate() + { + Alter.Table("foo"); // stop here, don't Do it + + // and try to start another one + Alter.Table("bar"); + } + } } }