Fail null deserialization on non-nullable reference types

[bwanner]

When deserializing "null" for a non-nullable value type System.Text.Json throws an Exception. However, for reference types this is not the case even if the project has the "Nullable" feature enabled.

Expectation: When nullable is enabled for the project deserializing null into a non-nullable type should fail for both value and reference types.

Question: This could be worked around by writing a custom Converter that honors nullable during deserialization. Are there plans to expose a setting to control the deserialization behavior for non-nullable reference types?

Repro: Deserializing DateTime as null fails (by design per this issue 40922). Deserializing List as null succeeds.

    class Program
    {
        public class MyDataObject
        {
            public DateTime Dt { get; set; }
            public List<int> MyNonNullableList { get; set; } = new List<int>();
        }

        static void Main(string[] args)
        {
            string input1 = "{\"Dt\":null, \"MyNonNullableList\":[42,32]}";
            string invalidInput = "{\"Dt\":\"0001-01-01T00:00:00\",\"MyNonNullableList\":null}";
            // Throws System.Text.Json.JsonException: 'The JSON value could not be converted to System.DateTime. Path: $.Dt | LineNumber: 0 | BytePositionInLine: 10.'
            // var validConverted = JsonSerializer.Deserialize<MyDataObject>(input1);

            // Does not throw Exception, but MyNonNullableList is null.
            var invalidConverted = JsonSerializer.Deserialize<MyDataObject>(invalidInput);
            Console.ReadKey();
        }
    }

[markm77]

I fully agree. Also for consistency shouldn't the absence of a non-nullable also cause a fail?

(If we don't have something like this the whole advantage of nullable gets significantly negated as runtime types can deviate from compile-time checks. We're then back to manual fixes (run-time checks, custom converters or even attributes - all subject to human error and forgotten updates etc...).)

[ahsonkhan]

Expectation: When nullable is enabled for the project deserializing null into a non-nullable type should fail for both value and reference types.

I believe "nullable reference type" is a compile-time/static analysis feature. I don't think we can leverage it to affect runtime behavior of APIs like the JsonSerializer (or any other API in the framework). This is especially true because the nullable is opt-in, and the API runtime behavior should be the same regardless of whether the nullability analysis is enabled or not. Maybe I am mistaken, but I don't see how that's feasible.

The nullable annotation on the JsonSerializer.Deserialize method expresses this intent (return: MaybeNull), which is that it may return null even if the type is marked as non-nullable. That's because the JSON payload passed in could be the "null" literal and I don't think there is a runtime check we can leverage to detect that the user has a non-nullable reference type that they want to deserialize the unknown JSON string into.

runtime/src/libraries/System.Text.Json/ref/System.Text.Json.cs

Lines 439 to 440 in 3a844ad

	[return: System.Diagnostics.CodeAnalysis.MaybeNull]
	public static TValue Deserialize<TValue>(string json, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }

What enabling the nullability will do is the compiler will warn you if you try to deserialize into a non-nullable reference type like your class.

cc @safern, @stephentoub, @jcouv

Are there plans to expose a setting to control the deserialization behavior for non-nullable reference types?

Aside from implementing your own JsonConverter<T>, we could consider extending the behavior of [JsonIgnore] and the IgnoreNullValue option to apply at a property level, in which case, the null JSON value will be ignored and not override the initialized non-nullable reference type's value (in your case, list). We can consider this requirement as part of these two related feature: #30687 / #29861

cc @steveharter, @layomia

[safern]

Yes I also agree that the nullable annotation for Deserialize is correct as the payload may contain nulls no matter what the nullable reference type is when calling the deserialize method. Unfortunately even if IgnoreNullValue is honored to ignore null we don't have a way to express that via nullable reference types. We can't tell the compiler, if this property is true, the return value won't be null.

So that's why the compiler will warn you, this method my return null and you're using a non-nullable type. So what you got to do is declare your generic parameter and variable type as nullable.

MyDataObject? invalidConverted = JsonSerializer.Deserialize<MyDataObject?>(invalidInput);

[Suchiman]

@ahsonkhan EF Core is actually doing this, when you have NRTs enabled, they'll make columns NULL / NOT NULL based on how your model class is annotated, see https://docs.microsoft.com/en-us/ef/core/miscellaneous/nullable-reference-types

[markm77]

So @ahsonkhan if I understand correctly you are suggesting a property attribute could be used to ensure NULL is ignored when de-serialising a non-nullable reference type. But don't we want a way to trigger an exception to be equivalent to the non-nullable value type case?

