U4-10988 - MultiPocoFactory has a serious bug

Created by Lennard Fonteijn 19 Feb 2018, 16:25:51 Updated by Maarten van der Donk 01 May 2018, 13:24:06

Relates to: U4-10545

Introduction Long post incoming, last-section is advice to fix found issues.

Problem Upon inspecting U4-10545, I found the bug that was causing this issue. However, due to its severity I figured an own issue was justified.

MultiPocoFactory is used by a lot of internal classes, for example when Umbraco does the following to retrieve a whole range of Dto's at once:

https://github.com/umbraco/Umbraco-CMS/blob/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs#L60

When it does, a dynamic PetaPoco factory is created, tasked with coverting columns to mapped properties, for example UserDto:

https://github.com/umbraco/Umbraco-CMS/blob/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Models/Rdbms/UserDto.cs

However, because it has to do this in a batch of multiple types, some magic happens to determine when a new factory has to create for a given type. This magic happens in FindSplitPoint:

https://github.com/umbraco/Umbraco-CMS/blob/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Persistence/PetaPoco.cs#L1038

This is also where things go wrong, more specifically, this line:

https://github.com/umbraco/Umbraco-CMS/blob/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Persistence/PetaPoco.cs#L1055

On a clean Umbraco database, where all columns are in the correct order, this works great. But on a database that has seen multiple upgrades, or coming from much older versions. The order of items is not always as expected. For example, and Umbraco 7.6 has a startMediaID, userType and a startStructureID. In Umbraco 7.7, umbracoUserGroup-table gets new columns introduced, one of which is also startMediaId. And this is where the problem begins.

Due to the current splitting logic, it will see the startMediaID that's still part of the umbracoUser-table as the start of umbracoUserGroup-table, and stop gathering column for the Dto, meanign the Dto is not completely filled with information, making things topple over!

Follow-up The workaround for U4-10545 is to remove, at the very least, the startMediaID-column from the umbracoUser-table, prior to attempting an upgrade.

Code-wise, to make sure someone can at least login during an upgrade prior to removing above columns, we have to remove the following two lines and make it fallback to the "simpler" version of filling the UserDto (I tested this and was able to succesfully upgrade):

https://github.com/umbraco/Umbraco-CMS/blob/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Services/UserService.cs#L212 https://github.com/umbraco/Umbraco-CMS/blob/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Services/UserService.cs#L794

During the upgrade, a migration can then be run to remove the superfluous columns, as this is currently not the case upon inspecting the migrations here:

https://github.com/umbraco/Umbraco-CMS/tree/cf86409e3fec4198744189a48255543fc2f32614/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenSevenZero

Last but not least, the prevent issues like these in the future, the FindSplitPoint should be revised to just gather as much columns as possible, or find a different way to detect the split point in general.

Comments

Lennard Fonteijn 20 Feb 2018, 13:03:43

I have updated the workaround for U4-10545 - the key is not to drop it, but to move it towards the end of the table. Be sure to perform the upgrade without Umbraco Deploy installed.


Stephan 18 Apr 2018, 17:35:07

Ouch. Thanks for the report.

Just to be sure: the issue comes from us doing "SELECT * FROM..." and the database returning fields in varying order (depending on its history, upgrades, etc) -- and in some cases in an order that breaks or at least confuses FindSplitPoint, which moves to the next DTO without completing the current one? Correct?

I was afraid this might happen. In coming v8, we try to stop doing "SELECT *" and instead specify the exact fields to select, precisely to avoid this (which also confuses NPoco). Nevertheless. We probably need to look into this.


Lennard Fonteijn 18 Apr 2018, 19:08:09

@zpqrtbnk That is a correct assessment of the problem yes :) Right now, the easiest way for an user running into this is re-ordering the columns. Code-wise, this is indeed a pesky one to fix. I reckon the "easiest" way right now would be to ask PetaPoco which columns are expected to be there for a certain DTO, grab as much as possible (independant of order), before moving to the next DTO in the list.


Arnold Visser 01 May 2018, 11:08:29

We just did an upgrade that afterwards had an admin account with no section rights, when fixed via db we got sections but no content in them and then when debugging and trying things out, found this issue on the tracker. We'll try to dive deeper and give feedback in the issue.


Maarten van der Donk 01 May 2018, 13:24:06

As @arnold mentioned we did an upgrade today (7.6.11 to 7.10.4) which gave us the same problems. Changing the order of columns allowed me to log back into Umbraco but then I couldn't see any sections. This was because my user wasn't in any of the user groups, apparently this wasn't migrated while upgrading Umbraco. I created the following query to fix this: INSERT INTO umbracoUser2UserGroup (userId, userGroupId) SELECT UU.id, UU.userType FROM umbracoUser as UU WHERE UU.id > 0 --skip the default admin account

