JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.74k stars 3.25k forks source link

I got "Self referencing loop detected with type" without loops... #2432

Open jakerdy opened 3 years ago

jakerdy commented 3 years ago

Source/destination types

using System;
using Newtonsoft.Json;

public class ISomeJsonConverter : JsonConverter<ISome>
 {
    public override void WriteJson(JsonWriter writer, ISome value, JsonSerializer serializer)
    {
        serializer.Serialize(writer, value);
    }

    public override ISome ReadJson(JsonReader reader, Type objectType, ISome existingValue, bool hasExistingValue,
        JsonSerializer serializer)
    {
        return null;
    }
}

[JsonConverter(typeof(ISomeJsonConverter))]
public interface ISome { }

public class Impl : ISome { }

public class Other
{
    public ISome SomeImpl = new Impl();
}

public static void Main()
{
    //Here 'loop detected' exception will be thrown...
    var sdf = JsonConvert.SerializeObject(new Other());
}

Expected behavior

It should just works, without loop exceptions.

Actual behavior

Well, i thought, this kind of construction should work fine, acording to docs (https://www.newtonsoft.com/json/help/html/JsonConverterAttributeClass.htm).

But it throws.

If i add [JsonConverter(typeof(ISomeJsonConverter))], to the field Other.SomeImpl, it will work as expected.

Steps to reproduce

Just run this code, in your IDE

P.S.

I got plenty of classes where interface based serialization used. And it will be nice if i could use JsonConverter attribute on Interface it self, cause it makes classes more clear, due to less attributes.

Hope you could fix it, or at least explain why this is "ok" behaviour.

Kind Regards.

jakerdy commented 3 years ago

I ve got this issue on 12.0.3 nuget version. Runtimes 3.1.100 and 5.0.100

elgonzo commented 3 years ago

This is not a bug in Json.NET, but a bug in your project.

Before i explain what happens on a technical level in your code there, i want to point out a major flaw in your ISomeJsonConverter: From what i can tell, the ISomeJsonConverter should be invoked when serializing an ISome instance. But all the converter does is calling the serializer again to serialize the object. There is no point in doing that. The JsonConverter attribute tells the serializer to not serialize an ISome instance the normal way as the serializer would do, but rather invoke the ISomeJsonConverter. But then, all the ISomeJsonConverter is to tell the serializer to serialize the ISome object instance the normal way anyway. If the converter just invokes the serializer with the given ISome object instance again, what is the point of having that converter?

But back to why your serialization code loops in one case, but not in the other...

In the first case - the JsonConverter attribute associated with the ISome interface itself. the serializer eventually has to serialize an object instance that implements ISome. So, it invokes the ISomeJsonConverter associated with this interface type. The ISomeJsonConverter just tells the serializer to serialize the object that implements ISome. So the serializer invokes the ISomeJsonConverter associated with this interface type. The ISomeJsonConverter then tells the serializer to serialize the object that implements ISome. So the serializer invokes the ISomeJsonConverter ... add nauseum... there is your loop. Because every time the serialize has to serialize an ISome instance, it sees and obeys the JsonConverter attribute associated with the type.

Removing the JsonConverter attribute from the interface type will break this loop. But then, why does the loop not happen if you have the JsonConverter attribute on the Other.SomeImpl property?

In the second case (JsonConverter attribute only on the Other.SomeImpl property), the serializer will eventually serialize the SomeImpl property of the Other object instance. Without looking at the type of the object assigned to the property, the serializer invokes the ISomeJsonConverter with the property value (an object implementing the ISome interface). The ISomeJsonConverter tells the serializer to serialize this object implementing the ISome interface. Note that the ISome interface itself, nor any member of the ISome interface is associated with the ISomeJsonConverter. Thus, when serializing the object implementing ISome, the serializer won't ever come across a JsonConverter attribute attached to the ISome interface or its members, so there is no situation that the converter will be called recursively when serializing the object implementing the ISome interface.

Lastly, an advice: stackoverflow.com is a much better place for asking questions about how to correctly use Json.NET. Not only will your question/problem be seen by many, many more eyes than here on the issue tracker, but you would probably also get an answer much, much quicker than here - if you are getting an answer here at all (keep in mind, this issue tracker here is for reporting and following up on bugs in Newtonsoft Json.NET itself, not about bugs/issues with code that merely uses this library incorrectly, so problems with user code might perhaps not get much attention here). Newtonsoft Json.Net's homepage also encourages people to ask on stackoverflow.com if they struggle with how to use Json.NET correctly...

jakerdy commented 3 years ago

Well, thanks for this explanation. I got your point of view, that this is not a bug , but a feature. But as a user of json.net library, and with out of clear knowlage of it's internal behaviours, i (and even my colleagues) couldn't tell that this particular case isn't a bug, or not some kind of unintended behaviour.

Again, from user perspective, "reference loop detected", is a wrong kinda error for this situation at the top level of serialization logic. May be for some internal error handling error it is ok, as one of the factors for resolving more descriptive user facing error, which should tell about bad/wrong library api usage. But for end user it isn't good enough. Because it missleading. I've spent lot of time debugging, and trying figure out where in my project i got loop references (but i was sure that there is no any, and even couldn't be). But after many googling attempts, trying many different thing i found that, particular scenario (from example), which makes this "loop". May be i should ask about it on SO, and got answer quicker, but common, this is looks like a bug. This is why i opened this issue. This behaviour should at least be documented, or mentioned somewhere. Like, "if you got ref loop, this might be, because of real loop refs, and also because we couldn't track some mad attribute usages", or "you shouldn't not mark ifaces with JsonConverter atrribute, because it may lead to wierd/strange behaviour at serialization time", but there is no any of that done anywhere in the docs/examples.

The best way of course will be throwing other type of exception, which is more clear for end user of json.net library, but i don't know if it possible at all. Anyway, i understood, that my usage of this attribute isn't compatible with current logic of lib, but it isn't obvious, and this, i think should be fixed. At least by me writing about this case in issues on github.

About example, and about "bug in my project". My project little bit more complex than this boiled down example. And, obviously i shouldn't copy paste 50kloc+ here, to demonstrate where some strange thing happened. This example just shows, how you could get, weird loop ref error. About no point of doing things like that. There actually is point of doing something like that. In my particular case, i got node graph, which stores nodes in a list of INode derived objects, and list of links between them. INode contains common properties, and special property called NodeKind, which used to identify node when deserializing graph from json. I won't use explicit type handling, because this graph may be used outside of .net runtimes, and in case of refactoring namespaces, i won't deal with problems when loading some dated graphs with old ns/type names. And i am pretty happy how default serializer doing it's job when serializing this graphs. This is why JsonConverter from example written exactly like that. It should serialize node in a default way, and deserialize them with custom logic, based on node_kind property which was omited in example cause it doesn't deal with that error.

I've use kinda work around in my project, specifying item converter through JsonProperty attribute for lists. It worked, but i must use it everywhere, to get INode deserialized correctly. Which isn't as nice as i if could just mark my ifaces/abstract classes with JsonConverter attrib. And forgot about the fact that i should always specify converter type here and there.

Hope that description was clear enough, and problem became more obvious. But in my opinion, this error should be handled somehow else then just throwing loop ref detected.

Json.net is a great project, i really love it, and i hope it could become even better and more user friendly, if this kind of errors will be handled/documented.

gwartnes commented 2 years ago

I feel like I should leave a comment here, because after googling for answers to the same exact issue, I came across this page.

I think what is missing from the OP's original bug report is the intent of the action they were trying to perform. In my case, my intent was to create a custom JsonConverter that performs a custom Deserialization on a particular type, and executes default Serialization on the same type. My first attempt had me trying OP's exact same method, because it feels like the right method in order to get whatever the default serialization behavior is: serializer.Serialize(writer, value);, with the same result - the self-referencing loop error. After wasting a couple of hours on the problem, I finally found the solution.

public class ISomeJsonConverter : JsonConverter<ISome>
{
    public override void WriteJson(JsonWriter writer, ISome value, JsonSerializer serializer)
    {
        throw new NotImplementedException();
    }

    public override bool CanWrite => false;

    public override ISome ReadJson(JsonReader reader, Type objectType, ISome existingValue, bool hasExistingValue, JsonSerializer serializer)
    {
        return null;
    }
}

Somehow, setting CanWrite to return false will actually pass serialization back to the default converter. To me, this is not intuitive at all - I figured that when trying to serialize it would throw the NotImplementedException (spoiler: it doesn't). And there is nothing anywhere that tells me that this would pass serialization control back to some other converter. It was basically just trial and error that got me here.

Ryochan7 commented 2 years ago

I ran into a similar issue recently. I was using a JsonConverterAttribute to mark a base class for items in a List to use a custom JsonConverter. My code could deserialize objects fine and that was all I was worried about for a while. Tried to test serializing objects back to JSON recently and then ran across the Self referencing loop issue. Tried many workarounds found on SO but nothing worked for both serializing and deserializing. Luckily I stumbled across this GitHub issue particularly the post by gwartnes.

The exception thrown in this case is not too clear and there aren't any docs, that I could find, that cover this use case.

GeXiaoguo commented 1 year ago

My first attempt had me trying OP's exact same method

I also spent half a day arriving at the same initial solution, the same problem and after googling, arriving at the same page here. This seems to be a common problem to customize only Deserialization but not Serialization and there is no intuitive way to do it. This is what I ended up with

public class AbstractUnionTypeConverter : JsonConverter
    {
        public override bool CanConvert(Type objectType) => 
            objectType.IsAbstract && typeof(UnionType).IsAssignableFrom(objectType);
        public override bool CanRead => true;
        public override object? ReadJson(JsonReader reader, Type objectType, object? existingValue, JsonSerializer serializer)
        {
            var result = Result<JObject, Exception>.Invoke(()=>JObject.Load(reader))
                .Select(jo => 
                {
                    jo.TryGetValue("Kind", StringComparison.OrdinalIgnoreCase, out var kindToken);
                    if (kindToken == null)
                    {
                        throw new JsonSerializationException($"{objectType} is an abstract type and needs a Kind property to specify the concrete type");
                    }
                    var kind = kindToken?.Value<string>();
                    var targetType = objectType.Assembly.GetType(kind);
                    if (targetType == null)
                    {
                        throw new JsonSerializationException($"can not find concrete type {kind}");
                    }
                    var obj = serializer.Deserialize(new JTokenReader(jo), targetType);
                    return obj;
                });
            var (obj, error) = result;
            return obj;
        }
        public override bool CanWrite => false;
        public override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer)
        {
            throw new NotImplementedException("This converter handles deserialization only. Code execution should not have reached here");
        }
    }

This is intended to serialize union types like below

public abstract record FeeDefinition(FeeNames FeeName) : UnionType
{
    public record RateFee(FeeNames FeeName, BasisPoint Rate) : FeeDefinition(FeeName);
    public record FixedFee(FeeNames FeeName, Money Amount) : FeeDefinition(FeeName);
    public record ArbitraryFee(FeeNames FeeName, Money Amount) : FeeDefinition(FeeName);
}

and UnionType is defined as

    public abstract record UnionType
    {
        public string Kind =>  GetType().FullName;
    }
FrankSzendzielarz commented 4 months ago

2024 and Bing AI / MS Copilot makes the same error! :-D I.e. not setting CanWrite to false to force the converter back to default. Agreed the Newtonsoft design is unintuitive and weird.