feat: adds setter for config.Value and updates forms

Install now submits the proper database name and is properly set using
the config.Value class. This extends the getter functionality so now
config.Value can be used to both get and set values.

Author: markbrown87

models/system/setting.go

@@ -150,3 +150,34 @@ func (d *dbConfigCachedGetter) InvalidateCache() {
 func NewDatabaseDynKeyGetter() config.DynKeyGetter {
 	return &dbConfigCachedGetter{}
 }
+
+type dbConfigSetter struct {
+	mu sync.RWMutex
+}
+
+var _ config.DynKeySetter = (*dbConfigSetter)(nil)
+
+func (d *dbConfigSetter) SetValue(ctx context.Context, dynKey, value string) error {
+	d.mu.RLock()
+	defer d.mu.RUnlock()
+	_ = GetRevision(ctx) // prepare the "revision" key ahead
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		e := db.GetEngine(ctx)
+		res, err := e.Exec("UPDATE system_setting SET version=version+1, setting_value=? WHERE setting_key=?", value, dynKey)
+		if err != nil {
+			return err
+		}
+		rows, _ := res.RowsAffected()
+		if rows == 0 { // if no existing row, insert a new row
+			if _, err = e.Insert(&Setting{SettingKey: dynKey, SettingValue: value}); err != nil {
+				return err
+			}
+		}
+
+		return nil
+	})
+}
+
+func NewDatabaseDynKeySetter() config.DynKeySetter {
+	return &dbConfigSetter{}
+}

modules/setting/config.go

