dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.72k forks source link

System.Text.Json: `JsonIgnoreCondition.WhenWritingNull` is not working #92383

Closed rcdailey closed 1 year ago

rcdailey commented 1 year ago

Description

When serializing types that have properties set to null, those properties still get serialized out with null values instead of being omitted.

Reproduction Steps

I don't have steps and I struggled to reproduce this in a simple MCVE, so I'm just going to link to my open source project. I created a branch named json-serializing-nullable-fields-issue that demonstrates the issue. You can look at the diff to see the new unit test I introduced that you can run to repro the issue.

See the test named DemoNullableNotWorking() (below):

[Test]
public void DemoNullableNotWorking()
{
    var fs = new MockFileSystem();

    var jsonFile = fs.CurrentDirectory().File("testdata.json");
    fs.AddSameFileFromEmbeddedResource(jsonFile, typeof(QualityProfileConfigPhaseTest));
    jsonFile.Refresh();

    using var data = jsonFile.OpenRead();

    var unboxed = JsonSerializer.Deserialize<QualityProfileDto>(data, GlobalJsonSerializerSettings.Services);
    var boxed = JsonSerializer.Serialize(unboxed, GlobalJsonSerializerSettings.Services);
}

If you inspect boxed in your debugger, you'll see the entire serialized JSON string. Look at the "items" array for an element like this:

    {
      "id": null,
      "name": null,
      "allowed": true,
      "quality": {
        "id": 7,
        "name": "Bluray-1080p",
        "source": "bluray",
        "resolution": 1080
      },
      "items": []
    },

Link to my repo and the relevant test file is below:

https://github.com/recyclarr/recyclarr/blob/json-serializing-nullable-fields-issue/src/tests/Recyclarr.Cli.Tests/Pipelines/QualityProfile/PipelinePhases/QualityProfileConfigPhaseTest.cs#L274

Expected behavior

Properties "id" and "name" should not be serialized because they are null.

Actual behavior

Null values are serialized when they shouldn't be.

Regression?

No response

Known Workarounds

No response

Configuration

.NET 7

Other information

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When serializing types that have properties set to `null`, those properties still get serialized out with `null` values instead of being omitted. ### Reproduction Steps I don't have steps and I struggled to reproduce this in a simple MCVE, so I'm just going to link to my open source project. I created a branch named `json-serializing-nullable-fields-issue` that demonstrates the issue. You can look at the diff to see the new unit test I introduced that you can run to repro the issue. See the test named `DemoNullableNotWorking()` (below): ```cs [Test] public void DemoNullableNotWorking() { var fs = new MockFileSystem(); var jsonFile = fs.CurrentDirectory().File("testdata.json"); fs.AddSameFileFromEmbeddedResource(jsonFile, typeof(QualityProfileConfigPhaseTest)); jsonFile.Refresh(); using var data = jsonFile.OpenRead(); var unboxed = JsonSerializer.Deserialize(data, GlobalJsonSerializerSettings.Services); var boxed = JsonSerializer.Serialize(unboxed, GlobalJsonSerializerSettings.Services); } ``` If you inspect `boxed` in your debugger, you'll see the entire serialized JSON string. Look at the `"items"` array for an element like this: ```json { "id": null, "name": null, "allowed": true, "quality": { "id": 7, "name": "Bluray-1080p", "source": "bluray", "resolution": 1080 }, "items": [] }, ``` Link to my repo and the relevant test file is below: https://github.com/recyclarr/recyclarr/blob/json-serializing-nullable-fields-issue/src/tests/Recyclarr.Cli.Tests/Pipelines/QualityProfile/PipelinePhases/QualityProfileConfigPhaseTest.cs#L274 ### Expected behavior Properties `"id"` and `"name"` should not be serialized because they are `null`. ### Actual behavior Null values are serialized when they shouldn't be. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration .NET 7 ### Other information _No response_
Author: rcdailey
Assignees: -
Labels: `area-System.Text.Json`
Milestone: -
pinkfloydx33 commented 1 year ago

Unless I'm missing something, I think your modifiers are causing the properties to be serialized.

They don't have the attribute, so HasAttribute would return false and you say to serialize iif it's false

rcdailey commented 1 year ago

Ok that's interesting. I thought the ShouldSerialize property supplemented the existing conditions like whether or not a property is null. So I am overriding that setting? How do I make ShouldSerialize only kick in after the other logic has been checked? I do not want to overwrite previous logic.

pinkfloydx33 commented 1 year ago

I might be wrong. I'm not sure of the exact rules... Just an observer and that's what popped out at me

rcdailey commented 1 year ago

I verified that you are right. I commented out the modifiers and it worked just fine. I don't fully understand how I'm supposed to update the Modifiers without breaking the rest of the logic. I migrated from Newtonsoft.Json recently and so far I'm finding System.Text.Json much more confusing and frustrating to use.

rcdailey commented 1 year ago

For now, I modified my logic to this and I think it works:

public static class JsonSerializationModifiers
{
    public static void IgnoreNoSerializeAttribute(JsonTypeInfo type)
    {
        var propertiesToRemove = type.Properties
            .Where(x => x.AttributeProvider?.IsDefined(typeof(JsonNoSerializeAttribute), false) ?? false)
            .ToList();

        foreach (var prop in propertiesToRemove)
        {
            prop.ShouldSerialize = (_, _) => false;
        }
    }
}

It's still really confusing, though. Maybe one of the devs can drop in to clarify the behavior and how this should be done.

eiriktsarpalis commented 1 year ago

@pinkfloydx33 is right, JsonIgnoreConditionAttribute declarations get mapped to the ShouldSerialize delegate, so overwriting that would result in these being lost. You could try chaining the old delegate with your custom logic like so:

var shouldSerialize = prop.ShouldSerialize;
prop.ShouldSerialize = (obj, value) => shouldSerialize?.Invoke(obj, value) != false && MyCustomLogic(obj, value);