refactor: simplify app_config service and fix race conditions (#423)

backend/internal/model/app_config.go

@@ -1,17 +1,17 @@
 package model
 
 import (
+	"errors"
+	"fmt"
+	"reflect"
 	"strconv"
+	"strings"
 	"time"
 )
 
 type AppConfigVariable struct {
-	Key          string `gorm:"primaryKey;not null"`
-	Type         string
-	IsPublic     bool
-	IsInternal   bool
-	Value        string
-	DefaultValue string
+	Key   string `gorm:"primaryKey;not null"`
+	Value string
 }
 
 // IsTrue returns true if the value is a truthy string, such as "true", "t", "yes", "1", etc.
@@ -31,41 +31,153 @@ func (a *AppConfigVariable) AsDurationMinutes() time.Duration {
 
 type AppConfig struct {
 	// General
-	AppName             AppConfigVariable
-	SessionDuration     AppConfigVariable
-	EmailsVerified      AppConfigVariable
-	AllowOwnAccountEdit AppConfigVariable
+	AppName             AppConfigVariable `key:"appName,public"` // Public
+	SessionDuration     AppConfigVariable `key:"sessionDuration"`
+	EmailsVerified      AppConfigVariable `key:"emailsVerified"`
+	AllowOwnAccountEdit AppConfigVariable `key:"allowOwnAccountEdit,public"` // Public
 	// Internal
-	BackgroundImageType AppConfigVariable
-	LogoLightImageType  AppConfigVariable
-	LogoDarkImageType   AppConfigVariable
+	BackgroundImageType AppConfigVariable `key:"backgroundImageType,internal"` // Internal
+	LogoLightImageType  AppConfigVariable `key:"logoLightImageType,internal"`  // Internal
+	LogoDarkImageType   AppConfigVariable `key:"logoDarkImageType,internal"`   // Internal
 	// Email
-	SmtpHost                      AppConfigVariable
-	SmtpPort                      AppConfigVariable
-	SmtpFrom                      AppConfigVariable
-	SmtpUser                      AppConfigVariable
-	SmtpPassword                  AppConfigVariable
-	SmtpTls                       AppConfigVariable
-	SmtpSkipCertVerify            AppConfigVariable
-	EmailLoginNotificationEnabled AppConfigVariable
-	EmailOneTimeAccessEnabled     AppConfigVariable
+	SmtpHost                      AppConfigVariable `key:"smtpHost"`
+	SmtpPort                      AppConfigVariable `key:"smtpPort"`
+	SmtpFrom                      AppConfigVariable `key:"smtpFrom"`
+	SmtpUser                      AppConfigVariable `key:"smtpUser"`
+	SmtpPassword                  AppConfigVariable `key:"smtpPassword"`
+	SmtpTls                       AppConfigVariable `key:"smtpTls"`
+	SmtpSkipCertVerify            AppConfigVariable `key:"smtpSkipCertVerify"`
+	EmailLoginNotificationEnabled AppConfigVariable `key:"emailLoginNotificationEnabled"`
+	EmailOneTimeAccessEnabled     AppConfigVariable `key:"emailOneTimeAccessEnabled,public"` // Public
 	// LDAP
-	LdapEnabled                        AppConfigVariable
-	LdapUrl                            AppConfigVariable
-	LdapBindDn                         AppConfigVariable
-	LdapBindPassword                   AppConfigVariable
-	LdapBase                           AppConfigVariable
-	LdapUserSearchFilter               AppConfigVariable
-	LdapUserGroupSearchFilter          AppConfigVariable
-	LdapSkipCertVerify                 AppConfigVariable
-	LdapAttributeUserUniqueIdentifier  AppConfigVariable
-	LdapAttributeUserUsername          AppConfigVariable
-	LdapAttributeUserEmail             AppConfigVariable
-	LdapAttributeUserFirstName         AppConfigVariable
-	LdapAttributeUserLastName          AppConfigVariable
-	LdapAttributeUserProfilePicture    AppConfigVariable
-	LdapAttributeGroupMember           AppConfigVariable
-	LdapAttributeGroupUniqueIdentifier AppConfigVariable
-	LdapAttributeGroupName             AppConfigVariable
-	LdapAttributeAdminGroup            AppConfigVariable
+	LdapEnabled                        AppConfigVariable `key:"ldapEnabled,public"` // Public
+	LdapUrl                            AppConfigVariable `key:"ldapUrl"`
+	LdapBindDn                         AppConfigVariable `key:"ldapBindDn"`
+	LdapBindPassword                   AppConfigVariable `key:"ldapBindPassword"`
+	LdapBase                           AppConfigVariable `key:"ldapBase"`
+	LdapUserSearchFilter               AppConfigVariable `key:"ldapUserSearchFilter"`
+	LdapUserGroupSearchFilter          AppConfigVariable `key:"ldapUserGroupSearchFilter"`
+	LdapSkipCertVerify                 AppConfigVariable `key:"ldapSkipCertVerify"`
+	LdapAttributeUserUniqueIdentifier  AppConfigVariable `key:"ldapAttributeUserUniqueIdentifier"`
+	LdapAttributeUserUsername          AppConfigVariable `key:"ldapAttributeUserUsername"`
+	LdapAttributeUserEmail             AppConfigVariable `key:"ldapAttributeUserEmail"`
+	LdapAttributeUserFirstName         AppConfigVariable `key:"ldapAttributeUserFirstName"`
+	LdapAttributeUserLastName          AppConfigVariable `key:"ldapAttributeUserLastName"`
+	LdapAttributeUserProfilePicture    AppConfigVariable `key:"ldapAttributeUserProfilePicture"`
+	LdapAttributeGroupMember           AppConfigVariable `key:"ldapAttributeGroupMember"`
+	LdapAttributeGroupUniqueIdentifier AppConfigVariable `key:"ldapAttributeGroupUniqueIdentifier"`
+	LdapAttributeGroupName             AppConfigVariable `key:"ldapAttributeGroupName"`
+	LdapAttributeAdminGroup            AppConfigVariable `key:"ldapAttributeAdminGroup"`
+}
+
+func (c *AppConfig) ToAppConfigVariableSlice(showAll bool) []AppConfigVariable {
+	// Use reflection to iterate through all fields
+	cfgValue := reflect.ValueOf(c).Elem()
+	cfgType := cfgValue.Type()
+
+	res := make([]AppConfigVariable, cfgType.NumField())
+
+	for i := range cfgType.NumField() {
+		field := cfgType.Field(i)
+
+		key, attrs, _ := strings.Cut(field.Tag.Get("key"), ",")
+		if key == "" {
+			continue
+		}
+
+		// If we're only showing public variables and this is not public, skip it
+		if !showAll && attrs != "public" {
+			continue
+		}
+
+		fieldValue := cfgValue.Field(i)
+
+		res[i] = AppConfigVariable{
+			Key:   key,
+			Value: fieldValue.FieldByName("Value").String(),
+		}
+	}
+
+	return res
+}
+
+func (c *AppConfig) FieldByKey(key string) (string, error) {
+	rv := reflect.ValueOf(c).Elem()
+	rt := rv.Type()
+
+	// Find the field in the struct whose "key" tag matches
+	for i := range rt.NumField() {
+		// Grab only the first part of the key, if there's a comma with additional properties
+		tagValue, _, _ := strings.Cut(rt.Field(i).Tag.Get("key"), ",")
+		if tagValue != key {
+			continue
+		}
+
+		valueField := rv.Field(i).FieldByName("Value")
+		return valueField.String(), nil
+	}
+
+	// If we are here, the config key was not found
+	return "", AppConfigKeyNotFoundError{field: key}
+}
+
+func (c *AppConfig) UpdateField(key string, value string, noInternal bool) error {
+	rv := reflect.ValueOf(c).Elem()
+	rt := rv.Type()
+
+	// Find the field in the struct whose "key" tag matches, then update that
+	for i := range rt.NumField() {
+		// Separate the key (before the comma) from any optional attributes after
+		tagValue, attrs, _ := strings.Cut(rt.Field(i).Tag.Get("key"), ",")
+		if tagValue != key {
+			continue
+		}
+
+		// If the field is internal and noInternal is true, we skip that
+		if noInternal && attrs == "internal" {
+			return AppConfigInternalForbiddenError{field: key}
+		}
+
+		valueField := rv.Field(i).FieldByName("Value")
+		if !valueField.CanSet() {
+			return fmt.Errorf("field Value in AppConfigVariable is not settable for config key '%s'", key)
+		}
+
+		// Update the value
+		valueField.SetString(value)
+
+		// Return once updated
+		return nil
+	}
+
+	// If we're here, we have not found the right field to update
+	return AppConfigKeyNotFoundError{field: key}
+}
+
+type AppConfigKeyNotFoundError struct {
+	field string
+}
+
+func (e AppConfigKeyNotFoundError) Error() string {
+	return fmt.Sprintf("cannot find config key '%s'", e.field)
+}
+
+func (e AppConfigKeyNotFoundError) Is(target error) bool {
+	// Ignore the field property when checking if an error is of the type AppConfigKeyNotFoundError
+	x := AppConfigKeyNotFoundError{}
+	return errors.As(target, &x)
+}
+
+type AppConfigInternalForbiddenError struct {
+	field string
+}
+
+func (e AppConfigInternalForbiddenError) Error() string {
+	return fmt.Sprintf("field '%s' is internal and can't be updated", e.field)
+}
+
+func (e AppConfigInternalForbiddenError) Is(target error) bool {
+	// Ignore the field property when checking if an error is of the type AppConfigInternalForbiddenError
+	x := AppConfigInternalForbiddenError{}
+	return errors.As(target, &x)
 }

backend/internal/model/app_config_test.go

@@ -1,10 +1,16 @@
-package model
+// We use model_test here to avoid an import cycle
+package model_test
 
 import (
+	"reflect"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/pocket-id/pocket-id/backend/internal/dto"
+	"github.com/pocket-id/pocket-id/backend/internal/model"
 )
 
 func TestAppConfigVariable_AsMinutesDuration(t *testing.T) {
@@ -48,7 +54,7 @@ func TestAppConfigVariable_AsMinutesDuration(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			configVar := AppConfigVariable{
+			configVar := model.AppConfigVariable{
 				Value: tt.value,
 			}
 
@@ -58,3 +64,66 @@ func TestAppConfigVariable_AsMinutesDuration(t *testing.T) {
 		})
 	}
 }
+
+// This test ensures that the model.AppConfig and dto.AppConfigUpdateDto structs match:
+// - They should have the same properties, where the "json" tag of dto.AppConfigUpdateDto should match the "key" tag in model.AppConfig
+// - dto.AppConfigDto should not include "internal" fields from model.AppConfig
+// This test is primarily meant to catch discrepancies between the two structs as fields are added or removed over time
+func TestAppConfigStructMatchesUpdateDto(t *testing.T) {
+	appConfigType := reflect.TypeOf(model.AppConfig{})
+	updateDtoType := reflect.TypeOf(dto.AppConfigUpdateDto{})
+
+	// Process AppConfig fields
+	appConfigFields := make(map[string]string)
+	for i := 0; i < appConfigType.NumField(); i++ {
+		field := appConfigType.Field(i)
+		if field.Tag.Get("key") == "" {
+			// Skip internal fields
+			continue
+		}
+
+		// Extract the key name from the tag (takes the part before any comma)
+		keyTag := field.Tag.Get("key")
+		keyName, _, _ := strings.Cut(keyTag, ",")
+
+		appConfigFields[field.Name] = keyName
+	}
+
+	// Process AppConfigUpdateDto fields
+	dtoFields := make(map[string]string)
+	for i := 0; i < updateDtoType.NumField(); i++ {
+		field := updateDtoType.Field(i)
+
+		// Extract the json name from the tag (takes the part before any binding constraints)
+		jsonTag := field.Tag.Get("json")
+		jsonName, _, _ := strings.Cut(jsonTag, ",")
+
+		dtoFields[jsonName] = field.Name
+	}
+
+	// Verify every AppConfig field has a matching DTO field with the same name
+	for fieldName, keyName := range appConfigFields {
+		if strings.HasSuffix(fieldName, "ImageType") {
+			// Skip internal fields that shouldn't be in the DTO
+			continue
+		}
+
+		// Check if there's a DTO field with a matching JSON tag
+		_, exists := dtoFields[keyName]
+		assert.True(t, exists, "Field %s with key '%s' in AppConfig has no matching field in AppConfigUpdateDto", fieldName, keyName)
+	}
+
+	// Verify every DTO field has a matching AppConfig field
+	for jsonName, fieldName := range dtoFields {
+		// Find a matching field in AppConfig by key tag
+		found := false
+		for _, keyName := range appConfigFields {
+			if keyName == jsonName {
+				found = true
+				break
+			}
+		}
+
+		assert.True(t, found, "Field %s with json tag '%s' in AppConfigUpdateDto has no matching field in AppConfig", fieldName, jsonName)
+	}
+}