Skip to content
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

[release/9.0] Ensure required properties are validated before invoking the deserialization constructor. #107126

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 29, 2024

Backport of #107083 to release/9.0

/cc @eiriktsarpalis

Customer Impact

  • Customer reported
  • Found internally

.NET 9 introduces the JsonSerializerOptions.RespectRequiredConstructorParameters property that enables treating non-optional constructor parameters as required in the JSON payload when deserializing an object. This builds upon existing infrastructure for properties marked required. It has been reported by a customer that this validation is being run after the deserialization constructor has been invoked, meaning that uninitialized parameters could be passed to the constructor even with validation turned on. The issue typically manifests as exceptions being surfaced from the constructor itself (NRE or ArgumentException usually) as opposed to the expected JsonException detailing which required parameter is missing from the payload.

Regression

  • Yes
  • No

Testing

Added unit testing validating the impacted use case.

Risk

Low. The change is a straightforward moving of required parameter validation to the top of the deserialization methods. Pedantically speaking this could be seen as a behavior change in cases where deserialization constructors perform side-effects, however that's an unlikely scenario we don't want to condone or support.

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.

@jeffhandley
Copy link
Member

/ba-g The failures are known and unrelated

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved.

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 4, 2024
@artl93 artl93 merged commit ea363cc into release/9.0 Sep 4, 2024
86 of 89 checks passed
@jeffhandley jeffhandley deleted the backport/pr-107083-to-release/9.0 branch September 4, 2024 20:57
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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