Pass concrete enum type to MetaDataDrivenConversionService.

michael-simons · michael-simons · commit 89c7ab0472ac · 2021-04-08T12:51:32.000+02:00
`Enum` properties always use an `AttributeConverter` and this is always passed to the parent conversion service. The parent conversion service however creates an adapter on the passed source type. Originally we passed `java.lang.Enum` here, leading to a scenario in which the parent decided to not be able to convert a concrete enum instance (and rightfully so). The solution in this commit is to pass the concrete field type if it is an enum. The conversion failed only for constructor calls. This fixes #2213.
diff --git a/spring-data-neo4j/src/main/java/org/springframework/data/neo4j/conversion/MetaDataDrivenConversionService.java b/spring-data-neo4j/src/main/java/org/springframework/data/neo4j/conversion/MetaDataDrivenConversionService.java
@@ -20,7 +20,9 @@
 import org.neo4j.ogm.metadata.ClassInfo;
 import org.neo4j.ogm.metadata.FieldInfo;
 import org.neo4j.ogm.metadata.MetaData;
+import org.neo4j.ogm.support.ClassUtils;
 import org.neo4j.ogm.typeconversion.AttributeConverter;
+import org.neo4j.ogm.typeconversion.EnumStringConverter;
 import org.neo4j.ogm.typeconversion.ProxyAttributeConverter;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -52,20 +54,20 @@ public MetaDataDrivenConversionService(MetaData metaData) {
 		for (ClassInfo classInfo : metaData.persistentEntities()) {
 			for (FieldInfo fieldInfo : classInfo.propertyFields()) {
 				if (fieldInfo.hasPropertyConverter()) {
-					addWrappedConverter(fieldInfo.getPropertyConverter());
+					addWrappedConverter(fieldInfo.getField().getType(), fieldInfo.getPropertyConverter());
 				}
 			}
 		}
 	}
 
 	@SuppressWarnings({ "unchecked", "rawtypes" })
