GH-2348 - Use default values for primitive attributes when Graph properties are unset.

This changes the behaviour in regards of default values for primitives attributes of domain objects: They have well known default values on the JVM and we should use them, even if the property is non-existent in the graph (materializing as literal null). People migrating from SDN+OGM rely on the behaviour.

This change does not apply to attributes assigned via constructor arguments.
Constructor argument don't have default values (in Java) and we try to avoid guessing as much as possible.
In addition, it would also clash with actually possible default values in Kotlin data classes.

The change cannot be done in `DefaultNeo4jConversionService` as originally planned, as the conversion service is of course unaware about the target of a converted value.

This fixes #2348.

Author: michael-simons

src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

@@ -43,6 +43,7 @@
 import org.neo4j.driver.types.Type;
 import org.neo4j.driver.types.TypeSystem;
 import org.springframework.core.CollectionFactory;
+import org.springframework.core.KotlinDetector;
 import org.springframework.data.annotation.ReadOnlyProperty;
 import org.springframework.data.mapping.AssociationHandler;
 import org.springframework.data.mapping.MappingException;
@@ -53,6 +54,7 @@
 import org.springframework.data.mapping.model.ParameterValueProvider;
 import org.springframework.data.neo4j.core.convert.Neo4jConversionService;
 import org.springframework.data.neo4j.core.schema.TargetNode;
+import org.springframework.data.util.ReflectionUtils;
 import org.springframework.data.util.TypeInformation;
 import org.springframework.lang.NonNull;
 import org.springframework.lang.Nullable;
@@ -108,7 +110,7 @@ public <R> R read(Class<R> targetType, MapAccessor mapAccessor) {
 		try {
 			return map(queryRoot, queryRoot, rootNodeDescription);
 		} catch (Exception e) {
-			throw new MappingException("Error mapping " + mapAccessor.toString(), e);
+			throw new MappingException("Error mapping " + mapAccessor, e);
 		}
 	}
 
@@ -270,6 +272,7 @@ private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescr
 			Neo4jPersistentEntity<ET> concreteNodeDescription = (Neo4jPersistentEntity<ET>) nodeDescriptionAndLabels
 					.getNodeDescription();
 
+			boolean isKotlinType = KotlinDetector.isKotlinType(concreteNodeDescription.getType());
 			ET instance = instantiate(concreteNodeDescription, queryResult,
 					nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, relationshipsFromResult, nodesFromResult);
 
@@ -282,7 +285,7 @@ private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescr
 				Predicate<Neo4jPersistentProperty> isConstructorParameter = concreteNodeDescription
 						.getPersistenceConstructor()::isConstructorParameter;
 				PropertyHandler<Neo4jPersistentProperty> handler = populateFrom(queryResult, propertyAccessor,
-						isConstructorParameter, nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity);
+						isConstructorParameter, nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, isKotlinType);
 				concreteNodeDescription.doWithProperties(handler);
 
 				// in a cyclic graph / with bidirectional relationships, we could end up in a state in which we
@@ -396,26 +399,33 @@ public Object getParameterValue(PreferredConstructor.Parameter parameter) {
 
 	private PropertyHandler<Neo4jPersistentProperty> populateFrom(MapAccessor queryResult,
 			PersistentPropertyAccessor<?> propertyAccessor, Predicate<Neo4jPersistentProperty> isConstructorParameter,
-			Collection<String> surplusLabels, Object targetNode) {
+			Collection<String> surplusLabels, Object targetNode, boolean ownerIsKotlinType) {
 		return property -> {
 			if (isConstructorParameter.test(property)) {
 				return;
 			}
 
+			TypeInformation<?> typeInformation = property.getTypeInformation();
 			if (property.isDynamicLabels()) {
 				propertyAccessor.setProperty(property,
-						createDynamicLabelsProperty(property.getTypeInformation(), surplusLabels));
+						createDynamicLabelsProperty(typeInformation, surplusLabels));
 			} else if (property.isAnnotationPresent(TargetNode.class)) {
 				if (queryResult instanceof Relationship) {
 					propertyAccessor.setProperty(property, targetNode);
 				}
 			} else {
-				propertyAccessor.setProperty(property,
-						conversionService.readValue(extractValueOf(property, queryResult), property.getTypeInformation(), property.getOptionalReadingConverter()));
+				Object value = conversionService.readValue(extractValueOf(property, queryResult), typeInformation, property.getOptionalReadingConverter());
+				Class<?> rawType = typeInformation.getType();
+				propertyAccessor.setProperty(property, getValueOrDefault(ownerIsKotlinType, rawType, value));
 			}
 		};
 	}
 
