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

Implement JsonSerializerOptions.RespectRequiredConstructorParameters #103096

Merged
merged 4 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ public JsonSourceGenerationOptionsAttribute(JsonSerializerDefaults defaults)
/// </summary>
public bool RespectNullableAnnotations { get; set; }

/// <summary>
/// Specifies the default value of <see cref="JsonSerializerOptions.RespectRequiredConstructorParameters"/> when set.
/// </summary>
public bool RespectRequiredConstructorParameters { get; set; }

/// <summary>
/// Specifies the default value of <see cref="JsonSerializerOptions.UnknownTypeHandling"/> when set.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,9 @@ private static void GetLogicForDefaultSerializerOptionsInit(SourceGenerationOpti
if (optionsSpec.RespectNullableAnnotations is bool respectNullableAnnotations)
writer.WriteLine($"RespectNullableAnnotations = {FormatBoolLiteral(respectNullableAnnotations)},");

if (optionsSpec.RespectRequiredConstructorParameters is bool respectRequiredConstructorParameters)
writer.WriteLine($"RespectRequiredConstructorParameters = {FormatBoolLiteral(respectRequiredConstructorParameters)},");

if (optionsSpec.IgnoreReadOnlyFields is bool ignoreReadOnlyFields)
writer.WriteLine($"IgnoreReadOnlyFields = {FormatBoolLiteral(ignoreReadOnlyFields)},");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ private SourceGenerationOptionsSpec ParseJsonSourceGenerationOptionsAttribute(IN
JsonKnownNamingPolicy? dictionaryKeyPolicy = null;
bool? respectNullableAnnotations = null;
bool? ignoreReadOnlyFields = null;
bool? respectRequiredConstructorParameters = null;
bool? ignoreReadOnlyProperties = null;
bool? includeFields = null;
int? maxDepth = null;
Expand Down Expand Up @@ -334,6 +335,10 @@ private SourceGenerationOptionsSpec ParseJsonSourceGenerationOptionsAttribute(IN
respectNullableAnnotations = (bool)namedArg.Value.Value!;
break;

case nameof(JsonSourceGenerationOptionsAttribute.RespectRequiredConstructorParameters):
respectRequiredConstructorParameters = (bool)namedArg.Value.Value!;
break;

case nameof(JsonSourceGenerationOptionsAttribute.IgnoreReadOnlyFields):
ignoreReadOnlyFields = (bool)namedArg.Value.Value!;
break;
Expand Down Expand Up @@ -418,6 +423,7 @@ private SourceGenerationOptionsSpec ParseJsonSourceGenerationOptionsAttribute(IN
DefaultIgnoreCondition = defaultIgnoreCondition,
DictionaryKeyPolicy = dictionaryKeyPolicy,
RespectNullableAnnotations = respectNullableAnnotations,
RespectRequiredConstructorParameters = respectRequiredConstructorParameters,
IgnoreReadOnlyFields = ignoreReadOnlyFields,
IgnoreReadOnlyProperties = ignoreReadOnlyProperties,
IncludeFields = includeFields,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public sealed record SourceGenerationOptionsSpec

public required bool? RespectNullableAnnotations { get; init; }

public required bool? RespectRequiredConstructorParameters { get; init; }

public required bool? IgnoreReadOnlyFields { get; init; }

public required bool? IgnoreReadOnlyProperties { get; init; }
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ public JsonSerializerOptions(System.Text.Json.JsonSerializerOptions options) { }
public System.Text.Json.JsonCommentHandling ReadCommentHandling { get { throw null; } set { } }
public System.Text.Json.Serialization.ReferenceHandler? ReferenceHandler { get { throw null; } set { } }
public bool RespectNullableAnnotations { get { throw null; } set { } }
public bool RespectRequiredConstructorParameters { get { throw null; } set { } }
public System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver? TypeInfoResolver { get { throw null; } set { } }
public System.Collections.Generic.IList<System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver> TypeInfoResolverChain { get { throw null; } }
public System.Text.Json.Serialization.JsonUnknownTypeHandling UnknownTypeHandling { get { throw null; } set { } }
Expand Down Expand Up @@ -1093,6 +1094,7 @@ public JsonSourceGenerationOptionsAttribute(System.Text.Json.JsonSerializerDefau
public System.Text.Json.Serialization.JsonKnownNamingPolicy PropertyNamingPolicy { get { throw null; } set { } }
public System.Text.Json.JsonCommentHandling ReadCommentHandling { get { throw null; } set { } }
public bool RespectNullableAnnotations { get { throw null; } set { } }
public bool RespectRequiredConstructorParameters { get { throw null; } set { } }
public System.Text.Json.Serialization.JsonUnknownTypeHandling UnknownTypeHandling { get { throw null; } set { } }
public System.Text.Json.Serialization.JsonUnmappedMemberHandling UnmappedMemberHandling { get { throw null; } set { } }
public bool UseStringEnumConverter { get { throw null; } set { } }
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@
<value>JsonPropertyInfo '{0}' defined in type '{1}' is marked both as required and as an extension data property. This combination is not supported.</value>
</data>
<data name="JsonRequiredPropertiesMissing" xml:space="preserve">
<value>JSON deserialization for type '{0}' was missing required properties, including the following: {1}</value>
<value>JSON deserialization for type '{0}' was missing required properties including: {1}.</value>
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="ObjectCreationHandlingPopulateNotSupportedByConverter" xml:space="preserve">
<value>Property '{0}' on type '{1}' is marked with JsonObjectCreationHandling.Populate but it doesn't support populating. This can be either because the property type is immutable or it could use a custom converter.</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,11 @@ internal static class AppContextSwitchHelper
switchName: "System.Text.Json.Serialization.RespectNullableAnnotationsDefault",
isEnabled: out bool value)
? value : false;

public static bool RespectRequiredConstructorParametersDefault { get; } =
AppContext.TryGetSwitch(
switchName: "System.Text.Json.Serialization.RespectRequiredConstructorParametersDefault",
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
isEnabled: out bool value)
? value : false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ public bool Equals(JsonSerializerOptions? left, JsonSerializerOptions? right)
left._allowOutOfOrderMetadataProperties == right._allowOutOfOrderMetadataProperties &&
left._allowTrailingCommas == right._allowTrailingCommas &&
left._respectNullableAnnotations == right._respectNullableAnnotations &&
left._respectRequiredConstructorParameters == right._respectRequiredConstructorParameters &&
left._ignoreNullValues == right._ignoreNullValues &&
left._ignoreReadOnlyProperties == right._ignoreReadOnlyProperties &&
left._ignoreReadonlyFields == right._ignoreReadonlyFields &&
Expand Down Expand Up @@ -567,6 +568,7 @@ public int GetHashCode(JsonSerializerOptions options)
AddHashCode(ref hc, options._allowOutOfOrderMetadataProperties);
AddHashCode(ref hc, options._allowTrailingCommas);
AddHashCode(ref hc, options._respectNullableAnnotations);
AddHashCode(ref hc, options._respectRequiredConstructorParameters);
AddHashCode(ref hc, options._ignoreNullValues);
AddHashCode(ref hc, options._ignoreReadOnlyProperties);
AddHashCode(ref hc, options._ignoreReadonlyFields);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public static JsonSerializerOptions Web
private bool _allowOutOfOrderMetadataProperties;
private bool _allowTrailingCommas;
private bool _respectNullableAnnotations = AppContextSwitchHelper.RespectNullableAnnotationsDefault;
private bool _respectRequiredConstructorParameters = AppContextSwitchHelper.RespectRequiredConstructorParametersDefault;
private bool _ignoreNullValues;
private bool _ignoreReadOnlyProperties;
private bool _ignoreReadonlyFields;
Expand Down Expand Up @@ -141,6 +142,7 @@ public JsonSerializerOptions(JsonSerializerOptions options)
_allowOutOfOrderMetadataProperties = options._allowOutOfOrderMetadataProperties;
_allowTrailingCommas = options._allowTrailingCommas;
_respectNullableAnnotations = options._respectNullableAnnotations;
_respectRequiredConstructorParameters = options._respectRequiredConstructorParameters;
_ignoreNullValues = options._ignoreNullValues;
_ignoreReadOnlyProperties = options._ignoreReadOnlyProperties;
_ignoreReadonlyFields = options._ignoreReadonlyFields;
Expand Down Expand Up @@ -797,6 +799,9 @@ public string NewLine
/// Due to restrictions in how nullable reference types are represented at run time,
/// this setting only governs nullability annotations of non-generic properties and fields.
/// It cannot be used to enforce nullability annotations of root-level types or generic parameters.
///
/// The default setting for this property can be toggled application-wide using the
/// "System.Text.Json.Serialization.RespectNullableAnnotationsDefault" feature switch.
/// </remarks>
public bool RespectNullableAnnotations
{
Expand All @@ -808,6 +813,29 @@ public bool RespectNullableAnnotations
}
}

/// <summary>
/// Gets or sets a value that indicates whether non-optional constructor parameters should be specified during deserialization.
/// </summary>
/// <exception cref="InvalidOperationException">
/// Thrown if this property is set after serialization or deserialization has occurred.
/// </exception>
/// <remarks>
/// For historical reasons constructor-based deserialization treats all constructor parameters as optional by default.
/// This flag allows users to toggle that behavior as necessary for each <see cref="JsonSerializerOptions"/> instance.
///
/// The default setting for this property can be toggled application-wide using the
/// "System.Text.Json.Serialization.RespectRequiredConstructorParametersDefault" feature switch.
/// </remarks>
public bool RespectRequiredConstructorParameters
{
get => _respectRequiredConstructorParameters;
set
{
VerifyMutable();
_respectRequiredConstructorParameters = value;
}
}

/// <summary>
/// Returns true if options uses compatible built-in resolvers or a combination of compatible built-in resolvers.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,5 +123,9 @@ public ICustomAttributeProvider? AttributeProvider
internal JsonNumberHandling? NumberHandling => MatchingProperty.EffectiveNumberHandling;
internal JsonTypeInfo JsonTypeInfo => MatchingProperty.JsonTypeInfo;
internal bool ShouldDeserialize => !MatchingProperty.IsIgnored;
internal bool IsRequiredParameter =>
Options.RespectRequiredConstructorParameters &&
!HasDefaultValue &&
!IsMemberInitializer;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ public bool IsRequired
}
}

