Merge pull request #837 from jqno/entity-lazy-fields

Improve handling of lazy fields

CHANGELOG.md

@@ -13,6 +13,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Check that getters are used for all mapped fields in JPA entities, not just the ones with `FetchType.LAZY`. ([Issue 830](https://github.com/jqno/equalsverifier/issues/830))<br/>Note that this is a **breaking change** for JPA entity tests. This can be disabled by suppressing `Warning.JPA_GETTER`. See the [manual page about JPA entities](/equalsverifier/manual/jpa-entities), specifically the section on Materialized fields, for more details.
+
+### Added
+
+- `#withFieldnameToGetterConverter()` to override the derivation of getter names from field names when testing JPA entities. ([Issue 829](https://github.com/jqno/equalsverifier/issues/829))
+- `Warning.JPA_GETTER` to suppress the check that getters should be used instead of direct field references in JPA entities.
+
+
 ## [3.14.3] - 2023-06-23
 
 ### Fixed

docs/_errormessages/jpa-direct-reference-instead-of-getter.md

@@ -0,0 +1,36 @@
+---
+title: "JPA Entity: direct reference to field ... used in equals instead of getter"
+---
+For fields with a mapping annotation like `@OneToMany`, `@ManyToOne` or `@ManyToMany`, you might run into this error:
+
+    JPA Entity: direct reference to field employee used in equals
+      instead of getter getEmployee.
+
+This error occurs when the field is used directly in `equals` or `hashCode`:
+
+{% highlight java %}
+@ManyToOne
+private Employee employee;
+
+public boolean equals(Object other) {
+    // ...
+    return Objects.equals(employee, that.employee);
+}
+{% endhighlight %}
+
+This is problematic, because the field `employee` might not be materialized yet. In other words, JPA may not have queried the `employee` yet and the reference could still be null. This might lead to incorrect results when calling `equals` or `hashCode`. JPA will materialize the field when the getter is called, but not when the field is referenced directly. Therefore, the field should always be referenced through its getter, in `equals` and `hashCode`:
+
+{% highlight java %}
+public boolean equals(Object other) {
+    // ...
+    return Objects.equals(getEmployee(), that.getEmployee());
+}
+
+public int hashCode() {
+    return Objects.hash(getEmployee());
+}
+{% endhighlight %}
+
+If you have a reason, you can disable this check by suppressing `Warning.JPA_GETTER`. Also, EqualsVerifier assumes you use the JavaBeans convention to name your fields and getters. If you use a different convention, you can use `#withFieldnameToGetterConverter()` to override that.
+
+See the [manual page about JPA entities](/equalsverifier/manual/jpa-entities), specifically the section on Materialized fields, for more details.

docs/_manual/09-jpa-entities.md

@@ -54,6 +54,29 @@ In that case, you can call `suppress(Warning.IDENTICAL_COPY_FOR_VERSIONED_ENTITY
 (`Warning.IDENTICAL_COPY`, which the error message suggests, is not appropriate in this case because that is meant for classes which have no state at all.)
 
 
+### Materialized fields
+Some fields have a mapping annotation that links them with data from a different database table or entity. These annotations include `@OneToMany`, `@ManyToOne` and `@ManyToMany`. In certain situations, you can have an instance where these fields are not materialized yet. In other words, they're not fetched from the database, and their content is undefined. Most often, this happens when they have `fetchType = FetchType.LAZY`, but even with `FetchType.EAGER`, it can happen that they are not yet materialized. This also applies to fields with `@Basic(fetchType = FetchType.LAZY)`. JPA will materialize this data on demand. For example, when the getter for such a field is called, JPA is triggered and queries the data. However, this trigger does not happen when the field is referenced directly.
+
+Therefore, when these fields are used in `equals` and `hashCode`, it's important to call their getter method instead of referencing the field directly. Otherwise, the data may not be materialized, and it's possible that calling `equals` on two equal objects returns `false`, because one instance doesn't have the content yet while the other does.
+
+EqualsVerifier checks for these fields that their getter is used. If they're referenced directly, EqualsVerifier will fail. Note that this can be disabled by suppressing `Warning.JPA_GETTER`.
+
+By default, EqualsVerifier assumes that the JavaBeans conventions are used to determine the name of the getter. For example, if a field is called `employee`, it assumes that the getter is called `getEmployee()`. If your project uses a different convention, you can use `#withFieldnameToGetterConverter()` to override that behavior.
+
+For example, if in your project, a field must have a prefix, like so: `m_employee`, but the getter is still `getEmployee()`, you might call EqualsVerifier like this:
+
+{% highlight java %}
+EqualsVerifier
+    .forClass(Foo.class)
+    .withFieldnameToGetterConverter(
+        fn -> "get" + Character.toUpperCase(fn.charAt(2)) + fn.substring(3)
+    )
+    .verify();
+{% endhighlight %}
+
+This will chop off the `m_` prefix, uppercase the first letter, and prepend the word `get`.
+
+
 ### Transient fields
 Since fields marked with the `@Transient` annotation are not persisted, they should generally not participate in `equals` and `hashCode` either. Therefore, EqualsVerifier will implicitly call [`withIgnoredFields`](/equalsverifier/manual/ignoring-fields) for these fields.
 

docs/_pages/errormessages.md

@@ -24,6 +24,7 @@ This is not a complete list. I'll add to it as needed, so if you need help with
 * [Coverage is not 100%](/equalsverifier/errormessages/coverage-is-not-100-percent)
 * [Double: equals doesn't use Double.compare for field foo](/equalsverifier/errormessages/double-equals-doesnt-use-doublecompare-for-field-foo)
 * [Float: equals doesn't use Float.compare for field foo](/equalsverifier/errormessages/float-equals-doesnt-use-floatcompare-for-field-foo)
+* [JPA Entity: direct reference to field ... used instead of getter](/equalsverifier/errormessages/jpa-direct-reference-instead-of-getter)
 * [Mutability: equals depends on mutable field](/equalsverifier/errormessages/mutability-equals-depends-on-mutable-field)
 * [NoClassDefFoundError](/equalsverifier/errormessages/noclassdeffounderror)
 * [Non-nullity: equals/hashCode/toString throws NullPointerException](/equalsverifier/errormessages/non-nullity-equals-hashcode-tostring-throws-nullpointerexception)

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/ConfiguredEqualsVerifier.java

@@ -3,6 +3,7 @@
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
+import java.util.function.Function;
 import nl.jqno.equalsverifier.Func.Func1;
 import nl.jqno.equalsverifier.Func.Func2;
 import nl.jqno.equalsverifier.api.EqualsVerifierApi;
@@ -20,21 +21,24 @@ public final class ConfiguredEqualsVerifier implements EqualsVerifierApi<Void> {
     private final EnumSet<Warning> warningsToSuppress;
     private final FactoryCache factoryCache;
     private boolean usingGetClass;
+    private Function<String, String> fieldnameToGetter;
 
     /** Constructor. */
     public ConfiguredEqualsVerifier() {
-        this(EnumSet.noneOf(Warning.class), new FactoryCache(), false);
+        this(EnumSet.noneOf(Warning.class), new FactoryCache(), false, null);
     }
 
     /** Private constructor. For internal use only. */
     private ConfiguredEqualsVerifier(
         EnumSet<Warning> warningsToSuppress,
         FactoryCache factoryCache,
-        boolean usingGetClass
+        boolean usingGetClass,
+        Function<String, String> fieldnameToGetter
     ) {
         this.warningsToSuppress = warningsToSuppress;
         this.factoryCache = factoryCache;
         this.usingGetClass = usingGetClass;
+        this.fieldnameToGetter = fieldnameToGetter;
     }
 
     /**
@@ -46,7 +50,8 @@ public ConfiguredEqualsVerifier copy() {
         return new ConfiguredEqualsVerifier(
             EnumSet.copyOf(warningsToSuppress),
             new FactoryCache().merge(factoryCache),
-            usingGetClass
+            usingGetClass,
+            fieldnameToGetter
         );
     }
 
@@ -91,6 +96,14 @@ public ConfiguredEqualsVerifier usingGetClass() {
         return this;
     }
 
+    @Override
+    public ConfiguredEqualsVerifier withFieldnameToGetterConverter(
+        Function<String, String> converter
+    ) {
+        this.fieldnameToGetter = converter;
+        return this;
+    }
+
     /** {@inheritDoc} */
     @Override
     public ConfiguredEqualsVerifier withResetCaches() {
@@ -110,7 +123,8 @@ public <T> SingleTypeEqualsVerifierApi<T> forClass(Class<T> type) {
             type,
             EnumSet.copyOf(warningsToSuppress),
             factoryCache,
-            usingGetClass
+            usingGetClass,
+            fieldnameToGetter
         );
     }
 

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/Warning.java

@@ -165,6 +165,19 @@ public enum Warning {
      */
     SURROGATE_OR_BUSINESS_KEY,
 
+    /**
+     * Disables the check that collection fields in JPA, or @Basic fields marked with
+     * FetchType.LAZY, should be accessed through their getter methods in {@code equals} and
+     * {@code hashCode} methods.
+     *
+     * <p>Normally, it is necessary to go through the getter for these fields, because their
+     * content may not be materialized in some instances. Calling the getter will materialize them,
+     * but referencing the field directly will not. This can lead to situations where the
+     * {@code equals} method of objects that should be equal to each other returns false, because
+     * one instance has the content materialized and the other does not.
+     */
+    JPA_GETTER,
+
     /**
      * Disables the check that transient fields not be part of the {@code equals} contract.
      *

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/api/EqualsVerifierApi.java

@@ -1,5 +1,6 @@
 package nl.jqno.equalsverifier.api;
 
+import java.util.function.Function;
 import nl.jqno.equalsverifier.EqualsVerifier;
 import nl.jqno.equalsverifier.Func.Func1;
 import nl.jqno.equalsverifier.Func.Func2;
@@ -70,6 +71,19 @@ public interface EqualsVerifierApi<T> {
      */
     EqualsVerifierApi<T> usingGetClass();
 
+    /**
+     * Determines how a getter name can be derived from a field name.
+     *
+     * The default behavior is to uppercase the field's first letter and prepend 'get'. For
+     * instance, a field name 'employee' would correspond to getter name 'getEmployee'.
+     *
+     * This method can be used if your project has a different naming convention.
+     *
+     * @param converter A function that converts from field name to getter name.
+     * @return {@code this}, for easy method chaining.
+     */
+    EqualsVerifierApi<T> withFieldnameToGetterConverter(Function<String, String> converter);
+
     /**
      * Signals that all internal caches need to be reset. This is useful when the test framework
      * uses multiple ClassLoaders to run tests, causing {@link java.lang.Class} instances

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/api/MultipleTypeEqualsVerifierApi.java

@@ -2,6 +2,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import nl.jqno.equalsverifier.ConfiguredEqualsVerifier;
@@ -70,6 +71,15 @@ public MultipleTypeEqualsVerifierApi usingGetClass() {
         return this;
     }
 
+    /** {@inheritDoc} */
+    @Override
+    public MultipleTypeEqualsVerifierApi withFieldnameToGetterConverter(
+        Function<String, String> converter
+    ) {
+        ev.withFieldnameToGetterConverter(converter);
+        return this;
+    }
+
     /** {@inheritDoc} */
     @Override
     public MultipleTypeEqualsVerifierApi withResetCaches() {

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/api/SingleTypeEqualsVerifierApi.java

@@ -1,16 +1,38 @@
 package nl.jqno.equalsverifier.api;
 
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.function.Function;
 import nl.jqno.equalsverifier.EqualsVerifier;
 import nl.jqno.equalsverifier.EqualsVerifierReport;
 import nl.jqno.equalsverifier.Func.Func1;
 import nl.jqno.equalsverifier.Func.Func2;
 import nl.jqno.equalsverifier.Warning;
-import nl.jqno.equalsverifier.internal.checkers.*;
+import nl.jqno.equalsverifier.internal.checkers.AbstractDelegationChecker;
+import nl.jqno.equalsverifier.internal.checkers.CachedHashCodeChecker;
+import nl.jqno.equalsverifier.internal.checkers.Checker;
+import nl.jqno.equalsverifier.internal.checkers.ExamplesChecker;
+import nl.jqno.equalsverifier.internal.checkers.FieldsChecker;
+import nl.jqno.equalsverifier.internal.checkers.HierarchyChecker;
+import nl.jqno.equalsverifier.internal.checkers.MapEntryHashCodeRequirementChecker;
+import nl.jqno.equalsverifier.internal.checkers.NullChecker;
+import nl.jqno.equalsverifier.internal.checkers.RecordChecker;
+import nl.jqno.equalsverifier.internal.checkers.SignatureChecker;
 import nl.jqno.equalsverifier.internal.exceptions.MessagingException;
 import nl.jqno.equalsverifier.internal.prefabvalues.FactoryCache;
-import nl.jqno.equalsverifier.internal.util.*;
+import nl.jqno.equalsverifier.internal.util.CachedHashCodeInitializer;
+import nl.jqno.equalsverifier.internal.util.Configuration;
+import nl.jqno.equalsverifier.internal.util.ErrorMessage;
+import nl.jqno.equalsverifier.internal.util.FieldNameExtractor;
 import nl.jqno.equalsverifier.internal.util.Formatter;
+import nl.jqno.equalsverifier.internal.util.ObjenesisWrapper;
+import nl.jqno.equalsverifier.internal.util.PrefabValuesApi;
+import nl.jqno.equalsverifier.internal.util.Validations;
 
 /**
  * Helps to construct an {@link EqualsVerifier} test with a fluent API.
@@ -29,6 +51,7 @@ public class SingleTypeEqualsVerifierApi<T> implements EqualsVerifierApi<T> {
     private FactoryCache factoryCache = new FactoryCache();
     private CachedHashCodeInitializer<T> cachedHashCodeInitializer =
         CachedHashCodeInitializer.passthrough();
+    private Function<String, String> fieldnameToGetter = null;
     private Set<String> allExcludedFields = new HashSet<>();
     private Set<String> allIncludedFields = new HashSet<>();
     private Set<String> nonnullFields = new HashSet<>();
@@ -54,17 +77,20 @@ public SingleTypeEqualsVerifierApi(Class<T> type) {
      * @param factoryCache Factories that can be used to create values.
      * @param usingGetClass Whether {@code getClass} is used in the implementation of the {@code
      *     equals} method, instead of an {@code instanceof} check.
+     * @param converter A function that converts from field name to getter name.
      */
     public SingleTypeEqualsVerifierApi(
         Class<T> type,
         EnumSet<Warning> warningsToSuppress,
         FactoryCache factoryCache,
-        boolean usingGetClass
+        boolean usingGetClass,
+        Function<String, String> converter
     ) {
         this(type);
         this.warningsToSuppress = EnumSet.copyOf(warningsToSuppress);
         this.factoryCache = this.factoryCache.merge(factoryCache);
         this.usingGetClass = usingGetClass;
+        this.fieldnameToGetter = converter;
     }
 
     /**
@@ -129,6 +155,15 @@ public SingleTypeEqualsVerifierApi<T> usingGetClass() {
         return this;
     }
 
+    /** {@inheritDoc} */
+    @Override
+    public SingleTypeEqualsVerifierApi<T> withFieldnameToGetterConverter(
+        Function<String, String> converter
+    ) {
+        this.fieldnameToGetter = converter;
+        return this;
+    }
+
     /**
      * Signals that all given fields are not relevant for the {@code equals} contract. {@code
      * EqualsVerifier} will not fail if one of these fields does not affect the outcome of {@code
@@ -396,6 +431,7 @@ private Configuration<T> buildConfig() {
             redefinedSubclass,
             usingGetClass,
             warningsToSuppress,
+            fieldnameToGetter,
             factoryCache,
             ignoredAnnotationClassNames,
             actualFields,

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/internal/checkers/FieldsChecker.java

@@ -95,7 +95,10 @@ public void check() {
         }
 
         AnnotationCache cache = config.getAnnotationCache();
-        if (cache.hasClassAnnotation(config.getType(), SupportedAnnotations.ENTITY)) {
+        if (
+            cache.hasClassAnnotation(config.getType(), SupportedAnnotations.ENTITY) &&
+            !config.getWarningsToSuppress().contains(Warning.JPA_GETTER)
+        ) {
             inspector.check(jpaLazyGetterFieldCheck);
         }
     }

equalsverifier-core/src/main/java/nl/jqno/equalsverifier/internal/checkers/fieldchecks/JpaLazyGetterFieldCheck.java

@@ -5,6 +5,7 @@
 import static nl.jqno.equalsverifier.internal.util.Assert.assertTrue;
 
 import java.util.Set;
+import java.util.function.Function;
 import nl.jqno.equalsverifier.internal.exceptions.EqualsVerifierInternalBugException;
 import nl.jqno.equalsverifier.internal.prefabvalues.PrefabValues;
 import nl.jqno.equalsverifier.internal.prefabvalues.TypeTag;
@@ -24,13 +25,15 @@ public class JpaLazyGetterFieldCheck<T> implements FieldCheck<T> {
     private final Set<String> ignoredFields;
     private final PrefabValues prefabValues;
     private final AnnotationCache annotationCache;
+    private final Function<String, String> fieldnameToGetter;
 
     public JpaLazyGetterFieldCheck(Configuration<T> config) {
         this.type = config.getType();
         this.accessor = config.getClassAccessor();
         this.ignoredFields = config.getIgnoredFields();
         this.prefabValues = config.getPrefabValues();
         this.annotationCache = config.getAnnotationCache();
+        this.fieldnameToGetter = config.getFieldnameToGetter();
     }
 
     @Override
@@ -40,8 +43,7 @@ public void execute(
         FieldAccessor fieldAccessor
     ) {
         String fieldName = fieldAccessor.getFieldName();
-        String getterName =
-            "get" + Character.toUpperCase(fieldName.charAt(0)) + fieldName.substring(1);
+        String getterName = fieldnameToGetter.apply(fieldName);
 
         if (ignoredFields.contains(fieldName) || !fieldIsLazy(fieldAccessor)) {
             return;
@@ -71,10 +73,17 @@ public void execute(
     }
 
     private boolean fieldIsLazy(FieldAccessor fieldAccessor) {
-        return annotationCache.hasFieldAnnotation(
-            type,
-            fieldAccessor.getFieldName(),
-            SupportedAnnotations.LAZY_FIELD
+        return (
+            annotationCache.hasFieldAnnotation(
+                type,
+                fieldAccessor.getFieldName(),
+                SupportedAnnotations.JPA_LINKED_FIELD
+            ) ||
+            annotationCache.hasFieldAnnotation(
+                type,
+                fieldAccessor.getFieldName(),
+                SupportedAnnotations.JPA_LAZY_FIELD
+            )
         );
     }