Allow CustomValueComparators to handle nulls

[bartoszwalacik]

Currently, before calling a CustomValueComparator impl, Javers checks nulls and assumes that :

null != any non-null value

This assumption simplifies an impl, but from the other hand it
makes impossible to write a Comparator which treats nulls as empty values.
See https://stackoverflow.com/questions/66497652/javers-comparison-string-collection-and-boolean-with-defined-rules

public interface CustomValueComparator<T> {
    /**
     * Called by JaVers to compare two Values.
     *
     * @param a not null
     * @param b not null
     */
    boolean equals(T a, T b);

New feature
Skip the Javers internal null-safety logic and allow passing nulls to CustomValueComparators.

Unfortunately it could break existing users' code

Test case
CaseWithCustomComparatorBlejwas.groovy

[djavaphp]

Hi
do you have any update on this?
I want to ignore upcoming fields which is empty or null.
Is there any way to do?

[bartoszwalacik]

Hi, if you want to speed up things, feel free to contribute a PR

[bluebartle]

@bartoszwalacik - I'd be happy to raise a PR for this, but wanted to check if you had any thoughts on approach before I do.

Currently, what I am thinking is that I would modify CustomValueComparator to include a method to indicate whether wrapping with the CustomValueComparatorNullSafe should be skipped (with a default that returns false):

public interface CustomValueComparator<T> {
  
  // ...

  default boolean skipNullSafety() { return false; }
}

This could then be extended with an interface that clearly describes it's behaviour:

public interface NonNullSafeCustomValueComparator<T> extends CustomValueComparator<T> {

  @Override
  default boolean skipNullSafety() { return true; }
}

Then PrimitiveOrValueType can be modified to check this flag:

  PrimitiveOrValueType(Type baseJavaType, CustomValueComparator<T> comparator) {
    super(baseJavaType);
    this.valueComparator = (comparator == null || comparator.skipNullSafety()) ? comparator : new CustomValueComparatorNullSafe<>(comparator);
  }

That seems simple enough, as well as backwards compatible. I'm not sure if the new comparator interface should be advertised more clearly (e.g. through a new method on the builder, but that seems a bit bloating). An abstract class might be preferable to prevent a somewhat confusing further override of the flag.

Any thoughts or concerns?

Thanks very much for your work, by the way. Javers has saved me many, many hours of effort over recent months!

[bartoszwalacik]

Ok @ratskinmahoney, your design looks good, but maybe we can avoid double negations?

public interface CustomValueComparator<T> {
  
  // ...

  default boolean nullSafety() { return true; }
}

[bluebartle]

Thanks @bartoszwalacik - and good point. I'll make the naming more straightforward as suggested. I'll get a PR raised presently.

[bluebartle]

I've raised PR #1159. I've adjusted some of the naming so we refer to null handling, rather than null safety as it seemed to make things a bit clearer. Perfectly happy if we'd prefer to use the naming suggested above.

[bartoszwalacik]

fixed in 6.5.2