+	private static Object getValueOrDefault(boolean ownerIsKotlinType, Class<?> rawType, @Nullable Object value) {
+
+		return value == null && !ownerIsKotlinType && rawType.isPrimitive() ? ReflectionUtils.getPrimitiveDefault(rawType) : value;
+	}
+
 	private AssociationHandler<Neo4jPersistentProperty> populateFrom(MapAccessor queryResult,
 			PersistentPropertyAccessor<?> propertyAccessor, Predicate<Neo4jPersistentProperty> isConstructorParameter, Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult) {
 		return association -> {

src/test/java/org/springframework/data/neo4j/integration/conversion_imperative/TypeConversionIT.java

@@ -18,7 +18,6 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
-import java.lang.reflect.Field;
 import java.text.SimpleDateFormat;
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
@@ -41,7 +40,6 @@
 import org.junit.jupiter.api.DynamicTest;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestFactory;
-import org.junit.platform.commons.util.AnnotationUtils;
 import org.neo4j.driver.Driver;
 import org.neo4j.driver.Session;
 import org.neo4j.driver.Value;
@@ -54,17 +52,16 @@
 import org.springframework.data.mapping.MappingException;
 import org.springframework.data.neo4j.config.AbstractNeo4jConfig;
 import org.springframework.data.neo4j.core.DatabaseSelectionProvider;
-import org.springframework.data.neo4j.core.convert.ConvertWith;
+import org.springframework.data.neo4j.core.Neo4jTemplate;
 import org.springframework.data.neo4j.core.convert.Neo4jConversions;
 import org.springframework.data.neo4j.core.transaction.Neo4jBookmarkManager;
 import org.springframework.data.neo4j.core.transaction.Neo4jTransactionManager;
+import org.springframework.data.neo4j.integration.shared.common.ThingWithAllCypherTypes2;
 import org.springframework.data.neo4j.integration.shared.conversion.Neo4jConversionsITBase;
 import org.springframework.data.neo4j.integration.shared.conversion.ThingWithAllAdditionalTypes;
 import org.springframework.data.neo4j.integration.shared.common.ThingWithAllCypherTypes;
 import org.springframework.data.neo4j.integration.shared.common.ThingWithAllSpatialTypes;
-import org.springframework.data.neo4j.integration.shared.conversion.ThingWithCompositeProperties;
 import org.springframework.data.neo4j.integration.shared.conversion.ThingWithCustomTypes;
-import org.springframework.data.neo4j.integration.shared.common.ThingWithNonExistingPrimitives;
 import org.springframework.data.neo4j.integration.shared.common.ThingWithUUIDID;
 import org.springframework.data.neo4j.repository.Neo4jRepository;
 import org.springframework.data.neo4j.repository.config.EnableNeo4jRepositories;
@@ -73,7 +70,6 @@
 import org.springframework.test.util.ReflectionTestUtils;
 import org.springframework.transaction.PlatformTransactionManager;
 import org.springframework.transaction.annotation.EnableTransactionManagement;
-import org.springframework.util.ReflectionUtils;
 
 /**
  * @author Michael J. Simons
@@ -83,42 +79,44 @@
 @Neo4jIntegrationTest
 class TypeConversionIT extends Neo4jConversionsITBase {
 
-	private final Driver driver;
-
-	@Autowired CypherTypesRepository cypherTypesRepository;
+	private final CypherTypesRepository cypherTypesRepository;
 
 	private final AdditionalTypesRepository additionalTypesRepository;
 
 	private final SpatialTypesRepository spatialTypesRepository;
 
 	private final CustomTypesRepository customTypesRepository;
-	private final BookmarkCapture bookmarkCapture;
 
 	private final DefaultConversionService defaultConversionService;
 
-	@Autowired TypeConversionIT(Driver driver, CypherTypesRepository cypherTypesRepository,
+	@Autowired TypeConversionIT(CypherTypesRepository cypherTypesRepository,
 			AdditionalTypesRepository additionalTypesRepository, SpatialTypesRepository spatialTypesRepository,
 			CustomTypesRepository customTypesRepository,
-			Neo4jConversions neo4jConversions, BookmarkCapture bookmarkCapture) {
-		this.driver = driver;
+			Neo4jConversions neo4jConversions) {
 		this.cypherTypesRepository = cypherTypesRepository;
 		this.additionalTypesRepository = additionalTypesRepository;
 		this.spatialTypesRepository = spatialTypesRepository;
 		this.customTypesRepository = customTypesRepository;
-		this.bookmarkCapture = bookmarkCapture;
 		this.defaultConversionService = new DefaultConversionService();
 		neo4jConversions.registerConvertersIn(defaultConversionService);
 	}
 
 	@Test
-	void thereShallBeNoDefaultValuesForNonExistingAttributes(@Autowired NonExistingPrimitivesRepository repository) {
+	void thereShallBeNoDefaultValuesForNonExistingAttributes() {
+
+		Long id;
+		try (Session session = neo4jConnectionSupport.getDriver().session(bookmarkCapture.createSessionConfig())) {
+
+			id = session.writeTransaction(tx -> tx.run("CREATE (n:CypherTypes) RETURN id(n)").single().get(0).asLong());
+			bookmarkCapture.seedWith(session.lastBookmark());
+		}
 
 		assertThatExceptionOfType(MappingException.class)
-				.isThrownBy(() -> repository.findById(ID_OF_NON_EXISTING_PRIMITIVES_NODE))
+				.isThrownBy(() -> cypherTypesRepository.findById(id))
 				.withMessageMatching(
-						"Error mapping Record<\\{n: \\{__internalNeo4jId__: \\d+, id: NULL, someBoolean: NULL, __nodeLabels__: \\[\"NonExistingPrimitives\"\\]\\}\\}>")
-				.withStackTraceContaining("unboxBoolean")
-				.withRootCauseInstanceOf(NullPointerException.class);
+						"Error mapping Record<\\{n: \\{__internalNeo4jId__: \\d+, aBoolean: NULL, aString: NULL, aLong: NULL, anOffsetTime: NULL, aLocalDateTime: NULL, aDouble: NULL, aByteArray: NULL, aPoint: NULL, aZeroDuration: NULL, aZoneDateTime: NULL, __nodeLabels__: \\[\"CypherTypes\"], aLocalDate: NULL, aZeroPeriod: NULL, anIsoDuration: NULL, aLocalTime: NULL, id: NULL}}>")
+				.withStackTraceContaining("Illegal arguments for constructor")
+				.withRootCauseInstanceOf(IllegalArgumentException.class);
 	}
 
 	@TestFactory
@@ -165,10 +163,10 @@ Stream<DynamicNode> conversionsShouldBeAppliedToEntities() {
 							() -> assertThat(ReflectionTestUtils.getField(thing, a.getKey()))
 									.isEqualTo(a.getValue()))));
 
-			DynamicContainer writes = DynamicContainer.dynamicContainer("write", entry.getValue().entrySet().stream()
-					.map(a -> DynamicTest
-							.dynamicTest(a.getKey(),
-									() -> assertWrite(copyOfThing, a.getKey(), defaultConversionService))));
+			DynamicContainer writes = DynamicContainer.dynamicContainer("write", entry.getValue().keySet().stream()
+					.map(o -> DynamicTest
+							.dynamicTest(o,
+									() -> assertWrite(copyOfThing, o, defaultConversionService))));
 
 			return DynamicContainer.dynamicContainer(entry.getKey(), Arrays.asList(reads, writes));
 		});
@@ -179,8 +177,6 @@ void assertWrite(Object thing, String fieldName, ConversionService conversionSer
 		long id = (long) ReflectionTestUtils.getField(thing, "id");
 		Object domainValue = ReflectionTestUtils.getField(thing, fieldName);
 
-		Field field = ReflectionUtils.findField(thing.getClass(), fieldName);
-		Optional<ConvertWith> annotation = AnnotationUtils.findAnnotation(field, ConvertWith.class);
 		Function<Object, Value> conversion;
 		if (fieldName.equals("dateAsLong")) {
 			conversion = o -> Values.value(((Date) o).getTime());
@@ -192,8 +188,7 @@ void assertWrite(Object thing, String fieldName, ConversionService conversionSer
 		Value driverValue;
 		if (domainValue != null && Collection.class.isAssignableFrom(domainValue.getClass())) {
 			Collection<?> sourceCollection = (Collection<?>) domainValue;
-			Object[] targetCollection = (sourceCollection).stream()
-					.map(element -> conversion.apply(element)).toArray();
+			Object[] targetCollection = (sourceCollection).stream().map(conversion).toArray();
 			driverValue = Values.value(targetCollection);
 		} else {
 			driverValue = conversion.apply(domainValue);
@@ -243,6 +238,35 @@ void parametersTargetingConvertedAttributesMustBeConverted(@Autowired CustomType
 				.hasSizeGreaterThan(0);
 	}
 
+	@Test // GH-2348
+	void nonExistingPrimitivesShouldNotFailWithFieldAccess(@Autowired Neo4jTemplate template) {
+		Long id;
+		try (Session session = neo4jConnectionSupport.getDriver().session(bookmarkCapture.createSessionConfig())) {
+
+			id = session.writeTransaction(tx -> tx.run("CREATE (n:ThingWithAllCypherTypes2) RETURN id(n)").single().get(0).asLong());
+			bookmarkCapture.seedWith(session.lastBookmark());
+		}
+
+		Optional<ThingWithAllCypherTypes2> optionalResult = template.findById(id, ThingWithAllCypherTypes2.class);
+		assertThat(optionalResult).hasValueSatisfying(result -> {
+			assertThat(result.isABoolean()).isFalse();
+			assertThat(result.getALong()).isEqualTo(0L);
+			assertThat(result.getAnInt()).isEqualTo(0);
+			assertThat(result.getADouble()).isEqualTo(0.0);
+			assertThat(result.getAString()).isNull();
+			assertThat(result.getAByteArray()).isNull();
+			assertThat(result.getALocalDate()).isNull();
+			assertThat(result.getAnOffsetTime()).isNull();
+			assertThat(result.getALocalTime()).isNull();
+			assertThat(result.getAZoneDateTime()).isNull();
+			assertThat(result.getALocalDateTime()).isNull();
+			assertThat(result.getAnIsoDuration()).isNull();
+			assertThat(result.getAPoint()).isNull();
+			assertThat(result.getAZeroPeriod()).isNull();
+			assertThat(result.getAZeroDuration()).isNull();
+		});
+	}
+
 	public interface ConvertedIDsRepository extends Neo4jRepository<ThingWithUUIDID, UUID> {
 	}
 
@@ -255,18 +279,11 @@ public interface AdditionalTypesRepository extends Neo4jRepository<ThingWithAllA
 	public interface SpatialTypesRepository extends Neo4jRepository<ThingWithAllSpatialTypes, Long> {
 	}
 
-	public interface NonExistingPrimitivesRepository extends Neo4jRepository<ThingWithNonExistingPrimitives, Long> {
-	}
-
 	public interface CustomTypesRepository extends Neo4jRepository<ThingWithCustomTypes, Long> {
 
 		List<ThingWithCustomTypes> findAllByDateAsString(Date theDate);
 	}
 
-	public interface ThingWithCompositePropertiesRepository
-			extends Neo4jRepository<ThingWithCompositeProperties, Long> {
-	}
-
 	@Configuration
 	@EnableNeo4jRepositories(considerNestedRepositories = true)
 	@EnableTransactionManagement
@@ -284,7 +301,7 @@ public Neo4jConversions neo4jConversions() {
 
 		@Bean
 		public BookmarkCapture bookmarkCapture() {
-			return new BookmarkCapture();
+			return Neo4jConversionsITBase.bookmarkCapture;
 		}
 
 		@Override

src/test/java/org/springframework/data/neo4j/integration/shared/common/ThingWithAllCypherTypes2.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.integration.shared.common;
+
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Setter;
+
+import java.time.Duration;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.time.OffsetTime;
+import java.time.Period;
+import java.time.ZonedDateTime;
+
+import org.neo4j.driver.types.IsoDuration;
+import org.neo4j.driver.types.Point;
+import org.springframework.data.neo4j.core.schema.GeneratedValue;
+import org.springframework.data.neo4j.core.schema.Id;
+import org.springframework.data.neo4j.core.schema.Node;
+
+/**
+ * Similar to {@link ThingWithAllCypherTypes} but using field access.
+ * @author Michael J. Simons
+ */
+@Node
+@Getter
+@Setter
+@RequiredArgsConstructor
+public class ThingWithAllCypherTypes2 {
+
+	@Id @GeneratedValue private final Long id;
+
+	private boolean aBoolean;
+
+	private long aLong;
+
+	private int anInt;
+
+	private double aDouble;
+
+	private String aString;
+
+	private byte[] aByteArray;
+
+	private LocalDate aLocalDate;
+
+	private OffsetTime anOffsetTime;
+
+	private LocalTime aLocalTime;
+
+	private ZonedDateTime aZoneDateTime;
+
+	private LocalDateTime aLocalDateTime;
+
+	private IsoDuration anIsoDuration;
+
+	private Point aPoint;
+
+	private Period aZeroPeriod;
+
+	private Duration aZeroDuration;
+}