From 5aa6a8288d997d1505caa8c8c7f85c27863673b3 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Fri, 4 Aug 2023 14:24:13 +0800 Subject: [PATCH] Fix the wrong derive path (#26271) (#26318) Backport #26271 by @lunny This PR will fix #26264, caused by #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: Lunny Xiao Co-authored-by: wxiaoguang (cherry picked from commit 88f6f7579cdaa557333bc86b3e45bf6458d889b6) --- modules/setting/storage.go | 73 +++++++++++---- modules/setting/storage_test.go | 159 ++++++++++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 17 deletions(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index ed804a910a..c28f2be4ed 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -84,12 +84,15 @@ func getDefaultStorageSection(rootCfg ConfigProvider) ConfigSection { return storageSec } +// getStorage will find target section and extra special section first and then read override +// items from extra section func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*Storage, error) { if name == "" { return nil, errors.New("no name for storage") } var targetSec ConfigSection + // check typ first if typ != "" { var err error targetSec, err = rootCfg.GetSection(storageSectionName + "." + typ) @@ -111,24 +114,40 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S } } - storageNameSec, _ := rootCfg.GetSection(storageSectionName + "." + name) - - if targetSec == nil { - targetSec = sec + if targetSec == nil && sec != nil { + secTyp := sec.Key("STORAGE_TYPE").String() + if IsValidStorageType(StorageType(secTyp)) { + targetSec = sec + } else if secTyp != "" { + targetSec, _ = rootCfg.GetSection(storageSectionName + "." + secTyp) + } } + + targetSecIsStoragename := false + storageNameSec, _ := rootCfg.GetSection(storageSectionName + "." + name) if targetSec == nil { targetSec = storageNameSec + targetSecIsStoragename = storageNameSec != nil } + if targetSec == nil { targetSec = getDefaultStorageSection(rootCfg) } else { targetType := targetSec.Key("STORAGE_TYPE").String() switch { case targetType == "": - if targetSec.Key("PATH").String() == "" { - targetSec = getDefaultStorageSection(rootCfg) + if targetSec != storageNameSec && storageNameSec != nil { + targetSec = storageNameSec + targetSecIsStoragename = true + if targetSec.Key("STORAGE_TYPE").String() == "" { + return nil, fmt.Errorf("storage section %s.%s has no STORAGE_TYPE", storageSectionName, name) + } } else { - targetSec.Key("STORAGE_TYPE").SetValue("local") + if targetSec.Key("PATH").String() == "" { + targetSec = getDefaultStorageSection(rootCfg) + } else { + targetSec.Key("STORAGE_TYPE").SetValue("local") + } } default: newTargetSec, _ := rootCfg.GetSection(storageSectionName + "." + targetType) @@ -153,26 +172,46 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S return nil, fmt.Errorf("invalid storage type %q", targetType) } + // extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible + extraConfigSec := sec + if extraConfigSec == nil { + extraConfigSec = storageNameSec + } + var storage Storage storage.Type = StorageType(targetType) switch targetType { case string(LocalStorageType): - storage.Path = ConfigSectionKeyString(targetSec, "PATH", filepath.Join(AppDataPath, name)) - if !filepath.IsAbs(storage.Path) { - storage.Path = filepath.Join(AppWorkPath, storage.Path) + targetPath := ConfigSectionKeyString(targetSec, "PATH", "") + if targetPath == "" { + targetPath = AppDataPath + } else if !filepath.IsAbs(targetPath) { + targetPath = filepath.Join(AppDataPath, targetPath) } - case string(MinioStorageType): - storage.MinioConfig.BasePath = name + "/" + var fallbackPath string + if targetSecIsStoragename { + fallbackPath = targetPath + } else { + fallbackPath = filepath.Join(targetPath, name) + } + + if extraConfigSec == nil { + storage.Path = fallbackPath + } else { + storage.Path = ConfigSectionKeyString(extraConfigSec, "PATH", fallbackPath) + if !filepath.IsAbs(storage.Path) { + storage.Path = filepath.Join(targetPath, storage.Path) + } + } + + case string(MinioStorageType): if err := targetSec.MapTo(&storage.MinioConfig); err != nil { return nil, fmt.Errorf("map minio config failed: %v", err) } - // extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH to override the targetsec - extraConfigSec := sec - if extraConfigSec == nil { - extraConfigSec = storageNameSec - } + + storage.MinioConfig.BasePath = name + "/" if extraConfigSec != nil { storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(extraConfigSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect) diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index 4eda7646e8..9a83f31d91 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -4,6 +4,7 @@ package setting import ( + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -90,3 +91,161 @@ STORAGE_TYPE = minio assert.EqualValues(t, "gitea", RepoAvatar.Storage.MinioConfig.Bucket) assert.EqualValues(t, "repo-avatars/", RepoAvatar.Storage.MinioConfig.BasePath) } + +type testLocalStoragePathCase struct { + loader func(rootCfg ConfigProvider) error + storagePtr **Storage + expectedPath string +} + +func testLocalStoragePath(t *testing.T, appDataPath, iniStr string, cases []testLocalStoragePathCase) { + cfg, err := NewConfigProviderFromData(iniStr) + assert.NoError(t, err) + AppDataPath = appDataPath + for _, c := range cases { + assert.NoError(t, c.loader(cfg)) + storage := *c.storagePtr + + assert.EqualValues(t, "local", storage.Type) + assert.True(t, filepath.IsAbs(storage.Path)) + assert.EqualValues(t, filepath.Clean(c.expectedPath), filepath.Clean(storage.Path)) + } +} + +func Test_getStorageInheritStorageTypeLocal(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage] +STORAGE_TYPE = local +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/repo-archive"}, + {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalPath(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage] +STORAGE_TYPE = local +PATH = /data/gitea +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/repo-archive"}, + {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalRelativePath(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage] +STORAGE_TYPE = local +PATH = storages +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/appdata/storages/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/storages/repo-archive"}, + {loadActionsFrom, &Actions.LogStorage, "/appdata/storages/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/appdata/storages/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/storages/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalPathOverride(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage] +STORAGE_TYPE = local +PATH = /data/gitea + +[repo-archive] +PATH = /data/gitea/the-archives-dir +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/the-archives-dir"}, + {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalPathOverrideEmpty(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage] +STORAGE_TYPE = local +PATH = /data/gitea + +[repo-archive] +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/repo-archive"}, + {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalRelativePathOverride(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage] +STORAGE_TYPE = local +PATH = /data/gitea + +[repo-archive] +PATH = the-archives-dir +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/the-archives-dir"}, + {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/data/gitea/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/data/gitea/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalPathOverride3(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage.repo-archive] +STORAGE_TYPE = local +PATH = /data/gitea/archives +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/archives"}, + {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalPathOverride4(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage.repo-archive] +STORAGE_TYPE = local +PATH = /data/gitea/archives + +[repo-archive] +PATH = /tmp/gitea/archives +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/tmp/gitea/archives"}, + {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"}, + }) +} + +func Test_getStorageInheritStorageTypeLocalPathOverride5(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage.repo-archive] +STORAGE_TYPE = local +PATH = /data/gitea/archives + +[repo-archive] +`, []testLocalStoragePathCase{ + {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/archives"}, + {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"}, + }) +}