After this I could navigate in Umbraco (seeing sections) but content pickers didn't work (for example when selecting a user's root node) and no content was shown in the content section. This problem was caused by the columns order in the umbracoUser table, so I started changing that table: First I made sure all content- and mediastart nodes where migrated by using the following query: -- Content tree start node INSERT INTO umbracoUserStartNode (userId, startNode, startNodeType) SELECT UU.id, UU.startStructureID, 1 --I grabbed this number from the Umbraco source FROM umbracoUser as UU WHERE UU.id > 0 --skip the default admin account AND startStructureID > -1 --Only do this if a rootnode was selected

-- Media tree start node begin tran INSERT INTO umbracoUserStartNode (userId, startNode, startNodeType) SELECT UU.id, UU.startMediaID, 2 --I grabbed this number from the Umbraco source FROM umbracoUser as UU WHERE UU.id > 0 --skip the default admin account AND startMediaID > -1 --Only do this

After this was done I did some cleanup by removing the following columns (which where now migrated):

ALTER TABLE umbracoUser DROP CONSTRAINT FK_umbracoUser_umbracoUserType_id ALTER TABLE umbracoUser DROP COLUMN userType ALTER TABLE umbracoUser DROP COLUMN startStructureID ALTER TABLE umbracoUser DROP COLUMN startMediaID

Now I was able to start changing the order of the columns to match the new table structure (I created a new empty Umbraco website to compare tables):

This query was created by Microsoft SQL Server Management Studio, it can probably be more optimized. /* To prevent any potential data loss issues, you should review this script in detail before running it outside the context of the database designer.*/ BEGIN TRANSACTION SET QUOTED_IDENTIFIER ON SET ARITHABORT ON SET NUMERIC_ROUNDABORT OFF SET CONCAT_NULL_YIELDS_NULL ON SET ANSI_NULLS ON SET ANSI_PADDING ON SET ANSI_WARNINGS ON COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUser DROP CONSTRAINT DF_umbracoUser_userDisabled GO ALTER TABLE dbo.umbracoUser DROP CONSTRAINT DF_umbracoUser_userNoConsole GO ALTER TABLE dbo.umbracoUser DROP CONSTRAINT DF_umbracoUser_createDate GO ALTER TABLE dbo.umbracoUser DROP CONSTRAINT DF_umbracoUser_updateDate GO CREATE TABLE dbo.Tmp_umbracoUser ( id int NOT NULL IDENTITY (1, 1), userDisabled bit NOT NULL, userNoConsole bit NOT NULL, userName nvarchar(255) NOT NULL, userLogin nvarchar(125) NOT NULL, userPassword nvarchar(500) NOT NULL, passwordConfig nvarchar(500) NULL, userEmail nvarchar(255) NOT NULL, userLanguage nvarchar(10) NULL, securityStampToken nvarchar(255) NULL, failedLoginAttempts int NULL, lastLockoutDate datetime NULL, lastPasswordChangeDate datetime NULL, lastLoginDate datetime NULL, emailConfirmedDate datetime NULL, invitedDate datetime NULL, createDate datetime NOT NULL, updateDate datetime NOT NULL, avatar nvarchar(500) NULL, tourData ntext NULL ) ON [PRIMARY] TEXTIMAGE_ON [PRIMARY] GO ALTER TABLE dbo.Tmp_umbracoUser SET (LOCK_ESCALATION = TABLE) GO ALTER TABLE dbo.Tmp_umbracoUser ADD CONSTRAINT DF_umbracoUser_userDisabled DEFAULT ('0') FOR userDisabled GO ALTER TABLE dbo.Tmp_umbracoUser ADD CONSTRAINT DF_umbracoUser_userNoConsole DEFAULT ('0') FOR userNoConsole GO ALTER TABLE dbo.Tmp_umbracoUser ADD CONSTRAINT DF_umbracoUser_createDate DEFAULT (getdate()) FOR createDate GO ALTER TABLE dbo.Tmp_umbracoUser ADD CONSTRAINT DF_umbracoUser_updateDate DEFAULT (getdate()) FOR updateDate GO SET IDENTITY_INSERT dbo.Tmp_umbracoUser ON GO IF EXISTS(SELECT * FROM dbo.umbracoUser) EXEC('INSERT INTO dbo.Tmp_umbracoUser (id, userDisabled, userNoConsole, userName, userLogin, userPassword, passwordConfig, userEmail, userLanguage, securityStampToken, failedLoginAttempts, lastLockoutDate, lastPasswordChangeDate, lastLoginDate, emailConfirmedDate, invitedDate, createDate, updateDate, avatar, tourData) SELECT id, userDisabled, userNoConsole, userName, userLogin, userPassword, passwordConfig, userEmail, userLanguage, securityStampToken, failedLoginAttempts, lastLockoutDate, lastPasswordChangeDate, lastLoginDate, emailConfirmedDate, invitedDate, createDate, updateDate, avatar, tourData FROM dbo.umbracoUser WITH (HOLDLOCK TABLOCKX)') GO SET IDENTITY_INSERT dbo.Tmp_umbracoUser OFF GO ALTER TABLE dbo.umbracoUser2UserGroup DROP CONSTRAINT FK_umbracoUser2UserGroup_umbracoUser_id GO ALTER TABLE dbo.umbracoUserStartNode DROP CONSTRAINT FK_umbracoUserStartNode_umbracoUser_id GO ALTER TABLE dbo.umbracoUserLogin DROP CONSTRAINT FK_umbracoUserLogin_umbracoUser_id GO ALTER TABLE dbo.cmsTask DROP CONSTRAINT FK_cmsTask_umbracoUser GO ALTER TABLE dbo.cmsTask DROP CONSTRAINT FK_cmsTask_umbracoUser1 GO ALTER TABLE dbo.umbracoUser2NodeNotify DROP CONSTRAINT FK_umbracoUser2NodeNotify_umbracoUser_id GO ALTER TABLE dbo.umbracoUser2NodePermission DROP CONSTRAINT FK_umbracoUser2NodePermission_umbracoUser_id GO ALTER TABLE dbo.umbracoUser2app DROP CONSTRAINT FK_umbracoUser2app_umbracoUser_id GO DROP TABLE dbo.umbracoUser GO EXECUTE sp_rename N'dbo.Tmp_umbracoUser', N'umbracoUser', 'OBJECT' GO ALTER TABLE dbo.umbracoUser ADD CONSTRAINT PK_user PRIMARY KEY CLUSTERED ( id ) WITH( STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]