private bool _isRequired;
private protected bool _isRequired;

/// <summary>
/// Gets the constructor parameter associated with the current property.
Expand Down Expand Up @@ -448,7 +448,7 @@ internal void Configure()

if (IsRequired)
{
if (!CanDeserialize)
if (!CanDeserialize && AssociatedParameter?.IsRequiredParameter != true)
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
{
ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndNotDeserializable(this);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ internal override void AddJsonParameterInfo(JsonParameterInfoValues parameterInf
AssociatedParameter = new JsonParameterInfo<T>(parameterInfoValues, this);
// Overwrite the nullability annotation of property setter with the parameter.
_isSetNullable = parameterInfoValues.IsNullable;
// If the property has been associated with a non-optional parameter, mark it as required.
_isRequired |= AssociatedParameter.IsRequiredParameter;
}

internal new JsonConverter<T> EffectiveConverter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo p
Debug.Assert(parent.PropertyCache != null);

// Soft cut-off length - once message becomes longer than that we won't be adding more elements
const int CutOffLength = 50;
Copy link
Member Author

Choose a reason for hiding this comment

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

Because I've updated the error message so that property names are wrapped with single quotes, I'm also increasing the cutoff so that roughly speaking the same number of properties are being displayed.

const int CutOffLength = 60;

foreach (KeyValuePair<string, JsonPropertyInfo> kvp in parent.PropertyCache.List)
{
Expand All @@ -313,7 +313,9 @@ public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo p
listOfMissingPropertiesBuilder.Append(' ');
}

listOfMissingPropertiesBuilder.Append('\'');
listOfMissingPropertiesBuilder.Append(property.Name);
listOfMissingPropertiesBuilder.Append('\'');
first = false;

if (listOfMissingPropertiesBuilder.Length >= CutOffLength)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ namespace System.Text.Json.Serialization.Tests
{
public abstract partial class ConstructorTests : SerializerTests
{
private static readonly JsonSerializerOptions s_respectRequiredParamsOptions = new() { RespectRequiredConstructorParameters = true };

public ConstructorTests(JsonSerializerWrapper stringSerializer)
: base(stringSerializer)
{
Expand Down Expand Up @@ -1636,5 +1638,35 @@ public class CustomCtorParameterConverter : JsonConverter<string>
public override void Write(Utf8JsonWriter writer, string value, JsonSerializerOptions options)
=> writer.WriteStringValue(value);
}

[Fact]
public async Task RespectRequiredConstructorParameters_RequiredParameterMissing_ThrowsJsonException()
{
string json = """{"X":1,"Z":3}""";
JsonException ex = await Assert.ThrowsAsync<JsonException>(() => Serializer.DeserializeWrapper<Point_3D>(json, s_respectRequiredParamsOptions));
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
Assert.DoesNotContain("'X'", ex.Message);
Assert.Contains("'Y'", ex.Message);
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
Assert.DoesNotContain("'Z'", ex.Message);
}

[Fact]
public async Task RespectRequiredConstructorParameters_OptionalParameterMissing_Succeeds()
{
string json = """{"X":1,"Y":2}""";
Point_3D result = await Serializer.DeserializeWrapper<Point_3D>(json, s_respectRequiredParamsOptions);
Assert.Equal(1, result.X);
Assert.Equal(2, result.Y);
Assert.Equal(50, result.Z);
}

[Fact]
public async Task RespectRequiredConstructorParameters_NoParameterMissing_Succeeds()
{
string json = """{"X":1,"Y":2,"Z":3}""";
Point_3D result = await Serializer.DeserializeWrapper<Point_3D>(json, s_respectRequiredParamsOptions);
Assert.Equal(1, result.X);
Assert.Equal(2, result.Y);
Assert.Equal(3, result.Z);
}
}
}
45 changes: 45 additions & 0 deletions src/libraries/System.Text.Json/tests/Common/MetadataTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,39 @@ public void JsonTypeInfo_ElementType_ReturnsExpectedValue(Type type, Type? expec
Assert.Equal(expectedKeyType, typeInfo.ElementType);
}

[Theory]
[InlineData(typeof(ClassWithParameterizedCtor))]
[InlineData(typeof(StructWithParameterizedCtor))]
[InlineData(typeof(ClassWithRequiredAndOptionalConstructorParameters))]
public void RespectRequiredConstructorParameters_false_ReportsCorrespondingPropertiesAsNotRequired(Type type)
{
var options = new JsonSerializerOptions { RespectRequiredConstructorParameters = false };
JsonTypeInfo typeInfo = Serializer.GetTypeInfo(type, options);

Assert.NotEmpty(typeInfo.Properties);
Assert.All(typeInfo.Properties, property =>
{
Assert.False(property.IsRequired);
});
}

[Theory]
[InlineData(typeof(ClassWithParameterizedCtor))]
[InlineData(typeof(StructWithParameterizedCtor))]
[InlineData(typeof(ClassWithRequiredAndOptionalConstructorParameters))]
public void RespectRequiredConstructorParameters_true_ReportsCorrespondingPropertiesAsRequired(Type type)
{
var options = new JsonSerializerOptions { RespectRequiredConstructorParameters = true };
JsonTypeInfo typeInfo = Serializer.GetTypeInfo(type, options);

Assert.NotEmpty(typeInfo.Properties);
Assert.All(typeInfo.Properties, property =>
{
bool isRequiredParam = property.AssociatedParameter is { HasDefaultValue: false, IsMemberInitializer: false };
Assert.Equal(isRequiredParam, property.IsRequired);
});
}

private static object? GetDefaultValue(ParameterInfo parameterInfo)
{
Type parameterType = parameterInfo.ParameterType;
Expand Down Expand Up @@ -506,6 +539,18 @@ public sealed class CustomConverter : JsonConverter<DerivedDictionaryWithCustomC
public override void Write(Utf8JsonWriter writer, DerivedDictionaryWithCustomConverter value, JsonSerializerOptions options) => throw new NotImplementedException();
}
}

internal class ClassWithRequiredAndOptionalConstructorParameters
{
[JsonConstructor]
public ClassWithRequiredAndOptionalConstructorParameters(string? x, string? y = null)
{
X = x;
Y = y;
}
public string? X { get; }
public string? Y { get; }
}
}