And if we have an attribute or way to trigger an exception with NULL would that not also solve the compiler warning issue mentioned by @safern in that the compiler flow analysis can see that an incoming NULL will lead to an exception and hence the conversion to non-nullable is safe?

I think it would be really great to have a solution that is compatible with the compile-time checker if possible. 😀

[markm77]

A couple of additional thoughts meant fully in the interests of making C# and .NET Core the best development environment possible.

Unfortunately even if IgnoreNullValue is honored to ignore null we don't have a way to express that via nullable reference types. We can't tell the compiler, if this property is true, the return value won't be null.

If the NRT complier flow analysis is not detecting things correctly, is it worth raising this with the compiler team? For example if the complier flow analysis is not smart about attributes shouldn't this be fixed?

So what you got to do is declare your generic parameter and variable type as nullable.

I realise it may take work from multiple teams but shouldn't we aim for a better solution than disabling the complier flow analysis (System.Diagnostics.CodeAnalysis.MaybeNull) and effectively not supporting NRT?

[Trolldemorted]

Any chance a fix for this is coming with .net 5?

[layomia]

From @Eneuman in #46431:

Using ReadFromJsonAsync in a nullable value type enabled .Net 5 ASP project has som strange behaviours.

For example: the following code return a MyClass? nullable object:
var problemDetails = await response.Content.ReadFromJsonAsync<MyClass>();

This code returns a int (not nullable) (what happens if it is not a a numeric value in the response):
var result = await response.Content.ReadFromJsonAsync<int>();

But this code returns a nullable string:
var result = await response.Content.ReadFromJsonAsync<string>();
Shouldn't it return a none nullable string and throw if it is null?

[layomia]

From @rathga in #47178:

Summary

When deserializing Json to DTOs in MVC model binding, for non-nullable value type members null Json values are rejected, a nice error message is added to ModelState and a 400 error returned. However, for non-nullable reference types null values are let through which can then trip ArgumentNullExceptions in object constructors or property setters leading to a more cryptic 500 error rather than producing a 400 and a friendlier ModelState error.

An option could be added to (probably) JsonSerializerOptions so that it respects non-nullable reference types and throws a JsonException when encountering a null value, rather than binding the null value.

Motivation and goals

• Current work around is to always use nullable reference types for all reference type members in DTOs, and defer null checks until after the constructor in validation or business logic. The DTO is unable to defend its not-null invariants itself, leading to pollution of the codebase with null checks elsewhere. Effectively this makes using nullable reference types not usable/useful for controller action DTOs.
• An option could be added to (probably) JsonSerializerOptions so that it respects non-nullable reference types and throws a JsonException when encountering a null value, rather than binding the null value.

e.g. in Startup.cs

services.AddControllersWithViews()
    .AddJsonOptions(cfg => 
        cfg.JsonSerializerOptions.RespectNonNullableReferenceTypes = true
        );

• This may be more aptly posted in the runtime repo as presumably the changes need to be in System.Text.Json, but as it is impacting me in my controllers I thought it would make sense to get feedback here first.

Examples

This small console app demonstrates the issue:

namespace JsonNullable
{
    static class Program
    {
        public record NullableInt(int? someNumber);
        public record NonNullableInt(int someNumber);

        public record NullableString(string? SomeString);
        public record NonNullableString
        {
            public string SomeString { get; }
            public NonNullableString(string someString) =>
                SomeString = someString ?? throw new ArgumentNullException(nameof(someString));
        }

        static void Main(string[] args)
        {
            var nullableInt = new NullableInt(null);
            var jsonInt = JsonSerializer.Serialize(nullableInt);
            var nonNullableInt = JsonSerializer.Deserialize<NonNullableInt>(jsonInt); 
            // ^^ throws Cannot get the value of a token type 'Null' as a number.
            // Gets handled gracefully by MVC model binding

            var nullableString = new NullableString(null);
            var jsonString = JsonSerializer.Serialize(nullableString); 
            var nonNullableString = JsonSerializer.Deserialize<NonNullableString>(jsonString);
            // ^^ throws ArgumentNullException from constructor
            // Causes 500 exception in MVC, forcing the null check to be removed from the DTO and performed after binding
        }
    }
}

[Trolldemorted]

@layomia I have seen that this issue is also manifesting in #52227. Do you have the required information about nullability in your code generators at hand that would allow you to support this with compile-time source generation?

[JVimes]

