EqualsVerifier should check for field usage

[Euklios]

Is your feature request related to a problem? Please describe.
Hibernate does sometimes initialize objects lazily via a proxy.
It can be nice; however, the comparison will use invalid data if the equals method accesses the fields directly rather than using getters.

The result will always be false if we compare a lazy-loaded object against an eagerly loaded object.
If we compare two lazy-loaded objects against each other, the result is presumably always true.

Describe the solution you'd like
It would be nice to have a test as follows:

1. Create a regular red object (set fields via reflection)
2. Create a subclass and override the getter methods
3. Instantiate a blue class using the subclass (return blue values in the getter, fields should be null)
4. Call equals -> Expect true

Describe alternatives you've considered
An alternative would be creating a subclass and verifying that the getters are called.

Additional context
Here a sample equals-method to demonstrate the problem:

public final boolean equals(Object o) {
  if (this == o) {
    return true;
  }

  if (o instanceof Model other) {
     return other.canEquals(this) && Objects.equals(this.field, other.field);
  }

  return false;
}

[jqno]

Hi! Thanks for opening this issue.

I'm not too familiar with Hibernate, so I don't completely understand what you're asking for. Can you explain to me how these lazy-loaded objects work? And how it can be that they can equal each other but not identical non-lazy objects? Is the equals method you posted from a lazy or a non-lazy entity?

The more information you give me the better, I have to work on this on my spare time and I don't have a lot of it. Every minute spent researching is a minute spent not coding :)

[Euklios]

@jqno Sorry for the late reply. Didn't notice the notification.

To be precise: It is more an issue with Hibernate in combination with JPA.
Hibernate allows for multiply different DB access patterns, not all of them are affected by this issue.
However, spring boot and JPA are designed in such a way, that this issue can occur quite frequently if the equals method is not properly designed.

Given a simple Book table, the corresponding model in Java would look like this:
(Some code has been omitted for brevity).

class Book {
  private int id;
  private String isbn;
  private String title;

  public String getIsbn() {
    return this.isbn;
  }

  public String getTitle() {
    return this.title;
  }

  public boolean equals(Object o) {
    ...
    return Object.equals(this.isbn, other.isbn) && Object.equals(this.title, other.isbn);
  }

  public boolean equals2(Object o) {
    ...
    return Object.equals(this.getIsbn(), other.getIsbn()) && Object.equals(this.getTitle(), other.getIsbn());
  }
}

Please note the equals2 method, it will be relevant down the line.

Additionally, we have a JPA repository interface that looks like this:

@Repository
interface BookRepository extends CrudRepository<BookModel, Integer> {
// This will be implemented by JPA/Hibernate. The corresponding query would be something like this:
// SELECT book FROM BookModel book WHERE book.isbn = ?1 AND book.title = ?2
BookModel findBookByIsbnAndTitle(String isbn, String title);
}

This interface will be implemented by JPA and used in further snippets by the variable name bookRepository.
However, this is just an example.
The method findBookByIsbnAndTitle stands for something that should return a model that is equal to the other, locally instantiated book, but is instantiated by Hibernate directly.

Now, given this simple model, we can assume the following two lines to be true:

new Book("isbn", "title").equals(new Book("isbn", "title"));
bookRepository.findBookByIsbnAndTitle("isbn", "title").equals(new Book("isbn", "title"));

The main problem however is the lazy-loading feature.
This will cause Hibernate to instantiate an empty object, with the most minimal information that allows retrieving the given object (usual identifier) and wraps the thing in a proxy (or they use a subclass, more likely given the behavior).
All getters would be wrapped by some custom "magic", that looks something like this (greatly simplified):

public String getTitle() {
  if (!this.isLoaded()) {
    // Load the original model by id and publish the fields locally 
    this.loadPropertiesFromDb();
    this.setLoaded(true);
  }
  return super.getTitle();
}

In terms of memory representation, the object fetched by Hibernate would look like this:

Book:
  id: 5
  isbn: null
  title: null

while the locally created instance would look like this

Book:
  id: null
  isbn: "isbn"
  title: "title"

The important bit about this information is, that the book fetched by Hibernate references a that does equal the locally created book. However, until the first call to any getter, the information will not be represented within the objects' fields.
Therefore, the original equals method will return false, as the following comparison would be made:

Objects.equals(this.isbn, other.isbn) && Objects.equals(this.title, other.title);
// Results in following test:
Objects.equals(null, "isbn") && Objects.equals(null, "title");

While on the other hand, equals2 would return true, as the first getter would publish the locally created fields:

Objects.equals(this.getIsbn(), other.getIsbn()) && Objects.equals(this.getTitle(), other.getTitle());
// First call to getIsbn() causes Hibernate to load the actual data.
// Therefore, the comparison would look like this:
Objects.equals("isbn", "isbn") && Objects.equals("title", "title");

