From 8caea1062c45aeea1f2a54da623f3596b7dccda5 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Sun, 19 Oct 2025 08:13:57 +0200 Subject: [PATCH] fix: replace buggy limit/offset pagination in DoctorUserStarNum (#9752) Unsafe pagination with LIMIT/OFFSET and no ordering in `DoctorUserStarNum`, which is used during the `doctor check --run recalculate-stars-number --fix` command. Replaced with a single `UPDATE` statement to perform the work in bulk, rather than user-by-user. Marking this as "Bug / Confirmed" based upon the theoretical limit/offset risks, not reproduced risks. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests `TestDoctorUserStarNum` is slightly tested by `TestDoctorUserStarNum`, verifying it runs with no errors. I've also performed manual testing, corrupting my `user.num_stars` and then using the doctor to recalc it. Further automated tests probably aren't justified because tracking the number of stars a user has outstanding doesn't seem like important functionality anyway... it's just part of a few API responses on a user. ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9752 Reviewed-by: Earl Warren Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- models/repo.go | 34 +--------------------------------- 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/models/repo.go b/models/repo.go index 6f7ae25615..da3d33cf4a 100644 --- a/models/repo.go +++ b/models/repo.go @@ -292,42 +292,10 @@ func UpdateRepoStats(ctx context.Context, id int64) error { return nil } -func updateUserStarNumbers(ctx context.Context, users []user_model.User) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - for _, user := range users { - if _, err = db.Exec(ctx, "UPDATE `user` SET num_stars=(SELECT COUNT(*) FROM `star` WHERE uid=?) WHERE id=?", user.ID, user.ID); err != nil { - return err - } - } - - return committer.Commit() -} - // DoctorUserStarNum recalculate Stars number for all user func DoctorUserStarNum(ctx context.Context) (err error) { - const batchSize = 100 - - for start := 0; ; start += batchSize { - users := make([]user_model.User, 0, batchSize) - if err = db.GetEngine(ctx).Limit(batchSize, start).Where("type = ?", 0).Cols("id").Find(&users); err != nil { - return err - } - if len(users) == 0 { - break - } - - if err = updateUserStarNumbers(ctx, users); err != nil { - return err - } - } - + _, err = db.Exec(ctx, "UPDATE `user` SET num_stars=(SELECT COUNT(*) FROM `star` WHERE uid=`user`.id) WHERE type = 0") log.Debug("recalculate Stars number for all user finished") - return err }