From 18201b512ac8ecba81f91ba0148f2a40addcf03e Mon Sep 17 00:00:00 2001 From: konstantin Date: Fri, 20 Feb 2026 09:10:12 +0100 Subject: [PATCH] feat: protect final deserialization in PatchToDate with skip conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: PatchToDate has two steps: (1) ApplyPatchesToDate applies patches one-by-one to a JToken — each protected by skip conditions, and (2) _deserialize() converts the accumulated JToken back to TEntity. Step 2 was NOT protected by skip conditions. Symptom: When the initial entity has a null navigation property (e.g. due to a missing EF Core Include), jsondiffpatch.net's Unpatch silently produces a garbled JToken — no error is thrown during patching. The configured skip conditions (e.g. IgnoreEverythingSkipCondition) are never invoked because no individual patch fails. The unprotected _deserialize() then crashes with a JsonException because the garbled JToken cannot be deserialized back into the target type (e.g. List at a path that became a non-array). This was discovered in production (DEV-107694). Fix: - Wrap _deserialize() and _populateEntity() calls in both PatchToDate overloads with try-catch that consults skip conditions - When skip conditions allow skipping: return initialEntity as-is (the unpatched entity at +/- infinity) and set FinalDeserializationFailed = true - When no skip conditions match: rethrow as before (no behavior change) Consequences for servers using this library: - Without this fix: _deserialize() throws → unhandled exception → HTTP 500. - With this fix: _deserialize() is caught → initialEntity is returned. The server no longer crashes, but the returned entity is the CURRENT state (at +/- infinity), NOT the state at the requested keyDate. This is fachlich incorrect — the caller gets the wrong time slice. - The entity cannot be partially rescued: System.Text.Json deserializes the entire object or not at all, even though only one property path (e.g. konfigurationsprodukte) is garbled while the rest was correctly unpatched. - Servers SHOULD check chain.FinalDeserializationFailed after PatchToDate and log a warning, because the returned data is silently wrong. - The correct fix remains on the consumer side: ensure all navigation properties are properly loaded (e.g. via EF Core Include) so that the Unpatch operates on valid arrays instead of null. New API: - FinalDeserializationFailed (bool): true when the final deserialization was skipped. The returned entity is NOT the state at keyDate but the unpatched initial entity. - PatchesHaveBeenSkipped now also returns true when FinalDeserializationFailed is true. BREAKING CHANGE: ISkipCondition.ShouldSkipPatch parameter failedPatch changed from TimeRangePatch to TimeRangePatch? (nullable). The parameter is null when the error occurred during final deserialization (no single patch failed). If your skip condition only inspects the exception type, just update the method signature. Co-Authored-By: Claude Opus 4.6 --- .../ChronoJsonDiffPatch/SkipCondition.cs | 4 +- .../SkipPatchesWithUnmatchedListItems.cs | 2 +- .../TimeRangePatchChain.cs | 61 +++++++- .../ListPatchingTests.cs | 145 ++++++++++++++++++ 4 files changed, 205 insertions(+), 7 deletions(-) diff --git a/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipCondition.cs b/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipCondition.cs index 105d25e..5989954 100644 --- a/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipCondition.cs +++ b/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipCondition.cs @@ -19,11 +19,11 @@ public interface ISkipCondition /// /// state before applying the patch (but not necessarily before apply any patch in the chain) /// any error that occurred - /// the patch that lead to the exception + /// the patch that lead to the exception ; null if the error occurred during final deserialization after all patches were applied /// true if the patch should be skipped public bool ShouldSkipPatch( TEntity initialEntity, - TimeRangePatch failedPatch, + TimeRangePatch? failedPatch, Exception errorWhilePatching ); } diff --git a/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipPatchesWithUnmatchedListItems.cs b/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipPatchesWithUnmatchedListItems.cs index 7b11c4b..d1ae46f 100644 --- a/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipPatchesWithUnmatchedListItems.cs +++ b/ChronoJsonDiffPatch/ChronoJsonDiffPatch/SkipPatchesWithUnmatchedListItems.cs @@ -23,7 +23,7 @@ public SkipPatchesWithUnmatchedListItems(Func?> listAcc /// public virtual bool ShouldSkipPatch( TEntity initialEntity, - TimeRangePatch failedPatch, + TimeRangePatch? failedPatch, Exception errorWhilePatching ) { diff --git a/ChronoJsonDiffPatch/ChronoJsonDiffPatch/TimeRangePatchChain.cs b/ChronoJsonDiffPatch/ChronoJsonDiffPatch/TimeRangePatchChain.cs index 102e555..97d79dc 100644 --- a/ChronoJsonDiffPatch/ChronoJsonDiffPatch/TimeRangePatchChain.cs +++ b/ChronoJsonDiffPatch/ChronoJsonDiffPatch/TimeRangePatchChain.cs @@ -52,9 +52,22 @@ public IReadOnlyList SkippedPatches /// public bool PatchesHaveBeenSkipped { - get => SkippedPatches?.Any() == true; + get => SkippedPatches?.Any() == true || FinalDeserializationFailed; } + /// + /// Set to true if the final deserialization of the accumulated JToken back to + /// failed after all patches were applied. This means the accumulated patch result was structurally invalid + /// for the target type, and the returned entity is the unpatched initial entity. + /// + /// + /// When this is true, returns initialEntity as-is. + /// In mode, initialEntity is the state at +infinity + /// (the "current" state), NOT the historical state at the requested key date. + /// In mode, initialEntity is the state at -infinity. + /// + public bool FinalDeserializationFailed { get; private set; } + /// /// converts the given to an JToken using the serializer configured in the constructor (or default) /// @@ -556,6 +569,7 @@ private JToken ApplyPatchesToDate(TEntity initialEntity, DateTimeOffset keyDate) var jdp = new JsonDiffPatch(); var left = ToJToken(initialEntity); _skippedPatches = new(); + FinalDeserializationFailed = false; switch (PatchingDirection) { @@ -670,11 +684,30 @@ var existingPatch in GetAll() /// the state of at the beginning of time /// the date up to which you'd like to apply the patches /// the state of the entity after all the patches up to have been applied - [Pure] public TEntity PatchToDate(TEntity initialEntity, DateTimeOffset keyDate) { var left = ApplyPatchesToDate(initialEntity, keyDate); - return _deserialize(JsonConvert.SerializeObject(left)); + try + { + return _deserialize(JsonConvert.SerializeObject(left)); + } + catch (Exception exc) + { + if ( + _skipConditions?.Any(sc => + sc.ShouldSkipPatch(initialEntity, failedPatch: null, exc) + ) == true + ) + { + // The final deserialization failed, but skip conditions say we should tolerate it. + // Return the initial entity as-is because the accumulated patches produced an invalid state. + // Callers should check FinalDeserializationFailed to detect this. + FinalDeserializationFailed = true; + return initialEntity; + } + + throw; + } } /// @@ -697,7 +730,27 @@ public void PatchToDate(TEntity initialEntity, DateTimeOffset keyDate, TEntity t } var left = ApplyPatchesToDate(initialEntity, keyDate); - _populateEntity(JsonConvert.SerializeObject(left), targetEntity); + try + { + _populateEntity(JsonConvert.SerializeObject(left), targetEntity); + } + catch (Exception exc) + { + if ( + _skipConditions?.Any(sc => + sc.ShouldSkipPatch(initialEntity, failedPatch: null, exc) + ) == true + ) + { + // The final population failed, but skip conditions say we should tolerate it. + // Leave targetEntity unchanged because the accumulated patches produced an invalid state. + // Callers should check FinalDeserializationFailed to detect this. + FinalDeserializationFailed = true; + return; + } + + throw; + } } /// diff --git a/ChronoJsonDiffPatch/ChronoJsonDiffPatchTests/ListPatchingTests.cs b/ChronoJsonDiffPatch/ChronoJsonDiffPatchTests/ListPatchingTests.cs index b0257ce..b7c6a1c 100644 --- a/ChronoJsonDiffPatch/ChronoJsonDiffPatchTests/ListPatchingTests.cs +++ b/ChronoJsonDiffPatch/ChronoJsonDiffPatchTests/ListPatchingTests.cs @@ -193,6 +193,151 @@ public void Test_ArgumentOutOfRangeException_Can_Be_Surpressed() antiparallelChain.PatchesHaveBeenSkipped.Should().BeTrue(); } + /// + /// Reproduces a scenario where the final deserialization after patching fails (e.g. because the + /// accumulated JToken has a structurally invalid list), and verifies that WITHOUT skip conditions + /// the exception propagates to the caller. + /// + /// + /// This simulates a production issue (DEV-107694 in TechnicalMasterData) where: + /// - The initial entity had a null list (because of a missing EF Core Include) + /// - Patches that added/modified list items were applied to the null JToken + /// - The accumulated JToken had a structurally invalid list representation + /// - The final System.Text.Json deserialization threw a JsonException + /// + /// The custom deserializer here simulates this by always throwing when deserializing. + /// + [Fact] + public void PatchToDate_Without_SkipConditions_Throws_When_Final_Deserialization_Fails() + { + // Build a valid chain first with the default deserializer + var buildChain = new TimeRangePatchChain(); + var initialEntity = new EntityWithList + { + MyList = new List { new() { Value = "A" } }, + }; + var keyDate = new DateTimeOffset(2024, 1, 1, 0, 0, 0, TimeSpan.Zero); + var updatedEntity = new EntityWithList + { + MyList = new List + { + new() { Value = "A" }, + new() { Value = "B" }, + }, + }; + buildChain.Add(initialEntity, updatedEntity, keyDate); + + // Extract raw patches and create a chain with a broken deserializer + var patches = buildChain.GetAll().ToList(); + var chainWithBrokenDeserializer = new TimeRangePatchChain( + patches, + deserializer: _ => + throw new System.Text.Json.JsonException( + "The JSON value could not be converted to List`1" + ) + ); + + var act = () => + chainWithBrokenDeserializer.PatchToDate(initialEntity, keyDate + TimeSpan.FromDays(1)); + + act.Should().ThrowExactly(); + chainWithBrokenDeserializer.FinalDeserializationFailed.Should().BeFalse(); + } + + /// + /// Verifies that when the final deserialization fails AND skip conditions are configured, + /// the error is caught and the initial entity is returned. + /// Also verifies that is set. + /// + [Fact] + public void PatchToDate_With_SkipConditions_Returns_InitialEntity_When_Final_Deserialization_Fails() + { + // Build a valid chain first with the default deserializer + var buildChain = new TimeRangePatchChain(); + var initialEntity = new EntityWithList + { + MyList = new List { new() { Value = "A" } }, + }; + var keyDate = new DateTimeOffset(2024, 1, 1, 0, 0, 0, TimeSpan.Zero); + var updatedEntity = new EntityWithList + { + MyList = new List + { + new() { Value = "A" }, + new() { Value = "B" }, + }, + }; + buildChain.Add(initialEntity, updatedEntity, keyDate); + + // Extract raw patches and create a chain with a broken deserializer + skip condition + var patches = buildChain.GetAll().ToList(); + var chainWithBrokenDeserializer = new TimeRangePatchChain( + patches, + deserializer: _ => + throw new System.Text.Json.JsonException( + "The JSON value could not be converted to List`1" + ), + skipConditions: new List> + { + new IgnoreAllSkipCondition(), + } + ); + + var result = chainWithBrokenDeserializer.PatchToDate( + initialEntity, + keyDate + TimeSpan.FromDays(1) + ); + + result.Should().BeSameAs(initialEntity); + chainWithBrokenDeserializer.PatchesHaveBeenSkipped.Should().BeTrue(); + chainWithBrokenDeserializer.FinalDeserializationFailed.Should().BeTrue(); + } + + /// + /// Verifies that is reset + /// between calls to PatchToDate. + /// + [Fact] + public void FinalDeserializationFailed_Is_Reset_Between_Calls() + { + var chain = new TimeRangePatchChain(); + var initialEntity = new EntityWithList + { + MyList = new List { new() { Value = "A" } }, + }; + var keyDate = new DateTimeOffset(2024, 1, 1, 0, 0, 0, TimeSpan.Zero); + chain.Add( + initialEntity, + new EntityWithList + { + MyList = new List + { + new() { Value = "A" }, + new() { Value = "B" }, + }, + }, + keyDate + ); + + // Normal patching should NOT set FinalDeserializationFailed + var result = chain.PatchToDate(initialEntity, keyDate + TimeSpan.FromDays(1)); + chain.FinalDeserializationFailed.Should().BeFalse(); + result.MyList.Should().HaveCount(2); + } + + /// + /// A skip condition that always returns true for any error. + /// This simulates IgnoreEverythingSkipCondition from downstream consumers. + /// + private class IgnoreAllSkipCondition : ISkipCondition + { + public bool ShouldSkipPatch( + EntityWithList initialEntity, + TimeRangePatch? failedPatch, + Exception errorWhilePatching + ) => true; + } + private static void ReverseAndRevert( TimeRangePatchChain chain, EntityWithList initialEntity