internal class WeatherForecastWithPOCOs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public partial class MetadataTests_SourceGen() : MetadataTests(new StringSeriali
[JsonSerializable(typeof(ClassWithMultipleConstructors))]
[JsonSerializable(typeof(DerivedClassWithShadowingProperties))]
[JsonSerializable(typeof(IDerivedInterface))]
[JsonSerializable(typeof(ClassWithRequiredAndOptionalConstructorParameters))]
partial class Context : JsonSerializerContext;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ public static void JsonSerializerOptions_EqualityComparer_ChangingAnySettingShou
yield return (GetProp(nameof(JsonSerializerOptions.UnknownTypeHandling)), JsonUnknownTypeHandling.JsonNode);
yield return (GetProp(nameof(JsonSerializerOptions.WriteIndented)), true);
yield return (GetProp(nameof(JsonSerializerOptions.RespectNullableAnnotations)), !JsonSerializerOptions.Default.RespectNullableAnnotations);
yield return (GetProp(nameof(JsonSerializerOptions.RespectRequiredConstructorParameters)), !JsonSerializerOptions.Default.RespectRequiredConstructorParameters);
yield return (GetProp(nameof(JsonSerializerOptions.IndentCharacter)), '\t');
yield return (GetProp(nameof(JsonSerializerOptions.IndentSize)), 1);
yield return (GetProp(nameof(JsonSerializerOptions.ReferenceHandler)), ReferenceHandler.Preserve);
Expand Down
Loading
Loading