From f9b8b97792b93c758e5bf53fbaa21f48b4b9db9c Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Wed, 21 Aug 2019 14:42:11 +0200 Subject: [PATCH] GH-656 - Correctly discover and handle typed and parameterized fields. Relationships can either live both in a typed and a parameterized fields. This changes the field detection algorithm in FieldsInfo to take of both scenarios and closes #656. --- .../org/neo4j/ogm/metadata/FieldInfo.java | 3 +- .../org/neo4j/ogm/metadata/FieldsInfo.java | 36 ++++---------- .../ogm/metadata/reflect/GenericUtils.java | 17 +++++-- .../org/neo4j/ogm/domain/gh656/Group.java | 48 +++++++++++++++++++ .../neo4j/ogm/domain/gh656/GroupVersion.java | 42 ++++++++++++++++ .../ogm/domain/gh656/VersionedEntity.java | 40 ++++++++++++++++ .../ogm/metadata/GenericsFieldsTest.java | 41 ++++++++++------ .../metadata/TestMetaDataTypeResolution.java | 13 ++++- .../model/RelationshipMappingTest.java | 42 +++++++++++++++- .../SessionDelegateIntegrationTest.java | 27 +---------- 10 files changed, 233 insertions(+), 76 deletions(-) create mode 100644 test/src/test/java/org/neo4j/ogm/domain/gh656/Group.java create mode 100644 test/src/test/java/org/neo4j/ogm/domain/gh656/GroupVersion.java create mode 100644 test/src/test/java/org/neo4j/ogm/domain/gh656/VersionedEntity.java diff --git a/core/src/main/java/org/neo4j/ogm/metadata/FieldInfo.java b/core/src/main/java/org/neo4j/ogm/metadata/FieldInfo.java index 319f98bbe9..4255dcf621 100644 --- a/core/src/main/java/org/neo4j/ogm/metadata/FieldInfo.java +++ b/core/src/main/java/org/neo4j/ogm/metadata/FieldInfo.java @@ -191,8 +191,7 @@ public ObjectAnnotations getAnnotations() { public boolean persistableAsProperty() { return PRIMITIVES.contains(descriptor) - || (AUTOBOXERS.contains(descriptor) && typeParameterDescriptor == null) - || (typeParameterDescriptor != null && AUTOBOXERS.contains(typeParameterDescriptor)) + || AUTOBOXERS.contains(getTypeDescriptor()) || propertyConverter != null || compositeConverter != null; } diff --git a/core/src/main/java/org/neo4j/ogm/metadata/FieldsInfo.java b/core/src/main/java/org/neo4j/ogm/metadata/FieldsInfo.java index 436d681551..120b6493ae 100644 --- a/core/src/main/java/org/neo4j/ogm/metadata/FieldsInfo.java +++ b/core/src/main/java/org/neo4j/ogm/metadata/FieldsInfo.java @@ -18,13 +18,11 @@ */ package org.neo4j.ogm.metadata; -import java.lang.annotation.Annotation; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.lang.reflect.TypeVariable; -import java.lang.reflect.WildcardType; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -32,9 +30,9 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Predicate; import java.util.stream.Stream; -import org.neo4j.ogm.annotation.Relationship; import org.neo4j.ogm.annotation.Transient; import org.neo4j.ogm.metadata.reflect.GenericUtils; @@ -57,7 +55,7 @@ public class FieldsInfo { // all generics fields of possible superclasses that resolve to concrete // types through this class. List allFieldsInfluencedByThisClass = new ArrayList<>(); - allFieldsInfluencedByThisClass.addAll(getGenericFieldsInHierarchyOf(clazz)); + allFieldsInfluencedByThisClass.addAll(getGenericOrParameterizedFieldsInHierarchyOf(clazz)); allFieldsInfluencedByThisClass.addAll(Arrays.asList(clazz.getDeclaredFields())); for (Field field : allFieldsInfluencedByThisClass) { @@ -68,28 +66,7 @@ public class FieldsInfo { String typeParameterDescriptor = null; final Type genericType = field.getGenericType(); if (genericType instanceof ParameterizedType) { - ParameterizedType parameterizedType = (ParameterizedType) genericType; - final Type[] actualTypeArguments = parameterizedType.getActualTypeArguments(); - if (actualTypeArguments.length > 0) { - for (Type typeArgument : actualTypeArguments) { - if (typeArgument instanceof ParameterizedType) { - ParameterizedType parameterizedTypeArgument = (ParameterizedType) typeArgument; - typeParameterDescriptor = parameterizedTypeArgument.getRawType().getTypeName(); - break; - } else if ((typeArgument instanceof TypeVariable || typeArgument instanceof WildcardType) - // The type parameter descriptor doesn't matter if we're dealing with an explicit relationship - // We must not try to persist it as a property if the user explicitly asked for storing it as a node. - && !objectAnnotations.has(Relationship.class)) { - typeParameterDescriptor = Object.class.getName(); - break; - } else if (typeArgument instanceof Class) { - typeParameterDescriptor = ((Class) typeArgument).getName(); - } - } - } - if (typeParameterDescriptor == null) { - typeParameterDescriptor = parameterizedType.getRawType().getTypeName(); - } + typeParameterDescriptor = GenericUtils.findFieldType(field, clazz).getName(); } if (typeParameterDescriptor == null && (genericType instanceof TypeVariable)) { typeParameterDescriptor = field.getType().getTypeName(); @@ -101,13 +78,16 @@ public class FieldsInfo { } } - private static List getGenericFieldsInHierarchyOf(Class clazz) { + private static List getGenericOrParameterizedFieldsInHierarchyOf(Class clazz) { + + Predicate predicate = GenericUtils::isGenericField; + predicate = predicate.or(GenericUtils::isParameterizedField); List genericFieldsInHierarchy = new ArrayList<>(); Class currentClass = clazz.getSuperclass(); while(currentClass != null) { Stream.of(currentClass.getDeclaredFields()) - .filter(GenericUtils::isGenericField) + .filter(predicate) .forEach(genericFieldsInHierarchy::add); currentClass = currentClass.getSuperclass(); } diff --git a/core/src/main/java/org/neo4j/ogm/metadata/reflect/GenericUtils.java b/core/src/main/java/org/neo4j/ogm/metadata/reflect/GenericUtils.java index 77a249cc71..5f0e12bc8a 100644 --- a/core/src/main/java/org/neo4j/ogm/metadata/reflect/GenericUtils.java +++ b/core/src/main/java/org/neo4j/ogm/metadata/reflect/GenericUtils.java @@ -48,10 +48,21 @@ public static boolean isGenericField(Field field) { } /** - * Tries to discover type of given field + * Helper to check whether a given field is a parameterized field (A field described by a type variable presenting a parameter type). + * + * @param field The field to check for a parameterized type + * @return True, if {@code field} is a parameterized field. + */ + public static boolean isParameterizedField(Field field) { + return field.getGenericType() instanceof ParameterizedType; + } + + /** + * Tries to discover type of given field. * If the field ha a concrete type then there is nothing to do and it's type is returned. * If the field has a generic type then it traverses class hierarchy of the concrete class to discover - * ParameterizedType with type parameter + * ParameterizedType with type parameter. + * If the field is parameterized but no parameter can be discovered, than {@code Object.class} will be returned. * * @param field field * @param concreteClass concrete class that either declares the field or is a subclass of such class @@ -61,7 +72,7 @@ public static Class findFieldType(Field field, Class concreteClass) { Class[] arguments = resolveRawArguments(field.getGenericType(), concreteClass); if (arguments == null || arguments.length == 0 || arguments[0] == Unknown.class) { - return field.getType(); + return isParameterizedField(field) ? Object.class : field.getType(); } return arguments[0]; diff --git a/test/src/test/java/org/neo4j/ogm/domain/gh656/Group.java b/test/src/test/java/org/neo4j/ogm/domain/gh656/Group.java new file mode 100644 index 0000000000..fa4d28c41a --- /dev/null +++ b/test/src/test/java/org/neo4j/ogm/domain/gh656/Group.java @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2002-2019 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.ogm.domain.gh656; + +import java.util.Set; + +import org.neo4j.ogm.annotation.GeneratedValue; +import org.neo4j.ogm.annotation.Id; +import org.neo4j.ogm.annotation.NodeEntity; +import org.neo4j.ogm.annotation.Relationship; +import org.neo4j.ogm.id.UuidStrategy; + +/** + * @author Michael J. Simons + */ +@NodeEntity(label = "Group") +public class Group extends VersionedEntity { + @Id + @GeneratedValue(strategy = UuidStrategy.class) + private String uuid; + + @Relationship(type = "HAS_VERSION2") + private Set versions2; + + public String getUuid() { + return uuid; + } + + public void setUuid(String uuid) { + this.uuid = uuid; + } +} diff --git a/test/src/test/java/org/neo4j/ogm/domain/gh656/GroupVersion.java b/test/src/test/java/org/neo4j/ogm/domain/gh656/GroupVersion.java new file mode 100644 index 0000000000..3bece9041e --- /dev/null +++ b/test/src/test/java/org/neo4j/ogm/domain/gh656/GroupVersion.java @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2002-2019 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.ogm.domain.gh656; + +import org.neo4j.ogm.annotation.GeneratedValue; +import org.neo4j.ogm.annotation.Id; +import org.neo4j.ogm.annotation.NodeEntity; +import org.neo4j.ogm.id.UuidStrategy; + +/** + * @author Michael J. Simons + */ +@NodeEntity(label = "GroupVersion") +public class GroupVersion { + @Id + @GeneratedValue(strategy = UuidStrategy.class) + private String uuid; + + public String getUuid() { + return uuid; + } + + public void setUuid(String uuid) { + this.uuid = uuid; + } +} diff --git a/test/src/test/java/org/neo4j/ogm/domain/gh656/VersionedEntity.java b/test/src/test/java/org/neo4j/ogm/domain/gh656/VersionedEntity.java new file mode 100644 index 0000000000..c563091e02 --- /dev/null +++ b/test/src/test/java/org/neo4j/ogm/domain/gh656/VersionedEntity.java @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2002-2019 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.ogm.domain.gh656; + +import java.util.Set; + +import org.neo4j.ogm.annotation.Relationship; + +/** + * @param + * @author Michael J. Simons + */ +abstract class VersionedEntity { + @Relationship(type = "HAS_VERSION") + private Set versions; + + public Set getVersions() { + return versions; + } + + public void setVersions(Set versions) { + this.versions = versions; + } +} diff --git a/test/src/test/java/org/neo4j/ogm/metadata/GenericsFieldsTest.java b/test/src/test/java/org/neo4j/ogm/metadata/GenericsFieldsTest.java index 8d462f45d5..be030ec442 100644 --- a/test/src/test/java/org/neo4j/ogm/metadata/GenericsFieldsTest.java +++ b/test/src/test/java/org/neo4j/ogm/metadata/GenericsFieldsTest.java @@ -34,72 +34,72 @@ public class GenericsFieldsTest extends TestMetaDataTypeResolution { @Test public void testUnboundedGeneric() { - checkField("genericObject", "java.lang.Object", Object.class); + checkField("genericObject", "java.lang.Object", Object.class, null); } @Test public void testGenericComparable() { // from java.lang - checkField("genericComparable", "java.lang.Comparable", Comparable.class); + checkField("genericComparable", "java.lang.Comparable", Comparable.class, null); } @Test public void testGenericSerializable() { // from java.io - checkField("genericSerializable", "java.io.Serializable", Serializable.class); + checkField("genericSerializable", "java.io.Serializable", Serializable.class, null); } @Test public void testGenericSelfReference() { - checkField("next", "org.neo4j.ogm.metadata.POJO", POJO.class); + checkField("next", "org.neo4j.ogm.metadata.POJO", POJO.class, null); } @Test // List public void testCollectionWithUnboundGenericParameter() { - checkField("elements", "java.lang.Object", Object.class); + checkField("elements", "java.lang.Object", Object.class, "java.util.List"); } @Test // List> neighbours; public void testCollectionWithConcreteParameterizedType() { - checkField("neighbours", "org.neo4j.ogm.metadata.POJO", POJO.class); + checkField("neighbours", "org.neo4j.ogm.metadata.POJO", POJO.class, "java.util.List"); } @Test // List superIntegers public void testCollectionWithExtendedConcreteParameterizedType() { - checkField("superIntegers", "java.lang.Object", Object.class); + checkField("superIntegers", "java.lang.Object", Object.class, "java.util.List"); } @Test // List subIntegers; public void testCollectionWithReducedConcreteParameterizedType() { - checkField("subIntegers", "java.lang.Object", Object.class); + checkField("subIntegers", "java.lang.Object", Object.class, "java.util.List"); } @Test // List superS; public void testCollectionOfWildcardExtendingGenericType() { - checkField("superS", "java.lang.Object", Object.class); + checkField("superS", "java.lang.Object", Object.class, "java.util.List"); } @Test // List subS; public void testCollectionOfWildcardReducingGenericType() { - checkField("subS", "java.lang.Object", Object.class); + checkField("subS", "java.lang.Object", Object.class, "java.util.List"); } @Test // List; public void testListGenericWildcard() { - checkField("listOfAnything", "java.lang.Object", Object.class); + checkField("listOfAnything", "java.lang.Object", Object.class, "java.util.List"); } @Test // Vector; public void testVectorGenericWildcard() { - checkField("vectorOfAnything", "java.lang.Object", Object.class); + checkField("vectorOfAnything", "java.lang.Object", Object.class, "java.util.Vector"); } @Test // Set; public void testSetGenericWildcard() { - checkField("setOfAnything", "java.lang.Object", Object.class); + checkField("setOfAnything", "java.lang.Object", Object.class, "java.util.Set"); } @Test // Iterable, POJO>> iterable; public void testIterableOfMapOfParameterizedClasses() { - checkField("iterable", "java.util.Map", Map.class); + checkField("iterable", "java.util.Map", Map.class, "java.lang.Iterable"); } @Test // GH-492 @@ -121,4 +121,17 @@ public void shouldDetectWrapperArraysInGenericFields() { assertThat(fieldInfo.isArray()).isTrue(); assertThat(fieldInfo.type()).isEqualTo(Integer[].class); } + + @Test // GH-656 + public void parameterizedFieldsInParentClassesShouldWork() { + + MetaData metaData = new MetaData("org.neo4j.ogm.domain.gh656"); + ClassInfo classInfo = metaData.classInfo("Group"); + assertThat(classInfo).isNotNull(); + assertThat(classInfo.getFieldInfo("uuid")).isNotNull(); + FieldInfo hasVersionField = classInfo.relationshipField("HAS_VERSION"); + assertThat(hasVersionField).isNotNull(); + assertThat(hasVersionField.getCollectionClassname()).isEqualTo("java.util.Set"); + assertThat(hasVersionField.getTypeDescriptor()).isEqualTo("org.neo4j.ogm.domain.gh656.GroupVersion"); + } } diff --git a/test/src/test/java/org/neo4j/ogm/metadata/TestMetaDataTypeResolution.java b/test/src/test/java/org/neo4j/ogm/metadata/TestMetaDataTypeResolution.java index 71269af63b..fca55ca6ae 100644 --- a/test/src/test/java/org/neo4j/ogm/metadata/TestMetaDataTypeResolution.java +++ b/test/src/test/java/org/neo4j/ogm/metadata/TestMetaDataTypeResolution.java @@ -23,19 +23,28 @@ import org.neo4j.ogm.utils.ClassUtils; /** - * @author vince + * @author Vince Bickers + * @author Michael J. Simons */ - public class TestMetaDataTypeResolution { private MetaData metaData = new MetaData("org.neo4j.ogm.metadata"); public void checkField(String name, String expectedDescriptor, Class expectedPersistableType) { + this.checkField(name, expectedDescriptor, expectedPersistableType, null); + } + + public void checkField(String name, String expectedDescriptor, Class expectedPersistableType, String fieldType) { ClassInfo classInfo = metaData.classInfo("POJO"); FieldInfo fieldInfo = classInfo.fieldsInfo().get(name); String fieldDescriptor = fieldInfo.getTypeDescriptor(); assertThat(fieldDescriptor).isEqualTo(expectedDescriptor); Class clazz = ClassUtils.getType(fieldDescriptor); assertThat(clazz).isEqualTo(expectedPersistableType); + if (fieldType != null) { + assertThat(fieldInfo.getCollectionClassname()).isEqualTo(fieldType); + } else { + assertThat(fieldInfo.getCollectionClassname()).isEqualTo(fieldDescriptor); + } } } diff --git a/test/src/test/java/org/neo4j/ogm/persistence/model/RelationshipMappingTest.java b/test/src/test/java/org/neo4j/ogm/persistence/model/RelationshipMappingTest.java index 54a01174b1..332da8f597 100644 --- a/test/src/test/java/org/neo4j/ogm/persistence/model/RelationshipMappingTest.java +++ b/test/src/test/java/org/neo4j/ogm/persistence/model/RelationshipMappingTest.java @@ -21,6 +21,7 @@ import static org.assertj.core.api.Assertions.*; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -35,8 +36,12 @@ import org.neo4j.ogm.domain.gh640.MyNodeWithAssignedId; import org.neo4j.ogm.domain.gh641.Entity1; import org.neo4j.ogm.domain.gh641.MyRelationship; +import org.neo4j.ogm.domain.gh656.Group; +import org.neo4j.ogm.domain.gh656.GroupVersion; import org.neo4j.ogm.domain.policy.Person; import org.neo4j.ogm.domain.policy.Policy; +import org.neo4j.ogm.domain.typed_relationships.SomeEntity; +import org.neo4j.ogm.domain.typed_relationships.TypedEntity; import org.neo4j.ogm.session.Session; import org.neo4j.ogm.session.SessionFactory; import org.neo4j.ogm.testutil.GraphTestUtils; @@ -57,7 +62,9 @@ public static void oneTimeSetup() { "org.neo4j.ogm.domain.policy", "org.neo4j.ogm.domain.election", "org.neo4j.ogm.domain.gh640", - "org.neo4j.ogm.domain.gh641"); + "org.neo4j.ogm.domain.gh641", + "org.neo4j.ogm.domain.typed_relationships", + "org.neo4j.ogm.domain.gh656"); } @Before @@ -286,4 +293,37 @@ public void shouldKeepOrderOfRelatedElements() { + "ORDER BY r.ordering", Collections.emptyMap()); assertThat(entity1.getEntries()).extracting(MyRelationship::getOrdering).containsExactly(1, 2, 3); } + + @Test // GH-528 + public void shouldDealWithTypedRelationships() { + SomeEntity someEntity = new SomeEntity(); + + someEntity.setThing(new TypedEntity<>(42.21)); + someEntity.setMoreThings(Arrays.asList(new TypedEntity<>("Die halbe Wahrheit"), new TypedEntity<>("21"))); + someEntity.setSomeOtherStuff(Arrays.asList("A", "B", "C")); + + session.save(someEntity); + session.clear(); + + someEntity = session.load(SomeEntity.class, someEntity.getId()); + assertThat(someEntity.getThing().getSomeThing()) + .isEqualTo(42.21); + assertThat(someEntity.getMoreThings()) + .extracting(t -> (String) t.getSomeThing()) + .containsExactlyInAnyOrder("Die halbe Wahrheit", "21"); + assertThat(someEntity.getSomeOtherStuff()) + .containsExactlyInAnyOrder("A", "B", "C"); + } + + @Test // GH-656 + public void genericRelationshipsInParentClassesShouldWork() { + Group group = new Group(); + GroupVersion groupVersion = new GroupVersion(); + group.setVersions(Collections.singleton(groupVersion)); + + sessionFactory.openSession().save(group); + + group = sessionFactory.openSession().load(Group.class, group.getUuid()); + assertThat(group.getVersions()).hasSize(1); + } } diff --git a/test/src/test/java/org/neo4j/ogm/session/delegates/SessionDelegateIntegrationTest.java b/test/src/test/java/org/neo4j/ogm/session/delegates/SessionDelegateIntegrationTest.java index af4b5fce2d..09399375b7 100644 --- a/test/src/test/java/org/neo4j/ogm/session/delegates/SessionDelegateIntegrationTest.java +++ b/test/src/test/java/org/neo4j/ogm/session/delegates/SessionDelegateIntegrationTest.java @@ -29,10 +29,7 @@ import org.junit.Test; import org.neo4j.ogm.cypher.ComparisonOperator; import org.neo4j.ogm.cypher.Filter; -import org.neo4j.ogm.domain.generic_hierarchy.ConcreteChild; import org.neo4j.ogm.domain.music.Album; -import org.neo4j.ogm.domain.typed_relationships.SomeEntity; -import org.neo4j.ogm.domain.typed_relationships.TypedEntity; import org.neo4j.ogm.session.Neo4jSession; import org.neo4j.ogm.session.Session; import org.neo4j.ogm.session.SessionFactory; @@ -53,8 +50,7 @@ public class SessionDelegateIntegrationTest extends MultiDriverTestClass { @BeforeClass public static void createSessionFactory() { - sessionFactory = new SessionFactory(driver, "org.neo4j.ogm.domain.music", - "org.neo4j.ogm.domain.typed_relationships"); + sessionFactory = new SessionFactory(driver, "org.neo4j.ogm.domain.music"); } @Before @@ -107,25 +103,4 @@ public void shouldPickupCorrectFieldInfo() { .as("Specified provider should be used") .isInstanceOf(DateLongConverter.class); } - - @Test // GH-528 - public void shouldDealWithTypedRelationships() { - SomeEntity someEntity = new SomeEntity(); - - someEntity.setThing(new TypedEntity<>(42.21)); - someEntity.setMoreThings(Arrays.asList(new TypedEntity<>("Die halbe Wahrheit"), new TypedEntity<>("21"))); - someEntity.setSomeOtherStuff(Arrays.asList("A", "B", "C")); - - session.save(someEntity); - session.clear(); - - someEntity = session.load(SomeEntity.class, someEntity.getId()); - assertThat(someEntity.getThing().getSomeThing()) - .isEqualTo(42.21); - assertThat(someEntity.getMoreThings()) - .extracting(t -> (String) t.getSomeThing()) - .containsExactlyInAnyOrder("Die halbe Wahrheit", "21"); - assertThat(someEntity.getSomeOtherStuff()) - .containsExactlyInAnyOrder("A", "B", "C"); - } }