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 <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,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))
+		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 + "/"
 
-		if err := targetSec.MapTo(&storage.MinioConfig); err != nil {
+		var fallbackPath string
-			return nil, fmt.Errorf("map minio config failed: %v", err)
+		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)

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