Skip to content

Fix primary key change in user_ldap migration#30568

Merged
skjnldsv merged 6 commits intomasterfrom
fix/user_ldap-fix-long-dn-migration
Jan 14, 2022
Merged

Fix primary key change in user_ldap migration#30568
skjnldsv merged 6 commits intomasterfrom
fix/user_ldap-fix-long-dn-migration

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 10, 2022

Use a backup table to copy the data, drop table and recreate it with
correct primary key, then copy the data back and drop the backup table.

Fix #30539

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

@come-nc come-nc self-assigned this Jan 10, 2022
@come-nc come-nc requested a review from nickvergessen January 10, 2022 11:09
@come-nc come-nc added the 2. developing Work in progress label Jan 10, 2022
blizzz
blizzz reviewed Jan 10, 2022
View reviewed changes
@come-nc come-nc force-pushed the fix/user_ldap-fix-long-dn-migration branch from 5d42916 to e2fc013 Compare January 10, 2022 13:03
blizzz
blizzz reviewed Jan 10, 2022
View reviewed changes
nickvergessen
nickvergessen requested changes Jan 10, 2022
View reviewed changes
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 11, 2022
blizzz
blizzz reviewed Jan 11, 2022
View reviewed changes
nickvergessen
nickvergessen approved these changes Jan 11, 2022
View reviewed changes
apps/user_ldap/lib/Migration/Version1130Date20211102154716.php Outdated
$changeSchema = true;
}

if ($table->getPrimaryKeyColumns() !== ['owncloud_name']) {
Copy link
Member

@nickvergessen nickvergessen Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind to comment which case there could be? From this file it's either none (then it's created) or 'owncloud_name'

Copy link
Contributor Author

@come-nc come-nc Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 23 primary key for ldap_group_mapping was ldap_dn, which is the reason for all this.
It was changed in https://github.com/nextcloud/server/pull/29523/files#diff-305e20bed2077c1acd13cbf1051427e1179e19d0bfa3054635a99a83827e01a3R89-R95

@come-nc come-nc force-pushed the fix/user_ldap-fix-long-dn-migration branch from e7f9d75 to 9d11d71 Compare January 13, 2022 09:58
@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

Ok, I tested the migration and had to refactor a bit, as enlarging the ldap_dn column on ldap_group_mapping was failing because it was a primary key. So now it always goes through the double copy/drop to get the ldap_group_mapping table in the right shape. It worked for my test but I only tested mysql.
I also had to update the autoloader which triggered a lot of small psalm changes, not sure if expected or not.

@nickvergessen
Copy link
Member

nickvergessen commented Jan 13, 2022

Conflicting files
apps/dav/composer/composer/installed.php

Maybe you can revert the autoloading paart and then only commit the modified files in apps/user_ldap?

nickvergessen
nickvergessen approved these changes Jan 13, 2022
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from the psalm/composer change

@skjnldsv skjnldsv requested a review from blizzz January 13, 2022 13:01
@skjnldsv
Copy link
Member

skjnldsv commented Jan 13, 2022

Please rebase so we can merge :)

come-nc and others added 4 commits January 13, 2022 14:06
@come-nc
Fix primary key change in user_ldap migration
e64e08e 
Use a backup table to copy the data, drop table and recreate it with
 correct primary key, then copy the data back and drop the backup table.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Split dropTable and createTable in two migrations
d4cabf8 
It is not possible to drop and create the same table in one migration

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @nickvergessen
Move duplicated code to a base class for group_mapping migrations
2384575 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
@come-nc
Fix user_ldap migration for long DNs support
7e4e919 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/user_ldap-fix-long-dn-migration branch from f567734 to 7e4e919 Compare January 13, 2022 13:07
@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

Rebased and removed the autoloader update, can someone which understand this stuff do the autoloader update?

@nickvergessen
Update autoloader
61c261b 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member

nickvergessen commented Jan 13, 2022 

bash build/autoloaderchecker.sh
git add apps/user_ldap/composer/composer/autoload_*
git commit
git restore apps/

Done

@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

/backport to stable23

@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

/backport to stable22

@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

/backport to stable21

@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

/backport to stable20

@come-nc
Copy link
Contributor Author

come-nc commented Jan 13, 2022

Drone failure is unrelated

@come-nc
Remove useless indexes with duplicated names on backup table
843b20c 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@blizzz
Copy link
Member

blizzz commented Jan 13, 2022

With the latest commit, migration run through like a hot knife through butter! 🏅 @come-nc

@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 13, 2022
@skjnldsv skjnldsv merged commit 4adb606 into master Jan 14, 2022
@skjnldsv skjnldsv deleted the fix/user_ldap-fix-long-dn-migration branch January 14, 2022 08:20
This was referenced Jan 14, 2022
Merged
Merged
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Jan 19, 2022
Closed
@joshtrichards joshtrichards mentioned this pull request Sep 10, 2023
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@blizzz blizzz blizzz approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@miaulalala miaulalala Awaiting requested review from miaulalala

Assignees

@come-nc come-nc

Labels

4. to release Ready to be released and/or waiting for tests to finish

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[23.0.1RC1, 22.2.4RC1] DB migration issue with user_ldap with postgres

4 participants

@come-nc @nickvergessen @skjnldsv @blizzz