Skip to content

Refuse to write Newtonsoft JSON deeper than it can read back [patch] - #158

Merged
matt-edmondson merged 2 commits into
mainfrom
fix/155-newtonsoft-write-depth
Oct 20, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
fix/155-newtonsoft-write-depth

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #155

What changed

Newtonsoft.Json enforces MaxDepth (default 64) only when reading, so NewtonsoftJsonSerializationProvider.TrySerialize wrote graphs of any depth and returned true, while Deserialize on the same output returned null.

  • The settings now carry an explicit MaxDepth = 64, so the reader's limit is stated rather than inherited.
  • TrySerialize writes through a private DepthLimitedJsonTextWriter that throws JsonWriterException once an object or array nests past that same limit. The existing catch turns it into false.

Any graph TrySerialize accepts now deserializes back.

Tests

  • TrySerialize_DeepChain_SucceedsOnlyWhenItDeserializes with depths 64, 65 and 200: 64 round-trips; 65 and 200 make TrySerialize return false. On main, the 65 and 200 cases fail (serialize returns true).
  • Full Essentials.Tests suite on net10.0: 1088 passed. The provider project builds clean on every target.

Note

This and #157 (#154, buffer output until success) touch the same using JsonTextWriter line, so whichever merges second needs a small merge. With both in, a depth failure also leaves the caller's writer empty, as the issue asks.


Generated by Claude Code

Newtonsoft enforces MaxDepth only when reading, so TrySerialize wrote
graphs deeper than 64 levels that Deserialize then returned null for.
Write through a JsonTextWriter that throws past the same explicit
MaxDepth, which TrySerialize reports as false.

Fixes #155

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SETCEsuMvZFcF6qXRguNds

Copy link
Copy Markdown
Contributor Author

ci / .NET / Test on windows-latest failed in PersistenceProviderTests.Concurrent_Stores_To_One_Key_Never_Throw_Or_Hide_The_Key. RetrieveAsync threw UnauthorizedAccessException: Access to the path ...\k.json is denied from AtomicFile.ReadAllTextAsync (Shared/AtomicFile.cs:91).

This failure isn't caused by this PR, which only changes the Newtonsoft serialization provider. The cause is a timing race in persistence code that already exists on main. On Windows, a read that arrives while a concurrent overwriting move has the target pending delete gets ERROR_ACCESS_DENIED instead of a sharing violation. ReadAllTextAsync only retries sharing and lock violations, so this case fails straight away. No fix exists yet; #143 covers the same race on the delete paths, not the read path.

Proposed fix, separate from this PR: in ReadAllTextAsync, also retry UnauthorizedAccessException while the file still exists, so a real permissions problem still surfaces after the retries run out.

I've re-run the failed job once.


Generated by Claude Code

@sonarqubecloud

Copy link
Copy Markdown

@matt-edmondson
matt-edmondson merged commit 37ad30c into main Oct 20, 2026
15 checks passed
@matt-edmondson
matt-edmondson deleted the fix/155-newtonsoft-write-depth branch October 20, 2026 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants