diff --git a/docs/requirements/Component-CLI.md b/docs/requirements/Component-CLI.md index 77cdf04f..1b209303 100644 --- a/docs/requirements/Component-CLI.md +++ b/docs/requirements/Component-CLI.md @@ -241,6 +241,13 @@ warning; migrate scripts to `audit-config`. scadabridge audit-config query [--user ] [--entity-type ] [--action ] [--from ] [--to ] [--page ] [--page-size ] ``` +The response is an `AuditLogQueryResult` — `{ "entries": [ … ], "totalCount": }`, where +`totalCount` is the unpaged match count. The result type is a named record specifically +because the handler must not return the repository's `(Entries, TotalCount)` ValueTuple: +`System.Text.Json` serializes properties only, so a tuple result renders as the literal +`{}` and every query looks empty regardless of how many rows matched. `ManagementActor.SerializeResult` +now rejects tuple results outright so the failure mode cannot recur silently. + The new centralized Audit Log component (#23) is exposed via the `scadabridge audit` group below. ### Centralized Audit Commands diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index c4d7f802..0f94170a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -1802,6 +1802,28 @@ scadabridge --url audit-config query [options] | `--page` | no | `1` | Page number | | `--page-size` | no | `50` | Results per page | +Output shape: + +```json +{ + "entries": [ + { + "id": 412, + "user": "multi-role", + "action": "Created", + "entityType": "Template", + "entityId": "7", + "entityName": "PumpTemplate", + "timestamp": "2026-08-15T12:00:00+00:00" + } + ], + "totalCount": 137 +} +``` + +`totalCount` is the unpaged number of rows matching the filters, so a client can page +without a second call. + --- ### `shared-script` — Manage shared scripts diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/AuditLogCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/AuditLogCommands.cs index 402dbe25..2d00ca7c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/AuditLogCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/AuditLogCommands.cs @@ -1,6 +1,25 @@ +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Audit; + namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; public record QueryAuditLogCommand( string? User = null, string? EntityType = null, string? Action = null, DateTimeOffset? From = null, DateTimeOffset? To = null, int Page = 1, int PageSize = 50); + +/// +/// Result wrapper for . +/// +/// +/// This record exists specifically so the handler does not return the repository's +/// (IReadOnlyList<AuditLogEntry> Entries, int TotalCount) ValueTuple directly. +/// A ValueTuple exposes Item1/Item2 as public fields, and +/// serializes only properties unless +/// JsonSerializerOptions.IncludeFields is set — so a tuple result serialized to +/// the literal string {} and every audit-config query looked empty no +/// matter how many rows the query matched (deferred-work register row 46). Mirrors the +/// SecuredWriteListResult shape used by the other paged management query. +/// +/// The page of configuration-change audit entries. +/// Total number of rows matching the filters (unpaged). +public record AuditLogQueryResult(IReadOnlyList Entries, int TotalCount); diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 5a6c8f66..8c3bc658 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -90,10 +90,35 @@ public class ManagementActor : ReceiveActor /// Serializes a command result to JSON using — the same /// serializer the HTTP endpoint uses — with cycle-safe settings. /// + /// + /// Rejects tuple results outright. writes only + /// properties by default, and a ValueTuple exposes its members as public + /// fields — so a handler that returns a repository's + /// (rows, totalCount) tuple serializes to the literal {}, silently + /// handing every caller an empty result with no error anywhere in the stack. That is + /// exactly how audit-config query shipped broken (deferred-work register row + /// 46). Failing loudly here converts the whole class of bug into an immediate, + /// obvious error the first time the handler is exercised; the fix is always to + /// project the tuple into a named result record before returning it. + /// /// The command result object to serialize, or null. /// The JSON string representation of , or "null" when null. - public static string SerializeResult(object? result) => - JsonSerializer.Serialize(result, ResultSerializerOptions); + /// + /// Thrown when is a tuple, which cannot round-trip through + /// the JSON contract. + /// + public static string SerializeResult(object? result) + { + if (result is System.Runtime.CompilerServices.ITuple) + { + throw new InvalidOperationException( + $"Management command results must be named types, not tuples — " + + $"System.Text.Json serializes '{result.GetType().Name}' to '{{}}' because tuple " + + "members are fields, not properties. Project the tuple into a result record."); + } + + return JsonSerializer.Serialize(result, ResultSerializerOptions); + } private void HandleEnvelope(ManagementEnvelope envelope) { @@ -2578,12 +2603,24 @@ public class ManagementActor : ReceiveActor // Audit Log handler // ======================================================================== + /// + /// Queries the configuration-change audit trail and projects the repository's + /// ValueTuple into . + /// + /// + /// The projection is load-bearing, not cosmetic: returning the repository tuple + /// directly made emit the literal {} (ValueTuple + /// exposes Item1/Item2 as fields, which System.Text.Json skips), so + /// audit-config query reported an empty result for every filter combination + /// regardless of how many rows matched — deferred-work register row 46. + /// private static async Task HandleQueryAuditLog(IServiceProvider sp, QueryAuditLogCommand cmd) { var repo = sp.GetRequiredService(); - return await repo.GetAuditLogEntriesAsync( + var (entries, totalCount) = await repo.GetAuditLogEntriesAsync( cmd.User, cmd.EntityType, cmd.Action, cmd.From, cmd.To, page: cmd.Page, pageSize: cmd.PageSize); + return new AuditLogQueryResult(entries, totalCount); } // ======================================================================== diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 84ff8003..4406edd3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -238,6 +238,85 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Equal(envelope.CorrelationId, response.CorrelationId); } + // ======================================================================== + // 2b. Configuration-change audit query -- result must not serialize to "{}" + // ======================================================================== + + /// + /// Deferred-work register row 46: audit-config query returned the literal + /// {} for every filter combination against the rig even though the + /// AuditLogEntries rows were present. The handler returned the repository's + /// (Entries, TotalCount) ValueTuple straight to SerializeResult, and + /// System.Text.Json writes properties only — a tuple's Item1/Item2 are + /// fields, so the whole payload vanished with no error anywhere. Pins that the rows + /// and the total actually reach the wire. + /// + [Fact] + public void QueryAuditLogCommand_ReturnsEntriesAndTotalCount() + { + var uiRepo = Substitute.For(); + var entries = new List + { + new("alice", "Created", "Template", "7", "PumpTemplate") + { + Id = 1, + Timestamp = new DateTimeOffset(2026, 8, 15, 12, 0, 0, TimeSpan.Zero) + } + }; + uiRepo.GetAuditLogEntriesAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(((IReadOnlyList)entries, 137)); + _services.AddScoped(_ => uiRepo); + + var actor = CreateActor(); + var envelope = Envelope(new QueryAuditLogCommand(PageSize: 25), "Administrator"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.NotEqual("{}", response.JsonData); + Assert.Contains("PumpTemplate", response.JsonData); + Assert.Contains("\"entries\"", response.JsonData); + Assert.Contains("\"totalCount\":137", response.JsonData); + } + + /// + /// Guard for the whole class of bug behind row 46: any handler that returns a tuple + /// silently serializes to {}. + /// now rejects tuples outright so the next occurrence is a loud failure rather than + /// an empty response. + /// + [Fact] + public void SerializeResult_TupleResult_ThrowsInsteadOfEmittingEmptyObject() + { + var ex = Assert.Throws( + () => ManagementActor.SerializeResult((new[] { "a" }, 1))); + + Assert.Contains("tuple", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + /// + /// Pins the positive half of the guard: the named result record the handler now + /// returns serializes to a populated, camelCased object. + /// + [Fact] + public void SerializeResult_AuditLogQueryResult_EmitsEntriesAndTotalCount() + { + var json = ManagementActor.SerializeResult(new AuditLogQueryResult( + new List + { + new("bob", "Deleted", "Instance", "3", "Pump1") + }, + 42)); + + Assert.Contains("\"entries\"", json); + Assert.Contains("\"totalCount\":42", json); + Assert.Contains("Pump1", json); + } + // ======================================================================== // 3. Template list test -- verify data flows through // ========================================================================