Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IgnoreNullValues does not work with non-nullable value types #30795

Closed
youcefnb opened this issue Sep 8, 2019 · 13 comments
Closed

IgnoreNullValues does not work with non-nullable value types #30795

youcefnb opened this issue Sep 8, 2019 · 13 comments
Labels
area-System.Text.Json bug json-functionality-doc Missing JSON specific functionality that needs documenting
Milestone

Comments

@youcefnb
Copy link

youcefnb commented Sep 8, 2019

Setting JsonSerializerOptions property of IgnoreNullValues to true does not work.

When attempting to deserialize a DateTime value that is null in the json string it throws the exception below. I thought setting the option to IgnoreNullValues should ignore the property, no?

converted to System.DateTime. Path: $.value[0].deletedDateTime
InvalidOperationException: Cannot get the value of a token type 'Null' as a string.

Thanks
Youcef

@ViktorHofer
Copy link
Member

cc @ahsonkhan, @steveharter

@ahsonkhan
Copy link
Member

ahsonkhan commented Sep 9, 2019

Here's a sample repro (looks like it can repro with any value type - not just DateTime, like int32, but works fine with reference types like string):

[Fact]
public static void IgnoreNullValuesBug()
{
    var options = new JsonSerializerOptions { IgnoreNullValues = true };
    Temp result = JsonSerializer.Deserialize<Temp>("{\"Foo\": null}", options);
    Console.WriteLine(JsonSerializer.Serialize(result, options));
}

