Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix primary key change in user_ldap migration #30568

Merged
merged 6 commits into from
Jan 14, 2022
Merged

Fix primary key change in user_ldap migration #30568

merged 6 commits into from
Jan 14, 2022

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 added the 2. developing Work in progress label Jan 10, 2022
@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
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 11, 2022
nickvergessen
nickvergessen approved these changes Jan 11, 2022
View reviewed changes
apps/user_ldap/lib/Migration/GroupMappingMigration.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Migration/Version1130Date20211102154716.php Outdated
$table->setPrimaryKey(['owncloud_name']);
$changeSchema = true;
}

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

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

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

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

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 

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