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

Prevent NPE on commenting on lines with invalidated comments (with migration) #12549

Merged

Conversation

zeripath
Copy link
Contributor

#12239 reports an NPE when viewing the diff page of a PR when comments are made on a line that has previously had comments invalidated on it.

@mrsdizzie discovered the mechanism and reason this occurs.

This PR fixes the above NPE by preventing a review being assigned to 0 by only checking for a review if we are replying to a previous review and migrates all broken reviews to create a review and code comment for them.

Fix #12239
Closes #12548

Signed-off-by: Andrew Thornton [email protected]

Only check for a review if we are replying to a previous review.

Prevent the NPE in go-gitea#12239 by assuming that a comment without a Review is
non-pending.

Fix go-gitea#12239

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added type/bug issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP labels Aug 20, 2020
@zeripath zeripath added this to the 1.13.0 milestone Aug 20, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2020

Codecov Report

Merging #12549 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12549      +/-   ##
==========================================
- Coverage   43.48%   43.44%   -0.05%     
==========================================
  Files         642      643       +1     
  Lines       71040    71129      +89     
==========================================
+ Hits        30893    30900       +7     
- Misses      35136    35222      +86     
+ Partials     5011     5007       -4     
Impacted Files Coverage Δ
models/migrations/migrations.go 4.66% <ø> (ø)
models/migrations/v147.go 0.00% <0.00%> (ø)
services/pull/review.go 47.74% <0.00%> (ø)
modules/indexer/stats/queue.go 52.94% <0.00%> (-23.53%) ⬇️
modules/indexer/stats/db.go 43.47% <0.00%> (-8.70%) ⬇️
modules/queue/workerpool.go 58.77% <0.00%> (-1.23%) ⬇️
models/gpg_key.go 55.17% <0.00%> (+0.59%) ⬆️
services/pull/pull.go 42.26% <0.00%> (+0.69%) ⬆️
modules/queue/unique_queue_disk_channel.go 55.38% <0.00%> (+1.53%) ⬆️
... and 3 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 c6943cc...b5e9c48. 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 20, 2020
@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 Aug 20, 2020
@zeripath
Copy link
Contributor Author

Use #12548 as basis for backport.

lafriks pushed a commit that referenced this pull request Aug 21, 2020
…#12550)

* Prevent NPE on commenting on lines with invalidated comments

Only check for a review if we are replying to a previous review.

Prevent the NPE in #12239 by assuming that a comment without a Review is
non-pending.

Fix #12239

Signed-off-by: Andrew Thornton <[email protected]>

* Add hack around to show the broken comments

Signed-off-by: Andrew Thornton <[email protected]>
@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 Aug 21, 2020
@lafriks lafriks merged commit 9c9c334 into go-gitea:master Aug 21, 2020
@zeripath zeripath deleted the fix-12239-review-page-500-with-migration branch August 21, 2020 08:28
@zeripath zeripath added the backport/done All backports for this PR have been created label Aug 22, 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.
Labels
backport/done All backports for this PR have been created issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP 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.

Locked out of the ability to comment on the code of a PR sometimes
5 participants