DATAJDBC-370 - Fixed handling of entities with no withers.

We tried to set all the properties, even when they were already set via constructor.
Fixed it by unifying the three instances where we created and populated instances.

Original Pull Request: #151

Author: schauder

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java

@@ -30,7 +30,6 @@
 import org.springframework.data.jdbc.core.mapping.AggregateReference;
 import org.springframework.data.jdbc.support.JdbcUtil;
 import org.springframework.data.mapping.MappingException;
-import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentPropertyAccessor;
 import org.springframework.data.mapping.PreferredConstructor;
 import org.springframework.data.mapping.context.MappingContext;
@@ -285,30 +284,22 @@ public ReadingContext(RelationalPersistentEntity<T> entity, DataAccessStrategy a
 		}
 
 		private ReadingContext<?> extendBy(RelationalPersistentProperty property) {
-			return new ReadingContext<>(entity, accessStrategy, resultSet, path.extendBy(property));
+			return new ReadingContext(getMappingContext().getRequiredPersistentEntity(property.getActualType()),
+					accessStrategy, resultSet, path.extendBy(property));
 		}
 
 		T mapRow() {
 
 			RelationalPersistentProperty idProperty = entity.getIdProperty();
 
-			Object idValue = null;
-			if (idProperty != null) {
-				idValue = readFrom(idProperty);
-			}
-
-			T result = createInstanceInternal(entity, idValue);
+			Object idValue = idProperty == null ? null : readFrom(idProperty);
 
-			return entity.requiresPropertyPopulation() //
-					? populateProperties(result) //
-					: result;
+			return createInstanceInternal(idValue);
 		}
 
