From c7642ecca5251c7559265bd0c8efcc798014939a Mon Sep 17 00:00:00 2001 From: NanamiAdmin Date: Mon, 28 Sep 2026 23:28:47 +0800 Subject: [PATCH] feat(settings): report which saved keys need a restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every settings update answered with a bare success, so the console could not tell a change that took effect at once from one written to the file that the running program would keep ignoring until it was restarted — the listener address, the storage paths, the Komari dashboard URL. Classify the patch instead. config.RestartRequiredKeys expands a patch into the dot-separated paths of its leaves and intersects them with the settings main reads before it starts serving, and the settings endpoint returns that list as data.restartRequired. It is a pure function of the patch, so UpdateSettings keeps its signature and nothing else has to change; the write succeeds either way, and the field only says which edits are not live. Matching is by overlap rather than equality, so a patch that names a section — replacing it, or deleting it with a null — reports the startup-only keys inside it, while a sibling subtree like webhook.endpoints is not mistaken for webhook.enabled. The result is never nil, so an update with nothing to report serializes as [] rather than null and a client can iterate it without a guard. The console reads the field in ConfigSection and warns instead of confirming, naming the keys that are pending; Settings.vue's hints for the Komari URL, the listen address and the storage paths now say which of their fields is affected rather than labelling the whole card. --- README.md | 2 +- config/settings.go | 84 ++++++++++++ config/settings_test.go | 157 ++++++++++++++++++++++ frontend/src/api/index.js | 4 + frontend/src/components/ConfigSection.vue | 11 +- frontend/src/views/Settings.vue | 4 +- handler/settings.go | 9 +- handler/settings_test.go | 128 ++++++++++++++++++ 8 files changed, 393 insertions(+), 6 deletions(-) create mode 100644 handler/settings_test.go diff --git a/README.md b/README.md index 6d7c017..108133a 100644 --- a/README.md +++ b/README.md @@ -334,7 +334,7 @@ Browser WebSocket handshakes cannot carry custom headers, so `/api/system/getLog | `/api/server/getStatus` | GET | admin | Live server status (mirrors the Bot's `/status`). Query `?uuid=` (or `all`). Returns `data: {: {uuid, name, online, report}}`; `report` is `null` when the node has not reported yet. `404` for an unknown single uuid. | | `/api/server/exec` | POST | bot / admin | Execute a command. Body `{uuid: [...], command}`. Dispatches a Komari task and polls until completion (or timeout). Returns `data: {taskID, results}`. | | `/api/settings/get` | GET | admin | `?type=global\|bot_user_config\|bot_node_config` | Returns `data: {config}`, where `config` is the selected config file's content (same layout as the JSON file). | -| `/api/settings/set` | POST | admin | `?type=` + JSON body of partial updates, e.g. `{"system":{"debugMode":true}}` | Deep-merges the body into the selected config file, persists it, and reloads it in memory. Only the given keys change; arrays replace. See [What applies without a restart](#what-applies-without-a-restart). | +| `/api/settings/set` | POST | admin | `?type=` + JSON body of partial updates, e.g. `{"system":{"debugMode":true}}` | Deep-merges the body into the selected config file, persists it, and reloads it in memory. Only the given keys change; arrays replace. Returns `data: {type, restartRequired}`, where `restartRequired` lists the keys the update changed that are only read at startup (see [What applies without a restart](#what-applies-without-a-restart)) — the write succeeds regardless, this only says which edits are not live yet. Always an array, empty when everything took effect. | | `/api/webhook/add` | POST | admin | Add an incoming webhook endpoint. Body `{name, enabled?, token?, notifyPipes?}` — only the fields given are stored, the rest start at their defaults. `409` when the name is already configured. | | `/api/webhook/modify` | POST | admin | Change an existing endpoint. Body `{name, ...}` — the fields given are the fields that change (same partial-update rule as `/api/settings/set`, but scoped to one endpoint). `404` for an unknown name, `400` when no other field is given. | | `/api/webhook/delete` | POST | admin | Remove an endpoint. Body `{name}`. `404` for an unknown name. | diff --git a/config/settings.go b/config/settings.go index 15a3868..31ce22c 100644 --- a/config/settings.go +++ b/config/settings.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "strings" "sync" "nukumizu-backend/global" @@ -23,6 +24,89 @@ const ( // accepted constants above. var ErrUnsupportedSettingsType = errors.New("unsupported settings type") +// startupOnlySettings are the config.json keys that are read once before the +// program starts serving and never again: the listener addresses, the paths the +// databases are opened from, and the Komari dashboard its client is built +// against. Editing one writes the file and replaces the in-memory +// configuration, but the running program keeps the old value, so an update that +// touches one is reported back to the caller instead of being silently +// accepted. +// +// Keep this in step with main: these are exactly the settings main reads before +// the HTTP server comes up. Everything else — controllerMethod, networkProxy, +// the message templates, the debug switches — is picked up at runtime. +var startupOnlySettings = []string{ + "system.listenAddr", + "system.listenPort", + "webhook.enabled", + "webhook.listenAddr", + "webhook.listenPort", + "komari.dashboardURL", + "dataPath", + "dbPath", +} + +// RestartRequiredKeys lists the settings in patch that only take effect at +// startup, as dot-separated paths, in the order startupOnlySettings declares +// them. Only config.json carries such settings; an update to one of the other +// files always reports nothing. +// +// The write itself succeeds either way — this is advice for the user, not a +// rejection. The result is never nil, so a caller can put it straight into a +// JSON response and get [] rather than null. +func RestartRequiredKeys(settingsType string, patch map[string]interface{}) []string { + keys := []string{} + if settingsType != SettingGlobal { + return keys + } + + patched := patchPaths(patch) + for _, watched := range startupOnlySettings { + for _, path := range patched { + if pathsOverlap(path, watched) { + keys = append(keys, watched) + break + } + } + } + return keys +} + +// patchPaths expands a nested settings patch into the dot-separated paths of its +// leaves. An object is descended into rather than reported, so a patch that only +// names sections still resolves to the keys it changes, and a JSON null is a +// leaf because it deletes the key it names. +func patchPaths(patch map[string]interface{}) []string { + paths := []string{} + var walk func(prefix string, node map[string]interface{}) + walk = func(prefix string, node map[string]interface{}) { + for key, value := range node { + path := key + if prefix != "" { + path = prefix + "." + key + } + if nested, ok := value.(map[string]interface{}); ok && nested != nil { + walk(path, nested) + continue + } + paths = append(paths, path) + } + } + walk("", patch) + return paths +} + +// pathsOverlap reports whether a patched path and a watched setting can affect +// each other: they are the same key, the patch names something inside the +// watched setting, or the patch names a section the watched setting lives in. +// The last case matters because a patch may replace a whole section, which +// changes every key under it. +func pathsOverlap(patched, watched string) bool { + return patched == watched || + strings.HasPrefix(patched, watched+".") || + strings.HasPrefix(watched, patched+".") +} + // settingsLock serializes read-modify-write access to the on-disk configuration // files so concurrent admin edits (UpdateSettings) and the node tracker's // background save (SaveBotNodeConfig) cannot lose each other's updates. diff --git a/config/settings_test.go b/config/settings_test.go index 0380a09..6a864e1 100644 --- a/config/settings_test.go +++ b/config/settings_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "reflect" "strings" "testing" @@ -148,6 +149,162 @@ func TestUpdateSettingsReplacesArraysAndKeepsNumbers(t *testing.T) { } } +func TestRestartRequiredKeys(t *testing.T) { + cases := []struct { + name string + settingsType string + patch map[string]interface{} + want []string + }{ + { + name: "a runtime switch needs no restart", + settingsType: SettingGlobal, + patch: map[string]interface{}{"system": map[string]interface{}{"debugMode": true}}, + want: []string{}, + }, + { + name: "the listen port does", + settingsType: SettingGlobal, + patch: map[string]interface{}{"system": map[string]interface{}{"listenPort": "9090"}}, + want: []string{"system.listenPort"}, + }, + { + name: "only the startup key of a mixed patch is reported", + settingsType: SettingGlobal, + patch: map[string]interface{}{ + "system": map[string]interface{}{"listenPort": "9090", "debugMode": true}, + }, + want: []string{"system.listenPort"}, + }, + { + name: "a top-level path is reported", + settingsType: SettingGlobal, + patch: map[string]interface{}{"dataPath": "/srv/data"}, + want: []string{"dataPath"}, + }, + { + name: "results follow the declared order, not the patch order", + settingsType: SettingGlobal, + patch: map[string]interface{}{"dbPath": "/srv/db", "dataPath": "/srv/data"}, + want: []string{"dataPath", "dbPath"}, + }, + { + name: "deleting a startup key with null is reported", + settingsType: SettingGlobal, + patch: map[string]interface{}{"system": map[string]interface{}{"listenPort": nil}}, + want: []string{"system.listenPort"}, + }, + { + name: "replacing a whole section reports the startup keys inside it", + settingsType: SettingGlobal, + patch: map[string]interface{}{"webhook": map[string]interface{}{"listenAddr": "127.0.0.1"}}, + want: []string{"webhook.listenAddr"}, + }, + { + // Deleting the section resets the URL to its built-in default. + name: "deleting a section the startup key lives in reports it", + settingsType: SettingGlobal, + patch: map[string]interface{}{"komari": nil}, + want: []string{"komari.dashboardURL"}, + }, + { + name: "deleting a section reports every startup key inside it", + settingsType: SettingGlobal, + patch: map[string]interface{}{"webhook": nil}, + want: []string{"webhook.enabled", "webhook.listenAddr", "webhook.listenPort"}, + }, + { + // An empty object merges nothing, so it changes no key and needs no + // restart — surprising enough to pin. + name: "an empty object changes nothing", + settingsType: SettingGlobal, + patch: map[string]interface{}{"komari": map[string]interface{}{}}, + want: []string{}, + }, + { + // webhook.endpoints must not be mistaken for webhook.enabled. + name: "a sibling subtree is not mistaken for the startup key", + settingsType: SettingGlobal, + patch: map[string]interface{}{ + "webhook": map[string]interface{}{ + "endpoints": map[string]interface{}{"example": map[string]interface{}{"enabled": true}}, + }, + }, + want: []string{}, + }, + { + // The Komari credentials are re-read on the next login, so only the + // dashboard URL is startup-only. + name: "komari credentials are not startup-only", + settingsType: SettingGlobal, + patch: map[string]interface{}{ + "komari": map[string]interface{}{ + "account": map[string]interface{}{"username": "admin", "password": "x"}, + }, + }, + want: []string{}, + }, + { + name: "controller settings are not startup-only", + settingsType: SettingGlobal, + patch: map[string]interface{}{ + "controllerMethod": map[string]interface{}{ + "telegram": map[string]interface{}{"enabled": true, "botToken": "t"}, + }, + }, + want: []string{}, + }, + { + name: "an empty patch reports nothing", + settingsType: SettingGlobal, + patch: map[string]interface{}{}, + want: []string{}, + }, + { + // Only config.json has settings that are read once at startup. + name: "the other settings files never need a restart", + settingsType: SettingBotUserConfig, + patch: map[string]interface{}{"dataPath": "/srv/data"}, + want: []string{}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := RestartRequiredKeys(tc.settingsType, tc.patch) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("RestartRequiredKeys() = %v, want %v", got, tc.want) + } + if got == nil { + t.Error("the result must never be nil, so it serializes as [] rather than null") + } + }) + } +} + +// TestRestartRequiredKeysIsAdvisory pins that reporting a startup-only key does +// not stop the write: the caller is told, the file is still updated. +func TestRestartRequiredKeysIsAdvisory(t *testing.T) { + writeTempConfig(t, &global.ConfigPath.Global, `{"system":{"listenPort":"8080"}}`) + + keys := RestartRequiredKeys(SettingGlobal, map[string]interface{}{ + "system": map[string]interface{}{"listenPort": "9090"}, + }) + if len(keys) != 1 { + t.Fatalf("expected the listen port to be reported, got %v", keys) + } + + if err := UpdateSettings(SettingGlobal, map[string]interface{}{ + "system": map[string]interface{}{"listenPort": "9090"}, + }); err != nil { + t.Fatalf("UpdateSettings: %v", err) + } + + if got := Current().System.ListenPort; got != "9090" { + t.Errorf("the update was not applied: listenPort = %q", got) + } +} + func TestSettingsTypeValidation(t *testing.T) { for _, valid := range []string{SettingGlobal, SettingBotUserConfig, SettingBotNodeConfig} { if !IsValidSettingsType(valid) { diff --git a/frontend/src/api/index.js b/frontend/src/api/index.js index 6741f0f..8b9a335 100644 --- a/frontend/src/api/index.js +++ b/frontend/src/api/index.js @@ -13,6 +13,10 @@ export const serverApi = { // /api/settings/get?type=… / /api/settings/set?type=… // get → { success, message, data: { config } }. +// set → { success, message, data: { type, restartRequired } }, where +// restartRequired lists the keys the update changed that are only read at +// startup, so the caller can say which edits are not live yet. It is +// always an array, empty when the whole update took effect. // `type` is one of global | bot_user_config | bot_node_config. // For set, pass a partial object; a JSON null value removes that key. export const settingsApi = { diff --git a/frontend/src/components/ConfigSection.vue b/frontend/src/components/ConfigSection.vue index e9501d9..588fdab 100644 --- a/frontend/src/components/ConfigSection.vue +++ b/frontend/src/components/ConfigSection.vue @@ -133,8 +133,15 @@ async function save() { const patch = wrapRoot(props.section, nest(obj)); saving.value = true; try { - await settingsApi.set('global', patch); - toast.success(`${props.section.title} saved`); + const res = await settingsApi.set('global', patch); + // The backend reports the keys it wrote that are only read at startup. + // A plain "saved" would suggest those are live too. + const pending = (res && res.data && res.data.restartRequired) || []; + if (pending.length) { + toast.warn(`${props.section.title} saved — restart to apply: ${pending.join(', ')}`, 7000); + } else { + toast.success(`${props.section.title} saved`); + } emit('saved'); } catch (e) { toast.error('Failed to save: ' + e.message); diff --git a/frontend/src/views/Settings.vue b/frontend/src/views/Settings.vue index 46b9146..022cfa8 100644 --- a/frontend/src/views/Settings.vue +++ b/frontend/src/views/Settings.vue @@ -11,7 +11,7 @@ const sections = [ { id: 'system', title: 'System', - hint: 'HTTP listener and global runtime switches.', + hint: 'HTTP listener and global runtime switches. A changed listen address or port applies on restart.', root: ['system'], fields: [ { key: 'debugMode', type: 'bool', label: 'Debug mode', help: 'Skipped X-Timestamp checks and verbose debug logging.' }, @@ -37,7 +37,7 @@ const sections = [ { id: 'komari', title: 'Komari dashboard', - hint: 'Connection the monitor reads node data from. Takes effect on restart.', + hint: 'Connection the monitor reads node data from. The URL applies on restart; the account is re-read on the next login.', root: ['komari'], fields: [ { key: 'dashboardURL', type: 'text', label: 'Dashboard URL' }, diff --git a/handler/settings.go b/handler/settings.go index 9200b80..9f8bc16 100644 --- a/handler/settings.go +++ b/handler/settings.go @@ -47,6 +47,10 @@ func SettingsGetHandler(w http.ResponseWriter, r *http.Request) { // {"system": {"debugMode": true}} // // Multiple entries may be given at once; only the provided keys are changed. +// +// The response carries data.restartRequired: the keys the update changed that +// are only read at startup, so the caller can say which edits are not live yet. +// It is empty for an update that took effect in full. func SettingsSetHandler(w http.ResponseWriter, r *http.Request) { if !utils.Auth(w, r, "POST", "admin") { return @@ -80,7 +84,10 @@ func SettingsSetHandler(w http.ResponseWriter, r *http.Request) { return } + // The write landed either way. This only tells the caller which of the keys + // it changed will not be live until the program is restarted. utils.SendSuccessResponse(w, "settings updated successfully", map[string]interface{}{ - "type": settingsType, + "type": settingsType, + "restartRequired": config.RestartRequiredKeys(settingsType, patch), }) } diff --git a/handler/settings_test.go b/handler/settings_test.go new file mode 100644 index 0000000..5bca366 --- /dev/null +++ b/handler/settings_test.go @@ -0,0 +1,128 @@ +package handler + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "nukumizu-backend/config" + "nukumizu-backend/global" +) + +// tempConfigFile points one of the settings paths at a throwaway file, so a +// test can drive the settings API without touching the config files in the +// working directory. +func tempConfigFile(t *testing.T, field *string, content string) { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write temp config: %v", err) + } + original := *field + *field = path + t.Cleanup(func() { *field = original }) +} + +// settingsSetRequest builds an authenticated POST for the settings endpoint. +func settingsSetRequest(t *testing.T, settingsType, body string) *http.Request { + t.Helper() + req := httptest.NewRequest( + http.MethodPost, + "/api/settings/set?type="+settingsType, + strings.NewReader(body), + ) + req.Header.Set("X-Token", "test-admin-token") + req.Header.Set("X-Timestamp", strconv.FormatInt(time.Now().Unix(), 10)) + return req +} + +// settingsSetResponse is the envelope /api/settings/set answers with. +type settingsSetResponse struct { + Success bool `json:"success"` + Data struct { + Type string `json:"type"` + RestartRequired []string `json:"restartRequired"` + } `json:"data"` +} + +func decodeSettingsSetResponse(t *testing.T, w *httptest.ResponseRecorder) settingsSetResponse { + t.Helper() + var body settingsSetResponse + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v; body=%s", err, w.Body.String()) + } + return body +} + +// TestSettingsSetReportsStartupOnlyKeys covers the field the console reads to +// tell the user which of their edits are not live yet. +func TestSettingsSetReportsStartupOnlyKeys(t *testing.T) { + setupAdminToken() + tempConfigFile(t, &global.ConfigPath.Global, `{"system":{"debugMode":false,"listenPort":"8080"}}`) + + w := httptest.NewRecorder() + SettingsSetHandler(w, settingsSetRequest(t, "global", + `{"system":{"listenPort":"9090","debugMode":true}}`)) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + body := decodeSettingsSetResponse(t, w) + if !body.Success { + t.Fatalf("not a success envelope: %s", w.Body.String()) + } + if body.Data.Type != "global" { + t.Errorf("data.type = %q, want global", body.Data.Type) + } + if len(body.Data.RestartRequired) != 1 || body.Data.RestartRequired[0] != "system.listenPort" { + t.Errorf("restartRequired = %v, want [system.listenPort]", body.Data.RestartRequired) + } + + // Being reported as startup-only must not stop the write. + data, err := config.GetSettings(config.SettingGlobal) + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + for _, want := range []string{`"listenPort": "9090"`, `"debugMode": true`} { + if !strings.Contains(string(data), want) { + t.Errorf("the patch was not applied, missing %s:\n%s", want, data) + } + } +} + +// TestSettingsSetRestartRequiredIsAlwaysAnArray pins the shape a client +// iterates over: an update with nothing to report must answer [] and not null. +func TestSettingsSetRestartRequiredIsAlwaysAnArray(t *testing.T) { + setupAdminToken() + tempConfigFile(t, &global.ConfigPath.Global, `{"system":{"debugMode":false}}`) + + w := httptest.NewRecorder() + SettingsSetHandler(w, settingsSetRequest(t, "global", `{"system":{"debugMode":true}}`)) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), `"restartRequired":[]`) { + t.Errorf("restartRequired should serialize as an empty array: %s", w.Body.String()) + } +} + +// TestSettingsSetRejectsUnknownType keeps the failure path intact now that the +// success path computes an extra field. +func TestSettingsSetRejectsUnknownType(t *testing.T) { + setupAdminToken() + tempConfigFile(t, &global.ConfigPath.Global, `{}`) + + w := httptest.NewRecorder() + SettingsSetHandler(w, settingsSetRequest(t, "nonsense", `{}`)) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", w.Code) + } +}