Polishing.

This change extracts entity modifying behaviour into separate methods, so it doesn't appear as an unexpected side effect of the creation of aggregate changes.

Also some formatting.

Original pull request #1196
See #1137

Author: schauder

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

@@ -150,8 +150,9 @@ public <T> T save(T instance) {
 
 		RelationalPersistentEntity<?> persistentEntity = context.getRequiredPersistentEntity(instance.getClass());
 
-		Function<T, MutableAggregateChange<T>> changeCreator = persistentEntity.isNew(instance) ? this::createInsertChange
-				: this::createUpdateChange;
+		Function<T, MutableAggregateChange<T>> changeCreator = persistentEntity.isNew(instance)
+				? entity -> createInsertChange(prepareVersionForInsert(entity))
+				: entity -> createUpdateChange(prepareVersionForUpdate(entity));
 
 		return store(instance, changeCreator, persistentEntity);
 	}
@@ -170,7 +171,7 @@ public <T> T insert(T instance) {
 
 		RelationalPersistentEntity<?> persistentEntity = context.getRequiredPersistentEntity(instance.getClass());
 
-		return store(instance, this::createInsertChange, persistentEntity);
+		return store(instance, entity -> createInsertChange(prepareVersionForInsert(entity)), persistentEntity);
 	}
 
 	/**
@@ -187,7 +188,7 @@ public <T> T update(T instance) {
 
 		RelationalPersistentEntity<?> persistentEntity = context.getRequiredPersistentEntity(instance.getClass());
 
-		return store(instance, this::createUpdateChange, persistentEntity);
+		return store(instance, entity -> createUpdateChange(prepareVersionForUpdate(entity)), persistentEntity);
 	}
 
 	/*
@@ -365,6 +366,21 @@ private <T> void deleteTree(Object id, @Nullable T entity, Class<T> domainType)
 
 	private <T> MutableAggregateChange<T> createInsertChange(T instance) {
 
+		MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(instance);
+		jdbcEntityInsertWriter.write(instance, aggregateChange);
+		return aggregateChange;
+	}
+
+	private <T> MutableAggregateChange<T> createUpdateChange(EntityAndPreviousVersion<T> entityAndVersion) {
+
+		MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(entityAndVersion.entity,
+				entityAndVersion.version);
+		jdbcEntityUpdateWriter.write(entityAndVersion.entity, aggregateChange);
+		return aggregateChange;
+	}
+
+	private <T> T prepareVersionForInsert(T instance) {
+
 		RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(instance);
 		T preparedInstance = instance;
 		if (persistentEntity.hasVersionProperty()) {
@@ -375,31 +391,26 @@ private <T> MutableAggregateChange<T> createInsertChange(T instance) {
 			preparedInstance = RelationalEntityVersionUtils.setVersionNumberOnEntity( //
 					instance, initialVersion, persistentEntity, converter);
 		}
-		MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(preparedInstance);
-		jdbcEntityInsertWriter.write(preparedInstance, aggregateChange);
-		return aggregateChange;
+		return preparedInstance;
 	}
 
-	private <T> MutableAggregateChange<T> createUpdateChange(T instance) {
+	private <T> EntityAndPreviousVersion<T> prepareVersionForUpdate(T instance) {
 
 		RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(instance);
 		T preparedInstance = instance;
 		Number previousVersion = null;
 		if (persistentEntity.hasVersionProperty()) {
 			// If the root aggregate has a version property, increment it.
-			previousVersion = RelationalEntityVersionUtils.getVersionNumberFromEntity(instance,
-					persistentEntity, converter);
+			previousVersion = RelationalEntityVersionUtils.getVersionNumberFromEntity(instance, persistentEntity, converter);
 
 			Assert.notNull(previousVersion, "The root aggregate cannot be updated because the version property is null.");
 
 			long newVersion = previousVersion.longValue() + 1;
 
-			preparedInstance = RelationalEntityVersionUtils.setVersionNumberOnEntity(instance, newVersion,
-					persistentEntity, converter);
+			preparedInstance = RelationalEntityVersionUtils.setVersionNumberOnEntity(instance, newVersion, persistentEntity,
+					converter);
 		}
-		MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(preparedInstance, previousVersion);
-		jdbcEntityUpdateWriter.write(preparedInstance, aggregateChange);
-		return aggregateChange;
+		return new EntityAndPreviousVersion<>(preparedInstance, previousVersion);
 	}
 
 	@SuppressWarnings("unchecked")
@@ -489,4 +500,16 @@ private <T> T triggerBeforeDelete(@Nullable T aggregateRoot, Object id, MutableA
 
 		return null;
 	}
+
+	private static class EntityAndPreviousVersion<T> {
+
+		private final T entity;
+		private final Number version;
+
+		EntityAndPreviousVersion(T entity, @Nullable Number version) {
+
+			this.entity = entity;
+			this.version = version;
+		}
+	}
 }

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

@@ -821,9 +821,10 @@ void saveAndUpdateAggregateWithImmutableVersion() {
 
 	@Test // GH-1137
 	void testUpdateEntityWithVersionDoesNotTriggerAnewConstructorInvocation() {
+
 		AggregateWithImmutableVersion aggregateWithImmutableVersion = new AggregateWithImmutableVersion(null, null);
 
-		final AggregateWithImmutableVersion savedRoot = template.save(aggregateWithImmutableVersion);
+		AggregateWithImmutableVersion savedRoot = template.save(aggregateWithImmutableVersion);
 
 		assertThat(savedRoot).isNotNull();
 		assertThat(savedRoot.version).isEqualTo(0L);
@@ -836,12 +837,12 @@ void testUpdateEntityWithVersionDoesNotTriggerAnewConstructorInvocation() {
 
 		AggregateWithImmutableVersion.clearConstructorInvocationData();
 
-		final AggregateWithImmutableVersion updatedRoot = template.save(savedRoot);
+		AggregateWithImmutableVersion updatedRoot = template.save(savedRoot);
 
 		assertThat(updatedRoot).isNotNull();
 		assertThat(updatedRoot.version).isEqualTo(1L);
 
-		// Expect only one assignnment of the version to AggregateWithImmutableVersion
+		// Expect only one assignment of the version to AggregateWithImmutableVersion
 		assertThat(AggregateWithImmutableVersion.constructorInvocations).containsOnly(new ConstructorInvocation(savedRoot.id, updatedRoot.version));
 	}
 
@@ -1261,6 +1262,7 @@ public static void clearConstructorInvocationData() {
 		}
 
 		public AggregateWithImmutableVersion(Long id, Long version) {
+
 			constructorInvocations.add(new ConstructorInvocation(id, version));
 			this.id = id;
 			this.version = version;
@@ -1270,8 +1272,9 @@ public AggregateWithImmutableVersion(Long id, Long version) {
 	@Value
 	@EqualsAndHashCode
 	private static class ConstructorInvocation {
-		private Long id;
-		private Long version;
+
+		Long id;
+		Long version;
 	}
 
 	@Data

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

@@ -22,8 +22,8 @@
 
 import lombok.AllArgsConstructor;
 import lombok.Data;
-
 import lombok.RequiredArgsConstructor;
+
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -115,7 +115,7 @@ public void callbackOnSave() {
 		assertThat(last).isEqualTo(third);
 	}
 
-	@Test
+	@Test // GH-1137
 	void savePreparesInstanceWithInitialVersion_onInsert() {
 
 		EntityWithVersion entity = new EntityWithVersion(1L);
@@ -125,11 +125,12 @@ void savePreparesInstanceWithInitialVersion_onInsert() {
 
 		ArgumentCaptor<Object> aggregateRootCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeSaveCallback.class), aggregateRootCaptor.capture(), any());
+
 		EntityWithVersion afterConvert = (EntityWithVersion) aggregateRootCaptor.getValue();
 		assertThat(afterConvert.getVersion()).isEqualTo(0L);
 	}
 
-	@Test
+	@Test // GH-1137
 	void savePreparesInstanceWithInitialVersion_onInsert_whenVersionPropertyIsImmutable() {
 
 		EntityWithImmutableVersion entity = new EntityWithImmutableVersion(1L, null);
@@ -139,11 +140,12 @@ void savePreparesInstanceWithInitialVersion_onInsert_whenVersionPropertyIsImmuta
 
 		ArgumentCaptor<Object> aggregateRootCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeSaveCallback.class), aggregateRootCaptor.capture(), any());
+
 		EntityWithImmutableVersion afterConvert = (EntityWithImmutableVersion) aggregateRootCaptor.getValue();
 		assertThat(afterConvert.getVersion()).isEqualTo(0L);
 	}
 
-	@Test  // DATAJDBC-507
+	@Test // GH-1137
 	void savePreparesInstanceWithInitialVersion_onInsert_whenVersionPropertyIsPrimitiveType() {
 
 		EntityWithPrimitiveVersion entity = new EntityWithPrimitiveVersion(1L);
@@ -153,11 +155,12 @@ void savePreparesInstanceWithInitialVersion_onInsert_whenVersionPropertyIsPrimit
 
 		ArgumentCaptor<Object> aggregateRootCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeSaveCallback.class), aggregateRootCaptor.capture(), any());
+
 		EntityWithPrimitiveVersion afterConvert = (EntityWithPrimitiveVersion) aggregateRootCaptor.getValue();
 		assertThat(afterConvert.getVersion()).isEqualTo(1L);
 	}
 
-	@Test  // DATAJDBC-507
+	@Test // GH-1137
 	void savePreparesInstanceWithInitialVersion_onInsert__whenVersionPropertyIsImmutableAndPrimitiveType() {
 
 		EntityWithImmutablePrimitiveVersion entity = new EntityWithImmutablePrimitiveVersion(1L, 0L);
@@ -167,11 +170,13 @@ void savePreparesInstanceWithInitialVersion_onInsert__whenVersionPropertyIsImmut
 
 		ArgumentCaptor<Object> aggregateRootCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeSaveCallback.class), aggregateRootCaptor.capture(), any());
-		EntityWithImmutablePrimitiveVersion afterConvert = (EntityWithImmutablePrimitiveVersion) aggregateRootCaptor.getValue();
+
+		EntityWithImmutablePrimitiveVersion afterConvert = (EntityWithImmutablePrimitiveVersion) aggregateRootCaptor
+				.getValue();
 		assertThat(afterConvert.getVersion()).isEqualTo(1L);
 	}
 
-	@Test
+	@Test // GH-1137
 	void savePreparesChangeWithPreviousVersion_onUpdate() {
 
 		when(dataAccessStrategy.updateWithVersion(any(), any(), any())).thenReturn(true);
@@ -183,11 +188,12 @@ void savePreparesChangeWithPreviousVersion_onUpdate() {
 
 		ArgumentCaptor<Object> aggregateChangeCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeSaveCallback.class), any(), aggregateChangeCaptor.capture());
+
 		MutableAggregateChange<?> aggregateChange = (MutableAggregateChange<?>) aggregateChangeCaptor.getValue();
 		assertThat(aggregateChange.getPreviousVersion()).isEqualTo(1L);
 	}
 
-	@Test
+	@Test // GH-1137
 	void savePreparesInstanceWithNextVersion_onUpdate() {
 
 		when(dataAccessStrategy.updateWithVersion(any(), any(), any())).thenReturn(true);
@@ -199,11 +205,12 @@ void savePreparesInstanceWithNextVersion_onUpdate() {
 
 		ArgumentCaptor<Object> aggregateRootCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeSaveCallback.class), aggregateRootCaptor.capture(), any());
+
 		EntityWithVersion afterConvert = (EntityWithVersion) aggregateRootCaptor.getValue();
 		assertThat(afterConvert.getVersion()).isEqualTo(2L);
 	}
 
-	@Test
+	@Test // GH-1137
 	void savePreparesInstanceWithNextVersion_onUpdate_whenVersionPropertyIsImmutable() {
 
 		when(dataAccessStrategy.updateWithVersion(any(), any(), any())).thenReturn(true);
@@ -218,7 +225,7 @@ void savePreparesInstanceWithNextVersion_onUpdate_whenVersionPropertyIsImmutable
 		assertThat(afterConvert.getVersion()).isEqualTo(2L);
 	}
 
-	@Test
+	@Test // GH-1137
 	void deletePreparesChangeWithPreviousVersion_onDeleteByInstance() {
 
 		EntityWithImmutableVersion entity = new EntityWithImmutableVersion(1L, 1L);
@@ -228,11 +235,11 @@ void deletePreparesChangeWithPreviousVersion_onDeleteByInstance() {
 
 		ArgumentCaptor<Object> aggregateChangeCaptor = ArgumentCaptor.forClass(Object.class);
 		verify(callbacks).callback(eq(BeforeDeleteCallback.class), any(), aggregateChangeCaptor.capture());
+
 		MutableAggregateChange<?> aggregateChange = (MutableAggregateChange<?>) aggregateChangeCaptor.getValue();
 		assertThat(aggregateChange.getPreviousVersion()).isEqualTo(1L);
 	}
 
-
 	@Test // DATAJDBC-393
 	public void callbackOnDelete() {