Fix JSON required read-only properties used as parametrized ctor

[krwq]

Fixes: #78098

Fixes following scenario:

using System.Text.Json.Serialization;

const string input = """{"value":1}""";
var obj = System.Text.Json.JsonSerializer.Deserialize<RequiredAttributeCtor>(input);

public sealed class RequiredAttributeCtor
{
    private int _value;

    [JsonPropertyName("value")]
    [JsonRequired]
    public int Value
    {
        get => _value;
        // Works when you uncomment:
        // set { throw new NotImplementedException(); }
        // Without setter:
        // Unhandled exception. System.InvalidOperationException: JsonPropertyInfo 'value' defined in type 'JsonRequired+RequiredAttributeCtor' is marked required but does not specify a setter.
    }

    public RequiredAttributeCtor(int value)
    {
        _value = value;
    }
}

[ghost]

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

Issue Details

---

Fixes: #78098

Consider for servicing because it makes properties without setter not usable with new JsonRequired feature when they use parametrized constructor - it's not uncommon scenario. Note this issue is not relevant to required keyword because that combination is not possible in that context and that's likely why it was missed during testing.

Author:	krwq
Assignees:	-
Labels:	Servicing-consider, area-System.Text.Json
Milestone:	-

[eiriktsarpalis]

I think this check could be moved inside the InitializePropertyCache method to avoid iterating over the items yet again.

[krwq]

Unfortunately not in current state because we initialize constructor parameter cache after InitializePropertyCache is called which means we can't check for that correctly in here because we don't know which ones are initialized as part parametrized constructor. It would need likely quite a bit of refactoring to move this logic - probably makes more sense to do that refactoring as part of adding parametrized constructors to the public contract.

[eiriktsarpalis]

Then this check could be moved inside JsonPropertyInfo.Configure with all the other propertyInfo-level validations.

[krwq]

as I mentioned here: #78152 (comment) unfortunately that can't be move (at least not in the current state of the code), the ordering is following and each step depends on the previous:

• we initialize properties
• we initialize ctor parameters
• we check required properties used correctly

if I moved last step to first one we wouldn't know about which property is constructor parameter and be back to original bug

[eiriktsarpalis]

IsWritable sounds kind of ambiguous when compared with CanDeserialize. Given that it's only being called in one place I think you could simply inline CanDeserialize || IsConstructorParameter to make it more clear what is being checked?

[krwq]

I initially done it this way but I ended up splitting it because I eventually want to fix #77307 which will need similar property.

[eiriktsarpalis]

Do we really need this method? IsConstructorParameter has a private setter so we can assume that the assert invariant is preserved.

[krwq]

This is called from JsonTypeInfo.CreateConstructorParameter - I prefer to have it here rather than making IsConstructorParameter internal - this way I can add extra check and make sure it's not going to be misused in the future.

[eiriktsarpalis]

Per feedback in #78098 (comment), I don't think this meets the bar for servicing. I'm not entirely convinced that we should be supporting [JsonRequired] on property getters, since the C# required keyword is not supported on these properties either.

That being said, there clearly is a gap in functionality when it comes to specifying required constructor parameters (currently, STJ will pass default values to constructor parameters not bound to JSON properties). We should be more deliberate about how we end up addressing this and the final design should incorporate elements from #78151.

We also need to be careful about what the relationship between JsonPropertyInfo metadata and constructor parameter metadata should be. Even though current STJ requires a 1:1 relationship between properties and constructor parameters it is evident from customer reports that we would need to relax that association -- this necessitates a metadata model that lets constructor parameters being configured independently of each other.

[ericstj]

@krwq please do not add servicing-consider to PRs to main. We only add that to the servicing branch PRs and they must have a template.

[jeffhandley]

@krwq I'm putting this into Draft status since it's accumulated some merge conflicts and it might be easier to start with a fresh PR when we return to this.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[MartyIX]

Reopen? :)

[krwq]

I'll focus on this PR after #78556

[eiriktsarpalis]

@krwq should we close this? We can revisit constructor parameters overall in the next release.

[jeffhandley]

Yeah, let's go ahead and close it for revisiting in net9.