Skip to content

ReflectionUtil.calculateHierarchyDistance should return superinterfaces - #1369

Closed
SimoneGiusso wants to merge 1 commit into
javers:masterfrom
SimoneGiusso:issue/916
Closed

ReflectionUtil.calculateHierarchyDistance should return superinterfaces#1369
SimoneGiusso wants to merge 1 commit into
javers:masterfrom
SimoneGiusso:issue/916

Conversation

@SimoneGiusso

@SimoneGiusso SimoneGiusso commented Mar 22, 2024

Copy link
Copy Markdown

This is related to this issue. Actually we noticed this problem when we upgraded our project from Java 17 to Java 21.

In fact the call to Javers#compare on objects which have a property of Type SortedMap (assigned to TreeMap concrete class) fails in Java 21 with an error similar to:

JaversException MANAGED_CLASS_MAPPING_ERROR: given javaClass ‘class java.util.TreeMap’ is mapped to MapType, expected ManagedType

While in Java 17, it works.

The reason is that the mapping between SortedMap and JaversType changes when using the two Java versions:

  • in Java 17 SortedMap is mapped to MapType (not a ManagedType) ✅
  • in Java 21 SortedMap is mapped to ValueObjectType (subclass of ManagedType) ❌

This is the issue SortedMap is not correctly mapped when using Java 21.

Full stack error:

Caused by: org.javers.common.exception.JaversException: MANAGED_CLASS_MAPPING_ERROR: given javaClass ‘class java.util.TreeMap’ is mapped to MapType, expected ManagedType
at org.javers.core.metamodel.type.TypeMapper.getJaversManagedType(TypeMapper.java:178)
at org.javers.core.metamodel.type.TypeMapper.getJaversManagedType(TypeMapper.java:161)
at org.javers.core.metamodel.object.GlobalIdFactory.createId(GlobalIdFactory.java:45)
at org.javers.core.graph.LiveCdoFactory.create(LiveCdoFactory.java:37)
at org.javers.core.graph.EdgeBuilder.buildSingleEdge(EdgeBuilder.java:33)
at org.javers.core.graph.ObjectGraphBuilder.buildSingleEdges(ObjectGraphBuilder.java:107)
at org.javers.core.graph.ObjectGraphBuilder.buildEdges(ObjectGraphBuilder.java:97)
at org.javers.core.graph.ObjectGraphBuilder.buildGraphFromCdo(ObjectGraphBuilder.java:65)
at org.javers.core.graph.ObjectGraphBuilder.buildGraph(ObjectGraphBuilder.java:54)
at org.javers.core.graph.LiveGraphFactory.createLiveGraph(LiveGraphFactory.java:38)
at org.javers.core.diff.DiffFactory.buildGraph(DiffFactory.java:102)
at org.javers.core.diff.DiffFactory.compare(DiffFactory.java:55)
at org.javers.core.JaversCore.compare(JaversCore.java:176)

P.S. The message exception, as it is written, is due to the fact that, at first the SortedMap property is considered as ManagedType as explained above. Therefore the call to ObjectGraphBuilder.buildSingleEdges(ObjectGraphBuilder.java:107) with this property. However since the instance is of TreeMap, when retrieving the JaversManagedType at TypeMapper.getJaversManagedType(TypeMapper.java:170) ,right before the exception, it returns MapType.

It may be related to the following issue as well:

#1090

Comment on lines +242 to +252
Class<?>[] currentInterfaces = current.getInterfaces();
while (currentInterfaces.length != 0) {
for (final Class<?> i : currentInterfaces) {
if (!interfaces.contains(i)) {
interfaces.add(i);
}
}

currentInterfaces = Arrays.stream(currentInterfaces)
.flatMap(currentInterface -> Arrays.stream(currentInterface.getInterfaces()))
.toArray(Class<?>[]::new);

@SimoneGiusso SimoneGiusso Mar 22, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Java 21 SortedMap doesn't directly extend the Map interface. Instead it extends SequencedMap and SequencedMap extends Map. Given an interface or class, getInterfaces(), returns only the interfaces directly implemented. At the end of this method the interfaces variable should contains also the possible super interfaces.

If we do this, SortedMap in Java 21 will "see" its super interface Map and consequently mapped to the MapType since the mapping Map -> MapType is one of the entry used to initialize the TypeMapperEngine here.

def "should calculate hierarchy distance as follows (parents first, interfaces last)"() {
expect:
ReflectionUtil.calculateHierarchyDistance(HashMap) == [AbstractMap, Map, Cloneable, Serializable]
ReflectionUtil.calculateHierarchyDistance(TreeMap) == [AbstractMap, NavigableMap, Cloneable, Serializable, SortedMap, Map]

@SimoneGiusso SimoneGiusso Mar 22, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the old implementation SortedMap is not part of the output here as we do not iterate the parent interfaces as we do for classes.

@bartoszwalacik

Copy link
Copy Markdown
Member

@SimoneGiusso thanks for the detailed bug report, I will do CR of your PR

@bartoszwalacik

bartoszwalacik commented Mar 30, 2024

Copy link
Copy Markdown
Member

Unfortunately, I can't confirm your bug report and solution , which version of JVM do you use exactly?

I tested on Corretto 21, and the mapping works as expected, your test is passing on ReflectionUtil from master

Screenshot 2024-03-30 at 18 37 25

@SimoneGiusso

SimoneGiusso commented Mar 30, 2024

Copy link
Copy Markdown
Author

Unfortunately, I can't confirm your bug report and solution , which version of JVM do you use exactly?

I tested on Corretto 21, and the mapping works as expected, your test is passing on ReflectionUtil from master

Screenshot 2024-03-30 at 18 37 25

I'm using temurin but it should not make any difference. Can you please make sure you are running Java 21? Try to print out this please:

System.getProperty("java.version")

@bartoszwalacik

Copy link
Copy Markdown
Member

You are right, btw can you give me a hint how to run a test on higher Java version than specified in build.gradle?
Whatever I do, Gradle runs a test on JVM version from build.gradle :

java {
    toolchain {
        languageVersion = JavaLanguageVersion.of(11)
    }
}

@bartoszwalacik

Copy link
Copy Markdown
Member

ok, got it
Screenshot 2024-03-30 at 20 26 13

@bartoszwalacik

Copy link
Copy Markdown
Member

merged #1371

@bartoszwalacik

Copy link
Copy Markdown
Member

the issue is fixed, released in 7.4.2
Thanks @SimoneGiusso !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants