Skip to content

Commit 78b5a71

Browse files
iesrezaclaude
andcommitted
fix(settings): persist Set/SetMulti to the correct DB row
settings.Set/SetMulti silently failed to write to the database for all callers, and even once writing they targeted the wrong row. Three compounding bugs: 1. PRIMARY: Setting.SettingsID had `gorm:"primaryKey"` with no column tag, so GORM derived column `settings_id` while the table PK is `id`. Every INSERT/UPDATE referenced a missing column and errored; the error was swallowed by `_ = saveSingleSetting(...)`. Reads survived because Find uses SELECT *. Fixed to `gorm:"column:id;primaryKey"`. 2. Setting.Protected mapped to a non-existent `protected` column, breaking full-struct Save. Changed to `gorm:"-"` (kept for JSON only). 3. Read/write key asymmetry: LoadDatabaseSettings reconstructs keys as <domainPath>.<name> from the domain hierarchy, but the writer flattened the whole dotted key into one name under a hardcoded `default` domain. Rewrote the writer: Set/SetMulti pass the original dotted key; splitSettingKey splits on the last dot; resolveDomainID resolves/creates the domain hierarchy using Attrs() for create-only columns so an existing migration-seeded domain is matched instead of spawning a duplicate. Flat (non-dotted) keys still resolve to the `default` domain, so SaveToDB and flat-key callers are unchanged. Verified end-to-end against the dev DB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e41ac6a commit 78b5a71

2 files changed

Lines changed: 108 additions & 65 deletions

File tree

lib/settings/database.go

Lines changed: 100 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package settings
33
import (
44
"fmt"
55
"github.com/getevo/evo/v2/lib/db"
6+
"strings"
67
"time"
78
)
89

@@ -23,7 +24,12 @@ func (SettingDomain) TableName() string {
2324
}
2425

