RespectNullableAnnotations option should disallow setting non-nullable record parameter to null

[Dreamescaper]

Description

When RespectNullableAnnotations == true, and the non-nullable record parameter does not have corresponding value in the JSON, JsonSerializer still sets the value as null.

Please note that I don't want to set RespectRequiredConstructorParameters to true - I'm fine with JsonSerializer not "respecting" required parameters, and providing the default value - as long as this default value respects nullable constraints.

Reproduction Steps

record Request(string P1, string P2);

var input = """{"P1": "test"}""";
var ser = JsonSerializer.Deserialize<Request>(test, new JsonSerializerOptions(JsonSerializerDefaults.Web)
{
    RespectNullableAnnotations = true
};
// Exception expected, but I receive an object with P2 == null

Expected behavior

JsonSerializer shoudn't set record parameters to null if RespectNullableAnnotations == true.

Actual behavior

JsonSerializer sets record parameters to null if RespectNullableAnnotations == true, but the value is not present in json.

Regression?

No

Known Workarounds

I can set RespectRequiredConstructorParameters to true. However, that would also require me to add null default value to any nullable parameters, which I don't want to do:

record Request(string P1, string P2, string? Nullable = null);

Configuration

System.Text.Json 9.0.0-preview.6.24327.7

Other information

No response

[eiriktsarpalis]

This is by-design behavior. The nullability and requiredness of a parameter (or property setter) are treated as orthogonal concepts, which reflects the semantics of C# itself: whether a parameter is optional is independent of whether it is annotated as nullable. Similarly whether a property is marked required is independent of whether it is nullable.

Translating this to JSON terms, the serializer distinguishes between the case where the payload of a property is explicitly set to null and where the property is missing altogether. The RespectNullableAnnotations setting only regulates the former case, whereas RespectRequiredConstructorParameters controls the latter.

[Dreamescaper]

@eiriktsarpalis I completely agree that nullability and requiredness are orthogonal concepts, that's fine. However, currently JsonSerializer breaks nullability, even though I explicitely tell it not to.

When I have constructor with non-nullable parameter, and JsonSerializer puts null there (doesn't matter whether its from json or just a default value) - it breaks nullability, not requiredness.

[eiriktsarpalis]

The null you are seeing is a consequence of STJ passing in default for non-optional parameters that haven't been specified in the payload (when it should be throwing an exception). While this is arguably a bug, we couldn't outright change the behaviour because we found that too many users were accidentally depending on the behavior. Instead, this is addressed by the RespectRequiredConstructorParameters opt-in flag.

[Dreamescaper]

Again, requiredness aside - I'm non-accidentally fine with requiredness not being enforced in this case (as long as nullability is respected).

I'm explicitly telling JsonSerializer to respect nullable annotations. For the following constructor: record Request(string P1, string P2)

JsonSerializer invokes something like this deep inside: new Request("test", null)

Do you think nullability is respected here? I don't, therefore I consider this to be a bug.

[eiriktsarpalis]

The issue you are seeing is unrelated to nullability annotations that RespectNullableAnnotations controls. If P2 in your example was of type Guid then JsonSerializer will happily use new Request("test", Guid.Empty) which is a different manifestation of the same bug. The flag intended to correct this is RespectRequiredConstructorParameters.

[Dreamescaper]

@eiriktsarpalis No, it's different, because your example is about requiredness. There are no nullable annotations broken. Mine is about nullability specifically. JsonSerializer creates an instance, ignoring nullable annotations in constructor.

[Dreamescaper]

Btw, it is different from non-nullable non-required properties. In that case JsonSerializer would simply ignore that property (instead of setting them to null).

And in order 'null' to appear in this property, there would be some suppression: public string P2 {get; set;} = null!

So I treat serializer's behavior correct in this case - I've suppressed nullability warning explicitly, shame on me.

But with records - I have zero nullable warnings in my code, zero nullability suppressions, and I still get NRE when I try to access that property. That's not respecting nullability.

[eiriktsarpalis]

I understand the point you are trying to make, and how the default requiredness semantics of constructor deserialization will result in non-nullable annotations not being respected. Like I said, this is by design and is there because requiredness handling should remain consistent irrespective of whether the parameter is a struct, reference type or generic parameter.

My recommendation is to enable both RespectNullableAnnotations and RespectRequiredConstructorParameters, however you mentioned you don't want to turn on the latter because you want to model requiredness using nullability. That's not supported out of the box, however you might be able to do this using the contract customization APIs.

[eiriktsarpalis]

But with records - I have zero nullable warnings in my code, zero nullability suppressions, and I still get NRE when I try to access that property. That's not respecting nullability.

I would recommend reading the remarks section of the new property. C# nullability annotations are far from a complete system, and this is doubly true about serializers working with reflection metadata.

[Dreamescaper]

Like I said, this is by design and is there because requiredness handling should remain consistent irrespective of whether the parameter is a struct, reference type or generic parameter.

Well, I kindly ask to reconsider this design then. It shouldn't explicitly set nulls to non-nullable constructor parameters, when asked to respect nullable annotations.

Heck, it could even provide empty strings or new T() if no values in json are provided - I would consider that to be a strange behavior, but I would agree with your point that it's about requiredness VS nullability.

[Dreamescaper]

C# nullability annotations are far from a complete system, and this is doubly true about serializers working with reflection metadata.

Sure, that's understandable. But in this case there is an explicit setting to respect nullable annotations, and yet there is a design desision where it would still disrespect them.

I would recommend reading the remarks section of the new property.

There are no mentions that it would be ignored for constructors (without RespectRequiredConstructorParameters). Probably it requires an update.

[eiriktsarpalis]

It shouldn't explicitly set nulls to non-nullable constructor parameters, when asked to respect nullable annotations.

It could even provide empty strings or new T() if no values in json are provided - I would consider that to be a strange behavior, but I would agree with your point that it's about requiredness VS nullability.

I think the premise of stuffing random values when none is specified in the payload is problematic in general, doing so in order to avoid null at all costs might make things even worse -- an invalid null value tends to fail faster than an invalid empty string or guid.

The RespectNullabilityAnnotations flag specifically governs the serialization and deserialization of nullable types (with caveats per the documentation). In the case of the issue you are experiencing, there is no serialization or deserialization happening as such, it is just that the converter for parameterized constructors happens to have historically been using default(T) for unspecified parameters. I appreciate that this might look like a technicality from an outsider's perspective, especially when the deserialized value violates the nullability contract for the type. However I can't stress enough that this is a broader problem not specifically constrained to NRTs: the serializer randomly assigning 0 to an unspecified parameter that requires positive numbers is also a violation of the type's contract.

Again, I would strongly recommend enabling both RespectNullabilityAnnotations and RespectRequiredConstructorParameters for modern apps. If we were building STJ today that would have been its default and only behaviour. I know you said you disliked RespectRequiredConstructorParameters's handling of nullable parameters, but do give it a shot -- it makes a lot more sense once you start getting used to it.

[elgonzo]

In the case of the issue you are experiencing, there is no serialization or deserialization happening as such, it is just that the converter for parameterized constructors happens. [...] I appreciate that this might look like a technicality from an outsider's perspective

Considering that the deserializer creates the Request instance and the deserializer passes values to the intance's constructor, it's only rational to argue this being a part of the deserialization process that happens during deserialization. I get it is an explanation of what is technically going on here, but based on the reference API documentation for RespectNullabilityAnnotations that's not an understanding i believe many users will reach unless they just so happen to stumble over your comment (or similar comments/blog posts/Stackoverflow answers). The documentation not sufficiently explaining the behavior of the API's it strives to document shouldn't be hand-waved away by declaring it an "outsider perspective".

[eiriktsarpalis]

The documentation not sufficiently explaining the behavior of the API's it strives to document shouldn't be hand-waved away by declaring it an "outsider perspective".

What specifically would you like added to the documentation? Feel free to include suggestions in the relevant PR.

[elgonzo]

What specifically would you like added to the documentation?

I would like the documentation to accurately describe the behavior governed by the RespectNullabilityAnnotations property. I do not know exactly what the accurate behavior is, except for the the slice of behavior described here in this thread. Are there any other situations where RespectNullableAnnotations = true would not apply during the deserialization process? I don't know...

[eiriktsarpalis]

To rephrase then. What would do you see as missing given the conversation that was had here? Please forward any points in the PR directly as it's easier to work with that way.

[Dreamescaper]

However I can't stress enough that this is a broader problem not specifically constrained to NRTs: the serializer randomly assigning 0 to an unspecified parameter that requires positive numbers is also a violation of the type's contract.

requiredness handling should remain consistent irrespective of whether the parameter is a struct, reference type or generic parameter.

Personally, I would expect such difference in behavior (struct VS reference type), because that would be consistent with nullability in C# in general. Again, specifically constrained to NRTs.

The lines below do not cause nullability warnings:

Guid id = default;
int positiveInt = default;

but those ones do:

// Converting null literal or possible null value to non-nullable type.
string str = default;
int[] ints = default;