GO CREATE NONCLUSTERED INDEX IX_umbracoUser_userLogin ON dbo.umbracoUser ( userLogin ) WITH( STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY] GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUser2app ADD CONSTRAINT FK_umbracoUser2app_umbracoUser_id FOREIGN KEY ( [user] ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.umbracoUser2app SET (LOCK_ESCALATION = TABLE) GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUser2NodePermission ADD CONSTRAINT FK_umbracoUser2NodePermission_umbracoUser_id FOREIGN KEY ( userId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.umbracoUser2NodePermission SET (LOCK_ESCALATION = TABLE) GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUser2NodeNotify ADD CONSTRAINT FK_umbracoUser2NodeNotify_umbracoUser_id FOREIGN KEY ( userId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.umbracoUser2NodeNotify SET (LOCK_ESCALATION = TABLE) GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.cmsTask ADD CONSTRAINT FK_cmsTask_umbracoUser FOREIGN KEY ( parentUserId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.cmsTask ADD CONSTRAINT FK_cmsTask_umbracoUser1 FOREIGN KEY ( userId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.cmsTask SET (LOCK_ESCALATION = TABLE) GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUserLogin ADD CONSTRAINT FK_umbracoUserLogin_umbracoUser_id FOREIGN KEY ( userId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.umbracoUserLogin SET (LOCK_ESCALATION = TABLE) GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUserStartNode ADD CONSTRAINT FK_umbracoUserStartNode_umbracoUser_id FOREIGN KEY ( userId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.umbracoUserStartNode SET (LOCK_ESCALATION = TABLE) GO COMMIT BEGIN TRANSACTION GO ALTER TABLE dbo.umbracoUser2UserGroup ADD CONSTRAINT FK_umbracoUser2UserGroup_umbracoUser_id FOREIGN KEY ( userId ) REFERENCES dbo.umbracoUser ( id ) ON UPDATE NO ACTION ON DELETE NO ACTION

GO ALTER TABLE dbo.umbracoUser2UserGroup SET (LOCK_ESCALATION = TABLE) GO COMMIT

Now I started seeing content nodes and content pickers started working again.

There is still work to be done, for example: all userGroups have their default sections selected, in the old Umbraco this was done on user level so this information is now lost. Also all user specific node permissions are gone (didn't verify this)? If so than that should also be migrated.


Priority: Show-stopper

Type: Bug

State: Submitted

Assignee:

Difficulty: Easy

Category: Architecture

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.7.0

Due in version:

Sprint:

Story Points:

Cycle: