[Bug] Still Trying to operate directly on the abstract class with instances provided

[xenoterracide]

Describe the bug

So, I'm guessing that the problem is that I have to provide 2 sets of prefabs for AbstractIdentity "Id" classes that subclass it are unique for each entity. None of the 4 instances I've provided would equal each other, but FooAggregate.id could never be a BarEntity.Id

Steps to reproduce

Unfortunately the structure is fairly complex, I think it's easier to link you to it

https://github.com/xenoterracide/spring-app-commons/tree/test/equalsverifier/module/jpa/src/test/java/com/xenoterracide/jpa

to run locally since it's now relying on another of my repos snapshots. You need to add a ~/.gradle/gradle.properties with

ghUsername=<your user>
ghPassword=<a class personal access token that has read packages>

Error message and version number

java.lang.AssertionError: EqualsVerifier found a problem in class com.xenoterracide.jpa.FooAggregate.
-> Missing implementation of resolved method 'abstract boolean canEqual(com.xenoterracide.jpa.AbstractIdentity)' of abstract class com.xenoterracide.jpa.AbstractIdentity.

For more information, go to: https://www.jqno.nl/equalsverifier/errormessages
(EqualsVerifier 3.16, JDK 21.0.2 on Linux)

Code: EqualsVerifier invocation

  @Test
  void abstractAggregateEquality() {
    EqualsVerifier.forClass(FooAggregate.class)
      .withRedefinedSuperclass()
      .withPrefabValues(BarEntity.Id.class, BarEntity.Id.create(), BarEntity.Id.create())
      .withPrefabValues(FooAggregate.Id.class, FooAggregate.Id.create(), FooAggregate.Id.create())
      .withPrefabValues(BarEntity.class, BarEntity.create(null, "bar"), BarEntity.create(null, "baz"))
      .suppress(Warning.SURROGATE_OR_BUSINESS_KEY)
      .withOnlyTheseFields("id", "version", "dirty")
      .verify();
  }

Code: class under test

No response

Additional context

No response

[jqno]

I'm sorry, I'm really going to have to ask you to make a minimal reproducer for this one. You've created 9 new issues in less than 2 weeks, where normally this project averages less than 1 issue per month. I'm struggling to keep up here.