-	private void addWrappedConverter(final AttributeConverter attributeConverter) {
+	private void addWrappedConverter(Class<?> type, final AttributeConverter attributeConverter) {
 
 		if (attributeConverter instanceof ProxyAttributeConverter) {
 			return;
 		}
 
-		EntityToGraphTypeMapping entityToGraphTypeMapping = getEntityToGraphTypeMapping(attributeConverter);
+		EntityToGraphTypeMapping entityToGraphTypeMapping = getEntityToGraphTypeMapping(type, attributeConverter);
 
 		if (canConvert(entityToGraphTypeMapping.entityType, entityToGraphTypeMapping.graphType)
 				&& canConvert(entityToGraphTypeMapping.graphType, entityToGraphTypeMapping.entityType)) {
@@ -92,7 +94,7 @@ private EntityToGraphTypeMapping(Class<?> entityType, Class<?> target) {
 		}
 	}
 
-	static EntityToGraphTypeMapping getEntityToGraphTypeMapping(AttributeConverter attributeConverter) {
+	static EntityToGraphTypeMapping getEntityToGraphTypeMapping(Class<?> type, AttributeConverter attributeConverter) {
 
 		ResolvableType resolvableType = ResolvableType.forClass(AttributeConverter.class,
 				attributeConverter.getClass());
@@ -103,7 +105,7 @@ static EntityToGraphTypeMapping getEntityToGraphTypeMapping(AttributeConverter a
 							+ attributeConverter.getClass());
 		}
 
-		Class<?> sourceType = nestedTypeOrType(resolvableType.getGeneric(0));
+		Class<?> sourceType = ClassUtils.isEnum(type) ? type : nestedTypeOrType(resolvableType.getGeneric(0));
 		Class<?> targetType = nestedTypeOrType(resolvableType.getGeneric(1));
 
 		return new EntityToGraphTypeMapping(sourceType, targetType);
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/MetaDataDrivenConversionServiceTests.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/MetaDataDrivenConversionServiceTests.java
@@ -21,6 +21,8 @@
 import org.springframework.data.neo4j.conversion.support.ConvertedClass;
 import org.springframework.data.neo4j.conversion.support.Converters;
 
+import java.util.List;
+
 /**
  * @author Michael J. Simons
  * @soundtrack Murray Gold - Doctor Who Season 9
@@ -30,7 +32,7 @@ public class MetaDataDrivenConversionServiceTests {
 	@Test
 	public void shouldDetermineConvertersForClasses() {
 		MetaDataDrivenConversionService.EntityToGraphTypeMapping entityToGraphTypeMapping = MetaDataDrivenConversionService
-				.getEntityToGraphTypeMapping(new Converters.DoubleToStringConverter());
+				.getEntityToGraphTypeMapping(Double.class, new Converters.DoubleToStringConverter());
 
 		assertThat(entityToGraphTypeMapping.entityType).isEqualTo(Double.class);
 		assertThat(entityToGraphTypeMapping.graphType).isEqualTo(String.class);
@@ -39,7 +41,7 @@ public void shouldDetermineConvertersForClasses() {
 	@Test
 	public void shouldDetermineConvertersForTypedClasses() {
 		MetaDataDrivenConversionService.EntityToGraphTypeMapping entityToGraphTypeMapping = MetaDataDrivenConversionService
-				.getEntityToGraphTypeMapping(new Converters.ListToStringConverter());
+				.getEntityToGraphTypeMapping(List.class, new Converters.ListToStringConverter());
 
 		assertThat(entityToGraphTypeMapping.entityType).isEqualTo(Double.class);
 		assertThat(entityToGraphTypeMapping.graphType).isEqualTo(String.class);
@@ -48,7 +50,7 @@ public void shouldDetermineConvertersForTypedClasses() {
 	@Test // DATAGRAPH-1131
 	public void shouldWorkWithConvertersInvolvingAbstractBaseClasses() {
 		MetaDataDrivenConversionService.EntityToGraphTypeMapping entityToGraphTypeMapping = MetaDataDrivenConversionService
-				.getEntityToGraphTypeMapping(new Converters.ConvertedClassToStringConverter());
+				.getEntityToGraphTypeMapping(ConvertedClass.class, new Converters.ConvertedClassToStringConverter());
 
 		assertThat(entityToGraphTypeMapping.entityType).isEqualTo(ConvertedClass.class);
 		assertThat(entityToGraphTypeMapping.graphType).isEqualTo(String.class);
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/DescribeType.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/DescribeType.java
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.springframework.data.neo4j.conversion.gh2213;
+
+/**
+ * @author Michael J. Simons
+ */
+public enum DescribeType {
+    A, B
+}
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/Describes.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/Describes.java
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.springframework.data.neo4j.conversion.gh2213;
+
+import org.neo4j.ogm.annotation.EndNode;
+import org.neo4j.ogm.annotation.GeneratedValue;
+import org.neo4j.ogm.annotation.Id;
+import org.neo4j.ogm.annotation.RelationshipEntity;
+import org.neo4j.ogm.annotation.StartNode;
+
+/**
+ * @author Michael J. Simons
+ */
+@RelationshipEntity("DESCRIBES")
+public class Describes {
+
+    @Id @GeneratedValue
+    private Long id;
+
+    private DescribeType something;
+
+    @StartNode
+    private NodeA nodeA;
+
+    @EndNode
+    private NodeB nodeB;
+
+    Describes() {
+    }
+
+    public Describes(DescribeType something, NodeA nodeA, NodeB nodeB) {
+        this.something = something;
+        this.nodeA = nodeA;
+        this.nodeB = nodeB;
+    }
+
+    public Long getId() {
+        return id;
+    }
+
+    public DescribeType getSomething() {
+        return something;
+    }
+
+    public void setSomething(DescribeType something) {
+        this.something = something;
+    }
+
+    public NodeA getNodeA() {
+        return nodeA;
+    }
+
+    public void setNodeA(NodeA nodeA) {
+        this.nodeA = nodeA;
+    }
+
+    public NodeB getNodeB() {
+        return nodeB;
+    }
+
+    public void setNodeB(NodeB nodeB) {
+        this.nodeB = nodeB;
+    }
+}
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/NodeA.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/NodeA.java
@@ -0,0 +1,63 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.springframework.data.neo4j.conversion.gh2213;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+import org.neo4j.ogm.annotation.Id;
+import org.neo4j.ogm.annotation.NodeEntity;
+import org.neo4j.ogm.annotation.Relationship;
+
+/**
+ * @author Michael J. Simons
+ */
+@NodeEntity
+public class NodeA {
+
+    @Id @GeneratedValue
+    private Long id;
+
+    private DescribeType describeType;
+
+    @Relationship(type = "DESCRIBES")
+    private List<Describes> describes = new ArrayList<>();
+
+	public NodeA(DescribeType describeType) {
+		this.describeType = describeType;
+	}
+
+	public Long getId() {
+        return id;
+    }
+
+    public DescribeType getDescribeType() {
+        return describeType;
+    }
+
+    public void setDescribeType(DescribeType describeType) {
+        this.describeType = describeType;
+    }
+
+    public List<Describes> getDescribes() {
+        return describes;
+    }
+
+    public void setDescribes(List<Describes> describes) {
+        this.describes = describes;
+    }
+}
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/NodeB.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/NodeB.java
@@ -0,0 +1,34 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.springframework.data.neo4j.conversion.gh2213;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+import org.neo4j.ogm.annotation.Id;
+import org.neo4j.ogm.annotation.NodeEntity;
+
+/**
+ * @author Michael J. Simons
+ */
+@NodeEntity
+public class NodeB {
+
+    @Id @GeneratedValue
+    private Long id;
+
+    public Long getId() {
+        return id;
+    }
+}
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/RepositoryUnderTest.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/RepositoryUnderTest.java
@@ -0,0 +1,24 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.springframework.data.neo4j.conversion.gh2213;
+
+import org.springframework.data.neo4j.repository.Neo4jRepository;
+
+/**
+ * @author Michael J. Simons
+ */
+public interface RepositoryUnderTest extends Neo4jRepository<NodeA, Long> {
+}
diff --git a/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/integration/conversion/ConversionServiceTests.java b/spring-data-neo4j/src/test/java/org/springframework/data/neo4j/integration/conversion/ConversionServiceTests.java
@@ -19,6 +19,7 @@
 import java.math.BigInteger;
 import java.math.RoundingMode;
 import java.util.Arrays;
+import java.util.Optional;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -29,12 +30,16 @@
 import org.neo4j.ogm.session.SessionFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.annotation.Bean;
-import org.springframework.context.annotation.ComponentScan;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.core.convert.ConversionService;
 import org.springframework.core.convert.ConverterNotFoundException;
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.data.neo4j.conversion.MetaDataDrivenConversionService;
+import org.springframework.data.neo4j.conversion.gh2213.DescribeType;
+import org.springframework.data.neo4j.conversion.gh2213.Describes;
+import org.springframework.data.neo4j.conversion.gh2213.NodeB;
+import org.springframework.data.neo4j.conversion.gh2213.RepositoryUnderTest;
+import org.springframework.data.neo4j.conversion.gh2213.NodeA;
 import org.springframework.data.neo4j.integration.conversion.domain.JavaElement;
 import org.springframework.data.neo4j.integration.conversion.domain.MonetaryAmount;
 import org.springframework.data.neo4j.integration.conversion.domain.PensionPlan;
@@ -65,6 +70,8 @@ public class ConversionServiceTests {
 	@Autowired private PensionRepository pensionRepository;
 	@Autowired private JavaElementRepository javaElementRepository;
 	@Autowired private SiteMemberRepository siteMemberRepository;
+	@Autowired private RepositoryUnderTest repositoryUnderTest;
+
 	// TODO See below, for the time being at least be explicit on which type of conversion service we're working on here
 	// The only thing that is under test, is the instance of MetaDataDrivenConversionService which get's even
 	// modified heavily be this test. This needs to be fixed in the near future.
@@ -251,9 +258,28 @@ public void shouldRecognizeJavaEnums() {
 		assertThat(siteMember.getRoundingModes().contains(RoundingMode.FLOOR)).isTrue();
 	}
 
+	@Test // GH-2213
+	public void enumConvertersOnConstructorsShouldWork() {
+
+		long id = transactionTemplate.execute(tx -> {
+			NodeA s = new NodeA(DescribeType.A);
+			Describes d = new Describes(DescribeType.B, s, new NodeB());
+
+			s.setDescribes(Arrays.asList(d));
+			return repositoryUnderTest.save(s).getId();
+		});
+		Optional<NodeA> optionalResult = repositoryUnderTest.findById(id);
+		assertThat(optionalResult).hasValueSatisfying(nl -> {
+			assertThat(nl.getDescribeType()).isEqualTo(DescribeType.A);
+			assertThat(nl.getDescribes()).hasSize(1)
+					.first().satisfies(dl -> assertThat(dl.getSomething()).isEqualTo(DescribeType.B));
+		});
+
+	}
+
 	@Configuration
-	@Neo4jIntegrationTest(domainPackages = "org.springframework.data.neo4j.integration.conversion.domain",
-			repositoryPackages = "org.springframework.data.neo4j.integration.conversion")
+	@Neo4jIntegrationTest(domainPackages = {"org.springframework.data.neo4j.integration.conversion.domain", "org.springframework.data.neo4j.conversion.gh2213"},
+			repositoryPackages = {"org.springframework.data.neo4j.integration.conversion", "org.springframework.data.neo4j.conversion.gh2213"})
 	static class ConversionServicePersistenceContext {
 
 		@Bean

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,9 @@`
`20`	`20`	`import org.neo4j.ogm.metadata.ClassInfo;`
`21`	`21`	`import org.neo4j.ogm.metadata.FieldInfo;`
`22`	`22`	`import org.neo4j.ogm.metadata.MetaData;`
	`23`	`+import org.neo4j.ogm.support.ClassUtils;`
`23`	`24`	`import org.neo4j.ogm.typeconversion.AttributeConverter;`
	`25`	`+import org.neo4j.ogm.typeconversion.EnumStringConverter;`
`24`	`26`	`import org.neo4j.ogm.typeconversion.ProxyAttributeConverter;`
`25`	`27`	`import org.slf4j.Logger;`
`26`	`28`	`import org.slf4j.LoggerFactory;`
`@@ -52,20 +54,20 @@ public MetaDataDrivenConversionService(MetaData metaData) {`
`52`	`54`	`for (ClassInfo classInfo : metaData.persistentEntities()) {`
`53`	`55`	`for (FieldInfo fieldInfo : classInfo.propertyFields()) {`
`54`	`56`	`if (fieldInfo.hasPropertyConverter()) {`
`55`		`- addWrappedConverter(fieldInfo.getPropertyConverter());`
	`57`	`+ addWrappedConverter(fieldInfo.getField().getType(), fieldInfo.getPropertyConverter());`
`56`	`58`	`}`
`57`	`59`	`}`
`58`	`60`	`}`
`59`	`61`	`}`
`60`	`62`
`61`	`63`	`@SuppressWarnings({ "unchecked", "rawtypes" })`
`62`		`- private void addWrappedConverter(final AttributeConverter attributeConverter) {`
	`64`	`+ private void addWrappedConverter(Class<?> type, final AttributeConverter attributeConverter) {`
`63`	`65`
`64`	`66`	`if (attributeConverter instanceof ProxyAttributeConverter) {`
`65`	`67`	`return;`
`66`	`68`	`}`
`67`	`69`
`68`		`- EntityToGraphTypeMapping entityToGraphTypeMapping = getEntityToGraphTypeMapping(attributeConverter);`
	`70`	`+ EntityToGraphTypeMapping entityToGraphTypeMapping = getEntityToGraphTypeMapping(type, attributeConverter);`
`69`	`71`
`70`	`72`	`if (canConvert(entityToGraphTypeMapping.entityType, entityToGraphTypeMapping.graphType)`
`71`	`73`	`&& canConvert(entityToGraphTypeMapping.graphType, entityToGraphTypeMapping.entityType)) {`
`@@ -92,7 +94,7 @@ private EntityToGraphTypeMapping(Class<?> entityType, Class<?> target) {`
`92`	`94`	`}`
`93`	`95`	`}`
`94`	`96`
`95`		`- static EntityToGraphTypeMapping getEntityToGraphTypeMapping(AttributeConverter attributeConverter) {`
	`97`	`+ static EntityToGraphTypeMapping getEntityToGraphTypeMapping(Class<?> type, AttributeConverter attributeConverter) {`
`96`	`98`
`97`	`99`	`ResolvableType resolvableType = ResolvableType.forClass(AttributeConverter.class,`
`98`	`100`	`attributeConverter.getClass());`
`@@ -103,7 +105,7 @@ static EntityToGraphTypeMapping getEntityToGraphTypeMapping(AttributeConverter a`
`103`	`105`	`+ attributeConverter.getClass());`
`104`	`106`	`}`
`105`	`107`
`106`		`- Class<?> sourceType = nestedTypeOrType(resolvableType.getGeneric(0));`
	`108`	`+ Class<?> sourceType = ClassUtils.isEnum(type) ? type : nestedTypeOrType(resolvableType.getGeneric(0));`
`107`	`109`	`Class<?> targetType = nestedTypeOrType(resolvableType.getGeneric(1));`
`108`	`110`
`109`	`111`	`return new EntityToGraphTypeMapping(sourceType, targetType);`