From 26b538f13dfd44b9535b9715cf485ef5453f7366 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 4 Aug 2023 11:41:16 +0800 Subject: [PATCH] Fix the wrong derive path (#26271) 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: wxiaoguang --- 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 ed804a910a461..c28f2be4ed714 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,27 +172,47 @@ 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 + "/" - if err := targetSec.MapTo(&storage.MinioConfig); err != nil { - return nil, fmt.Errorf("map minio config failed: %v", err) + var fallbackPath string + if targetSecIsStoragename { + fallbackPath = targetPath + } else { + fallbackPath = filepath.Join(targetPath, name) } - // extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH to override the targetsec - extraConfigSec := sec + if extraConfigSec == nil { - extraConfigSec = storageNameSec + 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) } + storage.MinioConfig.BasePath = name + "/" + if extraConfigSec != nil { storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(extraConfigSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect) storage.MinioConfig.BasePath = ConfigSectionKeyString(extraConfigSec, "MINIO_BASE_PATH", storage.MinioConfig.BasePath) diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index 4eda7646e81d0..9a83f31d918b4 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"}, + }) +}