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

Add a STJ feature flag treating non-optional constructor parameters as required #100075

Closed
Tracked by #100159
eiriktsarpalis opened this issue Mar 21, 2024 · 4 comments · Fixed by #103096
Closed
Tracked by #100159
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Mar 21, 2024

Background and motivation

System.Text.Json constructor-based deserialization currently treats all constructor parameters as optional, as can be highlighted in the following example:

var result = JsonSerializer.Deserialize<Person>("{}");
Console.WriteLine(result); // Person { Name = , Age = 0 }

public record Person(string Name, int Age);

This can create unexpected validation problems since users typically think of non-optional parameters as required values in the object contract. It typically also results in deserialized properties violating their own nullability annotations. Another problem is that it could result either in discrepancies or overly verbose outputs in components that are attempting to derive a JSON schema from the STJ contracts.

Changing this behavior would most likely constitute a serious breaking change, so we would instead like to expose a feature flag that opts in to new behavior, marking non-optional constructor parameters as required. Rather than making this an application-wide feature switch, this should instead be a per-JsonSerializerOptions setting to avoid breaking third-party components making use of STJ.

Related to #1256

API Proposal

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool RequireConstructorParameters { get; set; } = false;
}

namespace System.Text.Json.Serialization;

public partial class JsonSourceGenerationOptionsAttribute
{
    public bool RequireConstructorParameters { get; set; } = false;
}

API Usage

var options = new JsonSerializerOptions {  RequireNonOptionalConstructorParameters = true };

JsonSerializer.Deserialize<Person>("""{"Name" : "John", "Age" : 27 }""", options); // Success
JsonSerializer.Deserialize<Person>("""{"Name" : "John", "Address" : "known"}""", options);
//  JSON deserialization for type 'Person' was missing required properties, including the following: Age

public record Person(string Name, int Age, string Address = "unknown");

Alternative Designs

This proposal is very closely related to #100144. We should consider a design that unifies both behaviors under a single feature switch.

Risks

No response

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 21, 2024
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.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 21, 2024
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Mar 21, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 21, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this Mar 21, 2024
@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 21, 2024
@eiriktsarpalis eiriktsarpalis changed the title [API Proposal]: Add a STJ feature flag treating non-optional constructor parameters as required Add a STJ feature flag treating non-optional constructor parameters as required Mar 22, 2024
@eiriktsarpalis eiriktsarpalis added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 23, 2024
@bartonjs
Copy link
Member

bartonjs commented Apr 23, 2024

Video

  • RequireConstructorParameters => RespectRequiredConstructorParameters
  • Should use an AppContext value to determine the default, but absent one it should be on (breaking change).
    • System.Text.Json.JsonSerializer.RespectRequiredConstructorParametersDefault
namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool RespectRequiredConstructorParameters { get; set; } = AppContext(true);
}

namespace System.Text.Json.Serialization;

public partial class JsonSourceGenerationOptionsAttribute
{
    public bool RespectRequiredConstructorParameters { get; set; } = AppContext(true);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 23, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 23, 2024
Copy link
Contributor

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

  1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

@eiriktsarpalis eiriktsarpalis removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Jun 5, 2024
@eiriktsarpalis
Copy link
Member Author

Similar to nullability annotations, we're reverting this to be an opt-in feature hence it no longer constitutes a breaking change.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants