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 <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

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 && sec != nil {
-
+		secTyp := sec.Key("STORAGE_TYPE").String()
-	if targetSec == nil {
+		if IsValidStorageType(StorageType(secTyp)) {
-		targetSec = sec
+			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() == "" {
+			if targetSec != storageNameSec && storageNameSec != nil {
-				targetSec = getDefaultStorageSection(rootCfg)
+				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))
+		targetPath := ConfigSectionKeyString(targetSec, "PATH", "")
-		if !filepath.IsAbs(storage.Path) {
+		if targetPath == "" {
-			storage.Path = filepath.Join(AppWorkPath, storage.Path)
+			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
+		storage.MinioConfig.BasePath = name + "/"
-		if extraConfigSec == nil {
-			extraConfigSec = storageNameSec
-		}
 
 		if extraConfigSec != nil {
 			storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(extraConfigSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect)

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"},
+	})
+}