From 348a6890bcdc2d39f87253e28e62487bac8d8ef0 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:52:59 +0200 Subject: [PATCH] Add new config keys to existing files at startup LoadModConfig fills a missing key from the class default in memory and leaves the file alone, so an admin upgrading from 0.1.0 never saw the Attribution block appear in pulse.json, nor ServiceName in pulse-otlp.json, and had to learn about them from the README and type them in. ConfigUpgrade compares the file on disk against the config object the loader handed back, recursively and by key name only, and reports both what the file is missing and what it carries that the config knows nothing about. When something is missing the file is rewritten from that object, which already holds every value the admin set, so the write only ever adds; the log then names the keys that were added. An unknown key does not survive that rewrite, so it gets a warning of its own rather than disappearing quietly. A file that is already complete is not written at all, which keeps a read-only or version-tracked ModConfig untouched. The comparison is against JsonUtil.ToPrettyString, which is the same JsonConvert.SerializeObject(obj, Formatting.Indented) call StoreModConfig writes with, so the keys weighed here are exactly the keys a rewrite produces. Pulse.Otlp compiles the helper from the same source file rather than referencing Pulse.dll, since the two mods still meet at nothing but a meter name. --- CHANGELOG.md | 6 + .../OtlpConfigUpgradeScenarios.cs | 28 +++ .../data/configupgrade/pulse-otlp.json | 8 + .../data/configupgrade/pulse.json | 12 ++ Pulse.Otlp/Pulse.Otlp.csproj | 7 + Pulse.Otlp/PulseOtlpModSystem.cs | 8 +- Pulse.Scenarios/ConfigUpgradeScenarios.cs | 44 +++++ Pulse.Scenarios/data/configupgrade/pulse.json | 4 + Pulse.Tests/ConfigUpgradeTests.cs | 183 ++++++++++++++++++ Pulse/ConfigUpgrade.cs | 133 +++++++++++++ Pulse/PulseModSystem.cs | 8 +- README.md | 8 + tools/mutation-check.sh | 16 +- 13 files changed, 462 insertions(+), 3 deletions(-) create mode 100644 Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs create mode 100644 Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json create mode 100644 Pulse.Otlp.Scenarios/data/configupgrade/pulse.json create mode 100644 Pulse.Scenarios/ConfigUpgradeScenarios.cs create mode 100644 Pulse.Scenarios/data/configupgrade/pulse.json create mode 100644 Pulse.Tests/ConfigUpgradeTests.cs create mode 100644 Pulse/ConfigUpgrade.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 48ce1fe..fc99140 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ first. ### Added +- Config files are brought up to date at startup instead of only on first boot. Each mod compares + the file on disk against the keys it knows and writes the missing ones back with their defaults, + keeping every value already in the file, so a server upgrading from 0.1.0 gets the `Attribution` + block in `pulse.json` and `ServiceName` in `pulse-otlp.json` without anyone editing them by hand. + Keys neither mod recognises are named in a warning, since the rewrite drops them. A file that + already holds every key is left alone, modification time included. - Per-mod tick attribution, behind a new `Attribution` block in `pulse.json` and off by default. `pulse_mod_tick_share{modid}` is the fraction of profiled main-thread busy time one mod took over the last burst, `pulse_mod_tick_seconds_total{modid}` the sampled seconds behind it, diff --git a/Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs b/Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs new file mode 100644 index 0000000..c544f2f --- /dev/null +++ b/Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs @@ -0,0 +1,28 @@ +using System.Text.Json.Nodes; +using Atlas.XUnit; +using Xunit; + +namespace Pulse.Otlp.Scenarios; + +/// The same upgrade on the other mod's file: a pulse-otlp.json written before +/// ServiceName existed gets it, and keeps the endpoint the admin pointed it at. The base +/// mod is seeded complete and turned off, because nothing here is about what it serves; the OTLP +/// mod only needs it present, which its modinfo dependency requires anyway. Nothing listens on the +/// configured endpoint, and nothing needs to: an export that cannot connect is swallowed by the +/// SDK's own export thread. +[AtlasDataFiles("data/configupgrade", TargetPath = "ModConfig")] +public class OtlpConfigUpgradeScenarios : AtlasScenarioBase +{ + [AtlasScenario] + public async Task Startup_Fills_ServiceName_IntoAnOlderConfigFile() + { + await World.Ticks(5); + + string path = Path.Combine(World.Api.GetOrCreateDataPath("ModConfig"), "pulse-otlp.json"); + JsonObject config = Assert.IsType(JsonNode.Parse(File.ReadAllText(path))); + + Assert.Equal("vintagestory", (string?)config["ServiceName"]); + Assert.Equal("http://127.0.0.1:39473", (string?)config["Endpoint"]); + Assert.Equal(60, (int)config["IntervalSeconds"]!); + } +} diff --git a/Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json b/Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json new file mode 100644 index 0000000..02b16b4 --- /dev/null +++ b/Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json @@ -0,0 +1,8 @@ +{ + "Enabled": true, + "Endpoint": "http://127.0.0.1:39473", + "Protocol": "http/protobuf", + "Headers": {}, + "IntervalSeconds": 60, + "IncludeRuntimeMetrics": false +} diff --git a/Pulse.Otlp.Scenarios/data/configupgrade/pulse.json b/Pulse.Otlp.Scenarios/data/configupgrade/pulse.json new file mode 100644 index 0000000..433613d --- /dev/null +++ b/Pulse.Otlp.Scenarios/data/configupgrade/pulse.json @@ -0,0 +1,12 @@ +{ + "Enabled": false, + "Bind": "127.0.0.1", + "Port": 39473, + "RuntimeMetrics": false, + "ChunksRefreshSeconds": 30, + "Attribution": { + "Enabled": false, + "BurstTicks": 30, + "IntervalSeconds": 10 + } +} diff --git a/Pulse.Otlp/Pulse.Otlp.csproj b/Pulse.Otlp/Pulse.Otlp.csproj index d5fba3b..1aeb47a 100644 --- a/Pulse.Otlp/Pulse.Otlp.csproj +++ b/Pulse.Otlp/Pulse.Otlp.csproj @@ -16,6 +16,13 @@ + + + + + diff --git a/Pulse.Otlp/PulseOtlpModSystem.cs b/Pulse.Otlp/PulseOtlpModSystem.cs index 70deadb..2ff553e 100644 --- a/Pulse.Otlp/PulseOtlpModSystem.cs +++ b/Pulse.Otlp/PulseOtlpModSystem.cs @@ -29,7 +29,13 @@ public sealed class PulseOtlpModSystem : ModSystem public override void StartServerSide(ICoreServerAPI api) { - PulseOtlpConfig config = api.LoadModConfig(ConfigFile) ?? StoreDefaults(api); + PulseOtlpConfig? existing = api.LoadModConfig(ConfigFile); + PulseOtlpConfig config = existing ?? StoreDefaults(api); + if (existing != null) + { + ConfigUpgrade.Upgrade(api, config, ConfigFile, "Pulse OTLP"); + } + if (!config.Enabled) { api.Logger.Notification("Pulse OTLP is disabled in " + ConfigFile + ", nothing registered."); diff --git a/Pulse.Scenarios/ConfigUpgradeScenarios.cs b/Pulse.Scenarios/ConfigUpgradeScenarios.cs new file mode 100644 index 0000000..145c240 --- /dev/null +++ b/Pulse.Scenarios/ConfigUpgradeScenarios.cs @@ -0,0 +1,44 @@ +using System.Text.Json.Nodes; +using Atlas.XUnit; +using Xunit; + +namespace Pulse.Scenarios; + +/// An admin's own pulse.json, brought up to date by a real server booting on it. The +/// seeded file is what an upgrade actually looks like: one key the admin set, nothing else the +/// current version declares, and one key that answers to nothing. +[AtlasDataFiles("data/configupgrade/pulse.json", TargetPath = "ModConfig")] +public class ConfigUpgradeScenarios : AtlasScenarioBase +{ + private const int Port = 39472; + + [AtlasScenario] + public async Task Startup_Fills_AnOlderConfigFile_WithoutLosingWhatTheAdminSet() + { + await World.Ticks(5); + + string path = Path.Combine(World.Api.GetOrCreateDataPath("ModConfig"), "pulse.json"); + JsonObject config = Assert.IsType(JsonNode.Parse(File.ReadAllText(path))); + + // The admin's one setting, still theirs on disk and still what the endpoint bound: a + // rewrite that reset it to the default would be the worst possible outcome here. + Assert.Equal(Port, (int)config["Port"]!); + Assert.Contains("pulse_server_ticks_total", await Scrape.Metrics(Port)); + + // The block that arrived after 0.1.0, written out with the defaults the class declares. + JsonObject attribution = Assert.IsType(config["Attribution"]); + Assert.False((bool)attribution["Enabled"]!); + Assert.Equal(30, (int)attribution["BurstTicks"]!); + Assert.Equal(10, (int)attribution["IntervalSeconds"]!); + + // And the rest of the keys the file never had. + Assert.True((bool)config["Enabled"]!); + Assert.Equal("127.0.0.1", (string?)config["Bind"]); + Assert.True((bool)config["RuntimeMetrics"]!); + Assert.Equal(30, (int)config["ChunksRefreshSeconds"]!); + + // The key nothing in PulseConfig answers to does not survive the rewrite, which is why the + // mod warns about it rather than dropping it quietly. + Assert.Null(config["Colour"]); + } +} diff --git a/Pulse.Scenarios/data/configupgrade/pulse.json b/Pulse.Scenarios/data/configupgrade/pulse.json new file mode 100644 index 0000000..04758ea --- /dev/null +++ b/Pulse.Scenarios/data/configupgrade/pulse.json @@ -0,0 +1,4 @@ +{ + "Port": 39472, + "Colour": "green" +} diff --git a/Pulse.Tests/ConfigUpgradeTests.cs b/Pulse.Tests/ConfigUpgradeTests.cs new file mode 100644 index 0000000..673cc17 --- /dev/null +++ b/Pulse.Tests/ConfigUpgradeTests.cs @@ -0,0 +1,183 @@ +using Xunit; + +namespace Pulse.Tests; + +/// The comparison an upgrade turns on. Both sides are JSON text here, which is what the +/// two mods hand it: the file as the admin left it, and the config object as it was loaded. +public class ConfigUpgradeTests +{ + /// A 0.1.0 file against a 0.2.0 config: the whole Attribution block arrived in the + /// release the admin is upgrading to. + private const string OldFile = """ + { + "Enabled": true, + "Bind": "127.0.0.1", + "Port": 9464, + "RuntimeMetrics": true, + "ChunksRefreshSeconds": 30 + } + """; + + private const string CurrentConfig = """ + { + "Enabled": true, + "Bind": "127.0.0.1", + "Port": 9464, + "RuntimeMetrics": true, + "ChunksRefreshSeconds": 30, + "Attribution": { "Enabled": false, "BurstTicks": 30, "IntervalSeconds": 10 } + } + """; + + [Fact] + public void Compare_Reports_ABlockTheFilePredates() + { + ConfigDiff diff = ConfigUpgrade.Compare(OldFile, CurrentConfig); + + // The block by its own name, not its three children: the admin never had any of them, and + // naming them would only pad the log line. + Assert.Equal(["Attribution"], diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// The recursive half. A file that has the block but predates one key inside it gets + /// that one key named, and nothing else. + [Fact] + public void Compare_Reports_AKeyMissingFromAPresentBlock_ByItsPath() + { + const string file = """ + { + "Enabled": true, + "Attribution": { "Enabled": true, "IntervalSeconds": 10 } + } + """; + const string config = """ + { + "Enabled": true, + "Attribution": { "Enabled": true, "BurstTicks": 30, "IntervalSeconds": 10 } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Equal(["Attribution.BurstTicks"], diff.Missing); + Assert.Empty(diff.Unknown); + } + + [Fact] + public void Compare_Reports_AKeyTheConfigDoesNotKnow_AtEitherDepth() + { + const string file = """ + { + "Enabled": true, + "Colour": "green", + "Attribution": { "Enabled": true, "Burstticks": 5 } + } + """; + const string config = """ + { + "Enabled": true, + "Attribution": { "Enabled": true, "BurstTicks": 30 } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Equal(["Attribution.BurstTicks"], diff.Missing); + Assert.Equal(["Colour", "Attribution.Burstticks"], diff.Unknown); + } + + /// Key order and whitespace are the serializer's business, not the admin's, and a + /// reordered file must not read as an upgrade. + [Fact] + public void Compare_Ignores_KeyOrderAndFormatting() + { + const string file = """{"Port":9464,"Bind":"127.0.0.1","Attribution":{"BurstTicks":30,"Enabled":false}}"""; + const string config = """ + { + "Bind": "127.0.0.1", + "Port": 9464, + "Attribution": { + "Enabled": false, + "BurstTicks": 30 + } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// The admin's own settings are the whole point of the exercise: a file that differs + /// from the defaults in every value is complete, not out of date. + [Fact] + public void Compare_Ignores_Values() + { + const string file = """ + { + "Enabled": false, + "Bind": "0.0.0.0", + "Port": 19464, + "Attribution": { "Enabled": true, "BurstTicks": 5 } + } + """; + const string config = """ + { + "Enabled": true, + "Bind": "127.0.0.1", + "Port": 9464, + "Attribution": { "Enabled": false, "BurstTicks": 30 } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// A block that is not a block on disk stops the walk there. Whatever the admin put + /// in its place is theirs, and the rewrite replaces the lot with one default block. + [Fact] + public void Compare_Stops_AtAKeyThatIsAnObjectOnOnlyOneSide() + { + ConfigDiff diff = ConfigUpgrade.Compare("""{"Attribution": true}""", """{"Attribution": {"Enabled": false}}"""); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// Newtonsoft loads a file with comments and trailing commas without complaint, so a + /// server running on one must not silently stop getting new keys. + [Fact] + public void Compare_Reads_AFileWithCommentsAndATrailingComma() + { + const string file = """ + { + // the port the panel scrapes + "Port": 9464, + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, """{"Port": 9464, "Bind": "127.0.0.1"}"""); + + Assert.Equal(["Bind"], diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// Nothing missing means nothing to write, and text that is not a JSON object at all + /// has to land there too: rewriting a file this cannot read would destroy it. + [Theory] + [InlineData("not json at all")] + [InlineData("[1, 2, 3]")] + [InlineData("")] + public void Compare_Reports_Nothing_ForTextThatIsNotAJsonObject(string file) + { + ConfigDiff diff = ConfigUpgrade.Compare(file, """{"Port": 9464}"""); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } +} diff --git a/Pulse/ConfigUpgrade.cs b/Pulse/ConfigUpgrade.cs new file mode 100644 index 0000000..f849fa2 --- /dev/null +++ b/Pulse/ConfigUpgrade.cs @@ -0,0 +1,133 @@ +using System.Text.Json; +using System.Text.Json.Nodes; +using Vintagestory.API.Common; +using Vintagestory.API.Server; + +namespace Pulse; + +/// Keys the config file on disk does not carry, and keys it carries that the mod knows +/// nothing about. Dotted paths, so a key inside a nested block reads +/// Attribution.BurstTicks. +internal readonly record struct ConfigDiff(IReadOnlyList Missing, IReadOnlyList Unknown); + +/// Keeps the config file an admin already has in step with the keys a newer version of the +/// mod introduced. +/// Both mods compile this from the one source file: Pulse.Otlp links it rather than +/// referencing Pulse.dll, because the two mods deliberately share no assembly. +internal static class ConfigUpgrade +{ + private const string AddedKeys = + "{0} added these keys to {1} with their defaults: {2}. Everything already in the file was " + + "kept as it was."; + + private const string DroppedKeys = + "{0} does not know these keys in {1}, and rewriting the file has just dropped them: {2}. " + + "Check them for typos."; + + private const string IgnoredKeys = + "{0} does not know these keys in {1}: {2}. They do nothing; check them for typos."; + + private const string UpgradeFailed = + "{0} could not bring {1} up to date ({2}). The server runs on the values the file does " + + "have, with defaults for the rest."; + + /// Newtonsoft accepts both when it loads the config, so a file the game read happily + /// must not be one this refuses to look at. + private static readonly JsonDocumentOptions Lenient = + new() { CommentHandling = JsonCommentHandling.Skip, AllowTrailingCommas = true }; + + /// Adds whatever keys a newer version of the mod introduced to the config file the + /// admin already has, and says in the log what changed. + /// The file is rewritten from , which the loader filled with + /// defaults wherever the file was silent, so the write only ever adds: every value the admin + /// set is already in the object. It happens solely when a key is missing, because a complete + /// file must not be touched at all, not even its modification time, on a host that mounts + /// ModConfig read-only or tracks it. The same rewrite is what drops a key the config does not + /// know, which is why an unknown key is worth a warning rather than silence. + /// Nothing in here may take a server down over a config file, so a read or a write that + /// fails is one warning and then the server carries on unchanged. + public static void Upgrade(ICoreServerAPI api, T config, string filename, string modName) + where T : class + { + ConfigDiff diff; + try + { + // ToPrettyString is the very call StoreModConfig makes to write the file: Newtonsoft + // with no settings at all, so the keys compared here are the keys a rewrite produces. + string path = Path.Combine(api.GetOrCreateDataPath("ModConfig"), filename); + diff = Compare(File.ReadAllText(path), JsonUtil.ToPrettyString(config)); + if (diff.Missing.Count > 0) + { + api.StoreModConfig(config, filename); + api.Logger.Notification(AddedKeys, modName, filename, string.Join(", ", diff.Missing)); + } + } + catch (Exception e) + { + api.Logger.Warning(UpgradeFailed, modName, filename, e.Message); + return; + } + + if (diff.Unknown.Count > 0) + { + api.Logger.Warning( + diff.Missing.Count > 0 ? DroppedKeys : IgnoredKeys, + modName, filename, string.Join(", ", diff.Unknown)); + } + } + + /// Which keys of are absent from , + /// and which keys of are absent from . + /// Keys only, never values, so key order and formatting make no difference. A missing + /// block is reported by its own name and not walked: naming its children would only pad the log + /// line with keys the admin never had. Text that does not parse as a JSON object reports + /// nothing, which leaves the file alone rather than rewriting something unreadable. + public static ConfigDiff Compare(string onDisk, string loaded) + { + List missing = []; + List unknown = []; + if (Parse(onDisk) is { } file && Parse(loaded) is { } config) + { + Walk(file, config, string.Empty, missing, unknown); + } + + return new ConfigDiff(missing, unknown); + } + + private static void Walk( + JsonObject file, JsonObject config, string prefix, List missing, List unknown) + { + // This level before the blocks under it, so both lists read outermost key first. + foreach (KeyValuePair entry in file) + { + if (!config.ContainsKey(entry.Key)) + { + unknown.Add(prefix + entry.Key); + } + } + + foreach (KeyValuePair entry in config) + { + if (!file.TryGetPropertyValue(entry.Key, out JsonNode? theirs)) + { + missing.Add(prefix + entry.Key); + } + else if (entry.Value is JsonObject nested && theirs is JsonObject nestedFile) + { + Walk(nestedFile, nested, prefix + entry.Key + ".", missing, unknown); + } + } + } + + private static JsonObject? Parse(string json) + { + try + { + return JsonNode.Parse(json, nodeOptions: null, Lenient) as JsonObject; + } + catch (JsonException) + { + return null; + } + } +} diff --git a/Pulse/PulseModSystem.cs b/Pulse/PulseModSystem.cs index 19d60d8..d8384a8 100644 --- a/Pulse/PulseModSystem.cs +++ b/Pulse/PulseModSystem.cs @@ -103,7 +103,13 @@ public override void StartServerSide(ICoreServerAPI api) { sapi = api; - PulseConfig config = api.LoadModConfig(ConfigFile) ?? StoreDefaults(api); + PulseConfig? existing = api.LoadModConfig(ConfigFile); + PulseConfig config = existing ?? StoreDefaults(api); + if (existing != null) + { + ConfigUpgrade.Upgrade(api, config, ConfigFile, "Pulse"); + } + if (!config.Enabled) { api.Logger.Notification("Pulse is disabled in " + ConfigFile + ", nothing registered."); diff --git a/README.md b/README.md index 3b2da23..2ced3d6 100644 --- a/README.md +++ b/README.md @@ -220,6 +220,14 @@ costs; lower it only if you know why. `Attribution` is the per-mod breakdown des because it costs tick time; with it off, nothing in that section is registered and the engine's profiler is never touched. Every one of these takes a server restart. +Upgrading does not mean editing the file by hand. Each mod checks its config file at startup and +writes back any key it knows about that the file is missing, with that key's default; the values +you already set are kept exactly as they are, and the log lists what was added. A key neither mod +recognises does not survive that rewrite, so it is reported as a warning instead of disappearing +quietly: usually it is a typo, and the setting you meant has been running on its default. A file +that already holds every key is not written at all, which matters if you mount `ModConfig` +read-only or keep it under version control. + ## Scraping it ```yaml diff --git a/tools/mutation-check.sh b/tools/mutation-check.sh index c7c9876..5cac9b2 100755 --- a/tools/mutation-check.sh +++ b/tools/mutation-check.sh @@ -47,7 +47,7 @@ mutate() { #