Improve LoadUnitConfig to handle invalid or duplicate units (#23736)

The old code just parses an invalid key to `TypeInvalid` and uses it as
normal, and duplicate keys will be kept.

So this PR will ignore invalid key and log warning and also deduplicate
valid units.
This commit is contained in:
Jason Song 2023-04-03 16:42:38 +08:00 committed by GitHub
parent ca905b82df
commit d67e40684f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 77 additions and 17 deletions

View file

@ -151,7 +151,11 @@ func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type {
// LoadUnitConfig load units from settings // LoadUnitConfig load units from settings
func LoadUnitConfig() { func LoadUnitConfig() {
DisabledRepoUnits = FindUnitTypes(setting.Repository.DisabledRepoUnits...) var invalidKeys []string
DisabledRepoUnits, invalidKeys = FindUnitTypes(setting.Repository.DisabledRepoUnits...)
if len(invalidKeys) > 0 {
log.Warn("Invalid keys in disabled repo units: %s", strings.Join(invalidKeys, ", "))
}
// Check that must units are not disabled // Check that must units are not disabled
for i, disabledU := range DisabledRepoUnits { for i, disabledU := range DisabledRepoUnits {
if !disabledU.CanDisable() { if !disabledU.CanDisable() {
@ -160,9 +164,15 @@ func LoadUnitConfig() {
} }
} }
setDefaultRepoUnits := FindUnitTypes(setting.Repository.DefaultRepoUnits...) setDefaultRepoUnits, invalidKeys := FindUnitTypes(setting.Repository.DefaultRepoUnits...)
if len(invalidKeys) > 0 {
log.Warn("Invalid keys in default repo units: %s", strings.Join(invalidKeys, ", "))
}
DefaultRepoUnits = validateDefaultRepoUnits(DefaultRepoUnits, setDefaultRepoUnits) DefaultRepoUnits = validateDefaultRepoUnits(DefaultRepoUnits, setDefaultRepoUnits)
setDefaultForkRepoUnits := FindUnitTypes(setting.Repository.DefaultForkRepoUnits...) setDefaultForkRepoUnits, invalidKeys := FindUnitTypes(setting.Repository.DefaultForkRepoUnits...)
if len(invalidKeys) > 0 {
log.Warn("Invalid keys in default fork repo units: %s", strings.Join(invalidKeys, ", "))
}
DefaultForkRepoUnits = validateDefaultRepoUnits(DefaultForkRepoUnits, setDefaultForkRepoUnits) DefaultForkRepoUnits = validateDefaultRepoUnits(DefaultForkRepoUnits, setDefaultForkRepoUnits)
} }
@ -334,22 +344,19 @@ var (
} }
) )
// FindUnitTypes give the unit key names and return unit // FindUnitTypes give the unit key names and return valid unique units and invalid keys
func FindUnitTypes(nameKeys ...string) (res []Type) { func FindUnitTypes(nameKeys ...string) (res []Type, invalidKeys []string) {
m := map[Type]struct{}{}
for _, key := range nameKeys { for _, key := range nameKeys {
var found bool t := TypeFromKey(key)
for t, u := range Units { if t == TypeInvalid {
if strings.EqualFold(key, u.NameKey) { invalidKeys = append(invalidKeys, key)
} else if _, ok := m[t]; !ok {
res = append(res, t) res = append(res, t)
found = true m[t] = struct{}{}
break
} }
} }
if !found { return res, invalidKeys
res = append(res, TypeInvalid)
}
}
return res
} }
// TypeFromKey give the unit key name and return unit // TypeFromKey give the unit key name and return unit

53
models/unit/unit_test.go Normal file
View file

@ -0,0 +1,53 @@
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package unit
import (
"testing"
"code.gitea.io/gitea/modules/setting"
"github.com/stretchr/testify/assert"
)
func TestLoadUnitConfig(t *testing.T) {
defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) {
DisabledRepoUnits = disabledRepoUnits
DefaultRepoUnits = defaultRepoUnits
DefaultForkRepoUnits = defaultForkRepoUnits
}(DisabledRepoUnits, 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)
t.Run("regular", func(t *testing.T) {
setting.Repository.DisabledRepoUnits = []string{"repo.issues"}
setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls"}
setting.Repository.DefaultForkRepoUnits = []string{"repo.releases"}
LoadUnitConfig()
assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits)
assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits)
assert.Equal(t, []Type{TypeCode, TypeReleases}, DefaultForkRepoUnits)
})
t.Run("invalid", func(t *testing.T) {
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"}
LoadUnitConfig()
assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits)
assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits)
assert.Equal(t, []Type{TypeCode, TypeReleases}, DefaultForkRepoUnits)
})
t.Run("duplicate", func(t *testing.T) {
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"}
LoadUnitConfig()
assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits)
assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits)
assert.Equal(t, []Type{TypeCode, TypeReleases}, DefaultForkRepoUnits)
})
}

View file

@ -135,7 +135,7 @@ func GetTeam(ctx *context.APIContext) {
} }
func attachTeamUnits(team *organization.Team, units []string) { func attachTeamUnits(team *organization.Team, units []string) {
unitTypes := unit_model.FindUnitTypes(units...) unitTypes, _ := unit_model.FindUnitTypes(units...)
team.Units = make([]*organization.TeamUnit, 0, len(units)) team.Units = make([]*organization.TeamUnit, 0, len(units))
for _, tp := range unitTypes { for _, tp := range unitTypes {
team.Units = append(team.Units, &organization.TeamUnit{ team.Units = append(team.Units, &organization.TeamUnit{