Issue 104700: Clarify error message for collection property with no getter by julien98Qc · Pull Request #104861 · dotnet/runtime

julien98Qc · 2024-07-13T19:57:55Z

Added verification to distinguish between different error types.
Renamed existing test to more accurately reflect that it tests required properties without a setter.
Added a new test to ensure that a collection property without a getter throws an InvalidOperationException.

dotnet-policy-service · 2024-07-13T19:58:22Z

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

julien98Qc · 2024-07-13T19:59:24Z

@julien98Qc please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@dotnet-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"
Contributor License Agreement

@dotnet-policy-service agree

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs

src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs

eiriktsarpalis · 2024-07-26T10:02:22Z

src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs

+            Assert.Contains(nameof(ClassWithRequiredCollectionPropertyWithoutGetter.TestCollectionPropertyWithoutGetter), exception.Message);
+        }
+
+        public class ClassWithRequiredCollectionPropertyWithoutGetter


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.

eiriktsarpalis · 2024-07-26T10:11:32Z

src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs

+        public async Task RequiredCollectionPropertyWithoutGetterThrows()
+        {
+            string json = """{"Foo":"foo","Bar":"bar"}""";
+            InvalidOperationException exception = await Assert.ThrowsAsync<InvalidOperationException>(


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.

…etter - Added verification to distinguish between different error types. - Renamed existing test to more accurately reflect that it tests required properties without a setter. - Added a new test to ensure that a collection property without a getter throws an InvalidOperationException.

…edKeywordTests

- Logic back inline and adjusted to not expand failure.

eiriktsarpalis

Per https://github.com/dotnet/runtime/pull/104861/files#r1692841347 I don't believe clarifying the error message is the correct way to address the issue. We should instead make the impact scenario work similar to how it does when [JsonRequired] isn't being specified.

eiriktsarpalis · 2024-07-30T10:32:42Z

@julien98Qc Thank you for taking the time to contribute a fix!

julien98Qc · 2024-07-30T10:34:47Z

@eiriktsarpalis Thank you for reviewing :D

ghost added the area-System.Text.Json label Jul 13, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 13, 2024

julien98Qc marked this pull request as draft July 14, 2024 03:37

julien98Qc marked this pull request as ready for review July 14, 2024 12:27