From a3c6c78e084856ea8543ee3ae3df4dfb4f571739 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 31 Oct 2025 23:50:05 +0100 Subject: [PATCH] fix: reduce deadlocks merging PRs by using caching for repo issue count stats (#9922) The `repository` table has quite a few "count of related objects" fields on it, including the number of issues, closed issues, pull requests, and closed pull requests. These fields specifically will cause deadlocks during concurrent PR merges as documented in #9785. These fields are not used in database queries. In order to eliminate the deadlock possibility on them, I've moved them to be calculated on-demand with caching, with the cache being invalidated in the same places that the recalc used to be triggered. I've supplemented the already in-place automated testing with manual testing performing simple close & reopen of issues & PRs, and the counts which are used in the tabs at the top of the repo page are updated correctly as expected. Near future work: - Similar change can probably be performed to fix #9846 - Last known deadlock identified from #9785; I'm hoping to incorporate the synthetic deadlock test in a near future PR to prevent regressions ## 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 - Tests were already in-place covering these fields; they've been adjusted from using the fields to the new accessor methods. - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### 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. ## Release notes - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/9922): reduce deadlocks merging PRs by using caching for repo issue count stats Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9922 Reviewed-by: Gusted Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- .deadcode-out | 1 - models/fixtures/repository.yml | 237 ------------------ .../v14a_rm-repository-issue-stat-fields.go | 49 ++++ models/repo.go | 24 -- models/repo/TestGetUserForkLax/repository.yml | 4 - .../repository.yml | 8 - .../repository.yml | 4 - models/repo/repo.go | 109 +++++--- models/unittest/consistency.go | 16 -- modules/cache/cache.go | 6 +- routers/web/repo/badges/badges.go | 12 +- routers/web/repo/card.go | 8 +- routers/web/user/home_test.go | 2 +- services/convert/repository.go | 4 +- templates/repo/header.tmpl | 8 +- tests/e2e/fixtures/repository.yml | 1 - tests/integration/api_issue_test.go | 9 +- .../TestAdminDeleteUser/repository.yml | 4 - .../repository.yml | 8 - .../repository.yml | 4 - tests/integration/migrate_test.go | 2 +- tests/mysql.ini.tmpl | 4 + tests/pgsql.ini.tmpl | 4 + tests/sqlite.ini.tmpl | 4 + 24 files changed, 163 insertions(+), 369 deletions(-) create mode 100644 models/forgejo_migrations/v14a_rm-repository-issue-stat-fields.go diff --git a/.deadcode-out b/.deadcode-out index 6b6de32d17..b2db98c36b 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -80,7 +80,6 @@ forgejo.org/modules/base SetupGiteaRoot forgejo.org/modules/cache - GetInt WithNoCacheContext RemoveContextData diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index f537ee02d5..d3214f39f8 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -9,10 +9,6 @@ num_watches: 4 num_stars: 0 num_forks: 0 - num_issues: 2 - num_closed_issues: 1 - num_pulls: 3 - num_closed_pulls: 0 num_milestones: 3 num_closed_milestones: 1 num_projects: 1 @@ -43,10 +39,6 @@ num_watches: 1 num_stars: 1 num_forks: 0 - num_issues: 2 - num_closed_issues: 1 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -74,10 +66,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 1 @@ -108,10 +96,6 @@ num_watches: 0 num_stars: 1 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -140,10 +124,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -173,10 +153,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -206,10 +182,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -239,10 +211,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -272,10 +240,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -304,10 +268,6 @@ num_watches: 0 num_stars: 0 num_forks: 1 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 1 num_closed_milestones: 0 num_projects: 0 @@ -336,10 +296,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -367,10 +323,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -398,10 +350,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -430,10 +378,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -462,10 +406,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -494,10 +434,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -525,10 +461,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -556,10 +488,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -587,10 +515,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -618,10 +542,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -649,10 +569,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -680,10 +596,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -711,10 +623,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -742,10 +650,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -773,10 +677,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -804,10 +704,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -835,10 +731,6 @@ num_watches: 0 num_stars: 0 num_forks: 1 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -866,10 +758,6 @@ num_watches: 0 num_stars: 0 num_forks: 1 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -897,10 +785,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -928,10 +812,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -960,10 +840,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -991,10 +867,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 2 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1025,10 +897,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1056,10 +924,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1087,10 +951,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1119,10 +979,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1151,10 +1007,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1183,10 +1035,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1215,10 +1063,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1247,10 +1091,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1279,10 +1119,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1311,10 +1147,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 1 num_closed_milestones: 0 num_projects: 0 @@ -1342,10 +1174,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1374,10 +1202,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1405,10 +1229,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1437,10 +1257,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1469,10 +1285,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1501,10 +1313,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1534,10 +1342,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1566,10 +1370,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1598,10 +1398,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1630,10 +1426,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1662,10 +1454,6 @@ is_archived: false is_empty: false is_private: false - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_watches: 0 @@ -1703,7 +1491,6 @@ is_empty: false is_archived: false is_private: true - num_issues: 1 status: 0 topics: '[]' @@ -1718,7 +1505,6 @@ is_archived: false is_private: true status: 0 - num_issues: 0 topics: '[]' - @@ -1732,7 +1518,6 @@ is_archived: false is_private: false status: 0 - num_issues: 0 topics: '[]' - @@ -1745,10 +1530,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1778,7 +1559,6 @@ is_archived: false is_private: false status: 0 - num_issues: 0 topics: '[]' - @@ -1792,7 +1572,6 @@ is_archived: false is_private: true status: 0 - num_issues: 0 topics: '[]' - @@ -1805,10 +1584,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1837,10 +1612,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 1 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1868,10 +1639,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -1900,10 +1667,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 diff --git a/models/forgejo_migrations/v14a_rm-repository-issue-stat-fields.go b/models/forgejo_migrations/v14a_rm-repository-issue-stat-fields.go new file mode 100644 index 0000000000..72442fc387 --- /dev/null +++ b/models/forgejo_migrations/v14a_rm-repository-issue-stat-fields.go @@ -0,0 +1,49 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "fmt" + + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +func init() { + registerMigration(&Migration{ + Description: "remove from table repository fields num_issues, num_closed_issues, num_pulls, num_closed_pulls", + Upgrade: removeRepositoryIssueStatFields, + }) +} + +func removeRepositoryIssueStatFields(x *xorm.Engine) error { + switch x.Dialect().URI().DBType { + case schemas.SQLITE: + // Can't drop multiple columns in one statement in SQLite. + _, err := x.Exec("ALTER TABLE `repository` DROP COLUMN num_issues") + if err != nil { + return err + } + _, err = x.Exec("ALTER TABLE `repository` DROP COLUMN num_closed_issues") + if err != nil { + return err + } + _, err = x.Exec("ALTER TABLE `repository` DROP COLUMN num_pulls") + if err != nil { + return err + } + _, err = x.Exec("ALTER TABLE `repository` DROP COLUMN num_closed_pulls") + if err != nil { + return err + } + case schemas.MYSQL, schemas.POSTGRES: + _, err := x.Exec("ALTER TABLE `repository` DROP COLUMN num_issues, DROP COLUMN num_closed_issues, DROP COLUMN num_pulls, DROP COLUMN num_closed_pulls") + if err != nil { + return err + } + default: + return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType) + } + return nil +} diff --git a/models/repo.go b/models/repo.go index d25e4180a0..9a1b318a36 100644 --- a/models/repo.go +++ b/models/repo.go @@ -155,30 +155,6 @@ func CheckRepoStats(ctx context.Context) error { repoStatsCorrectNumStars, "repository count 'num_stars'", }, - // Repository.NumIssues - { - statsQuery("SELECT repo.id FROM `repository` repo WHERE repo.num_issues!=(SELECT COUNT(*) FROM `issue` WHERE repo_id=repo.id AND is_pull=?)", false), - repoStatsCorrectNumIssues, - "repository count 'num_issues'", - }, - // Repository.NumClosedIssues - { - statsQuery("SELECT repo.id FROM `repository` repo WHERE repo.num_closed_issues!=(SELECT COUNT(*) FROM `issue` WHERE repo_id=repo.id AND is_closed=? AND is_pull=?)", true, false), - repoStatsCorrectNumClosedIssues, - "repository count 'num_closed_issues'", - }, - // Repository.NumPulls - { - statsQuery("SELECT repo.id FROM `repository` repo WHERE repo.num_pulls!=(SELECT COUNT(*) FROM `issue` WHERE repo_id=repo.id AND is_pull=?)", true), - repoStatsCorrectNumPulls, - "repository count 'num_pulls'", - }, - // Repository.NumClosedPulls - { - statsQuery("SELECT repo.id FROM `repository` repo WHERE repo.num_closed_pulls!=(SELECT COUNT(*) FROM `issue` WHERE repo_id=repo.id AND is_closed=? AND is_pull=?)", true, true), - repoStatsCorrectNumClosedPulls, - "repository count 'num_closed_pulls'", - }, // Label.NumIssues { statsQuery("SELECT label.id FROM `label` WHERE label.num_issues!=(SELECT COUNT(*) FROM `issue_label` WHERE label_id=label.id)"), diff --git a/models/repo/TestGetUserForkLax/repository.yml b/models/repo/TestGetUserForkLax/repository.yml index c91ba18678..556804af37 100644 --- a/models/repo/TestGetUserForkLax/repository.yml +++ b/models/repo/TestGetUserForkLax/repository.yml @@ -8,10 +8,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 diff --git a/models/repo/TestGetUserForkLaxWithTwoChoices/repository.yml b/models/repo/TestGetUserForkLaxWithTwoChoices/repository.yml index 859e76db88..b94b1488e3 100644 --- a/models/repo/TestGetUserForkLaxWithTwoChoices/repository.yml +++ b/models/repo/TestGetUserForkLaxWithTwoChoices/repository.yml @@ -8,10 +8,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 @@ -39,10 +35,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 diff --git a/models/repo/TestSearchRepositoryIDsByCondition/repository.yml b/models/repo/TestSearchRepositoryIDsByCondition/repository.yml index b10fbc9226..e83300c2a0 100644 --- a/models/repo/TestSearchRepositoryIDsByCondition/repository.yml +++ b/models/repo/TestSearchRepositoryIDsByCondition/repository.yml @@ -8,10 +8,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 diff --git a/models/repo/repo.go b/models/repo/repo.go index 688c03b3d5..4ff3b22c8a 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -18,6 +18,7 @@ import ( "forgejo.org/models/db" "forgejo.org/models/unit" user_model "forgejo.org/models/user" + "forgejo.org/modules/cache" "forgejo.org/modules/git" "forgejo.org/modules/log" "forgejo.org/modules/markup" @@ -143,12 +144,6 @@ type Repository struct { NumWatches int NumStars int NumForks int - NumIssues int - NumClosedIssues int - NumOpenIssues int `xorm:"-"` - NumPulls int - NumClosedPulls int - NumOpenPulls int `xorm:"-"` NumMilestones int `xorm:"NOT NULL DEFAULT 0"` NumClosedMilestones int `xorm:"NOT NULL DEFAULT 0"` NumOpenMilestones int `xorm:"-"` @@ -294,8 +289,6 @@ func (repo *Repository) MarkAsBrokenEmpty() { // AfterLoad is invoked from XORM after setting the values of all fields of this object. func (repo *Repository) AfterLoad() { - repo.NumOpenIssues = repo.NumIssues - repo.NumClosedIssues - repo.NumOpenPulls = repo.NumPulls - repo.NumClosedPulls repo.NumOpenMilestones = repo.NumMilestones - repo.NumClosedMilestones repo.NumOpenProjects = repo.NumProjects - repo.NumClosedProjects repo.NumOpenActionRuns = repo.NumActionRuns - repo.NumClosedActionRuns @@ -917,32 +910,6 @@ func CountRepositories(ctx context.Context, opts CountRepositoryOptions) (int64, return count, nil } -// UpdateRepoIssueNumbers updates one of a repositories amount of (open|closed) (issues|PRs) with the current count -func UpdateRepoIssueNumbers(ctx context.Context, repoID int64, isPull, isClosed bool) error { - field := "num_" - if isClosed { - field += "closed_" - } - if isPull { - field += "pulls" - } else { - field += "issues" - } - - subQuery := builder.Select("count(*)"). - From("issue").Where(builder.Eq{ - "repo_id": repoID, - "is_pull": isPull, - }.And(builder.If(isClosed, builder.Eq{"is_closed": isClosed}))) - - // builder.Update(cond) will generate SQL like UPDATE ... SET cond - query := builder.Update(builder.Eq{field: subQuery}). - From("repository"). - Where(builder.Eq{"id": repoID}) - _, err := db.Exec(ctx, query) - return err -} - // CountNullArchivedRepository counts the number of repositories with is_archived is null func CountNullArchivedRepository(ctx context.Context) (int64, error) { return db.GetEngine(ctx).Where(builder.IsNull{"is_archived"}).Count(new(Repository)) @@ -962,3 +929,77 @@ func UpdateRepositoryOwnerName(ctx context.Context, oldUserName, newUserName str } return nil } + +type repoCacheKeyBase string + +const ( + countIssues = repoCacheKeyBase("CountIssues") + countIssuesClosed = repoCacheKeyBase("CountIssuesClosed") + countPulls = repoCacheKeyBase("CountPulls") + countPullsClosed = repoCacheKeyBase("CountPullsClosed") +) + +func repoCacheKey(cacheKeyBase repoCacheKeyBase, repoID int64) string { + return fmt.Sprintf("Repo:%s:%d", cacheKeyBase, repoID) +} + +func (repo *Repository) cacheIssueCount(ctx context.Context, cacheKeyBase repoCacheKeyBase, cond builder.Cond) int { + num, err := cache.GetInt(repoCacheKey(cacheKeyBase, repo.ID), func() (int, error) { + cond = builder.Eq{"repo_id": repo.ID}.And(cond) + count, err := db.GetEngine(ctx).Table("issue").Where(cond).Count() // can't use &issues.Issue{}; cyclical import + if err != nil { + return 0, fmt.Errorf("query error: %v", err) + } + return int(count), nil + }) + if err != nil { + log.Error("failed to retrieve NumIssues: %v", err) + return 0 + } + return num +} + +func (repo *Repository) NumIssues(ctx context.Context) int { + return repo.cacheIssueCount(ctx, countIssues, builder.Eq{"is_pull": false}) +} + +func (repo *Repository) NumClosedIssues(ctx context.Context) int { + return repo.cacheIssueCount(ctx, countIssuesClosed, builder.Eq{"is_pull": false, "is_closed": true}) +} + +func (repo *Repository) NumOpenIssues(ctx context.Context) int { + return repo.NumIssues(ctx) - repo.NumClosedIssues(ctx) +} + +func (repo *Repository) NumPulls(ctx context.Context) int { + return repo.cacheIssueCount(ctx, countPulls, builder.Eq{"is_pull": true}) +} + +func (repo *Repository) NumClosedPulls(ctx context.Context) int { + return repo.cacheIssueCount(ctx, countPullsClosed, builder.Eq{"is_pull": true, "is_closed": true}) +} + +func (repo *Repository) NumOpenPulls(ctx context.Context) int { + return repo.NumPulls(ctx) - repo.NumClosedPulls(ctx) +} + +// UpdateRepoIssueNumbers triggers a recalculation of the number of (open|closed) (issues|PRs) on a repo. It +// invalidates a cache which will cause this value to be calculated when accessed. +func UpdateRepoIssueNumbers(ctx context.Context, repoID int64, isPull, isClosed bool) error { + var cacheKeyBase repoCacheKeyBase + if isPull { + if isClosed { + cacheKeyBase = countPullsClosed + } else { + cacheKeyBase = countPulls + } + } else { + if isClosed { + cacheKeyBase = countIssuesClosed + } else { + cacheKeyBase = countIssues + } + } + cache.Remove(repoCacheKey(cacheKeyBase, repoID)) + return nil +} diff --git a/models/unittest/consistency.go b/models/unittest/consistency.go index e61c5a20f5..fbff51bf81 100644 --- a/models/unittest/consistency.go +++ b/models/unittest/consistency.go @@ -94,22 +94,6 @@ func init() { assert.EqualValues(t, repo.int("NumWatches"), actual, "Unexpected number of watches for repo id: %d", repo.int("ID")) - actual = GetCountByCond(t, "issue", builder.Eq{"is_pull": false, "repo_id": repo.int("ID")}) - assert.EqualValues(t, repo.int("NumIssues"), actual, - "Unexpected number of issues for repo id: %d", repo.int("ID")) - - actual = GetCountByCond(t, "issue", builder.Eq{"is_pull": false, "is_closed": true, "repo_id": repo.int("ID")}) - assert.EqualValues(t, repo.int("NumClosedIssues"), actual, - "Unexpected number of closed issues for repo id: %d", repo.int("ID")) - - actual = GetCountByCond(t, "issue", builder.Eq{"is_pull": true, "repo_id": repo.int("ID")}) - assert.EqualValues(t, repo.int("NumPulls"), actual, - "Unexpected number of pulls for repo id: %d", repo.int("ID")) - - actual = GetCountByCond(t, "issue", builder.Eq{"is_pull": true, "is_closed": true, "repo_id": repo.int("ID")}) - assert.EqualValues(t, repo.int("NumClosedPulls"), actual, - "Unexpected number of closed pulls for repo id: %d", repo.int("ID")) - actual = GetCountByCond(t, "milestone", builder.Eq{"is_closed": true, "repo_id": repo.int("ID")}) assert.EqualValues(t, repo.int("NumClosedMilestones"), actual, "Unexpected number of closed milestones for repo id: %d", repo.int("ID")) diff --git a/modules/cache/cache.go b/modules/cache/cache.go index aa69a0d3a7..9bf4e9a00e 100644 --- a/modules/cache/cache.go +++ b/modules/cache/cache.go @@ -80,7 +80,7 @@ func GetCache() mc.Cache { // GetString returns the key value from cache with callback when no key exists in cache func GetString(key string, getFunc func() (string, error)) (string, error) { - if conn == nil || setting.CacheService.TTL == 0 { + if conn == nil || setting.CacheService.TTL <= 0 { return getFunc() } @@ -107,7 +107,7 @@ func GetString(key string, getFunc func() (string, error)) (string, error) { // GetInt returns key value from cache with callback when no key exists in cache func GetInt(key string, getFunc func() (int, error)) (int, error) { - if conn == nil || setting.CacheService.TTL == 0 { + if conn == nil || setting.CacheService.TTL <= 0 { return getFunc() } @@ -142,7 +142,7 @@ func GetInt(key string, getFunc func() (int, error)) (int, error) { // GetInt64 returns key value from cache with callback when no key exists in cache func GetInt64(key string, getFunc func() (int64, error)) (int64, error) { - if conn == nil || setting.CacheService.TTL == 0 { + if conn == nil || setting.CacheService.TTL <= 0 { return getFunc() } diff --git a/routers/web/repo/badges/badges.go b/routers/web/repo/badges/badges.go index e623a21fc0..8759541a91 100644 --- a/routers/web/repo/badges/badges.go +++ b/routers/web/repo/badges/badges.go @@ -118,27 +118,27 @@ func getPullBadge(ctx *context_module.Context, variant string, num int) { } func GetOpenIssuesBadge(ctx *context_module.Context) { - getIssueBadge(ctx, "open", ctx.Repo.Repository.NumOpenIssues) + getIssueBadge(ctx, "open", ctx.Repo.Repository.NumOpenIssues(ctx)) } func GetClosedIssuesBadge(ctx *context_module.Context) { - getIssueBadge(ctx, "closed", ctx.Repo.Repository.NumClosedIssues) + getIssueBadge(ctx, "closed", ctx.Repo.Repository.NumClosedIssues(ctx)) } func GetTotalIssuesBadge(ctx *context_module.Context) { - getIssueBadge(ctx, "", ctx.Repo.Repository.NumIssues) + getIssueBadge(ctx, "", ctx.Repo.Repository.NumIssues(ctx)) } func GetOpenPullsBadge(ctx *context_module.Context) { - getPullBadge(ctx, "open", ctx.Repo.Repository.NumOpenPulls) + getPullBadge(ctx, "open", ctx.Repo.Repository.NumOpenPulls(ctx)) } func GetClosedPullsBadge(ctx *context_module.Context) { - getPullBadge(ctx, "closed", ctx.Repo.Repository.NumClosedPulls) + getPullBadge(ctx, "closed", ctx.Repo.Repository.NumClosedPulls(ctx)) } func GetTotalPullsBadge(ctx *context_module.Context) { - getPullBadge(ctx, "", ctx.Repo.Repository.NumPulls) + getPullBadge(ctx, "", ctx.Repo.Repository.NumPulls(ctx)) } func GetStarsBadge(ctx *context_module.Context) { diff --git a/routers/web/repo/card.go b/routers/web/repo/card.go index b758daec62..a1f5061673 100644 --- a/routers/web/repo/card.go +++ b/routers/web/repo/card.go @@ -200,16 +200,16 @@ func drawRepoSummaryCard(ctx *context.Context, repo *repo_model.Repository) (*ca } issuesText := ctx.Locale.TrN( - repo.NumOpenIssues, + repo.NumOpenIssues(ctx), "repo.activity.title.issues_1", "repo.activity.title.issues_n", - repo.NumOpenIssues, + repo.NumOpenIssues(ctx), ) pullRequestsText := ctx.Locale.TrN( - repo.NumOpenPulls, + repo.NumOpenPulls(ctx), "repo.activity.title.prs_1", "repo.activity.title.prs_n", - repo.NumOpenPulls, + repo.NumOpenPulls(ctx), ) bottomCountText := fmt.Sprintf("%s • %s", issuesText, pullRequestsText) diff --git a/routers/web/user/home_test.go b/routers/web/user/home_test.go index f3a2f12ae6..d9f99bed05 100644 --- a/routers/web/user/home_test.go +++ b/routers/web/user/home_test.go @@ -37,7 +37,7 @@ func TestArchivedIssues(t *testing.T) { NumIssues := make(map[int64]int) for _, repo := range repos { IsArchived[repo.ID] = repo.IsArchived - NumIssues[repo.ID] = repo.NumIssues + NumIssues[repo.ID] = repo.NumIssues(t.Context()) } assert.False(t, IsArchived[50]) assert.Equal(t, 1, NumIssues[50]) diff --git a/services/convert/repository.go b/services/convert/repository.go index 1b0f46b3da..f3603ee7f1 100644 --- a/services/convert/repository.go +++ b/services/convert/repository.go @@ -204,8 +204,8 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR Stars: repo.NumStars, Forks: repo.NumForks, Watchers: repo.NumWatches, - OpenIssues: repo.NumOpenIssues, - OpenPulls: repo.NumOpenPulls, + OpenIssues: repo.NumOpenIssues(ctx), + OpenPulls: repo.NumOpenPulls(ctx), Releases: int(numReleases), DefaultBranch: repo.DefaultBranch, Created: repo.CreatedUnix.AsTime(), diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index e7e9757f7d..ac57d98064 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -99,8 +99,8 @@ {{if .Permission.CanRead $.UnitTypeIssues}} {{svg "octicon-issue-opened"}} {{ctx.Locale.Tr "repo.issues"}} - {{if .Repository.NumOpenIssues}} - {{CountFmt .Repository.NumOpenIssues}} + {{if (.Repository.NumOpenIssues ctx)}} + {{CountFmt (.Repository.NumOpenIssues ctx)}} {{end}} {{end}} @@ -114,8 +114,8 @@ {{if and .Repository.CanEnablePulls (.Permission.CanRead $.UnitTypePullRequests)}} {{svg "octicon-git-pull-request"}} {{ctx.Locale.Tr "repo.pulls"}} - {{if .Repository.NumOpenPulls}} - {{CountFmt .Repository.NumOpenPulls}} + {{if (.Repository.NumOpenPulls ctx)}} + {{CountFmt (.Repository.NumOpenPulls ctx)}} {{end}} {{end}} diff --git a/tests/e2e/fixtures/repository.yml b/tests/e2e/fixtures/repository.yml index 153e6e7752..809a6877fa 100644 --- a/tests/e2e/fixtures/repository.yml +++ b/tests/e2e/fixtures/repository.yml @@ -24,5 +24,4 @@ is_archived: false is_private: false status: 0 - num_issues: 0 topics: '[]' diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index dc369e93b9..797d315f27 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -159,6 +159,8 @@ func TestAPICreateIssue(t *testing.T) { repoBefore := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repoBefore.OwnerID}) + beforeNumIssues := repoBefore.NumIssues(t.Context()) + beforeNumClosedIssues := repoBefore.NumClosedIssues(t.Context()) session := loginUser(t, owner.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) @@ -182,8 +184,8 @@ func TestAPICreateIssue(t *testing.T) { }) repoAfter := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - assert.Equal(t, repoBefore.NumIssues+1, repoAfter.NumIssues) - assert.Equal(t, repoBefore.NumClosedIssues, repoAfter.NumClosedIssues) + assert.Equal(t, beforeNumIssues+1, repoAfter.NumIssues(t.Context())) + assert.Equal(t, beforeNumClosedIssues, repoAfter.NumClosedIssues(t.Context())) } func TestAPICreateIssueParallel(t *testing.T) { @@ -238,6 +240,7 @@ func TestAPIEditIssue(t *testing.T) { require.NoError(t, issueBefore.LoadAttributes(db.DefaultContext)) assert.Equal(t, int64(1019307200), int64(issueBefore.DeadlineUnix)) assert.Equal(t, api.StateOpen, issueBefore.State()) + beforeNumClosedIssues := repoBefore.NumClosedIssues(t.Context()) session := loginUser(t, owner.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) @@ -278,7 +281,7 @@ func TestAPIEditIssue(t *testing.T) { assert.Equal(t, int64(-1), apiIssue.Poster.ID) // check repo change - assert.Equal(t, repoBefore.NumClosedIssues+1, repoAfter.NumClosedIssues) + assert.Equal(t, beforeNumClosedIssues+1, repoAfter.NumClosedIssues(t.Context())) // API response assert.Equal(t, api.StateClosed, apiIssue.State) diff --git a/tests/integration/fixtures/TestAdminDeleteUser/repository.yml b/tests/integration/fixtures/TestAdminDeleteUser/repository.yml index d71232d834..d411358847 100644 --- a/tests/integration/fixtures/TestAdminDeleteUser/repository.yml +++ b/tests/integration/fixtures/TestAdminDeleteUser/repository.yml @@ -8,10 +8,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 diff --git a/tests/integration/fixtures/TestAdminModerationViewReports/repository.yml b/tests/integration/fixtures/TestAdminModerationViewReports/repository.yml index cee5d6a74c..dfeecbec0d 100644 --- a/tests/integration/fixtures/TestAdminModerationViewReports/repository.yml +++ b/tests/integration/fixtures/TestAdminModerationViewReports/repository.yml @@ -10,10 +10,6 @@ num_watches: 1 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 is_private: false is_empty: false is_archived: false @@ -33,10 +29,6 @@ num_watches: 1 num_stars: 0 num_forks: 0 - num_issues: 1 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 is_private: false is_empty: false is_archived: false diff --git a/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml b/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml index ea76a569ea..68ceca921e 100644 --- a/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml +++ b/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml @@ -9,10 +9,6 @@ num_watches: 0 num_stars: 0 num_forks: 0 - num_issues: 0 - num_closed_issues: 0 - num_pulls: 0 - num_closed_pulls: 0 num_milestones: 0 num_closed_milestones: 0 num_projects: 0 diff --git a/tests/integration/migrate_test.go b/tests/integration/migrate_test.go index 5165986eba..7d244ce60f 100644 --- a/tests/integration/migrate_test.go +++ b/tests/integration/migrate_test.go @@ -140,7 +140,7 @@ func TestMigrateWithIssueComments(t *testing.T) { } repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: repoOwner.ID, Name: repoName}) - assert.Equal(t, 1, repo.NumIssues) + assert.Equal(t, 1, repo.NumIssues(t.Context())) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: repo.ID, Index: 1}) assert.Equal(t, numComments, issue.NumComments) diff --git a/tests/mysql.ini.tmpl b/tests/mysql.ini.tmpl index 28065ebf23..4678cf5087 100644 --- a/tests/mysql.ini.tmpl +++ b/tests/mysql.ini.tmpl @@ -136,3 +136,7 @@ FILE_EXTENSIONS = .iframehtml RENDER_COMMAND = cat RENDER_CONTENT_MODE = iframe NEED_POSTPROCESS = false + +[cache] +# Disable caching so that data isn't kept in-memory between test cases +ITEM_TTL = -1 diff --git a/tests/pgsql.ini.tmpl b/tests/pgsql.ini.tmpl index c17c89fcad..2e5dda5c59 100644 --- a/tests/pgsql.ini.tmpl +++ b/tests/pgsql.ini.tmpl @@ -150,3 +150,7 @@ FILE_EXTENSIONS = .iframehtml RENDER_COMMAND = cat RENDER_CONTENT_MODE = iframe NEED_POSTPROCESS = false + +[cache] +# Disable caching so that data isn't kept in-memory between test cases +ITEM_TTL = -1 diff --git a/tests/sqlite.ini.tmpl b/tests/sqlite.ini.tmpl index b275b8e017..10280cae19 100644 --- a/tests/sqlite.ini.tmpl +++ b/tests/sqlite.ini.tmpl @@ -137,3 +137,7 @@ FILE_EXTENSIONS = .iframehtml RENDER_COMMAND = cat RENDER_CONTENT_MODE = iframe NEED_POSTPROCESS = false + +[cache] +# Disable caching so that data isn't kept in-memory between test cases +ITEM_TTL = -1