I believe "nullable reference type" is a compile-time/static analysis feature. I don't think we can leverage it to affect runtime behavior...

Would this help? "Expose top-level nullability information from reflection" (#29723) is slated for .NET 6 Preview 7.

[APIWT]

I might be missing something, but to me it may be more helpful to provide a way to define a fallback value for these types of fields. Imagine you create a model that you want to persist the serialized version of like so:

public class Dog
{
  public string Name { get; set; }
}

You serialize it and persist it, then go on to add another fields in an update:

public class Dog
{
  public string Name { get; set; }

  public string FavoriteFood { get; set; }
}

If you attempt to deserialize the JSON that was generated in a prior iteration of the class, it seems a little harsh to get exceptions and could break backwards compatibility. Instead, wouldn't it make sense to just be able to fallback the FavoriteFood field to string.Empty (or even better, a custom value). I realize I could create a default value like this:

public class Dog
{
  public string Name { get; set; }

  public string FavoriteFood { get; set; } = string.Empty;
}

But at that point, any Dog that is constructed has this default rather than those that are deserialized with the field missing.

If there is a feature that allows me to do this already, please let me know! I really appreciate it.

[miroljub1995]

How about checking for System.Runtime.CompilerServices.NullableAttribute and System.Runtime.CompilerServices.NullableContextAttribute to see if reference property is defined as nullable or not?

Example:

class Test
{
    public string WithoutAttribute { get; set; }
    public string? WithAttribute { get; set; }
    public int PureValue { get; set; }
    public int? ValueWrappedInNullable { get; set; }
}

So those are 4 cases that needs to be considered like follows:

1. First one is reference type and property doesn't have NullableAttribute
2. Second one is reference type and property has NullableAttribute
3. Third one is value type
4. Forth one is value type wrapped as Nullable

I'm not seeing why this can not be implemented without any new attribute, interface, or similar stuff.
The only thing I would add to public api is some property in JsonSerializerOptions to opt on or off this behavior.

Let me know if I'm missing something.

[krwq]

This is currently possible to achieve with contract customization except for top level (we don't have info on nullability for top level - for top level you can do null check separately). Here is example implementation:

using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;

JsonSerializerOptions options = new()
{
    TypeInfoResolver = new DefaultJsonTypeInfoResolver()
    {
        Modifiers = { BlockNullForNonNullableReferenceTypesModifier }
    }
};

string json = """
    {
      "NonNullableString": null,
      "NullableString": null
    }
    """;

TestClass testClass = JsonSerializer.Deserialize<TestClass>(json, options); // JsonException: Null value not allowed for non-nullable property.


static void BlockNullForNonNullableReferenceTypesModifier(JsonTypeInfo jsonTypeInfo)
{
    if (jsonTypeInfo.Type.IsValueType)
        return;

    NullabilityInfoContext context = new();
    foreach (JsonPropertyInfo property in jsonTypeInfo.Properties)
    {
        Action<object, object?>? setter = property.Set;

        if (setter == null)
            continue;

        NullabilityInfo? nullabilityInfo = null;
        switch (property.AttributeProvider)
        {
            case FieldInfo fieldInfo:
                nullabilityInfo = context.Create(fieldInfo);
                break;
            case PropertyInfo propertyInfo:
                nullabilityInfo = context.Create(propertyInfo);
                break;
        }

        if (nullabilityInfo == null)
            continue;

        if (nullabilityInfo.WriteState == NullabilityState.NotNull)
        {
            property.Set = (obj, val) =>
            {
                if (val == null)
                    throw new JsonException("Null value not allowed for non-nullable property.");

                setter(obj, val);
            };
        }
    }
}

class TestClass
{
    public string NonNullableString { get; set; }
    public string? NullableString { get; set; }
}

there is extra work needed to cover all corner cases (array values etc) but this should hopefully cover most of the scenarios

[StepKie]

This has been open for years, and many related requests are also closed as duplicates, pointing to this issue.

We are also facing this issue. What is the actual problem here to provide a solution for this?

If nullable annottations are turned on, fail/warn (potentially based on a JsonSerializer setting/annotation), when null is received as the value of the attribute in Json.

Currently it only fails when the attribute is not present at all (when I mark the property as required).
Funnily enough, the deprecated IgnoreNullValues will work also for deserialization (as opposed to JsonIgnoreCondition.WhenWritingNull, which only handles the serialization direction).

[krwq]

@Hottemax biggest problem is that (de)serializers in general have virtually infinite number of knobs and expectations how people want it to work. We have significant number of open issues and requests and we try to make sure we look at them holistically rather than per single request/issue. We cannot just remove APIs once added so we need to be conservative in that choice. This issue has accumulated enough upvotes for it to be considered for next release but note that it doesn't guarantee we will do it. Soon we will look at all issues in 9.0 and Future milestone and make some initial choices but keep in mind that it's overall almost 200 issues (12 being already considered for 9.0). There is a huge difference in how much time it takes to write simple prototype code like one above vs actual code which ships - latter requires much more time for testing and convincing people on the right design. We need to pick which of these issues are currently most important for product as a whole (i.e. our main scenario is seamless interaction with ASP.NET Core which might be invisible for most of the users) but upvoted issues like this one are also a big factor for us when picking. Certainly having someone from community doing some prototyping would help here. I.e. extending code above to figure out all corner cases, some test cases etc would decrease amount of work for us and make it more likely it would ship next release.

[StepKie]

Thanks for the detailed provided context @krwq ! 👍 Replies like these are so valuable, so us library users know what is going on behind the scenes.

[eiriktsarpalis]

Here's a prototype showing how NRT metadata can be extracted in both reflection and source gen:

eiriktsarpalis/PolyType@e96ef20

One thing to note is that this will most likely require an opt-in switch -- it would otherwise be a breaking change:

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool EnforceNonNullableReferenceTypes { get; set; }
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public bool EnforceNonNullableReferenceTypes { get; set; }
}

[eiriktsarpalis]

One open question is whether the feature will be supported in the netfx/netstandard2.0 TFMs. Even though officially speaking NRTs are not supported in older TFMs, it still is possible to use them there. Supporting the feature would require OOBing the NullabilityInfoContext/NullabilityInfo APIs for these TFMs which to my knowledge hasn't happened yet.

@dotnet/area-system-reflection @stephentoub @ericstj thoughts?

[stephentoub]

https://github.com/orgs/dotnet/teams/area-system-reflection @stephentoub @ericstj thoughts?

FWIW, I don't think this is a desirable change. NRTs have never affected runtime behavior; in fact it's been a key aspect of the design that they're a compile-time-only assistance to provide warnings. Changing deserialization behavior based on these annotations goes against that.

[Methuselah96]

They do affect runtime behavior for ASP.NET Core model validation.

[Trolldemorted]

They also affect how Entity Framework creates columns - an NRT causes a column to be nullable.

This "key aspect" is so bad that not even Microsoft's products stick to it. Kotlin handles the "Billion Dollar Mistake" way better, despite interoperability with the Java ecosystem.

[eiriktsarpalis]

NRTs convey intent as such it seems reasonable to me that automatic serializers generate schemas honoring that intent.

[stephentoub]

NRTs convey intent as such it seems reasonable to me that automatic serializers generate schemas honoring that intent.

I don't believe it's any different from:

List<string> list =... ;
list.Add(null);

That will result in a compile-time warning if <Nullable>enabled</Nullable> is set, but it won't impact the runtime behavior and result in Add throwing an exception, nor should it.

[eiriktsarpalis]

I understand, but at the same time a type system is not the same thing as a serialization contract. In automatic serializers we use convention to map type system features to serialization contracts in a way that feels intuitive to the user. One good example is the required keyword, which is mapped from a C# type system concept to a similar (but not identical) feature in the serialization contract, and despite the fact that required isn't enforced at run time.

Mapping non-nullable annotations to a "require non-null" deserialization contract seems useful to me for a couple of reasons:

1. It provides a language-integrated way for declaring intent, as opposed to needing to decorate the property with yet another attribute.
2. The deserialized result honours the nullability annotation of its model, meaning that users don't need to take extra steps checking for nulls downstream.

I don't believe it's any different from:

I should clarify that the scope of this feature is limited to member and constructor parameter annotations. We can't add this to generic parameters/collection elements due to constraints in their runtime representation, at least in the reflection serializer.

[rathga]

NRTs convey intent as such it seems reasonable to me that automatic serializers generate schemas honoring that intent.

I don't believe it's any different from:

List<string> list =... ;
list.Add(null);

That will result in a compile-time warning if <Nullable>enabled</Nullable> is set, but it won't impact the runtime behavior and result in Add throwing an exception, nor should it.

I think it is different because one of the most useful purposes of both reflection and source generators is to save the developer from writing a whole bunch of 'handwritten' code. It is from this perspective this feature request is thought about I believe.

Taking your example above, your point is (I think) that the compiler warning helps the author remember to insert a null check. In code generation, the 'author' is runtime code using reflection or a compile-time source generator. This automated author should have the benefit of the same information that a human author would. How else can it produce code of similar value to 'handwritten' code? The <nullable>enabled</nullable> is therefore analogous to a serializer flag.

[stephentoub]

I understand, but at the same time a type system is not the same thing as a serialization contract

Neither C# nullable annotations nor the C# required keyword are part of the runtime type system. Activator.CreateInstance doesn't know about required, for example, whereas the C# level new constraint does.

The deserialized result honours the nullability annotation of its model, meaning that users don't need to take extra steps checking for nulls downstream.

NRT was bolted on 20 years after C# and NET were introduced. The annotations provide a guide, and are most useful in published APIs coveying intent, but they are fallible and cannot be trusted 100% and users do need to take extra steps downstream. Augmenting my previous example:

string[] values = new string[42];
List<string> list = new List<string>();
list.Add(values[0]);

No compiler warnings, no exceptions, yet the list contains null, and a consumer of the list needs to be able to deal with that.

I should clarify that the scope of this feature is limited to member and constructor parameter annotations. We can't add this to generic parameters/collection elements

Which means some would be respected and some wouldn't, making the story even more convoluted.

The enabled is therefore analogous to a serializer flag.

But it's non-local. Project-level settings (like for checked/unchecked) that impact code semantics are generally considered now to have been a mistake, because the code you write has different meanings based on the project in which it's used. Nullable as a project level setting factored that in and was intended to impact diagnostics but not runtime semantics.

The guidance for developers enabling nullable is to iterate until their code compiles and then they're done: that breaks down here, as enabling the project setting will change the meaning of existing use of System.Text.Json, such that just successfully compiling is no longer enough.

I could be wrong, but I suspect enabling this by default would be a massive breaking change.

One good example is the required keyword, which is mapped from a C# type system concept to a similar (but not identical) feature in the serialization contract, and despite the fact that required isn't enforced at run time.

I had no problem with S.T.J respecting required because its use in a type definition is unambiguous. But for NRT, a type "string" in a type definition is ambiguous and could be either nullable or non-nullable depending on project settings, such that its meaning changes when the project setting is set. That meaning change was intended to be compile-time only... this makes it a run-time break.

[eiriktsarpalis]

The proposal is to have this as an opt-in setting. Otherwise it would be a breaking change as you're pointing out.

[stephentoub]

The proposal is to have this as an opt-in setting. Otherwise it would be a breaking change as you're pointing out.

Which comment has the actual proposal?

[eiriktsarpalis]

Far from final, but here's a sketch: #1256 (comment). Will also need switches on the contract model for the source generator to be able to access.

[stephentoub]

Far from final, but here's a sketch: #1256 (comment). Will also need switches on the contract model for the source generator to be able to access.

Thanks. If it's opt-in, my primary concerns are alleviated.

Supporting the feature would require OOBing the NullabilityInfoContext/NullabilityInfo APIs for these TFMs which to my knowledge hasn't happened yet.

Those APIs are just using reflection to get at the relevant information. If those APIs aren't exposed publicly downlevel, S.T.J could still achieve the same thing using reflection, even by building in a copy of those implementations as internal (and with whatever tweaks might be needed to work downlevel).

[eiriktsarpalis]

Updated API proposal following up from #1256 (comment):

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool ResolveNullableReferenceTypes { get; set; }
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public bool ResolveNullableReferenceTypes { get; set; }
}

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonPropertyInfo
{
    // Fail serialization if the getter returns null. 
    // Defaults to true if the global flag has been enabled 
    // and the property is annotated as non-nullable 
    // (e.g. via type annotation or `NotNull` or `MaybeNull` attributes)
    public bool AllowNullWrites { get; set; }

    // Fail deserialization if the setter is passed a null.
    // Defaults to true if the global flag has been enabled
    // and the property is annotated as non-nullable
    // (e.g. via type annotation or `DisallowNull` or `AllowNull` attributes)
    public bool AllowNullReads { get; set; }
}

The two added properties on JsonPropertyInfo are of interest because they can be consulted when building a JSON schema for the type.

This proposal dovetails with #100075, so it might be conceivable that could unify both behaviours under a single feature switch. cc @stephentoub

[eiriktsarpalis]

I created a proposal in #100144 which establishes scope for the feature. Closing in favor of that.