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.mapping.AssociationHandler;
 import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.PersistentPropertyAccessor;
@@ -52,6 +53,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;
@@ -107,7 +109,7 @@ public <R> R read(Class<R> targetType, MapAccessor mapAccessor) {
 		try {
 			return map(queryRoot, queryRoot, rootNodeDescription, new HashSet<>());
 		} catch (Exception e) {
-			throw new MappingException("Error mapping " + mapAccessor.toString(), e);
+			throw new MappingException("Error mapping " + mapAccessor, e);
 		}
 	}
 
@@ -267,6 +269,7 @@ private <ET> ET map(MapAccessor queryResult, MapAccessor allValues, Neo4jPersist
 			Collection<RelationshipDescription> relationships = nodeDescription
 					.getRelationshipsInHierarchy(includeAllFields);
 
+			boolean isKotlinType = KotlinDetector.isKotlinType(concreteNodeDescription.getType());
 			ET instance = instantiate(concreteNodeDescription, queryResult, allValues, relationships,
 					nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity);
 
@@ -279,7 +282,7 @@ private <ET> ET map(MapAccessor queryResult, MapAccessor allValues, Neo4jPersist
 				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
@@ -393,22 +396,24 @@ 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));
 			}
 		};
 	}
@@ -427,6 +432,11 @@ private AssociationHandler<Neo4jPersistentProperty> populateFrom(MapAccessor que
 		};
 	}
 
+	private static Object getValueOrDefault(boolean ownerIsKotlinType, Class<?> rawType, @Nullable Object value) {
+
+		return value == null && !ownerIsKotlinType && rawType.isPrimitive() ? ReflectionUtils.getPrimitiveDefault(rawType) : value;
+	}
+
 	private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty persistentProperty, MapAccessor values,
 			MapAccessor allValues, RelationshipDescription relationshipDescription) {
 

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;
@@ -53,22 +51,20 @@
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.data.mapping.MappingException;
 import org.springframework.data.neo4j.config.AbstractNeo4jConfig;
-import org.springframework.data.neo4j.core.convert.ConvertWith;
 import org.springframework.data.neo4j.core.convert.Neo4jConversions;
+import org.springframework.data.neo4j.core.Neo4jTemplate;
+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;
 import org.springframework.data.neo4j.test.Neo4jIntegrationTest;
 import org.springframework.test.util.ReflectionTestUtils;
 import org.springframework.transaction.annotation.EnableTransactionManagement;
-import org.springframework.util.ReflectionUtils;
 
 /**
  * @author Michael J. Simons
@@ -78,9 +74,7 @@
 @Neo4jIntegrationTest
 class TypeConversionIT extends Neo4jConversionsITBase {
 
-	private final Driver driver;
-
-	@Autowired CypherTypesRepository cypherTypesRepository;
+	private final CypherTypesRepository cypherTypesRepository;
 
 	private final AdditionalTypesRepository additionalTypesRepository;
 
@@ -90,11 +84,10 @@ class TypeConversionIT extends Neo4jConversionsITBase {
 
 	private final DefaultConversionService defaultConversionService;
 
-	@Autowired TypeConversionIT(Driver driver, CypherTypesRepository cypherTypesRepository,
+	@Autowired TypeConversionIT(CypherTypesRepository cypherTypesRepository,
 			AdditionalTypesRepository additionalTypesRepository, SpatialTypesRepository spatialTypesRepository,
 			CustomTypesRepository customTypesRepository,
 			Neo4jConversions neo4jConversions) {
-		this.driver = driver;
 		this.cypherTypesRepository = cypherTypesRepository;
 		this.additionalTypesRepository = additionalTypesRepository;
 		this.spatialTypesRepository = spatialTypesRepository;
@@ -104,14 +97,20 @@ class TypeConversionIT extends Neo4jConversionsITBase {
 	}
 
 	@Test
-	void thereShallBeNoDefaultValuesForNonExistingAttributes(@Autowired NonExistingPrimitivesRepository repository) {
+	void thereShallBeNoDefaultValuesForNonExistingAttributes() {
+
+		Long id;
+		try (Session session = neo4jConnectionSupport.getDriver().session()) {
+
+			id = session.writeTransaction(tx -> tx.run("CREATE (n:CypherTypes) RETURN id(n)").single().get(0).asLong());
+		}
 
 		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
@@ -158,10 +157,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));
 		});
@@ -172,8 +171,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());
@@ -185,8 +182,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);
@@ -236,6 +232,34 @@ void parametersTargetingConvertedAttributesMustBeConverted(@Autowired CustomType
 				.hasSizeGreaterThan(0);
 	}
 
+	@Test // GH-2348
+	void nonExistingPrimitivesShouldNotFailWithFieldAccess(@Autowired Neo4jTemplate template) {
+		Long id;
+		try (Session session = neo4jConnectionSupport.getDriver().session()) {
+
+			id = session.writeTransaction(tx -> tx.run("CREATE (n:ThingWithAllCypherTypes2) RETURN id(n)").single().get(0).asLong());
+		}
+
+		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> {
 	}
 
@@ -248,18 +272,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

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;
+}

src/test/java/org/springframework/data/neo4j/integration/shared/common/ThingWithNonExistingPrimitives.java

@@ -187,7 +187,6 @@ GeographicPoint3d asGeo3d(double height) {
 	protected static long ID_OF_CYPHER_TYPES_NODE;
 	protected static long ID_OF_ADDITIONAL_TYPES_NODE;
 	protected static long ID_OF_SPATIAL_TYPES_NODE;
-	protected static long ID_OF_NON_EXISTING_PRIMITIVES_NODE;
 	protected static long ID_OF_CUSTOM_TYPE_NODE;
 
 	@BeforeAll
@@ -199,9 +198,6 @@ static void prepareData() {
 
 				w.run("MATCH (n) detach delete n");
 
-				ID_OF_NON_EXISTING_PRIMITIVES_NODE = w.run("CREATE (n:NonExistingPrimitives) RETURN id(n) AS id").single()
-						.get("id").asLong();
-
 				parameters = new HashMap<>();
 				parameters.put("aByteArray", "A thing".getBytes());
 				ID_OF_CYPHER_TYPES_NODE = w.run("CREATE (n:CypherTypes) SET  n.aBoolean = true,\n"