From 89c7ab0472acac554cfb38b79634e66e10dbbd21 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Thu, 8 Apr 2021 12:48:28 +0200 Subject: [PATCH] Pass concrete enum type to MetaDataDrivenConversionService. `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. --- .../MetaDataDrivenConversionService.java | 12 +-- .../MetaDataDrivenConversionServiceTests.java | 8 +- .../neo4j/conversion/gh2213/DescribeType.java | 23 ++++++ .../neo4j/conversion/gh2213/Describes.java | 77 +++++++++++++++++++ .../data/neo4j/conversion/gh2213/NodeA.java | 63 +++++++++++++++ .../data/neo4j/conversion/gh2213/NodeB.java | 34 ++++++++ .../gh2213/RepositoryUnderTest.java | 24 ++++++ .../conversion/ConversionServiceTests.java | 32 +++++++- 8 files changed, 262 insertions(+), 11 deletions(-) create mode 100644 spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/DescribeType.java create mode 100644 spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/Describes.java create mode 100644 spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/NodeA.java create mode 100644 spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/NodeB.java create mode 100644 spring-data-neo4j/src/test/java/org/springframework/data/neo4j/conversion/gh2213/RepositoryUnderTest.java 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 index b12355551..c8f4985af 100644 --- 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 java.util.Optional; 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 class MetaDataDrivenConversionService extends GenericConversionService { 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 @@ public class MetaDataDrivenConversionService extends GenericConversionService { } } - static EntityToGraphTypeMapping getEntityToGraphTypeMapping(AttributeConverter attributeConverter) { + static EntityToGraphTypeMapping getEntityToGraphTypeMapping(Class type, AttributeConverter attributeConverter) { ResolvableType resolvableType = ResolvableType.forClass(AttributeConverter.class, attributeConverter.getClass()); @@ -103,7 +105,7 @@ public class MetaDataDrivenConversionService extends GenericConversionService { + 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 index 0f97132c6..7c4781f2c 100644 --- 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.junit.Test; 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 class MetaDataDrivenConversionServiceTests { @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 class MetaDataDrivenConversionServiceTests { @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 new file mode 100644 index 000000000..7865976d3 --- /dev/null +++ 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 new file mode 100644 index 000000000..5e0f0b969 --- /dev/null +++ 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 new file mode 100644 index 000000000..df7781677 --- /dev/null +++ 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 = 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 getDescribes() { + return describes; + } + + public void setDescribes(List 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 new file mode 100644 index 000000000..9b4267e2e --- /dev/null +++ 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 new file mode 100644 index 000000000..8d1c1f1d6 --- /dev/null +++ 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 { +} 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 index ecd31e932..95c4b2d4b 100644 --- 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.lang.annotation.ElementType; 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.Session; 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 class ConversionServiceTests { 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 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