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

JSON support required properties #73063

Merged
merged 4 commits into from
Aug 2, 2022
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 @@ -29,6 +29,7 @@ private sealed partial class Emitter
private const string DefaultContextBackingStaticVarName = "s_defaultContext";
internal const string GetConverterFromFactoryMethodName = "GetConverterFromFactory";
private const string InfoVarName = "info";
private const string PropertyInfoVarName = "propertyInfo";
internal const string JsonContextVarName = "jsonContext";
private const string NumberHandlingPropName = "NumberHandling";
private const string ObjectCreatorPropName = "ObjectCreator";
Expand Down Expand Up @@ -709,6 +710,7 @@ private static string GeneratePropMetadataInitFunc(TypeGenerationSpec typeGenera
string memberTypeCompilableName = memberTypeMetadata.TypeRef;

string infoVarName = $"{InfoVarName}{i}";
string propertyInfoVarName = $"{PropertyInfoVarName}{i}";

sb.Append($@"
{JsonPropertyInfoValuesTypeRef}<{memberTypeCompilableName}> {infoVarName} = new {JsonPropertyInfoValuesTypeRef}<{memberTypeCompilableName}>()
Expand All @@ -728,7 +730,16 @@ private static string GeneratePropMetadataInitFunc(TypeGenerationSpec typeGenera
JsonPropertyName = {jsonPropertyNameValue}
}};
{PropVarName}[{i}] = {JsonMetadataServicesTypeRef}.CreatePropertyInfo<{memberTypeCompilableName}>({OptionsLocalVariableName}, {infoVarName});
{JsonPropertyInfoTypeRef} {propertyInfoVarName} = {JsonMetadataServicesTypeRef}.CreatePropertyInfo<{memberTypeCompilableName}>({OptionsLocalVariableName}, {infoVarName});");

if (memberMetadata.IsRequired)
{
sb.Append($@"
krwq marked this conversation as resolved.
Show resolved Hide resolved
{propertyInfoVarName}.IsRequired = true;");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we wish to set other applicable properties directly on JsonPropertyInfo rather than the intermediate JsonPropertyInfoValues (e.g. IsExtensionData)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally speaking we could probably get rid of most of the usages of JsonMetadataServices but that might be a bit more work since we'd have to refactor also JsonTypeInfo part. While I could start doing it gradually it probably makes more sense to do it in one go but for that we need to be able to customize parameters which currently contract customization doesn't support. The reason this one is separate is because it doesn't have API in JsonMetadataServices but because we can avoid adding such API I went ahead and did it this way. I'll keep changes in this PR to minimum, we can revisit this once we get full parameters support

}

sb.Append($@"
{PropVarName}[{i}] = {propertyInfoVarName};
");
}

Expand Down
14 changes: 12 additions & 2 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ private sealed class Parser
private const string JsonNumberHandlingAttributeFullName = "System.Text.Json.Serialization.JsonNumberHandlingAttribute";
private const string JsonPropertyNameAttributeFullName = "System.Text.Json.Serialization.JsonPropertyNameAttribute";
private const string JsonPropertyOrderAttributeFullName = "System.Text.Json.Serialization.JsonPropertyOrderAttribute";
private const string JsonRequiredAttributeFullName = "System.Text.Json.Serialization.JsonRequiredAttribute";
private const string JsonSerializerContextFullName = "System.Text.Json.Serialization.JsonSerializerContext";
private const string JsonSourceGenerationOptionsAttributeFullName = "System.Text.Json.Serialization.JsonSourceGenerationOptionsAttribute";

Expand Down Expand Up @@ -1197,7 +1198,8 @@ private PropertyGenerationSpec GetPropertyGenerationSpec(
out string? converterInstantiationLogic,
out int order,
out bool hasFactoryConverter,
out bool isExtensionData);
out bool isExtensionData,
out bool isRequired);

ProcessMember(
memberInfo,
Expand Down Expand Up @@ -1229,6 +1231,7 @@ private PropertyGenerationSpec GetPropertyGenerationSpec(
IsProperty = memberInfo.MemberType == MemberTypes.Property,
IsPublic = isPublic,
IsVirtual = isVirtual,
IsRequired = isRequired,
JsonPropertyName = jsonPropertyName,
RuntimePropertyName = runtimePropertyName,
PropertyNameVarName = propertyNameVarName,
Expand Down Expand Up @@ -1275,7 +1278,8 @@ private void ProcessMemberCustomAttributes(
out string? converterInstantiationLogic,
out int order,
out bool hasFactoryConverter,
out bool isExtensionData)
out bool isExtensionData,
out bool isRequired)
{
hasJsonInclude = false;
jsonPropertyName = null;
Expand All @@ -1284,6 +1288,7 @@ private void ProcessMemberCustomAttributes(
converterInstantiationLogic = null;
order = 0;
isExtensionData = false;
isRequired = false;

bool foundDesignTimeCustomConverter = false;
hasFactoryConverter = false;
Expand Down Expand Up @@ -1350,6 +1355,11 @@ private void ProcessMemberCustomAttributes(
isExtensionData = true;
}
break;
case JsonRequiredAttributeFullName:
{
isRequired = true;
krwq marked this conversation as resolved.
Show resolved Hide resolved
}
break;
default:
break;
}
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Text.Json/gen/PropertyGenerationSpec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ internal sealed class PropertyGenerationSpec

public bool IsVirtual { get; init; }

/// <summary>
/// The property has JsonRequiredAttribute.
/// </summary>
public bool IsRequired { get; init; }

/// <summary>
/// The property name specified via JsonPropertyNameAttribute, if available.
/// </summary>
Expand Down
6 changes: 6 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 @@ -944,6 +944,11 @@ public sealed partial class JsonPropertyOrderAttribute : System.Text.Json.Serial
public JsonPropertyOrderAttribute(int order) { }
public int Order { get { throw null; } }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Field | System.AttributeTargets.Property, AllowMultiple = false)]
public sealed partial class JsonRequiredAttribute : System.Text.Json.Serialization.JsonAttribute
{
public JsonRequiredAttribute() { }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Class, AllowMultiple=true)]
public sealed partial class JsonSerializableAttribute : System.Text.Json.Serialization.JsonAttribute
{
Expand Down Expand Up @@ -1152,6 +1157,7 @@ internal JsonPropertyInfo() { }
public System.Text.Json.Serialization.JsonConverter? CustomConverter { get { throw null; } set { } }
public System.Func<object, object?>? Get { get { throw null; } set { } }
public bool IsExtensionData { get { throw null; } set { } }
public bool IsRequired { get { throw null; } set { } }
public string Name { get { throw null; } set { } }
public System.Text.Json.Serialization.JsonNumberHandling? NumberHandling { get { throw null; } set { } }
public System.Text.Json.JsonSerializerOptions Options { get { throw null; } }
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ The System.Text.Json library is built-in as part of the shared framework in .NET
<Compile Include="System\Text\Json\Serialization\Attributes\JsonNumberHandlingAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Attributes\JsonPolymorphicAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Attributes\JsonPropertyNameAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Attributes\JsonRequiredAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Attributes\JsonPropertyOrderAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\CastingConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ImmutableDictionaryOfTKeyTValueConverterWithReflection.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization
{
/// <summary>
/// Indicates that the annotated member must bind to a JSON property on deserialization.
/// </summary>
krwq marked this conversation as resolved.
Show resolved Hide resolved
/// <remarks>
/// <see langword="null"/> token in JSON will not trigger a validation error.
/// For contracts originating from <see cref="DefaultJsonTypeInfoResolver"/> or <see cref="JsonSerializerContext"/>,
/// this attribute will be mapped to <see cref="JsonPropertyInfo.IsRequired"/>.
/// </remarks>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
krwq marked this conversation as resolved.
Show resolved Hide resolved
public sealed class JsonRequiredAttribute : JsonAttribute
{
/// <summary>
/// Initializes a new instance of <see cref="JsonRequiredAttribute"/>.
/// </summary>
public JsonRequiredAttribute() { }
Copy link
Member

Choose a reason for hiding this comment

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

Can't the default constructor be removed here? Or is it required by ApiCompat?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not, I followed existing pattern with JsonInclude. We can put better xml doc comment I guess

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,21 @@ public bool IsExtensionData

private bool _isExtensionDataProperty;

internal bool IsRequired
/// <summary>
/// Specifies whether the current property is required for deserialization to be successful.
/// </summary>
/// <exception cref="InvalidOperationException">
/// The <see cref="JsonPropertyInfo"/> instance has been locked for further modification.
/// </exception>
/// <remarks>
/// For contracts originating from <see cref="DefaultJsonTypeInfoResolver"/> or <see cref="JsonSerializerContext"/>,
/// the value of this property will be mapped from <see cref="JsonRequiredAttribute"/> annotations.
///
/// For contracts using <see cref="DefaultJsonTypeInfoResolver"/>, properties using the <see langword="required"/> keyword
/// will also map to this setting, unless deserialization uses a SetsRequiredMembersAttribute on a constructor that populates all required properties.
/// <see langword="required"/> keyword is currently not supported in <see cref="JsonSerializerContext"/> contracts.
/// </remarks>
public bool IsRequired
{
get => _isRequired;
set
Expand Down Expand Up @@ -506,10 +520,8 @@ private bool NumberHandingIsApplicable()

private void DetermineIsRequired(MemberInfo memberInfo, bool shouldCheckForRequiredKeyword)
{
if (shouldCheckForRequiredKeyword && memberInfo.HasRequiredMemberAttribute())
{
IsRequired = true;
}
IsRequired = memberInfo.GetCustomAttribute<JsonRequiredAttribute>(inherit: false) != null
|| (shouldCheckForRequiredKeyword && memberInfo.HasRequiredMemberAttribute());
}

internal abstract bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf8JsonWriter writer);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -646,10 +646,9 @@ public static JsonSerializerOptions ResolveOptionsInstanceWithSmallBuffer(JsonSe

JsonSerializerOptions smallBufferCopy = new JsonSerializerOptions(options)
{
// Copy the resolver explicitly until https://github.com/dotnet/aspnetcore/issues/38720 is resolved.
TypeInfoResolver = options.TypeInfoResolver,
DefaultBufferSize = 1,
};

s_smallBufferMap.Add(options, smallBufferCopy);
return smallBufferCopy;
}
Expand Down
Loading