Skip to content

Commit 26b538f

Browse files
lunnywxiaoguang
authored and
GiteaBot
committed
Fix the wrong derive path (go-gitea#26271)
This PR will fix go-gitea#26264, caused by go-gitea#23911. The package configuration derive is totally wrong when storage type is local in that PR. This PR fixed the inherit logic when storage type is local with some unit tests. --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
1 parent a57568b commit 26b538f

File tree

2 files changed

+215
-17
lines changed

2 files changed

+215
-17
lines changed

‎modules/setting/storage.go

+56-17
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,15 @@ func getDefaultStorageSection(rootCfg ConfigProvider) ConfigSection {
8484
return storageSec
8585
}
8686

87+
// getStorage will find target section and extra special section first and then read override
88+
// items from extra section
8789
func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*Storage, error) {
8890
if name == "" {
8991
return nil, errors.New("no name for storage")
9092
}
9193

9294
var targetSec ConfigSection
95+
// check typ first
9396
if typ != "" {
9497
var err error
9598
targetSec, err = rootCfg.GetSection(storageSectionName + "." + typ)
@@ -111,24 +114,40 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S
111114
}
112115
}
113116

114-
storageNameSec, _ := rootCfg.GetSection(storageSectionName + "." + name)
115-
116-
if targetSec == nil {
117-
targetSec = sec
117+
if targetSec == nil && sec != nil {
118+
secTyp := sec.Key("STORAGE_TYPE").String()
119+
if IsValidStorageType(StorageType(secTyp)) {
120+
targetSec = sec
121+
} else if secTyp != "" {
122+
targetSec, _ = rootCfg.GetSection(storageSectionName + "." + secTyp)
123+
}
118124
}
125+
126+
targetSecIsStoragename := false
127+
storageNameSec, _ := rootCfg.GetSection(storageSectionName + "." + name)
119128
if targetSec == nil {
120129
targetSec = storageNameSec
130+
targetSecIsStoragename = storageNameSec != nil
121131
}
132+
122133
if targetSec == nil {
123134
targetSec = getDefaultStorageSection(rootCfg)
124135
} else {
125136
targetType := targetSec.Key("STORAGE_TYPE").String()
126137
switch {
127138
case targetType == "":
128-
if targetSec.Key("PATH").String() == "" {
129-
targetSec = getDefaultStorageSection(rootCfg)
139+
if targetSec != storageNameSec && storageNameSec != nil {
140+
targetSec = storageNameSec
141+
targetSecIsStoragename = true
142+
if targetSec.Key("STORAGE_TYPE").String() == "" {
143+
return nil, fmt.Errorf("storage section %s.%s has no STORAGE_TYPE", storageSectionName, name)
144+
}
130145
} else {
131-
targetSec.Key("STORAGE_TYPE").SetValue("local")
146+
if targetSec.Key("PATH").String() == "" {
147+
targetSec = getDefaultStorageSection(rootCfg)
148+
} else {
149+
targetSec.Key("STORAGE_TYPE").SetValue("local")
150+
}
132151
}
133152
default:
134153
newTargetSec, _ := rootCfg.GetSection(storageSectionName + "." + targetType)
@@ -153,27 +172,47 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S
153172
return nil, fmt.Errorf("invalid storage type %q", targetType)
154173
}
155174

175+
// extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible
176+
extraConfigSec := sec
177+
if extraConfigSec == nil {
178+
extraConfigSec = storageNameSec
179+
}
180+
156181
var storage Storage
157182
storage.Type = StorageType(targetType)
158183

159184
switch targetType {
160185
case string(LocalStorageType):
161-
storage.Path = ConfigSectionKeyString(targetSec, "PATH", filepath.Join(AppDataPath, name))
162-
if !filepath.IsAbs(storage.Path) {
163-
storage.Path = filepath.Join(AppWorkPath, storage.Path)
186+
targetPath := ConfigSectionKeyString(targetSec, "PATH", "")
187+
if targetPath == "" {
188+
targetPath = AppDataPath
189+
} else if !filepath.IsAbs(targetPath) {
190+
targetPath = filepath.Join(AppDataPath, targetPath)
164191
}
165-
case string(MinioStorageType):
166-
storage.MinioConfig.BasePath = name + "/"
167192

168-
if err := targetSec.MapTo(&storage.MinioConfig); err != nil {
169-
return nil, fmt.Errorf("map minio config failed: %v", err)
193+
var fallbackPath string
194+
if targetSecIsStoragename {
195+
fallbackPath = targetPath
196+
} else {
197+
fallbackPath = filepath.Join(targetPath, name)
170198
}
171-
// extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH to override the targetsec
172-
extraConfigSec := sec
199+
173200
if extraConfigSec == nil {
174-
extraConfigSec = storageNameSec
201+
storage.Path = fallbackPath
202+
} else {
203+
storage.Path = ConfigSectionKeyString(extraConfigSec, "PATH", fallbackPath)
204+
if !filepath.IsAbs(storage.Path) {
205+
storage.Path = filepath.Join(targetPath, storage.Path)
206+
}
207+
}
208+
209+
case string(MinioStorageType):
210+
if err := targetSec.MapTo(&storage.MinioConfig); err != nil {
211+
return nil, fmt.Errorf("map minio config failed: %v", err)
175212
}
176213

214+
storage.MinioConfig.BasePath = name + "/"
215+
177216
if extraConfigSec != nil {
178217
storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(extraConfigSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect)
179218
storage.MinioConfig.BasePath = ConfigSectionKeyString(extraConfigSec, "MINIO_BASE_PATH", storage.MinioConfig.BasePath)

‎modules/setting/storage_test.go

+159
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package setting
55

66
import (
7+
"path/filepath"
78
"testing"
89

910
"github.com/stretchr/testify/assert"
@@ -90,3 +91,161 @@ STORAGE_TYPE = minio
9091
assert.EqualValues(t, "gitea", RepoAvatar.Storage.MinioConfig.Bucket)
9192
assert.EqualValues(t, "repo-avatars/", RepoAvatar.Storage.MinioConfig.BasePath)
9293
}
94+
95+
type testLocalStoragePathCase struct {
96+
loader func(rootCfg ConfigProvider) error
97+
storagePtr **Storage
98+
expectedPath string
99+
}
100+
101+
func testLocalStoragePath(t *testing.T, appDataPath, iniStr string, cases []testLocalStoragePathCase) {
102+
cfg, err := NewConfigProviderFromData(iniStr)
103+
assert.NoError(t, err)
104+
AppDataPath = appDataPath
105+
for _, c := range cases {
106+
assert.NoError(t, c.loader(cfg))
107+
storage := *c.storagePtr
108+
109+
assert.EqualValues(t, "local", storage.Type)
110+
assert.True(t, filepath.IsAbs(storage.Path))
111+
assert.EqualValues(t, filepath.Clean(c.expectedPath), filepath.Clean(storage.Path))
112+
}
113+
}
114+
115+
func Test_getStorageInheritStorageTypeLocal(t *testing.T) {
116+
testLocalStoragePath(t, "/appdata", `
117+
[storage]
118+
STORAGE_TYPE = local
119+
`, []testLocalStoragePathCase{
120+
{loadPackagesFrom, &Packages.Storage, "/appdata/packages"},
121+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/repo-archive"},
122+
{loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"},
123+
{loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"},
124+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"},
125+
})
126+
}
127+
128+
func Test_getStorageInheritStorageTypeLocalPath(t *testing.T) {
129+
testLocalStoragePath(t, "/appdata", `
130+
[storage]
131+
STORAGE_TYPE = local
132+
PATH = /data/gitea
133+
`, []testLocalStoragePathCase{
134+
{loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"},
135+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/repo-archive"},
136+
{loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"},
137+
{loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"},
138+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"},
139+
})
140+
}
141+
142+
func Test_getStorageInheritStorageTypeLocalRelativePath(t *testing.T) {
143+
testLocalStoragePath(t, "/appdata", `
144+
[storage]
145+
STORAGE_TYPE = local
146+
PATH = storages
147+
`, []testLocalStoragePathCase{
148+
{loadPackagesFrom, &Packages.Storage, "/appdata/storages/packages"},
149+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/storages/repo-archive"},
150+
{loadActionsFrom, &Actions.LogStorage, "/appdata/storages/actions_log"},
151+
{loadAvatarsFrom, &Avatar.Storage, "/appdata/storages/avatars"},
152+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/storages/repo-avatars"},
153+
})
154+
}
155+
156+
func Test_getStorageInheritStorageTypeLocalPathOverride(t *testing.T) {
157+
testLocalStoragePath(t, "/appdata", `
158+
[storage]
159+
STORAGE_TYPE = local
160+
PATH = /data/gitea
161+
162+
[repo-archive]
163+
PATH = /data/gitea/the-archives-dir
164+
`, []testLocalStoragePathCase{
165+
{loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"},
166+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/the-archives-dir"},
167+
{loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"},
168+
{loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"},
169+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"},
170+
})
171+
}
172+
173+
func Test_getStorageInheritStorageTypeLocalPathOverrideEmpty(t *testing.T) {
174+
testLocalStoragePath(t, "/appdata", `
175+
[storage]
176+
STORAGE_TYPE = local
177+
PATH = /data/gitea
178+
179+
[repo-archive]
180+
`, []testLocalStoragePathCase{
181+
{loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"},
182+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/repo-archive"},
183+
{loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"},
184+
{loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"},
185+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"},
186+
})
187+
}
188+
189+
func Test_getStorageInheritStorageTypeLocalRelativePathOverride(t *testing.T) {
190+
testLocalStoragePath(t, "/appdata", `
191+
[storage]
192+
STORAGE_TYPE = local
193+
PATH = /data/gitea
194+
195+
[repo-archive]
196+
PATH = the-archives-dir
197+
`, []testLocalStoragePathCase{
198+
{loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"},
199+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/the-archives-dir"},
200+
{loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"},
201+
{loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"},
202+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"},
203+
})
204+
}
205+
206+
func Test_getStorageInheritStorageTypeLocalPathOverride3(t *testing.T) {
207+
testLocalStoragePath(t, "/appdata", `
208+
[storage.repo-archive]
209+
STORAGE_TYPE = local
210+
PATH = /data/gitea/archives
211+
`, []testLocalStoragePathCase{
212+
{loadPackagesFrom, &Packages.Storage, "/appdata/packages"},
213+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/archives"},
214+
{loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"},
215+
{loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"},
216+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"},
217+
})
218+
}
219+
220+
func Test_getStorageInheritStorageTypeLocalPathOverride4(t *testing.T) {
221+
testLocalStoragePath(t, "/appdata", `
222+
[storage.repo-archive]
223+
STORAGE_TYPE = local
224+
PATH = /data/gitea/archives
225+
226+
[repo-archive]
227+
PATH = /tmp/gitea/archives
228+
`, []testLocalStoragePathCase{
229+
{loadPackagesFrom, &Packages.Storage, "/appdata/packages"},
230+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/tmp/gitea/archives"},
231+
{loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"},
232+
{loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"},
233+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"},
234+
})
235+
}
236+
237+
func Test_getStorageInheritStorageTypeLocalPathOverride5(t *testing.T) {
238+
testLocalStoragePath(t, "/appdata", `
239+
[storage.repo-archive]
240+
STORAGE_TYPE = local
241+
PATH = /data/gitea/archives
242+
243+
[repo-archive]
244+
`, []testLocalStoragePathCase{
245+
{loadPackagesFrom, &Packages.Storage, "/appdata/packages"},
246+
{loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/archives"},
247+
{loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"},
248+
{loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"},
249+
{loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"},
250+
})
251+
}

0 commit comments

Comments
 (0)