public class Temp
{
    public DateTime Foo { get; set; }
}
System.Text.Json.Tests.Utf8JsonReaderTests.IgnoreNullValuesBug [FAIL]
        System.Text.Json.JsonException : The JSON value could not be converted to System.DateTime. Path: $.Foo | LineNumber: 0 | BytePositionInLine: 12.
        ---- System.InvalidOperationException : Cannot get the value of a token type 'Null' as a string.
        Stack Trace:
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\ThrowHelper.Serialization.cs(178,0): at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& readStack, Utf8JsonReader& reader, Exception ex)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.cs(132,0): at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(22,0): at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(74,0): at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(31,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
          E:\GitHub\Fork\corefx\src\System.Text.Json\tests\Utf8JsonReaderTests.cs(19,0): at System.Text.Json.Tests.Utf8JsonReaderTests.IgnoreNullValuesBug()
          ----- Inner Stack Trace -----
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Reader\Utf8JsonReader.TryGet.cs(820,0): at System.Text.Json.Utf8JsonReader.TryGetDateTime(DateTime& value)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Reader\Utf8JsonReader.TryGet.cs(396,0): at System.Text.Json.Utf8JsonReader.GetDateTime()
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\Converters\JsonValueConverterDateTime.cs(11,0): at System.Text.Json.Serialization.Converters.JsonConverterDateTime.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoNotNullable.cs(25,0): at System.Text.Json.JsonPropertyInfoNotNullable`4.OnRead(JsonTokenType tokenType, ReadStack& state, Utf8JsonReader& reader)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfo.cs(353,0): at System.Text.Json.JsonPropertyInfo.Read(JsonTokenType tokenType, ReadStack& state, Utf8JsonReader& reader)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.HandleNull.cs(62,0): at System.Text.Json.JsonSerializer.HandleNull(Utf8JsonReader& reader, ReadStack& state)
          E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.cs(117,0): at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)

Note that serialization (i.e. writing) works as expected (where null values are getting ignored). The issue is specific to deserialization (reading).

@steveharter steveharter changed the title IgnoreNullValues functionality broken IgnoreNullValues does not work with non-nullable value types Sep 9, 2019
@steveharter
Copy link
Member

Work-arounds:

  • Use a nullable type (e.g. Nullable<DateTime>)
  • Create a custom converter for that type that can handle null

@steveharter
Copy link
Member

There is a question here on correctness and validation. Since the value can never be null anyway, should IgnoreNullValues actually ignore "invalid" JSON? i.e. there can be reasons to want an exception in this case.

I compared against Json.NET (using NullValueHandling = NullValueHandling.Ignore) and it does ignore non-nullable values on deserialization, so perhaps compat is reason enough.

@steveharter
Copy link
Member

steveharter commented Sep 10, 2019

Some options assuming IgnoreNullValues=true.

  1. Keep existing semantics; the workaround is to use a nullable type such as DateTime? (or a custom converter if that is not desired). IgnoreNullValues=true means "ignore null only when null is a valid value".

  2. Keep existing semantics but add an attribute that can be applied per property -- such as a new JsonIgnoreNullValueAttribute that will always ignore nulls. This feature was originally discussed but cut. It is also possible to extend the existing JsonIgnoreAttribute.

  3. Make the change to ignore "null" on non-nullable structs. This could potentially break users that depend on the existing exception for validation reasons (since a non-nullable struct can't be null). IgnoreNullValues=true means "ignore null even when null is not a valid value".

  4. ...

@ahsonkhan
Copy link
Member

@rynowak, @JamesNK - what do you think? To me, option 3 makes the most sense. When I see the IgnoreNullValues option from an end-user perspective, I assume it would work for both serialization and deserialization (even on value types that can't be null).

Although, I can see an argument to be made about option 1, but that requires the user to change their object model or workaround it with their own converter (maybe that's OK).

Given this is effectively by-design, moving to 5.0 for now.

@steveharter
Copy link
Member

For the "ignore null" feature we discussed the following benefits:

  • Trimming the JSON during serialization, primarily for performance including subsequent deserialization since the JSON being deserialized is not littered with nulls.
  • Avoiding erasing default values during deserialization -- for example a constructor may initialize some values to a non-null values and may not want them overwritten with a null during deserialization. Also helps performance a bit since no setter needs to be called. We also discussed a "default value" feature that is a superset of this, but that was cut.

We never discussed whether this feature can be used to disable validation by removing null values from non-nullable structs.

@rynowak
Copy link
Member

rynowak commented Sep 16, 2019

Option 3 makes the most sense. I don't think there's anything terrible about the JSON.NET behaviour.

@steveharter steveharter removed their assignment Sep 17, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@layomia layomia self-assigned this Apr 9, 2020
@layomia
Copy link
Contributor

layomia commented Apr 17, 2020

Punting this to future, as we don't want to loosen this behavior before designing upcoming validation features including required properties and (de)serialization callbacks.

@layomia layomia modified the milestones: 5.0, Future Apr 17, 2020
@layomia layomia removed their assignment Apr 19, 2020
@chris-rogala
Copy link

Today I ran into an issue with this with the a DateTime property. Basically, my object is something like:

public class SomeDTO
{
    public DateTime SomeAwesomeDate { get; set; }
}

When the body looks like this:

{
  "someAwesomeDate": null
}

And the method looks like this:

public Task<IActionResult> Post([FromBody] SomeDTO model)

The model is always null. I try the same thing with a double, and it reacts the same way with a double. Keep in mind, this is for a more complex contract and even if all the other properties are set, it will set the model from the body to null. This is using .NET Core 3.1. Thoughts?

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 20, 2021

IgnoreNullValues has now been marked obsolete and is superseded by the JsonIgnoreCondition enum. However, JsonIgnoreCondition doesn't seem to specificy a case for ignoring null json tokens on read.

Would it make sense to try to fix an obsolete setting? If not, we could possibly evaluate extending JsonIgnoreCondition for the read null case. cc @layomia

We might instead want to consider a more extensible model for ignoring properties: #55781

@layomia
Copy link
Contributor

layomia commented Nov 22, 2021

JsonIgnoreCondition doesn't seem to specificy a case for ignoring null json tokens on read.

In API review we determined that there was no good scenario for ignoring null tokens on deserialization (superseding #30795 (comment)) Performance was considered i.e. saving some calls to setters, but deemed insufficient as justification for the feature. So we corrected the mistake of 3.0 semantics by not including the feature in JsonIgnoreCondition.

@eiriktsarpalis
Copy link
Member

Rather than extending functionality of an obsoleted property, we should look to providing more extensibility points so that users can have a greater deal of control over what properties should be ignored and when. This was expressed initially in #55781 but ultimately we'd be looking at addressing this via #63686.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json bug json-functionality-doc Missing JSON specific functionality that needs documenting
Projects
None yet
Development

No branches or pull requests

9 participants