[PR #749] [MERGED] Fix FIXME and remove superfluous queries in models/org #15554

New Issue

GiteaMirror · 2025-11-02T11:49:17-06:00

GiteaMirror commented

2025-11-02 11:49:17 -06:00

📋 Pull Request Information

Original PR: https://github.com/go-gitea/gitea/pull/749
Author: @ethantkoenig
Created: 1/24/2017
Status: ✅ Merged
Merged: 1/25/2017
Merged by: @lunny

Base: master ← Head: fixme/org

📝 Commits (1)

dca2cc8 Fix FIXME and remove superfluous queries in models/org

📊 Changes

3 files changed (+101 additions, -76 deletions)

View changed files

📝 models/action.go (+5 -8)
📝 models/org.go (+69 -61)
📝 routers/user/home.go (+27 -7)

📄 Description

I realize this is long, but I want to give some context for my changes. Please bear with me, or jump to the TL;DR at the bottom if you're impatient.

This PR addresses the following FIXME in models/org.go, line 474-475 (an identical FIXME is also in models/action.go, line 661):

// FIXME: only need to get IDs here, not all fields of repository.
repos, _, err := org.GetUserRepositories(user.ID, 1, org.NumRepos)

org.GetUserRepositories(uid, ..) first computes the necessary repoIDs, then loads the repos with those repoIDs, then finally computes a count (the second return value, which is ignored above). It would therefore make sense to create a separate function for computing the repoIDs (say org.GetUserRepositoryIDs(uid, ..)) that both org.GetUserRepositories(..) and models/org.go, 474-475 can use.

There's more: of the several places where org.GetUserRepositories(..) is called, the count is ignored in all but one place (routers/user/home.go, line 355). So most of the time when org.GetUserRepositories(..) is called, the count is calculated (which involves a join), and then the count is immediately thrown away. I therefore decided to not have org.GetUserRepositories(..) compute the count, but instead move that logic to another function (say org.GetAccessibleReposCount(uid)) to avoid wasting work. routers/user/home.go, line 355 would then make two calls: one to org.GetUserRepositories(..) and another to org.GetAccessibleRepoCount(..).

This seems promising, but there's one problem with this approach: both computing the repoIDs (in org.GetUserRepositoryIDs(..), and by extension org.GetUserRepositories(..)) and computing the count (org.GetAccessibleRepoCount(..)) require determining which teams the specified user belongs to. In routers/user/home.go, line 355, where I actually care about the count, I will need to compute the teams that a user belongs to twice (once for org.GetUserRepositories(), another time for org.GetAccessibleRepoCount()). The same problem occurs in places where both org.GetUserRepositories() and org.GetUserMirrorRepositories()) are called, (e.g. routers/user/home.go, line 116).

Taking a step back, there are three functions

org.GetUserRepositoryIDs(uid, ..)
org.GetAccessibleRepositoryCount(uid)
org.GetUserMirrorRepositories(uid)

all of which require the same preprocessing, and we would like to avoid doing the preprocessing more than once if we call these functions multiple times with the same uid. The solution that occurred to me was to move these functions to a AccessibleRepoEnvironment interface. The implementation of the interface, accessibleRepoEnv, would store the results of the preprocessing (i.e. the teamIDs a particular user belongs to), and each function/method would just use those results instead of (re)computing them.

Under my changes, to call these functions/method, one can do the following

env, err := org.AccessibleRepoEnv(userID) // preprocessing done here
// check err
count, err := env.CountRepos() // org and uid not provided, they're specific to env
// check err
repos := env.Repos()
// check err
...

and no work will be wasted or duplicated 😃. The interface, or at least the idea behind, could also be used in other parts of the codebase.

TL;DR: I got rid of useless/duplicated queries by moving some functions to an interface.

I'm open to suggestions (particularly for naming) and would be happy to hear any feedback on this.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/go-gitea/gitea/pull/749 **Author:** [@ethantkoenig](https://github.com/ethantkoenig) **Created:** 1/24/2017 **Status:** ✅ Merged **Merged:** 1/25/2017 **Merged by:** [@lunny](https://github.com/lunny) **Base:** `master` ← **Head:** `fixme/org` --- ### 📝 Commits (1) - [`dca2cc8`](https://github.com/go-gitea/gitea/commit/dca2cc8a30b0348c0490c62d6aa8b32436690725) Fix FIXME and remove superfluous queries in models/org ### 📊 Changes **3 files changed** (+101 additions, -76 deletions) <details> <summary>View changed files</summary> 📝 `models/action.go` (+5 -8) 📝 `models/org.go` (+69 -61) 📝 `routers/user/home.go` (+27 -7) </details> ### 📄 Description I realize this is long, but I want to give some context for my changes. Please bear with me, or jump to the TL;DR at the bottom if you're impatient. This PR addresses the following `FIXME` in `models/org.go, line 474-475` (an identical `FIXME` is also in `models/action.go, line 661`): ``` // FIXME: only need to get IDs here, not all fields of repository. repos, _, err := org.GetUserRepositories(user.ID, 1, org.NumRepos) ``` `org.GetUserRepositories(uid, ..)` first computes the necessary repoIDs, then loads the repos with those repoIDs, then finally computes a count (the second return value, which is ignored above). It would therefore make sense to create a separate function for computing the repoIDs (say `org.GetUserRepositoryIDs(uid, ..)`) that both `org.GetUserRepositories(..)` and `models/org.go, 474-475` can use. There's more: of the several places where `org.GetUserRepositories(..)` is called, the count is ignored in all but one place (`routers/user/home.go, line 355`). So most of the time when `org.GetUserRepositories(..)` is called, the count is calculated (which involves a join), and then the count is immediately thrown away. I therefore decided to not have `org.GetUserRepositories(..)` compute the count, but instead move that logic to another function (say `org.GetAccessibleReposCount(uid)`) to avoid wasting work. `routers/user/home.go, line 355` would then make two calls: one to `org.GetUserRepositories(..)` and another to `org.GetAccessibleRepoCount(..)`. This seems promising, but there's one problem with this approach: both computing the repoIDs (in `org.GetUserRepositoryIDs(..)`, and by extension `org.GetUserRepositories(..)`) and computing the count (`org.GetAccessibleRepoCount(..)`) require determining which teams the specified user belongs to. In `routers/user/home.go, line 355`, where I actually care about the count, I will need to compute the teams that a user belongs to twice (once for `org.GetUserRepositories()`, another time for `org.GetAccessibleRepoCount()`). The same problem occurs in places where both `org.GetUserRepositories()` and `org.GetUserMirrorRepositories()`) are called, (e.g. `routers/user/home.go, line 116`). Taking a step back, there are three functions * `org.GetUserRepositoryIDs(uid, ..)` * `org.GetAccessibleRepositoryCount(uid)` * `org.GetUserMirrorRepositories(uid)` all of which require the same preprocessing, and we would like to avoid doing the preprocessing more than once if we call these functions multiple times with the same `uid`. The solution that occurred to me was to move these functions to a `AccessibleRepoEnvironment` interface. The implementation of the interface, `accessibleRepoEnv`, would store the results of the preprocessing (i.e. the teamIDs a particular user belongs to), and each function/method would just use those results instead of (re)computing them. Under my changes, to call these functions/method, one can do the following ``` env, err := org.AccessibleRepoEnv(userID) // preprocessing done here // check err count, err := env.CountRepos() // org and uid not provided, they're specific to env // check err repos := env.Repos() // check err ... ``` and no work will be wasted or duplicated :smiley:. The interface, or at least the idea behind, could also be used in other parts of the codebase. **TL;DR:** I got rid of useless/duplicated queries by moving some functions to an interface. I'm open to suggestions (particularly for naming) and would be happy to hear any feedback on this. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

GiteaMirror added the pull-request label 2025-11-02 11:49:17 -06:00

GiteaMirror closed this issue

2025-11-02 11:49:17 -06:00

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/gitea#15554