Skip to content

Commit 8d38c9d

Browse files
mwadamsclaude
andcommitted
Fix #820: refresh stale parent handle during nested merge patch
RFC 7396 merge-patch application recursed into a nested target object via a held `JsonElement.Mutable`, but the recursion mutated the document (bumping its version) without refreshing that parent handle. Processing any further property of the same non-root parent then failed its staleness check — surfacing as `InvalidOperationException` during the merge, or `ObjectDisposedException: 'JsonDocument'` when a frozen patch copied into the merge was read afterwards. The root element is exempt from the staleness check, so only nested ("real") merges with an object-valued property followed by another property were affected; the existing RFC 7396 suite (top-level / single-property-per-level only) missed it. A recursive merge mutates only the child's own subtree, so the parent's start index is unchanged and only its cached version is stale. `ApplyMergePatch` now re-mints the parent handle after each nested merge via a new `IMutableJsonDocument.RefreshElementUnsafe<TElement>(int)` seam, surfaced as a public, deliberately unsafe high-performance helper `JsonMarshal.RefreshUnsafe<T>(in T)`. No `InternalsVisibleTo` is used (it would leak the internal netstandard `ObsoleteAttribute` polyfill into the Patch package); the merge reaches the seam alloc-free through constrained generics. Adds regression tests (nested object-then-sibling merges, frozen-mutable patch readability, an RFC 7396 correctness fuzz, and direct `JsonMarshal.RefreshUnsafe` tests), documents the helper under the version-tracking section of JsonDocumentBuilder.md, and adds the V5.1.17 version history entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 215c480 commit 8d38c9d

12 files changed

Lines changed: 593 additions & 5 deletions

File tree

VERSIONHISTORY.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,17 @@
11
# Version History
22

