2

When serializing the object below, Json.Net detects a self-referencing loop and throws an exception.

The class has two important features:

  • it has a self-referential property, Child
  • it overrides Equals() and GetHashCode()
public class Foo
{
    public int Value { get; set; }

    public Foo Child { get; set; }

    public override bool Equals(object obj) => (obj as Foo).Value == this.Value;

    public override int GetHashCode() => this.Value.GetHashCode();
}

...

var foo = new Foo { Value = 42, Child = new Foo { Value = 42 } };
JsonConvert.SerializeObject(foo); // Throws JsonSerializationException

It appears that Json.Net uses the override of Equals() to detect the reference loop (confirmed by debugging). But there is no loop here.

Why doesn't it use reference equality to check for a reference loop?

I found a test demonstrating that one can supply a different EqualityComparer which does use reference equality, but I'm interested to know why that isn't the default behaviour.

Tim S
  • 689
  • 6
  • 16
  • It does. By default `Object.Equals(...)` checks for reference equality for referential types. You have overridden it to not do this. It thinks there's a loop because your equality test checks for value equality based on a single property. Why is this surprising? –  Oct 25 '17 at 15:44
  • 2
    Hi @Amy, I would say that the point of overriding `Equals()` is to check for *logical* equality. My suggestion here is that, for the purpose of detecting a *reference* loop, Json.Net should not care about logical equality, only reference equality. – Tim S Oct 25 '17 at 15:50
  • I was answering your question. You asked why it doesn't. It does. You explicitly told it not to. Don't expect referential equality when you specifically check for value equality. –  Oct 25 '17 at 15:50
  • 1
    You should *never* use a mutable value in `GetHashCode()`. – itsme86 Oct 25 '17 at 15:53
  • Hi @itsme86, thanks for the advice. I will update my production version of the `Foo` class accordingly ;) – Tim S Oct 25 '17 at 15:55
  • 1
    To expand on @itsme86's point, the documentation for `Object.Equals` says: "You should not override Equals on a mutable reference type. This is because overriding Equals requires that you also override the GetHashCode method, as discussed in the previous section. This means that the hash code of an instance of a mutable reference type can change during its lifetime, which can cause the object to be lost in a hash table." –  Oct 25 '17 at 16:00
  • 1
    @Amy I think the OP is asking why it uses `Equals` instead of using `object.ReferenceEquals`. As in it should not blindly assume that `Equals` will do reference equality. – juharr Oct 25 '17 at 16:08
  • 4
    This question feels primarily opinion-based to me. Anyway Newtonsoft answered your question in [Object reference equality should be used when checking for circular references #401](https://github.com/JamesNK/Newtonsoft.Json/issues/401#issuecomment-77711185): *I prefer the current behavior which lets devs customize logic by overriding Equals. Besides, this is a big breaking change.* – dbc Oct 25 '17 at 16:10
  • @juharr by default `Object.Equals` calls `Object.ReferenceEquals` for referential types. For value types, `Object.Equals` checks for value equality. Calling `Object.ReferenceEquals` directly when value types are involved will result in boxing. –  Oct 25 '17 at 16:21
  • Hi @dbc, thank you for addressing the question and linking to the GitHub issue, that is exactly the kind of discussion I failed to find myself. If you can be bothered to post that as an answer, I'll accept it. – Tim S Oct 25 '17 at 16:23
  • @Amy I know. I'm saying the OP might wonder why the Json.Net code is using `Equals` instead of explicitly calling `ReferenceEquals` is all. – juharr Oct 25 '17 at 16:23
  • This question is spot on. When detecting _reference loops_, only _reference equality_ matters. Allowing users to make the comparison differently introduces nothing except potential trouble. Custom equality could fail to detect the loop. It's also very annoying when objects happen to consciously implement `Equals()` by throwing a `NotSupportedException`. – Timo Feb 08 '22 at 10:37

1 Answers1

2

Newtonsoft explicitly addressed this question in Issue #401: Object reference equality should be used when checking for circular references:

I prefer the current behavior which lets devs customize logic by overriding Equals.

Besides, this is a big breaking change.

But then later added:

Added EqualityComparer to JsonSerializer 3cc797c.

This enhancement added support for JsonSerializerSettings.EqualityComparer which allows the default behavior of calling object.Equals in reference loop detection to be overridden in settings:

public IEqualityComparer EqualityComparer { get; set; }

Gets or sets the equality comparer used by the serializer when comparing references.

dbc
  • 104,963
  • 20
  • 228
  • 340