Change the equality of JsonArray and JsonObject for the symmetry of #equals(Object)

[dmikurube]

I asked Claude to review our EEPs from two years ago, and it found a problem in both the specification and the implementation of the SPI. This pull request tries to fix it. This change slightly modifies the SPI specification, but I've judged that it is acceptable, and that it should be fixed.

The problem is that #equals of JsonValue, explicitly defined in embulk/embulk#1541, did not satisfy the symmetry required for #equals. For example, the problem was that:

jsonArray.equals(arrayList) == false

while:

arrayList.equals(jsonArray) == true

Embulk cannot interfere with the latter behavior of ArrayList. As long as JsonArray implements java.util.List, this problem is unavoidable.

Therefore, this pull request gives up the specification that "JsonArray is equal only to JsonArray." However, I don't think it is a big problem because, in the end, they are never equal unless the leaf values contained in JsonArray or JsonObject are JsonValues.

[hiroyuki-sato]

LGTM👍

Notes for myself

ArrayList and LinkedList both implement the same List interface, while HashMap and LinkedHashMap both implement the same Map interface.

According to the Java API specification, two List implementations are considered equal if they contain equal elements in the same order, and two Map implementations are considered equal if they contain the same key-value mappings. Therefore, equals can return true even when the objects being compared are instances of different classes.

For example:

new LinkedHashMap<>(Map.of("a", 1))
    .equals(new HashMap<>(Map.of("a", 1))) // -> true

new ArrayList<>(List.of("a", "b"))
    .equals(new LinkedList<>(List.of("a", "b"))) // -> true

Embulk's JsonArray and JsonObject also implement List and Map, respectively. However, their previous equals implementations behaved differently from other List and Map implementations: a JsonArray could only be equal to another JsonArray, and a JsonObject could only be equal to another JsonObject.

https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/List.html#equals(java.lang.Object)

https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/Map.html#equals(java.lang.Object)

[kou]

BTW, we may need to revisit JsonLong and JsonDouble from a similar perspective as a separated task.

https://github.com/embulk/embulk/blob/master/docs/eeps/eep-0002.md#final-class-jsonlong says the following:

    // Returns true when otherObject is also JsonLong, and has the same long value
    @Override public boolean equals(Object otherObject);

https://github.com/embulk/embulk/blob/master/docs/eeps/eep-0002.md#final-class-jsondouble says the following:

    // Returns true when otherObject is also JsonDouble, and has the same double value
    @Override public boolean equals(Object otherObject);

But JsonLong accepts JsonDouble and JsonDouble accepts JsonLong:

embulk-spi/src/main/java/org/embulk/spi/json/JsonLong.java

Lines 467 to 470 in dccebea

	if (otherObject instanceof JsonDouble) {
	final JsonDouble other = (JsonDouble) otherObject;
	return other.isLongValue() && this.value.toLong() == other.longValue();
	}

embulk-spi/src/main/java/org/embulk/spi/json/JsonDouble.java

Lines 485 to 488 in dccebea

	if (otherObject instanceof JsonLong) {
	final JsonLong other = (JsonLong) otherObject;
	return this.isLongValue() && this.value.toLong() == other.longValue();
	}

I have one more separated topic. https://github.com/embulk/embulk/blob/master/docs/eeps/eep-0002.md#equality refers https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#equals-java.lang.Object- and it says the following:

Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

Should we satisfy the hashCode contract too in our implementation?

[kou]

Should we extract this change from this PR to a separated PR because this is not related to this change?
(I understand that this is required to pass CI jobs.)

[dmikurube]

That's true, but we're not really "strict" about that. For such a short clear fix that is blocking another important fix, we may do that. Everything is not ruled especially inside the team.

[kou]

It seems that we can remove this check entirely because the new testEqualityWithGeneralList covers JsonArray.equals(NonJsonArrayObject) case.

We may be able to remove FakeJsonArray entirely because it's needless with this PR's change.

[dmikurube]

Hmm, it could be, but let's think about it in a different PR. Here, we wanted to focus on the behavior change, and highlight the difference.

[kou]

I'm not strongly opposed to splitting it into a separate PR, but I didn't quite see why removing this assertion and FakeJsonArray would make the difference less clear.

By removing assertFalse(), it already demonstrates that jsonArray.equals(FakeJsonArray.of()) is no longer false (i.e., it is true), so I don't feel it's necessary to change it to assertTrue(). Also, while deleting FakeJsonArray involves removing many lines of code, it's the deletion of an entire file, so I don't think it clutters the diff of this PR.

[dmikurube]

That's the other topic Claude has found about JsonValue, and that would be addressed in a different PR, but that's a different topic from this PR. Let's discuss it in another location.

[dmikurube]

That's true, but we're not really "strict" about that. For such a short clear fix that is blocking another important fix, we may do that. Everything is not ruled especially inside the team.

[dmikurube]

Hmm, it could be, but let's think about it in a different PR. Here, we wanted to focus on the behavior change, and highlight the difference.

[komainu8]

Just to confirm, is this new comment related to the changes in this PR?
It seems to me that it might be outside the scope.

Is this based on the reason described in https://github.com/embulk/embulk-spi/pull/6/changes#r4117974898 ? If so, I understand.
Or is it actually part of this PR's scope and I just misunderstand the context?