3+
## V5.1.17
4+
5+
V5.1.17 fixes an RFC 7396 JSON Merge Patch bug where merging a nested object corrupted the parent element handle.
6+
7+
### Bug fixes
8+
9+
- **`ApplyMergePatch` no longer leaves the parent element stale when merging a nested object** — When a merge patch recursed into a nested object (`JsonMergePatchExtensions.ApplyMergePatch`), the recursion mutated the document but the parent `JsonElement.Mutable` it was iterating kept its now-stale cached document version. Processing any further property of that same (non-root) parent then failed its staleness check, and a frozen patch document copied into the merge could appear disposed on a subsequent read — surfacing as `InvalidOperationException` ("Operation is not valid due to the current state of the object") during the merge or `ObjectDisposedException: 'JsonDocument'` when the patch was read afterwards. Because a recursive merge only mutates the child's own subtree, the parent element's start index is still valid; the merge now re-mints the parent handle with the current version after each nested merge so subsequent siblings and reads succeed. Simple single-property-per-level merges were unaffected, which is why the existing RFC 7396 suite did not catch it. See [#820](https://github.com/corvus-dotnet/Corvus.JsonSchema/issues/820).
10+
11+
### New features
12+
13+
- **`JsonMarshal.RefreshUnsafe<T>(in T)`** — Returns a fresh handle to a mutable JSON element whose cached document version is brought up to date with its parent document's current version, without re-validating the element's position. This is a deliberately **dangerous** marshalling helper: it must only be called when the caller knows the element's start index is still valid — i.e. the document has been mutated only *within that element's own subtree* (descendant nodes). It backs the RFC 7396 merge-patch fix above, where a recursive merge into a child object leaves the (structurally still valid) parent element version-stale. See [#820](https://github.com/corvus-dotnet/Corvus.JsonSchema/issues/820).
14+
315
## V5.1.16
416

517
V5.1.16 fixes schema reference resolution for `$ref`s expressed as `file://` URIs.

docs/JsonDocumentBuilder.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1366,6 +1366,35 @@ Version tracking is a **safety feature** that prevents bugs caused by:
13661366

13671367
Without version tracking, you could silently access incorrect data or crash with memory corruption. The `InvalidOperationException` is intentional and helps you write correct code. The root element exemption is safe because the root is always at index 0 and is never relocated.
13681368

1369+
### Refreshing a stale reference without re-navigating (`JsonMarshal.RefreshUnsafe`) — advanced
1370+
1371+
> ⚠️ **`JsonMarshal.RefreshUnsafe<T>` is a deliberately unsafe, high-performance escape hatch.** It is **only** for use where you *know* your changes cannot have altered the element's index in its parent document. Reach for it only in allocation- and lookup-sensitive code where you can *prove* the rule below holds. The safe, idiomatic option is always to re-navigate from the root (see [Best Practices](#best-practices-for-version-tracking)).
1372+
1373+
There is one common case where re-navigating from the root is wasteful: you hold a cached *intermediate* element and then mutate **only inside that element's own subtree** (a descendant property or item). The mutation bumps the document version, so your cached handle is now version-stale — but the element itself has **not** moved: every descendant lives at a higher index than the element's own start row, so that start row is unchanged. Re-navigating from the root to find it again would be pure overhead.
1374+
1375+
`Corvus.Runtime.InteropServices.JsonMarshal.RefreshUnsafe<T>(in T element)` takes such an element and returns a fresh handle to the *same* element, stamped with the document's current version — no lookup, no allocation:
1376+
1377+
```csharp
1378+
using Corvus.Runtime.InteropServices;
1379+
1380+
JsonElement.Mutable root = doc.RootElement;
1381+
JsonElement.Mutable order = root.GetProperty("order"u8);
1382+
1383+
// Mutate only WITHIN 'order' (a descendant). This bumps the version, so 'order' is now stale,
1384+
// but 'order' itself has not moved — only its subtree grew.
1385+
order.GetProperty("lines"u8).SetProperty("count"u8, 3);
1386+
1387+
// Refresh the handle in place instead of re-navigating from the root.
1388+
order = JsonMarshal.RefreshUnsafe(in order);
1389+
1390+
// 'order' is usable again.
1391+
order.SetProperty("status"u8, "packed"u8);
1392+
```
1393+
1394+
This is exactly how RFC 7396 merge-patch application (`JsonMergePatchExtensions.ApplyMergePatch`) keeps the parent object live while recursively merging into its children.
1395+
1396+
**The contract you must guarantee.** Use `RefreshUnsafe` **only where you know your changes cannot have modified the element's index in the parent document** — i.e. the document has been mutated **solely within the refreshed element's own subtree**, never at or before the element's position (the node itself replaced, an ancestor, or a *preceding sibling* changing size). If anything at or before the element moved, its start index is no longer valid and `RefreshUnsafe` returns a handle that **silently addresses the wrong node** — the very memory-corruption class that version tracking exists to prevent. `RefreshUnsafe` only checks for document disposal, never the correctness of the position, so it cannot catch this for you. When in doubt, re-navigate from the (always-live) root instead.
1397+
13691398
## Comparison with System.Text.Json.Nodes
13701399

13711400
### Similar Capabilities

docs/code-sample-catalog.yaml

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -967,11 +967,12 @@ main-docs:
967967
- {index: 38, language: csharp, lines: [1331, 1337], category: compilable, verified: false}
968968
- {index: 39, language: csharp, lines: [1340, 1348], category: compilable, verified: false}
969969
- {index: 40, language: csharp, lines: [1351, 1358], category: compilable, verified: false}
970-
- {index: 41, language: csharp, lines: [1401, 1407], category: compilable, verified: false}
971-
- {index: 42, language: csharp, lines: [1410, 1417], category: compilable, verified: false}
972-
- {index: 43, language: csharp, lines: [1441, 1445], category: compilable, verified: false}
973-
- {index: 44, language: csharp, lines: [1448, 1452], category: compilable, verified: false}
974-
- {index: 45, language: csharp, lines: [1455, 1459], category: compilable, verified: false}
970+
- {index: 41, language: csharp, lines: [1377, 1392], category: compilable, verified: false}
971+
- {index: 42, language: csharp, lines: [1430, 1436], category: compilable, verified: false}
972+
- {index: 43, language: csharp, lines: [1439, 1446], category: compilable, verified: false}
973+
- {index: 44, language: csharp, lines: [1470, 1474], category: compilable, verified: false}
974+
- {index: 45, language: csharp, lines: [1477, 1481], category: compilable, verified: false}
975+
- {index: 46, language: csharp, lines: [1484, 1488], category: compilable, verified: false}
975976
- path: "docs/JsonLogic.md"
976977
blocks:
977978
- {index: 0, language: bash, lines: [29, 32]}

src/Corvus.Text.Json.Patch/JsonMergePatchExtensions.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Copyright (c) Endjin Limited. All rights reserved.
33
// </copyright>
44

5+
using Corvus.Runtime.InteropServices;
56
using Corvus.Text.Json.Internal;
67

78
namespace Corvus.Text.Json.Patch;
@@ -65,6 +66,12 @@ public static void ApplyMergePatch(ref JsonElement.Mutable target, in JsonElemen
6566
}
6667

6768
ApplyMergePatch(ref child, in patchValue);
69+
70+
// The recursive merge mutated only `child`'s subtree, so `target`'s start index is
71+
// still valid, but its cached document version is now stale. Mint a fresh handle for
72+
// the same element so the next property's staleness check — and any read of `target`
73+
// after this method returns — succeeds.
74+
target = JsonMarshal.RefreshUnsafe(in target);
6875
}
6976
else
7077
{

src/Corvus.Text.Json/Corvus/Runtime/InteropServices/JsonMarshal.cs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,4 +87,45 @@ public static RawUtf8JsonString GetRawUtf8Value<T>(T element)
8787
element.CheckValidInstance();
8888
return element.ParentDocument.GetRawValue(element.ParentDocumentIndex, includeQuotes: true);
8989
}
90+
91+
/// <summary>
92+
/// Returns a fresh handle to a mutable JSON element whose cached document version is brought up to
93+
/// date with the parent document's current version, <strong>without</strong> re-validating the
94+
/// element's position.
95+
/// </summary>
96+
/// <typeparam name="T">The type of the mutable <see cref="IJsonElement"/>.</typeparam>
97+
/// <param name="element">The mutable element to refresh.</param>
98+
/// <returns>A new <typeparamref name="T"/> for the same element, stamped with the parent document's
99+
/// current version.</returns>
100+
/// <exception cref="ObjectDisposedException">The underlying <see cref="JsonDocument"/> has been disposed.</exception>
101+
/// <remarks>
102+
/// <para>
103+
/// This is a <strong>dangerous</strong>, high-performance helper. It is <strong>only</strong> safe
104+
/// to call when you know that your mutations cannot have changed <paramref name="element"/>'s index
105+
/// in its parent document. Unlike the other accessors here it deliberately does <strong>not</strong>
106+
/// validate the element first — refreshing a version-stale handle is the whole point — so the
107+
/// position is taken on trust.
108+
/// </para>
109+
/// <para>
110+
/// That guarantee holds <strong>only</strong> when the document has been mutated entirely
111+
/// <em>within this element's own subtree</em> (its descendant nodes) — never the element itself
112+
/// (e.g. replaced), an ancestor, or a <em>preceding sibling</em> (whose size change would shift this
113+
/// element). A descendant mutation only grows or shrinks the document at indices <em>after</em> the
114+
/// element's start row, so that start row — and hence the element's index — is unchanged, and only
115+
/// the cached version needs bringing up to date so subsequent staleness checks pass. Call it after
116+
/// any other kind of mutation and it returns a handle that <strong>silently addresses the wrong
117+
/// node</strong> — the memory-corruption class that version tracking exists to prevent. If you
118+
/// cannot prove the index is unchanged, re-navigate from the (always-live) root element instead.
119+
/// </para>
120+
/// <para>
121+
/// The canonical use is RFC 7396 merge-patch application, which re-mints the parent element after
122+
/// recursively merging into one of its child objects.
123+
/// </para>
124+
/// </remarks>
125+
[CLSCompliant(false)]
126+
public static T RefreshUnsafe<T>(in T element)
127+
where T : struct, IMutableJsonElement<T>
128+
{
129+
return ((IMutableJsonDocument)element.ParentDocument).RefreshElementUnsafe<T>(element.ParentDocumentIndex);
130+
}
90131
}

