From 6f0d2ca49933a91f15f3142e46a5822b3d5c776b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 21 May 2026 04:00:20 -0400 Subject: [PATCH] refactor(auditlog): consolidate SiteCall DTO mapper into Communication Extract the verbatim-duplicated SiteCallOperationalDto -> SiteCall mapper into a single public SiteCallDtoMapper static class in ScadaLink.Communication.Grpc, mirroring AuditEventDtoMapper. Replaces three identical private copies (SiteStreamGrpcServer.MapSiteCallFromDto, ClusterClientSiteAuditClient.MapSiteCall, and the test-infra DirectActorSiteStreamAuditClient.MapSiteCallFromDto), removes the now-stale doc comment that justified the duplication, and drops the using directives that became unused. Adds SiteCallDtoMapperTests for field-by-field coverage. Only the FromDto direction is provided: nothing maps SiteCall back onto the wire, so a ToDto would be dead code. --- .../Telemetry/ClusterClientSiteAuditClient.cs | 30 +--- .../Grpc/SiteCallDtoMapper.cs | 70 +++++++++ .../Grpc/SiteStreamGrpcServer.cs | 28 +--- .../DirectActorSiteStreamAuditClient.cs | 34 +---- .../SiteCallDtoMapperTests.cs | 135 ++++++++++++++++++ 5 files changed, 211 insertions(+), 86 deletions(-) create mode 100644 src/ScadaLink.Communication/Grpc/SiteCallDtoMapper.cs create mode 100644 tests/ScadaLink.Communication.Tests/SiteCallDtoMapperTests.cs diff --git a/src/ScadaLink.AuditLog/Site/Telemetry/ClusterClientSiteAuditClient.cs b/src/ScadaLink.AuditLog/Site/Telemetry/ClusterClientSiteAuditClient.cs index 2bf5f43..492065e 100644 --- a/src/ScadaLink.AuditLog/Site/Telemetry/ClusterClientSiteAuditClient.cs +++ b/src/ScadaLink.AuditLog/Site/Telemetry/ClusterClientSiteAuditClient.cs @@ -1,7 +1,6 @@ using Akka.Actor; using ScadaLink.Commons.Entities.Audit; using ScadaLink.Commons.Messages.Audit; -using ScadaLink.Commons.Types; using ScadaLink.Communication.Grpc; namespace ScadaLink.AuditLog.Site.Telemetry; @@ -92,7 +91,7 @@ public sealed class ClusterClientSiteAuditClient : ISiteStreamAuditClient foreach (var packet in batch.Packets) { var audit = AuditEventDtoMapper.FromDto(packet.AuditEvent); - var siteCall = MapSiteCall(packet.Operational); + var siteCall = SiteCallDtoMapper.FromDto(packet.Operational); entries.Add(new CachedTelemetryEntry(audit, siteCall)); } @@ -115,31 +114,4 @@ public sealed class ClusterClientSiteAuditClient : ISiteStreamAuditClient } return ack; } - - /// - /// Translates a into the - /// persistence entity. Mirrors - /// SiteStreamGrpcServer.MapSiteCallFromDto — there is no shared - /// mapper because that lives in ScadaLink.Communication as a private - /// helper. is a placeholder; the - /// central AuditLogIngestActor overwrites it inside the dual-write - /// transaction so the AuditLog and SiteCalls rows share one instant. - /// - private static SiteCall MapSiteCall(SiteCallOperationalDto dto) => new() - { - TrackedOperationId = TrackedOperationId.Parse(dto.TrackedOperationId), - Channel = dto.Channel, - Target = dto.Target, - SourceSite = dto.SourceSite, - Status = dto.Status, - RetryCount = dto.RetryCount, - LastError = string.IsNullOrEmpty(dto.LastError) ? null : dto.LastError, - HttpStatus = dto.HttpStatus, - CreatedAtUtc = DateTime.SpecifyKind(dto.CreatedAtUtc.ToDateTime(), DateTimeKind.Utc), - UpdatedAtUtc = DateTime.SpecifyKind(dto.UpdatedAtUtc.ToDateTime(), DateTimeKind.Utc), - TerminalAtUtc = dto.TerminalAtUtc is null - ? null - : DateTime.SpecifyKind(dto.TerminalAtUtc.ToDateTime(), DateTimeKind.Utc), - IngestedAtUtc = DateTime.UtcNow, // overwritten by AuditLogIngestActor - }; } diff --git a/src/ScadaLink.Communication/Grpc/SiteCallDtoMapper.cs b/src/ScadaLink.Communication/Grpc/SiteCallDtoMapper.cs new file mode 100644 index 0000000..c61e3e5 --- /dev/null +++ b/src/ScadaLink.Communication/Grpc/SiteCallDtoMapper.cs @@ -0,0 +1,70 @@ +using ScadaLink.Commons.Entities.Audit; +using ScadaLink.Commons.Types; + +namespace ScadaLink.Communication.Grpc; + +/// +/// Canonical bridge for Site Call Audit (#22) operational rows between the +/// wire-format exchanged on the +/// CachedCallTelemetry packet and the in-process +/// persistence entity central writes into the SiteCalls table. +/// +/// +/// +/// This mapper lives in ScadaLink.Communication (which owns the generated +/// and references Commons for +/// ) so both SiteStreamGrpcServer and +/// ScadaLink.AuditLog can share one implementation without the +/// project-reference cycle that would result from hosting it in +/// ScadaLink.AuditLog (AuditLog → Communication, never the reverse). +/// Mirrors the sibling . +/// +/// +/// Only the DTO→entity direction is provided: nothing in the system maps a +/// back onto the wire (sites emit the operational state +/// from SiteCallOperational, never from the central +/// entity), so an entity→DTO method would be dead code. +/// +/// +/// String nullability convention: proto3 scalar strings cannot be absent, so the +/// optional rehydrates from an empty string back +/// to null. The optional HttpStatus and TerminalAtUtc use proto +/// wrappers so they preserve true null semantics. +/// +/// +public static class SiteCallDtoMapper +{ + /// + /// Reconstructs a persistence entity from its + /// wire-format DTO. An empty LastError rehydrates as null; absent + /// HttpStatus/TerminalAtUtc wrappers stay null. + /// + /// + /// is stamped here as a placeholder + /// (); the central ingest actor overwrites it + /// inside the dual-write transaction so the AuditLog and SiteCalls rows + /// share one instant. The value sent on the wire is informational only. + /// + public static SiteCall FromDto(SiteCallOperationalDto dto) + { + ArgumentNullException.ThrowIfNull(dto); + + return new SiteCall + { + TrackedOperationId = TrackedOperationId.Parse(dto.TrackedOperationId), + Channel = dto.Channel, + Target = dto.Target, + SourceSite = dto.SourceSite, + Status = dto.Status, + RetryCount = dto.RetryCount, + LastError = string.IsNullOrEmpty(dto.LastError) ? null : dto.LastError, + HttpStatus = dto.HttpStatus, + CreatedAtUtc = DateTime.SpecifyKind(dto.CreatedAtUtc.ToDateTime(), DateTimeKind.Utc), + UpdatedAtUtc = DateTime.SpecifyKind(dto.UpdatedAtUtc.ToDateTime(), DateTimeKind.Utc), + TerminalAtUtc = dto.TerminalAtUtc is null + ? null + : DateTime.SpecifyKind(dto.TerminalAtUtc.ToDateTime(), DateTimeKind.Utc), + IngestedAtUtc = DateTime.UtcNow, // overwritten by AuditLogIngestActor + }; + } +} diff --git a/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs b/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs index 23a19d8..e75db33 100644 --- a/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs +++ b/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs @@ -7,7 +7,6 @@ using Microsoft.Extensions.Options; using ScadaLink.Commons.Entities.Audit; using ScadaLink.Commons.Interfaces.Services; using ScadaLink.Commons.Messages.Audit; -using ScadaLink.Commons.Types; using GrpcStatus = Grpc.Core.Status; namespace ScadaLink.Communication.Grpc; @@ -326,7 +325,7 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase foreach (var packet in request.Packets) { var auditEvent = AuditEventDtoMapper.FromDto(packet.AuditEvent); - var siteCall = MapSiteCallFromDto(packet.Operational); + var siteCall = SiteCallDtoMapper.FromDto(packet.Operational); entries.Add(new CachedTelemetryEntry(auditEvent, siteCall)); } @@ -451,31 +450,6 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase return response; } - /// - /// Translates a into the persistence - /// entity. is stamped here as a - /// placeholder; the central ingest actor overwrites it inside the - /// dual-write transaction so the AuditLog and SiteCalls rows share one - /// instant. - /// - private static SiteCall MapSiteCallFromDto(SiteCallOperationalDto dto) => new() - { - TrackedOperationId = TrackedOperationId.Parse(dto.TrackedOperationId), - Channel = dto.Channel, - Target = dto.Target, - SourceSite = dto.SourceSite, - Status = dto.Status, - RetryCount = dto.RetryCount, - LastError = string.IsNullOrEmpty(dto.LastError) ? null : dto.LastError, - HttpStatus = dto.HttpStatus, - CreatedAtUtc = DateTime.SpecifyKind(dto.CreatedAtUtc.ToDateTime(), DateTimeKind.Utc), - UpdatedAtUtc = DateTime.SpecifyKind(dto.UpdatedAtUtc.ToDateTime(), DateTimeKind.Utc), - TerminalAtUtc = dto.TerminalAtUtc is null - ? null - : DateTime.SpecifyKind(dto.TerminalAtUtc.ToDateTime(), DateTimeKind.Utc), - IngestedAtUtc = DateTime.UtcNow, // overwritten by AuditLogIngestActor - }; - /// /// Tracks a single active stream so cleanup only removes its own entry. /// diff --git a/tests/ScadaLink.AuditLog.Tests/Integration/Infrastructure/DirectActorSiteStreamAuditClient.cs b/tests/ScadaLink.AuditLog.Tests/Integration/Infrastructure/DirectActorSiteStreamAuditClient.cs index cb5e455..9fa1482 100644 --- a/tests/ScadaLink.AuditLog.Tests/Integration/Infrastructure/DirectActorSiteStreamAuditClient.cs +++ b/tests/ScadaLink.AuditLog.Tests/Integration/Infrastructure/DirectActorSiteStreamAuditClient.cs @@ -2,7 +2,6 @@ using Akka.Actor; using ScadaLink.AuditLog.Site.Telemetry; using ScadaLink.Commons.Entities.Audit; using ScadaLink.Commons.Messages.Audit; -using ScadaLink.Commons.Types; using ScadaLink.Communication.Grpc; namespace ScadaLink.AuditLog.Tests.Integration.Infrastructure; @@ -113,10 +112,9 @@ public sealed class DirectActorSiteStreamAuditClient : ISiteStreamAuditClient /// back into the proto ack. /// /// - /// Uses the shared for the audit half; - /// the SiteCall DTO is decoded inline because the AuditLog mapper does not - /// (and should not) know about — the - /// production gRPC server (Bundle D) uses the same inline shape. + /// Uses the shared for the audit half + /// and for the SiteCall half — the same + /// canonical mappers the production SiteStreamGrpcServer uses. /// public async Task IngestCachedTelemetryAsync(CachedTelemetryBatch batch, CancellationToken ct) { @@ -132,7 +130,7 @@ public sealed class DirectActorSiteStreamAuditClient : ISiteStreamAuditClient foreach (var packet in batch.Packets) { var audit = AuditEventDtoMapper.FromDto(packet.AuditEvent); - var siteCall = MapSiteCallFromDto(packet.Operational); + var siteCall = SiteCallDtoMapper.FromDto(packet.Operational); entries.Add(new CachedTelemetryEntry(audit, siteCall)); } @@ -149,28 +147,4 @@ public sealed class DirectActorSiteStreamAuditClient : ISiteStreamAuditClient } return ack; } - - /// - /// Mirrors SiteStreamGrpcServer.MapSiteCallFromDto — keep the two in - /// sync. The placeholder stamped here - /// is overwritten by the central ingest actor inside the dual-write - /// transaction, so the value sent on the wire is informational only. - /// - private static SiteCall MapSiteCallFromDto(SiteCallOperationalDto dto) => new() - { - TrackedOperationId = TrackedOperationId.Parse(dto.TrackedOperationId), - Channel = dto.Channel, - Target = dto.Target, - SourceSite = dto.SourceSite, - Status = dto.Status, - RetryCount = dto.RetryCount, - LastError = string.IsNullOrEmpty(dto.LastError) ? null : dto.LastError, - HttpStatus = dto.HttpStatus, - CreatedAtUtc = DateTime.SpecifyKind(dto.CreatedAtUtc.ToDateTime(), DateTimeKind.Utc), - UpdatedAtUtc = DateTime.SpecifyKind(dto.UpdatedAtUtc.ToDateTime(), DateTimeKind.Utc), - TerminalAtUtc = dto.TerminalAtUtc is null - ? null - : DateTime.SpecifyKind(dto.TerminalAtUtc.ToDateTime(), DateTimeKind.Utc), - IngestedAtUtc = DateTime.UtcNow, - }; } diff --git a/tests/ScadaLink.Communication.Tests/SiteCallDtoMapperTests.cs b/tests/ScadaLink.Communication.Tests/SiteCallDtoMapperTests.cs new file mode 100644 index 0000000..4de1d37 --- /dev/null +++ b/tests/ScadaLink.Communication.Tests/SiteCallDtoMapperTests.cs @@ -0,0 +1,135 @@ +using Google.Protobuf.WellKnownTypes; +using ScadaLink.Communication.Grpc; + +namespace ScadaLink.Communication.Tests; + +/// +/// Field-coverage + edge tests for the that +/// decodes (proto) into the +/// persistence entity. +/// Only the DTO→entity direction exists — nothing in the system maps a +/// SiteCall back onto the wire — so there is no round-trip test. +/// IngestedAtUtc is a site-side placeholder the central ingest actor +/// overwrites, so it is asserted as "recent UTC" rather than a fixed value. +/// +public class SiteCallDtoMapperTests +{ + [Fact] + public void FromDto_FullyPopulated_MapsEveryField() + { + var trackedOperationId = Guid.NewGuid(); + var createdAt = new DateTime(2026, 5, 20, 10, 0, 0, DateTimeKind.Utc); + var updatedAt = new DateTime(2026, 5, 20, 10, 5, 0, DateTimeKind.Utc); + var terminalAt = new DateTime(2026, 5, 20, 10, 10, 0, DateTimeKind.Utc); + + var dto = new SiteCallOperationalDto + { + TrackedOperationId = trackedOperationId.ToString(), + Channel = "ApiOutbound", + Target = "ERP.GetOrder", + SourceSite = "site-melbourne", + Status = "Delivered", + RetryCount = 3, + LastError = "transient 503", + HttpStatus = 200, + CreatedAtUtc = Timestamp.FromDateTime(createdAt), + UpdatedAtUtc = Timestamp.FromDateTime(updatedAt), + TerminalAtUtc = Timestamp.FromDateTime(terminalAt), + }; + + var entity = SiteCallDtoMapper.FromDto(dto); + + Assert.Equal(trackedOperationId, entity.TrackedOperationId.Value); + Assert.Equal("ApiOutbound", entity.Channel); + Assert.Equal("ERP.GetOrder", entity.Target); + Assert.Equal("site-melbourne", entity.SourceSite); + Assert.Equal("Delivered", entity.Status); + Assert.Equal(3, entity.RetryCount); + Assert.Equal("transient 503", entity.LastError); + Assert.Equal(200, entity.HttpStatus); + Assert.Equal(createdAt, entity.CreatedAtUtc); + Assert.Equal(updatedAt, entity.UpdatedAtUtc); + Assert.Equal(terminalAt, entity.TerminalAtUtc); + } + + [Fact] + public void FromDto_EmptyLastError_BecomesNull() + { + var dto = NewMinimalDto(); + dto.LastError = string.Empty; + + var entity = SiteCallDtoMapper.FromDto(dto); + + Assert.Null(entity.LastError); + } + + [Fact] + public void FromDto_AbsentHttpStatus_StaysNull() + { + // Int32Value wrapper unset on the wire — preserves true null semantics + // for non-API cached writes. + var dto = NewMinimalDto(); + + Assert.Null(dto.HttpStatus); + + var entity = SiteCallDtoMapper.FromDto(dto); + + Assert.Null(entity.HttpStatus); + } + + [Fact] + public void FromDto_AbsentTerminalAt_StaysNull() + { + // Timestamp wrapper unset while the call is still active. + var dto = NewMinimalDto(); + + Assert.Null(dto.TerminalAtUtc); + + var entity = SiteCallDtoMapper.FromDto(dto); + + Assert.Null(entity.TerminalAtUtc); + } + + [Fact] + public void FromDto_Timestamps_RehydrateAsUtcKind() + { + var dto = NewMinimalDto(); + + var entity = SiteCallDtoMapper.FromDto(dto); + + Assert.Equal(DateTimeKind.Utc, entity.CreatedAtUtc.Kind); + Assert.Equal(DateTimeKind.Utc, entity.UpdatedAtUtc.Kind); + } + + [Fact] + public void FromDto_IngestedAtUtc_StampedAsRecentPlaceholder() + { + // IngestedAtUtc is a site-side DateTime.UtcNow placeholder; the central + // ingest actor overwrites it inside the dual-write transaction. + var before = DateTime.UtcNow; + + var entity = SiteCallDtoMapper.FromDto(NewMinimalDto()); + + var after = DateTime.UtcNow; + Assert.InRange(entity.IngestedAtUtc, before, after); + Assert.Equal(DateTimeKind.Utc, entity.IngestedAtUtc.Kind); + } + + [Fact] + public void FromDto_Null_Throws() + { + Assert.Throws(() => SiteCallDtoMapper.FromDto(null!)); + } + + private static SiteCallOperationalDto NewMinimalDto() => new() + { + TrackedOperationId = Guid.NewGuid().ToString(), + Channel = "DbOutbound", + Target = "warehouse.dbo.WriteOrder", + SourceSite = "site-brisbane", + Status = "Submitted", + RetryCount = 0, + CreatedAtUtc = Timestamp.FromDateTime(DateTime.UtcNow), + UpdatedAtUtc = Timestamp.FromDateTime(DateTime.UtcNow), + }; +}