If I need to clone a repoal and fiddle with an unfamiliar build, that's already a big investment of time. But then I'd also have to figure out what the actual problem is (you start the issue with "So, I'm guessing that the problem is..." which isn't very helpful, and then priced directly to a potential solution, without describing the actual problem)... that just takes too much time, I'm sorry.

If you have something that I can copy/paste directly into my editor, without having to figure out imports and without any code that doesn't contribute directly to the issue, I'll be happy to take a look!

[xenoterracide]

ou've created 9 new issues in less than 2 weeks, where normally this project averages less than 1 issue per month. I'm struggling to keep up here.

sorry about that.

I can flatten the thing, but it really doesn't get much simpler in this case. Since the problem goes all the way down into the nature of the AbstractIdentity, simpler is mostly stripping annotations and setters. I'm not even sure if stripping annotations is a good idea... as I'm not certain how having @Id might affect this, as it seems to affect a lot. Since 2 Id sub classes are required as well in this scenario.

[xenoterracide]

import jakarta.persistence.CascadeType;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.MappedSuperclass;
import jakarta.persistence.OneToMany;
import jakarta.persistence.Transient;
import jakarta.persistence.Version;
import jakarta.validation.constraints.NotNull;
import java.io.Serial;
import java.io.Serializable;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.UUID;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;

public class Scratch {

  @MappedSuperclass
  public static abstract class AbstractEntity<ID extends AbstractIdentity<? extends Serializable>> {

    @Transient
    private boolean dirty;

    private @NonNull ID id;
    private @Nullable Integer version;

    protected AbstractEntity() {
      this.dirty = false;
    }

    protected AbstractEntity(@NonNull ID id) {
      this.id = id;
    }

    @Version
    @Nullable
    @Column(nullable = false)
    Integer getVersion() {
      return this.version;
    }

    void setVersion(Integer version) {
      this.version = version;
    }

    protected void markDirty() {
      this.dirty = true;
    }

    @Id
    @Column(nullable = false, updatable = false, unique = true)
    public @NonNull ID getId() {
      return this.id;
    }

    void setId(@NonNull ID id) {
      this.id = id;
    }

    @Override
    public final int hashCode() {
      return Objects.hash(this.id, this.version, this.dirty);
    }

    protected abstract boolean canEqual(@NonNull AbstractEntity<?> that);

    @Override
    public final boolean equals(@Nullable Object other) {
      if (other instanceof AbstractEntity<?> that) {
        // CHECKSTYLE.OFF: UnnecessaryParentheses
        return (
          that.canEqual(this) &&
            Objects.equals(this.id, that.id) &&
            Objects.equals(this.version, that.version) &&
            this.dirty == that.dirty
        );
        // CHECKSTYLE.ON: UnnecessaryParentheses
      }
      return false;
    }
  }

  @MappedSuperclass
  public static abstract class AbstractIdentity<ID extends Serializable> implements Serializable {

    @Serial
    private static final long serialVersionUID = 1L;

    private @Nullable ID id;

    protected AbstractIdentity() {
    }

    protected AbstractIdentity(@NonNull ID id) {
      this.id = Objects.requireNonNull(id);
    }

    @Override
    public final int hashCode() {
      return Objects.hashCode(this.id);
    }

    protected abstract boolean canEqual(@NonNull AbstractIdentity<?> that);

    @Override
    public final boolean equals(@Nullable Object other) {
      if (other instanceof AbstractIdentity<?> that) {
        return that.canEqual(this) && Objects.equals(this.id, that.id);
      }
      return false;
    }

    @Override
    public String toString() {
      return String.valueOf(this.id);
    }
  }

  @Entity
  public static class BarEntity extends AbstractEntity<BarEntity.@NonNull Id> {

    private @NonNull String name;

    private @NonNull FooAggregate foo;

    BarEntity(@NonNull Id id, @NonNull FooAggregate foo, @NonNull String name) {
      super(id);
      this.foo = foo;
      this.name = name;
    }

    public BarEntity() {
    }

    static @NonNull BarEntity create(@NonNull FooAggregate foo, @NonNull String name) {
      return new BarEntity(Id.create(), foo, name);
    }

    @ManyToOne(
      optional = false,
      fetch = FetchType.LAZY,
      cascade = {CascadeType.DETACH, CascadeType.MERGE, CascadeType.PERSIST, CascadeType.REFRESH}
    )
    @JoinColumn(nullable = false, updatable = false, name = "foo_id")
    public @NonNull FooAggregate getFoo() {
      return foo;
    }

    void setFoo(@NonNull FooAggregate foo) {
      this.foo = foo;
    }

    @Override
    protected boolean canEqual(@NonNull AbstractEntity<?> that) {
      return that instanceof BarEntity;
    }

    @NotNull
    @Column(nullable = false)
    public @NonNull String getName() {
      return name;
    }

    void setName(@NonNull String name) {
      this.name = name;
    }

    public static class Id extends AbstractIdentity<@NonNull UUID> {

      @Serial
      private static final long serialVersionUID = 1L;

      protected Id() {
      }

      private Id(@NonNull UUID id) {
        super(id);
      }

      public static Id create() {
        return new Id(UUID.randomUUID());
      }

      @Override
      protected boolean canEqual(@NonNull AbstractIdentity<?> that) {
        return that instanceof Id;
      }
    }
  }

  @Entity
  public static class FooAggregate extends AbstractEntity<FooAggregate.@NonNull Id> {

    private String name;

    private Set<@NonNull BarEntity> bars = new HashSet<>();

    protected FooAggregate() {
    }

    public FooAggregate(@NonNull Id id, @NonNull String name) {
      super(id);
      this.name = name;
    }

    static @NonNull FooAggregate create(@NonNull String name) {
      return new FooAggregate(Id.create(), name);
    }

    public @NonNull BarEntity addBar(@NonNull String name) {
      var bar = BarEntity.create(this, name);
      this.bars.add(bar);
      return bar;
    }

    @OneToMany(orphanRemoval = true, cascade = CascadeType.ALL, fetch = FetchType.LAZY, mappedBy = "foo")
    @NonNull
    Set<@NonNull BarEntity> getBars() {
      return this.bars;
    }

    void setBars(@NonNull Set<@NonNull BarEntity> bars) {
      this.bars = bars;
    }

    @NotNull
    @Column(nullable = false)
    public @NonNull String getName() {
      return name;
    }

    void setName(@NonNull String name) {
      this.name = name;
    }

    @Override
    protected boolean canEqual(@NonNull AbstractEntity<?> that) {
      return that instanceof FooAggregate;
    }

    public static class Id extends AbstractIdentity<@NonNull UUID> {

      @Serial
      private static final long serialVersionUID = 1L;

      protected Id() {
      }

      private Id(@NonNull UUID id) {
        super(id);
      }

      public static Id create() {
        return new Id(UUID.randomUUID());
      }

      @Override
      protected boolean canEqual(@NonNull AbstractIdentity<?> that) {
        return that instanceof Id;
      }
    }
  }
}

Tried to clean things up, sorry I left in JPA and Jspecify imports in case they affect behavior here in any way. It's wrapped in a Scratch class, which you can rename as a test or whatever, it's irrelevant.

As you can see I have 2 implementations of AbstractIdentity but in a final implementation you can't actually use AbstractIdentity itself. My workaround was to add

      .withPrefabValues(AbstractIdentity.class, FooAggregate.Id.create(), BarEntity.Id.create())

To be honest, I'm not certain what the correct solution for this is, or what equalsverifier expects here. FooAggregate.Id can never equal BarEntity.Id by type, but also they'd never get the same UUID (note to any future readers, don't use randomUUID() for your database identifier ever, it'll create insert slowdown issues with your index eventually, this actually uses UuidCreator.getTimeOrderedEpoch() which is not part of java).

[jqno]

sorry I left in JPA and Jspecify imports in case they affect behavior here in any way.

I mean, that's the point of having a minimal reproducer, right? If you remove them and the problem goes away, you know they were relevant. If you remove them and the problem persists, they weren't needed and you can keep them out. Did you try it?

I could try it too I suppose, but the smaller you make it, the bigger the chance I can find a moment to look into it between meetings.

[xenoterracide]

How's this?

package com.xenoterracide.model.security;

import java.io.Serial;
import java.io.Serializable;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.UUID;
import nl.jqno.equalsverifier.EqualsVerifier;
import nl.jqno.equalsverifier.Warning;
import org.junit.jupiter.api.Test;

public class EvTest {

  @Test
  void tev() {
    EqualsVerifier.forClass(FooAggregate.class)
      .withRedefinedSuperclass()
      .withPrefabValues(BarEntity.Id.class, BarEntity.Id.create(), BarEntity.Id.create())
      .withPrefabValues(FooAggregate.Id.class, FooAggregate.Id.create(), FooAggregate.Id.create())
      .withPrefabValues(BarEntity.class, BarEntity.create(null, "bar"), BarEntity.create(null, "baz"))
      .suppress(Warning.SURROGATE_OR_BUSINESS_KEY)
      .withOnlyTheseFields("id")
      .verify();
  }

  public abstract static class AbstractEntity<ID extends AbstractIdentity<? extends Serializable>> {

    private ID id;

    protected AbstractEntity() {}

    protected AbstractEntity(ID id) {
      this.id = id;
    }

    public ID getId() {
      return this.id;
    }

    void setId(ID id) {
      this.id = id;
    }

    @Override
    public final int hashCode() {
      return Objects.hash(this.id);
    }

    protected abstract boolean canEqual(AbstractEntity<?> that);

    @Override
    public final boolean equals(Object other) {
      if (other instanceof AbstractEntity<?> that) {
        // CHECKSTYLE.OFF: UnnecessaryParentheses
        return (that.canEqual(this) && Objects.equals(this.id, that.id));
        // CHECKSTYLE.ON: UnnecessaryParentheses
      }
      return false;
    }
  }

  public abstract static class AbstractIdentity<ID extends Serializable> implements Serializable {

    @Serial
    private static final long serialVersionUID = 1L;

    private ID id;

    protected AbstractIdentity() {}

    protected AbstractIdentity(ID id) {
      this.id = Objects.requireNonNull(id);
    }

    @Override
    public final int hashCode() {
      return Objects.hashCode(this.id);
    }

    protected abstract boolean canEqual(AbstractIdentity<?> that);

    @Override
    public final boolean equals(Object other) {
      if (other instanceof AbstractIdentity<?> that) {
        return that.canEqual(this) && Objects.equals(this.id, that.id);
      }
      return false;
    }

    @Override
    public String toString() {
      return String.valueOf(this.id);
    }
  }

  public static class BarEntity extends AbstractEntity<BarEntity.Id> {

    private String name;

    private FooAggregate foo;

    BarEntity(BarEntity.Id id, FooAggregate foo, String name) {
      super(id);
      this.foo = foo;
      this.name = name;
    }

    public BarEntity() {}

    static BarEntity create(FooAggregate foo, String name) {
      return new BarEntity(BarEntity.Id.create(), foo, name);
    }

    public FooAggregate getFoo() {
      return foo;
    }

    void setFoo(FooAggregate foo) {
      this.foo = foo;
    }

    @Override
    protected boolean canEqual(AbstractEntity<?> that) {
      return that instanceof BarEntity;
    }

    public String getName() {
      return name;
    }

    void setName(String name) {
      this.name = name;
    }

    public static class Id extends AbstractIdentity<UUID> {

      @Serial
      private static final long serialVersionUID = 1L;

      protected Id() {}

      private Id(UUID id) {
        super(id);
      }

      public static Id create() {
        return new Id(UUID.randomUUID());
      }

      @Override
      protected boolean canEqual(AbstractIdentity<?> that) {
        return that instanceof Id;
      }
    }
  }

  public static class FooAggregate extends AbstractEntity<FooAggregate.Id> {

    private String name;

    private Set<BarEntity> bars = new HashSet<>();

    protected FooAggregate() {}

    public FooAggregate(FooAggregate.Id id, String name) {
      super(id);
      this.name = name;
    }

    static FooAggregate create(String name) {
      return new FooAggregate(FooAggregate.Id.create(), name);
    }

    public BarEntity addBar(String name) {
      var bar = BarEntity.create(this, name);
      this.bars.add(bar);
      return bar;
    }

    Set<BarEntity> getBars() {
      return this.bars;
    }

    void setBars(Set<BarEntity> bars) {
      this.bars = bars;
    }

    public String getName() {
      return name;
    }

    void setName(String name) {
      this.name = name;
    }

    @Override
    protected boolean canEqual(AbstractEntity<?> that) {
      return that instanceof FooAggregate;
    }

    public static class Id extends AbstractIdentity<UUID> {

      @Serial
      private static final long serialVersionUID = 1L;

      protected Id() {}

      private Id(UUID id) {
        super(id);
      }

      public static Id create() {
        return new Id(UUID.randomUUID());
      }

      @Override
      protected boolean canEqual(AbstractIdentity<?> that) {
        return that instanceof Id;
      }
    }
  }
}

[jqno]

That was really helpful! I was able to reproduce the problem with that. I also reduced the code even further:

package nl.jqno.equalsverifier;

import java.util.Objects;
import java.util.UUID;
import org.junit.jupiter.api.Test;

public class EvTest {

    @Test
    void tev() {
        EqualsVerifier
            .forClass(FooAggregate.class)
            .withPrefabValues(BarEntity.class, new BarEntity(), new BarEntity())
            .verify();
    }

    public abstract static class AbstractEntity<ID extends AbstractIdentity<?>> {

        private ID id;

        protected AbstractEntity() {}

        protected AbstractEntity(ID id) {
            this.id = id;
        }

        @Override
        public final boolean equals(Object other) {
            if (!(other instanceof AbstractEntity<?>)) {
                return false;
            }
            AbstractEntity<?> that = (AbstractEntity<?>) other;
            return Objects.equals(this.id, that.id);
        }
    }

    public abstract static class AbstractIdentity<ID> {

        private ID id;

        protected AbstractIdentity() {}

        protected AbstractIdentity(ID id) {
            this.id = id;
        }

        protected abstract boolean canEqual(AbstractIdentity<?> that);

        @Override
        public final boolean equals(Object other) {
            if (!(other instanceof AbstractIdentity<?>)) {
                return false;
            }
            AbstractIdentity<?> that = (AbstractIdentity<?>) other;
            return that.canEqual(this) && Objects.equals(this.id, that.id);
        }
    }

    public static class BarEntity extends AbstractEntity<BarEntity.Id> {

        BarEntity() {
            super(new BarEntity.Id());
        }

        public static class Id extends AbstractIdentity<UUID> {

            private Id() {
                super(UUID.randomUUID());
            }

            @Override
            protected boolean canEqual(AbstractIdentity<?> that) {
                return that instanceof Id;
            }
        }
    }

    public static class FooAggregate extends AbstractEntity<FooAggregate.Id> {

        public static class Id extends AbstractIdentity<UUID> {

            @Override
            protected boolean canEqual(AbstractIdentity<?> that) {
                return that instanceof Id;
            }
        }
    }
}

I noticed that if I remove the prefab values, there's still an error, but a slightly different one. That's very strange. I need to investigate further. But in the mean time, thanks for this!

[jqno]

Managed to simplify even further:

package nl.jqno.equalsverifier;

import java.util.Objects;
import org.junit.jupiter.api.Test;

public class EvTest {

    @Test
    void tev() {
        EqualsVerifier.forClass(FooAggregate.class).verify();
    }

    public static class FooAggregate {

        private AbstractIdentity id;

        @Override
        public final boolean equals(Object other) {
            if (other instanceof FooAggregate that) {
                return Objects.equals(this.id, that.id);
            }
            return false;
        }
    }

    public abstract static class AbstractIdentity {

        private int id;

        protected abstract boolean canEqual(AbstractIdentity that);

        @Override
        public final boolean equals(Object other) {
            if (other instanceof AbstractIdentity that) {
                return that.canEqual(this) && Objects.equals(this.id, that.id);
            }
            return false;
        }
    }
}

Turns out that it's the abstract call in AbstractIdentity's equals. I thought I'd thoroughly tested that scenario, but in my tests, there was a if (this == other) return true; clause, and in yours there isn't. There is one specific path in EqualsVerifier that calls equals before the abstract methods are checked, and your class happened to hit that path.

The reason why the error message was different with a prefab value, is because BarAggregate calls the same abstract method, and the path through the code becomes slightly different.

Anyway, I've made a fix. I can release it now if you want, or we can wait for #940 and we'll have a two-for-one. What do you prefer?

[jqno]

Version 3.16.1 is now syncing with Maven Central.