Implement a concept of "Required" properties

[ryanbrandenburg]

AB#1130869

API proposal:

namespace System.Text.Json.Serialization
{
  [AttributeTargets.Field | AttributeTargets.Property, AllowMultiple=false)]
  public sealed class JsonRequiredAttribute : System.Text.Json.Serialization.JsonAttribute
  {
    public JsonRequiredAttribute() {}
  }
}

namespace System.Text.Json.Serialization.Metadata
{
  public abstract partial class JsonPropertyInfo
  {
    public bool IsRequired { get; set; }
  }
}

Current situation

There's no way to indicate that if an object doesn't have a particular property that serialization should fail.

For example:

public class Person
{
    [JsonRequired]
    public string Name { get; set; }
    public int Age { get; set; }
}

{
    "Age": 46
}

Trying to deserialize the above json as a Person will succeed, even though it's been indicated that the name attribute is required, and is missing.

Describe the solution you'd like

System.Text.Json should respect either System.ComponentModel.DataAnnotations.RequiredAttribute or a new attribute (likely extending System.Text.Json.JsonAttribute) and error when an attempt is made to serialize an object which doesn't have that property.

Describe alternatives you've considered

Using System.ComponentModel.DataAnnotations.RequiredAttribute might be troublesome because it could indicate Requirement in a different context. The alternative is that users implement their own "Required" functionality (and likely get it wrong or incomplete).

CC @pranavkm who I talked about this with.

[Clockwork-Muse]

.... if Name is a non-nullable reference type, wouldn't that indicate that it was required?

So:

public class Person
{
    public string Name { get; set; }
    public int Age { get; set; }
}

would fail the deserialize, but:

public class Person
{
    public string? Name { get; set; }
    public int Age { get; set; }
}

... would let it succeed.

[scalablecory]

If we implement Required we'd also want to implement all the other validation attributes. I'm not sure that System.Text.Json is the correct place to do this.

[pranavkm]

Having another look at https://github.com/dotnet/corefx/issues/37485, it looks like there's already another issue that is effectively asking for the same thing. Perhaps we should just close this and use the other issue to track work here?

[ahsonkhan]

I believe we should have the support for Required (similar to how we have the JsonIgnore attribute), but this will likely be something we would address in the future.

This issue talks more directly to the issue at hand, so I would prefer keeping this one as a feature request.

cc @steveharter

[ahsonkhan]

