Umbraco-CMS: Upgrade from v7 to v8.6+ throws exception on media URL migration
Overview
In trying to upgrade a database that uses RTEs and Grid heavily, the upgrade fails if I got directly to v8.6.0. It works if I first go to an earlier version, and then try to go to v8.6.0. The error is in the ConvertTinyMceAndGridMediaUrlsToLocalLink migration, on line 113.
Details
Umbraco version
I am seeing this issue on Umbraco version: v8.6.0, from v7.15.3
Bug summary
This is caused by the ConvertTinyMceAndGridMediaUrlsToLocalLink migration using a service reference to retrieve data instead of using direct database access. Thus when new columns were introduced in the AddPropertyTypeValidationMessageColumns migration, the service is trying to read from these new columns that don’t yet exist in the database.
Steps to reproduce
Create a v7.15.3 database, and add Grid and RTE data that contain media links. Then try to upgrade the database to v8.6.0.
Expected result
The site is upgraded to v8.6.0 successfully.
Actual result
The upgrade wizard fails with the message “The database failed to upgrade. ERROR: The database configuration failed with the following message: Invalid column name ‘mandatoryMessage’. Invalid column name ‘validationRegExpMessage’. Please check log file for additional information (can be found in ‘/App_Data/Logs/’)”. Reviewing the trace log shows the following stack trace for that error:
System.Data.SqlClient.SqlException (0x80131904): Invalid column name 'mandatoryMessage'.
Invalid column name 'validationRegExpMessage'.
at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
at System.Data.SqlClient.SqlDataReader.TryConsumeMetaData()
at System.Data.SqlClient.SqlDataReader.get_MetaData()
at System.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString, Boolean isInternal, Boolean forDescribeParameterEncryption, Boolean shouldCacheForAlwaysEncrypted)
at System.Data.SqlClient.SqlCommand.RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, Boolean async, Int32 timeout, Task& task, Boolean asyncWrite, Boolean inRetry, SqlDataReader ds, Boolean describeParameterEncryptionRequest)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)
at System.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior, String method)
at StackExchange.Profiling.Data.ProfiledDbCommand.ExecuteDbDataReader(CommandBehavior behavior) in C:\projects\dotnet\src\MiniProfiler.Shared\Data\ProfiledDbCommand.cs:line 223
at Umbraco.Core.Persistence.FaultHandling.RetryPolicy.ExecuteAction[TResult](Func`1 func) in C:\projects\umbraco-cms\src\Umbraco.Core\Persistence\FaultHandling\RetryPolicy.cs:line 172
at NPoco.Database.ExecuteReaderHelper(DbCommand cmd)
at NPoco.Database.ExecuteDataReader(DbCommand cmd)
at NPoco.Database.<QueryImp>d__164`1.MoveNext()
at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
at Umbraco.Core.Persistence.Repositories.Implement.ContentTypeCommonRepository.MapGroupsAndProperties(IDictionary`2 contentTypes) in C:\projects\umbraco-cms\src\Umbraco.Core\Persistence\Repositories\Implement\ContentTypeCommonRepository.cs:line 192
at Umbraco.Core.Persistence.Repositories.Implement.ContentTypeCommonRepository.GetAllTypesInternal() in C:\projects\umbraco-cms\src\Umbraco.Core\Persistence\Repositories\Implement\ContentTypeCommonRepository.cs:line 114
at Umbraco.Core.Cache.AppCacheExtensions.<>c__DisplayClass0_0`1.<GetCacheItem>b__0() in C:\projects\umbraco-cms\src\Umbraco.Core\Cache\AppCacheExtensions.cs:line 22
at Umbraco.Core.Cache.FastDictionaryAppCacheBase.<>c__DisplayClass21_0.<GetSafeLazy>b__0() in C:\projects\umbraco-cms\src\Umbraco.Core\Cache\FastDictionaryAppCacheBase.cs:line 285
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at Umbraco.Core.Cache.WebCachingAppCache.GetInternal(String key, Func`1 factory, Nullable`1 timeout, Boolean isSliding, CacheItemPriority priority, CacheItemRemovedCallback removedCallback, String[] dependentFiles) in C:\projects\umbraco-cms\src\Umbraco.Core\Cache\WebCachingAppCache.cs:line 174
at Umbraco.Core.Cache.WebCachingAppCache.Get(String key, Func`1 factory, Nullable`1 timeout, Boolean isSliding, CacheItemPriority priority, CacheItemRemovedCallback removedCallback, String[] dependentFiles) in C:\projects\umbraco-cms\src\Umbraco.Core\Cache\WebCachingAppCache.cs:line 40
at Umbraco.Core.Cache.DeepCloneAppCache.Get(String key, Func`1 factory, Nullable`1 timeout, Boolean isSliding, CacheItemPriority priority, CacheItemRemovedCallback removedCallback, String[] dependentFiles)
at Umbraco.Core.Persistence.Repositories.Implement.ContentTypeCommonRepository.GetAllTypes() in C:\projects\umbraco-cms\src\Umbraco.Core\Persistence\Repositories\Implement\ContentTypeCommonRepository.cs:line 48
at Umbraco.Core.Persistence.Repositories.Implement.MediaTypeRepository.PerformGetAll(Int32[] ids) in C:\projects\umbraco-cms\src\Umbraco.Core\Persistence\Repositories\Implement\MediaTypeRepository.cs:line 54
at Umbraco.Core.Cache.FullDataSetRepositoryCachePolicy`2.GetAllCached(Func`2 performGetAll) in C:\projects\umbraco-cms\src\Umbraco.Core\Cache\FullDataSetRepositoryCachePolicy.cs:line 166
at Umbraco.Core.Cache.FullDataSetRepositoryCachePolicy`2.Get(TId id, Func`2 performGet, Func`2 performGetAll)
at Umbraco.Core.Persistence.Repositories.Implement.MediaRepository.MapDtoToContent(ContentDto dto) in C:\projects\umbraco-cms\src\Umbraco.Core\Persistence\Repositories\Implement\MediaRepository.cs:line 538
at Umbraco.Core.Services.Implement.MediaService.GetMediaByPath(String mediaPath) in C:\projects\umbraco-cms\src\Umbraco.Core\Services\Implement\MediaService.cs:line 631
at Umbraco.Core.Migrations.Upgrade.V_8_1_0.ConvertTinyMceAndGridMediaUrlsToLocalLink.<>c__DisplayClass3_0.<UpdateMediaUrls>b__0(Match match) in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\Upgrade\V_8_1_0\ConvertTinyMceAndGridMediaUrlsToLocalLink.cs:line 113
at System.Text.RegularExpressions.RegexReplacement.Replace(MatchEvaluator evaluator, Regex regex, String input, Int32 count, Int32 startat)
at System.Text.RegularExpressions.Regex.Replace(String input, MatchEvaluator evaluator)
at Umbraco.Core.Migrations.Upgrade.V_8_1_0.ConvertTinyMceAndGridMediaUrlsToLocalLink.UpdateMediaUrls(Regex mediaLinkPattern, String value, Boolean& changed) in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\Upgrade\V_8_1_0\ConvertTinyMceAndGridMediaUrlsToLocalLink.cs:line 103
at Umbraco.Core.Migrations.Upgrade.V_8_1_0.ConvertTinyMceAndGridMediaUrlsToLocalLink.Migrate() in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\Upgrade\V_8_1_0\ConvertTinyMceAndGridMediaUrlsToLocalLink.cs:line 59
at Umbraco.Core.Migrations.MigrationBase.Umbraco.Core.Migrations.IMigration.Migrate() in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\MigrationBase.cs:line 73
at Umbraco.Core.Migrations.MigrationPlan.Execute(IScope scope, String fromState, IMigrationBuilder migrationBuilder, ILogger logger) in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\MigrationPlan.cs:line 316
at Umbraco.Core.Migrations.Upgrade.Upgrader.Execute(IScopeProvider scopeProvider, IMigrationBuilder migrationBuilder, IKeyValueService keyValueService, ILogger logger) in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\Upgrade\Upgrader.cs:line 67
at Umbraco.Core.Migrations.Install.DatabaseBuilder.UpgradeSchemaAndData(MigrationPlan plan) in C:\projects\umbraco-cms\src\Umbraco.Core\Migrations\Install\DatabaseBuilder.cs:line 501
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Reactions: 1
- Comments: 38 (25 by maintainers)
Its pretty disrespectful to throw shade on the publicly available package I created to fix the problem I identified for you when your internal testing failed, and then submitted a tested fix for which you ignored, and so finally created a community package to fix the issue instead of just fixing it for myself, all because my one-off package to fix your hole doesn’t have as much documentation as you’d like to see. That’s pretty low.
And as has been said above, this isn’t affecting just a few people. This bug actually affect EVERY upgrade that tries to go directly from v7 to v8.6 or above. So either you were an early adopter, or you hit this bug. There isn’t any way around it, and it doesn’t only affect you if you have a specific set of data. It affects every upgrade. If you haven’t had more reports of it, that means one of two things: either your clients aren’t upgrading to v8 or they are so used to poor service that they aren’t even bothering to mention it to you anymore. Both of those should really be pretty chilling options to anyone who cares about their product.
The problem comes because a migration (designed to interact directly with the database while the database is in a state of flux) is calling into the service layer (designed to interact with a stable database to apply current business logic). If you look at what it is trying to get from the service layer, it is just trying to get a single piece of information about media items. This could very easily be achieved with a simple DTO query, my guess is that the migration developer was simply more familiar with how to get it via the service layer, and so went with what they were familiar with. Without any standards on what should or shouldn’t be done in migrations, this approach completely makes sense.
I think the long term fix for this is that migrations included as part of the core should simply not be allowed to use the service layer. Any data access should be done with queries and version-specific DTOs, similar to what was done for the bulk of the 8.0 and 8.1 migrations. It was done right for the bulk of the 8.X upgrade migrations, I think this one just slipped through because there wasn’t a standard that a reviewer could check against, because the problem wasn’t well understood. Now that the impact of this is seen, just add a note for reviewers to check for and not allow service layer access in migrations, and rewrite this one existing migration to not use the service layer, but to do a direct DB query. Problem solved.
@nul800sebastiaan So, this morning I had a new thought about the issues with the DTDs missing columns… (see my original investigation of the failure point):
It occurred to me that it’s possible this sort of issue could easily arise in the future - with any DB changes made in future versions.
Perhaps it would make sense to have a error catch on any DTO “Fetch” operations, which if triggered, then calls a method (present on all defined DTOs) named something like “CheckAndFixCols()”, which would just loop through all the expected columns, and if a column doesn’t exist on the table, adds it. The method could count how many cols were added, and pass that back to the error catch so that if cols were fixed, it could attempt the operation again, and if nothing was fixed, the error would go through with default logging/YSOD (to avoid a never-ending loop if the error was not caused by missing columns).
Anyway, this is a broader change than just this one issue - something which might handle any sort of DB changes in the future without the need for extra “temporary” DTOs, etc.
Yeah, I couldn’t agree more @arknu it certainly has seemed that way for quite a while, like the cloud gubbins getting features and not the open source variant its a shame considering the open source side is what “made” it. As for the upgrades, migrations from old versions of a product like this to the newer latest version should always work, not to mention its completely false to say its because of not anticipating people having such custom databases or whatever rubbish – that should not affect Umbraco migrating its own data and tables over, not to mention we’re talking about Umbraco’s own data migrations not including 3rd party data, if 3rd party changes or so break it then it would be unreasonable to expect that to work or be fixed without issue.
In terms of saying that people are slow to upgrade or similar, its not really surprising in the instance of anyone dealing with either large companies or even small ones that have invested. The process of getting either of the 2 to put money into the upgrade effort is exceptionally hard to do, hence why 80-90% of the client sites I and many others work on are still V7. So no, its not that surprising to be migrating from V7 to the very latest version.
If you want to work around this issue until one of the PRs fixing this is merged in, and you don’t want to have to stop along the way, you can use the ProWorks.Umbraco8.Migrations package (with source code here). It works around this issue with a custom IMediaService implementation that is used only during the initial v7 to v8 upgrade, and isn’t used after that. It also includes a bunch of fixes for the #7939 issue related to content that didn’t get migrated properly from v7 to v8, which were in another PR that is still pending, #7957.
For all who have left a comment, be aware that you can still upgrade to v8.6.X right now, you just have to do it in two stages. First upgrade to v8.1, then once the DB has been upgraded apply v8.6.X. Multiple PRs have been submitted for this, so hopefully one of those will make it into v8.7 to allow for direct migration up from v7. I don’t think we can expect a migration change in a patch release on v8.6.
I’m not sure how things work in terms of messing with the Cloud if you are referring to all that Umbraco made gubbins, I don’t bother with it, what you need to do however you do it, is grab a copy of say 8.2 or so, upgrade to it, the DB should then be modified as required, after which you can go straight to the latest version.
I’m not sure that you should have to mess with the direct versions of the original Umbraco source as you should be able to get the files needed from the releases page which should be all you need I believe, either that or just use NuGet to change versions.
https://our.umbraco.com/download/releases/850 for example 😃
So for anyone else reading this until Umbraco at some point hopefully get round to fixing this irritating issue which hopefully won’t be long considering how important it is the steps are below:
Let me know if you need any help 😃
Very interesting @hfloyd and I much prefer this change to the addition of a lot of code by @benjaminc 😃
However, I am wondering if we should use an intermittent DTO instead, I think that’s what we used to do in v7. I’ll need to have a chat at HQ and see what we want to do with this!
I had the same issue migrating from a 7.15.4 site to 8.6. What fixed it for me was to update “UmbracoPlan.cs” to move:
To<AddPropertyTypeValidationMessageColumns>("{3D67D2C8-5E65-47D0-A9E1-DC2EE0779D6B}");up above the line:
To<ConvertTinyMceAndGridMediaUrlsToLocalLink>("{B69B6E8C-A769-4044-A27E-4A4E18D1645A}");I think the issue is actually caused by a call to “MapDtoToContent()” - the v8.6 Dto assumes the existence of those DB columns which haven’t been added yet.
This is a screenshot of the code with a debugger running:
The green highlight is where the error was bubbling up. I’ve created a pull request for this: https://github.com/umbraco/Umbraco-CMS/pull/7940