Skip to content

[release/10.0-preview6] JSON: Don't mark unmapped properties as read #116828

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jun 19, 2025

Backport of #116807 to release/10.0-preview6

/cc @PranavSenthilnathan

Customer Impact

  • Customer reported
  • Found internally

Found in #116801 and blocking dotnet/aspnetcore#53467 / dotnet/aspnetcore#62112 (passkey support for ASP.NET). If a class with a constructor and/or required properties is being deserialized into and the JSON payload contains an unmapped property, then there will be a NullReferenceException. Here is the reported example:

using System.Text.Json;
using System.Text.Json.Serialization;

// Note the additional "Baz" property
var json = """
    {
        "Bar": "asdf",
        "Baz": "hello"
    }
    """;

_ = JsonSerializer.Deserialize(json, MyJsonSerializerContext.Default.Foo); // <-- NullReferenceException here

class Foo
{
    // Note the use of 'required' here
    public required string? Bar { get; set; }
}

[JsonSerializable(typeof(Foo))]
partial class MyJsonSerializerContext : JsonSerializerContext;

Regression

  • Yes
  • No

Yes, this was introduced as part a new feature but part of the changes in that PR regressed old behavior. Introduced in #115856.

During deserialization we keep track of the properties that are read in order to do duplicate detection and required property validation. When a parametrized constructor is specified for deserialization, there are four types of JSON properties:

  1. Properties that map to a constructor parameter
  2. Properties that don't map to a constructor parameter but still map to a property on the class
  3. Properties that map to the ExtensionData property
  4. Properties that are unmapped

(The last two cases are only differentiated by whether the class has a property with the ExtensionData attribute.)

#115856 added duplicate detection by keeping track of properties that were assigned. Paths to handle cases 1, 2 and 3 were added but case 4 was an edge case that was not considered. So when this case was hit we would try to find the mapping like we do for case 1 or 2 when we actually don't have one. This PR fixes this case by explicitly checking for it and skipping duplicate detection for it.

Testing

The example in the reported issue was added as a test case and now passes with the new fix. This tests both source generation and reflection modes. The reported issue is linked in the test case.

Risk

Low, the fix is essentially preventing a NullReferenceException by checking that the object is a valid to dereference.

@PranavSenthilnathan
Copy link
Member

/cc @joperezr @MackinnonBuck if you want to track this

@PranavSenthilnathan PranavSenthilnathan added Servicing-consider Issue for next servicing release review area-System.Text.Json labels Jun 19, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 19, 2025
@PranavSenthilnathan
Copy link
Member

/ba-g Known test failures, unrelated to this PR. #116671 and #116558

@jozkee jozkee removed the request for review from jeffhandley June 19, 2025 20:25
@jozkee jozkee merged commit 3d44749 into release/10.0-preview6 Jun 19, 2025
90 of 103 checks passed
@ViktorHofer ViktorHofer deleted the backport/pr-116807-to-release/10.0-preview6 branch June 19, 2025 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants