From 93a2def96b8172d111b00ad11c8f1de0872c45a3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 12 Apr 2025 11:22:18 -0700 Subject: [PATCH 1/6] Move and rename UpdateRepository (#34136) --- modules/repository/create.go | 102 ----------------- modules/repository/create_test.go | 21 ---- services/repository/adopt.go | 2 +- services/repository/create.go | 2 +- services/repository/fork.go | 2 +- services/repository/repository.go | 151 ++++++++++++++++++++----- services/repository/repository_test.go | 21 ++++ 7 files changed, 145 insertions(+), 156 deletions(-) diff --git a/modules/repository/create.go b/modules/repository/create.go index a53632bb57..a75598a84b 100644 --- a/modules/repository/create.go +++ b/modules/repository/create.go @@ -8,17 +8,9 @@ import ( "fmt" "os" "path/filepath" - "strings" - activities_model "code.gitea.io/gitea/models/activities" - "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" - access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" - issue_indexer "code.gitea.io/gitea/modules/indexer/issues" - "code.gitea.io/gitea/modules/log" - api "code.gitea.io/gitea/modules/structs" - "code.gitea.io/gitea/modules/util" ) const notRegularFileMode = os.ModeSymlink | os.ModeNamedPipe | os.ModeSocket | os.ModeDevice | os.ModeCharDevice | os.ModeIrregular @@ -63,97 +55,3 @@ func UpdateRepoSize(ctx context.Context, repo *repo_model.Repository) error { return repo_model.UpdateRepoSize(ctx, repo.ID, size, lfsSize) } - -// CheckDaemonExportOK creates/removes git-daemon-export-ok for git-daemon... -func CheckDaemonExportOK(ctx context.Context, repo *repo_model.Repository) error { - if err := repo.LoadOwner(ctx); err != nil { - return err - } - - // Create/Remove git-daemon-export-ok for git-daemon... - daemonExportFile := filepath.Join(repo.RepoPath(), `git-daemon-export-ok`) - - isExist, err := util.IsExist(daemonExportFile) - if err != nil { - log.Error("Unable to check if %s exists. Error: %v", daemonExportFile, err) - return err - } - - isPublic := !repo.IsPrivate && repo.Owner.Visibility == api.VisibleTypePublic - if !isPublic && isExist { - if err = util.Remove(daemonExportFile); err != nil { - log.Error("Failed to remove %s: %v", daemonExportFile, err) - } - } else if isPublic && !isExist { - if f, err := os.Create(daemonExportFile); err != nil { - log.Error("Failed to create %s: %v", daemonExportFile, err) - } else { - f.Close() - } - } - - return nil -} - -// UpdateRepository updates a repository with db context -func UpdateRepository(ctx context.Context, repo *repo_model.Repository, visibilityChanged bool) (err error) { - repo.LowerName = strings.ToLower(repo.Name) - - e := db.GetEngine(ctx) - - if _, err = e.ID(repo.ID).AllCols().Update(repo); err != nil { - return fmt.Errorf("update: %w", err) - } - - if err = UpdateRepoSize(ctx, repo); err != nil { - log.Error("Failed to update size for repository: %v", err) - } - - if visibilityChanged { - if err = repo.LoadOwner(ctx); err != nil { - return fmt.Errorf("LoadOwner: %w", err) - } - if repo.Owner.IsOrganization() { - // Organization repository need to recalculate access table when visibility is changed. - if err = access_model.RecalculateTeamAccesses(ctx, repo, 0); err != nil { - return fmt.Errorf("recalculateTeamAccesses: %w", err) - } - } - - // If repo has become private, we need to set its actions to private. - if repo.IsPrivate { - _, err = e.Where("repo_id = ?", repo.ID).Cols("is_private").Update(&activities_model.Action{ - IsPrivate: true, - }) - if err != nil { - return err - } - - if err = repo_model.ClearRepoStars(ctx, repo.ID); err != nil { - return err - } - } - - // Create/Remove git-daemon-export-ok for git-daemon... - if err := CheckDaemonExportOK(ctx, repo); err != nil { - return err - } - - forkRepos, err := repo_model.GetRepositoriesByForkID(ctx, repo.ID) - if err != nil { - return fmt.Errorf("getRepositoriesByForkID: %w", err) - } - for i := range forkRepos { - forkRepos[i].IsPrivate = repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate - if err = UpdateRepository(ctx, forkRepos[i], true); err != nil { - return fmt.Errorf("updateRepository[%d]: %w", forkRepos[i].ID, err) - } - } - - // If visibility is changed, we need to update the issue indexer. - // Since the data in the issue indexer have field to indicate if the repo is public or not. - issue_indexer.UpdateRepoIndexer(ctx, repo.ID) - } - - return nil -} diff --git a/modules/repository/create_test.go b/modules/repository/create_test.go index e1f981ba3c..b85a10adad 100644 --- a/modules/repository/create_test.go +++ b/modules/repository/create_test.go @@ -6,7 +6,6 @@ package repository import ( "testing" - activities_model "code.gitea.io/gitea/models/activities" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" @@ -14,26 +13,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestUpdateRepositoryVisibilityChanged(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - // Get sample repo and change visibility - repo, err := repo_model.GetRepositoryByID(db.DefaultContext, 9) - assert.NoError(t, err) - repo.IsPrivate = true - - // Update it - err = UpdateRepository(db.DefaultContext, repo, true) - assert.NoError(t, err) - - // Check visibility of action has become private - act := activities_model.Action{} - _, err = db.GetEngine(db.DefaultContext).ID(3).Get(&act) - - assert.NoError(t, err) - assert.True(t, act.IsPrivate) -} - func TestGetDirectorySize(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) repo, err := repo_model.GetRepositoryByID(db.DefaultContext, 1) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 6d5505c42c..7f1954145c 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -196,7 +196,7 @@ func adoptRepository(ctx context.Context, repo *repo_model.Repository, defaultBr return fmt.Errorf("setDefaultBranch: %w", err) } } - if err = repo_module.UpdateRepository(ctx, repo, false); err != nil { + if err = updateRepository(ctx, repo, false); err != nil { return fmt.Errorf("updateRepository: %w", err) } diff --git a/services/repository/create.go b/services/repository/create.go index 5a9b2ecd2a..897f2a1d14 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -464,7 +464,7 @@ func cleanupRepository(doer *user_model.User, repoID int64) { } func updateGitRepoAfterCreate(ctx context.Context, repo *repo_model.Repository) error { - if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + if err := checkDaemonExportOK(ctx, repo); err != nil { return fmt.Errorf("checkDaemonExportOK: %w", err) } diff --git a/services/repository/fork.go b/services/repository/fork.go index 1794cc18ab..c16c3d598a 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -227,7 +227,7 @@ func ConvertForkToNormalRepository(ctx context.Context, repo *repo_model.Reposit repo.IsFork = false repo.ForkID = 0 - if err := repo_module.UpdateRepository(ctx, repo, false); err != nil { + if err := updateRepository(ctx, repo, false); err != nil { log.Error("Unable to update repository %-v whilst converting from fork. Error: %v", repo, err) return err } diff --git a/services/repository/repository.go b/services/repository/repository.go index cd56010546..90aad4f9d8 100644 --- a/services/repository/repository.go +++ b/services/repository/repository.go @@ -7,20 +7,27 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" + "strings" + activities_model "code.gitea.io/gitea/models/activities" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" + access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/graceful" + issue_indexer "code.gitea.io/gitea/modules/indexer/issues" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/queue" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify" pull_service "code.gitea.io/gitea/services/pull" ) @@ -109,42 +116,32 @@ func Init(ctx context.Context) error { // UpdateRepository updates a repository func UpdateRepository(ctx context.Context, repo *repo_model.Repository, visibilityChanged bool) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - if err = repo_module.UpdateRepository(ctx, repo, visibilityChanged); err != nil { - return fmt.Errorf("updateRepository: %w", err) - } - - return committer.Commit() -} - -func UpdateRepositoryVisibility(ctx context.Context, repo *repo_model.Repository, isPrivate bool) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - - defer committer.Close() - - repo.IsPrivate = isPrivate - - if err = repo_module.UpdateRepository(ctx, repo, true); err != nil { - return fmt.Errorf("UpdateRepositoryVisibility: %w", err) - } - - return committer.Commit() + return db.WithTx(ctx, func(ctx context.Context) error { + if err = updateRepository(ctx, repo, visibilityChanged); err != nil { + return fmt.Errorf("updateRepository: %w", err) + } + return nil + }) } func MakeRepoPublic(ctx context.Context, repo *repo_model.Repository) (err error) { - return UpdateRepositoryVisibility(ctx, repo, false) + return db.WithTx(ctx, func(ctx context.Context) error { + repo.IsPrivate = false + if err = updateRepository(ctx, repo, true); err != nil { + return fmt.Errorf("MakeRepoPublic: %w", err) + } + return nil + }) } func MakeRepoPrivate(ctx context.Context, repo *repo_model.Repository) (err error) { - return UpdateRepositoryVisibility(ctx, repo, true) + return db.WithTx(ctx, func(ctx context.Context) error { + repo.IsPrivate = true + if err = updateRepository(ctx, repo, true); err != nil { + return fmt.Errorf("MakeRepoPrivate: %w", err) + } + return nil + }) } // LinkedRepository returns the linked repo if any @@ -170,3 +167,97 @@ func LinkedRepository(ctx context.Context, a *repo_model.Attachment) (*repo_mode } return nil, -1, nil } + +// checkDaemonExportOK creates/removes git-daemon-export-ok for git-daemon... +func checkDaemonExportOK(ctx context.Context, repo *repo_model.Repository) error { + if err := repo.LoadOwner(ctx); err != nil { + return err + } + + // Create/Remove git-daemon-export-ok for git-daemon... + daemonExportFile := filepath.Join(repo.RepoPath(), `git-daemon-export-ok`) + + isExist, err := util.IsExist(daemonExportFile) + if err != nil { + log.Error("Unable to check if %s exists. Error: %v", daemonExportFile, err) + return err + } + + isPublic := !repo.IsPrivate && repo.Owner.Visibility == structs.VisibleTypePublic + if !isPublic && isExist { + if err = util.Remove(daemonExportFile); err != nil { + log.Error("Failed to remove %s: %v", daemonExportFile, err) + } + } else if isPublic && !isExist { + if f, err := os.Create(daemonExportFile); err != nil { + log.Error("Failed to create %s: %v", daemonExportFile, err) + } else { + f.Close() + } + } + + return nil +} + +// updateRepository updates a repository with db context +func updateRepository(ctx context.Context, repo *repo_model.Repository, visibilityChanged bool) (err error) { + repo.LowerName = strings.ToLower(repo.Name) + + e := db.GetEngine(ctx) + + if _, err = e.ID(repo.ID).AllCols().Update(repo); err != nil { + return fmt.Errorf("update: %w", err) + } + + if err = repo_module.UpdateRepoSize(ctx, repo); err != nil { + log.Error("Failed to update size for repository: %v", err) + } + + if visibilityChanged { + if err = repo.LoadOwner(ctx); err != nil { + return fmt.Errorf("LoadOwner: %w", err) + } + if repo.Owner.IsOrganization() { + // Organization repository need to recalculate access table when visibility is changed. + if err = access_model.RecalculateTeamAccesses(ctx, repo, 0); err != nil { + return fmt.Errorf("recalculateTeamAccesses: %w", err) + } + } + + // If repo has become private, we need to set its actions to private. + if repo.IsPrivate { + _, err = e.Where("repo_id = ?", repo.ID).Cols("is_private").Update(&activities_model.Action{ + IsPrivate: true, + }) + if err != nil { + return err + } + + if err = repo_model.ClearRepoStars(ctx, repo.ID); err != nil { + return err + } + } + + // Create/Remove git-daemon-export-ok for git-daemon... + if err := checkDaemonExportOK(ctx, repo); err != nil { + return err + } + + forkRepos, err := repo_model.GetRepositoriesByForkID(ctx, repo.ID) + if err != nil { + return fmt.Errorf("getRepositoriesByForkID: %w", err) + } + for i := range forkRepos { + forkRepos[i].IsPrivate = repo.IsPrivate || repo.Owner.Visibility == structs.VisibleTypePrivate + if err = updateRepository(ctx, forkRepos[i], true); err != nil { + return fmt.Errorf("updateRepository[%d]: %w", forkRepos[i].ID, err) + } + } + + // If visibility is changed, we need to update the issue indexer. + // Since the data in the issue indexer have field to indicate if the repo is public or not. + issue_indexer.UpdateRepoIndexer(ctx, repo.ID) + } + + return nil +} diff --git a/services/repository/repository_test.go b/services/repository/repository_test.go index 892a11a23e..8f9fdf8fa1 100644 --- a/services/repository/repository_test.go +++ b/services/repository/repository_test.go @@ -6,6 +6,7 @@ package repository import ( "testing" + activities_model "code.gitea.io/gitea/models/activities" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -40,3 +41,23 @@ func TestLinkedRepository(t *testing.T) { }) } } + +func TestUpdateRepositoryVisibilityChanged(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // Get sample repo and change visibility + repo, err := repo_model.GetRepositoryByID(db.DefaultContext, 9) + assert.NoError(t, err) + repo.IsPrivate = true + + // Update it + err = updateRepository(db.DefaultContext, repo, true) + assert.NoError(t, err) + + // Check visibility of action has become private + act := activities_model.Action{} + _, err = db.GetEngine(db.DefaultContext).ID(3).Get(&act) + + assert.NoError(t, err) + assert.True(t, act.IsPrivate) +} From f4196a88432d983c337b24845301dcfd36f36feb Mon Sep 17 00:00:00 2001 From: Kerwin Bryant Date: Sun, 13 Apr 2025 09:59:36 +0800 Subject: [PATCH 2/6] Optimize overflow-menu (#34183) Optimized the overflow-menu: 1. Close the tippy when a menu item inside the tippy is clicked. 2. When a menu item inside the tippy is selected, move the active state of the menu to the tippy's button. --------- Co-authored-by: wxiaoguang --- web_src/css/base.css | 8 ++++ web_src/js/utils/dom.ts | 2 +- web_src/js/webcomponents/overflow-menu.ts | 45 ++++++++++++++++------- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/web_src/css/base.css b/web_src/css/base.css index 37ee7f5832..353ae851ad 100644 --- a/web_src/css/base.css +++ b/web_src/css/base.css @@ -747,6 +747,14 @@ overflow-menu .overflow-menu-button { padding: 0; } +/* match the styles of ".ui.secondary.pointing.menu .active.item" */ +overflow-menu.ui.secondary.pointing.menu .overflow-menu-button.active { + padding: 2px 0 0; + border-bottom: 2px solid currentcolor; + background-color: transparent; + font-weight: var(--font-weight-medium); +} + overflow-menu .overflow-menu-button:hover { color: var(--color-text-dark); } diff --git a/web_src/js/utils/dom.ts b/web_src/js/utils/dom.ts index b3debfde9e..98e5170a2b 100644 --- a/web_src/js/utils/dom.ts +++ b/web_src/js/utils/dom.ts @@ -360,7 +360,7 @@ export function querySingleVisibleElem(parent: Element, s export function addDelegatedEventListener(parent: Node, type: string, selector: string, listener: (elem: T, e: E) => Promisable, options?: boolean | AddEventListenerOptions) { parent.addEventListener(type, (e: Event) => { const elem = (e.target as HTMLElement).closest(selector); - if (!elem) return; + if (!elem || !parent.contains(elem)) return; listener(elem as T, e as E); }, options); } diff --git a/web_src/js/webcomponents/overflow-menu.ts b/web_src/js/webcomponents/overflow-menu.ts index 4e729a268a..ae93f2b758 100644 --- a/web_src/js/webcomponents/overflow-menu.ts +++ b/web_src/js/webcomponents/overflow-menu.ts @@ -1,6 +1,6 @@ import {throttle} from 'throttle-debounce'; import {createTippy} from '../modules/tippy.ts'; -import {isDocumentFragmentOrElementNode} from '../utils/dom.ts'; +import {addDelegatedEventListener, isDocumentFragmentOrElementNode} from '../utils/dom.ts'; import octiconKebabHorizontal from '../../../public/assets/img/svg/octicon-kebab-horizontal.svg'; window.customElements.define('overflow-menu', class extends HTMLElement { @@ -12,10 +12,14 @@ window.customElements.define('overflow-menu', class extends HTMLElement { mutationObserver: MutationObserver; lastWidth: number; + updateButtonActivationState() { + if (!this.button || !this.tippyContent) return; + this.button.classList.toggle('active', Boolean(this.tippyContent.querySelector('.item.active'))); + } + updateItems = throttle(100, () => { if (!this.tippyContent) { const div = document.createElement('div'); - div.classList.add('tippy-target'); div.tabIndex = -1; // for initial focus, programmatic focus only div.addEventListener('keydown', (e) => { if (e.key === 'Tab') { @@ -64,9 +68,10 @@ window.customElements.define('overflow-menu', class extends HTMLElement { } } }); - this.append(div); + div.classList.add('tippy-target'); + this.handleItemClick(div, '.tippy-target > .item'); this.tippyContent = div; - } + } // end if: no tippyContent and create a new one const itemFlexSpace = this.menuItemsEl.querySelector('.item-flex-space'); const itemOverFlowMenuButton = this.querySelector('.overflow-menu-button'); @@ -88,7 +93,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement { const menuRight = this.offsetLeft + this.offsetWidth; const menuItems = this.menuItemsEl.querySelectorAll('.item, .item-flex-space'); let afterFlexSpace = false; - for (const item of menuItems) { + for (const [idx, item] of menuItems.entries()) { if (item.classList.contains('item-flex-space')) { afterFlexSpace = true; continue; @@ -96,7 +101,10 @@ window.customElements.define('overflow-menu', class extends HTMLElement { if (afterFlexSpace) item.setAttribute('data-after-flex-space', 'true'); const itemRight = item.offsetLeft + item.offsetWidth; if (menuRight - itemRight < 38) { // roughly the width of .overflow-menu-button with some extra space - this.tippyItems.push(item); + const onlyLastItem = idx === menuItems.length - 1 && this.tippyItems.length === 0; + const lastItemFit = onlyLastItem && menuRight - itemRight > 0; + const moveToPopup = !onlyLastItem || !lastItemFit; + if (moveToPopup) this.tippyItems.push(item); } } itemFlexSpace?.style.removeProperty('display'); @@ -107,6 +115,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement { const btn = this.querySelector('.overflow-menu-button'); btn?._tippy?.destroy(); btn?.remove(); + this.button = null; return; } @@ -126,18 +135,17 @@ window.customElements.define('overflow-menu', class extends HTMLElement { // update existing tippy if (this.button?._tippy) { this.button._tippy.setContent(this.tippyContent); + this.updateButtonActivationState(); return; } // create button initially - const btn = document.createElement('button'); - btn.classList.add('overflow-menu-button'); - btn.setAttribute('aria-label', window.config.i18n.more_items); - btn.innerHTML = octiconKebabHorizontal; - this.append(btn); - this.button = btn; - - createTippy(btn, { + this.button = document.createElement('button'); + this.button.classList.add('overflow-menu-button'); + this.button.setAttribute('aria-label', window.config.i18n.more_items); + this.button.innerHTML = octiconKebabHorizontal; + this.append(this.button); + createTippy(this.button, { trigger: 'click', hideOnClick: true, interactive: true, @@ -151,6 +159,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement { }, 0); }, }); + this.updateButtonActivationState(); }); init() { @@ -187,6 +196,14 @@ window.customElements.define('overflow-menu', class extends HTMLElement { } }); this.resizeObserver.observe(this); + this.handleItemClick(this, '.overflow-menu-items > .item'); + } + + handleItemClick(el: Element, selector: string) { + addDelegatedEventListener(el, 'click', selector, () => { + this.button?._tippy?.hide(); + this.updateButtonActivationState(); + }); } connectedCallback() { From aada0370e7d72a591d58788f6db76bdbc3dddbb7 Mon Sep 17 00:00:00 2001 From: hiifong Date: Sun, 13 Apr 2025 10:24:32 +0800 Subject: [PATCH 3/6] fix webhook url (#34186) Co-authored-by: wxiaoguang --- models/fixtures/webhook.yml | 12 ++++++------ models/webhook/webhook_test.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/models/fixtures/webhook.yml b/models/fixtures/webhook.yml index ebc4062b60..ec282914b8 100644 --- a/models/fixtures/webhook.yml +++ b/models/fixtures/webhook.yml @@ -1,7 +1,7 @@ - id: 1 repo_id: 1 - url: www.example.com/url1 + url: https://www.example.com/url1 content_type: 1 # json events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}' is_active: true @@ -9,7 +9,7 @@ - id: 2 repo_id: 1 - url: www.example.com/url2 + url: https://www.example.com/url2 content_type: 1 # json events: '{"push_only":false,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":true}}' is_active: false @@ -18,7 +18,7 @@ id: 3 owner_id: 3 repo_id: 3 - url: www.example.com/url3 + url: https://www.example.com/url3 content_type: 1 # json events: '{"push_only":false,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":true}}' is_active: true @@ -26,7 +26,7 @@ - id: 4 repo_id: 2 - url: www.example.com/url4 + url: https://www.example.com/url4 content_type: 1 # json events: '{"push_only":true,"branch_filter":"{master,feature*}"}' is_active: true @@ -35,7 +35,7 @@ id: 5 repo_id: 0 owner_id: 0 - url: www.example.com/url5 + url: https://www.example.com/url5 content_type: 1 # json events: '{"push_only":true,"branch_filter":"{master,feature*}"}' is_active: true @@ -45,7 +45,7 @@ id: 6 repo_id: 0 owner_id: 0 - url: www.example.com/url6 + url: https://www.example.com/url6 content_type: 1 # json events: '{"push_only":true,"branch_filter":"{master,feature*}"}' is_active: true diff --git a/models/webhook/webhook_test.go b/models/webhook/webhook_test.go index 29f7735d09..e8a2547c65 100644 --- a/models/webhook/webhook_test.go +++ b/models/webhook/webhook_test.go @@ -90,7 +90,7 @@ func TestWebhook_EventsArray(t *testing.T) { func TestCreateWebhook(t *testing.T) { hook := &Webhook{ RepoID: 3, - URL: "www.example.com/unit_test", + URL: "https://www.example.com/unit_test", ContentType: ContentTypeJSON, Events: `{"push_only":false,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":true}}`, } From d0688cb2b3a858e4a87627dc10b101413283a165 Mon Sep 17 00:00:00 2001 From: Kerwin Bryant Date: Sun, 13 Apr 2025 12:44:57 +0800 Subject: [PATCH 4/6] Fix span svg layout (#34185) --- templates/explore/repo_list.tmpl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/explore/repo_list.tmpl b/templates/explore/repo_list.tmpl index 219b1255c0..ad190b89b2 100644 --- a/templates/explore/repo_list.tmpl +++ b/templates/explore/repo_list.tmpl @@ -39,12 +39,12 @@ {{end}} {{if not $.DisableStars}} - {{svg "octicon-star" 16}} + {{svg "octicon-star" 16}} {{CountFmt .NumStars}} {{end}} - {{svg "octicon-git-branch" 16}} + {{svg "octicon-git-branch" 16}} {{CountFmt .NumForks}} From 4dca869ed1c6ed6fae2a4e2331db756591d9eae4 Mon Sep 17 00:00:00 2001 From: Tomeamis Date: Sun, 13 Apr 2025 10:07:29 +0200 Subject: [PATCH 5/6] Allow admins and org owners to change org member public status (#28294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allows admins and org owners to change org member public status. Before, this would return `Error 403: Cannot publicize another member` despite the fact that the same user could make the same change through the GUI. Fixes #28372 --------- Co-authored-by: Tomáš Ženčák Co-authored-by: wxiaoguang --- routers/api/v1/org/member.go | 23 ++- tests/integration/api_org_test.go | 248 +++++++++++++----------- tests/integration/api_team_user_test.go | 42 ++-- 3 files changed, 176 insertions(+), 137 deletions(-) diff --git a/routers/api/v1/org/member.go b/routers/api/v1/org/member.go index 2663d78b73..a1875a7886 100644 --- a/routers/api/v1/org/member.go +++ b/routers/api/v1/org/member.go @@ -8,6 +8,7 @@ import ( "net/url" "code.gitea.io/gitea/models/organization" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/routers/api/v1/user" @@ -210,6 +211,20 @@ func IsPublicMember(ctx *context.APIContext) { } } +func checkCanChangeOrgUserStatus(ctx *context.APIContext, targetUser *user_model.User) { + // allow user themselves to change their status, and allow admins to change any user + if targetUser.ID == ctx.Doer.ID || ctx.Doer.IsAdmin { + return + } + // allow org owners to change status of members + isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID) + if err != nil { + ctx.APIError(http.StatusInternalServerError, err) + } else if !isOwner { + ctx.APIError(http.StatusForbidden, "Cannot change member visibility") + } +} + // PublicizeMember make a member's membership public func PublicizeMember(ctx *context.APIContext) { // swagger:operation PUT /orgs/{org}/public_members/{username} organization orgPublicizeMember @@ -240,8 +255,8 @@ func PublicizeMember(ctx *context.APIContext) { if ctx.Written() { return } - if userToPublicize.ID != ctx.Doer.ID { - ctx.APIError(http.StatusForbidden, "Cannot publicize another member") + checkCanChangeOrgUserStatus(ctx, userToPublicize) + if ctx.Written() { return } err := organization.ChangeOrgUserStatus(ctx, ctx.Org.Organization.ID, userToPublicize.ID, true) @@ -282,8 +297,8 @@ func ConcealMember(ctx *context.APIContext) { if ctx.Written() { return } - if userToConceal.ID != ctx.Doer.ID { - ctx.APIError(http.StatusForbidden, "Cannot conceal another member") + checkCanChangeOrgUserStatus(ctx, userToConceal) + if ctx.Written() { return } err := organization.ChangeOrgUserStatus(ctx, ctx.Org.Organization.ID, userToConceal.ID, false) diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index 46b96dc7c1..6577bd1684 100644 --- a/tests/integration/api_org_test.go +++ b/tests/integration/api_org_test.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIOrgCreateRename(t *testing.T) { @@ -110,121 +111,142 @@ func TestAPIOrgCreateRename(t *testing.T) { }) } -func TestAPIOrgEdit(t *testing.T) { +func TestAPIOrgGeneral(t *testing.T) { defer tests.PrepareTestEnv(t)() - session := loginUser(t, "user1") + user1Session := loginUser(t, "user1") + user1Token := getTokenForLoggedInUser(t, user1Session, auth_model.AccessTokenScopeWriteOrganization) - token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) - org := api.EditOrgOption{ - FullName: "Org3 organization new full name", - Description: "A new description", - Website: "https://try.gitea.io/new", - Location: "Beijing", - Visibility: "private", - } - req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) + t.Run("OrgGetAll", func(t *testing.T) { + // accessing with a token will return all orgs + req := NewRequest(t, "GET", "/api/v1/orgs").AddTokenAuth(user1Token) + resp := MakeRequest(t, req, http.StatusOK) + var apiOrgList []*api.Organization - var apiOrg api.Organization - DecodeJSON(t, resp, &apiOrg) + DecodeJSON(t, resp, &apiOrgList) + assert.Len(t, apiOrgList, 13) + assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName) + assert.Equal(t, "limited", apiOrgList[1].Visibility) - assert.Equal(t, "org3", apiOrg.Name) - assert.Equal(t, org.FullName, apiOrg.FullName) - assert.Equal(t, org.Description, apiOrg.Description) - assert.Equal(t, org.Website, apiOrg.Website) - assert.Equal(t, org.Location, apiOrg.Location) - assert.Equal(t, org.Visibility, apiOrg.Visibility) -} - -func TestAPIOrgEditBadVisibility(t *testing.T) { - defer tests.PrepareTestEnv(t)() - session := loginUser(t, "user1") - - token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) - org := api.EditOrgOption{ - FullName: "Org3 organization new full name", - Description: "A new description", - Website: "https://try.gitea.io/new", - Location: "Beijing", - Visibility: "badvisibility", - } - req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org). - AddTokenAuth(token) - MakeRequest(t, req, http.StatusUnprocessableEntity) -} - -func TestAPIOrgDeny(t *testing.T) { - defer tests.PrepareTestEnv(t)() - defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)() - - orgName := "user1_org" - req := NewRequestf(t, "GET", "/api/v1/orgs/%s", orgName) - MakeRequest(t, req, http.StatusNotFound) - - req = NewRequestf(t, "GET", "/api/v1/orgs/%s/repos", orgName) - MakeRequest(t, req, http.StatusNotFound) - - req = NewRequestf(t, "GET", "/api/v1/orgs/%s/members", orgName) - MakeRequest(t, req, http.StatusNotFound) -} - -func TestAPIGetAll(t *testing.T) { - defer tests.PrepareTestEnv(t)() - token := getUserToken(t, "user1", auth_model.AccessTokenScopeReadOrganization) - - // accessing with a token will return all orgs - req := NewRequest(t, "GET", "/api/v1/orgs"). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - var apiOrgList []*api.Organization - - DecodeJSON(t, resp, &apiOrgList) - assert.Len(t, apiOrgList, 13) - assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName) - assert.Equal(t, "limited", apiOrgList[1].Visibility) - - // accessing without a token will return only public orgs - req = NewRequest(t, "GET", "/api/v1/orgs") - resp = MakeRequest(t, req, http.StatusOK) - - DecodeJSON(t, resp, &apiOrgList) - assert.Len(t, apiOrgList, 9) - assert.Equal(t, "org 17", apiOrgList[0].FullName) - assert.Equal(t, "public", apiOrgList[0].Visibility) -} - -func TestAPIOrgSearchEmptyTeam(t *testing.T) { - defer tests.PrepareTestEnv(t)() - token := getUserToken(t, "user1", auth_model.AccessTokenScopeWriteOrganization) - orgName := "org_with_empty_team" - - // create org - req := NewRequestWithJSON(t, "POST", "/api/v1/orgs", &api.CreateOrgOption{ - UserName: orgName, - }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusCreated) - - // create team with no member - req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams", orgName), &api.CreateTeamOption{ - Name: "Empty", - IncludesAllRepositories: true, - Permission: "read", - Units: []string{"repo.code", "repo.issues", "repo.ext_issues", "repo.wiki", "repo.pulls"}, - }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusCreated) - - // case-insensitive search for teams that have no members - req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/%s/teams/search?q=%s", orgName, "empty")). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - data := struct { - Ok bool - Data []*api.Team - }{} - DecodeJSON(t, resp, &data) - assert.True(t, data.Ok) - if assert.Len(t, data.Data, 1) { - assert.Equal(t, "Empty", data.Data[0].Name) - } + // accessing without a token will return only public orgs + req = NewRequest(t, "GET", "/api/v1/orgs") + resp = MakeRequest(t, req, http.StatusOK) + + DecodeJSON(t, resp, &apiOrgList) + assert.Len(t, apiOrgList, 9) + assert.Equal(t, "org 17", apiOrgList[0].FullName) + assert.Equal(t, "public", apiOrgList[0].Visibility) + }) + + t.Run("OrgEdit", func(t *testing.T) { + org := api.EditOrgOption{ + FullName: "Org3 organization new full name", + Description: "A new description", + Website: "https://try.gitea.io/new", + Location: "Beijing", + Visibility: "private", + } + req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).AddTokenAuth(user1Token) + resp := MakeRequest(t, req, http.StatusOK) + + var apiOrg api.Organization + DecodeJSON(t, resp, &apiOrg) + + assert.Equal(t, "org3", apiOrg.Name) + assert.Equal(t, org.FullName, apiOrg.FullName) + assert.Equal(t, org.Description, apiOrg.Description) + assert.Equal(t, org.Website, apiOrg.Website) + assert.Equal(t, org.Location, apiOrg.Location) + assert.Equal(t, org.Visibility, apiOrg.Visibility) + }) + + t.Run("OrgEditBadVisibility", func(t *testing.T) { + org := api.EditOrgOption{ + FullName: "Org3 organization new full name", + Description: "A new description", + Website: "https://try.gitea.io/new", + Location: "Beijing", + Visibility: "badvisibility", + } + req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).AddTokenAuth(user1Token) + MakeRequest(t, req, http.StatusUnprocessableEntity) + }) + + t.Run("OrgDeny", func(t *testing.T) { + defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)() + + orgName := "user1_org" + req := NewRequestf(t, "GET", "/api/v1/orgs/%s", orgName) + MakeRequest(t, req, http.StatusNotFound) + + req = NewRequestf(t, "GET", "/api/v1/orgs/%s/repos", orgName) + MakeRequest(t, req, http.StatusNotFound) + + req = NewRequestf(t, "GET", "/api/v1/orgs/%s/members", orgName) + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("OrgSearchEmptyTeam", func(t *testing.T) { + orgName := "org_with_empty_team" + // create org + req := NewRequestWithJSON(t, "POST", "/api/v1/orgs", &api.CreateOrgOption{ + UserName: orgName, + }).AddTokenAuth(user1Token) + MakeRequest(t, req, http.StatusCreated) + + // create team with no member + req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams", orgName), &api.CreateTeamOption{ + Name: "Empty", + IncludesAllRepositories: true, + Permission: "read", + Units: []string{"repo.code", "repo.issues", "repo.ext_issues", "repo.wiki", "repo.pulls"}, + }).AddTokenAuth(user1Token) + MakeRequest(t, req, http.StatusCreated) + + // case-insensitive search for teams that have no members + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/%s/teams/search?q=%s", orgName, "empty")). + AddTokenAuth(user1Token) + resp := MakeRequest(t, req, http.StatusOK) + data := struct { + Ok bool + Data []*api.Team + }{} + DecodeJSON(t, resp, &data) + assert.True(t, data.Ok) + if assert.Len(t, data.Data, 1) { + assert.Equal(t, "Empty", data.Data[0].Name) + } + }) + + t.Run("User2ChangeStatus", func(t *testing.T) { + user2Session := loginUser(t, "user2") + user2Token := getTokenForLoggedInUser(t, user2Session, auth_model.AccessTokenScopeWriteOrganization) + + req := NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user2").AddTokenAuth(user2Token) + MakeRequest(t, req, http.StatusNoContent) + req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user2").AddTokenAuth(user2Token) + MakeRequest(t, req, http.StatusNoContent) + + // non admin but org owner could also change other member's status + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"}) + require.False(t, user2.IsAdmin) + req = NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user2Token) + MakeRequest(t, req, http.StatusNoContent) + req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user2Token) + MakeRequest(t, req, http.StatusNoContent) + }) + + t.Run("User4ChangeStatus", func(t *testing.T) { + user4Session := loginUser(t, "user4") + user4Token := getTokenForLoggedInUser(t, user4Session, auth_model.AccessTokenScopeWriteOrganization) + + // user4 is a normal team member, they could change their own status + req := NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user4").AddTokenAuth(user4Token) + MakeRequest(t, req, http.StatusNoContent) + req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user4").AddTokenAuth(user4Token) + MakeRequest(t, req, http.StatusNoContent) + req = NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user4Token) + MakeRequest(t, req, http.StatusForbidden) + req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user4Token) + MakeRequest(t, req, http.StatusForbidden) + }) } diff --git a/tests/integration/api_team_user_test.go b/tests/integration/api_team_user_test.go index adc9831aa0..cbbbe00a9e 100644 --- a/tests/integration/api_team_user_test.go +++ b/tests/integration/api_team_user_test.go @@ -21,29 +21,31 @@ import ( func TestAPITeamUser(t *testing.T) { defer tests.PrepareTestEnv(t)() + user2Session := loginUser(t, "user2") + user2Token := getTokenForLoggedInUser(t, user2Session, auth_model.AccessTokenScopeWriteOrganization) - normalUsername := "user2" - session := loginUser(t, normalUsername) - token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadOrganization) - req := NewRequest(t, "GET", "/api/v1/teams/1/members/user1"). - AddTokenAuth(token) - MakeRequest(t, req, http.StatusNotFound) + t.Run("User2ReadUser1", func(t *testing.T) { + req := NewRequest(t, "GET", "/api/v1/teams/1/members/user1").AddTokenAuth(user2Token) + MakeRequest(t, req, http.StatusNotFound) + }) - req = NewRequest(t, "GET", "/api/v1/teams/1/members/user2"). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - var user2 *api.User - DecodeJSON(t, resp, &user2) - user2.Created = user2.Created.In(time.Local) - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"}) + t.Run("User2ReadSelf", func(t *testing.T) { + // read self user + req := NewRequest(t, "GET", "/api/v1/teams/1/members/user2").AddTokenAuth(user2Token) + resp := MakeRequest(t, req, http.StatusOK) + var user2 *api.User + DecodeJSON(t, resp, &user2) + user2.Created = user2.Created.In(time.Local) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"}) - expectedUser := convert.ToUser(db.DefaultContext, user, user) + expectedUser := convert.ToUser(db.DefaultContext, user, user) - // test time via unix timestamp - assert.Equal(t, expectedUser.LastLogin.Unix(), user2.LastLogin.Unix()) - assert.Equal(t, expectedUser.Created.Unix(), user2.Created.Unix()) - expectedUser.LastLogin = user2.LastLogin - expectedUser.Created = user2.Created + // test time via unix timestamp + assert.Equal(t, expectedUser.LastLogin.Unix(), user2.LastLogin.Unix()) + assert.Equal(t, expectedUser.Created.Unix(), user2.Created.Unix()) + expectedUser.LastLogin = user2.LastLogin + expectedUser.Created = user2.Created - assert.Equal(t, expectedUser, user2) + assert.Equal(t, expectedUser, user2) + }) } From a2651c14ce5d9752c043b50157e83e30bf46d958 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Sun, 13 Apr 2025 11:40:36 +0200 Subject: [PATCH 6/6] Add cache for common package queries (#22491) This adds a cache for common package queries in `GetPackageDescriptor`. Code which needs to process a list of packages benefits from this change. This skips 350 queries in the package integration tests for example. --------- Co-authored-by: wxiaoguang --- models/packages/descriptor.go | 35 +++++-- modules/cache/context.go | 175 ++++------------------------------ modules/cache/context_test.go | 58 +++-------- modules/cache/ephemeral.go | 90 +++++++++++++++++ 4 files changed, 150 insertions(+), 208 deletions(-) create mode 100644 modules/cache/ephemeral.go diff --git a/models/packages/descriptor.go b/models/packages/descriptor.go index c97bd46c9e..1ea181c723 100644 --- a/models/packages/descriptor.go +++ b/models/packages/descriptor.go @@ -11,6 +11,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/packages/alpine" "code.gitea.io/gitea/modules/packages/arch" @@ -102,22 +103,26 @@ func (pd *PackageDescriptor) CalculateBlobSize() int64 { // GetPackageDescriptor gets the package description for a version func GetPackageDescriptor(ctx context.Context, pv *PackageVersion) (*PackageDescriptor, error) { - p, err := GetPackageByID(ctx, pv.PackageID) + return getPackageDescriptor(ctx, pv, cache.NewEphemeralCache()) +} + +func getPackageDescriptor(ctx context.Context, pv *PackageVersion, c *cache.EphemeralCache) (*PackageDescriptor, error) { + p, err := cache.GetWithEphemeralCache(ctx, c, "package", pv.PackageID, GetPackageByID) if err != nil { return nil, err } - o, err := user_model.GetUserByID(ctx, p.OwnerID) + o, err := cache.GetWithEphemeralCache(ctx, c, "user", p.OwnerID, user_model.GetUserByID) if err != nil { return nil, err } var repository *repo_model.Repository if p.RepoID > 0 { - repository, err = repo_model.GetRepositoryByID(ctx, p.RepoID) + repository, err = cache.GetWithEphemeralCache(ctx, c, "repo", p.RepoID, repo_model.GetRepositoryByID) if err != nil && !repo_model.IsErrRepoNotExist(err) { return nil, err } } - creator, err := user_model.GetUserByID(ctx, pv.CreatorID) + creator, err := cache.GetWithEphemeralCache(ctx, c, "user", pv.CreatorID, user_model.GetUserByID) if err != nil { if errors.Is(err, util.ErrNotExist) { creator = user_model.NewGhostUser() @@ -145,9 +150,13 @@ func GetPackageDescriptor(ctx context.Context, pv *PackageVersion) (*PackageDesc return nil, err } - pfds, err := GetPackageFileDescriptors(ctx, pfs) - if err != nil { - return nil, err + pfds := make([]*PackageFileDescriptor, 0, len(pfs)) + for _, pf := range pfs { + pfd, err := getPackageFileDescriptor(ctx, pf, c) + if err != nil { + return nil, err + } + pfds = append(pfds, pfd) } var metadata any @@ -221,7 +230,11 @@ func GetPackageDescriptor(ctx context.Context, pv *PackageVersion) (*PackageDesc // GetPackageFileDescriptor gets a package file descriptor for a package file func GetPackageFileDescriptor(ctx context.Context, pf *PackageFile) (*PackageFileDescriptor, error) { - pb, err := GetBlobByID(ctx, pf.BlobID) + return getPackageFileDescriptor(ctx, pf, cache.NewEphemeralCache()) +} + +func getPackageFileDescriptor(ctx context.Context, pf *PackageFile, c *cache.EphemeralCache) (*PackageFileDescriptor, error) { + pb, err := cache.GetWithEphemeralCache(ctx, c, "package_file_blob", pf.BlobID, GetBlobByID) if err != nil { return nil, err } @@ -251,9 +264,13 @@ func GetPackageFileDescriptors(ctx context.Context, pfs []*PackageFile) ([]*Pack // GetPackageDescriptors gets the package descriptions for the versions func GetPackageDescriptors(ctx context.Context, pvs []*PackageVersion) ([]*PackageDescriptor, error) { + return getPackageDescriptors(ctx, pvs, cache.NewEphemeralCache()) +} + +func getPackageDescriptors(ctx context.Context, pvs []*PackageVersion, c *cache.EphemeralCache) ([]*PackageDescriptor, error) { pds := make([]*PackageDescriptor, 0, len(pvs)) for _, pv := range pvs { - pd, err := GetPackageDescriptor(ctx, pv) + pd, err := getPackageDescriptor(ctx, pv, c) if err != nil { return nil, err } diff --git a/modules/cache/context.go b/modules/cache/context.go index 85eb9e6790..23f7c23a52 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -5,176 +5,39 @@ package cache import ( "context" - "sync" "time" - - "code.gitea.io/gitea/modules/log" ) -// cacheContext is a context that can be used to cache data in a request level context -// This is useful for caching data that is expensive to calculate and is likely to be -// used multiple times in a request. -type cacheContext struct { - data map[any]map[any]any - lock sync.RWMutex - created time.Time - discard bool -} +type cacheContextKeyType struct{} -func (cc *cacheContext) Get(tp, key any) any { - cc.lock.RLock() - defer cc.lock.RUnlock() - return cc.data[tp][key] -} +var cacheContextKey = cacheContextKeyType{} -func (cc *cacheContext) Put(tp, key, value any) { - cc.lock.Lock() - defer cc.lock.Unlock() - - if cc.discard { - return - } - - d := cc.data[tp] - if d == nil { - d = make(map[any]any) - cc.data[tp] = d - } - d[key] = value -} - -func (cc *cacheContext) Delete(tp, key any) { - cc.lock.Lock() - defer cc.lock.Unlock() - delete(cc.data[tp], key) -} - -func (cc *cacheContext) Discard() { - cc.lock.Lock() - defer cc.lock.Unlock() - cc.data = nil - cc.discard = true -} - -func (cc *cacheContext) isDiscard() bool { - cc.lock.RLock() - defer cc.lock.RUnlock() - return cc.discard -} - -// cacheContextLifetime is the max lifetime of cacheContext. -// Since cacheContext is used to cache data in a request level context, 5 minutes is enough. -// If a cacheContext is used more than 5 minutes, it's probably misuse. -const cacheContextLifetime = 5 * time.Minute - -var timeNow = time.Now - -func (cc *cacheContext) Expired() bool { - return timeNow().Sub(cc.created) > cacheContextLifetime -} - -var cacheContextKey = struct{}{} - -/* -Since there are both WithCacheContext and WithNoCacheContext, -it may be confusing when there is nesting. - -Some cases to explain the design: - -When: -- A, B or C means a cache context. -- A', B' or C' means a discard cache context. -- ctx means context.Backgrand(). -- A(ctx) means a cache context with ctx as the parent context. -- B(A(ctx)) means a cache context with A(ctx) as the parent context. -- With is alias of WithCacheContext. -- WithNo is alias of WithNoCacheContext. - -So: -- With(ctx) -> A(ctx) -- With(With(ctx)) -> A(ctx), not B(A(ctx)), always reuse parent cache context if possible. -- With(With(With(ctx))) -> A(ctx), not C(B(A(ctx))), ditto. -- WithNo(ctx) -> ctx, not A'(ctx), don't create new cache context if we don't have to. -- WithNo(With(ctx)) -> A'(ctx) -- WithNo(WithNo(With(ctx))) -> A'(ctx), not B'(A'(ctx)), don't create new cache context if we don't have to. -- With(WithNo(With(ctx))) -> B(A'(ctx)), not A(ctx), never reuse a discard cache context. -- WithNo(With(WithNo(With(ctx)))) -> B'(A'(ctx)) -- With(WithNo(With(WithNo(With(ctx))))) -> C(B'(A'(ctx))), so there's always only one not-discard cache context. -*/ +// contextCacheLifetime is the max lifetime of context cache. +// Since context cache is used to cache data in a request level context, 5 minutes is enough. +// If a context cache is used more than 5 minutes, it's probably abused. +const contextCacheLifetime = 5 * time.Minute func WithCacheContext(ctx context.Context) context.Context { - if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { - if !c.isDiscard() { - // reuse parent context - return ctx - } - } - // FIXME: review the use of this nolint directive - return context.WithValue(ctx, cacheContextKey, &cacheContext{ //nolint:staticcheck - data: make(map[any]map[any]any), - created: timeNow(), - }) -} - -func WithNoCacheContext(ctx context.Context) context.Context { - if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { - // The caller want to run long-life tasks, but the parent context is a cache context. - // So we should disable and clean the cache data, or it will be kept in memory for a long time. - c.Discard() + if c := GetContextCache(ctx); c != nil { return ctx } - - return ctx + return context.WithValue(ctx, cacheContextKey, NewEphemeralCache(contextCacheLifetime)) } -func GetContextData(ctx context.Context, tp, key any) any { - if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { - if c.Expired() { - // The warning means that the cache context is misused for long-life task, - // it can be resolved with WithNoCacheContext(ctx). - log.Warn("cache context is expired, is highly likely to be misused for long-life tasks: %v", c) - return nil - } - return c.Get(tp, key) - } - return nil -} - -func SetContextData(ctx context.Context, tp, key, value any) { - if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { - if c.Expired() { - // The warning means that the cache context is misused for long-life task, - // it can be resolved with WithNoCacheContext(ctx). - log.Warn("cache context is expired, is highly likely to be misused for long-life tasks: %v", c) - return - } - c.Put(tp, key, value) - return - } -} - -func RemoveContextData(ctx context.Context, tp, key any) { - if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { - if c.Expired() { - // The warning means that the cache context is misused for long-life task, - // it can be resolved with WithNoCacheContext(ctx). - log.Warn("cache context is expired, is highly likely to be misused for long-life tasks: %v", c) - return - } - c.Delete(tp, key) - } +func GetContextCache(ctx context.Context) *EphemeralCache { + c, _ := ctx.Value(cacheContextKey).(*EphemeralCache) + return c } // GetWithContextCache returns the cache value of the given key in the given context. +// FIXME: in some cases, the "context cache" should not be used, because it has uncontrollable behaviors +// For example, these calls: +// * GetWithContextCache(TargetID) -> OtherCodeCreateModel(TargetID) -> GetWithContextCache(TargetID) +// Will cause the second call is not able to get the correct created target. +// UNLESS it is certain that the target won't be changed during the request, DO NOT use it. func GetWithContextCache[T, K any](ctx context.Context, groupKey string, targetKey K, f func(context.Context, K) (T, error)) (T, error) { - v := GetContextData(ctx, groupKey, targetKey) - if vv, ok := v.(T); ok { - return vv, nil + if c := GetContextCache(ctx); c != nil { + return GetWithEphemeralCache(ctx, c, groupKey, targetKey, f) } - t, err := f(ctx, targetKey) - if err != nil { - return t, err - } - SetContextData(ctx, groupKey, targetKey, t) - return t, nil + return f(ctx, targetKey) } diff --git a/modules/cache/context_test.go b/modules/cache/context_test.go index 23dd789dbc..8371c2b908 100644 --- a/modules/cache/context_test.go +++ b/modules/cache/context_test.go @@ -8,27 +8,29 @@ import ( "testing" "time" + "code.gitea.io/gitea/modules/test" + "github.com/stretchr/testify/assert" ) func TestWithCacheContext(t *testing.T) { ctx := WithCacheContext(t.Context()) - - v := GetContextData(ctx, "empty_field", "my_config1") + c := GetContextCache(ctx) + v, _ := c.Get("empty_field", "my_config1") assert.Nil(t, v) const field = "system_setting" - v = GetContextData(ctx, field, "my_config1") + v, _ = c.Get(field, "my_config1") assert.Nil(t, v) - SetContextData(ctx, field, "my_config1", 1) - v = GetContextData(ctx, field, "my_config1") + c.Put(field, "my_config1", 1) + v, _ = c.Get(field, "my_config1") assert.NotNil(t, v) assert.Equal(t, 1, v.(int)) - RemoveContextData(ctx, field, "my_config1") - RemoveContextData(ctx, field, "my_config2") // remove a non-exist key + c.Delete(field, "my_config1") + c.Delete(field, "my_config2") // remove a non-exist key - v = GetContextData(ctx, field, "my_config1") + v, _ = c.Get(field, "my_config1") assert.Nil(t, v) vInt, err := GetWithContextCache(ctx, field, "my_config1", func(context.Context, string) (int, error) { @@ -37,42 +39,12 @@ func TestWithCacheContext(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 1, vInt) - v = GetContextData(ctx, field, "my_config1") + v, _ = c.Get(field, "my_config1") assert.EqualValues(t, 1, v) - now := timeNow - defer func() { - timeNow = now - }() - timeNow = func() time.Time { - return now().Add(5 * time.Minute) - } - v = GetContextData(ctx, field, "my_config1") + defer test.MockVariableValue(&timeNow, func() time.Time { + return time.Now().Add(5 * time.Minute) + })() + v, _ = c.Get(field, "my_config1") assert.Nil(t, v) } - -func TestWithNoCacheContext(t *testing.T) { - ctx := t.Context() - - const field = "system_setting" - - v := GetContextData(ctx, field, "my_config1") - assert.Nil(t, v) - SetContextData(ctx, field, "my_config1", 1) - v = GetContextData(ctx, field, "my_config1") - assert.Nil(t, v) // still no cache - - ctx = WithCacheContext(ctx) - v = GetContextData(ctx, field, "my_config1") - assert.Nil(t, v) - SetContextData(ctx, field, "my_config1", 1) - v = GetContextData(ctx, field, "my_config1") - assert.NotNil(t, v) - - ctx = WithNoCacheContext(ctx) - v = GetContextData(ctx, field, "my_config1") - assert.Nil(t, v) - SetContextData(ctx, field, "my_config1", 1) - v = GetContextData(ctx, field, "my_config1") - assert.Nil(t, v) // still no cache -} diff --git a/modules/cache/ephemeral.go b/modules/cache/ephemeral.go new file mode 100644 index 0000000000..6996010ac4 --- /dev/null +++ b/modules/cache/ephemeral.go @@ -0,0 +1,90 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package cache + +import ( + "context" + "sync" + "time" + + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" +) + +// EphemeralCache is a cache that can be used to store data in a request level context +// This is useful for caching data that is expensive to calculate and is likely to be +// used multiple times in a request. +type EphemeralCache struct { + data map[any]map[any]any + lock sync.RWMutex + created time.Time + checkLifeTime time.Duration +} + +var timeNow = time.Now + +func NewEphemeralCache(checkLifeTime ...time.Duration) *EphemeralCache { + return &EphemeralCache{ + data: make(map[any]map[any]any), + created: timeNow(), + checkLifeTime: util.OptionalArg(checkLifeTime, 0), + } +} + +func (cc *EphemeralCache) checkExceededLifeTime(tp, key any) bool { + if cc.checkLifeTime > 0 && timeNow().Sub(cc.created) > cc.checkLifeTime { + log.Warn("EphemeralCache is expired, is highly likely to be abused for long-life tasks: %v, %v", tp, key) + return true + } + return false +} + +func (cc *EphemeralCache) Get(tp, key any) (any, bool) { + if cc.checkExceededLifeTime(tp, key) { + return nil, false + } + cc.lock.RLock() + defer cc.lock.RUnlock() + ret, ok := cc.data[tp][key] + return ret, ok +} + +func (cc *EphemeralCache) Put(tp, key, value any) { + if cc.checkExceededLifeTime(tp, key) { + return + } + + cc.lock.Lock() + defer cc.lock.Unlock() + + d := cc.data[tp] + if d == nil { + d = make(map[any]any) + cc.data[tp] = d + } + d[key] = value +} + +func (cc *EphemeralCache) Delete(tp, key any) { + if cc.checkExceededLifeTime(tp, key) { + return + } + + cc.lock.Lock() + defer cc.lock.Unlock() + delete(cc.data[tp], key) +} + +func GetWithEphemeralCache[T, K any](ctx context.Context, c *EphemeralCache, groupKey string, targetKey K, f func(context.Context, K) (T, error)) (T, error) { + v, has := c.Get(groupKey, targetKey) + if vv, ok := v.(T); has && ok { + return vv, nil + } + t, err := f(ctx, targetKey) + if err != nil { + return t, err + } + c.Put(groupKey, targetKey, t) + return t, nil +}