Skip to content

Should we add support to ignore cycles on serialization? #40099

Closed

Description

@jozkee

Background and Motivation

Even though we have covered the blocking issue of not being able to (de)serialize reference loops (#30820, #29900) by adding ReferenceHandler.Preserve to S.T.Json, there is many asks for adding an option equivalent to Json.NET's ReferenceLoopHandling.Ignore.

Motivations for doing this are:

  • ReferenceHandler.Preserve may be too cumbersome and increases payload size.
  • ReferenceHandler.Preserve creates JSON incomprehensible by serializers other than S.T.Json and Json.NET.

Proposed API

namespace System.Text.Json.Serialization
{
    public abstract partial class ReferenceHandler
    {   
        public static ReferenceHandler Preserve { get; }
+        public static ReferenceHandler IgnoreCycle { get; }
    }
}

Usage Examples

class Node
{
    public string Description { get; set; }
    public Node Next { get; set; }
}

void Test()
{
    var node = new Node { Description = "Node 1" };
    node.Next = node;
    
    var opts = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.IgnoreCycle };
    
    string json = JsonSerializer.Serialize(node, opts);
    Console.WriteLine(json); // Prints: {"Description":"Node 1"}. 
    // Note property "Next" being ignored due to cycle detection.

}

Alternative Designs

This new API is being added to ReferenceHandler class since this can be considered as an alternative to deal with references that is more isolated to the circularity problem during serialization.

Comparison with Newtonsoft.Json

void Test()
{
    var node = new Node { Description = "Node 1" };
    node.Next = node;
    
    var settings = new JsonSerializerSettings { ReferenceLoopHandling = ReferenceLoopHandling.Ignore };
    
    string json = JsonConvert.SerializeObject(node, settings);
    Console.WriteLine(json); // Prints: {"Description":"Node 1"}.
}

Comparison with existing ReferenceHandler.Preserve setting in S.T.Json

void Test()
{
    var node = new Node { Description = "Node 1" };
    node.Next = node;
    
    var opts = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve };
    
    string json = JsonSerializer.Serialize(node, opts);
    Console.WriteLine(json); // Prints: {"$id":"1","Description":"Node 1","Next":{"$ref":"1"}}. 
}

Risks

One downside is that users are uanble to implement their own ReferenceHandler and cannot make their own "Ignore Cycle" handler. The discrimination between preserve and ignore would occur with an internal flag.

