Closed smainz closed 1 year ago
jqno
commented
1 year ago Hm, I think your assessment is correct, but I'll have to investigate further. I'll let you know when I've done so.
Thanks for the verbosity btw, it's actually extremely helpful in investigating issues!
jqno
commented
1 year ago Took a while to get to this, sorry about that.
It turns out that the abstract class thing was a red herring, there is an actual Symmetry problem. On the failing check, the call to equals() actually goes through the else { return false; } branch at the end of the method. It doesn't touch the branch where the acutal check is performed, and therefore doesn't fail on calling the (still abstract) getId() method.
Then it tries to print an error message, and because toString() also tries to call getId(), that is where the AbstractMethodError occurs. This is consequently eaten by EqualsVerifier to produce a nicer looking error message, which is what you see.
You can check this for yourself in two ways. First, let's make the class non-abstract:
public static class EntityId implements Serializable {
private String id;
public EntityId(String id) {
this.id = id;
}
public String getId() {
return id;
};
public boolean equals(Object obj) {
if (obj == null) return false;
if (this.getClass().isAssignableFrom(obj.getClass())) {
String otherId = ((EntityId) obj).getId();
return Objects.equals(getId(), otherId);
}
return false;
}
public int hashCode() {
return Objects.hash(this.getId());
}
@Override
public String toString() {
return getClass().getSimpleName() + "{" +
"id=" + getId() +
'}';
}
}
public static class AdditionalCompanyDataId extends EntityId {
private AdditionalCompanyDataId(String id) {
super(id);
}
}You'll get the same error message as before, but without the abstract method stuff:
[ERROR] AbstractHierarchyTest.issue716:78 EqualsVerifier found a problem in class nl.jqno.equalsverifier.integration.inheritance.AbstractHierarchyTest$AdditionalCompanyDataId.
-> Symmetry:
AdditionalCompanyDataId{id=one}
does not equal superclass instance
EntityId{id=one}
For more information, go to: https://www.jqno.nl/equalsverifier/errormessagesWe can leave the class abstract, and change the equals() implementation slightly:
public abstract class EntityId<ID> implements Serializable {
@NonNull
public abstract ID getId();
public final boolean equals(Object other) {
if (!(other instanceof EntityId)) {
return false;
}
Object otherId = ((EntityId) other).getId();
return Objects.equals(getId(), otherId);
}
public final int hashCode() {
return Objects.hash(this.getId());
}
@Override
public String toString() {
return getClass().getSimpleName() + "{" +
"id=" + getId() +
'}';
}
}With this implementation, the EqualsVerifier test passes. (Note that I've also made equals() and hashCode() final here.)
Now of course, this is not the implementation that you wanted. In order to make it work the way you want to, I'd suggest checking this part of the EqualsVerifier manual
For the next release, I'll change the way EqualsVerifier deals with exceptions thrown by toString(), to make things less confusing :sweat_smile:
jqno
commented
1 year ago Better error messages are released in EqualsVerifier 3.12.1.
smainz
commented
1 year ago Thank you for this detailed analysis and sorry for putting you on the wrong track.
The whole idea of this is to create type save hibernate ids without having to re-type all the boilerplate code. My only requirement is that you can not pass an instance of class A with
class A extends EntityId<String> {
...
}to a method which expects an id of class B with
class B extends EntityId<String> {
...
}In hibernate this kind of id is an embeddable and somtimes is replaced by an - on the fly created - proxy.
While I do not consider new A("1") be equal to new B("1") if have to consider new B("1") equal to the proxy of B.
Sounds like there is no possible solution for this.
As it is unlikely to compare A to B i can make equals symmetric by using your approach for equals:
public final boolean equals(Object other) {
if (!(other instanceof EntityId)) {
return false;
}
Object otherId = ((EntityId) other).getId();
return Objects.equals(getId(), otherId);
}There will never be instances of EntityId<> (the abstract class) nor will there be derived classes of A´ orB` except maybe the generated proxies.
jqno
commented
1 year ago Oh but it certainly is possible! It's not easy, but it is possible. Check out this link: https://www.artima.com/articles/how-to-write-an-equality-method-in-java (and skip to "Pitfall 4" if you're impatient).
Then you can check out this part of the EqualsVerifier manual to see how to translate this to EqualsVerifier: https://jqno.nl/equalsverifier/manual/inheritance/
Describe the bug
EqualsVerifier tries to instantiate abstract base classes and fails if the euals method in this class calls an abstract method.
EqualsVerifier fails on every derived class with this error message:
To Reproduce Steps to reproduce the behavior
Create the class EntityId
`2. Create a derived class like this
EqualsVerifier.forClass(AdditionalCompanyDataId.class).verify();`Code that triggers the behavior See above,
Error message Complete stacktrace:
The root couse is this: Method threw 'java.lang.AbstractMethodError' exception. Cannot evaluate de.dynaware.vbank.cryptowallet.additionaldata.valueobject.EntityId$$DynamicSubclass$1827725498.toString()
https://github.com/jqno/equalsverifier/blob/35f8df72426923d7ca43ec10d3546b3c4af5d46b/equalsverifier-core/src/main/java/nl/jqno/equalsverifier/internal/reflection/InPlaceObjectAccessor.java#L23
called from:
https://github.com/jqno/equalsverifier/blob/35f8df72426923d7ca43ec10d3546b3c4af5d46b/equalsverifier-core/src/main/java/nl/jqno/equalsverifier/internal/checkers/HierarchyChecker.java#L156-L158
Expected behavior
Expected behaviour was to ignore this check if the base class is abstract as it does here:
https://github.com/jqno/equalsverifier/blob/35f8df72426923d7ca43ec10d3546b3c4af5d46b/equalsverifier-core/src/main/java/nl/jqno/equalsverifier/internal/checkers/HierarchyChecker.java#L115-L122
Version 3.10
Additional context
Workaround is to
.suppress( Warning.STRICT_INHERITANCE)I think it is wrong to try to instantiate an abstract base class.
Sory for beeing so verbose and thank you for thi snice library.