-		private T populateProperties(T result) {
+		private T populateProperties(T instance, @Nullable Object idValue) {
 
-			PersistentPropertyAccessor<T> propertyAccessor = getPropertyAccessor(entity, result);
-
-			Object id = idProperty == null ? null : readFrom(idProperty);
+			PersistentPropertyAccessor<T> propertyAccessor = getPropertyAccessor(entity, instance);
 
 			PreferredConstructor<T, RelationalPersistentProperty> persistenceConstructor = entity.getPersistenceConstructor();
 
@@ -318,7 +309,7 @@ private T populateProperties(T result) {
 					continue;
 				}
 
-				propertyAccessor.setProperty(property, readOrLoadProperty(id, property));
+				propertyAccessor.setProperty(property, readOrLoadProperty(idValue, property));
 			}
 
 			return propertyAccessor.getBean();
@@ -358,27 +349,17 @@ private Object readFrom(RelationalPersistentProperty property) {
 		}
 
 		@SuppressWarnings("unchecked")
-		private Object readEmbeddedEntityFrom(@Nullable Object id, RelationalPersistentProperty property) {
+		private Object readEmbeddedEntityFrom(@Nullable Object idValue, RelationalPersistentProperty property) {
 
 			ReadingContext newContext = extendBy(property);
 
-			RelationalPersistentEntity<?> entity = getMappingContext().getRequiredPersistentEntity(property.getActualType());
-
-			Object instance = newContext.createInstanceInternal(entity, null);
-
-			PersistentPropertyAccessor<?> accessor = getPropertyAccessor((PersistentEntity<Object, ?>) entity, instance);
-
-			for (RelationalPersistentProperty p : entity) {
-				accessor.setProperty(p, newContext.readOrLoadProperty(id, p));
-			}
-
-			return instance;
+			return newContext.createInstanceInternal(idValue);
 		}
 
 		@Nullable
 		private <S> S readEntityFrom(RelationalPersistentProperty property, PersistentPropertyPathExtension path) {
 
-			ReadingContext<?> newContext = extendBy(property);
+			ReadingContext<S> newContext = (ReadingContext<S>) extendBy(property);
 
 			RelationalPersistentEntity<S> entity = (RelationalPersistentEntity<S>) getMappingContext()
 					.getRequiredPersistentEntity(property.getActualType());
@@ -398,15 +379,7 @@ private <S> S readEntityFrom(RelationalPersistentProperty property, PersistentPr
 				return null;
 			}
 
-			S instance = newContext.createInstanceInternal(entity, idValue);
-
-			PersistentPropertyAccessor<S> accessor = getPropertyAccessor(entity, instance);
-
-			for (RelationalPersistentProperty p : entity) {
-				accessor.setProperty(p, newContext.readOrLoadProperty(idValue, p));
-			}
-
-			return instance;
+			return newContext.createInstanceInternal(idValue);
 		}
 
 		@Nullable
@@ -419,9 +392,9 @@ private Object getObjectFromResultSet(String backreferenceName) {
 			}
 		}
 
-		private <S> S createInstanceInternal(RelationalPersistentEntity<S> entity, @Nullable Object idValue) {
+		private T createInstanceInternal(@Nullable Object idValue) {
 
-			return createInstance(entity, parameter -> {
+			T instance = createInstance(entity, parameter -> {
 
 				String parameterName = parameter.getName();
 
@@ -431,6 +404,7 @@ private <S> S createInstanceInternal(RelationalPersistentEntity<S> entity, @Null
 
 				return readOrLoadProperty(idValue, property);
 			});
+			return populateProperties(instance, idValue);
 		}
 
 	}

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/EntityRowMapperUnitTests.java

@@ -284,6 +284,131 @@ public void chainedEntitiesWithoutId() throws SQLException {
 		fixture.assertOn(extracted);
 	}
 
+	// Model classes to be used in tests
+
+	@RequiredArgsConstructor
+	static class TrivialImmutable {
+
+		@Id private final Long id;
+		private final String name;
+	}
+
+	static class Trivial {
+
+		@Id Long id;
+		String name;
+	}
+
+	static class OneToOne {
+
+		@Id Long id;
+		String name;
+		Trivial child;
+	}
+
+	@RequiredArgsConstructor
+	static class OneToOneImmutable {
+
+		private final @Id Long id;
+		private final String name;
+		private final TrivialImmutable child;
+	}
+
+	static class OneToSet {
+
+		@Id Long id;
+		String name;
+		Set<Trivial> children;
+	}
+
+	static class OneToMap {
+
+		@Id Long id;
+		String name;
+		Map<String, Trivial> children;
+	}
+
+	static class OneToList {
+
+		@Id Long id;
+		String name;
+		List<Trivial> children;
+	}
+
+	static class EmbeddedEntity {
+
+		@Id Long id;
+		String name;
+		@Embedded("prefix_") Trivial children;
+	}
+
+	private static class DontUseSetter {
+		String value;
+
+		DontUseSetter(@Param("value") String value) {
+			this.value = "setThroughConstructor:" + value;
+		}
+	}
+
+	static class MixedProperties {
+
+		final String one;
+		String two;
+		final String three;
+
+		@PersistenceConstructor
+		MixedProperties(String one) {
+			this.one = one;
+			this.three = "unset";
+		}
+
+		private MixedProperties(String one, String two, String three) {
+
+			this.one = one;
+			this.two = two;
+			this.three = three;
+		}
+
+		MixedProperties withThree(String three) {
+			return new MixedProperties(one, two, three);
+		}
+	}
+
+	@AllArgsConstructor
+	static class EntityWithListInConstructor {
+
+		@Id final Long id;
+
+		final List<Trivial> content;
+	}
+
+	static class NoIdChain0 {
+		String zeroValue;
+	}
+
+	static class NoIdChain1 {
+		String oneValue;
+		NoIdChain0 chain0;
+	}
+
+	static class NoIdChain2 {
+		String twoValue;
+		NoIdChain1 chain1;
+	}
+
+	static class NoIdChain3 {
+		String threeValue;
+		NoIdChain2 chain2;
+	}
+
+	static class NoIdChain4 {
+		@Id Long four;
+		String fourValue;
+		NoIdChain3 chain3;
+	}
+
+	// Infrastructure for assertions and constructing mocks
+
 	private <T> FixtureBuilder<T> buildFixture() {
 		return new FixtureBuilder<>();
 	}
@@ -418,129 +543,6 @@ private boolean next() {
 		}
 	}
 
-	@RequiredArgsConstructor
-	@Wither
-	static class TrivialImmutable {
-
-		@Id private final Long id;
-		private final String name;
-	}
-
-	static class Trivial {
-
-		@Id Long id;
-		String name;
-	}
-
-	static class OneToOne {
-
-		@Id Long id;
-		String name;
-		Trivial child;
-	}
-
-	@RequiredArgsConstructor
-	@Wither
-	static class OneToOneImmutable {
-
-		private final @Id Long id;
-		private final String name;
-		private final TrivialImmutable child;
-	}
-
-	static class OneToSet {
-
-		@Id Long id;
-		String name;
-		Set<Trivial> children;
-	}
-
-	static class OneToMap {
-
-		@Id Long id;
-		String name;
-		Map<String, Trivial> children;
-	}
-
-	static class OneToList {
-
-		@Id Long id;
-		String name;
-		List<Trivial> children;
-	}
-
-	static class EmbeddedEntity {
-
-		@Id Long id;
-		String name;
-		@Embedded("prefix_") Trivial children;
-	}
-
-	private static class DontUseSetter {
-		String value;
-
-		DontUseSetter(@Param("value") String value) {
-			this.value = "setThroughConstructor:" + value;
-		}
-	}
-
-	static class MixedProperties {
-
-		final String one;
-		String two;
-		final String three;
-
-		@PersistenceConstructor
-		MixedProperties(String one) {
-			this.one = one;
-			this.three = "unset";
-		}
-
-		private MixedProperties(String one, String two, String three) {
-
-			this.one = one;
-			this.two = two;
-			this.three = three;
-		}
-
-		MixedProperties withThree(String three) {
-			return new MixedProperties(one, two, three);
-		}
-	}
-
-	@AllArgsConstructor
-	static class EntityWithListInConstructor {
-
-		@Id final Long id;
-
-		final List<Trivial> content;
-	}
-
-	static class NoIdChain0 {
-		String zeroValue;
-	}
-
-	static class NoIdChain1 {
-		String oneValue;
-		NoIdChain0 chain0;
-	}
-
-	static class NoIdChain2 {
-		String twoValue;
-		NoIdChain1 chain1;
-	}
-
-	static class NoIdChain3 {
-		String threeValue;
-		NoIdChain2 chain2;
-	}
-
-	static class NoIdChain4 {
-		@Id Long four;
-		String fourValue;
-		NoIdChain3 chain3;
-	}
-
 	private interface SetValue<T> {
 		SetColumns<T> value(Object value);
 
@@ -609,6 +611,7 @@ public SetValue<T> endUpIn(Function<T, Object> extractor) {
 
 	@AllArgsConstructor
 	private static class Fixture<T> {
+
 		final ResultSet resultSet;
 		final List<Expectation<T>> expectations;