From e0518b8c5939c63c582644dd69e5c5ce1b9f8565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Wed, 2 Sep 2026 11:48:51 +0200 Subject: [PATCH] fix: an update validator sees the existing node typed like the proposed one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plugins' UpdateNode_VersionDowngrade_ShouldFail went red against core main at 3d6faa284 (#3056): the validator saw ExistingNode.Content as a JsonElement while Node.Content was the caller's CLR instance, skipped its typed comparison and answered Valid — a refused write landed, unlogged. Until #3056, MessageService.Post rendered every delivery through the logging options eagerly and ObjectPolymorphicConverter.Write registered each value's type on the hub as a side effect, so a hub that had merely handed a typed instance to CreateNode could re-type the node when the update pipeline read it back. #3056 removed that render (the OOM) and with it the accidental registration. NodeUpdatePipeline now types the existing content like the proposal — node.Content.GetType() is the type authority for this update, and a concrete-type deserialisation needs no registry entry — through a new runtime-Type overload ObjectAsExtensions.As(object, Type, options, …). A snapshot that will not deserialise as the proposal's type is left as it was and logged at Error; no registry is mutated on the read path. UpdateValidatorSeesTypedExistingContentTest pins the contract in core (content type registered on no hub; fails before: seen == JsonElement). Doc: Architecture/UpdateValidatorsSeeTypedContent. What's New: Fix. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01G4xPnGYEbdu8AVvR5jytyj --- .../UpdateValidatorsSeeTypedContent.md | 66 +++++++ ...9-02-a-rejected-update-is-refused-again.md | 21 +++ src/MeshWeaver.Hosting/NodeUpdatePipeline.cs | 40 +++- .../ObjectAsExtensions.cs | 71 +++++++ ...teValidatorSeesTypedExistingContentTest.cs | 177 ++++++++++++++++++ 5 files changed, 374 insertions(+), 1 deletion(-) create mode 100644 src/MeshWeaver.Documentation/Data/Architecture/UpdateValidatorsSeeTypedContent.md create mode 100644 src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-rejected-update-is-refused-again.md create mode 100644 test/MeshWeaver.Graph.Test/UpdateValidatorSeesTypedExistingContentTest.cs diff --git a/src/MeshWeaver.Documentation/Data/Architecture/UpdateValidatorsSeeTypedContent.md b/src/MeshWeaver.Documentation/Data/Architecture/UpdateValidatorsSeeTypedContent.md new file mode 100644 index 0000000000..5b6ae17073 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/Architecture/UpdateValidatorsSeeTypedContent.md @@ -0,0 +1,66 @@ +--- +Name: Update Validators See Typed Content +Category: Architecture +Description: An update validator compares the existing node with the proposed one, so the pipeline owes it both sides typed alike. How a hub used to learn content types by accident, how #3056 removed that accident, and why the update pipeline now types the existing snapshot off the proposal's own CLR type. +Icon: +--- + +# Update Validators See Typed Content + +**An `INodeValidator` for `Update` compares `context.ExistingNode` with `context.Node`. The +pipeline that calls it — `NodeUpdatePipeline` behind `IMeshService.UpdateNode` — owes it BOTH +sides with content of the same CLR type.** A validator that finds a `JsonElement` on one side and a +typed record on the other skips its own comparison and answers Valid, and a write that should have +been refused lands with nothing logged as an error. This page records how that happened once, and +where the guarantee now lives. + +## How a hub learned content types by accident + +Every hub carries a `TypeRegistry` behind its `JsonSerializerOptions`; `$type` discriminators are +resolved against it. The documented way to teach a hub a content type is `WithType(typeof(T), …)`. +There was, until 2026-09-02, a second, undocumented way: `MessageService.Post` rendered every +delivery through the logging options — eagerly, as a method argument, whether or not Debug logging +was on — and `ObjectPolymorphicConverter.Write` calls `typeRegistry.GetOrAddType(valueType)` for +every value it serialises. So a hub that merely handed a typed instance to `CreateNode` had, by the +time the request left, registered that instance's type in its own registry. Reading the node back +on the same hub then re-typed it. Nothing declared that dependency; everything that did not call +`WithType` relied on it. + +Core #3056 removed the eager render (it was the allocation that threw `OutOfMemoryException` on a +production pod), and with it the accidental registration. The very next Plugins run against core +`main` failed `NodeOperationsWithUpdateValidatorTest.UpdateNode_VersionDowngrade_ShouldFail`: the +test hub never registered `UpdatableContent`, `MeshNodeStreamCache.GetStream` logged +`Content for … stayed an untyped JsonElement after deserialization`, the validator's +`ExistingNode.Content is UpdatableContent` was false, and the downgrade went through. The bisect is +exact — `804b9beca` (#3059) passes, `3d6faa284` (#3056) fails — and the warning line is the +mechanism, not a guess. + +## Where the guarantee lives now + +`NodeUpdatePipeline` reads the existing node through the stream cache (typed with the running hub's +options) and, before building the `NodeValidationContext`, types its content **like the proposal**: +the proposed node carries a live CLR instance of exactly the type the existing content must have +(same node, same NodeType), and the hub running the validators demonstrably holds that type. The +degraded snapshot is deserialised as `node.Content.GetType()` through the runtime-`Type` overload +of `As` (`ObjectAsExtensions.As(object, Type, options, …)`) — a concrete-type deserialisation, which +needs no registry entry for the discriminator. It is the same recovery `ContentAs` performs at a +consumer, done once for every validator instead of being each one's problem. + +A snapshot that will not deserialise as the proposal's type is left as it was and logged at Error by +`As`: hiding a shape mismatch from the validators would be a second silent pass. + +`UpdateValidatorSeesTypedExistingContentTest` (MeshWeaver.Graph.Test) pins the contract with a +content type registered on no hub, records the CLR type the validator actually saw, and asserts the +refusal. It exists in core because the test that found the hole runs only in MeshWeaver.Plugins — +the cross-repo blind spot that let #3056 merge green. + +## What this does not change + +- A validator should still read payloads through `.As()` / `.ContentAs()` rather than a cast + (see [CQRS and content access](../CqrsAndContentAccess)); the pipeline guarantee makes the fleet's + existing validators safe, it does not make the cast idiom right. +- A test fixture that posts a custom content type should still register it with `WithType` on the + hub that reads it back. The Plugins fixture above does not, and should. +- No registry is mutated on the read path. `As` deliberately deserialises to the concrete type + without adopting it, so a same-named type from another collectible assembly cannot poison the + running hub's `$type` resolution. diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-rejected-update-is-refused-again.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-rejected-update-is-refused-again.md new file mode 100644 index 0000000000..ac5ed2e389 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-rejected-update-is-refused-again.md @@ -0,0 +1,21 @@ +--- +Name: A rejected update is refused again +Category: Fix +Description: An update that a validation rule should have refused — a version going backwards, for instance — could briefly go through with no error anywhere, because the rule was shown the current content in an unreadable form and stepped aside. Rules now see the current content the same way they see the proposed one. +Icon: ShieldCheckmark +Order: -20260902 +--- + +# A rejected update is refused again + +Some node types carry rules about how they may change: a version may not go backwards, a name may +not be emptied, a reference may not point at nothing. Those rules compare what a node currently +holds with what an update proposes, and refuse when the two do not fit together. + +For a short window on 2 September 2026 a rule could be shown the node's current content in a raw, +unreadable form while the proposed content arrived readable. A rule that compares the two found +nothing to compare, stepped aside, and the update landed — no error, no warning where a person +would see it. The rule was fine; what it was handed was not. + +Updates now always present the current content to the rules in the same readable form as the +proposed content, so a refusal is a refusal again. Nothing changes for updates the rules accept. diff --git a/src/MeshWeaver.Hosting/NodeUpdatePipeline.cs b/src/MeshWeaver.Hosting/NodeUpdatePipeline.cs index 48da58648b..4090962d04 100644 --- a/src/MeshWeaver.Hosting/NodeUpdatePipeline.cs +++ b/src/MeshWeaver.Hosting/NodeUpdatePipeline.cs @@ -2,11 +2,14 @@ using System.Linq; using System.Reactive.Disposables; using System.Reactive.Linq; +using System.Text.Json; +using System.Text.Json.Nodes; using MeshWeaver.Mesh; using MeshWeaver.Mesh.Security; using MeshWeaver.Mesh.Services; using MeshWeaver.Messaging; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; namespace MeshWeaver.Hosting; @@ -119,6 +122,41 @@ private static IObservable DoUpdate( return candidate with { LastModified = DateTimeOffset.UtcNow }; })); + // 🚨 A VALIDATOR COMPARES THE EXISTING NODE WITH THE PROPOSED ONE, SO BOTH MUST BE TYPED + // ALIKE — and the pipeline, not the validator, owes it that (#3056 fallout, found by the + // Plugins UpdateNode_VersionDowngrade_ShouldFail test on 2026-09-02). + // + // `existing` arrives through MeshNodeStreamCache.GetStream, which re-types the cache's raw + // JSON with THIS hub's registry — and a content type this hub never registered stays a + // JsonElement there. Until #3056, every hub registered every type it ever posted as a SIDE + // EFFECT: MessageService.Post rendered each delivery through the logging options eagerly + // (the argument was built before the LogDebug call), and ObjectPolymorphicConverter.Write + // calls typeRegistry.GetOrAddType for each value it meets. A hub that had merely handed a + // typed instance to CreateNode therefore "knew" the type when it read the node back. #3056 + // removed that render (it was the OOM), and with it the accidental registration — so a + // validator written as `context.ExistingNode.Content is Versioned e && context.Node.Content + // is Versioned p && p.Version < e.Version` now sees a JsonElement on the left, skips its + // own `if`, and answers Valid. A refused write went through with nothing logged as an error. + // + // The proposed node carries a LIVE CLR instance of exactly the type the existing content + // must have (same node, same NodeType), and this hub demonstrably holds that type: it is + // the type authority for this update. Deserialise the degraded snapshot AS that type, here, + // on the hub that runs the validators — the same recovery ContentAs performs at a + // consumer, done once for every validator instead of being each one's problem. A snapshot + // that will not deserialise as the proposal's type is left as it was (logged loud by As): + // hiding a shape mismatch from the validators would be a second silent pass. + private static MeshNode WithContentTypedLikeTheProposal(IMessageHub hub, MeshNode node, MeshNode existing) + { + var proposed = node.Content; + if (proposed is null or JsonElement or JsonNode || existing.Content is null) + return existing; + var logger = hub.ServiceProvider.GetService()?.CreateLogger(typeof(NodeUpdatePipeline)); + var typed = existing.Content.As(proposed.GetType(), hub.JsonSerializerOptions, logger, existing.Path); + return typed is null || ReferenceEquals(typed, existing.Content) + ? existing + : existing with { Content = typed }; + } + // 2. Run client-side Update validators sequentially (Concat preserves short-circuit: // the chain stops at the first failure). Returns the mapped exception or null. private static IObservable RunUpdateValidators( @@ -136,7 +174,7 @@ private static IObservable DoUpdate( { Operation = NodeOperation.Update, Node = node, - ExistingNode = existing, + ExistingNode = WithContentTypedLikeTheProposal(hub, node, existing), AccessContext = ctx, }; diff --git a/src/MeshWeaver.Mesh.Contract/ObjectAsExtensions.cs b/src/MeshWeaver.Mesh.Contract/ObjectAsExtensions.cs index 9321eb6826..ef98726434 100644 --- a/src/MeshWeaver.Mesh.Contract/ObjectAsExtensions.cs +++ b/src/MeshWeaver.Mesh.Contract/ObjectAsExtensions.cs @@ -163,6 +163,77 @@ public static class ObjectAsExtensions } } + /// + /// The runtime- form of , for the seams that learn the + /// target type from a LIVE instance rather than from a type parameter — the update pipeline + /// handing validators an existing node typed like the proposed one is the canonical case. + /// Same contract: already an instance of → as-is; a degraded + /// / → deserialized as + /// (a concrete-type deserialization needs no registry entry for the $type discriminator); + /// a same-named foreign CLR type → recovered by a JSON round-trip; anything else → null, + /// logged loud. Callers keep their original value when this answers null. + /// + public static object? As( + this object? value, + Type type, + JsonSerializerOptions options, + ILogger? logger = null, + string? what = null) + { + ArgumentNullException.ThrowIfNull(type); + switch (value) + { + case null: + return null; + case var typed when type.IsInstanceOfType(typed): + return typed; + case JsonElement je: + try + { + return je.Deserialize(type, options); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException) + { + logger?.LogError(ex, "As<{TargetType}> could not recover {What}: {RawJson}", + type.Name, what ?? "value", Excerpt(je.GetRawText())); + return null; + } + case JsonNode jn: + try + { + return jn.Deserialize(type, options); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException) + { + logger?.LogError(ex, "As<{TargetType}> could not recover {What}: {RawJson}", + type.Name, what ?? "value", Excerpt(jn.ToJsonString())); + return null; + } + default: + if (value.GetType().Name == type.Name) + { + try + { + // Same reasoning as the generic overload: serialize AS the concrete runtime + // type so the read does not adopt a foreign type into the caller's registry. + var element = JsonSerializer.SerializeToElement(value, value.GetType(), options); + var recovered = element.Deserialize(type, options); + if (recovered is not null) + return recovered; + } + catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException) + { + // fall through to the loud log below + } + } + logger?.LogError( + "As<{TargetType}> for {What}: value is {ActualType} ({ActualAssembly}), not convertible", + type.Name, what ?? "value", value.GetType().FullName, + value.GetType().Assembly.GetName().Name); + return null; + } + } + /// /// The head of a payload, for a diagnostic log line. Bounded because the value being logged is /// by definition the one we could NOT parse — a degraded content blob or a whole snapshot can be diff --git a/test/MeshWeaver.Graph.Test/UpdateValidatorSeesTypedExistingContentTest.cs b/test/MeshWeaver.Graph.Test/UpdateValidatorSeesTypedExistingContentTest.cs new file mode 100644 index 0000000000..5fb62d4a97 --- /dev/null +++ b/test/MeshWeaver.Graph.Test/UpdateValidatorSeesTypedExistingContentTest.cs @@ -0,0 +1,177 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reactive.Linq; +using System.Reactive.Subjects; +using System.Threading.Tasks; +using MeshWeaver.Data; +using MeshWeaver.Fixture; +using MeshWeaver.Graph.Configuration; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Graph.Test; + +/// +/// 🚨 AN UPDATE VALIDATOR SEES THE EXISTING NODE TYPED LIKE THE PROPOSED ONE — the +/// NodeUpdatePipeline contract that core #3056 broke and MeshWeaver.Plugins' +/// UpdateNode_VersionDowngrade_ShouldFail caught on 2026-09-02. +/// +/// The content type here is deliberately registered on NO hub (no WithType). Before +/// #3056 that did not matter: every hub registered every type it posted as a side effect of +/// MessageService.Post rendering each delivery through the logging options, so the hub that +/// created the node could re-type it when the update pipeline read it back. #3056 removed that +/// render — correctly, it was an OOM — and with it the accidental registration, so +/// NodeValidationContext.ExistingNode.Content reached validators as a JsonElement +/// while Node.Content was the caller's CLR instance. A validator comparing the two skipped +/// its own check and answered Valid: a refused write went through, with nothing logged as an +/// error. +/// +/// The validator below is written the way validators across the fleet are written — a typed +/// comparison of both sides. That shape is the SUBJECT here, not a lapse: the pipeline owes a +/// validator content typed alike on both sides, and this test pins that it delivers it. The +/// observed CLR type is recorded separately so a regression names the shape it saw. +/// +public class UpdateValidatorSeesTypedExistingContentTest(ITestOutputHelper output) : MonolithMeshTestBase(output) +{ + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + { + // 🚨 No WithType(typeof(VersionedContent), …) anywhere: the pipeline must not depend on it. + builder.ConfigureServices(services => services + .AddSingleton() + .AddSingleton()); + return base.ConfigureMesh(builder); + } + + private IMeshService MeshService => Mesh.ServiceProvider.GetRequiredService(); + + private NoVersionDowngradeValidator Validator => + Mesh.ServiceProvider.GetServices().OfType().Single(); + + private static string NewId() => "vc" + Guid.NewGuid().ToString("N")[..8]; + + private static MeshNode Versioned(string id, string title, int version) => new(id, TestPartition) + { + Name = title, + NodeType = VersionedTypeProvider.TypeName, + State = MeshNodeState.Active, + Content = new VersionedContent(title, version), + }; + + [Fact(Timeout = 180000)] + public async Task UpdateNode_Downgrade_IsRefused_BecauseTheValidatorSawTypedExistingContent() + { + var id = NewId(); + var path = $"{TestPartition}/{id}"; + await MeshService.CreateNode(Versioned(id, "High", 5)).Take(1) + .Should().Within(60.Seconds()).Emit("the node to downgrade must exist first"); + + var failure = await Record.ExceptionAsync(() => + MeshService.UpdateNode(Versioned(id, "Downgraded", 3)) + .Take(1).Timeout(60.Seconds()).Await()); + + var seen = await Validator.ObservedExistingContentType.FirstAsync().Timeout(60.Seconds()).Await(); + seen.Should().Be(typeof(VersionedContent), + "the pipeline must hand validators the existing node with content typed like the " + + "proposed one — a JsonElement here is the silent pass #3056 opened"); + + failure.Should().BeOfType( + "a validator's ValidationFailed maps to UnauthorizedAccessException on the IMeshService surface"); + failure!.Message.Should().Contain("downgrade"); + + var after = await Mesh.GetWorkspace().GetMeshNodeStream(path) + .Where(n => n is not null).FirstAsync().Timeout(60.Seconds()).Await(); + after.ContentAs(Mesh.JsonSerializerOptions)!.Version.Should().Be(5, + "a refused update must leave the node as it was"); + } + + [Fact(Timeout = 180000)] + public async Task UpdateNode_Upgrade_StillLands() + { + var id = NewId(); + var path = $"{TestPartition}/{id}"; + await MeshService.CreateNode(Versioned(id, "Low", 1)).Take(1) + .Should().Within(60.Seconds()).Emit("the node to upgrade must exist first"); + + await MeshService.UpdateNode(Versioned(id, "Upgraded", 2)).Take(1) + .Should().Within(60.Seconds()).Emit("an upgrade passes the validator and lands"); + + var seen = await Validator.ObservedExistingContentType.FirstAsync().Timeout(60.Seconds()).Await(); + seen.Should().Be(typeof(VersionedContent)); + + var after = await Mesh.GetWorkspace().GetMeshNodeStream(path) + .Where(n => n is not null && n.ContentAs(Mesh.JsonSerializerOptions)?.Version == 2) + .FirstAsync().Timeout(60.Seconds()).Await(); + after.Name.Should().Be("Upgraded"); + } +} + +/// Content type for the typed-existing-node contract. Registered on NO hub on purpose. +public record VersionedContent(string Title, int Version); + +/// +/// Refuses a version downgrade. Written as validators are written across the fleet — a typed +/// comparison of the existing and the proposed content — because that shape is what the pipeline +/// owes it; see the test class remarks. +/// +public sealed class NoVersionDowngradeValidator : INodeValidator +{ + private readonly ReplaySubject observed = new(); + + /// The CLR type of ExistingNode.Content as this validator saw it, per call. + public IObservable ObservedExistingContentType => observed; + + public IReadOnlyCollection SupportedOperations => [NodeOperation.Update]; + + public IObservable Validate(NodeValidationContext context) + { + if (context.ExistingNode is null || context.ExistingNode.NodeType != VersionedTypeProvider.TypeName) + return Observable.Return(NodeValidationResult.Valid()); + + observed.OnNext(context.ExistingNode.Content?.GetType()); + + if (context.ExistingNode.Content is VersionedContent existing + && context.Node.Content is VersionedContent proposed + && proposed.Version < existing.Version) + return Observable.Return(new NodeValidationResult( + false, + $"Cannot downgrade version from {existing.Version} to {proposed.Version}", + NodeRejectionReason.ValidationFailed)); + + return Observable.Return(NodeValidationResult.Valid()); + } +} + +/// The static NodeType the versioned nodes are instances of, plus its static partition. +internal sealed class VersionedTypeProvider : IStaticNodeProvider +{ + public const string TypeName = "VersionedProbe"; + + public IEnumerable GetStaticNodes() + { + yield return new MeshNode(TypeName) + { + Name = "Versioned probe", + NodeType = "NodeType", + HubConfiguration = c => c.AddMeshDataSource(), + Content = new NodeTypeDefinition { Description = "Test NodeType for the typed-existing-node contract." }, + }; + yield return new MeshNode(TypeName, "Admin/Partition") + { + NodeType = "Partition", + Name = $"{TypeName} (static)", + State = MeshNodeState.Active, + Content = new PartitionDefinition + { + Namespace = TypeName, + DataSource = "static", + Description = $"Test NodeType definition partition for '{TypeName}'", + }, + }; + } +}