Skip to content

Issue 104700: Clarify error message for collection property with no getter #104861

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.

Sign up for GitHub

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

Closed
julien98Qc wants to merge 3 commits into from
Closed

Issue 104700: Clarify error message for collection property with no getter #104861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82da637
Issue 104700: Clarify error message for collection property with no g…
julien98Qc Jul 13, 2024
165b874
Issue 104700: Added JsonSerializeAttribute for the new test in Requir…
julien98Qc Jul 14, 2024
bc99c11
Issue 104700: Adjusted logic
julien98Qc Jul 28, 2024
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
3 changes: 3 additions & 0 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,9 @@
<data name="JsonSchemaExporter_DepthTooLarge" xml:space="preserve">
<value>The depth of the generated JSON schema exceeds the JsonSerializerOptions.MaxDepth setting.</value>
</data>
<data name="JsonPropertyRequiredAndCollectionPropertyMissingGetter" xml:space="preserve">
<value>JsonPropertyInfo '{0}' defined in type '{1}' is marked required but is a collection property and does not specify a getter.</value>
</data>
<!-- System.Collections polyfills -->
<data name="Arg_WrongType" xml:space="preserve">
<value>The value '{0}' is not of type '{1}' and cannot be used in this generic collection.</value>
Expand Down
5 changes: 5 additions & 0 deletions ...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,11 @@ internal void Configure()
!(AssociatedParameter?.IsRequiredParameter is true &&
Options.RespectRequiredConstructorParameters))
{
if ((EffectiveConverter.ConverterStrategy & (ConverterStrategy.Enumerable | ConverterStrategy.Dictionary)) != 0 &&
Set != null && !_isUserSpecifiedSetter)
{
ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndCollectionPropertyMissingGetter(this);
}
ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndNotDeserializable(this);
}

Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,12 @@ public static void ThrowInvalidOperationException_SerializerPropertyNameNull(Jso
throw new InvalidOperationException(SR.Format(SR.SerializerPropertyNameNull, jsonPropertyInfo.DeclaringType, jsonPropertyInfo.MemberName));
}

[DoesNotReturn]
public static void ThrowInvalidOperationException_JsonPropertyRequiredAndCollectionPropertyMissingGetter(JsonPropertyInfo jsonPropertyInfo)
{
throw new InvalidOperationException(SR.Format(SR.JsonPropertyRequiredAndCollectionPropertyMissingGetter, jsonPropertyInfo.Name, jsonPropertyInfo.DeclaringType));
}

[DoesNotReturn]
public static void ThrowInvalidOperationException_JsonPropertyRequiredAndNotDeserializable(JsonPropertyInfo jsonPropertyInfo)
{
Expand Down
17 changes: 17 additions & 0 deletions src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,23 @@ public class ClassWithRequiredExtensionDataProperty
public required Dictionary<string, JsonElement>? TestExtensionData { get; set; }
}

[Fact]
public async Task RequiredCollectionPropertyWithoutGetterThrows()
{
string json = """{"Foo":"foo","Bar":"bar"}""";
InvalidOperationException exception = await Assert.ThrowsAsync<InvalidOperationException>(
Copy link
Member

@eiriktsarpalis eiriktsarpalis Jul 26, 2024

Choose a reason for hiding this comment

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

I don't think this is the correct way to address the problem. A fix should instead try to make the scenario work as expected. If you remove JsonRequired from the repro in #104700 then deserialization works as expected and this should too.

async () => await Serializer.DeserializeWrapper<ClassWithRequiredCollectionPropertyWithoutGetter>(json));
Assert.Contains(nameof(ClassWithRequiredCollectionPropertyWithoutGetter.TestCollectionPropertyWithoutGetter), exception.Message);
}

public class ClassWithRequiredCollectionPropertyWithoutGetter
Copy link
Member

@eiriktsarpalis eiriktsarpalis Jul 26, 2024

Choose a reason for hiding this comment

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

Just to make sure we're not regressing things here, could you also add a test for class like so:

class ClassWithCollectionCtorParam(Dictionary<string, JsonElement> value)
{
    public Dictionary<string, JsonElement> Value { get; } = value;
}

And then try to deserialize it using an options instance with RespectRequiredConstructorParameters set to true.

{
public required Dictionary<string, JsonElement>? TestCollectionPropertyWithoutGetter
{
set => TestCollectionPropertyWithoutGetter = value;
}
}

[Fact]
public async Task RequiredKeywordAndJsonRequiredCustomAttributeWorkCorrectlyTogether()
{
Expand Down
1 change: 1 addition & 0 deletions ....Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/RequiredKeywordTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public RequiredKeywordTests_SourceGen()
[JsonSerializable(typeof(ClassWithInitOnlyRequiredProperty))]
[JsonSerializable(typeof(ClassWithRequiredField))]
[JsonSerializable(typeof(ClassWithRequiredExtensionDataProperty))]
[JsonSerializable(typeof(ClassWithRequiredCollectionPropertyWithoutGetter))]
[JsonSerializable(typeof(ClassWithRequiredKeywordAndJsonRequiredCustomAttribute))]
[JsonSerializable(typeof(ClassWithCustomRequiredPropertyName))]
[JsonSerializable(typeof(DerivedClassWithRequiredInitOnlyProperty))]
Expand Down