Add new config keys to existing files at startup

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,

Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs

@@ -0,0 +1,28 @@
+using System.Text.Json.Nodes;
+using Atlas.XUnit;
+using Xunit;
+
+namespace Pulse.Otlp.Scenarios;
+
+/// <summary>The same upgrade on the other mod's file: a pulse-otlp.json written before
+/// <c>ServiceName</c> 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.</summary>
+[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<JsonObject>(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"]!);
+    }
+}

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
+}

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
+  }
+}

Pulse.Otlp/Pulse.Otlp.csproj

@@ -16,6 +16,13 @@
     <Reference Include="VintagestoryAPI" HintPath="$(VINTAGE_STORY)/VintagestoryAPI.dll" Private="false" />
   </ItemGroup>
 
+  <ItemGroup>
+    <!-- Shared as source, not as a reference: the two mods meet at a meter name and nothing else
+         (see the remarks on PulseOtlpModSystem), so each assembly compiles its own internal copy
+         of the config upgrade rather than one of them depending on the other's dll. -->
+    <Compile Include="../Pulse/ConfigUpgrade.cs" Link="ConfigUpgrade.cs" />
+  </ItemGroup>
+
   <ItemGroup>
     <None Include="modinfo.json" CopyToOutputDirectory="PreserveNewest" />
   </ItemGroup>

Pulse.Otlp/PulseOtlpModSystem.cs

@@ -29,7 +29,13 @@ public sealed class PulseOtlpModSystem : ModSystem
 
     public override void StartServerSide(ICoreServerAPI api)
     {
-        PulseOtlpConfig config = api.LoadModConfig<PulseOtlpConfig>(ConfigFile) ?? StoreDefaults(api);
+        PulseOtlpConfig? existing = api.LoadModConfig<PulseOtlpConfig>(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.");

Pulse.Scenarios/ConfigUpgradeScenarios.cs

@@ -0,0 +1,44 @@
+using System.Text.Json.Nodes;
+using Atlas.XUnit;
+using Xunit;
+
+namespace Pulse.Scenarios;
+
+/// <summary>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.</summary>
+[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<JsonObject>(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<JsonObject>(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"]);
+    }
+}

Pulse.Scenarios/data/configupgrade/pulse.json

@@ -0,0 +1,4 @@
+{
+  "Port": 39472,
+  "Colour": "green"
+}

Pulse.Tests/ConfigUpgradeTests.cs

@@ -0,0 +1,183 @@
+using Xunit;
+
+namespace Pulse.Tests;
+
+/// <summary>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.</summary>
+public class ConfigUpgradeTests
+{
+    /// <summary>A 0.1.0 file against a 0.2.0 config: the whole Attribution block arrived in the
+    /// release the admin is upgrading to.</summary>
+    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);
+    }
+
+    /// <summary>The recursive half. A file that has the block but predates one key inside it gets
+    /// that one key named, and nothing else.</summary>
+    [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);
+    }
+
+    /// <summary>Key order and whitespace are the serializer's business, not the admin's, and a
+    /// reordered file must not read as an upgrade.</summary>
+    [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);
+    }
+
+    /// <summary>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.</summary>
+    [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);
+    }
+
+    /// <summary>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.</summary>
+    [Fact]
+    public void Compare_Stops_AtAKeyThatIsAnObjectOnOnlyOneSide()
+    {
+        ConfigDiff diff = ConfigUpgrade.Compare("""{"Attribution": true}""", """{"Attribution": {"Enabled": false}}""");
+
+        Assert.Empty(diff.Missing);
+        Assert.Empty(diff.Unknown);
+    }
+
+    /// <summary>Newtonsoft loads a file with comments and trailing commas without complaint, so a
+    /// server running on one must not silently stop getting new keys.</summary>
+    [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);
+    }
+
+    /// <summary>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.</summary>
+    [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);
+    }
+}