null fields shouldn't be equal, but also shouldn't suppress identical copy

[xenoterracide]

This is one of those, JPA surrogate id's can be great but really suck for equals. Given the following @Embeddable superclass.

I'm thinking that if id == null on both sides the object should not be equal, even if they're identical. To be fair these should actually never have a null id past hibernate load in production code.

I don't see a good way to test this, this is how I'm testing right now (just lazy subclasses). Neither supressing IDENTICAL_COPY or withNonnullFields reads as appropriate because I don't really want to ignore these fields. Especially not because I was giving myself NPE's for a second (calling the updated getter eek).

-> Reflexivity: object does not equal an identical copy of itself:
  com.xenoterracide.jpa.TestEntity@0
If this is intentional, consider suppressing Warning.IDENTICAL_COPY

  @Test
  void equality() {
    EqualsVerifier.forClass(TestEntity.class)
      .withRedefinedSuperclass()
      .withPrefabValues(AbstractUuidEntityBase.AbstractIdentity.class, TestEntity.Id.create(), TestEntity.Id.create())
      .suppress(Warning.SURROGATE_KEY)
      .verify();
  }

 // © Copyright 2024 Caleb Cushing
// SPDX-License-Identifier: AGPL-3.0-or-later

package com.xenoterracide.jpa;

import jakarta.persistence.Column;
import jakarta.persistence.Id;
import jakarta.persistence.MappedSuperclass;
import jakarta.persistence.PostLoad;
import jakarta.persistence.PrePersist;
import jakarta.persistence.Transient;
import java.io.Serial;
import java.io.Serializable;
import java.util.Objects;
import java.util.UUID;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;

/**
 * The type Abstract uuid entity base.
 *
 * @param <ID> the type parameter
 */
@MappedSuperclass
public abstract class AbstractUuidEntityBase<ID extends AbstractUuidEntityBase.AbstractIdentity>
  implements Identifiable<@NonNull ID> {

  /**
   * Surrogate Identifier.
   */
  private @Nullable ID id;

  /**
   * NO-OP parent constuctor for JPA only.
   */
  protected AbstractUuidEntityBase() {}

  /**
   * Instantiates a new Abstract uuid entity base.
   *
   * @param id the id
   */
  protected AbstractUuidEntityBase(@NonNull ID id) {
    this.id = id;
  }

  @Id
  @Override
  public @NonNull ID getId() {
    return Objects.requireNonNull(id);
  }

  /**
   * Sets id.
   *
   * @apiNote for JPA use only
   * @param id the id
   */
  void setId(@NonNull ID id) {
    this.id = id;
  }

  @PrePersist
  @PostLoad
  void ensureId() {
    Objects.requireNonNull(this.id, "use static factory method to create new id");
    Objects.requireNonNull(this.id.getId(), "use static factory method to create new id");
  }

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

  /**
   * That is an {@code instanceof} this concrete class.
   *
   * @param that the other object
   * @return the boolean
   * @see <a href="https://www.artima.com/articles/how-to-write-an-equality-method-in-java">
   *   How to Write an Equality Method in Java
   *   </a>
   */
  protected abstract boolean canEqual(@NonNull AbstractUuidEntityBase<?> that);

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

  /**
   * Abstract domain identifier.
   */
  @MappedSuperclass
  public abstract static class AbstractIdentity implements Serializable {

    @Serial
    @Transient
    private static final long serialVersionUID = 1L;

    /**
     * The actual database UUID for id.
     */
    private @Nullable UUID id;

    /**
     * NO-OP parent constuctor for JPA only.
     */
    protected AbstractIdentity() {}

    /**
     * Instantiates a new Abstract identity.
     *
     * @param id the id
     */
    protected AbstractIdentity(@NonNull UUID id) {
      this.id = id;
    }

    /**
     * Gets id.
     *
     * @apiNote for JPA use only
     * @return the id
     */
    @Column(name = "id", updatable = false, nullable = false, unique = true, columnDefinition = "uuid")
    UUID getId() {
      return Objects.requireNonNull(id);
    }

    /**
     * Sets id.
     *
     * @apiNote for JPA use only
     * @param id the id
     */
    void setId(@NonNull UUID id) {
      this.id = id;
    }

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

    /**
     * That is an {@code instanceof} this concrete class.
     *
     * @param that the other object
     * @return the boolean
     * @see <a href="https://www.artima.com/articles/how-to-write-an-equality-method-in-java">
     *   How to Write an Equality Method in Java
     *   </a>
     */
    protected abstract boolean canEqual(@NonNull AbstractIdentity that);

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

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

[jqno]

So if I understand correctly, when the object is still "new" (i.e., not yet persisted), the id is null. If you have two such objects, you want equals to return false, but EqualsVerifier enforces that they be true.

I think you have a good point there, I'll see if I can make that work.

Do you think there's a use case for keeping them equal? If so, I'd have to introduce a new kind of Warning, and I'd prefer not to do that if I have the choice.

[xenoterracide]

Do you think there's a use case for keeping them equal? If so, I'd have to introduce a new kind of Warning, and I'd prefer not to do that if I have the choice.

if you don't you'll probably need to release a new major version as it would break backwards compatibility on peoples code in a subjective way.

Honestly, there probably is, but I don't know what it is. It's probably that people just don't share my opinion and it's hard for me to say for certain.

would you have to introduce a new kind of warning? or could you just introduce an alternate method to withNonnullFields that instead of ignoring null testing you would simply test that it's false instead of true.

[xenoterracide]

Also, I'm not certain how this impacts @Version, should 2 entities with the same id but different versions be equal? probably not... what if @Version is also nullable? it can be a date, so possibly?

I hate thinking of equality... because there's actually at least 2 different kinds, maybe 3.

[jqno]

OK, thanks. I'll think about how to approach this then.
In the mean time, if find any best practices or other supporting material, I'm open to that...

[xenoterracide]

ok, correct behavior id == null should not be considered equal. Java Equality is inappropriate for this case, and other methods should be used.

I've been documenting what I think are the various cases over in #935 and considering what Domain Driven Design would say.

The reality there is that there may not be a good way to say that 2 identical all the way down, new order line items aren't intentional from a DDD perspective.

I think the question for you though remains, what does changing this behavior mean? I new major version? how've you been dealing with this? Also it's probable that we'd need to add some better advice/examples of why.

I wonder if it would be appropriate to change (at least at the root) to change from only auto detection to having something like EqualsVerifier.forAggregate(), esp since hibernate isn't the only thing for this these days. Mongo (documents) have become very popular. Although this has a problem of how do you detect what's an entity, vs a value object as a property. :/ I'm not familliar with any ODM's for Java, and I don't recall what the OGM or SDN use for neo4j.

[jqno]

Turns out that EqualsVerifier already supports the case where you want equals to return false if the ids aren't initialized yet. You have to suppress Warning.IDENTICAL_COPY_FOR_VERSIONED_ENTITY, and in your case also Warning.SURROGATE_KEY. I've improved the error message surrounding this, and updated the manual, to reflect this.

And yes, I agree the names are awkward (at best). I wrote this bit many years ago, when I hadn't actually used Hibernate or JPA myself yet. Improving the API is already on my todo list, but it will take a while before I get to it.