Uh oh!
There was an error while loading. Please reload this page.
Fix GetValue<object>() and TryGetValue<JsonElement?> on JsonValueOfJsonPrimitive types - #125139
Conversation
Tagging subscribers to this area: @dotnet/area-system-text-json |
Handle typeof(T) == typeof(object) in TryGetValue for JsonValueOfJsonString, JsonValueOfJsonBool, and JsonValueOfJsonNumber. This fixes InvalidOperationException when calling GetValue<object>() on these types that were introduced in .NET 10. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@copilot@PranavSenthilnathan Based on the changes made in #116798 are there any further variants we should be considering?
The three types fixed here — The other subclasses are not affected:
No further variants to consider. |
There was a problem hiding this comment.
Pull request overview
Fixes a regression in System.Text.Json.Nodes where JsonValueOfJsonString/Bool/Number (used by JsonValueConverter/JsonNodeConverter for primitives) throw on GetValue<object>() by adding explicit typeof(T) == typeof(object) handling in their TryGetValue<T> implementations.
Changes:
- Add
objecthandling inTryGetValue<T>forJsonValueOfJsonString,JsonValueOfJsonBool, andJsonValueOfJsonNumber. - Extend existing
TryGetValue_From*tests withTryGetValue<object>assertions. - Add a new
GetValue_Objecttheory validatingGetValue<object>()for deserialized primitiveJsonValues.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs | Adds explicit object support in TryGetValue<T> for optimized primitive-backed JsonValue implementations. |
| src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonValueTests.cs | Adds regression tests covering TryGetValue<object> and GetValue<object> for primitives. |
Comments suppressed due to low confidence (2)
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs:82
- TryGetValue() for string values now returns a decoded string rather than a boxed JsonElement. In .NET 9 (JsonValueOfElement) and in JsonNode.Parse(...) today, GetValue() yields a JsonElement, so this changes the observable runtime type and may be a compatibility break. Please confirm the intended contract for GetValue/TryGetValue() on deserialized primitives; if compatibility is desired, consider returning JsonElement for object here (and similarly for bool) or update the public docs accordingly.
if (typeof(T) == typeof(string) || typeof(T) == typeof(object)) { string? result = JsonReaderHelper.TranscodeHelper(_value.Span); Debug.Assert(result != null); value = (T)(object)result; return true; }src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs:163
- TryGetValue() for booleans returns boxed bool, which differs from the historical JsonValueOfElement behavior (boxed JsonElement) and from JsonNode.Parse(...) today. If the goal is to preserve .NET 9 behavior for GetValue(), consider returning JsonElement for object here as well (or document that object returns CLR primitives for these optimized deserialized nodes).
if (typeof(T) == typeof(bool) || typeof(T) == typeof(bool?) || typeof(T) == typeof(object)) { value = (T)(object)_value; return true; }You can also share your feedback on Copilot code review. Take the survey.
- TryGetValue() for booleans returns boxed bool, which differs from the historical JsonValueOfElement behavior (boxed JsonElement) and from JsonNode.Parse(...) today. If the goal is to preserve .NET 9 behavior for GetValue(), consider returning JsonElement for object here as well (or document that object returns CLR primitives for these optimized deserialized nodes).
Uh oh!
There was an error while loading. Please reload this page.
stephentoub
commented
Mar 4, 2026
@copilot please invoke the code-review skill and post the analysis/comments as a comment on this PR |
Uh oh!
There was an error while loading. Please reload this page.
🤖 Copilot Code Review — PR #125476Holistic AssessmentMotivation: The regression is real and well-documented — Approach: Adding Summary: Detailed Findings
|
… and add test coverage Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Per reviewer feedback from @PranavSenthilnathan: GetValue<object>() should return JsonElement (not string/bool) for all three JsonValueOfJsonPrimitive types, matching the behavior of JsonValueOfElement where `Value is TypeToConvert` returned JsonElement for object. Updated tests accordingly. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
🤖 Copilot Code Review — PR #125476 (updated after latest changes)Holistic AssessmentMotivation: Real regression — Approach: Adding Summary: ✅ LGTM. The fix is correct, backward-compatible, and well-tested. All three affected types now handle Detailed Findings✅ Correctness — fix matches |
eiriktsarpalis
commented
Mar 18, 2026
/ba-g test failures unrelated. |
Uh oh!
There was an error while loading. Please reload this page.
Description
JsonValueOfJsonString,JsonValueOfJsonBool, andJsonValueOfJsonNumber(introduced in .NET 10) throwInvalidOperationExceptiononGetValue<object>()because theirTryGetValue<T>methods don't handletypeof(T) == typeof(object). The olderJsonValueOfElementhandled this implicitly viaif (Value is TypeToConvert element).Additionally,
TryGetValue<JsonElement?>()was broken on all three types because they use explicittypeof(T) == typeof(JsonElement)checks which don't matchtypeof(JsonElement?). The siblingJsonValueOfElementhandles this naturally viaValue is TypeToConvertpattern matching, but the new types needed explicittypeof(T) == typeof(JsonElement?)checks.Changes
JsonValueOfJsonString,JsonValueOfJsonBool,JsonValueOfJsonNumber): Addedtypeof(T) == typeof(object)andtypeof(T) == typeof(JsonElement?)to theJsonElementbranch ofTryGetValue<T>. This matchesJsonValueOfElement's behavior whereValue is TypeToConvert(line 54) naturally handlesJsonElement,JsonElement?, andobjectby boxing theJsonElementvalue.GetValue<object>()now returnsJsonElementuniformly across all types for backward compatibility.TryGetValue<object>assertions to existingTryGetValue_From*tests and a newGetValue_Objecttheory exercising the converter code path — assertsJsonElementreturn type for all primitive kindsTryGetValue_NullableTypes_Deserializedtest coveringJsonElement?on all three types,bool?onJsonValueOfJsonBool, and all numeric nullable types onJsonValueOfJsonNumberOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.