From @pranavkm (https://github.com/dotnet/corefx/issues/37485#issuecomment-501482970):

@JamesNK, did you mean JsonPropertyRequired

MVC users have often asked for a way to identify when a property isn't on the wire. While @steveharter's solutions are acceptable for the trivial cases, it really doesn't scale well if you want the behavior to apply in general. Plus, you're forced to manually perform the checks which seems cumbersome.

Could we consider introducing an attribute like Json.NET to enforce this behavior?

[steveharter]

Required was discussed but didn't meet the bar for 3.0 in lieu of other features. A workaround is to create a custom converter (pending feature) and override the Read method.

[pranavkm]

Required was discussed but didn't meet the bar for 3.0 in lieu of other features

Fair enough. I think we're ok with that for this release. We can keep this issue around to track the feature work for the Future.

[Tragetaschen]

Virtually all my model's properties have Newtonsoft Json's […, JsonProperty(Required = Required.Always)], which that input formatter honors. This is blocking my System.Text.Json adoption.

[markm77]

.... if Name is a non-nullable reference type, wouldn't that indicate that it was required?

So:

public class Person
{
    public string Name { get; set; }
    public int Age { get; set; }
}

would fail the deserialize, but:

public class Person
{
    public string? Name { get; set; }
    public int Age { get; set; }
}

... would let it succeed.

I agree such behaviour would be very desirable to ensure compiler checks with nullable reference types are not undermined at runtime by incorrect JSON (defeating the purpose of "nullable enable"). However this is perhaps best handled via the separate issue #1256.

A Required attribute would at least allow a reasonable manual workaround for this in the meantime though.

NB: I have sadly decided to roll back my conversion to System.Text.Json since as now using nullable reference types I don't want to go back to adding lots of null run-time checks.

[jebbinBjss]

@steveharter

Required was discussed but didn't meet the bar for 3.0 in lieu of other features. A workaround is to create a custom converter (pending feature) and override the Read method.

I see that this issue has been considered a nice to have, but I would like to point out that the workaround of creating a custom converter isn't an extensible one, as you would either need to create a custom converter for every class with required properties, or create a generic one that allows you to pass the names of all the required fields to the converter, while also handling the specifics of actually converting the values.

[Gambero81]

What about the progress of this issue?

It would be glad to have the JsonRequired attribute used in Newtonsoft.Json implemented in System.Text.Json

[ijagberg]

Also interested in a progress update on this issue. The workaround described here is not viable when you are working with many different classes.

[MythicManiac]

Just want to drop in and mention I've recently run into this, given that JsonZerializer.Deserialize returns invalid data if you deserialize a type that has explicitly non-nullable fields on it. If nullability is enabled (as it is by default for newer projects), the compiler will assume those non-nullable fields to always be non-null, yet the deserializer might return them with null values, thus breaking some static analysis guarantees.

This further propagates into different bugs that would normally be caught by static analysis during compile time (nullable value treated as if it was never null), including even warnings when you do add validation, such as the one below:

In other words, the type-generic variant of JsonSerializer.Deserialize method actively breaks the guarantees offered by static analysis. The only workaround I can think of is to always mark all fields used with json deserializer as nullable, and have a separate process for converting them to "validated" types.

Whether or not validators are something that should be included in the default Deserializer is a smaller problem overall, but if it can't guarantee fields that are typed as non-nullable to actually not be null, maybe it should disallow working with non-nullable types entirely given that it just doesn't work?

[Hoffs]

thus breaking some static analysis guarantees

@MythicManiac how do you get around the fact that you have non nullable type (string) without any initial value set? public string FileMd5 { get; set; } should be generating a warning at property level that your property can actually be null. As if youre breaking a guarantee at property level you can't expect the rest of the code to be sound.

[MythicManiac]

The same way as how you can declare non-nullable types that are filled in the constructor, meaning it's fine to declare such properties as long as they are populated appropriately during the creation of the object.

That is another good point however, the compiler as it is right now does not seem to issue an error if you attempt to initialize an object without all of it's non-nullable fields set to non-null values during initialization. How related that is to this issue I'm unsure, but it does surprise me how lax the compiler is with nullability checks like this.

[Hoffs]

The same way as how you can declare non-nullable types that are filled in the constructor, meaning it's fine to declare such properties as long as they are populated appropriately during the creation of the object.

I don't follow. If you have object initialization path that ensures that after creation of the instance these properties are not null then that is not a responsibility of static analysis to guess that a property that is said to be non null can be null. Just like setting a value of non nullable property to null just generates a warning on that part where it is setting the value. That won't affect the rest of the analysis because you are breaking the assumption at one point.

the compiler as it is right now does not seem to issue an error if you attempt to initialize an object without all of it's non-nullable fields set to non-null values during initialization

It does though?

public class ClassDomain
{
    [Required]
    public string Name { get; set; } = string.Empty;
    public string NonNullUninitialized { get; set; }
    public int Pass { get; set; }
    public int? Key { get; set; }
}

yields warning:

.../Class1.cs(10,19): warning CS8618: Non-nullable property 'NonNullUninitialized' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [/.../IntegrationTest.csproj]

[steveharter]

FWIW see also the C# proposal for required properties: dotnet/csharplang#3630. If the metadata is available for these properties, it could be leveraged by STJ during deserialization.

[MythicManiac]

@Hoffs I'm confident I have a project with the issue I presented, but I don't have it available here right now and given it's going a bit off topic, let's just leave it as IMO it'd be better to explicitly disallow non-nullable fields for the serializer, or alternatively make it respect and enforce the constraints they imply.

[jeffhandley]

This feature is being moved out of .NET 7 into the Future milestone. We will continue to follow the C# proposal for required properties as a possible integration.

[krwq]

I've internally added support for JsonPropertyInfo.IsRequired in #72937 since that was required in a sense (if we didn't do anything adding support for that in the future would be a breaking change) - I think we should be able to fix this issue with few lines of product code on top of that + testing. I'll add it back to 7.0 and soon update the API proposal. Note that I don't promise we will ship this in 7.0 - it will greatly depend on API review and since we're in RC it might get rejected and be moved to 8.0. (required keyword support and internal support will likely land in 7.0 though)

[terrajobst]

• Looks good as proposed

namespace System.Text.Json.Serialization
{
    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property,
                    AllowMultiple=false)]
    public sealed class JsonRequiredAttribute : JsonAttribute
    {
        public JsonRequiredAttribute();
    }
}

namespace System.Text.Json.Serialization.Metadata
{
    public abstract partial class JsonPropertyInfo
    {
        public bool IsRequired { get; set; }
    }
}