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.

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

Fix upgrade of user_ldap when oc_group_members contains duplicated uids #42576

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 4, 2024

Summary

When upgrading to 28, if oc_group_members contains duplicated uids, migration was failing on unicity constraint.
Also ignore DB errors for this migration step, as people which failed the upgrade before will have a non-empty table in oc_group_membership, and there is no data loss in this case since membership information is in the LDAP and will be fetched by the background job on the next run if incorrect or incomplete.

Checklist

@come-nc come-nc added the 3. to review Waiting for reviews label Jan 4, 2024
@come-nc come-nc added this to the Nextcloud 29 milestone Jan 4, 2024
@come-nc come-nc self-assigned this Jan 4, 2024
@come-nc
Copy link
Contributor Author

come-nc commented Jan 4, 2024

/backport to stable28

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/fix-ldap-upgrade-on-duplicate-membership branch from 654f862 to 6d0f0fc Compare January 4, 2024 10:51
@ArtificialOwl
Copy link
Member

should we check on $e->getReason() === DBException::REASON_UNIQUE_CONSTRAINT_VIOLATION ?

@come-nc
Copy link
Contributor Author

come-nc commented Jan 4, 2024

should we check on $e->getReason() === DBException::REASON_UNIQUE_CONSTRAINT_VIOLATION ?

No, see comment, even if it fails for another reason it’s better to ignore and keep going, the background job can fix the data later.

@come-nc come-nc merged commit 2ab852e into master Jan 4, 2024
@come-nc come-nc deleted the fix/fix-ldap-upgrade-on-duplicate-membership branch January 4, 2024 13:17
@schneidr
Copy link

schneidr commented Jan 4, 2024

Does the NextCloud 29 Milestone tag mean this will only be fixed in version 29, and everybody who wants to update from 27 to 28 and uses LDAP authentication needs to manually perform the suggested workaround?

@come-nc
Copy link
Contributor Author

come-nc commented Jan 4, 2024

Does the NextCloud 29 Milestone tag mean this will only be fixed in version 29, and everybody who wants to update from 27 to 28 needs to manually perform the suggested workaround?

No, it means this PR targets master branch which will be 29.
It will be backported to stable28 (in #42583 ) and included in 28.0.2.
And please capitalize as Nextcloud, not NextCloud.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Upgrade fails - app user_ldap update failed SQLSTATE[23000]: Integrity constraint violation
4 participants