src/Corvus.Text.Json/Corvus/Text/Json/DocumentBuilder/Internal/DefaultValueJsonDocument.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,15 @@ public TElement FreezeElement<TElement>(int index)
8484
=> JsonElementHelpers.CreateInstance<TElement>(this.inner, index);
8585
#endif
8686

87+
/// <inheritdoc />
88+
public TElement RefreshElementUnsafe<TElement>(int index)
89+
where TElement : struct, IJsonElement<TElement>
90+
#if NET
91+
=> TElement.CreateInstance(this, index);
92+
#else
93+
=> JsonElementHelpers.CreateInstance<TElement>(this, index);
94+
#endif
95+
8796
/// <inheritdoc />
8897
public JsonElement.Mutable GetArrayIndexElement(int currentIndex, int arrayIndex)
8998
{

src/Corvus.Text.Json/Corvus/Text/Json/DocumentBuilder/Internal/IMutableJsonDocument.cs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,27 @@ public interface IMutableJsonDocument : IWorkspaceManagedDocument
3333
TElement FreezeElement<TElement>(int index)
3434
where TElement : struct, IJsonElement<TElement>;
3535

36+
/// <summary>
37+
/// Creates a fresh element of type <typeparamref name="TElement"/> for the element at the
38+
/// specified index, capturing the document's current version.
39+
/// </summary>
40+
/// <typeparam name="TElement">The element type to return.</typeparam>
41+
/// <param name="index">The metadata index of the element.</param>
42+
/// <returns>A new element pointing at <paramref name="index"/>, stamped with the document's
43+
/// current <see cref="Version"/>.</returns>
44+
/// <remarks>
45+
/// <para>
46+
/// This is an <strong>unsafe</strong> operation: it does not validate that <paramref name="index"/>
47+
/// still addresses the same logical element. It exists to refresh a held element after mutations
48+
/// that are known to have affected only that element's descendant nodes (so its own start index —
49+
/// which precedes all of its content — is unchanged), bringing its cached version up to date so
50+
/// subsequent staleness checks pass. For example, RFC 7396 merge-patch application re-mints the
51+
/// parent element after recursively merging into one of its child objects.
52+
/// </para>
53+
/// </remarks>
54+
TElement RefreshElementUnsafe<TElement>(int index)
55+
where TElement : struct, IJsonElement<TElement>;
56+
3657
/// <summary>
3758
/// Gets the array element at the specified index as a mutable JSON element.
3859
/// </summary>

src/Corvus.Text.Json/Corvus/Text/Json/DocumentBuilder/JsonDocumentBuilder.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2119,6 +2119,17 @@ TElement IMutableJsonDocument.FreezeElement<TElement>(int index)
21192119
#endif
21202120
}
21212121

2122+
/// <inheritdoc />
2123+
TElement IMutableJsonDocument.RefreshElementUnsafe<TElement>(int index)
2124+
{
2125+
CheckNotDisposed();
2126+
#if NET
2127+
return TElement.CreateInstance(this, index);
2128+
#else
2129+
return JsonElementHelpers.CreateInstance<TElement>(this, index);
2130+
#endif
2131+
}
2132+
21222133
/// <summary>
21232134
/// Initializes this builder as a frozen (immutable) copy of a segment of the
21242135
/// source builder's metadb and value backing.

0 commit comments

Comments
 (0)