feat: replace Newtonsoft.Json with System.Text.Json#111
Conversation
ttu
force-pushed
the
newtonsoft-to-system-text-json
branch
12 times, most recently
from
b51bb12 to
4c7dacf
Compare
ttu
force-pushed
the
newtonsoft-to-system-text-json
branch
from
4c7dacf to
797709b
Compare
ttu
force-pushed
the
newtonsoft-to-system-text-json
branch
from
797709b to
fb7b991
Compare
WalkthroughThis pull request executes a major version upgrade (v3.0) that replaces Newtonsoft.Json with System.Text.Json across the entire library. The migration introduces new custom converters for ExpandoObject and DateTime handling, refactors the DataStore's internal JSON representation from JObject/JToken to JsonDocument/JsonElement, and updates all tests to use System.Text.Json APIs. Supporting infrastructure includes new helper methods for JSON navigation and mutation, synchronized access via dedicated locks, and comprehensive test updates reflecting the new serialization behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
JsonFlatFileDataStore/DataStore.cs (1)
144-150: Race condition: unprotectedSetJsonDatacall inGetItemmethods.When
_reloadBeforeGetCollectionis true,SetJsonDatais called without lock protection, creating a TOCTOU window. The background commit thread (CommitActionHandler) can modify_jsonDatabetween the reload and the subsequent read, causing inconsistent state or exceptions.Wrap the reload and read operations with
_jsonDataLock:public T GetItem<T>(string key) { if (_reloadBeforeGetCollection) { - SetJsonData(GetJsonObjectFromFile()); + lock (_jsonDataLock) + { + SetJsonData(GetJsonObjectFromFile()); + } } var convertedKey = _convertPathToCorrectCamelCase(key); - var token = TryGetElement(_jsonData.RootElement, convertedKey); + lock (_jsonDataLock) + { + var token = TryGetElement(_jsonData.RootElement, convertedKey); + }Apply the same fix to
GetItem(string key)at lines 169–185.Also applies to: 169-175
🧹 Nitpick comments (9)
JsonFlatFileDataStore/DocumentCollection.cs (1)
419-433: Consider cachingJsonSerializerOptionsto avoid repeated allocations.Creating
JsonSerializerOptionson everyGetFieldValuecall is inefficient, especially since this method is called frequently during collection operations (inserts, updates, ID generation). The options object is identical each time and can be safely cached as a static field.+ private static readonly JsonSerializerOptions _fieldValueOptions = new JsonSerializerOptions + { + Converters = { new SystemExpandoObjectConverter() }, + PropertyNameCaseInsensitive = true + }; + private dynamic GetFieldValue(T item, string fieldName) { - var options = new JsonSerializerOptions - { - Converters = { new SystemExpandoObjectConverter() }, - PropertyNameCaseInsensitive = true // Optional: make property name matching case-insensitive - }; - - var expando = JsonSerializer.Deserialize<ExpandoObject>(JsonSerializer.Serialize(item), options); + var expando = JsonSerializer.Deserialize<ExpandoObject>(JsonSerializer.Serialize(item, _fieldValueOptions), _fieldValueOptions); // Problem here is if we have typed data with upper camel case properties but lower camel case in JSON, so need to use OrdinalIgnoreCase string comparer var expandoAsIgnoreCase = new Dictionary<string, dynamic>(expando, StringComparer.OrdinalIgnoreCase);JsonFlatFileDataStore.Test/CollectionQueryTests.cs (1)
140-150: Minor inconsistency in JsonNode value extraction.Line 142 uses
ToString()while line 149 usesGetValue<string>()for the same purpose (extracting a string value fromJsonNode). Both work, butGetValue<string>()is more explicit and consistent with theGetValue<int>()call on line 150.var nextUpdate = JsonNode.Parse("{ \"myId\": \"somethingWrong2\" }"); collection.InsertOne(nextUpdate); - Assert.Equal(nextId, nextUpdate["myId"].ToString()); + Assert.Equal(nextId, nextUpdate["myId"].GetValue<string>());JsonFlatFileDataStore/NewtonsoftDateTimeConverter.cs (2)
27-27: Potential null reference when date string is null.
reader.GetString()can returnnullif the JSON contains an explicitnullvalue (thoughTokenTypewould beNull, notStringin that case). However, if the string is empty or whitespace,dateStringwill be non-null but fail all parsing attempts, reaching line 60. Consider using the already-fetcheddateStringin the exception message instead of callingGetString()again.- throw new JsonException($"Invalid date format: {reader.GetString()}"); + throw new JsonException($"Invalid date format: {dateString}");
30-37: Consider making formats array static for better performance.The
formatsarray is constant and recreated on everyReadcall. Moving it to aprivate static readonlyfield avoids repeated allocations.public class NewtonsoftDateTimeConverter : JsonConverter<DateTime> { private readonly string _defaultFormat = "yyyy'-'MM'-'dd'T'HH':'mm':'ss.FFFFFFFK"; // ISO 8601 format + + private static readonly string[] _formats = new[] + { + "yyyy-MM-ddTHH:mm:ss.FFFFFFFK", // Full precision with timezone + "yyyy-MM-ddTHH:mm:ss.FFFFFFF", // Full precision without timezone + "yyyy-MM-ddTHH:mm:ssK", // Seconds with timezone + "yyyy-MM-ddTHH:mm:ss", // Seconds without timezone (Newtonsoft default) + "yyyy-MM-dd" // Date only + }; public override DateTime Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (reader.TokenType == JsonTokenType.String) { string dateString = reader.GetString(); - // Common formats to try - string[] formats = new[] - { - "yyyy-MM-ddTHH:mm:ss.FFFFFFFK", // Full precision with timezone - "yyyy-MM-ddTHH:mm:ss.FFFFFFF", // Full precision without timezone - "yyyy-MM-ddTHH:mm:ssK", // Seconds with timezone - "yyyy-MM-ddTHH:mm:ss", // Seconds without timezone (Newtonsoft default) - "yyyy-MM-dd" // Date only - }; - // Try parsing with various formats - foreach (var format in formats) + foreach (var format in _formats)README.md (1)
26-28: Consider using proper heading syntax for consistency.Static analysis flags "Emphasis used instead of a heading" at line 26. Using a proper heading (
##or setext style) would be more consistent with the rest of the document.-**Major Version Changes** +## Major Version ChangesJsonFlatFileDataStore/ExpandoObjectConverter.cs (1)
94-224: Consider extracting shared value conversion logic to reduce duplication.The value conversion logic (handling
Number,String,True,False,Null,Object) is duplicated across object properties (lines 54-92), array elements (lines 98-149), and nested array elements (lines 156-215). Extracting a shared helper likeConvertJsonElement(JsonElement element)would improve maintainability.Example helper approach:
private static object ConvertJsonElement(JsonElement element) { return element.ValueKind switch { JsonValueKind.Null or JsonValueKind.Undefined => null, JsonValueKind.True => true, JsonValueKind.False => false, JsonValueKind.Number => GetNumber(element), JsonValueKind.String => TryParseDateTime(element.GetString()), JsonValueKind.Object => ConvertToExpando(element), JsonValueKind.Array => ConvertToList(element), _ => null }; }JsonFlatFileDataStore/DataStore.cs (3)
53-63:_toJsonFuncroundtrip may be inefficient.The
useLowerCamelCase=truepath deserializesJsonElement→ExpandoObject→ re-serializes with_serializerOptions. This roundtrip is expensive. Verify that it's necessary (e.g., for case-insensitive property handling viaObjectExtensions.CopyProperties) or consider a more direct serialization path that avoids the intermediate deserialization.
475-501: DateTime parsing heuristic inSingleDynamicItemReadConverteris fragile.The character-based heuristic (length ≥ 8, starts with digit, contains separators) may misidentify strings or fail on valid ISO dates without separators. Additionally, repeated
DateTime.TryParse()calls are expensive for non-date strings.Since
NewtonsoftDateTimeConverteris registered in_options(line 23), verify whether this manual parsing is necessary or if the converter already handles all required formats. If this code exists to handle an edge case, document why it's needed.
313-319: ExtractIsCollectionandIsItempredicates to class-level private methods.These locally-defined predicates are clear but would benefit from extraction for reusability and testability. If the same collection-vs-item detection is needed elsewhere, this avoids duplication.
private bool IsCollection(JsonElement property) => property.ValueKind == JsonValueKind.Array && (!property.EnumerateArray().Any() || property.EnumerateArray().First().ValueKind == JsonValueKind.Object); private bool IsItem(JsonElement property) => property.ValueKind != JsonValueKind.Array || (property.EnumerateArray().Any() && property.EnumerateArray().First().ValueKind != JsonValueKind.Object);Then simplify GetKeys to use
this.IsCollection(property.Value)andthis.IsItem(property.Value).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
JsonFlatFileDataStore.Benchmark/Program.cs(1 hunks)JsonFlatFileDataStore.Test/CollectionModificationTests.cs(13 hunks)JsonFlatFileDataStore.Test/CollectionQueryTests.cs(2 hunks)JsonFlatFileDataStore.Test/CopyPropertiesTests.cs(7 hunks)JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs(1 hunks)JsonFlatFileDataStore.Test/DataStoreTests.cs(10 hunks)JsonFlatFileDataStore.Test/FileContentTests.cs(2 hunks)JsonFlatFileDataStore.Test/SingleItemTests.cs(2 hunks)JsonFlatFileDataStore/CommitActionHandler.cs(1 hunks)JsonFlatFileDataStore/DataStore.cs(16 hunks)JsonFlatFileDataStore/DocumentCollection.cs(1 hunks)JsonFlatFileDataStore/ExpandoObjectConverter.cs(1 hunks)JsonFlatFileDataStore/GlobalUsings.cs(1 hunks)JsonFlatFileDataStore/IDataStore.cs(1 hunks)JsonFlatFileDataStore/IDocumentCollection.cs(1 hunks)JsonFlatFileDataStore/JsonFlatFileDataStore.csproj(2 hunks)JsonFlatFileDataStore/NewtonsoftDateTimeConverter.cs(1 hunks)JsonFlatFileDataStore/ObjectExtensions.cs(6 hunks)README.md(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
JsonFlatFileDataStore.Test/CollectionQueryTests.cs (3)
JsonFlatFileDataStore/DocumentCollection.cs (3)
ReplaceOne(102-129)ReplaceOne(131-131)InsertOne(38-50)JsonFlatFileDataStore/IDocumentCollection.cs (3)
ReplaceOne(74-74)ReplaceOne(83-83)InsertOne(44-44)JsonFlatFileDataStore/ObjectExtensions.cs (1)
GetValue(448-451)
JsonFlatFileDataStore/CommitActionHandler.cs (1)
JsonFlatFileDataStore/DataStore.cs (3)
JsonDocument(529-533)JsonDocument(547-550)JsonDocument(579-593)
JsonFlatFileDataStore/NewtonsoftDateTimeConverter.cs (1)
JsonFlatFileDataStore/ExpandoObjectConverter.cs (1)
Write(32-35)
JsonFlatFileDataStore/DocumentCollection.cs (3)
JsonFlatFileDataStore/DataStore.cs (4)
dynamic(169-185)dynamic(457-516)T(144-167)T(552-565)JsonFlatFileDataStore/ObjectExtensions.cs (3)
dynamic(131-142)dynamic(144-154)dynamic(489-492)JsonFlatFileDataStore/ExpandoObjectConverter.cs (2)
SystemExpandoObjectConverter(7-283)ExpandoObject(9-30)
JsonFlatFileDataStore.Test/SingleItemTests.cs (2)
JsonFlatFileDataStore/DataStore.cs (2)
InsertItem(187-187)DeleteItem(265-265)JsonFlatFileDataStore/IDataStore.cs (2)
InsertItem(71-71)DeleteItem(123-123)
JsonFlatFileDataStore.Test/CopyPropertiesTests.cs (2)
JsonFlatFileDataStore/ExpandoObjectConverter.cs (2)
SystemExpandoObjectConverter(7-283)ExpandoObject(9-30)JsonFlatFileDataStore/DataStore.cs (2)
dynamic(169-185)dynamic(457-516)
🪛 LanguageTool
README.md
[style] ~799-~799: Consider using “incompatible” to avoid wordiness.
Context: ...eatures require runtime support and are not compatible with .NET Standard 2.0. ## API API is...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
README.md
26-26: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
678-678: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
688-688: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
690-690: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
713-713: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
723-723: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
752-752: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
773-773: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
797-797: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (31)
JsonFlatFileDataStore.Benchmark/Program.cs (1)
9-12: LGTM!Formatting adjustment with no functional changes.
JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs (1)
77-142: Well-structured resource management test.The test effectively validates that JsonDocument instances are properly managed throughout insert/read/delete operations, ensuring no premature disposal causes ObjectDisposedException. The multiple GC cycles help expose any lifecycle issues.
Note: While the test name mentions "NoMemoryLeaks", it primarily validates disposal correctness rather than measuring actual memory consumption.
JsonFlatFileDataStore.Test/FileContentTests.cs (1)
26-53: Excellent documentation of serialization differences.The updated test data and detailed comments clearly explain the byte-length variations between Newtonsoft.Json and System.Text.Json across different formatting options and operating systems. This makes the test expectations explicit and maintainable.
JsonFlatFileDataStore/GlobalUsings.cs (1)
1-2: LGTM!Adding the global using for System.Text.Json aligns with the migration strategy and will simplify JSON-related code throughout the project.
JsonFlatFileDataStore/IDocumentCollection.cs (1)
2-2: LGTM!The using directive is appropriate for the interface's existing dynamic type usage (e.g.,
GetNextIdValue(),UpdateOne()parameters).JsonFlatFileDataStore/IDataStore.cs (1)
2-2: LGTM!The using directive appropriately supports the interface's existing dynamic type usage throughout its method signatures.
JsonFlatFileDataStore/CommitActionHandler.cs (1)
44-46: Proper JsonDocument lifecycle management.The migration correctly uses
JsonDocument.Parsewith a using statement and clones the RootElement before passing it toHandleAction. TheClone()call is essential here because it creates an independent JsonElement that survives the disposal of the parent JsonDocument at the end of the using block.JsonFlatFileDataStore/JsonFlatFileDataStore.csproj (1)
35-35: System.Text.Json version 10.0.0 is valid and available on NuGet. No known security vulnerabilities identified for this version.JsonFlatFileDataStore.Test/CopyPropertiesTests.cs (4)
4-5: LGTM!Import changes correctly reflect the migration from Newtonsoft.Json to System.Text.Json.
253-261: LGTM!The serialization/deserialization pattern using
SystemExpandoObjectConverteris the correct approach for converting JSON toExpandoObjectwith System.Text.Json, maintaining compatibility with the previous Newtonsoft.Json behavior.
286-289: LGTM!Good documentation of the xUnit 2.9.3+ workaround for ambiguity errors with dynamic types. The explicit
(object)cast is the correct solution.
322-330: LGTM!Consistent serialization pattern with
SystemExpandoObjectConverterfor typed object property copying.JsonFlatFileDataStore/DocumentCollection.cs (1)
408-414: Good addition of Int32 handling for System.Text.Json compatibility.System.Text.Json returns
Int32for smaller integers that fit within the range, unlike Newtonsoft.Json which often returnedInt64. This explicit check ensures correct ID incrementing behavior.However, note that the order of checks matters: checking
Int64first (line 408) beforeInt32(line 411) is fine since C# won't implicitly box anInt32asInt64for dynamic comparison.JsonFlatFileDataStore.Test/SingleItemTests.cs (2)
78-94: LGTM!Excellent test coverage for DateTime handling. The tests verify that both typed and dynamic retrieval correctly parse date strings to
DateTime, maintaining backward compatibility with Newtonsoft.Json behavior. The comments clearly document the expected behavior.
421-501: LGTM!Comprehensive test that validates the System.Text.Json migration for complex data types. Good coverage of:
- Nested objects with arrays and dictionaries
- Simple arrays
- Mixed type objects (strings, ints, doubles, booleans, nulls, dates)
- Deletion operations to verify
RemoveJsonDataElementworks correctlyThe test structure follows the existing patterns in the file and properly cleans up test artifacts.
JsonFlatFileDataStore.Test/DataStoreTests.cs (3)
3-7: LGTM!Using directives correctly updated to bring in
System.Text.JsonandSystem.Text.Json.Nodesnamespaces for the migration.
21-21: LGTM!JSON string updated to use double quotes as required by the JSON specification. System.Text.Json is stricter than Newtonsoft.Json and requires valid JSON syntax.
316-322: LGTM!Good documentation explaining the API difference between System.Text.Json's
JsonNode(requires explicitGetValue<T>()) and Newtonsoft.Json'sJToken(had implicit conversion operators). This helps future maintainers understand why the explicit calls are necessary.JsonFlatFileDataStore.Test/CollectionQueryTests.cs (1)
1-1: LGTM!Using directive correctly added for
System.Text.Json.Nodesto supportJsonNodeusage.JsonFlatFileDataStore/NewtonsoftDateTimeConverter.cs (1)
63-67: LGTM!The
Writemethod correctly serializesDateTimevalues using the ISO 8601 format with full precision, maintaining consistency with the documented behavior.README.md (1)
678-782: Comprehensive migration documentation.The migration notes clearly document the differences between Newtonsoft.Json and System.Text.Json, with practical code examples for common scenarios. The performance considerations and backward compatibility notes are particularly helpful for users upgrading from v2.x.
JsonFlatFileDataStore/ObjectExtensions.cs (4)
23-31: Serialize-then-deserialize pattern for JsonNode conversion.The approach of serializing and deserializing to convert
JsonNodetoExpandoObjectis correct and necessary given the API differences. ThePropertyNameCaseInsensitive = trueoption ensures consistency with the case-insensitive handling throughout.
41-48: LGTM on JsonNode handling.The
JsonObjecthandling correctly usesJsonValue.Create(data)to set field values dynamically. This aligns with System.Text.Json patterns.
299-315: Case-insensitive property matching correctly implemented.The implementation properly maintains backward compatibility with Newtonsoft.Json's case-insensitive property matching. The pattern of finding existing keys and preserving original casing is correct.
320-348: LGTM on HandleExpandoEnumerable updates.The case-insensitive key handling is consistent with the pattern used in
HandleExpando. The null check and fallback tosrcProp.Nameensures new properties are added correctly.JsonFlatFileDataStore.Test/CollectionModificationTests.cs (3)
1-4: Test imports updated for System.Text.Json migration.The using statements correctly reference
System.Text.JsonandSystem.Text.Json.Nodesfor the migration.
455-461: JsonNode parsing and iteration correctly migrated.The test properly uses
JsonNode.Parse()followed by.AsArray()and.Select()to iterate over JSON array elements, correctly replacing the Newtonsoft.Json pattern.
887-895: Test demonstrates correct SystemExpandoObjectConverter usage.The test properly uses the new
SystemExpandoObjectConverterpattern documented in the README for creating patchExpandoObjectinstances from dictionaries.JsonFlatFileDataStore/ExpandoObjectConverter.cs (2)
9-30: Read method correctly handles JsonDocument lifecycle.The
usingstatement ensures proper disposal ofJsonDocument, and the implementation correctly builds anExpandoObjectfrom the JSON structure.
258-282: Well-documented performance tradeoff for DateTime parsing.The
TryParseDateTimehelper clearly documents the performance implications and the reasoning for maintaining Newtonsoft.Json compatibility. The suggestion to use strongly-typed collections for performance-critical scenarios is helpful.JsonFlatFileDataStore/DataStore.cs (1)
519-526: Resource management forJsonDocumentis well-handled.The use of
usingstatements (lines 571, 605), explicit disposal inSetJsonData(line 525), and cleanup inDispose(line 119) properly manageJsonDocumentlifetime and prevent resource leaks. TheClone()pattern forJsonElementis correct and necessary given the struct lifetime constraints.Also applies to: 571-577, 605-609
| (bool, JsonElement) UpdateAction() | ||
| { | ||
| if (_jsonData[convertedKey] == null) | ||
| return (false, _jsonData); | ||
| var data = TryGetElement(_jsonData.RootElement, convertedKey); | ||
| if (data == null) | ||
| return (false, _jsonData.RootElement); | ||
|
|
||
| var toUpdate = SingleDynamicItemReadConverter(_jsonData[convertedKey]); | ||
| var toUpdate = SingleDynamicItemReadConverter(data.Value); | ||
|
|
||
| if (ObjectExtensions.IsReferenceType(item) && ObjectExtensions.IsReferenceType(toUpdate)) | ||
| { | ||
| ObjectExtensions.CopyProperties(item, toUpdate); | ||
| _jsonData[convertedKey] = JToken.FromObject(toUpdate); | ||
| var newElement = ConvertToJsonElement(toUpdate); | ||
| _jsonData = SetJsonDataElement(_jsonData.RootElement, convertedKey, newElement); | ||
| } | ||
| else | ||
| { | ||
| _jsonData[convertedKey] = JToken.FromObject(item); | ||
| var newElement = ConvertToJsonElement(item); | ||
| _jsonData = SetJsonDataElement(_jsonData.RootElement, convertedKey, newElement); | ||
| } | ||
|
|
||
| return (true, _jsonData); | ||
| return (true, _jsonData.RootElement); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the DataStore.cs file and examine CommitActionHandler and lock usage
find . -name "DataStore.cs" -type fRepository: ttu/json-flatfile-datastore
Length of output: 107
🏁 Script executed:
# Search for CommitActionHandler class definition and HandleAction
rg -B 3 -A 20 "class CommitActionHandler" --type csRepository: ttu/json-flatfile-datastore
Length of output: 1941
🏁 Script executed:
# Search for _jsonDataLock usage patterns
rg "_jsonDataLock|lock.*_jsonData" --type cs -B 2 -A 2Repository: ttu/json-flatfile-datastore
Length of output: 2050
🏁 Script executed:
# Look at UpdateAction to see where it's called from
rg -B 15 "UpdateAction\(\)" --type cs | head -80Repository: ttu/json-flatfile-datastore
Length of output: 4597
🏁 Script executed:
# Find CommitItem method definition
rg -B 5 -A 30 "CommitItem\(" --type cs | head -100Repository: ttu/json-flatfile-datastore
Length of output: 7657
🏁 Script executed:
# Look for where the UpdateAction callback is actually executed
rg -B 10 -A 10 "CommitAction\|HandleAction" --type cs | head -150Repository: ttu/json-flatfile-datastore
Length of output: 53
🏁 Script executed:
# Examine full DataStore.cs context around CommitItem and how actions are invoked
cat -n JsonFlatFileDataStore/DataStore.cs | sed -n '160,200p'Repository: ttu/json-flatfile-datastore
Length of output: 1601
🏁 Script executed:
# Look for where the action is invoked from CommitActionHandler
rg "action.Action\|\.Invoke\|action\.Callback" --type cs -B 3 -A 3Repository: ttu/json-flatfile-datastore
Length of output: 53
🏁 Script executed:
# Find CommitItem method definition more directly
rg -B 2 -A 40 "private.*Task.*CommitItem" --type csRepository: ttu/json-flatfile-datastore
Length of output: 3078
🏁 Script executed:
# Look for where the action/callback is actually executed in DataStore
rg -B 5 -A 15 "CommitAction|action\(\)" --type cs | head -120Repository: ttu/json-flatfile-datastore
Length of output: 8610
🏁 Script executed:
# Check SetJsonData method
rg -B 2 -A 8 "private.*SetJsonData\(" --type csRepository: ttu/json-flatfile-datastore
Length of output: 919
🏁 Script executed:
# Verify GetItem doesn't use lock
rg -B 3 -A 8 "public dynamic GetItem" --type csRepository: ttu/json-flatfile-datastore
Length of output: 879
🏁 Script executed:
# Verify the exact line numbers for lines 251 and 256 in Update method
sed -n '239,260p' JsonFlatFileDataStore/DataStore.cs | cat -nRepository: ttu/json-flatfile-datastore
Length of output: 1131
🏁 Script executed:
# Get full CommitActionHandler implementation to see if any locks are used
cat JsonFlatFileDataStore/CommitActionHandler.csRepository: ttu/json-flatfile-datastore
Length of output: 2374
Verify lock semantics for _jsonData assignment in Update method.
Lines 251 and 256 directly assign _jsonData without explicit _jsonDataLock protection. The UpdateAction callback executes in CommitActionHandler.HandleStoreCommitActions on a background thread, where concurrent reads from GetItem() are not serialized. Other code paths that modify _jsonData use explicit lock (_jsonDataLock) guards (e.g., UpdateAll, Reload, constructor), making this a thread-safety inconsistency. Confirm whether the unprotected write-read access is intentional or if _jsonDataLock is required here.
🤖 Prompt for AI Agents
In JsonFlatFileDataStore/DataStore.cs around lines 239 to 260, UpdateAction
assigns to the shared field _jsonData without using the existing _jsonDataLock,
causing a thread-safety inconsistency with other writers; fix by acquiring
lock(_jsonDataLock) around any read-modify-write access to _jsonData inside
UpdateAction (i.e., wrap the TryGetElement/read of RootElement and the
subsequent SetJsonDataElement assignment in the same lock) so readers and other
writers remain synchronized, or alternatively ensure CommitActionHandler invokes
UpdateAction while already holding the same lock — pick one approach and apply
it consistently.
| private JsonDocument SetJsonDataElement(JsonElement original, string key, object item) | ||
| { | ||
| // Convert _jsonData to a Dictionary to make modifications | ||
| var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(original.GetRawText()); | ||
|
|
||
| // Convert the item to JsonElement | ||
| var newElement = ConvertToJsonElement(item); | ||
|
|
||
| // Set or update the element in the dictionary | ||
| jsonDataDict[key] = newElement; | ||
|
|
||
| // Serialize back to JsonElement | ||
| var modifiedJson = JsonSerializer.Serialize(jsonDataDict); | ||
| return JsonDocument.Parse(modifiedJson); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the DataStore.cs file
fd -e cs | grep -i datastoreRepository: ttu/json-flatfile-datastore
Length of output: 1410
🏁 Script executed:
# Look at the file structure
ls -la JsonFlatFileDataStore/Repository: ttu/json-flatfile-datastore
Length of output: 1166
🏁 Script executed:
# Examine lines 579-609 of DataStore.cs
cat -n JsonFlatFileDataStore/DataStore.cs | sed -n '575,615p'Repository: ttu/json-flatfile-datastore
Length of output: 2056
🌐 Web query:
JsonElement deserialization Dictionary .NET safety orphaned references buffer
💡 Result:
Short answer
- JsonElement/JsonDocument expose views over an underlying UTF-8 buffer — they do NOT own a deep copy. If that buffer or the JsonDocument is disposed or reused (e.g., returned from a using or returned while the source buffer is pooled), the JsonElement becomes invalid (or “orphaned”). [1][2]
What to do (safe options)
- Deserialize directly into the target Dictionary (JsonSerializer.Deserialize<Dictionary<string,T>>) so you never hold JsonElement references. This is the simplest and safest approach. [2][3]
- If you must keep a JsonElement past the JsonDocument lifetime, call JsonElement.Clone() (or clone the RootElement). Clone produces a standalone copy safe to store. [1]
- When reading from a pooled buffer / Utf8JsonReader, prefer APIs that copy the data for you (JsonSerializer.Deserialize overloads that accept reader/span) or take care not to return elements that reference a rented buffer. [2]
Extra notes
- Avoid using Dictionary<TKey, TValue> with non-string keys unless you know the risks; System.Text.Json has limitations and deserializing arbitrary key types can be unsafe. [4]
Sources
[1] StackOverflow: JsonElement / JsonDocument lifetime and JsonElement.Clone. (stackoverflow.com)
[2] Microsoft docs: JsonSerializer.Deserialize overloads and notes about reader/span copying. (docs.microsoft.com)
[3] Microsoft docs: Deserialization guidance for System.Text.Json. (learn.microsoft.com)
[4] Microsoft: Migrate from Newtonsoft.Json — warning about Dictionary key types and security. (learn.microsoft.com)
If you want, paste the code path you’re using (JsonDocument.Parse/ParseAsync, Utf8JsonReader, JsonSerializer, etc.) and I’ll give exact code snippets to make it safe.
Citations:
- 1: https://stackoverflow.com/questions/63591180/system-text-json-cannot-access-a-dispose-jsondocument?utm_source=openai
- 2: https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.deserialize?utm_source=openai
- 3: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/deserialization?utm_source=openai
- 4: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?utm_source=openai
🌐 Web query:
JsonSerializer.Deserialize Dictionary<string, JsonElement> C# safety issues
💡 Result:
Short answer — common safety / correctness issues when calling JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(...) and how to mitigate them:
-
Deep / large JSON (DoS / stack/recursion limits, high memory). System.Text.Json enforces a max-depth and can throw or consume lots of memory for huge payloads; configure and validate MaxDepth/streaming or reject oversized input. [1][2][3]
-
Retained buffers / lifetime / allocations. JsonElement values come from parsed DOM structures (JsonDocument/parse path) and can hold references to underlying buffers if you misuse APIs (avoid returning JsonDocument.RootElement from a transient document; prefer JsonElement.Parse or streaming deserialization for large/long‑lived data). Use streaming (Utf8JsonReader) when possible. [2]
-
Unexpected types / polymorphism risk (security). While System.Text.Json does not use Newtonsoft's TypeNameHandling by default, deserializing into loosely typed containers (Dictionary<TKey, TValue> where TKey or TValue is not constrained) can allow attackers to send unexpected data shapes that your code later interprets unsafely. Treat untrusted JSON as untrusted input and validate contents before acting on them. Microsoft documents a caution about Dictionary key types and security. [1][4]
-
Converters / behavior differences vs Newtonsoft. System.Text.Json will materialize untyped object-like values as JsonElement — code that assumed Newtonsoft behavior may mis-handle types (e.g., expecting raw CLR types). Test code paths that enumerate or convert JsonElement values. [2][5]
-
Trimming/AOT and reflection issues. Calling parameterless Deserialize(string) can require reflection metadata that influences AOT/trimming; prefer source-generated JsonTypeInfo/JsonSerializerContext or supply JsonSerializerOptions in constrained environments. [2]
Mitigations (practical):
- Validate input size and depth up-front; set/verify JsonReaderOptions/JsonSerializerOptions.MaxDepth and reject overly deep or large payloads. [3]
- Use streaming APIs (Utf8JsonReader / Deserialize with reader) for large or untrusted payloads to avoid whole-DOM allocation. [2][6]
- If you need DOM but not to hold buffers, use JsonElement.Parse or create a JsonDocument you control and keep its lifetime explicit; don’t return RootElement from a disposed document. [2]
- Validate keys and values explicitly (types, allowed properties, lengths) before using them; prefer strongly typed DTOs where possible. [1][4]
- For AOT/trimming scenarios, use source-generated contexts or explicit JsonTypeInfo to avoid missing converters at runtime. [2]
Sources:
- Microsoft docs — “Migrate from Newtonsoft.Json to System.Text.Json” (caution about Dictionary key types). [1]
- Microsoft API docs & .NET blog (JsonSerializer.Deserialize overloads, JsonElement/JsonDocument lifetime, guidance to not return JsonDocument.RootElement). [2]
- .NET runtime / GitHub and community notes on MaxDepth and denial‑of‑service considerations. [3]
- .NET code‑analysis guidance about insecure deserialization (Newtonsoft example; general principle to restrict types and validate input). [4]
If you want, I can: (a) show example safe code (streaming + validation), or (b) review a short snippet of your current deserialization code and point out concrete fixes.
Unsafe JsonElement deserialization in SetJsonDataElement and RemoveJsonDataElement.
Deserializing to Dictionary<string, JsonElement> creates JsonElement instances that reference a temporary JsonDocument's buffer. When this document is garbage-collected, the JsonElement references become invalid, causing exceptions or data corruption during re-serialization at lines 591 and 604.
Replace the deserialization type with Dictionary<string, object>:
private JsonDocument SetJsonDataElement(JsonElement original, string key, object item)
{
- var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(original.GetRawText());
+ var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, object>>(original.GetRawText());
var newElement = ConvertToJsonElement(item);
jsonDataDict[key] = newElement;
var modifiedJson = JsonSerializer.Serialize(jsonDataDict);
return JsonDocument.Parse(modifiedJson);
}
public (bool, JsonElement) RemoveJsonDataElement(JsonElement original, string key)
{
- var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(original.GetRawText());
+ var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, object>>(original.GetRawText());
var removed = jsonDataDict.Remove(key);
var modifiedJson = JsonSerializer.Serialize(jsonDataDict);
using var jsonDocument = JsonDocument.Parse(modifiedJson);
return (removed, jsonDocument.RootElement.Clone());
}Also applies to: 595-609
| public (bool, JsonElement) RemoveJsonDataElement(JsonElement original, string key) | ||
| { | ||
| // Deserialize _jsonData to a dictionary for modification | ||
| var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(original.GetRawText()); | ||
|
|
||
| // Remove the specified key | ||
| var removed = jsonDataDict.Remove(key); | ||
|
|
||
| // Serialize the updated dictionary back to a JsonElement | ||
| var modifiedJson = JsonSerializer.Serialize(jsonDataDict); | ||
| using var jsonDocument = JsonDocument.Parse(modifiedJson); | ||
|
|
||
| // Clone the root element so it doesn't reference the disposed JsonDocument | ||
| return (removed, jsonDocument.RootElement.Clone()); | ||
| } |
There was a problem hiding this comment.
Unexpected public visibility on RemoveJsonDataElement helper.
RemoveJsonDataElement is marked public but only called internally from Delete (line 275). Consider making it private to avoid accidental public API exposure and clarify internal-only intent.
- public (bool, JsonElement) RemoveJsonDataElement(JsonElement original, string key)
+ private (bool, JsonElement) RemoveJsonDataElement(JsonElement original, string key)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public (bool, JsonElement) RemoveJsonDataElement(JsonElement original, string key) | |
| { | |
| // Deserialize _jsonData to a dictionary for modification | |
| var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(original.GetRawText()); | |
| // Remove the specified key | |
| var removed = jsonDataDict.Remove(key); | |
| // Serialize the updated dictionary back to a JsonElement | |
| var modifiedJson = JsonSerializer.Serialize(jsonDataDict); | |
| using var jsonDocument = JsonDocument.Parse(modifiedJson); | |
| // Clone the root element so it doesn't reference the disposed JsonDocument | |
| return (removed, jsonDocument.RootElement.Clone()); | |
| } | |
| private (bool, JsonElement) RemoveJsonDataElement(JsonElement original, string key) | |
| { | |
| // Deserialize _jsonData to a dictionary for modification | |
| var jsonDataDict = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(original.GetRawText()); | |
| // Remove the specified key | |
| var removed = jsonDataDict.Remove(key); | |
| // Serialize the updated dictionary back to a JsonElement | |
| var modifiedJson = JsonSerializer.Serialize(jsonDataDict); | |
| using var jsonDocument = JsonDocument.Parse(modifiedJson); | |
| // Clone the root element so it doesn't reference the disposed JsonDocument | |
| return (removed, jsonDocument.RootElement.Clone()); | |
| } |
🤖 Prompt for AI Agents
In JsonFlatFileDataStore/DataStore.cs around lines 595 to 609, the helper
RemoveJsonDataElement is declared public but is only used internally (called
from Delete at line 275); change its visibility to private to prevent exposing
it in the public API and clarify intent, ensuring any references (such as the
call in Delete) remain valid and run unit tests/compile to confirm no external
usages depend on it.
| else | ||
| { | ||
| arrayValues.Add(element.ToString()); | ||
| } |
There was a problem hiding this comment.
Type information lost for primitive values in deeply nested arrays.
element.ToString() converts all primitive values (numbers, booleans) to strings, which differs from the type-preserving behavior in AddPropertyToExpando. This could cause unexpected behavior when accessing deeply nested array values (e.g., 3 becomes "3").
Consider handling all value kinds consistently:
else
{
- arrayValues.Add(element.ToString());
+ switch (element.ValueKind)
+ {
+ case JsonValueKind.Number:
+ if (element.TryGetInt32(out int intVal)) arrayValues.Add(intVal);
+ else if (element.TryGetInt64(out long longVal)) arrayValues.Add(longVal);
+ else if (element.TryGetDouble(out double doubleVal)) arrayValues.Add(doubleVal);
+ else arrayValues.Add(element.GetDecimal());
+ break;
+ case JsonValueKind.True:
+ arrayValues.Add(true);
+ break;
+ case JsonValueKind.False:
+ arrayValues.Add(false);
+ break;
+ case JsonValueKind.String:
+ arrayValues.Add(TryParseDateTime(element.GetString()));
+ break;
+ default:
+ arrayValues.Add(null);
+ break;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In JsonFlatFileDataStore/ExpandoObjectConverter.cs around lines 249-252, the
code unconditionally calls element.ToString() when adding deeply nested array
primitives, losing original types (numbers, booleans, null). Replace that
ToString() call with type-aware handling consistent with AddPropertyToExpando:
inspect element.Type (or use element.Type switch / JTokenType) and for
Objects/Arrays recurse into the same conversion logic, for Strings use
element.Value<string>(), for Integers/Floats use
element.Value<long>/Value<double>/ToObject<decimal>() as appropriate (or
element.ToObject<object>() if you prefer a generic numeric), for Boolean use
element.Value<bool>(), and for Null add null; then add the preserved-typed value
to arrayValues.
1 participant
Implement #101
Summary by CodeRabbit
Release Notes
New Features
encryptionKeyandminifyJsonparameters for enhanced serialization control.Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.