2526
type Setting struct {
26-
SettingsID uint `gorm:"primaryKey" json:"-"`
27+
// The primary key column is `id`. Without the explicit column tag GORM
28+
// derives `settings_id`, which does not exist in the table — every
29+
// INSERT/UPDATE then references a missing column and fails (reads via
30+
// SELECT * still work, which is why this stayed hidden). Keep this in sync
31+
// with the canonical model in apps/settings/models.go.
32+
SettingsID uint `gorm:"column:id;primaryKey" json:"-"`
2733
DomainID uint `gorm:"column:domain_id;fk:settings_domain" json:"domain"`
2834
Domain string `gorm:"-" json:"-"`
2935
Name string `gorm:"column:name;size:128" json:"name"`
@@ -33,7 +39,9 @@ type Setting struct {
3339
DefaultValue string `gorm:"column:default_value" json:"default_value"`
3440
ReadOnly bool `gorm:"column:read_only" json:"read_only"`
3541
Visible bool `gorm:"column:visible" json:"visible"`
36-
Protected bool `gorm:"column:protected" json:"protected"`
42+
// No `protected` column exists in the table; mapping it to a column made
43+
// full-struct writes fail. Kept as a non-persisted field for API/JSON.
44+
Protected bool `gorm:"-" json:"protected"`
3745
Type string `gorm:"column:type" json:"type"`
3846
Params string `gorm:"column:params" json:"params"`
3947
SettingsDomain SettingDomain `gorm:"-" json:"-"`
@@ -97,31 +105,99 @@ func LoadDatabaseSettings() error {
97105
return nil
98106
}
99107

100-
// saveSingleSetting saves a single setting to the database.
101-
// Creates or updates the setting in the default domain.
108+
// splitSettingKey splits a hierarchical setting key into its domain path and
109+
// leaf name on the LAST dot separator. This mirrors LoadDatabaseSettings, which
110+
// reconstructs a key as "<domainPath>.<name>". A key with no dot has an empty
111+
// domain path and is stored in the default domain.
112+
//
113+
// "CMS.LLMS" -> ("CMS", "LLMS")
114+
// "A.B.C" -> ("A.B", "C")
115+
// "FLATKEY" -> ("", "FLATKEY")
116+
func splitSettingKey(key string) (domainPath, name string) {
117+
if idx := strings.LastIndex(key, "."); idx >= 0 {
118+
return key[:idx], key[idx+1:]
119+
}
120+
return "", key
121+
}
122+
123+
// resolveDomainID resolves a (possibly nested) domain path to its domain id,
124+
// creating any missing domains along the way. An empty path resolves to the
125+
// "default" domain. This is the write-side counterpart of getFullDomainPath
126+
// used by LoadDatabaseSettings, so a value written by Set/SetMulti is read back
127+
// under the exact same key after a reload (rather than landing in an orphan
128+
// row that nothing reads — the previous behaviour, which flattened the whole
129+
// dotted key into a single name under the "default" domain).
130+
func resolveDomainID(domainPath string) (uint, error) {
131+
if domainPath == "" {
132+
// Search only on the domain name; create-only columns go in Attrs so a
133+
// pre-existing "default" row matches regardless of its other column
134+
// values (see the loop below for why this matters).
135+
var defaultDomain SettingDomain
136+
err := db.Where("domain = ?", "default").
137+
Attrs(SettingDomain{
138+
Title: "Default Settings",
139+
Description: "Default settings domain",
140+
Visible: true,
141+
}).
142+
FirstOrCreate(&defaultDomain).Error
143+
if err != nil {
144+
return 0, fmt.Errorf("failed to create default domain: %w", err)
145+
}
146+
return defaultDomain.SettingsDomainID, nil
147+
}
148+
149+
var parentID *uint
150+
var leafID uint
151+
for _, segment := range strings.Split(domainPath, ".") {
152+
// Search ONLY on the domain's identity (name + parent). Create-only
153+
// columns go in Attrs — if they were passed as FirstOrCreate conds GORM
154+
// would fold their non-zero values (title, visible) into the SELECT, so
155+
// an existing domain seeded by a migration with a different title would
156+
// fail to match and a duplicate domain would be created.
157+
var domain SettingDomain
158+
query := db.Where("domain = ?", segment)
159+
if parentID == nil {
160+
query = query.Where("parent_domain IS NULL")
161+
} else {
162+
query = query.Where("parent_domain = ?", *parentID)
163+
}
164+
err := query.
165+
Attrs(SettingDomain{
166+
ParentDomain: parentID,
167+
Title: segment,
168+
Visible: true,
169+
}).
170+
FirstOrCreate(&domain).Error
171+
if err != nil {
172+
return 0, fmt.Errorf("failed to create/find domain %q: %w", segment, err)
173+
}
174+
leafID = domain.SettingsDomainID
175+
id := domain.SettingsDomainID
176+
parentID = &id
177+
}
178+
return leafID, nil
179+
}
180+
181+
// saveSingleSetting saves a single setting to the database under the domain
182+
// implied by its hierarchical key. Creates the domain hierarchy and the
183+
// setting row if they don't exist; otherwise updates the existing row in place
184+
// so the change is picked up by every node via the settings-table CDC hook.
102185
func saveSingleSetting(key string, value any) error {
103-
// Ensure default domain exists (cached after first call)
104-
var defaultDomain SettingDomain
105-
err := db.Where("domain = ?", "default").FirstOrCreate(&defaultDomain, SettingDomain{
106-
Domain: "default",
107-
Title: "Default Settings",
108-
Description: "Default settings domain",
109-
ReadOnly: false,
110-
Visible: true,
111-
}).Error
186+
domainPath, name := splitSettingKey(key)
187+
domainID, err := resolveDomainID(domainPath)
112188
if err != nil {
113-
return fmt.Errorf("failed to create default domain: %w", err)
189+
return err
114190
}
115191

116192
// Convert value to string for storage
117193
valueStr := fmt.Sprint(value)
118194

119-
// Find or create the setting
195+
// Find or create the setting under the resolved domain
120196
var setting Setting
121-
err = db.Where("domain_id = ? AND name = ?", defaultDomain.SettingsDomainID, key).
197+
err = db.Where("domain_id = ? AND name = ?", domainID, name).
122198
FirstOrCreate(&setting, Setting{
123-
DomainID: defaultDomain.SettingsDomainID,
124-
Name: key,
199+
DomainID: domainID,
200+
Name: name,
125201
}).Error
126202
if err != nil {
127203
return fmt.Errorf("failed to create/find setting %s: %w", key, err)
@@ -138,45 +214,13 @@ func saveSingleSetting(key string, value any) error {
138214
return nil
139215
}
140216

141-
// saveDatabaseSettings saves multiple settings to the database.
142-
// Creates or updates settings in the default domain (domain_id = 1).
143-
func saveDatabaseSettings(flattenedData map[string]any) error {
144-
// Ensure default domain exists
145-
var defaultDomain SettingDomain
146-
err := db.Where("domain = ?", "default").FirstOrCreate(&defaultDomain, SettingDomain{
147-
Domain: "default",
148-
Title: "Default Settings",
149-
Description: "Default settings domain",
150-
ReadOnly: false,
151-
Visible: true,
152-
}).Error
153-
if err != nil {
154-
return fmt.Errorf("failed to create default domain: %w", err)
155-
}
156-
157-
// Save or update each setting
158-
for key, value := range flattenedData {
159-
// Convert value to string for storage
160-
valueStr := fmt.Sprint(value)
161-
162-
var setting Setting
163-
err := db.Where("domain_id = ? AND name = ?", defaultDomain.SettingsDomainID, key).
164-
FirstOrCreate(&setting, Setting{
165-
DomainID: defaultDomain.SettingsDomainID,
166-
Name: key,
167-
}).Error
168-
if err != nil {
169-
return fmt.Errorf("failed to create/find setting %s: %w", key, err)
170-
}
171-
172-
// Update value if changed
173-
if setting.Value != valueStr {
174-
setting.Value = valueStr
175-
if err := db.Save(&setting).Error; err != nil {
176-
return fmt.Errorf("failed to save setting %s: %w", key, err)
177-
}
217+
// saveDatabaseSettings saves multiple settings to the database, each under the
218+
// domain implied by its hierarchical key (see saveSingleSetting).
219+
func saveDatabaseSettings(data map[string]any) error {
220+
for key, value := range data {
221+
if err := saveSingleSetting(key, value); err != nil {
222+
return err
178223
}
179224
}
180-
181225
return nil
182226
}

lib/settings/settings.go

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -146,9 +146,12 @@ func Set(key string, value any) {
146146
data[normalizedKey] = value
147147
mu.Unlock()
148148

149-
// Persist to database if enabled
149+
// Persist to database if enabled. The ORIGINAL (dotted) key is passed —
150+
// not the flattened normalizedKey — so the domain hierarchy can be
151+
// reconstructed and the value lands in the same row LoadDatabaseSettings
152+
// reads back.
150153
if db.IsEnabled() {
151-
_ = saveSingleSetting(normalizedKey, value)
154+
_ = saveSingleSetting(key, value)
152155
}
153156

154157
// Trigger change callbacks
@@ -177,15 +180,11 @@ func SetMulti(in map[string]any) {
177180
}
178181
mu.Unlock()
179182

180-
// Persist to database if enabled
183+
// Persist to database if enabled. Pass the ORIGINAL (dotted) keys so each
184+
// value is stored under the correct domain hierarchy (see saveSingleSetting).
181185
if db.IsEnabled() {
182-
normalizedMap := make(map[string]any, len(in))
183-
for key, value := range in {
184-
normalizedMap[normalizeKey(key)] = value
185-
}
186186
db.UseModel(Setting{}, SettingDomain{})
187-
_ = saveDatabaseSettings(normalizedMap)
188-
187+
_ = saveDatabaseSettings(in)
189188
}
190189

191190
// Trigger change callbacks outside lock

0 commit comments

Comments
 (0)