Conversation
b51bb12 to
4c7dacf
Compare
797709b to
fb7b991
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates JSON persistence, dynamic conversion, CRUD updates, commit handling, tests, and documentation from Newtonsoft.Json to System.Text.Json. It adds compatibility converters, JsonDocument lifetime handling, JsonNode-based test inputs, revised serialization expectations, and migration guidance for version 3.0. ChangesSystem.Text.Json Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
fb7b991 to
6a21d92
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
JsonFlatFileDataStore.Test/CollectionQueryTests.cs (1)
126-126: ⚡ Quick winUpdate test method name to reflect JsonNode usage.
The method name still references
JTokenbut the implementation now usesJsonNode. For consistency with the migration and with other renamed tests inJTokenInputTests.cs, consider renaming this toGetNextIdValue_StringType_JsonNode.📝 Proposed naming update
- public void GetNextIdValue_StringType_JToken() + public void GetNextIdValue_StringType_JsonNode()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore.Test/CollectionQueryTests.cs` at line 126, Rename the test method GetNextIdValue_StringType_JToken to GetNextIdValue_StringType_JsonNode to reflect the switch from JToken to JsonNode; update the method identifier and any references (e.g., test attribute names, calls, or comments) so the test framework and naming consistency with JTokenInputTests.cs are preserved and no dangling references to the old name remain.JsonFlatFileDataStore/CommitActionHandler.cs (2)
80-83: 💤 Low value
actionExceptionis delivered to every callback in the batch.When any action in the batch throws, the same
actionExceptionis passed to every caller'sReady(...), including callers whose own action succeeded (or whose action wasn't the one that threw). Those callers will observe an exception that isn't related to their request — potentially confusing for diagnostics and logging on the consumer side.Consider attributing the exception per-callback (only the throwing action's caller gets it; others get
null), e.g. by widening thecallbackstuple to(action, success, exception).♻️ Sketch
- var callbacks = new Queue<(DataStore.CommitAction action, bool success)>(); + var callbacks = new Queue<(DataStore.CommitAction action, bool success, Exception exception)>(); @@ - callbacks.Enqueue((action, actionSuccess)); + callbacks.Enqueue((action, actionSuccess, null)); @@ - callbacks.Enqueue((action, false)); + callbacks.Enqueue((action, false, e)); @@ - foreach (var (cbAction, cbSuccess) in callbacks) - { - cbAction.Ready(updateSuccess ? cbSuccess : false, actionException); - } + foreach (var (cbAction, cbSuccess, cbException) in callbacks) + { + cbAction.Ready(updateSuccess && cbSuccess, cbException ?? (updateSuccess ? null : actionException)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore/CommitActionHandler.cs` around lines 80 - 83, The loop in CommitActionHandler.cs calls cbAction.Ready(updateSuccess ? cbSuccess : false, actionException) which sends the same actionException to every callback; change the callbacks collection to carry a per-callback exception (e.g., widen tuple to (cbAction, cbSuccess, cbException)) and set cbException only when that specific action thrown, then call cbAction.Ready(cbSuccess, cbException) so only the failing action's caller receives the exception while others get null; update producers of the callbacks tuple and the catch/dispatch logic that assigns actionException to populate the per-callback exception.
46-56: 💤 Low valueReconsider the per-action
JsonDocument.Parse+RootElement.Clone()overhead.Two observations on the hot path inside the foreach:
jsonTextis re-parsed for every action in the batch (up to 50), even when the prior action didn't succeed and therefore didn't changejsonText. For larger documents this is N full parses where N-1 may be redundant.RootElement.Clone()is unnecessary. TheHandleActiondelegate signature returns(bool success, string json)— a string, not aJsonElement— so the passedJsonElementcannot be stored beyond the call. Since theJsonDocumentis alive for the entireHandleActioncall (disposed at the end of the iteration viausing), passingjsonDocument.RootElementdirectly is safe.A minimal change is to drop the
Clone()and passjsonDocument.RootElementdirectly; an additional optimization is to only re-parse when the previous action mutatedjsonText.♻️ Sketch of the optimization
- foreach (var action in batch) + JsonDocument jsonDocument = null; + try { - try - { - using var jsonDocument = JsonDocument.Parse(jsonText); - var rootElement = jsonDocument.RootElement.Clone(); - var (actionSuccess, updatedJson) = action.HandleAction(rootElement); - - callbacks.Enqueue((action, actionSuccess)); - - if (actionSuccess) - jsonText = updatedJson; - } - catch (Exception e) - { - // Record the failure but keep draining the batch ... - actionException = e; - callbacks.Enqueue((action, false)); - } + jsonDocument = JsonDocument.Parse(jsonText); + foreach (var action in batch) + { + try + { + var (actionSuccess, updatedJson) = action.HandleAction(jsonDocument.RootElement); + callbacks.Enqueue((action, actionSuccess)); + + if (actionSuccess) + { + jsonText = updatedJson; + jsonDocument.Dispose(); + jsonDocument = JsonDocument.Parse(jsonText); + } + } + catch (Exception e) + { + actionException = e; + callbacks.Enqueue((action, false)); + } + } + } + finally + { + jsonDocument?.Dispose(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore/CommitActionHandler.cs` around lines 46 - 56, The loop is re-parsing jsonText every iteration and unnecessarily cloning the root element; change it so you only call JsonDocument.Parse(jsonText) when the current jsonText has changed (use a bool like needParse initialized true and set to actionSuccess) and remove RootElement.Clone(), passing jsonDocument.RootElement directly into HandleAction; keep the using scope around the parsed document for each call and still enqueue callbacks via callbacks.Enqueue((action, actionSuccess)) and update jsonText only when actionSuccess is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 828-830: The README has a duplicated section header "## Unit Tests
& Benchmarks" repeated consecutively; remove the redundant second occurrence so
the heading appears only once (search for the exact header string "## Unit Tests
& Benchmarks" and delete the duplicate), and verify surrounding content still
flows correctly and any links/TOC entries point to the single remaining header.
- Around line 699-803: Update the "Dynamically Typed Data" examples to use
System.Text.Json APIs: replace JToken.Parse with JsonNode.Parse, replace
JObject.FromObject usage with an equivalent System.Text.Json pattern (serialize
the source object to JSON and parse/deserialize into JsonNode/JsonObject), and
replace JsonConvert.DeserializeObject<ExpandoObject>() with
JsonSerializer.Deserialize<ExpandoObject>(jsonString, options) using the
SystemExpandoObjectConverter when needed; update any examples and descriptive
text that reference JToken/JObject/JArray to instead reference
JsonNode/JsonObject/JsonArray and show use of GetValue<T>() for value extraction
so the early examples are consistent with v3.x.
- Line 812: The documentation references the internal/non-public subtype
System.Text.Json.JsonReaderException; update the README table row that lists
"Invalid-JSON exception type" to remove the JsonReaderException subtype
reference and instead only list the public API System.Text.Json.JsonException
(and keep the existing Newtonsoft.Json.JsonException column unchanged); ensure
any surrounding prose or footnotes that mention JsonReaderException are removed
or replaced with System.Text.Json.JsonException so callers see only the public
exception type.
---
Nitpick comments:
In `@JsonFlatFileDataStore.Test/CollectionQueryTests.cs`:
- Line 126: Rename the test method GetNextIdValue_StringType_JToken to
GetNextIdValue_StringType_JsonNode to reflect the switch from JToken to
JsonNode; update the method identifier and any references (e.g., test attribute
names, calls, or comments) so the test framework and naming consistency with
JTokenInputTests.cs are preserved and no dangling references to the old name
remain.
In `@JsonFlatFileDataStore/CommitActionHandler.cs`:
- Around line 80-83: The loop in CommitActionHandler.cs calls
cbAction.Ready(updateSuccess ? cbSuccess : false, actionException) which sends
the same actionException to every callback; change the callbacks collection to
carry a per-callback exception (e.g., widen tuple to (cbAction, cbSuccess,
cbException)) and set cbException only when that specific action thrown, then
call cbAction.Ready(cbSuccess, cbException) so only the failing action's caller
receives the exception while others get null; update producers of the callbacks
tuple and the catch/dispatch logic that assigns actionException to populate the
per-callback exception.
- Around line 46-56: The loop is re-parsing jsonText every iteration and
unnecessarily cloning the root element; change it so you only call
JsonDocument.Parse(jsonText) when the current jsonText has changed (use a bool
like needParse initialized true and set to actionSuccess) and remove
RootElement.Clone(), passing jsonDocument.RootElement directly into
HandleAction; keep the using scope around the parsed document for each call and
still enqueue callbacks via callbacks.Enqueue((action, actionSuccess)) and
update jsonText only when actionSuccess is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd92e33b-7d61-4401-9d61-7fb00399c573
📒 Files selected for processing (23)
JsonFlatFileDataStore.Benchmark/Program.csJsonFlatFileDataStore.Test/BehaviorPinningTests.csJsonFlatFileDataStore.Test/CollectionModificationTests.csJsonFlatFileDataStore.Test/CollectionQueryTests.csJsonFlatFileDataStore.Test/CopyPropertiesTests.csJsonFlatFileDataStore.Test/DataStoreDisposeTests.csJsonFlatFileDataStore.Test/DataStoreTests.csJsonFlatFileDataStore.Test/FileContentTests.csJsonFlatFileDataStore.Test/JTokenInputTests.csJsonFlatFileDataStore.Test/JsonOutputFormatTests.csJsonFlatFileDataStore.Test/SchemaFlexibilityTests.csJsonFlatFileDataStore.Test/SingleItemTests.csJsonFlatFileDataStore/CommitActionHandler.csJsonFlatFileDataStore/DataStore.csJsonFlatFileDataStore/DocumentCollection.csJsonFlatFileDataStore/ExpandoObjectConverter.csJsonFlatFileDataStore/GlobalUsings.csJsonFlatFileDataStore/IDataStore.csJsonFlatFileDataStore/IDocumentCollection.csJsonFlatFileDataStore/JsonFlatFileDataStore.csprojJsonFlatFileDataStore/NewtonsoftDateTimeConverter.csJsonFlatFileDataStore/ObjectExtensions.csREADME.md
✅ Files skipped from review due to trivial changes (3)
- JsonFlatFileDataStore.Benchmark/Program.cs
- JsonFlatFileDataStore/GlobalUsings.cs
- JsonFlatFileDataStore/IDataStore.cs
🚧 Files skipped from review as they are similar to previous changes (12)
- JsonFlatFileDataStore.Test/FileContentTests.cs
- JsonFlatFileDataStore.Test/DataStoreTests.cs
- JsonFlatFileDataStore/NewtonsoftDateTimeConverter.cs
- JsonFlatFileDataStore/IDocumentCollection.cs
- JsonFlatFileDataStore/ObjectExtensions.cs
- JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs
- JsonFlatFileDataStore.Test/SingleItemTests.cs
- JsonFlatFileDataStore/DocumentCollection.cs
- JsonFlatFileDataStore.Test/CollectionModificationTests.cs
- JsonFlatFileDataStore/JsonFlatFileDataStore.csproj
- JsonFlatFileDataStore.Test/CopyPropertiesTests.cs
- JsonFlatFileDataStore/DataStore.cs
c142a26 to
200f277
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
JsonFlatFileDataStore/ExpandoObjectConverter.cs (1)
92-214: ⚡ Quick winCollapse the inline array switch into
ProcessNestedArrayto eliminate duplication.The
JsonValueKind.Arraybranch inAddPropertyToExpando(lines 92–214) re-implements almost the same switch already provided byProcessNestedArray(lines 218–271). The two paths must stay in lockstep for type fidelity — the past fix for deep-nested type preservation had to be applied toProcessNestedArray, while this inline copy can easily drift again. SinceProcessNestedArrayalready returns aList<object>with full typed conversion (including the nested-array recursion at line 237), the outer case can delegate to it directly:♻️ Proposed refactor
case JsonValueKind.Array: - var arrayValues = new List<object>(); - foreach (var arrayElement in propertyValue.EnumerateArray()) - { - switch (arrayElement.ValueKind) - { - // ... ~115 lines of duplicated switch ... - } - } - expando[propertyName] = arrayValues; + expando[propertyName] = ProcessNestedArray(propertyValue); break;You may want to rename
ProcessNestedArrayto something likeConvertJsonArrayonce it's used for the top-level array case too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore/ExpandoObjectConverter.cs` around lines 92 - 214, The Array branch inside AddPropertyToExpando duplicates logic present in ProcessNestedArray; replace the entire inline JsonValueKind.Array handling loop with a call to ProcessNestedArray(arrayElement) (or ConvertJsonArray if you choose to rename it) and assign its returned List<object> to expando[propertyName], ensuring TryParseDateTime and object nesting behavior remain provided by ProcessNestedArray so both paths stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JsonFlatFileDataStore/ExpandoObjectConverter.cs`:
- Around line 33-36: The Write method in ExpandoObjectConverter is causing
infinite recursion because JsonSerializer.Serialize(writer, (object)value,
options) re-selects the same converter; update ExpandoObjectConverter.Write to
serialize the ExpandoObject as an IDictionary<string, object> (i.e., call
JsonSerializer.Serialize(writer, (IDictionary<string, object>)value, options))
so the serializer uses the dictionary converter instead and does not re-enter
the same converter; this change fixes the stack overflow observed when
DataStore.Serialize uses _serializerOptions containing
SystemExpandoObjectConverter.
---
Nitpick comments:
In `@JsonFlatFileDataStore/ExpandoObjectConverter.cs`:
- Around line 92-214: The Array branch inside AddPropertyToExpando duplicates
logic present in ProcessNestedArray; replace the entire inline
JsonValueKind.Array handling loop with a call to
ProcessNestedArray(arrayElement) (or ConvertJsonArray if you choose to rename
it) and assign its returned List<object> to expando[propertyName], ensuring
TryParseDateTime and object nesting behavior remain provided by
ProcessNestedArray so both paths stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c9f9efb-3960-4c0a-b39a-916c95b08210
📒 Files selected for processing (16)
JsonFlatFileDataStore.Test/BehaviorPinningTests.csJsonFlatFileDataStore.Test/CaseSensitivityTests.csJsonFlatFileDataStore.Test/CollectionQueryTests.csJsonFlatFileDataStore.Test/DictionarySerializationTests.csJsonFlatFileDataStore.Test/DynamicAccessEdgeCaseTests.csJsonFlatFileDataStore.Test/FileAccessTests.csJsonFlatFileDataStore.Test/JsonNodeInputTests.csJsonFlatFileDataStore.Test/JsonOutputFormatTests.csJsonFlatFileDataStore.Test/NullAndEdgeCaseTests.csJsonFlatFileDataStore.Test/NumericEdgeCaseTests.csJsonFlatFileDataStore.Test/SchemaFlexibilityTests.csJsonFlatFileDataStore.Test/SerializationRoundTripTests.csJsonFlatFileDataStore.Test/TemporalAndIdentifierTests.csJsonFlatFileDataStore/DataStore.csJsonFlatFileDataStore/ExpandoObjectConverter.csREADME.md
✅ Files skipped from review due to trivial changes (8)
- JsonFlatFileDataStore.Test/CaseSensitivityTests.cs
- JsonFlatFileDataStore.Test/NullAndEdgeCaseTests.cs
- JsonFlatFileDataStore.Test/DictionarySerializationTests.cs
- JsonFlatFileDataStore.Test/DynamicAccessEdgeCaseTests.cs
- JsonFlatFileDataStore.Test/NumericEdgeCaseTests.cs
- JsonFlatFileDataStore.Test/FileAccessTests.cs
- JsonFlatFileDataStore.Test/TemporalAndIdentifierTests.cs
- JsonFlatFileDataStore.Test/SerializationRoundTripTests.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- JsonFlatFileDataStore.Test/JsonOutputFormatTests.cs
- JsonFlatFileDataStore.Test/BehaviorPinningTests.cs
- JsonFlatFileDataStore.Test/CollectionQueryTests.cs
- JsonFlatFileDataStore.Test/SchemaFlexibilityTests.cs
- JsonFlatFileDataStore/DataStore.cs
200f277 to
d10a1fb
Compare
d10a1fb to
1650888
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
JsonFlatFileDataStore/DocumentCollection.cs (1)
417-433: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftOptimize field extraction to prevent heavy memory allocations.
Serializing the object to a string and then deserializing it into an
ExpandoObjectjust to extract a single field value (like theidfield) incurs significant performance and memory overhead, especially when called frequently during inserts or updates. Furthermore, creatingnew JsonSerializerOptions()on every call causes unnecessary allocations, and thePropertyNameCaseInsensitiveflag has no effect onExpandoObjectparsing because the custom converter directly uses the JSON property names.Consider extracting the field value directly using reflection for strongly-typed objects or dictionary lookups for dynamics, or use
JsonSerializer.SerializeToElementto avoid string allocations.⚡ Proposed optimization using
SerializeToElementprivate 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); - // 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); - - if (!expandoAsIgnoreCase.ContainsKey(fieldName)) + if (item is IDictionary<string, object> dict) + { + var key = dict.Keys.FirstOrDefault(k => string.Equals(k, fieldName, StringComparison.OrdinalIgnoreCase)); + return key != null ? dict[key] : null; + } + + var element = JsonSerializer.SerializeToElement(item); + foreach (var property in element.EnumerateObject()) + { + if (string.Equals(property.Name, fieldName, StringComparison.OrdinalIgnoreCase)) + { + return GetElementValue(property.Value); + } + } + return null; + } + + private dynamic GetElementValue(JsonElement element) + { + switch (element.ValueKind) + { + case JsonValueKind.Number: + if (element.TryGetInt64(out long l)) return l; + if (element.TryGetDouble(out double d)) return d; + return element.GetRawText(); + case JsonValueKind.String: + return element.GetString(); + case JsonValueKind.True: + return true; + case JsonValueKind.False: + return false; + default: + return element.GetRawText(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore/DocumentCollection.cs` around lines 417 - 433, Update GetFieldValue to avoid serializing to a JSON string and deserializing into an ExpandoObject on every lookup. Extract fields directly through reflection for strongly typed items and dictionary lookup for dynamic or dictionary values, using ordinal case-insensitive matching; if serialization remains necessary, reuse serializer options and use SerializeToElement instead of an intermediate string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs`:
- Around line 112-114: Update the assertions in
JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs at lines 112-114 and 138 to
cast expected integer values to long before comparing with retrieved values:
cast i and i * 10 at lines 112-114, and i at line 138. Preserve the existing
assertion structure and values.
In `@README.md`:
- Around line 26-29: Update the Version 3.0 entry under Major Version Changes to
qualify the System.Text.Json availability statement: say it is included in
modern .NET runtimes, while the netstandard2.0 target still obtains it through
the System.Text.Json package dependency in JsonFlatFileDataStore.csproj.
- Around line 774-793: Update the date-handling documentation around the dynamic
retrieval examples and performance note to distinguish the two converter paths:
document that SingleDynamicItemReadConverter() uses DateTime.TryParse()
heuristics, while SystemExpandoObjectConverter only parses explicitly supported
ISO 8601 formats through TryParseExact(). Revise the nearby comparison table
consistently, avoiding a single automatic-date-parsing rule for all dynamic
values.
---
Outside diff comments:
In `@JsonFlatFileDataStore/DocumentCollection.cs`:
- Around line 417-433: Update GetFieldValue to avoid serializing to a JSON
string and deserializing into an ExpandoObject on every lookup. Extract fields
directly through reflection for strongly typed items and dictionary lookup for
dynamic or dictionary values, using ordinal case-insensitive matching; if
serialization remains necessary, reuse serializer options and use
SerializeToElement instead of an intermediate string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4ad5aab-d4fe-4fe1-8913-7bf4717cb10d
📒 Files selected for processing (31)
JsonFlatFileDataStore.Benchmark/Program.csJsonFlatFileDataStore.Test/BehaviorPinningTests.csJsonFlatFileDataStore.Test/CaseSensitivityTests.csJsonFlatFileDataStore.Test/CollectionModificationTests.csJsonFlatFileDataStore.Test/CollectionQueryTests.csJsonFlatFileDataStore.Test/CopyPropertiesTests.csJsonFlatFileDataStore.Test/DataStoreDisposeTests.csJsonFlatFileDataStore.Test/DataStoreTests.csJsonFlatFileDataStore.Test/DictionarySerializationTests.csJsonFlatFileDataStore.Test/DynamicAccessEdgeCaseTests.csJsonFlatFileDataStore.Test/FileAccessTests.csJsonFlatFileDataStore.Test/FileContentTests.csJsonFlatFileDataStore.Test/JsonNodeInputTests.csJsonFlatFileDataStore.Test/JsonOutputFormatTests.csJsonFlatFileDataStore.Test/NullAndEdgeCaseTests.csJsonFlatFileDataStore.Test/NumericEdgeCaseTests.csJsonFlatFileDataStore.Test/SchemaFlexibilityTests.csJsonFlatFileDataStore.Test/SerializationRoundTripTests.csJsonFlatFileDataStore.Test/SingleItemTests.csJsonFlatFileDataStore.Test/TemporalAndIdentifierTests.csJsonFlatFileDataStore/CommitActionHandler.csJsonFlatFileDataStore/DataStore.csJsonFlatFileDataStore/DocumentCollection.csJsonFlatFileDataStore/ExpandoObjectConverter.csJsonFlatFileDataStore/GlobalUsings.csJsonFlatFileDataStore/IDataStore.csJsonFlatFileDataStore/IDocumentCollection.csJsonFlatFileDataStore/JsonFlatFileDataStore.csprojJsonFlatFileDataStore/NewtonsoftDateTimeConverter.csJsonFlatFileDataStore/ObjectExtensions.csREADME.md
🚧 Files skipped from review as they are similar to previous changes (24)
- JsonFlatFileDataStore/GlobalUsings.cs
- JsonFlatFileDataStore.Test/SerializationRoundTripTests.cs
- JsonFlatFileDataStore.Test/FileContentTests.cs
- JsonFlatFileDataStore.Benchmark/Program.cs
- JsonFlatFileDataStore.Test/NumericEdgeCaseTests.cs
- JsonFlatFileDataStore.Test/NullAndEdgeCaseTests.cs
- JsonFlatFileDataStore/IDocumentCollection.cs
- JsonFlatFileDataStore/JsonFlatFileDataStore.csproj
- JsonFlatFileDataStore/IDataStore.cs
- JsonFlatFileDataStore.Test/SingleItemTests.cs
- JsonFlatFileDataStore.Test/BehaviorPinningTests.cs
- JsonFlatFileDataStore.Test/SchemaFlexibilityTests.cs
- JsonFlatFileDataStore.Test/JsonOutputFormatTests.cs
- JsonFlatFileDataStore.Test/DataStoreTests.cs
- JsonFlatFileDataStore.Test/CollectionQueryTests.cs
- JsonFlatFileDataStore/NewtonsoftDateTimeConverter.cs
- JsonFlatFileDataStore.Test/TemporalAndIdentifierTests.cs
- JsonFlatFileDataStore/ObjectExtensions.cs
- JsonFlatFileDataStore.Test/CopyPropertiesTests.cs
- JsonFlatFileDataStore.Test/CollectionModificationTests.cs
- JsonFlatFileDataStore/ExpandoObjectConverter.cs
- JsonFlatFileDataStore.Test/JsonNodeInputTests.cs
- JsonFlatFileDataStore.Test/DictionarySerializationTests.cs
- JsonFlatFileDataStore/DataStore.cs
| Assert.Equal(i, retrieved.id); | ||
| Assert.Equal($"User{i}", retrieved.name); | ||
| Assert.Equal(i * 10, retrieved.data.value); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix strict type mismatch in xUnit assertions. The custom SystemExpandoObjectConverter maintains backward compatibility by parsing JSON integers as Int64. When xUnit compares these dynamically typed long values against int expected values, the assertion fails because Assert.Equal(object, object) strictly checks types. Cast the expected int values to long to prevent test failures.
JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs#L112-L114: Castiandi * 10tolongbefore comparison (e.g.,Assert.Equal((long)i, retrieved.id);).JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs#L138: Castitolongbefore comparison (e.g.,Assert.Equal((long)i, retrieved.id);).
📍 Affects 1 file
JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs#L112-L114(this comment)JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs#L138-L138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs` around lines 112 - 114,
Update the assertions in JsonFlatFileDataStore.Test/DataStoreDisposeTests.cs at
lines 112-114 and 138 to cast expected integer values to long before comparing
with retrieved values: cast i and i * 10 at lines 112-114, and i at line 138.
Preserve the existing assertion structure and values.
5d2e569 to
efecace
Compare
GetItem/GetItem<T> read JsonElement tokens that point into the current JsonDocument's buffer without holding _jsonDataLock. The background commit thread disposes the previous document the instant it swaps in a new one (SetJsonData), so a concurrent read raced into ObjectDisposedException. This is a regression from the Newtonsoft implementation, where JObject/JToken are managed objects that are never disposed, making lock-free reads safe. Take _jsonDataLock around both reader bodies, and around CommitItem's HandleAction (which reassigns _jsonData and serializes it) so a reader or Reload cannot dispose the document mid-operation. Reproduced deterministically with 4 reader threads + 1 writer before the fix (ObjectDisposedException); 0 errors after. All 239 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The camelCase _toJsonFunc round-tripped the whole document through ExpandoObject on every commit. SystemExpandoObjectConverter runs a date-detection heuristic over every string, so any string matching an ISO date format (e.g. a "2015-11-23" version field or an ISO date used as an opaque id) was silently rewritten to "2015-11-23T00:00:00" the next time any unrelated part of the store was written. Replace the ExpandoObject round-trip with a Utf8JsonWriter walk that applies lower camelCase to property names only, preserving every value byte-for-byte. This eliminates the coercion, is faster (no intermediate object graph), and preserves numeric precision: very large decimals now round-trip exactly instead of being truncated through double, so the behavior-pinning test that documented that Newtonsoft limitation is updated to assert exact round-trip. Verified: a "2015-11-23" value survives an unrelated insert; PascalCase keys are still normalized to camelCase. All 239 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SingleDynamicItemReadConverter (the GetItem(key) dynamic path) used a permissive DateTime.TryParse with a CultureInfo.CurrentCulture fallback, while the collection dynamic path (SystemExpandoObjectConverter) used a strict, invariant-culture TryParseExact against explicit ISO 8601 formats. Two problems: - Inconsistency: the same value (e.g. "6/15/2009") became a DateTime via GetItem(key) but stayed a string via GetCollection. - Portability: the CurrentCulture fallback made single-item results depend on the machine's locale, so the same file parsed differently across machines. Route the single-item path through the same strict helper (SystemExpandoObjectConverter.TryParseDateTime, now internal). Dynamic reads now make one strict, locale-independent date guess everywhere; typed reads (GetItem<DateTime>) remain permissive because the caller declared the type. Updated GetItem_DynamicAndTyped_DateType to pin the unified behavior. All 239 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two README sections described behavior that the date/decimal fixes reversed: - Decimal precision: the write path no longer routes through ExpandoObject/double, so values are stored verbatim and typed round-trips (including decimal.MaxValue) are now exact — they no longer emit scientific notation or throw on read. Rewrote the "Known Limitations" section and the comparison-table row to scope the remaining precision loss to dynamic reads only (ExpandoObject surfaces non-integer numbers as double). - Dynamic date parsing: the description still claimed permissive DateTime.TryParse over "all string values". Both dynamic paths now use a strict, invariant-culture ISO 8601 TryParseExact, so non-ISO/locale-specific strings (6/15/2009) stay strings. Updated the date-handling section, the performance note, and the migration note to match; this aligns the prose with the "Dynamic string -> DateTime promotion" table row that already documented the strict behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GetFieldValue runs on every insert via GetNextIdValue. It allocated a new JsonSerializerOptions per call, which throws away System.Text.Json's cached type metadata, and serialized the whole item to a JSON string only to parse it back. Dynamic items are already dictionaries, so read the field directly from them. Typed and JsonNode items still go through serialization, as the id value has to be in the same format as in JSON (e.g. a Guid as a string), but now with a reused options instance and SerializeToElement instead of an intermediate string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
System.Text.Json is included in modern .NET runtimes, but the netstandard2.0 target still obtains it through the package reference in the csproj. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f5b8af9 to
bf33fb0
Compare
This branch has not been deployed
1 participant
Implement #101
Summary by CodeRabbit
Release Notes
Breaking Changes
New Features / Enhancements
Bug Fixes / Quality
Documentation