Scanned classes ignore custom value comparators for their properties

[catautobox]

Clear description of my expectations versus reality
Registering a scanned class holding a certain value type property (for example: BigDecimal) and registering a custom value comparator for that value type (for example BigDecimalComparatorWithFixedEquals) via JaversBuilder should make the scanned class use the custom value comparator for that property.

In reality the scanned class seems to use for its property the JaversTypes registered early from WellKnownValueTypes (for example: BigDecimal), ignoring the custom value comparator registered via JaversBuilder.

The value type itself (here: BigDecimal) seems to hold the correct custom value comparator.
But the value type for the type mapping for the scanned class property does not hold the correct custom value comparator (in fact, in the BigDecimal case it does not hold any custom comparator at all).

Steps To Reproduce
I have a **runnable test case ** which isolates the bug and allows Javers Core Team to easily reproduce it. I have pushed this test case to my fork of this repository:

https://github.com/catautobox/javers/blob/master/javers-core/src/test/groovy/org/javers/core/cases/JaversBuilderIgnoresCustomValueComparatorForValueTypeForScannedClass.groovy

Javers' Version
6.7.1
Earlier versions are probably affected too.

Additional context

If I'm not mistaken, in org.javers.core.JaversBuilder#assembleJaversInstance

addModule(new TypeMapperModule(getContainer()));

registers WellKnownValueTypes early,
then

        for (Class c : classesToScan){
            typeMapper().getJaversType(c);
        }

registers scanned classes (at which point their properties will share JaversTypes and custom comparators already registered at that point ),
then

mapRegisteredClasses();

registers actual custom comparators.

Registering a CustomValueComparator for a JaversType replaces the entry in the mappedTypes (here for BigDecimal itself), but the mapping for the scanned class property seems to still use a different, earlier instance of the JaveryType or CustomValueComparator.

The fact, that JaveryType mappings are being replaced makes custom value comparators "disappear" when registering custom value type adapters too:

https://github.com/catautobox/javers/blob/master/javers-core/src/test/groovy/org/javers/core/cases/JaversBuilderIgnoresCustomValueComparatorWhenRegisteringValueTypeAdapter.groovy

Maybe that's a symptom of a similar problem: custom value comparators and custom value type adapters both replacing previous JaversType mappings with unexpected side-effects, and there does not seem to be an easy way of associating an existing JaversType with one of them without having to explicitly (custom value comparator) or implicitly (custom value type adapter) replace the JaversType mapping.

[bartoszwalacik]

@catautobox thanks for the high quality report, did you consider contributing also a PR with the fix?

[catautobox]

@bartoszwalacik I could try a PR for sure, but I would like to clarify which approach you prefer:

• (A) Delay the registration of WellKnownValueTypes and treated them just as fallback definitions:
  • will be run as late as possible to include custom types, type-adapters and comparators
  • will only register a WellKnownValueType if not already registered as a custom value type
  • maybe I'm too optimistic and there is a reason why TypeMapperEngine registers them already in its constructor
• (B) Keep the original value type registration when registering type-adapters or comparators:
  • I'm not sure about the intention of currently discarding already registered value type definitions. It may have been a fix for something, especially because ValueDefinition.customValueComparator() was marked as deprecated.
  • ValueDefinition would become mutable
  • alternatively replace ValueDefinition with early, mutable protypes or builders, resulting in immutable ValueDefinitions after the JaversBuilder.build()
• (C) Upon type-adapter/comparator registration visit all registered types and replace the old ValueDefinition with the new one:
  • Requires recursion-detection
  • Will probably result in slower runtime than A/B, especially if this would get triggered for every individual registration.

As for now my favorite would be A, followed by B.

Which one should I try?
Also, which version / branch should I base my PR on?

[bartoszwalacik]

@catautobox that's a good research, you are right that WellKnownValueTypes should be registered at the and of the javers build process, as not to override custom types, specified by a user. Please go for option A. The reason why WellKnownValueTypes are registered at the beginning might be historical

[catautobox]

@bartoszwalacik Though option A may solve the original reported problem, the root problem (replacing value definitions) will still cause custom value comparators to get lost when registering custom value type adapters (or get not associated with them correctly), like shown in:
https://github.com/catautobox/javers/blob/master/javers-core/src/test/groovy/org/javers/core/cases/JaversBuilderIgnoresCustomValueComparatorWhenRegisteringValueTypeAdapter.groovy

In other words, registerValueTypeAdapter seems to have an unexpected side-effect.

If I'll go with option A, I could remedy the second problem probably by introducing an overloaded registerValue that will accept both: a custom value comparator and type adapter at the same time.

It's just a hunch, but I think a lot would get resolved if clientsClassDefinitions would not get overwritten, or, more specifically, if registerType would use putIfAbsent instead of put.

Given ClientsClassDefinition.equals seems to use the baseJavaClass for the equals/hashCode contract, then even if a registerValueTypeAdapter would wrap value types into ValueDefinition(s), avoiding putting into clientsClassDefinitions twice should be possible.

Still, internally there would be still a race condition when registerValueTypeAdapter would register ValueDefinition(s) with default comparators before registering custom comparators. In other words: not replacing ValueDefinition(s) anymore would mean ignoring custom comparators if they have been registered too late.

The more I think about it, the more it seems that JaversBuilder should hold builders for ClientsClassDefinitionfor pe-build mutability. This would have the best of both worlds: a flexible JaversBuilder and immutable ClientsClassDefinition(s).

But all in all, getting rid of that side-effect could cause some applications to break if they (even unknowingly) benefited from that side-effect. But that applies to option A too, at least to some degree.

I think at this point I'll play around with option A and/or B and see how they will affect tests in practice.

[bartoszwalacik]

@catautobox I like your reasoning and proposed solution. Indeed, the current impl of TypeMapper is not perfect and might be vulnerable for race condition. Side effects should be removed, even if it would break some Javers' clients apps. Feel free to contribute PR(s) to fix it

[catautobox]

@bartoszwalacik Please see my PR #1281
I've tried to keep the public interfaces as compatible as possible and just introduce exceptions in cases where an unexpected overwrite may happen.

[bartoszwalacik]

@catautobox PR is merged, thanks for your excellent contribution. Will release tomorrow.