From de4a21fcb4476772c69c36d086549e89ed4dcf6c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 2 Jun 2023 17:27:30 +0800 Subject: [PATCH] Refactor INI package (first step) (#25024) The INI package has many bugs and quirks, and in fact it is unmaintained. This PR is the first step for the INI package refactoring: * Use Gitea's "config_provider" to provide INI access * Deprecate the INI package by golangci.yml rule --- .golangci.yml | 1 + build/backport-locales.go | 12 +- .../environment-to-ini/environment-to-ini.go | 15 +- modules/repository/repo.go | 4 +- modules/setting/config_env.go | 4 +- modules/setting/config_env_test.go | 7 +- modules/setting/config_provider.go | 196 +++++++++++++----- modules/setting/config_provider_test.go | 55 +++++ modules/setting/setting.go | 2 +- modules/translation/i18n/localestore.go | 9 +- routers/install/install.go | 13 +- 11 files changed, 219 insertions(+), 99 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 342fe97837..22de387fac 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -86,6 +86,7 @@ linters-settings: - io/ioutil: "use os or io instead" - golang.org/x/exp: "it's experimental and unreliable." - code.gitea.io/gitea/modules/git/internal: "do not use the internal package, use AddXxx function instead" + - gopkg.in/ini.v1: "do not use the ini package, use gitea's config system instead" issues: max-issues-per-linter: 0 diff --git a/build/backport-locales.go b/build/backport-locales.go index 054b623d69..0346215348 100644 --- a/build/backport-locales.go +++ b/build/backport-locales.go @@ -12,7 +12,7 @@ import ( "path/filepath" "strings" - "gopkg.in/ini.v1" + "code.gitea.io/gitea/modules/setting" ) func main() { @@ -22,14 +22,13 @@ func main() { os.Exit(1) } - ini.PrettyFormat = false mustNoErr := func(err error) { if err != nil { panic(err) } } - collectInis := func(ref string) map[string]*ini.File { - inis := map[string]*ini.File{} + collectInis := func(ref string) map[string]setting.ConfigProvider { + inis := map[string]setting.ConfigProvider{} err := filepath.WalkDir("options/locale", func(path string, d os.DirEntry, err error) error { if err != nil { return err @@ -37,10 +36,7 @@ func main() { if d.IsDir() || !strings.HasSuffix(d.Name(), ".ini") { return nil } - cfg, err := ini.LoadSources(ini.LoadOptions{ - IgnoreInlineComment: true, - UnescapeValueCommentSymbols: true, - }, path) + cfg, err := setting.NewConfigProviderForLocale(path) mustNoErr(err) inis[path] = cfg fmt.Printf("collecting: %s @ %s\n", path, ref) diff --git a/contrib/environment-to-ini/environment-to-ini.go b/contrib/environment-to-ini/environment-to-ini.go index ae8535d891..3405d7d429 100644 --- a/contrib/environment-to-ini/environment-to-ini.go +++ b/contrib/environment-to-ini/environment-to-ini.go @@ -9,10 +9,8 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" "github.com/urfave/cli" - "gopkg.in/ini.v1" ) // EnvironmentPrefix environment variables prefixed with this represent ini values to write @@ -97,19 +95,10 @@ func runEnvironmentToIni(c *cli.Context) error { providedWorkPath := c.String("work-path") setting.SetCustomPathAndConf(providedCustom, providedConf, providedWorkPath) - cfg := ini.Empty() - confFileExists, err := util.IsFile(setting.CustomConf) + cfg, err := setting.NewConfigProviderFromFile(&setting.Options{CustomConf: setting.CustomConf, AllowEmpty: true}) if err != nil { - log.Fatal("Unable to check if %s is a file. Error: %v", setting.CustomConf, err) + log.Fatal("Failed to load custom conf '%s': %v", setting.CustomConf, err) } - if confFileExists { - if err := cfg.Append(setting.CustomConf); err != nil { - log.Fatal("Failed to load custom conf '%s': %v", setting.CustomConf, err) - } - } else { - log.Warn("Custom config '%s' not found, ignore this if you're running first time", setting.CustomConf) - } - cfg.NameMapper = ini.SnackCase prefixGitea := c.String("prefix") + "__" suffixFile := "__FILE" diff --git a/modules/repository/repo.go b/modules/repository/repo.go index 62e1f31b9e..3991e9ad99 100644 --- a/modules/repository/repo.go +++ b/modules/repository/repo.go @@ -27,7 +27,7 @@ import ( "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" - "gopkg.in/ini.v1" + "gopkg.in/ini.v1" //nolint:depguard ) /* @@ -241,7 +241,7 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User, // cleanUpMigrateGitConfig removes mirror info which prevents "push --all". // This also removes possible user credentials. func cleanUpMigrateGitConfig(configPath string) error { - cfg, err := ini.Load(configPath) + cfg, err := ini.Load(configPath) // FIXME: the ini package doesn't really work with git config files if err != nil { return fmt.Errorf("open config file: %w", err) } diff --git a/modules/setting/config_env.go b/modules/setting/config_env.go index dca9f2bb47..6348803705 100644 --- a/modules/setting/config_env.go +++ b/modules/setting/config_env.go @@ -10,8 +10,6 @@ import ( "strings" "code.gitea.io/gitea/modules/log" - - "gopkg.in/ini.v1" ) const escapeRegexpString = "_0[xX](([0-9a-fA-F][0-9a-fA-F])+)_" @@ -89,7 +87,7 @@ func decodeEnvironmentKey(prefixGitea, suffixFile, envKey string) (ok bool, sect return ok, section, key, useFileValue } -func EnvironmentToConfig(cfg *ini.File, prefixGitea, suffixFile string, envs []string) (changed bool) { +func EnvironmentToConfig(cfg ConfigProvider, prefixGitea, suffixFile string, envs []string) (changed bool) { for _, kv := range envs { idx := strings.IndexByte(kv, '=') if idx < 0 { diff --git a/modules/setting/config_env_test.go b/modules/setting/config_env_test.go index d49464ecf7..d574554bcc 100644 --- a/modules/setting/config_env_test.go +++ b/modules/setting/config_env_test.go @@ -8,7 +8,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "gopkg.in/ini.v1" ) func TestDecodeEnvSectionKey(t *testing.T) { @@ -71,15 +70,15 @@ func TestDecodeEnvironmentKey(t *testing.T) { } func TestEnvironmentToConfig(t *testing.T) { - cfg := ini.Empty() + cfg, _ := NewConfigProviderFromData("") changed := EnvironmentToConfig(cfg, "GITEA__", "__FILE", nil) assert.False(t, changed) - cfg, err := ini.Load([]byte(` + cfg, err := NewConfigProviderFromData(` [sec] key = old -`)) +`) assert.NoError(t, err) changed = EnvironmentToConfig(cfg, "GITEA__", "__FILE", []string{"GITEA__sec__key=new"}) diff --git a/modules/setting/config_provider.go b/modules/setting/config_provider.go index 37f5754ffd..8b317d94e3 100644 --- a/modules/setting/config_provider.go +++ b/modules/setting/config_provider.go @@ -8,36 +8,71 @@ import ( "os" "path/filepath" "strings" + "time" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/util" - ini "gopkg.in/ini.v1" + "gopkg.in/ini.v1" //nolint:depguard ) +type ConfigKey interface { + Name() string + Value() string + SetValue(v string) + + In(defaultVal string, candidates []string) string + String() string + Strings(delim string) []string + + MustString(defaultVal string) string + MustBool(defaultVal ...bool) bool + MustInt(defaultVal ...int) int + MustInt64(defaultVal ...int64) int64 + MustDuration(defaultVal ...time.Duration) time.Duration +} + type ConfigSection interface { Name() string - MapTo(interface{}) error + MapTo(any) error HasKey(key string) bool - NewKey(name, value string) (*ini.Key, error) - Key(key string) *ini.Key - Keys() []*ini.Key - ChildSections() []*ini.Section + NewKey(name, value string) (ConfigKey, error) + Key(key string) ConfigKey + Keys() []ConfigKey + ChildSections() []ConfigSection } // ConfigProvider represents a config provider type ConfigProvider interface { Section(section string) ConfigSection + Sections() []ConfigSection NewSection(name string) (ConfigSection, error) GetSection(name string) (ConfigSection, error) Save() error + SaveTo(filename string) error } +type iniConfigProvider struct { + opts *Options + ini *ini.File + newFile bool // whether the file has not existed previously +} + +type iniConfigSection struct { + sec *ini.Section +} + +var ( + _ ConfigProvider = (*iniConfigProvider)(nil) + _ ConfigSection = (*iniConfigSection)(nil) + _ ConfigKey = (*ini.Key)(nil) +) + // ConfigSectionKey only searches the keys in the given section, but it is O(n). // ini package has a special behavior: with "[sec] a=1" and an empty "[sec.sub]", // then in "[sec.sub]", Key()/HasKey() can always see "a=1" because it always tries parent sections. // It returns nil if the key doesn't exist. -func ConfigSectionKey(sec ConfigSection, key string) *ini.Key { +func ConfigSectionKey(sec ConfigSection, key string) ConfigKey { if sec == nil { return nil } @@ -64,7 +99,7 @@ func ConfigSectionKeyString(sec ConfigSection, key string, def ...string) string // and the returned key is safe to be used with "MustXxx", it doesn't change the parent's values. // Otherwise, ini.Section.Key().MustXxx would pollute the parent section's keys. // It never returns nil. -func ConfigInheritedKey(sec ConfigSection, key string) *ini.Key { +func ConfigInheritedKey(sec ConfigSection, key string) ConfigKey { k := sec.Key(key) if k != nil && k.String() != "" { newKey, _ := sec.NewKey(k.Name(), k.String()) @@ -85,41 +120,64 @@ func ConfigInheritedKeyString(sec ConfigSection, key string, def ...string) stri return "" } -type iniFileConfigProvider struct { - opts *Options - *ini.File - newFile bool // whether the file has not existed previously +func (s *iniConfigSection) Name() string { + return s.sec.Name() } -// NewConfigProviderFromData this function is only for testing +func (s *iniConfigSection) MapTo(v any) error { + return s.sec.MapTo(v) +} + +func (s *iniConfigSection) HasKey(key string) bool { + return s.sec.HasKey(key) +} + +func (s *iniConfigSection) NewKey(name, value string) (ConfigKey, error) { + return s.sec.NewKey(name, value) +} + +func (s *iniConfigSection) Key(key string) ConfigKey { + return s.sec.Key(key) +} + +func (s *iniConfigSection) Keys() (keys []ConfigKey) { + for _, k := range s.sec.Keys() { + keys = append(keys, k) + } + return keys +} + +func (s *iniConfigSection) ChildSections() (sections []ConfigSection) { + for _, s := range s.sec.ChildSections() { + sections = append(sections, &iniConfigSection{s}) + } + return sections +} + +// NewConfigProviderFromData this function is mainly for testing purpose func NewConfigProviderFromData(configContent string) (ConfigProvider, error) { - var cfg *ini.File - var err error - if configContent == "" { - cfg = ini.Empty() - } else { - cfg, err = ini.Load(strings.NewReader(configContent)) - if err != nil { - return nil, err - } + cfg, err := ini.Load(strings.NewReader(configContent)) + if err != nil { + return nil, err } cfg.NameMapper = ini.SnackCase - return &iniFileConfigProvider{ - File: cfg, + return &iniConfigProvider{ + ini: cfg, newFile: true, }, nil } type Options struct { - CustomConf string // the ini file path - AllowEmpty bool // whether not finding configuration files is allowed (only true for the tests) - ExtraConfig string - DisableLoadCommonSettings bool + CustomConf string // the ini file path + AllowEmpty bool // whether not finding configuration files is allowed + ExtraConfig string + + DisableLoadCommonSettings bool // only used by "Init()", not used by "NewConfigProvider()" } -// newConfigProviderFromFile load configuration from file. +// NewConfigProviderFromFile load configuration from file. // NOTE: do not print any log except error. -func newConfigProviderFromFile(opts *Options) (*iniFileConfigProvider, error) { +func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) { cfg := ini.Empty() newFile := true @@ -147,61 +205,77 @@ func newConfigProviderFromFile(opts *Options) (*iniFileConfigProvider, error) { } cfg.NameMapper = ini.SnackCase - return &iniFileConfigProvider{ + return &iniConfigProvider{ opts: opts, - File: cfg, + ini: cfg, newFile: newFile, }, nil } -func (p *iniFileConfigProvider) Section(section string) ConfigSection { - return p.File.Section(section) +func (p *iniConfigProvider) Section(section string) ConfigSection { + return &iniConfigSection{sec: p.ini.Section(section)} } -func (p *iniFileConfigProvider) NewSection(name string) (ConfigSection, error) { - return p.File.NewSection(name) +func (p *iniConfigProvider) Sections() (sections []ConfigSection) { + for _, s := range p.ini.Sections() { + sections = append(sections, &iniConfigSection{s}) + } + return sections } -func (p *iniFileConfigProvider) GetSection(name string) (ConfigSection, error) { - return p.File.GetSection(name) +func (p *iniConfigProvider) NewSection(name string) (ConfigSection, error) { + sec, err := p.ini.NewSection(name) + if err != nil { + return nil, err + } + return &iniConfigSection{sec: sec}, nil } -// Save save the content into file -func (p *iniFileConfigProvider) Save() error { - if p.opts.CustomConf == "" { +func (p *iniConfigProvider) GetSection(name string) (ConfigSection, error) { + sec, err := p.ini.GetSection(name) + if err != nil { + return nil, err + } + return &iniConfigSection{sec: sec}, nil +} + +// Save saves the content into file +func (p *iniConfigProvider) Save() error { + filename := p.opts.CustomConf + if filename == "" { if !p.opts.AllowEmpty { return fmt.Errorf("custom config path must not be empty") } return nil } - if p.newFile { - if err := os.MkdirAll(filepath.Dir(CustomConf), os.ModePerm); err != nil { - return fmt.Errorf("failed to create '%s': %v", CustomConf, err) + if err := os.MkdirAll(filepath.Dir(filename), os.ModePerm); err != nil { + return fmt.Errorf("failed to create '%s': %v", filename, err) } } - if err := p.SaveTo(p.opts.CustomConf); err != nil { - return fmt.Errorf("failed to save '%s': %v", p.opts.CustomConf, err) + if err := p.ini.SaveTo(filename); err != nil { + return fmt.Errorf("failed to save '%s': %v", filename, err) } // Change permissions to be more restrictive - fi, err := os.Stat(CustomConf) + fi, err := os.Stat(filename) if err != nil { return fmt.Errorf("failed to determine current conf file permissions: %v", err) } if fi.Mode().Perm() > 0o600 { - if err = os.Chmod(CustomConf, 0o600); err != nil { + if err = os.Chmod(filename, 0o600); err != nil { log.Warn("Failed changing conf file permissions to -rw-------. Consider changing them manually.") } } return nil } -// a file is an implementation ConfigProvider and other implementations are possible, i.e. from docker, k8s, … -var _ ConfigProvider = &iniFileConfigProvider{} +func (p *iniConfigProvider) SaveTo(filename string) error { + return p.ini.SaveTo(filename) +} -func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting interface{}) { +func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting any) { if err := rootCfg.Section(sectionName).MapTo(setting); err != nil { log.Fatal("Failed to map %s settings: %v", sectionName, err) } @@ -219,3 +293,23 @@ func deprecatedSettingDB(rootCfg ConfigProvider, oldSection, oldKey string) { log.Error("Deprecated `[%s]` `%s` present which has been copied to database table sys_setting", oldSection, oldKey) } } + +// NewConfigProviderForLocale loads locale configuration from source and others. "string" if for a local file path, "[]byte" is for INI content +func NewConfigProviderForLocale(source any, others ...any) (ConfigProvider, error) { + iniFile, err := ini.LoadSources(ini.LoadOptions{ + IgnoreInlineComment: true, + UnescapeValueCommentSymbols: true, + }, source, others...) + if err != nil { + return nil, fmt.Errorf("unable to load locale ini: %w", err) + } + iniFile.BlockMode = false + return &iniConfigProvider{ + ini: iniFile, + newFile: true, + }, nil +} + +func init() { + ini.PrettyFormat = false +} diff --git a/modules/setting/config_provider_test.go b/modules/setting/config_provider_test.go index 76f7048d59..17650edea4 100644 --- a/modules/setting/config_provider_test.go +++ b/modules/setting/config_provider_test.go @@ -4,6 +4,7 @@ package setting import ( + "os" "testing" "github.com/stretchr/testify/assert" @@ -64,3 +65,57 @@ key = 123 assert.Equal(t, "", ConfigSectionKeyString(sec, "empty")) assert.Equal(t, "def", ConfigSectionKeyString(secSub, "empty")) } + +func TestNewConfigProviderFromFile(t *testing.T) { + _, err := NewConfigProviderFromFile(&Options{CustomConf: "no-such.ini", AllowEmpty: false}) + assert.ErrorContains(t, err, "unable to find configuration file") + + // load non-existing file and save + testFile := t.TempDir() + "/test.ini" + testFile1 := t.TempDir() + "/test1.ini" + cfg, err := NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true}) + assert.NoError(t, err) + + sec, _ := cfg.NewSection("foo") + _, _ = sec.NewKey("k1", "a") + assert.NoError(t, cfg.Save()) + _, _ = sec.NewKey("k2", "b") + assert.NoError(t, cfg.SaveTo(testFile1)) + + bs, err := os.ReadFile(testFile) + assert.NoError(t, err) + assert.Equal(t, "[foo]\nk1=a\n", string(bs)) + + bs, err = os.ReadFile(testFile1) + assert.NoError(t, err) + assert.Equal(t, "[foo]\nk1=a\nk2=b\n", string(bs)) + + // load existing file and save + cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true}) + assert.NoError(t, err) + assert.Equal(t, "a", cfg.Section("foo").Key("k1").String()) + sec, _ = cfg.NewSection("bar") + _, _ = sec.NewKey("k1", "b") + assert.NoError(t, cfg.Save()) + bs, err = os.ReadFile(testFile) + assert.NoError(t, err) + assert.Equal(t, "[foo]\nk1=a\n\n[bar]\nk1=b\n", string(bs)) +} + +func TestNewConfigProviderForLocale(t *testing.T) { + // load locale from file + localeFile := t.TempDir() + "/locale.ini" + _ = os.WriteFile(localeFile, []byte(`k1=a`), 0o644) + cfg, err := NewConfigProviderForLocale(localeFile) + assert.NoError(t, err) + assert.Equal(t, "a", cfg.Section("").Key("k1").String()) + + // load locale from bytes + cfg, err = NewConfigProviderForLocale([]byte("k1=foo\nk2=bar")) + assert.NoError(t, err) + assert.Equal(t, "foo", cfg.Section("").Key("k1").String()) + cfg, err = NewConfigProviderForLocale([]byte("k1=foo\nk2=bar"), []byte("k2=xxx")) + assert.NoError(t, err) + assert.Equal(t, "foo", cfg.Section("").Key("k1").String()) + assert.Equal(t, "xxx", cfg.Section("").Key("k2").String()) +} diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 71cd9a12a9..1967d9e79a 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -201,7 +201,7 @@ func Init(opts *Options) { opts.CustomConf = CustomConf } var err error - CfgProvider, err = newConfigProviderFromFile(opts) + CfgProvider, err = NewConfigProviderFromFile(opts) if err != nil { log.Fatal("Init[%v]: %v", opts, err) } diff --git a/modules/translation/i18n/localestore.go b/modules/translation/i18n/localestore.go index 0664bcfd1a..aa784e866f 100644 --- a/modules/translation/i18n/localestore.go +++ b/modules/translation/i18n/localestore.go @@ -7,8 +7,7 @@ import ( "fmt" "code.gitea.io/gitea/modules/log" - - "gopkg.in/ini.v1" + "code.gitea.io/gitea/modules/setting" ) // This file implements the static LocaleStore that will not watch for changes @@ -47,14 +46,10 @@ func (store *localeStore) AddLocaleByIni(langName, langDesc string, source, more l := &locale{store: store, langName: langName, idxToMsgMap: make(map[int]string)} store.localeMap[l.langName] = l - iniFile, err := ini.LoadSources(ini.LoadOptions{ - IgnoreInlineComment: true, - UnescapeValueCommentSymbols: true, - }, source, moreSource) + iniFile, err := setting.NewConfigProviderForLocale(source, moreSource) if err != nil { return fmt.Errorf("unable to load ini: %w", err) } - iniFile.BlockMode = false for _, section := range iniFile.Sections() { for _, key := range section.Keys() { diff --git a/routers/install/install.go b/routers/install/install.go index 4635cd7cb6..51ad6ec378 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -35,7 +35,6 @@ import ( "code.gitea.io/gitea/services/forms" "gitea.com/go-chi/session" - "gopkg.in/ini.v1" ) const ( @@ -371,17 +370,11 @@ func SubmitInstall(ctx *context.Context) { } // Save settings. - cfg := ini.Empty() - isFile, err := util.IsFile(setting.CustomConf) + cfg, err := setting.NewConfigProviderFromFile(&setting.Options{CustomConf: setting.CustomConf, AllowEmpty: true}) if err != nil { - log.Error("Unable to check if %s is a file. Error: %v", setting.CustomConf, err) - } - if isFile { - // Keeps custom settings if there is already something. - if err = cfg.Append(setting.CustomConf); err != nil { - log.Error("Failed to load custom conf '%s': %v", setting.CustomConf, err) - } + log.Error("Failed to load custom conf '%s': %v", setting.CustomConf, err) } + cfg.Section("database").Key("DB_TYPE").SetValue(setting.Database.Type.String()) cfg.Section("database").Key("HOST").SetValue(setting.Database.Host) cfg.Section("database").Key("NAME").SetValue(setting.Database.Name)