LeakyAbstraction: NumericNodes which represent the same Json value are not considered equal

[ludgerb]

• I searched in the issues and found nothing similar.

Describe the bug

Hi everyone.

when converting a java object to a JsonNode tree, depending on the underlying implementation chosen, the resulting Json trees are not considered equal, even though they represent the same Json value and serialize to the same Json string representation.

I.e. an IntNode representing 12345 is not equal to a LongNode, ShortNode, BigIntegerNode or DecimalNode representing the same value.

If I deserialize these nodes and compare the resulting Json string, they are considered equal.

This is a case of leaky abstraction.

Use case is:

When testing a class producing Json output, I want to compare the produced Json to a reference json to check correctness of the implementation.

Version Information

2.17.1

Reproduction

  @Test
  void jsonNumberSerialisationTest() throws JsonProcessingException {

    var value = 12345;
    var valueString = String.valueOf(value);

    var objectMapper = new ObjectMapper();

    var referenceNode = objectMapper.readTree(valueString);

    var intNode = IntNode.valueOf(value);
    var bigIntegerNode = BigIntegerNode.valueOf(BigInteger.valueOf(value));
    var longNode = LongNode.valueOf(Long.valueOf(value));
    var decimalNode = DecimalNode.valueOf(BigDecimal.valueOf(value));
    var shortNode = ShortNode.valueOf((short)value);

    // serialized Json is identical, representing the same Json
    assertEquals(valueString, objectMapper.writeValueAsString(referenceNode));
    assertEquals(valueString, objectMapper.writeValueAsString(intNode));
    assertEquals(valueString, objectMapper.writeValueAsString(bigIntegerNode));
    assertEquals(valueString, objectMapper.writeValueAsString(longNode));
    assertEquals(valueString, objectMapper.writeValueAsString(decimalNode));
    assertEquals(valueString, objectMapper.writeValueAsString(shortNode));

    // JsonNodes are not equal, depending on implementation used
    assertEquals(referenceNode, intNode); // passes - both are the same node type
    assertEquals(referenceNode, bigIntegerNode); // fails
    assertEquals(referenceNode, longNode); // fails
    assertEquals(referenceNode, decimalNode); // fails
    assertEquals(referenceNode, shortNode); // fails
  }

Expected behavior

According to my expectation JsonNode trees representing the same Json String are also considered equal and do not differ in equality based on internal (implementation) details.

I do understand that for float and double this might not always hold since there -depending on precision- they may yield differing results.

Current workaround in JunitTests before comparing json result trees with the respective expected reference json:

• deserialize objects first to a Json String representation and then converting this back to a JsonNode tree, to align the JsonNode trees and ensure truly equal-check safe JsonNode trees.

Though this workaround adds considerable computational overhead for the (imho unnecessary) additional deserialisation step required.

[cowtowncoder]

Ok, first, I disagree on specific ask wrt JsonNode.equals(): the behavior is as intended.
The challenge is not just performance overhead (although that alone could be disqualifying for me), but sheer complexity to attempt to implement all around numeric equality, transitivity (order of calls should not matter etc). We did think about that a while ago and decided it's really not worth the hassle.
This is different from JsonNodes that represent other JSON scalar types (Strings, Booleans).

But! Back then we last thought about this problem (there is likely closed old issue for this, will try to find it), we added this method in JsonNode:

public boolean equals(Comparator<JsonNode> comparator, JsonNode other) {

in JsonNode to allow for pluggable comparisons. It is implemented as expected by ObjectNode and ArrayNode to delegate, so comparator just needs to consider scalar types.
So implementing such Comparator allows pluggable equality implementation.

But the logical question, then, is whether Jackson project should offer an implementation that did use full equality as you described.
I would be supportive of such effort, being included as optional added comparator.
And maybe adding convenience methods as necessary in ObjectMapper and ObjectReader.

[cowtowncoder]

For context we have previous issues:

• JsonNode.equals is not actually comparing JSON nodes (?) #3698
• LongNode(3) != IntNode(3) #1758
• Equals across BigInteger and BigNumber values #1561
• Shouldn't Long and Int Nodes be equivalent? #333
• DecimalNode and equality: on purpose or not, I don't know, but... #80

as well as issue for which alternate equals() method was added:

• Add JsonNode.equals(Comparator<JsonNode>, JsonNode) to support configurable/external equality comparison #790

So this is definitely something users find challenging. But as I said, I think the path forward would be with providing "Number canonicalizing" Comparator.
It could even be configurable in some aspects. F.ex

• Does it unify Integral (int, long, BigInteger) and Fractional (double, float, BigDecimal) or not?
• What about trailing zeroes? (that is; Lexical equality (trailing zeroes matter) or Logical/Content equality (trailing zeroes would not matter)

Having said that, to get progress, a PR would probably be needed.

[JooHyukKim]

Yeah, the suggested equals method would do 👍🏼

    public boolean equals(Comparator<JsonNode> comparator, JsonNode other) {

Otherwise, you're welcome to read through mentioned issues by @cowtowncoder above and help come up with solution -- that hopefully satisfies every(or most) user's need. Saying this because you mentioned leaky abstraction and I think we can also say that JSON number specification itself is leaky. Converting from fine grained expression to rough is easy, but not the way around.

[yawkat]

I think a Comparator.<JsonNode, String>comparing(objectMapper::writeValueAsString) would be what most people expect for value comparison.

[cowtowncoder]

@yawkat That would be delightfully simple implementation indeed. So then it'd be just the question of plugging it in API. As I said I do not think it'd be good for JsonNode.equals(Object), but any other way to plug is fair game.