Skip to content

Fix comment broken issue ref dependence #12651

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
Merged
merged 17 commits into from
Sep 4, 2020
Merged

Fix comment broken issue ref dependence #12651

merged 17 commits into from
Sep 4, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Aug 30, 2020

To reproduce the bug:

  1. create repo A
  2. creat issue "Title123" on A
  3. create repo B
  4. create issue "OMG" on B
  5. add issue "Title123" as dependency to "OMG"
  6. delete repo A

-> visit issue "OMG" -> 500

source: https://codeberg.org/Codeberg/Community/issues/242
note: need backport

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2020 •
edited
Loading

Codecov Report

Merging #12651 into master will increase coverage by 0.01%.
The diff coverage is 11.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12651      +/-   ##
==========================================
+ Coverage   43.29%   43.30%   +0.01%     
==========================================
  Files         646      646              
  Lines       71592    71567      -25     
==========================================
- Hits        30993    30991       -2     
+ Misses      35583    35557      -26     
- Partials     5016     5019       +3
Impacted Files Coverage Δ
models/migrations/migrations.go 4.66% <ø> (ø)
models/migrations/v148.go 0.00% <0.00%> (ø)
models/user.go 53.56% <ø> (-0.20%) ⬇️
modules/graceful/server.go 47.00% <0.00%> (-0.41%) ⬇️
modules/highlight/highlight.go 24.69% <0.00%> (ø)
modules/migrations/gitlab.go 1.04% <0.00%> (-0.10%) ⬇️
modules/session/virtual.go 60.20% <0.00%> (ø)
routers/repo/issue.go 37.49% <0.00%> (-0.06%) ⬇️
modules/migrations/base/downloader.go 17.64% <5.12%> (-5.72%) ⬇️
models/issue.go 56.86% <33.33%> (-0.07%) ⬇️
... and 17 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 42a5e39...3f6b108. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 30, 2020
@lafriks
Copy link
Member

lafriks commented Aug 30, 2020

We should probably need migration that deletes all bad data

@lafriks lafriks added this to the 1.13.0 milestone Aug 30, 2020
@6543
Copy link
Member Author

6543 commented Aug 31, 2020 •
edited
Loading

@lafriks I think this is more a cleanup task for doctor as with 7c7eb9e it wont do any harm anymore

@lafriks
Copy link
Member

lafriks commented Aug 31, 2020

might be but we usually did also migration in such cases

@6543
Copy link
Member Author

6543 commented Aug 31, 2020

@lafriks Done

@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 Sep 1, 2020
@6543 6543 mentioned this pull request Sep 2, 2020
Merged
@6543 6543 mentioned this pull request Sep 2, 2020
Merged
techknowlogick
techknowlogick requested changes Sep 2, 2020
View reviewed changes
models/migrations/v148.go Outdated
)

func purgeInvalidDependenciesComments(x *xorm.Engine) error {
_, err := x.Exec("DELETE FROM comment WHERE dependent_issue_id NOT IN (SELECT id FROM issue)")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this query for a few reasons
1, raw sql, when we could probably leverage xorm
2, IIRC sqlite may fail if select id from issue too large due to "too many arguments"
3, default value of dependant_issue_id is 0, and this could end up deleting many comments as issue_id 0 doesn't exist

Perhaps this could be broken up into batches of queries of issues where dependant_issue_id != 0, this would increase the number of queries run, but it likely would lead to a safer migration

Copy link
Member Author

Choose a reason for hiding this comment

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

we may want to have a look at this too:
https://github.com/go-gitea/gitea/blob/master/models/migrations/v129.go

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the SQLite too many arguments issue is not the case here. That occurs if you do the selects in two stages and try to use "... IN (?)", []string{....})

Probably could use xorm here though although it would require two fake struct definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on the 0 dependent_issue_id though!

Copy link
Member Author

Choose a reason for hiding this comment

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

think 1. & 2. can be considered as "resolved"

to 3. I'm happy if somebody can translate this query into xorm (/-builder), I had a try and it didnt worked out jet

lunny pushed a commit that referenced this pull request Sep 3, 2020
@6543
[Backport] Fix comment broken issue ref dependence (#12651) (#12692)
0fa538e 
* deleteIssuesByRepoID: delete related CommentTypeRemoveDependency & CommentTypeAddDependency comments too

* Ignore ErrIssueNotExist on comment.LoadDepIssueDetails()

* CI.restart()
6543
6543 commented Sep 3, 2020
View reviewed changes
@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 Sep 3, 2020
lunny
lunny reviewed Sep 3, 2020
View reviewed changes
@6543
Copy link
Member Author

6543 commented Sep 4, 2020

@lunny done

@lunny lunny merged commit 6c5266c into go-gitea:master Sep 4, 2020
@lunny
Copy link
Member

lunny commented Sep 4, 2020

Please send back port to v1.12

@techknowlogick techknowlogick added the backport/done All backports for this PR have been created label Sep 4, 2020
@6543 6543 deleted the fix-comment-broken-issue-ref-dependence branch September 4, 2020 01:39
@6543
Copy link
Member Author

6543 commented Sep 4, 2020

( -> #12692 )

@zeripath zeripath changed the title [BugFix] Fix comment broken issue ref dependence Fix comment broken issue ref dependence Sep 4, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@zeripath zeripath zeripath approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
7 participants
@6543 @codecov-commenter @lafriks @lunny @techknowlogick @zeripath @GiteaBot