From 851356577c8ef7eefb351014a3ba8b6a670c0cd9 Mon Sep 17 00:00:00 2001 From: limiting-factor Date: Sat, 7 Mar 2026 02:03:22 +0100 Subject: [PATCH] chore: do not leak global repository unit defaults (#11470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The [test disabling the issue unit][0] took care of [reseting the disabled units][1]. However, it overlooked that calling [`LoadUnitConfig`][2] also has a [side effect on `DefaultRepoUnits`][3]. It happens when [`validateDefaultRepoUnits` has a side effect][4] on the array backing the slice, when it is [not recreated][5]. As a result the issue unit is disabled for all tests that run after this one. The subtle side effect is harmless because it only happens in tests, the `LoadUnitConfig` is otherwise only called once. For clarity `LoadUnitConfig` is modified to clone the unit array being validated so that the returned slice is never backed by the same array as the argument. As the global variables used for repository units should be saved and restored as a whole, a dedicated test function (`SaveUnits`) is provided to be used by both integration tests and unit tests. The test of the unit model is refactored to be a blackbox test in order to avoid an import cycle. [0]: https://codeberg.org/forgejo/forgejo/src/commit/cce5f868cec11830d19126b90c659772708cd323/tests/integration/repo_settings_test.go#L258 [1]: https://codeberg.org/forgejo/forgejo/src/commit/cce5f868cec11830d19126b90c659772708cd323/tests/integration/repo_settings_test.go#L253 [2]: https://codeberg.org/forgejo/forgejo/src/commit/cce5f868cec11830d19126b90c659772708cd323/models/unit/unit.go#L171 [3]: https://codeberg.org/forgejo/forgejo/src/commit/cce5f868cec11830d19126b90c659772708cd323/models/unit/unit.go#L182 [4]: https://codeberg.org/forgejo/forgejo/src/commit/cce5f868cec11830d19126b90c659772708cd323/models/unit/unit.go#L162 [5]: https://codeberg.org/forgejo/forgejo/src/commit/cce5f868cec11830d19126b90c659772708cd323/models/unit/unit.go#L148 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11470 Reviewed-by: Ellen Εμιλία Άννα Zscheile Reviewed-by: Gusted Co-authored-by: limiting-factor Co-committed-by: limiting-factor --- models/unit/tests/units.go | 35 +++++++++++ models/unit/tests/units_test.go | 37 ++++++++++++ models/unit/unit.go | 3 +- models/unit/unit_test.go | 80 +++++++------------------ tests/integration/repo_settings_test.go | 7 +-- 5 files changed, 100 insertions(+), 62 deletions(-) create mode 100644 models/unit/tests/units.go create mode 100644 models/unit/tests/units_test.go diff --git a/models/unit/tests/units.go b/models/unit/tests/units.go new file mode 100644 index 0000000000..7507b80e2a --- /dev/null +++ b/models/unit/tests/units.go @@ -0,0 +1,35 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package tests + +import ( + unit_model "forgejo.org/models/unit" + "forgejo.org/modules/setting" + "forgejo.org/modules/test" +) + +func SaveUnits() func() { + disabledGlobal := unit_model.DisabledRepoUnitsGet() + restoreDisabledGlobal := func() { + unit_model.DisabledRepoUnitsSet(disabledGlobal) + } + restoreDisabledRepo := test.MockProtect(&setting.Repository.DisabledRepoUnits) + + restoreDefaultGlobal := test.MockProtect(&unit_model.DefaultRepoUnits) + restoreDefaultRepo := test.MockProtect(&setting.Repository.DefaultRepoUnits) + + restoreForkGlobal := test.MockProtect(&unit_model.DefaultForkRepoUnits) + restoreForkRepo := test.MockProtect(&setting.Repository.DefaultForkRepoUnits) + + return func() { + restoreDisabledGlobal() + restoreDisabledRepo() + + restoreDefaultGlobal() + restoreDefaultRepo() + + restoreForkGlobal() + restoreForkRepo() + } +} diff --git a/models/unit/tests/units_test.go b/models/unit/tests/units_test.go new file mode 100644 index 0000000000..acf543285f --- /dev/null +++ b/models/unit/tests/units_test.go @@ -0,0 +1,37 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package tests + +import ( + "testing" + + unit_model "forgejo.org/models/unit" + "forgejo.org/modules/setting" + + "github.com/stretchr/testify/assert" +) + +func TestSaveUnits(t *testing.T) { + restoreUnits := SaveUnits() + + unit_model.DisabledRepoUnitsSet([]unit_model.Type{unit_model.TypeInvalid}) + setting.Repository.DisabledRepoUnits = []string{"invalid"} + + unit_model.DefaultRepoUnits = []unit_model.Type{unit_model.TypeInvalid} + setting.Repository.DefaultRepoUnits = []string{"invalid"} + + unit_model.DefaultForkRepoUnits = []unit_model.Type{unit_model.TypeInvalid} + setting.Repository.DefaultForkRepoUnits = []string{"invalid"} + + restoreUnits() + + assert.NotEqual(t, []unit_model.Type{unit_model.TypeInvalid}, unit_model.DisabledRepoUnitsGet()) + assert.NotEqual(t, []string{"invalid"}, setting.Repository.DisabledRepoUnits) + + assert.NotEqual(t, []unit_model.Type{unit_model.TypeInvalid}, unit_model.DefaultRepoUnits) + assert.NotEqual(t, []string{"invalid"}, setting.Repository.DefaultRepoUnits) + + assert.NotEqual(t, []unit_model.Type{unit_model.TypeInvalid}, unit_model.DefaultForkRepoUnits) + assert.NotEqual(t, []string{"invalid"}, setting.Repository.DefaultForkRepoUnits) +} diff --git a/models/unit/unit.go b/models/unit/unit.go index 2f8306d45f..434e5f0acc 100644 --- a/models/unit/unit.go +++ b/models/unit/unit.go @@ -7,6 +7,7 @@ package unit import ( "errors" "fmt" + "slices" "strings" "sync/atomic" @@ -141,7 +142,7 @@ func DisabledRepoUnitsSet(v []Type) { // Get valid set of default repository units from settings func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { - units := defaultUnits + units := slices.Clone(defaultUnits) // Use setting if not empty if len(settingDefaultUnits) > 0 { diff --git a/models/unit/unit_test.go b/models/unit/unit_test.go index efcad4a405..1da15c4d85 100644 --- a/models/unit/unit_test.go +++ b/models/unit/unit_test.go @@ -1,11 +1,13 @@ // Copyright 2023 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT -package unit +package unit_test import ( "testing" + unit_model "forgejo.org/models/unit" + "forgejo.org/models/unit/tests" "forgejo.org/modules/setting" "github.com/stretchr/testify/assert" @@ -14,83 +16,47 @@ import ( func TestLoadUnitConfig(t *testing.T) { t.Run("regular", func(t *testing.T) { - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnitsSet(disabledRepoUnits) - DefaultRepoUnits = defaultRepoUnits - DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { - setting.Repository.DisabledRepoUnits = disabledRepoUnits - setting.Repository.DefaultRepoUnits = defaultRepoUnits - setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits - }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + defer tests.SaveUnits()() setting.Repository.DisabledRepoUnits = []string{"repo.issues"} setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls"} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases"} - require.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) - assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) + require.NoError(t, unit_model.LoadUnitConfig()) + assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet()) + assert.Equal(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypeReleases, unit_model.TypePullRequests}, unit_model.DefaultRepoUnits) + assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits) }) t.Run("invalid", func(t *testing.T) { - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnitsSet(disabledRepoUnits) - DefaultRepoUnits = defaultRepoUnits - DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { - setting.Repository.DisabledRepoUnits = disabledRepoUnits - setting.Repository.DefaultRepoUnits = defaultRepoUnits - setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits - }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + defer tests.SaveUnits()() setting.Repository.DisabledRepoUnits = []string{"repo.issues", "invalid.1"} setting.Repository.DefaultRepoUnits = []string{"repo.code", "invalid.2", "repo.releases", "repo.issues", "repo.pulls"} setting.Repository.DefaultForkRepoUnits = []string{"invalid.3", "repo.releases"} - require.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) - assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) + require.NoError(t, unit_model.LoadUnitConfig()) + assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet()) + assert.Equal(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypeReleases, unit_model.TypePullRequests}, unit_model.DefaultRepoUnits) + assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits) }) t.Run("duplicate", func(t *testing.T) { - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnitsSet(disabledRepoUnits) - DefaultRepoUnits = defaultRepoUnits - DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { - setting.Repository.DisabledRepoUnits = disabledRepoUnits - setting.Repository.DefaultRepoUnits = defaultRepoUnits - setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits - }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + defer tests.SaveUnits()() setting.Repository.DisabledRepoUnits = []string{"repo.issues", "repo.issues"} setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls", "repo.code"} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"} - require.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) - assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) + require.NoError(t, unit_model.LoadUnitConfig()) + assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet()) + assert.Equal(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypeReleases, unit_model.TypePullRequests}, unit_model.DefaultRepoUnits) + assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits) }) t.Run("empty_default", func(t *testing.T) { - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnitsSet(disabledRepoUnits) - DefaultRepoUnits = defaultRepoUnits - DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { - setting.Repository.DisabledRepoUnits = disabledRepoUnits - setting.Repository.DefaultRepoUnits = defaultRepoUnits - setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits - }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + defer tests.SaveUnits()() setting.Repository.DisabledRepoUnits = []string{"repo.issues", "repo.issues"} setting.Repository.DefaultRepoUnits = []string{} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"} - require.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) - assert.ElementsMatch(t, []Type{TypeCode, TypePullRequests, TypeReleases, TypeWiki, TypePackages, TypeProjects, TypeActions}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) + require.NoError(t, unit_model.LoadUnitConfig()) + assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet()) + assert.ElementsMatch(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypePullRequests, unit_model.TypeReleases, unit_model.TypeWiki, unit_model.TypePackages, unit_model.TypeProjects, unit_model.TypeActions}, unit_model.DefaultRepoUnits) + assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits) }) } diff --git a/tests/integration/repo_settings_test.go b/tests/integration/repo_settings_test.go index a17b05c4e1..1e164563d8 100644 --- a/tests/integration/repo_settings_test.go +++ b/tests/integration/repo_settings_test.go @@ -12,6 +12,7 @@ import ( git_model "forgejo.org/models/git" repo_model "forgejo.org/models/repo" unit_model "forgejo.org/models/unit" + unit_tests "forgejo.org/models/unit/tests" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" "forgejo.org/modules/optional" @@ -222,13 +223,12 @@ func TestRepoAddMoreUnits(t *testing.T) { t.Run("no add more if unit is globally disabled", func(t *testing.T) { defer tests.PrintCurrentTest(t)() + defer unit_tests.SaveUnits()() defer func() { repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, []repo_model.RepoUnit{{ RepoID: repo.ID, Type: unit_model.TypePackages, }}, nil) - setting.Repository.DisabledRepoUnits = []string{} - unit_model.LoadUnitConfig() }() // Disable the Packages unit globally @@ -245,13 +245,12 @@ func TestRepoAddMoreUnits(t *testing.T) { t.Run("issues & ext tracker globally disabled", func(t *testing.T) { defer tests.PrintCurrentTest(t)() + defer unit_tests.SaveUnits()() defer func() { repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, []repo_model.RepoUnit{{ RepoID: repo.ID, Type: unit_model.TypeIssues, }}, nil) - setting.Repository.DisabledRepoUnits = []string{} - unit_model.LoadUnitConfig() }() // Disable both Issues and ExternalTracker units globally