Concerns of adding this feature (users must be aware of these problems when opting-in for it):

  • Silent loss of data given that the object where the loop is detected would be ignored from the JSON (see example: Should we add support to ignore cycles on serialization? #40099 (comment)).
  • Unable to round-trip data. the JSON may differ depending on the order of serialization (e.g: object properties and dictionary elements enumeration is non-deterministic).

Activity

  1. added this to the 5.0.0 milestone on Jul 29, 2020
  2. self-assigned this
    on Jul 29, 2020
  3. jozkee commented on Jul 29, 2020

    @jozkee
    MemberAuthor

    From #30820 (comment):

    I have to agree with many other concearning Ignore - and I'd like to try to add some arguments. Unfortunately video is not optimal for me, so I might have missed good arguments in the videos (but I've read through this issue/comments at least).

    My arguments are based on that Preserve is the only good alternative. My reasoning for that: [JsonIgnore] is not feasible whenever you have a two-way relationship and you sometimes need to fetch either type of object (and you want the related objects with it, but not the self-references). Except for that there doesn't seem to be a good workaround?

    • Like @diegogarber pointed out, it breaks backwards compatibility and can induce significant additional work (e.g. frontend wise)
    • It causes a significantly larger payload. The original issues first two examples (Ignore vs Preserve) is pretty clear (~twice the amount of lines). For Web API scenarios (which might be the most common scenario for serializing JSON), that's pretty horrendous (even if the added stuff is quite short).
    • The JSON structure is not obvious from a frontend perspective - which the Ignore structure is (imo). For scenarios with Ignore where you really need the references, you could easily identify that specific call (again, assuming a Web API) and do your ViewModel-layer instead. The Preserve structure on the other hand needs parsing - specifically turning lists (arrays) into collections (objects) will be quite confusing.

    To sum up: To break backwards compatibility, cause a significant larger payload, as well as breaking the structure by introducing a middle layer (as in Subordinates not being a list/array of values), should be reason enough to consider having Ignore.

    I believe the StackOverflow issue mentioned reflects the community's stand on this - the only reasonable way forward (unless for specific scenarios where you really need the C#/serialization performance) is to continue with Newtonsoft. To me, that should seen as a sign that this is not a reasonable way forward. It's been a long wait between 3.0 -> 5.0 (we're still waiting after all), and to still not having this resolved will definitely continue to stir up frustration.

  4. jozkee commented on Jul 29, 2020

    @jozkee
    MemberAuthor

    From #30820 (comment)

    The problem with Ignore is that you end-up losing data, this will probably cause an issue even more severe than having a large payload or having to do extra parsing in the front-end.

    e.g:

    static void Main()
    {
        var angela = new Employee { Name = "Angela" };
        var bob = new Employee { Name = "Bob", Manager = angela };
    
        angela.Subordinates = new List<Employee> { bob };
    
        var settings = new JsonSerializerSettings { ReferenceLoopHandling = ReferenceLoopHandling.Ignore };
        string json = JsonConvert.SerializeObject(bob, settings);
        Console.WriteLine(json);
        // {"Name":"Bob","Manager":{"Name":"Angela","Manager":null,"Subordinates":[]},"Subordinates":null}
    
        Employee bobCopy = JsonConvert.DeserializeObject<Employee>(json);
        Employee angelaCopy = bobCopy.Manager;
    
        Console.WriteLine("Number of subordinates for angela: " + angela.Subordinates.Count);
        // Number of subordinates for angela: 1
        Console.WriteLine("Number of subordinates for angelaCopy: " + angelaCopy.Subordinates.Count);
        // Number of subordinates for angelaCopy: 0
        Console.WriteLine(angelaCopy.Subordinates.Count == angela.Subordinates.Count);
        // False
    }

    @cjblomqvist How is that similar scenarios are not a concern for you or everyone else asking for Ignore?

  5. jozkee commented on Jul 29, 2020

    @jozkee
    MemberAuthor

    From #30820 (comment)

    @jozkee thanks for your reply!

    I believe a scenario which is probably quite common is that you have let's say 100 types, with various relationships between them. Some contian many, some not so many. Basically all are related to at least one other type one way or another, and you have a REST-like Web API to get them from a Web/Mobile application. To map out all relationships in each scenario and make sure you handle all self-referencing scenarios is quite cumbersome. So what happens is, the user is asking for a main type A, and then you ensure you also get all the related types that you believe the "user" might need, such as B, C, D, E, F, G (E, F, G might be sub-relationships). You do not filter this properly (i.e. you do not project out properties not needed) because of not wanting to "waste" time on it (as well as laziness). To map out this tree:

    A
    A -> B
    A -> C
    A -> D
    A -> B -> E
    A -> B -> E -> F
    A -> D -> G

    So far, all good. The problem is when you also have the following relationships:

    C -> A
    E -> A
    D -> A
    G -> A

    It's cumbersome to properly keep track of them and filter them out (e.g. through JsonIgnore - which also has the problem of filtering out the relationships in all scenarios, not only for this particular call for A). It would also be cumbersome to handle it using preserve due to the A) wasted space/size to transmit over the network (might be less of a concearn in above scenario since obviously we're not so picky with what we're including and not), and B) the data structure does not map in a fully obvious way to JSON, but with a quite new-JSON-library-in-.NET specific way.

    Then, to finally answer your question: The "user" (frontend consumer) simply does not care about the additional relationships [C, E, D, G] -> A - it only wants to know the original tree. The loss of data is irrelevant - because the relationship properties are not relevant.

    I'm not say that Ignore is perfect by any means, but it is darn convenient to simply add it to a project and then not having to think about it anymore. Something like Preserve could be very useful, but the current implementation is too cumbersome for most cases (with all the downsides listed above) - it doesn't fulfil that simple-and-convenient-albeit-not-100%-correct scenario. I do understand that there are reasons behind Preserve being the way it is, so I'm not saying the Preserve is wrong. What I'm saying is that Preserve is good for one thing (100% correctness while still handling self-referencing), while Ignore is good for another (simple/convenient, but not 100% correct in some cases).

    I do believe a lot of people in the community feel the same.

    Something in between Ignore and Preserve might be very useful, especially that doesn't break the assumed data structure (to avoid the frontend parsing/unexpected data structure for lists/arrays) while still not loosing data, but I understand that there are difficulties/complexities with this, so Ignore might be good enough to solve the relevant actual real world scenarios.

  6. removed
    untriagedNew issue has not been triaged by the area owner
    on Jul 29, 2020
  7. danmoseley commented on Jul 30, 2020

    @danmoseley
    Contributor

    @jozkee is this required to ship 5.0 (per milestone set here)?

  8. jozkee commented on Jul 30, 2020

    @jozkee
    MemberAuthor

    @danmosemsft I think it could disappoint many users if we don't ship this on 5.0.

  9. danmoseley commented on Jul 30, 2020

    @danmoseley
    Contributor

    @jozkee maybe, but master is a 6.0 branch in less than 3 weeks: if we add features, we're implicitly deciding to leave bugs un-fixed.

    That's a general statement - I don't have context on this issue or JSON in general so it would be a good discussion to have with @ericstj .

  10. modified the milestones: 5.0.0, 6.0.0 on Jul 30, 2020
  11. ericsampson commented on Aug 9, 2020

    @ericsampson

    @jozkee @ericstj FWIW when I heard that STJ would have loop handling in 5.0, I just assumed that would include something along the lines of Ignore. It would be disappointing to not have something along those lines in 5.0
    Also FWIW I don't love the Newtonsoft options. Like another commenter mention above, it would be great to have an in-between option that didn't suffer the data loss of Ignore; one option that seems great space-wise would be to only emit reference tags where they are needed, although that might require backtracking/rewriting?

  12. snow-jallen commented on Sep 30, 2020

    @snow-jallen

    What's the verdict? Is this going to ship in .net 5?

  13. 48 remaining items

  14. added
    Cost:SWork that requires one engineer up to 1 week
    on Jan 14, 2021
  15. added
    Priority:1Work that is critical for the release, but we could probably ship without
    and removed
    Priority:2Work that is important, but not critical for the release
    on Jan 20, 2021
  16. jozkee commented on Feb 1, 2021

    @jozkee
    MemberAuthor

    Sending back to api-ready-for-review in order to discuss a more suitable name for the API:

    1. IgnoreCycle: Original proposal.
    2. IgnoreCycles: From #46101 (comment) one could read Preserve as PreserveDuplicates, which would be plural too.
    3. BreakCycles: Given that we are breaking cycles by emitting the JSON null token and to be clear about the behavior diverging from Newtonsoft.
    4. EmitNullForCycles: From #40099 (comment).

  17. added
    api-ready-for-reviewAPI is ready for review, it is NOT ready for implementation
    and removed
    api-approvedAPI was approved in API review, it can be implemented
    on Feb 1, 2021
  18. ericsampson commented on Feb 1, 2021

    @ericsampson

    What about something like

    EmitNullForCycles

  19. jozkee commented on Feb 1, 2021

    @jozkee
    MemberAuthor

    @ericsampson sounds like a reasonable name that exactly describes what the API does.

  20. bartonjs commented on Feb 4, 2021

    @bartonjs
    Member

    Changed to plural via email:

    namespace System.Text.Json.Serialization
    {
        public partial class ReferenceHandler
        {   
            // Existing:
            // public static ReferenceHandler Preserve { get; }
            public static ReferenceHandler IgnoreCycles { get; }
        }
    }
  21. added
    api-approvedAPI was approved in API review, it can be implemented
    and removed
    api-ready-for-reviewAPI is ready for review, it is NOT ready for implementation
    api-suggestionEarly API idea and discussion, it is NOT ready for implementation
    on Feb 4, 2021
  22. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Feb 18, 2021
  23. ghost locked as resolved and limited conversation to collaborators on Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Cost:SWork that requires one engineer up to 1 weekPriority:1Work that is critical for the release, but we could probably ship withoutapi-approvedAPI was approved in API review, it can be implementedarea-System.Text.Jsonfeature-request

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions