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

@@ -146,8 +146,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);
 }
@@ -166,7 +167,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);
 }
 
 /**
@@ -183,7 +184,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);
 }
 
 @Override
@@ -321,6 +322,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()) {
@@ -331,31 +347,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")
@@ -445,4 +456,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() {