@@ -58,15 +58,15 @@ type ConfigStruct struct {
 }
 
 var (
-	defaultConfig     *ConfigStruct
-	defaultConfigOnce sync.Once
+	defaultConfig *ConfigStruct
+	ConfigOnce    sync.Once
 )
 
 func initDefaultConfig() {
 	config.SetCfgSecKeyGetter(&cfgSecKeyGetter{})
 	defaultConfig = &ConfigStruct{
 		Picture: &PictureStruct{
-			EnableGravatar:        config.ValueJSON[bool]("picture.disable_gravatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "DISABLE_GRAVATAR"}).Invert(),
+			EnableGravatar:        config.ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "DISABLE_GRAVATAR"}).Invert(),
 			EnableFederatedAvatar: config.ValueJSON[bool]("picture.enable_federated_avatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "ENABLE_FEDERATED_AVATAR"}),
 		},
 		Repository: &RepositoryStruct{
@@ -77,7 +77,7 @@ func initDefaultConfig() {
 }
 
 func Config() *ConfigStruct {
-	defaultConfigOnce.Do(initDefaultConfig)
+	ConfigOnce.Do(initDefaultConfig)
 	return defaultConfig
 }
 

modules/setting/config/setter.go

@@ -0,0 +1,29 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package config
+
+import (
+	"context"
+	"sync"
+)
+
+var setterMu sync.RWMutex
+
+type DynKeySetter interface {
+	SetValue(ctx context.Context, dynKey, value string) error
+}
+
+var dynKeySetterInternal DynKeySetter
+
+func SetDynSetter(p DynKeySetter) {
+	setterMu.Lock()
+	dynKeySetterInternal = p
+	setterMu.Unlock()
+}
+
+func GetDynSetter() DynKeySetter {
+	getterMu.RLock()
+	defer getterMu.RUnlock()
+	return dynKeySetterInternal
+}

modules/setting/config/value.go

@@ -5,6 +5,7 @@ package config
 
 import (
 	"context"
+	"fmt"
 	"sync"
 
 	"code.gitea.io/gitea/modules/json"
@@ -19,8 +20,8 @@ type CfgSecKey struct {
 type Value[T any] struct {
 	mu sync.RWMutex
 
-	cfgSecKey CfgSecKey
-	dynKey    string
+	cfgSecKey             CfgSecKey
+	dynKey, selectFromKey string
 
 	def, value  T
 	revision    int
@@ -35,19 +36,41 @@ func (value *Value[T]) parse(key, valStr string) (v T) {
 		}
 	}
 
+	return value.invert(v)
+}
+
+func (value *Value[T]) invertBoolStr(val string) (inverted string) {
+	if val == "true" {
+		return "false"
+	}
+
+	return "true"
+}
+
+func (value *Value[T]) invert(val T) (v T) {
+	v = val
 	if value.flipBoolean {
+		fmt.Printf("Flipping boolean value '%v'...\n", val)
 		// if value is of type bool
-		if _, ok := any(v).(bool); ok {
+		if _, ok := any(val).(bool); ok {
 			// invert the boolean value upon retrieval
-			v = any(!any(v).(bool)).(T)
+			v = any(!any(val).(bool)).(T)
 		} else {
-			log.Warn("Ignoring attempt to invert key '%q' for non boolean type", key)
+			log.Warn("Ignoring attempt to invert key '%q' for non boolean type", value.selectFromKey)
 		}
 	}
 
 	return v
 }
 
+func (value *Value[T]) getKey() string {
+	if value.selectFromKey != "" {
+		return value.selectFromKey
+	}
+
+	return value.dynKey
+}
+
 func (value *Value[T]) Value(ctx context.Context) (v T) {
 	dg := GetDynGetter()
 	if dg == nil {
@@ -69,7 +92,7 @@ func (value *Value[T]) Value(ctx context.Context) (v T) {
 
 	// try to parse the config and cache it
 	var valStr *string
-	if dynVal, has := dg.GetValue(ctx, value.dynKey); has {
+	if dynVal, has := dg.GetValue(ctx, value.getKey()); has {
 		valStr = &dynVal
 	} else if cfgVal, has := GetCfgSecKeyGetter().GetValue(value.cfgSecKey.Sec, value.cfgSecKey.Key); has {
 		valStr = &cfgVal
@@ -91,6 +114,10 @@ func (value *Value[T]) DynKey() string {
 	return value.dynKey
 }
 
+func (value *Value[T]) SelectFromKey() string {
+	return value.selectFromKey
+}
+
 func (value *Value[T]) WithDefault(def T) *Value[T] {
 	value.def = def
 	return value
@@ -110,6 +137,29 @@ func (value *Value[bool]) Invert() *Value[bool] {
 	return value
 }
 
+func (value *Value[any]) SelectFrom(sectionName string) *Value[any] {
+	value.selectFromKey = sectionName
+	return value
+}
+
+func (value *Value[any]) SetValue(val string) error {
+	ctx := context.Background()
+	ds := GetDynSetter()
+	if ds == nil {
+		// this is an edge case: the database is not initialized but the system setting is going to be used
+		// it should panic to avoid inconsistent config values (from config / system setting) and fix the code
+		panic("no config dyn value getter")
+	}
+
+	fmt.Printf("Setting value '%s' with old key '%s' using key '%s'\n", val, value.selectFromKey, value.dynKey)
+
+	if value.flipBoolean {
+		return ds.SetValue(ctx, value.getKey(), value.invertBoolStr(val))
+	}
+
+	return ds.SetValue(ctx, value.getKey(), val)
+}
+
 func ValueJSON[T any](dynKey string) *Value[T] {
 	return &Value[T]{dynKey: dynKey}
 }

modules/setting/config/value_test.go

@@ -33,3 +33,31 @@ func TestValue_parse(t *testing.T) {
 		})
 	}
 }
+
+func TestValue_getKey(t *testing.T) {
+	tests := []struct {
+		name       string // description of this test case
+		valueClass *Value[bool]
+		want       string
+	}{
+		{
+			name:       "Custom dynKey name",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar").WithFileConfig(CfgSecKey{Sec: "", Key: ""}),
+			want:       "picture.enable_gravatar",
+		},
+		{
+			name:       "Normal dynKey name",
+			valueClass: ValueJSON[bool]("picture.disable_gravatar").WithFileConfig(CfgSecKey{Sec: "", Key: ""}),
+			want:       "picture.disable_gravatar",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.valueClass.getKey()
+
+			if got != tt.want {
+				t.Errorf("getKey() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

routers/common/db.go

@@ -38,6 +38,7 @@ func InitDBEngine(ctx context.Context) (err error) {
 		log.Info("Backing off for %d seconds", int64(setting.Database.DBConnectBackoff/time.Second))
 		time.Sleep(setting.Database.DBConnectBackoff)
 	}
+	config.SetDynSetter(system_model.NewDatabaseDynKeySetter())
 	config.SetDynGetter(system_model.NewDatabaseDynKeyGetter())
 	return nil
 }

routers/install/install.go

@@ -427,7 +427,8 @@ func SubmitInstall(ctx *context.Context) {
 
 	cfg.Section("server").Key("OFFLINE_MODE").SetValue(strconv.FormatBool(form.OfflineMode))
 	if err := system_model.SetSettings(ctx, map[string]string{
-		setting.Config().Picture.EnableGravatar.DynKey():        strconv.FormatBool(!form.EnableGravatar), // invert value as it is stored as disable_gravatar for backwards compatability
+		// Form is submitted on install and should use the SelectFrom key and inverted this enter
+		setting.Config().Picture.EnableGravatar.SelectFromKey(): strconv.FormatBool(!form.EnableGravatar),
 		setting.Config().Picture.EnableFederatedAvatar.DynKey(): strconv.FormatBool(form.EnableFederatedAvatar),
 	}); err != nil {
 		ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form)

routers/web/admin/config.go

@@ -198,14 +198,13 @@ func ConfigSettings(ctx *context.Context) {
 func ChangeConfig(ctx *context.Context) {
 	cfg := setting.Config()
 
-	marshalBool := func(v string) ([]byte, error) {
-		b, _ := strconv.ParseBool(v)
-		return json.Marshal(b)
+	subValueSet := map[string]func(string) error{
+		cfg.Picture.EnableGravatar.DynKey(): cfg.Picture.EnableGravatar.SetValue,
 	}
 
-	marshalBoolInvert := func(v string) ([]byte, error) {
+	marshalBool := func(v string) ([]byte, error) {
 		b, _ := strconv.ParseBool(v)
-		return json.Marshal(!b)
+		return json.Marshal(b)
 	}
 
 	marshalString := func(emptyDefault string) func(v string) ([]byte, error) {
@@ -237,7 +236,7 @@ func ChangeConfig(ctx *context.Context) {
 	}
 
 	marshallers := map[string]func(string) ([]byte, error){
-		cfg.Picture.EnableGravatar.DynKey():        marshalBoolInvert, // Invert for backwards compatability with old database semantics
+		cfg.Picture.EnableGravatar.DynKey():        marshalBool,
 		cfg.Picture.EnableFederatedAvatar.DynKey(): marshalBool,
 		cfg.Repository.OpenWithEditorApps.DynKey(): marshalOpenWithApps,
 		cfg.Repository.GitGuideRemoteName.DynKey(): marshalString(cfg.Repository.GitGuideRemoteName.DefaultValue()),
@@ -266,7 +265,15 @@ loop:
 			ctx.JSONError(ctx.Tr("admin.config.set_setting_failed", key))
 			break loop
 		}
-		configSettings[key] = string(marshaledValue)
+
+		if setter, ok := subValueSet[key]; ok {
+			if err := setter(string(marshaledValue)); err != nil {
+				ctx.JSONError(ctx.Tr("admin.config.set_setting_failed", key))
+				break loop
+			}
+		} else {
+			configSettings[key] = string(marshaledValue)
+		}
 	}
 	if ctx.Written() {
 		return

templates/admin/config_settings/avatars.tmpl

@@ -6,14 +6,14 @@
 		<dt>{{ctx.Locale.Tr "admin.config.enable_gravatar"}}</dt>
 		<dd>
 			<div class="ui toggle checkbox" data-tooltip-content="{{ctx.Locale.Tr "admin.config.enable_gravatar"}}">
-				<input type="checkbox" data-config-dyn-key="picture.disable_gravatar" {{if .SystemConfig.Picture.EnableGravatar.Value ctx}}checked{{end}}><label></label>
+				<input type="checkbox" data-config-dyn-key="picture.enable_gravatar" {{if .SystemConfig.Picture.EnableGravatar.Value ctx}}checked{{end}}><label></label>
 			</div>
 		</dd>
 		<div class="divider"></div>
 		<dt>{{ctx.Locale.Tr "admin.config.enable_federated_avatar"}}</dt>
 		<dd>
 			<div class="ui toggle checkbox" data-tooltip-content="{{ctx.Locale.Tr "admin.config.enable_federated_avatar"}}">
-				<input type="checkbox" data-config-dyn-key="picture.enable_federated_avatar" {{if .SystemConfig.Picture.EnableFederatedAvatar.Value ctx}}checked{{end}}><label></label>
+				<input type="checkbox" data-config-dyn-key="picture.disable_federated_avatar" {{if .SystemConfig.Picture.EnableFederatedAvatar.Value ctx}}checked{{end}}><label></label>
 			</div>
 		</dd>
 	</dl>