From 1afab5efea784d5d72a5fba025a8585cffea1320 Mon Sep 17 00:00:00 2001 From: Warwick Schroeder Date: Tue, 1 Sep 2026 11:57:45 +0800 Subject: [PATCH 1/2] fix: change VersionFields to a method so serialisers dont persist it to the database. --- src/ServiceControl.Persistence/CustomCheck.cs | 2 +- src/ServiceControl.Persistence/EndpointsView.cs | 2 +- .../EventLog/EventLogItemView.cs | 2 +- src/ServiceControl.Persistence/FailureGroupView.cs | 2 +- src/ServiceControl.Persistence/GroupOperation.cs | 2 +- .../History/HistoricRetryOperation.cs | 2 +- .../History/UnacknowledgedRetryOperation.cs | 2 +- .../Infrastructure/DataVersion.cs | 2 +- .../Infrastructure/IVersionedRow.cs | 8 +++++--- .../MessageRedirects/MessageRedirect.cs | 2 +- src/ServiceControl.Persistence/QueueAddress.cs | 2 +- 11 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/ServiceControl.Persistence/CustomCheck.cs b/src/ServiceControl.Persistence/CustomCheck.cs index 3867e50e09..c76554ea81 100644 --- a/src/ServiceControl.Persistence/CustomCheck.cs +++ b/src/ServiceControl.Persistence/CustomCheck.cs @@ -14,7 +14,7 @@ public class CustomCheck : IVersionedRow public DateTime ReportedAt { get; set; } public string? FailureReason { get; set; } public EndpointDetails? OriginatingEndpoint { get; set; } - object?[] IVersionedRow.VersionFields => + object?[] IVersionedRow.GetVersionFields() => [ Id, CustomCheckId, Category, Status, ReportedAt, FailureReason, OriginatingEndpoint?.Name, OriginatingEndpoint?.HostId, OriginatingEndpoint?.Host diff --git a/src/ServiceControl.Persistence/EndpointsView.cs b/src/ServiceControl.Persistence/EndpointsView.cs index a0d33ae15b..96ad335f25 100644 --- a/src/ServiceControl.Persistence/EndpointsView.cs +++ b/src/ServiceControl.Persistence/EndpointsView.cs @@ -12,7 +12,7 @@ public class EndpointsView : IVersionedRow public bool MonitorHeartbeat { get; set; } public HeartbeatInformation? HeartbeatInformation { get; set; } public bool IsSendingHeartbeats { get; set; } - object?[] IVersionedRow.VersionFields => + object?[] IVersionedRow.GetVersionFields() => [ Id, Name, HostDisplayName, Monitored, MonitorHeartbeat, IsSendingHeartbeats, HeartbeatInformation?.LastReportAt, HeartbeatInformation?.ReportedStatus diff --git a/src/ServiceControl.Persistence/EventLog/EventLogItemView.cs b/src/ServiceControl.Persistence/EventLog/EventLogItemView.cs index f65aeda5fb..8f0bd666f8 100644 --- a/src/ServiceControl.Persistence/EventLog/EventLogItemView.cs +++ b/src/ServiceControl.Persistence/EventLog/EventLogItemView.cs @@ -19,7 +19,7 @@ public class EventLogItemView : IVersionedRow public List RelatedTo { get; set; } = []; public required string Category { get; set; } public required string EventType { get; set; } - object?[] IVersionedRow.VersionFields => + object?[] IVersionedRow.GetVersionFields() => [Id, Description, Severity, RaisedAt, Category, EventType, .. RelatedTo]; } } diff --git a/src/ServiceControl.Persistence/FailureGroupView.cs b/src/ServiceControl.Persistence/FailureGroupView.cs index 2106808720..8d668c3ad6 100644 --- a/src/ServiceControl.Persistence/FailureGroupView.cs +++ b/src/ServiceControl.Persistence/FailureGroupView.cs @@ -12,6 +12,6 @@ public class FailureGroupView : IVersionedRow public string? Comment { get; set; } public DateTime First { get; set; } public DateTime Last { get; set; } - object?[] IVersionedRow.VersionFields => [Id, Title, Type, Count, Comment, First, Last]; + object?[] IVersionedRow.GetVersionFields() => [Id, Title, Type, Count, Comment, First, Last]; } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence/GroupOperation.cs b/src/ServiceControl.Persistence/GroupOperation.cs index baa780f6ee..10b00582eb 100644 --- a/src/ServiceControl.Persistence/GroupOperation.cs +++ b/src/ServiceControl.Persistence/GroupOperation.cs @@ -18,7 +18,7 @@ public class GroupOperation : IVersionedRow public DateTime? OperationStartTime { get; set; } public DateTime? OperationCompletionTime { get; set; } public bool NeedUserAcknowledgement { get; set; } - object?[] IVersionedRow.VersionFields => + object?[] IVersionedRow.GetVersionFields() => [ Id, Title, Type, Count, Comment, First, Last, OperationStatus, OperationFailed, OperationProgress, OperationMessagesCompletedCount, OperationRemainingCount, OperationStartTime, OperationCompletionTime, diff --git a/src/ServiceControl.Persistence/History/HistoricRetryOperation.cs b/src/ServiceControl.Persistence/History/HistoricRetryOperation.cs index 87b6f882b3..34df87d8c4 100644 --- a/src/ServiceControl.Persistence/History/HistoricRetryOperation.cs +++ b/src/ServiceControl.Persistence/History/HistoricRetryOperation.cs @@ -13,7 +13,7 @@ public class HistoricRetryOperation : IVersionedRow public string? Originator { get; set; } public bool Failed { get; set; } public int NumberOfMessagesProcessed { get; set; } - object?[] IVersionedRow.VersionFields => + object?[] IVersionedRow.GetVersionFields() => [RequestId, RetryType, StartTime, CompletionTime, Originator, Failed, NumberOfMessagesProcessed]; } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence/History/UnacknowledgedRetryOperation.cs b/src/ServiceControl.Persistence/History/UnacknowledgedRetryOperation.cs index f67ab11834..e3ee1f5d23 100644 --- a/src/ServiceControl.Persistence/History/UnacknowledgedRetryOperation.cs +++ b/src/ServiceControl.Persistence/History/UnacknowledgedRetryOperation.cs @@ -15,7 +15,7 @@ public class UnacknowledgedRetryOperation : IVersionedRow public string? Classifier { get; set; } public bool Failed { get; set; } public int NumberOfMessagesProcessed { get; set; } - object?[] IVersionedRow.VersionFields => + object?[] IVersionedRow.GetVersionFields() => [RequestId, RetryType, StartTime, CompletionTime, Last, Originator, Classifier, Failed, NumberOfMessagesProcessed]; } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs b/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs index 51055362bd..d51bbd8d99 100644 --- a/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs +++ b/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs @@ -69,7 +69,7 @@ public static DataVersion OverRows((string Name, object? Value)[]? summary public static DataVersion OverRows((string Name, object? Value)[]? summary, IEnumerable rows) where TRow : IVersionedRow => - OverRows(summary, rows, row => row.VersionFields); + OverRows(summary, rows, row => row.GetVersionFields()); /// /// One version for a result gathered from several instances. Missing anywhere means missing overall. diff --git a/src/ServiceControl.Persistence/Infrastructure/IVersionedRow.cs b/src/ServiceControl.Persistence/Infrastructure/IVersionedRow.cs index c467397e95..055efce557 100644 --- a/src/ServiceControl.Persistence/Infrastructure/IVersionedRow.cs +++ b/src/ServiceControl.Persistence/Infrastructure/IVersionedRow.cs @@ -1,11 +1,13 @@ namespace ServiceControl.Persistence.Infrastructure { /// - /// A row the API sends to ServicePulse to be rendered. Every field the response shows has to appear in VersionFields, or it - /// can change without the cache tag moving and the client keeps a stale page. + /// A row the API sends to ServicePulse to be rendered. Every field the response shows has to appear in + /// GetVersionFields, or it can change without the cache tag moving and the client keeps a stale page. /// public interface IVersionedRow { - object?[] VersionFields { get; } + // A method, not a property: We dont want a serialiser to persist computed properties into the document, and these + // values are derived from the fields beside them. + object?[] GetVersionFields(); } } diff --git a/src/ServiceControl.Persistence/MessageRedirects/MessageRedirect.cs b/src/ServiceControl.Persistence/MessageRedirects/MessageRedirect.cs index 2fbf848dbf..6491b09f66 100644 --- a/src/ServiceControl.Persistence/MessageRedirects/MessageRedirect.cs +++ b/src/ServiceControl.Persistence/MessageRedirects/MessageRedirect.cs @@ -11,7 +11,7 @@ public class MessageRedirect : IVersionedRow public required string FromPhysicalAddress { get; set; } public required string ToPhysicalAddress { get; set; } public DateTime LastModified { get; set; } - object?[] IVersionedRow.VersionFields => [MessageRedirectId, ToPhysicalAddress, LastModified]; + object?[] IVersionedRow.GetVersionFields() => [MessageRedirectId, ToPhysicalAddress, LastModified]; static ConcurrentDictionary idCache = new ConcurrentDictionary(); } } diff --git a/src/ServiceControl.Persistence/QueueAddress.cs b/src/ServiceControl.Persistence/QueueAddress.cs index c74dba928a..8c4eb2b751 100644 --- a/src/ServiceControl.Persistence/QueueAddress.cs +++ b/src/ServiceControl.Persistence/QueueAddress.cs @@ -6,6 +6,6 @@ public class QueueAddress : IVersionedRow { public string? PhysicalAddress { get; set; } public int FailedMessageCount { get; set; } - object?[] IVersionedRow.VersionFields => [PhysicalAddress, FailedMessageCount]; + object?[] IVersionedRow.GetVersionFields() => [PhysicalAddress, FailedMessageCount]; } } \ No newline at end of file From d7f3eda5c8e548b4037b77621a30c2e72a0f58a4 Mon Sep 17 00:00:00 2001 From: Warwick Schroeder Date: Tue, 1 Sep 2026 12:33:45 +0800 Subject: [PATCH 2/2] fix: update serialization test to ensure only own fields are stored in the database --- .../Infrastructure/VersionedRowTests.cs | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/ServiceControl.UnitTests/Infrastructure/VersionedRowTests.cs b/src/ServiceControl.UnitTests/Infrastructure/VersionedRowTests.cs index 0b5d9dbc09..7262271fe9 100644 --- a/src/ServiceControl.UnitTests/Infrastructure/VersionedRowTests.cs +++ b/src/ServiceControl.UnitTests/Infrastructure/VersionedRowTests.cs @@ -6,12 +6,13 @@ namespace ServiceControl.UnitTests.Infrastructure using System.Linq; using System.Reflection; using NUnit.Framework; + using Raven.Client.Documents.Conventions; using ServiceControl.EventLog; using ServiceControl.Infrastructure.WebApi; using ServiceControl.Operations; using ServiceControl.Persistence; using ServiceControl.Persistence.Infrastructure; - using JsonSerializer = System.Text.Json.JsonSerializer; + using Sparrow.Json; [TestFixture] public class VersionedRowTests @@ -30,15 +31,19 @@ public void The_rows_are_found_at_all() "the scan below found nothing, so every other test here would pass without checking anything"); } - // The declaration is implemented explicitly so the member stays off the type's public surface. - // Declared as an ordinary public property it would be serialised alongside the real fields. + // RavenDB serialises explicit interface implementations, under their fully qualified name, where + // System.Text.Json ignores them. Some of these rows are stored as-is, so a computed member lands in the document. [TestCaseSource(nameof(RowTypes))] - public void The_covered_field_list_is_not_part_of_the_response(Type type) + public void Only_the_rows_own_fields_are_stored(Type type) { - var row = Seeded(type); + var conventions = new DocumentConventions(); + conventions.Serialization.Initialize(conventions); + + using var context = JsonOperationContext.ShortTermSingleUse(); + using var stored = conventions.Serialization.DefaultConverter.ToBlittable(Seeded(type), context); - Assert.That(JsonSerializer.Serialize(row, type, SerializerOptions.Default), - Does.Not.Contain("version_fields").IgnoreCase); + Assert.That(stored.GetPropertyNames(), Is.EquivalentTo(Stored(type)), + $"{type.Name} would be written to the database with something other than its own properties, so a value computed from the fields beside it gets stored next to them"); } [TestCaseSource(nameof(RowTypes))] @@ -75,6 +80,10 @@ public void Changing_any_rendered_field_moves_the_version(Type type) static PropertyInfo[] Rendered(Type type) => [.. type.GetProperties(BindingFlags.Public | BindingFlags.Instance).Where(property => property.CanWrite)]; + // Read-only ones count: a serialiser stores them too, and they are legitimate data. + static string[] Stored(Type type) => + [.. type.GetProperties(BindingFlags.Public | BindingFlags.Instance).Select(property => property.Name)]; + // Every field set to something, so a row is never checked with a null standing in for a real value // and a computed property never has to cope with an unset one. static IVersionedRow Seeded(Type type)