Inconsistent implementation of equals() and hashCode()

[Nadahar]

Java requires that equals() and hashCode() must be consistent, two objects that are "equal" must always produce the same hash code.

Failing to respect this will make things go wrong with HashMap, HashTable etc., i.e. some of the core Java building blocks that many other things build upon.

I've seen that this is implemented wrong in some of the Yaml classes, e.g:

openhab-core/bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/items/YamlMetadataDTO.java

Lines 40 to 55 in 6f7d46c

	@Override
	public int hashCode() {
	return Objects.hash(getValue());
	}
	@Override
	public boolean equals(@Nullable Object obj) {
	if (this == obj) {
	return true;
	} else if (obj == null || getClass() != obj.getClass()) {
	return false;
	}
	YamlMetadataDTO other = (YamlMetadataDTO) obj;
	return Objects.equals(getValue(), other.getValue()) && YamlElementUtils.equalsConfig(config, other.config);
	}
	}

Similar errors exist in YamlChannelDTO and YamlThingDTO.

[Nadahar]

@lolodomo This one is probably for you.

[lolodomo]

Java requires that equals() and hashCode() must be consistent, two objects that are "equal" must always produce the same hash code.

I think it is the case with current implementation. I will extend the existing unit tests related to object equality to also check hash code.

Do you already have an example of two objects that would be equal in my code but with a different hash code?

[Nadahar]

If you look very narrowly on it, you could just hardcode hashCode() to always return 1, and you would qualify for that requirement. But, it would defeat the purpose of hashCode(), and performance in hash-based implementations would be disastrous. The idea, as I understand it, is that hashCode() should, as far a possible, "mirror" equals(). So, if equals() is false, hashCode() is different. But, because of the limitations in hashing functions, this isn't possible to guarantee. As a result, it is allowed that "non-equal" objects return the same hashCode() if a "hash collision" occurs. But, it shouldn't be the norm.

To me, the consequence of how this works is quite simple: equals() and hashCode() should always take exactly the same information as the basis for their "calculation". For all practical purposes, hashCode() can be used to determine equality, that is how hash-based algorithms use them, but with the caveat that they must also take into consideration the rare case of hash-collisions where two non-equal objects might share a hash code.

[Nadahar]

There are lots of information about this online, so you don't have to take my word for it, for example: https://www.baeldung.com/java-hashcode#handling-hash-collisions

I just have it as "a rule" that equals() and hashCode() should always take the same information into consideration, so that they behave as similar as possible.

A quick seach on the topic results in the following:

The "best" hashCode() implementation in Java adheres to the contract defined in the Object class and aims for a good distribution of hash codes to minimize collisions in hash-based collections like HashMap or HashSet.

Key Principles for a Good hashCode() Implementation:

Consistency with equals():

If two objects are equal according to the equals() method, their hashCode() methods must produce the same integer result. This is a fundamental requirement of the hashCode() contract.

Determinism:

Multiple invocations of hashCode() on the same object must consistently return the same integer, provided the object's state (fields used in equals()) has not been modified.

Good Distribution:

The hashCode() method should aim to produce distinct hash codes for unequal objects to minimize collisions and ensure efficient performance of hash-based collections.

[lolodomo]

Ok to improve that but we are in pure theory because in practice the collisions will already never occur with real user files with the current implementation.

It is important to mention there is no functional bug in current implementation. No problem for users.

Time spent on that is of course not spent on something else (like review).

[lolodomo]

I am not sure if Objects.equals is able to properly compare two List of String and two Set of String, using values and not references ? I understood it was not the case when I initially implemented that and searched on Internet.
Maybe my methosds equalsListStrings and equalsSetStrings are then not necessary.

[lolodomo]

Looks like tests are OK using Objects.equals. So i use it now instead of the two specific methods.

[Nadahar]

Ok to improve that but we are in pure theory because in practice the collisions will already never occur with real user files with the current implementation.

Maybe you know that, I don't know that. I've just always considered it "a bug" if equals() and hashCode() don't evaluate the same data. I generally find it difficult to have an overview over all situations a code potentially can face, so I often write code that handles things I think will "probably never happen" just to make sure that things don't go wrong if it happens anyway.

I am not sure if Objects.equals is able to properly compare two List of String and two Set of String, using values and not references ?

As I wrote in the #4633, I'm not aware of any issues with Objects.equals() for any standard Collection. It will rely on the equals() method of the objects in the collection, so it will be "as good as what that equals() method is". So, assuming that the objects in the collection has a correct equals() implementation, it should work fine. Objects.equals() will compare the elements and the order for Lists, elements, but not order for Maps and Sets.

Maybe my methosds equalsListStrings and equalsSetStrings are then not necessary.

I saw that these were called from other places that equals() as well, but I didn't study exactly how they were used. Generally, I consider it fine to use such custom methods for "evaluating equality" for other purposes, but not for equals() itself. I mean, "equality" can mean so many things, you might consider two objects equal for one purpose while they "technically" aren't equal from Java's POV.

I made a Rationale class once for example, where I made a equalValue() method. equals() couldn't return true if you compare "1/2" and "2/4" (for several reasons, in some operations you need to treat these as unequal), but when you're just interested in comparing values, they are "equal". equalValue() will thus return true for those two, while equals() will return false. I think that's fine.