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

Include OriginalAuthor in Reaction constraint #13505

Conversation

zeripath
Copy link
Contributor

When migrating repositories with reactions with deleted users, the original
author id may be -1. This means that it is possible to end up attempting
to create multiple reactions with the same [ Type, IssueID, CommentID, UserID,
OriginalAuthorID ] thus breaking the constraints.

On SQLite this appears to cause a deadlock but on other dbs this will
cause the migration to fail.

This PR extends the constraint to include the original author username
in the constraint.

Fix #13271

Signed-off-by: Andrew Thornton art27@cantab.net

When migrating repositories with reactions with deleted users, the original
author id may be -1. This means that it is possible to end up attempting
to create multiple reactions with the same [ Type, IssueID, CommentID, UserID,
OriginalAuthorID ] thus breaking the constraints.

On SQLite this appears to cause a deadlock but on other dbs this will
cause the migration to fail.

This PR extends the constraint to include the original author username
in the constraint.

Fix go-gitea#13271

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added this to the 1.14.0 milestone Nov 10, 2020
@6543
Copy link
Member

6543 commented Nov 10, 2020

ok migration cant be backported ... I'll file a pull to drop entrys from API witch wont have any chance to get migrated to an existing user somehow

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 10, 2020
@zeripath
Copy link
Contributor Author

The backport would just have to drop the reactions if they have originalauthorid -1 I think

@zeripath
Copy link
Contributor Author

admittedly such a backport would not be a backport just an alternate fix but it's the only solution.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 10, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 10, 2020
zeripath and others added 3 commits November 10, 2020 22:02
Signed-off-by: Andrew Thornton <art27@cantab.net>
…' of github.com:zeripath/gitea into fix-13271-move-original-author-into-reaction-constraint
@zeripath
Copy link
Contributor Author

damn recreateTable has to be in a transaction for MSSQL - just put it in one

@codecov-io
Copy link

Codecov Report

Merging #13505 (ecac4cf) into master (3400928) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13505      +/-   ##
==========================================
- Coverage   42.21%   42.16%   -0.06%     
==========================================
  Files         694      695       +1     
  Lines       76351    76368      +17     
==========================================
- Hits        32231    32199      -32     
- Misses      38848    38886      +38     
- Partials     5272     5283      +11     
Impacted Files Coverage Δ
models/issue_reaction.go 82.38% <ø> (ø)
models/migrations/migrations.go 2.44% <ø> (ø)
models/migrations/v159.go 0.00% <0.00%> (ø)
models/repo_mirror.go 2.38% <0.00%> (-11.91%) ⬇️
modules/cron/tasks_basic.go 87.35% <0.00%> (-3.45%) ⬇️
services/pull/check.go 48.90% <0.00%> (-2.92%) ⬇️
modules/notification/ui/ui.go 93.05% <0.00%> (-2.78%) ⬇️
modules/process/manager.go 72.50% <0.00%> (-2.50%) ⬇️
modules/log/event.go 59.90% <0.00%> (-1.89%) ⬇️
services/mirror/mirror.go 15.98% <0.00%> (-1.17%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3400928...3f0fc41. Read the comment docs.

@gsantner
Copy link

potential issue from this -> #13588

@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Jan 4, 2021

No this is not the cause of your issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration: migrating migrated repo fail
7 participants