Skip to content

Fix primary key change in user_ldap migration - #30568

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

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

Conversation

@come-nc

@come-nccome-nc commented Jan 10, 2022

Copy link
Copy Markdown
Contributor

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-nccome-nc self-assigned this Jan 10, 2022
@come-nccome-nc added the 2. developing Work in progress label Jan 10, 2022
blizzz
blizzz reviewed Jan 10, 2022
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/Version1130Date20211102154716.php Outdated
@come-nc
come-ncforce-pushed the fix/user_ldap-fix-long-dn-migration branch from 5d42916 to e2fc013CompareJanuary 10, 2022 13:03
blizzz
blizzz reviewed Jan 10, 2022
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/Version1130Date20220110154717.php Outdated
nickvergessen
nickvergessen requested changes Jan 10, 2022
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/Version1130Date20211102154716.php Outdated
Comment threadapps/user_ldap/lib/Migration/Version1130Date20220110154717.php Outdated
@come-nccome-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
Comment threadapps/user_ldap/lib/Migration/Version1130Date20220110154718.php Outdated
nickvergessen
nickvergessen approved these changes Jan 11, 2022
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/GroupMappingMigration.php Outdated
Comment threadapps/user_ldap/lib/Migration/Version1130Date20211102154716.php Outdated
$changeSchema = true;
}

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

@nickvergessennickvergessenJan 11, 2022

Copy link
Copy Markdown
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'

@come-nccome-ncJan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

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-ncforce-pushed the fix/user_ldap-fix-long-dn-migration branch from e7f9d75 to 9d11d71CompareJanuary 13, 2022 09:58
@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

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

nickvergessen commented Jan 13, 2022

Copy link
Copy Markdown
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

@nickvergessennickvergessen left a comment

Copy link
Copy Markdown
Member

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 blizzzJanuary 13, 2022 13:01
@skjnldsv

skjnldsv commented Jan 13, 2022

Copy link
Copy Markdown
Member

Please rebase so we can merge :)

come-ncand 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-ncforce-pushed the fix/user_ldap-fix-long-dn-migration branch from f567734 to 7e4e919CompareJanuary 13, 2022 13:07
@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

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

nickvergessen commented Jan 13, 2022

Copy link
Copy Markdown
Member
bash build/autoloaderchecker.sh
git add apps/user_ldap/composer/composer/autoload_*
git commit
git restore apps/

Done

@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable23

@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable22

@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable21

@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable20

@come-nc

come-nc commented Jan 13, 2022

Copy link
Copy Markdown
ContributorAuthor

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

blizzz commented Jan 13, 2022

Copy link
Copy Markdown
Member

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

@blizzzblizzz 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 masterJan 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
@skjnldsvskjnldsv mentioned this pull request Jan 19, 2022
Closed
@joshtrichardsjoshtrichards mentioned this pull request Sep 10, 2023
Open
9 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessennickvergessennickvergessen approved these changes
@blizzzblizzzblizzz approved these changes
@ChristophWurstChristophWurstAwaiting requested review from ChristophWurst
@miaulalalamiaulalalaAwaiting requested review from miaulalala

Assignees

@come-nccome-nc

Labels

4. to releaseReady 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