dotnet / runtime

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

Provide option to ignore null collection elements, not just null property values. #79033

Open andref15 opened 1 year ago

andref15 commented 1 year ago

Is there an existing issue for this?

Describe the bug

I have an ASP.net core Web API, that uses the standard system text json serializer and I configured the reference handling as IgnoreCycles. The issue is that now when a Cycle is detected inside a collection, the resulting json looks like this:

{
  "MyCollection": [
    null
   ]
}

This json is sent to an angular application, that sends this exact object back and this null entry in the list causes some issues down the line. I wasn't able to find any way to fix this (I tried setting the DefaultIgnoreCondition, hoping that it might also work in collections)

Expected Behavior

IMO the serializer should skip the item entirely, if it's inside a collection, or at least provide some option to configure this behavior, sothat the resulting JSON looks like this:

{
  "MyCollection": []
}

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

7.0.100

Anything else?

ASP.net core 7 VS 17.4.0

brunolins16 commented 1 year ago

@andref15 thanks for contacting us.

that sends this exact object back and this null entry in the list causes some issues down the line

I am not 100% sure what issues you are talking about, maybe if you can share your repro I can provide a more specific suggestion, however, I believe you might have been receiving validation issues, eg.:

"errors": {
        "Items[0].Items": [
            "The Items field is required."
        ]
    }

If that is the case, it is happening probably because your model does not accept nullable type and change it might workaround the issue you are facing, eg.:

public class MyItem
{
-    public MyItem[] Items { get; set; }
+    public MyItem?[]? Items { get; set; }
}

IMO the serializer should skip the item entirely, if it's inside a collection, or at least provide some option to configure this behavior, sothat the resulting JSON looks like this:

I believe add a null item is the expected behavior, you might have an option to use ReferenceHandler.Preserve instead. The @dotnet/area-system-text-json could probably provide you more information.

ghost commented 1 year ago

Hi @andref15. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

andref15 commented 1 year ago

Hi, thank you for your reply. I don't have any issues deserializing the object. The Issue is that I have an Object with a Reference Cycle in a collection. When the API then serializes this object, the Collection in the json string contains null, which is what I would like to change. Sadly I cannot use ReferenceHandler.Preserve because this seems to cause some problems in the Angular HttpClient.

brunolins16 commented 1 year ago

he Issue is that I have an Object with a Reference Cycle in a collection. When the API then serializes this object, the Collection in the json string contains null, which is what I would like to change.

Probably @dotnet/area-system-text-json is the best group to help you.

eiriktsarpalis commented 1 year ago

This behavior is by design, cyclical reference types will be replaced with a null literal in the JSON -- which also holds for collection elements.

andref15 commented 1 year ago

@eiriktsarpalis Is there any way to change this? A null entry in a collection can be pretty annoying, for example the EntityFramework throws an exception in that case.

eiriktsarpalis commented 1 year ago

Is there any way to change this?

I don't believe there is -- generally speaking the library doesn't provide an option for skipping nulls (independent of reference handling).

A null entry in a collection can be pretty annoying

Is it just collections? What about null properties in an object?

for example the EntityFramework throws an exception in that case.

Why would throwing be preferable in this case? Is avoiding cycles in your object graph feasible in your use case?

andref15 commented 1 year ago

Is there any way to change this?

I don't believe there is -- generally speaking the library doesn't provide an option for skipping nulls (independent of reference handling).

Oh, too bad.

A null entry in a collection can be pretty annoying

Is it just collections? What about null properties in an object?

Yes, it's just about collections. Null for properties is fine.

for example the EntityFramework throws an exception in that case.

Why would throwing be preferable in this case? Is avoiding cycles in your object graph feasible in your use case?

Sorry for the confusion, I don't mean that throwing an exception would be preferable, I mean one of the cases where null entries in a collection are problematic is when using the EntityFramework because this causes an exception to be thrown.

Avoiding the object cycle might be an option for some of these cases, in others they are unavoidable, due to the way the API backend works...

eiriktsarpalis commented 1 year ago

A null entry in a collection can be pretty annoying

Is it just collections? What about null properties in an object?

Yes, it's just about collections. Null for properties is fine.

I mean that's kind of arbitrary. Assuming we implemented a null skipping logic (like Json.NET does) it would skip all nulls, not just in collection elements.

andref15 commented 1 year ago

Well skipping all nulls would be fine as well, I would have expected the DefaultIgnoreCondition WhenWritingNull to include the object cycle handling. However I don't see what's so arbitrary about skipping null entries in a collection. I would appreciate if you could explain this a little further

eiriktsarpalis commented 1 year ago

Well skipping all nulls would be fine as well, I would have expected the DefaultIgnoreCondition WhenWritingNull to include the object cycle handling.

The DefaultIgnoreCondition setting only configures null skipping semantics for properties. I did a bit of digging and it seems that the configuration is honored by cycles found in properties:

using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions 
{ 
    DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, 
    ReferenceHandler = ReferenceHandler.IgnoreCycles 
};

var value = new Poco();
value.NullProp = value;
value.Collection = new object?[] { value };

string json = JsonSerializer.Serialize(value, options);
Console.WriteLine(json); // {"Collection":[null]}

public class Poco
{
    public object? NullProp { get; set; }
    public object?[] Collection { get; set; }
}

IMHO that's also kind of arbitrary (but in the inverted sense) however IIRC the rationale for doing that was that stripping nulls from arrays result in a change in the collection count -- arguably resulting in loss of information not directly related to nullity of individual elements.

andref15 commented 1 year ago

Hm, the change of the collection count is true, however I'm also not sure how useful the count is when the items in the array are unusable. IMO it would still be nice to have the option to choose between either of these behaviors

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
### Is there an existing issue for this? - [X] I have searched the existing issues ### Describe the bug I have an ASP.net core Web API, that uses the standard system text json serializer and I configured the reference handling as IgnoreCycles. The issue is that now when a Cycle is detected inside a collection, the resulting json looks like this: { "MyCollection": [ null ] } This json is sent to an angular application, that sends this exact object back and this null entry in the list causes some issues down the line. I wasn't able to find any way to fix this (I tried setting the DefaultIgnoreCondition, hoping that it might also work in collections) ### Expected Behavior IMO the serializer should skip the item entirely, if it's inside a collection, or at least provide some option to configure this behavior, sothat the resulting JSON looks like this: { "MyCollection": [] } ### Steps To Reproduce _No response_ ### Exceptions (if any) _No response_ ### .NET Version 7.0.100 ### Anything else? ASP.net core 7 VS 17.4.0
Author: andref15
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 1 year ago

One possibility is extending JsonSerializerOptions.DefaultIgnoreCondition to collection elements, but that would be a possible breaking change. We need to compare what Json.NET is doing in this space, and if we do change it we would need to expose a ShouldSerialize predicate acting on collection elements in the contract model.

iongion commented 1 year ago

Can you not add a new ReferenceHandler.IgnoreCyclesSkipNull and make it act like Newtonsoft.Json ?

Other options

Keeping ReferenceHandler.Preserve but then - decyling client side libraries are lacking, one would expect some maturity here

Lacking a lot :(