I hope this explanation of the process is understandable, otherwise please feel free to contact me with any further questions.
For the one question I didn't address during my attempt at explaining Hibernate lazy loading:
As far as I could tell, Hibernate passes calls to equals directly to the original method.

[Euklios]

The simplest approach at testing this problem dynamically would be something like this:

BookModel instance = new BookModel("isbn", "title");

class DynamicSubClass extends BookModel {
  private BookModel _delegate;

  public String getIsbn() {
    return this._delegate.getIsbn();
  }

  public String getTitle() {
    return this._delegate.getTitle();
  }
}

final var DynamicSubClass lazyObject = new DynamicSubClass();
lazyObject.set_delegate(bookModel);

assertEquals(lazyObject, bookModel);

If the test fails, at least some of the equals method uses fields directly.
This could be made more advanced by testing field by field, but I don't think that is necessary.

[jqno]

Thanks for the clarification! I think I understand what you mean.

So if I understand you correctly, the issue is not that EqualsVerifier currently gives incorrect answers to a class that you give it. The issue is that sometimes at runtime, an object may not be fully loaded yet, and if the equals method was implemented using fields, the results will sometimes be incorrect because the fields are still null.

The correct way to write an equals method when dealing with lazy loading, is that you use the getters instead of the fields, and what you're asking is that we add a check to see if this is actually how the equals method is implemented, am I correct? So the new check will fail if equals is implemented using fields, and succeed when equals is implemented using getters.

What EqualsVerifier needs to do then, is 1) detect if a field is loaded lazily or eagerly, and 2) if lazy, check that the getter is used instead of the field itself.

Regarding 1, I've been reading up a bit, and I understand that eager loading is the default, and you can define lazy loading using an annotation on the field: @ManyToOne(fetch = FetchType.LAZY). Is it also possible to define this on fields that aren't a foreign key relation of some kind? And is there also a way to define it for a class as a whole, or is it always defined per field?

Regarding 2, I'm not 100% sure yet if this is even feasible (bytecode manipulation is really hard and so far I've been able to avoid doing it myself).

Looks like a cool challenge, but one that will take a while to complete 😅

[Euklios]

Yes, the equals verifier behaves precisely as I'd expect it to; this is just intended as an extension.

Regarding 1, there are multiple annotations to define lazy loading.
Here are the ones I'm aware of:

• javax.persistence.ManyToOne
• javax.persistence.ManyToMany
• javax.persistence.OneToMany
• javax.persistence.OneToOne
• javax.persistence.Basic
• javax.persistence.ElementCollection

The list is compiled based on what I've used before and the content of the javax.persistence package.
None of them target classes, therefore we should be relatively safe on that front.

The main problem I see on this front would be the ways Hibernate accepts configuration.
Hibernate allows XML-based configuration, which also allows setting the lazy flag.
I personally feel like parsing the XML and mapping the content to classes would be overkill.
It could be interesting for the future, but the primary way I've encountered lazy loading is using JPA and the javax-persistence annotations.

This might be entirely biased, but I'd say we should focus on the annotations first and maybe add something like .withLazyFields(...) or ..withAllFieldsLazy().

Regarding 2, According to this Baeldung article Hibernate uses a dynamic proxy.
I created a quick test example and failed to forget that Javas Proxy.newProxyInstance only works with interfaces.
In summary: It's not possible without some bytecode manipulation, but it is surprisingly easy to do with javassist.
Will push and provide a link to a sample repository.

My approach would be to emulate lazy loaded fields instead of detecting getter usage over direct property access.
However, both checks should be relatively easy to implement.

Small side note:
We could also use cglib or another implementation instead of javassist.
The only reason why I've used javassist is that it was the first StackOverflow entry

[Euklios]

POC pushed here

Notes:

• Relevant part is in src/test/java
• There are two models provided, one using getters and one using property access
• I've added some comments explaining how it all works; please feel free to ask if something is unclear
• The implementation is kept fairly minimal; it only serves as proof of concept.
• The property test fails intentionally, emulating the intended behavior.

[jqno]

Thanks once again for the elaborate reply and the PoC 🙂

Re 1, that's a lot of annotations, but that shouldn't be a problem. Is it true that on 6 each case, the thing to look for is fetch = FetchType.LAZY? Or is it sometimes a different field and/or value?

Re 2, since EqualsVerifier already uses ByteBuddy, I figure it's best to use that. Saves a dependency...

I'm still thinking of ways to keep things as simple as possible though. Maybe EqualsVerifier can generate for each lazy property an overridden getter that throws a specific exception, and then it should fail of the exception isn't thrown. It's not pretty but it's the least complicated thing I can think of right now...

[jqno]

I've just released EqualsVerifier 3.12, which checks that getters are used for lazy-loaded fields. Let me know if it works for you!