Remove DisallowNull annotation in JsonConverter.Write() #35022

steveharter · 2020-04-15T20:39:06Z

There are no behavioral changes in this PR other than removing [DisallowNull] in JsonConverter<T>.Write which was added in #32186. This was removed since some converters handle null including:

The internal string converter.
Any converters for value types including the internal converter for Nullable<T>.
As an opt-in when Provide opt-in for custom converters to handle null #34439 is implemented.

Also removed todos added in #32474.

Fixes #32523 (actually this was fixed a while ago but reopened to delete the todo comments).

Notes:

The T value in Write(...) can't be made T? value since T can either be a class or a value type.
Normally converters are not written for nullable types since there is a built-in converter for Nullable<T> that handles nulls automatically and forwards non-null values to the underlying T converter (e.g. the int converter is called for an int? type when the value is not null).
Converters for nullable types are passed JsonTokenType.Null on Read() and a null value on Write(). Tests were added to verify this behavior. Note that in 3.0\3.1 the converter was not called for JsonTokenType.Null in Read() but was called correctly for Write().

stephentoub · 2020-04-15T20:44:17Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -409,6 +407,6 @@ internal void VerifyWrite(int originalDepth, Utf8JsonWriter writer)
        /// <param name="writer">The <see cref="Utf8JsonWriter"/> to write to.</param>
        /// <param name="value">The value to convert.</param>
        /// <param name="options">The <see cref="JsonSerializerOptions"/> being used.</param>
-        public abstract void Write(Utf8JsonWriter writer, [DisallowNull] T value, JsonSerializerOptions options);
+        public abstract void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options);


Do we have tests that pass in null successfully?

We have some for for cases where the type to convert is a struct - https://github.com/dotnet/runtime/blob/cc132d790af6a5971925ef476d9411b251e91cfe/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests.NullValueType.cs.

Thanks. It'd be good to have null tests for both value types and reference types.

Do we have tests that pass in null successfully?

Null strings use this path.

Is there any special guidance for using these attributes on abstract or non-sealed types\members? A derived class, for example, may want\need different behavior.

Is it OK to place [DisallowNull] on an abstract base class but then still allow null and use a nullable Type (like we do in the String converter).

layomia

Would be nice to see a diff in the tests; maybe one explicitly showing that null isn't passed to converters for reference types.

layomia · 2020-04-15T23:31:10Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -409,6 +407,6 @@ internal void VerifyWrite(int originalDepth, Utf8JsonWriter writer)
        /// <param name="writer">The <see cref="Utf8JsonWriter"/> to write to.</param>
        /// <param name="value">The value to convert.</param>
        /// <param name="options">The <see cref="JsonSerializerOptions"/> being used.</param>
-        public abstract void Write(Utf8JsonWriter writer, [DisallowNull] T value, JsonSerializerOptions options);
+        public abstract void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options);


We have some for for cases where the type to convert is a struct - https://github.com/dotnet/runtime/blob/cc132d790af6a5971925ef476d9411b251e91cfe/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests.NullValueType.cs.

ahsonkhan · 2020-04-16T02:42:29Z

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

@@ -490,7 +490,7 @@ public abstract partial class JsonConverter<T> : System.Text.Json.Serialization.
        protected internal JsonConverter() { }
        public override bool CanConvert(System.Type typeToConvert) { throw null; }
        public abstract T Read(ref System.Text.Json.Utf8JsonReader reader, System.Type typeToConvert, System.Text.Json.JsonSerializerOptions options);
-        public abstract void Write(System.Text.Json.Utf8JsonWriter writer, [System.Diagnostics.CodeAnalysis.DisallowNull] T value, System.Text.Json.JsonSerializerOptions options);
+        public abstract void Write(System.Text.Json.Utf8JsonWriter writer, T value, System.Text.Json.JsonSerializerOptions options);


Are we sure we want to go ahead with this change without the accompanying #34439?

See #32186 (comment)

Would existing converters that don't currently check for null input need to be fixed up to handle that?

Our internal converts pass null (String converter for one)

The StringConverter is explicitly annotated to allow nullable string. Do we have other examples?

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/StringConverter.cs

Line 7 in c27376e

internal sealed class StringConverter : JsonConverter<string?>

Are we sure we want to go ahead with this change without the accompanying #34439?

I think so. Even without that, the String converter gets passed null (any internal converter that uses a reference type). Does it make sense to have DisallowNull since we override the behavior internally?

Also, for converter composition where one converter calls another converter there may be a desire to pass null as the value -- especially if the target converter does something special with null.

Would existing converters that don't currently check for null input need to be fixed up to handle that?

There is no behavior change with the PR. When #34439 is implemented, null will not be passed to any converters (except internal) unless they opt-in through the new virtual method.

steveharter · 2020-04-21T18:44:35Z

Build failures unrelated (Microsoft.Extensions.DependencyInjection.Tests)

Allow null on JsonConverter.Write

cc132d7

steveharter added the area-System.Text.Json label Apr 15, 2020

steveharter requested a review from layomia April 15, 2020 20:39

steveharter requested review from ahsonkhan and jozkee as code owners April 15, 2020 20:39

steveharter self-assigned this Apr 15, 2020

stephentoub reviewed Apr 15, 2020

View reviewed changes

layomia approved these changes Apr 15, 2020

View reviewed changes

ahsonkhan reviewed Apr 16, 2020

View reviewed changes

Add custom converter tests to verify null handling

68399ea

steveharter merged commit d4b5390 into dotnet:master Apr 21, 2020

steveharter deleted the SerializerTodos branch April 21, 2020 18:44

layomia added this to the 5.0 milestone May 15, 2020

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DisallowNull annotation in JsonConverter.Write() #35022

Remove DisallowNull annotation in JsonConverter.Write() #35022

steveharter commented Apr 15, 2020 •

edited

Loading

stephentoub Apr 15, 2020 •

edited

Loading

layomia Apr 15, 2020

stephentoub Apr 16, 2020

steveharter Apr 17, 2020

steveharter Apr 17, 2020

layomia left a comment

layomia Apr 15, 2020

ahsonkhan Apr 16, 2020

steveharter Apr 17, 2020 •

edited

Loading

steveharter commented Apr 21, 2020

Remove DisallowNull annotation in JsonConverter.Write() #35022

Remove DisallowNull annotation in JsonConverter.Write() #35022

Conversation

steveharter commented Apr 15, 2020 • edited Loading

stephentoub Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

layomia Apr 15, 2020

Choose a reason for hiding this comment

stephentoub Apr 16, 2020

Choose a reason for hiding this comment

steveharter Apr 17, 2020

Choose a reason for hiding this comment

steveharter Apr 17, 2020

Choose a reason for hiding this comment

layomia left a comment

Choose a reason for hiding this comment

layomia Apr 15, 2020

Choose a reason for hiding this comment

ahsonkhan Apr 16, 2020

Choose a reason for hiding this comment

steveharter Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

steveharter commented Apr 21, 2020

steveharter commented Apr 15, 2020 •

edited

Loading

stephentoub Apr 15, 2020 •

edited

Loading

steveharter Apr 17, 2020 •

edited

Loading