From b1f4251d75238cafad5a4913acc6662adf3898cd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 23:54:31 -0400 Subject: [PATCH] =?UTF-8?q?fix(commons):=20resolve=20Commons-008=20?= =?UTF-8?q?=E2=80=94=20replace=20ValueTuple=20in=20SetConnectionBindingsCo?= =?UTF-8?q?mmand=20with=20named=20ConnectionBinding=20record=20(CLI,=20Man?= =?UTF-8?q?agementService,=20TemplateEngine,=20CentralUI)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/Commons/findings.md | 36 ++++++++----- .../Commands/InstanceCommands.cs | 6 +-- .../Pages/Deployment/InstanceConfigure.razor | 3 +- .../Messages/Management/InstanceCommands.cs | 10 +++- .../Services/InstanceService.cs | 3 +- .../InstanceArgumentParsingTests.cs | 5 +- .../ConnectionBindingSerializationTests.cs | 54 +++++++++++++++++++ .../Services/InstanceServiceTests.cs | 3 +- 8 files changed, 97 insertions(+), 23 deletions(-) create mode 100644 tests/ScadaLink.Commons.Tests/Messages/ConnectionBindingSerializationTests.cs diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index c1b33a2..4e02c8f 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -357,7 +357,7 @@ carve-out makes these types compliant by design. |--|--| | Severity | Low | | Category | Akka.NET conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs:10` | **Description** @@ -379,18 +379,26 @@ Replace the tuple with a small named record, e.g. **Resolution** -_Open — deferred to a coordinated multi-module change._ The finding is confirmed valid: -the `ValueTuple` is the only positional/tuple element in `Messages/` and is unfriendly to -REQ-COM-5a additive evolution. However, `SetConnectionBindingsCommand.Bindings` is not a -Commons-internal type — its `IReadOnlyList<(string,int)>` shape is part of a cross-module -contract consumed by `ScadaLink.CLI` (`InstanceCommands.TryParseBindings` builds a -`List<(string,int)>` and passes it to the constructor), `ScadaLink.ManagementService` -(`ManagementActor` forwards `cmd.Bindings`), and `ScadaLink.TemplateEngine` -(`InstanceService.SetConnectionBindingsAsync` takes an `IReadOnlyList<(string AttributeName, -int DataConnectionId)>` parameter). Introducing a `ConnectionBinding` record therefore -requires editing those three modules in lock-step, which is outside the scope of this -Commons-only review pass (the constraint forbids touching other modules' source). Left -Open and flagged for a follow-up cross-module change; the fix itself is unambiguous. +Resolved 2026-05-16 (commit ``) — confirmed against the source: the `ValueTuple` +was the only positional/tuple element in `Messages/` and `System.Text.Json` serializes it +as `Item1`/`Item2`, unfriendly to REQ-COM-5a additive evolution. Introduced a named record +`ConnectionBinding(string AttributeName, int DataConnectionId)` in +`Messages/Management/InstanceCommands.cs` (alongside the command, matching the existing +record-per-file convention) and changed `SetConnectionBindingsCommand.Bindings` to +`IReadOnlyList`. All consumers were updated in lock-step: `ScadaLink.CLI` +(`InstanceCommands.TryParseBindings` now builds a `List`), +`ScadaLink.TemplateEngine` (`InstanceService.SetConnectionBindingsAsync` parameter), +`ScadaLink.ManagementService` (`ManagementActor` forwards `cmd.Bindings` unchanged — the +new element type flows through), and one consumer the finding did not list, +`ScadaLink.CentralUI` (`InstanceConfigure.razor`'s `SaveBindings` built a `List<(string,int)>`). +A repo-wide `src/` scan confirms no other references remain. Regression tests added in +`ConnectionBindingSerializationTests` (`ConnectionBinding_SerializesWithNamedProperties` +asserts named `AttributeName`/`DataConnectionId` JSON properties and the absence of +`Item1`/`Item2`; `SetConnectionBindingsCommand_RoundTripsThroughJson` asserts a full +JSON round-trip); existing parse/forward tests (`ParseBindings_ValidJson_ReturnsPairs`, +`SetConnectionBindings_BulkAssignment_Success`) were updated to the new type. Affected +suites are green (Commons 226, CLI 77, ManagementService 55, TemplateEngine 287) and +`dotnet build ScadaLink.slnx` is clean. ### Commons-009 — `Component-Commons.md` is stale relative to the actual file set diff --git a/src/ScadaLink.CLI/Commands/InstanceCommands.cs b/src/ScadaLink.CLI/Commands/InstanceCommands.cs index efdf3ad..fd77b26 100644 --- a/src/ScadaLink.CLI/Commands/InstanceCommands.cs +++ b/src/ScadaLink.CLI/Commands/InstanceCommands.cs @@ -73,7 +73,7 @@ public static class InstanceCommands /// internal static bool TryParseBindings( string json, - out List<(string, int)>? bindings, + out List? bindings, out string? error) { bindings = null; @@ -88,7 +88,7 @@ public static class InstanceCommands return false; } - var result = new List<(string, int)>(pairs.Count); + var result = new List(pairs.Count); foreach (var pair in pairs) { if (pair.Count != 2) @@ -107,7 +107,7 @@ public static class InstanceCommands error = "The second element of each binding (dataConnectionId) must be an integer."; return false; } - result.Add((pair[0].GetString()!, connectionId)); + result.Add(new ConnectionBinding(pair[0].GetString()!, connectionId)); } bindings = result; diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor index ce46136..df20c6c 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor @@ -4,6 +4,7 @@ @using ScadaLink.Commons.Entities.Sites @using ScadaLink.Commons.Entities.Templates @using ScadaLink.Commons.Interfaces.Repositories +@using ScadaLink.Commons.Messages.Management @using ScadaLink.Commons.Types.Enums @using ScadaLink.TemplateEngine.Flattening @using ScadaLink.TemplateEngine.Services @@ -467,7 +468,7 @@ _saving = true; try { - var bindings = _bindingSelections.Select(kv => (kv.Key, kv.Value)).ToList(); + var bindings = _bindingSelections.Select(kv => new ConnectionBinding(kv.Key, kv.Value)).ToList(); var user = await GetCurrentUserAsync(); var result = await InstanceService.SetConnectionBindingsAsync(Id, bindings, user); if (result.IsSuccess) diff --git a/src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs b/src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs index 8a70822..573fe20 100644 --- a/src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs +++ b/src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs @@ -7,7 +7,15 @@ public record MgmtDeployInstanceCommand(int InstanceId); public record MgmtEnableInstanceCommand(int InstanceId); public record MgmtDisableInstanceCommand(int InstanceId); public record MgmtDeleteInstanceCommand(int InstanceId); -public record SetConnectionBindingsCommand(int InstanceId, IReadOnlyList<(string AttributeName, int DataConnectionId)> Bindings); +/// +/// A single attribute-to-data-connection binding carried by +/// . This is a named record (not a +/// ValueTuple) so it serializes with stable, named JSON properties and can +/// evolve additively per REQ-COM-5a. +/// +public record ConnectionBinding(string AttributeName, int DataConnectionId); + +public record SetConnectionBindingsCommand(int InstanceId, IReadOnlyList Bindings); public record SetInstanceOverridesCommand(int InstanceId, IReadOnlyDictionary Overrides); public record SetInstanceAreaCommand(int InstanceId, int? AreaId); diff --git a/src/ScadaLink.TemplateEngine/Services/InstanceService.cs b/src/ScadaLink.TemplateEngine/Services/InstanceService.cs index 42d0767..26e6409 100644 --- a/src/ScadaLink.TemplateEngine/Services/InstanceService.cs +++ b/src/ScadaLink.TemplateEngine/Services/InstanceService.cs @@ -1,6 +1,7 @@ using ScadaLink.Commons.Entities.Instances; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; +using ScadaLink.Commons.Messages.Management; using ScadaLink.Commons.Types; using ScadaLink.Commons.Types.Enums; using ScadaLink.TemplateEngine; @@ -276,7 +277,7 @@ public class InstanceService /// public async Task>> SetConnectionBindingsAsync( int instanceId, - IReadOnlyList<(string AttributeName, int DataConnectionId)> bindings, + IReadOnlyList bindings, string user, CancellationToken cancellationToken = default) { diff --git a/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs b/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs index b9d3c9d..34397c7 100644 --- a/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs +++ b/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs @@ -1,4 +1,5 @@ using ScadaLink.CLI.Commands; +using ScadaLink.Commons.Messages.Management; namespace ScadaLink.CLI.Tests; @@ -17,8 +18,8 @@ public class InstanceArgumentParsingTests Assert.True(ok); Assert.Null(error); Assert.Equal(2, bindings!.Count); - Assert.Equal(("Speed", 5), bindings[0]); - Assert.Equal(("Mode", 7), bindings[1]); + Assert.Equal(new ConnectionBinding("Speed", 5), bindings[0]); + Assert.Equal(new ConnectionBinding("Mode", 7), bindings[1]); } [Fact] diff --git a/tests/ScadaLink.Commons.Tests/Messages/ConnectionBindingSerializationTests.cs b/tests/ScadaLink.Commons.Tests/Messages/ConnectionBindingSerializationTests.cs new file mode 100644 index 0000000..821f921 --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Messages/ConnectionBindingSerializationTests.cs @@ -0,0 +1,54 @@ +using System.Text.Json; +using ScadaLink.Commons.Messages.Management; + +namespace ScadaLink.Commons.Tests.Messages; + +/// +/// Regression tests for Commons-008 — +/// previously declared its bindings as IReadOnlyList<(string, int)>. +/// A ValueTuple serializes as Item1/Item2 and cannot evolve +/// additively (REQ-COM-5a). It is now a named record. +/// +public class ConnectionBindingSerializationTests +{ + [Fact] + public void ConnectionBinding_SerializesWithNamedProperties() + { + var json = JsonSerializer.Serialize(new ConnectionBinding("Temperature", 42)); + + using var doc = JsonDocument.Parse(json); + Assert.Equal(JsonValueKind.String, doc.RootElement.GetProperty("AttributeName").ValueKind); + Assert.Equal("Temperature", doc.RootElement.GetProperty("AttributeName").GetString()); + Assert.Equal(42, doc.RootElement.GetProperty("DataConnectionId").GetInt32()); + + // The ValueTuple failure mode: Item1/Item2 must NOT appear. + Assert.False(doc.RootElement.TryGetProperty("Item1", out _)); + Assert.False(doc.RootElement.TryGetProperty("Item2", out _)); + } + + [Fact] + public void SetConnectionBindingsCommand_RoundTripsThroughJson() + { + var original = new SetConnectionBindingsCommand( + 7, + new List + { + new("Speed", 5), + new("Mode", 11), + }); + + var json = JsonSerializer.Serialize(original); + var deserialized = JsonSerializer.Deserialize(json); + + Assert.NotNull(deserialized); + Assert.Equal(7, deserialized!.InstanceId); + Assert.Equal(2, deserialized.Bindings.Count); + Assert.Equal("Speed", deserialized.Bindings[0].AttributeName); + Assert.Equal(5, deserialized.Bindings[0].DataConnectionId); + Assert.Equal("Mode", deserialized.Bindings[1].AttributeName); + Assert.Equal(11, deserialized.Bindings[1].DataConnectionId); + + // ConnectionBinding is a record: each element compares by value. + Assert.Equal(original.Bindings, deserialized.Bindings); + } +} diff --git a/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs b/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs index c3ae1b8..61a9a23 100644 --- a/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs +++ b/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs @@ -3,6 +3,7 @@ using ScadaLink.Commons.Entities.Instances; using ScadaLink.Commons.Entities.Templates; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; +using ScadaLink.Commons.Messages.Management; using ScadaLink.Commons.Types.Enums; using ScadaLink.TemplateEngine.Services; @@ -260,7 +261,7 @@ public class InstanceServiceTests _repoMock.Setup(r => r.SaveChangesAsync(It.IsAny())) .ReturnsAsync(1); - var bindings = new List<(string, int)> { ("Temp", 100), ("Pressure", 200) }; + var bindings = new List { new("Temp", 100), new("Pressure", 200) }; var result = await _sut.SetConnectionBindingsAsync(1, bindings, "admin"); Assert.True(result.IsSuccess);