This reintroduces the behavior of BeforeSaveCallbacks to allow for creating an Id for an insert operation.

This behavior is strictly speaking not correct, since this kind of action should be performed in a BeforeConversionCallback.

Closes #1232

Author: schauder

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

@@ -31,6 +31,8 @@
 import org.springframework.data.mapping.IdentifierAccessor;
 import org.springframework.data.mapping.callback.EntityCallbacks;
 import org.springframework.data.relational.core.conversion.AggregateChange;
+import org.springframework.data.relational.core.conversion.DbAction;
+import org.springframework.data.relational.core.conversion.IdValueSource;
 import org.springframework.data.relational.core.conversion.MutableAggregateChange;
 import org.springframework.data.relational.core.conversion.RelationalEntityDeleteWriter;
 import org.springframework.data.relational.core.conversion.RelationalEntityInsertWriter;
@@ -339,9 +341,19 @@ private <T> T store(T aggregateRoot, Function<T, MutableAggregateChange<T>> chan
 
 		MutableAggregateChange<T> change = changeCreator.apply(aggregateRoot);
 
-		aggregateRoot = triggerBeforeSave(change.getEntity(), change);
+		T newAggregateRoot = triggerBeforeSave(change.getEntity(), change);
 
-		change.setEntity(aggregateRoot);
+		change.setEntity(newAggregateRoot);
+
+		change.forEachAction(a -> {
+
+			if (a instanceof DbAction.InsertRoot) {
+
+				IdValueSource idValueSource = IdValueSource.forInstance(newAggregateRoot, persistentEntity);
+				((DbAction.InsertRoot<T>) a).updateAction(newAggregateRoot,
+						idValueSource);
+			}
+		});
 
 		T entityAfterExecution = executor.execute(change);
 

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

@@ -39,6 +39,7 @@
 import java.util.stream.IntStream;
 
 import org.assertj.core.api.SoftAssertions;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -60,11 +61,15 @@
 import org.springframework.data.jdbc.testing.EnabledOnFeature;
 import org.springframework.data.jdbc.testing.TestConfiguration;
 import org.springframework.data.jdbc.testing.TestDatabaseFeatures;
+import org.springframework.data.mapping.callback.EntityCallbacks;
 import org.springframework.data.relational.core.conversion.DbActionExecutionException;
+import org.springframework.data.relational.core.conversion.MutableAggregateChange;
 import org.springframework.data.relational.core.mapping.Column;
 import org.springframework.data.relational.core.mapping.MappedCollection;
 import org.springframework.data.relational.core.mapping.RelationalMappingContext;
 import org.springframework.data.relational.core.mapping.Table;
+import org.springframework.data.relational.core.mapping.event.BeforeConvertCallback;
+import org.springframework.data.relational.core.mapping.event.BeforeSaveCallback;
 import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.TestExecutionListeners;
@@ -93,9 +98,17 @@ class JdbcAggregateTemplateIntegrationTests {
 
 	@Autowired JdbcAggregateOperations template;
 	@Autowired NamedParameterJdbcOperations jdbcTemplate;
+	@Autowired Config.ConfigurableBeforeSaveCallback beforeSaveCallback;
+	@Autowired Config.ConfigurableBeforeConvertCallback beforeConvertCallback;
 
 	LegoSet legoSet = createLegoSet("Star Destroyer");
 
+	@BeforeEach
+	void beforeEach() {
+		((JdbcAggregateTemplate) template)
+				.setEntityCallbacks(EntityCallbacks.create(beforeSaveCallback, beforeConvertCallback));
+	}
+
 	/**
 	 * creates an instance of {@link NoIdListChain4} with the following properties:
 	 * <ul>
@@ -258,14 +271,15 @@ void saveAndLoadManyEntitiesWithReferencedEntitySortedAndPaged() {
 	}
 
 	@Test // GH-821
-	@EnabledOnFeature({SUPPORTS_QUOTED_IDS, SUPPORTS_NULL_PRECEDENCE})
+	@EnabledOnFeature({ SUPPORTS_QUOTED_IDS, SUPPORTS_NULL_PRECEDENCE })
 	void saveAndLoadManyEntitiesWithReferencedEntitySortedWithNullPrecedence() {
 
 		template.save(createLegoSet(null));
 		template.save(createLegoSet("Star"));
 		template.save(createLegoSet("Frozen"));
 
-		Iterable<LegoSet> reloadedLegoSets = template.findAll(LegoSet.class, Sort.by(new Sort.Order(Sort.Direction.ASC, "name", Sort.NullHandling.NULLS_LAST)));
+		Iterable<LegoSet> reloadedLegoSets = template.findAll(LegoSet.class,
+				Sort.by(new Sort.Order(Sort.Direction.ASC, "name", Sort.NullHandling.NULLS_LAST)));
 
 		assertThat(reloadedLegoSets) //
 				.extracting("name") //
@@ -843,7 +857,8 @@ void testUpdateEntityWithVersionDoesNotTriggerAnewConstructorInvocation() {
 		assertThat(updatedRoot.version).isEqualTo(1L);
 
 		// Expect only one assignment of the version to AggregateWithImmutableVersion
-		assertThat(AggregateWithImmutableVersion.constructorInvocations).containsOnly(new ConstructorInvocation(savedRoot.id, updatedRoot.version));
+		assertThat(AggregateWithImmutableVersion.constructorInvocations)
+				.containsOnly(new ConstructorInvocation(savedRoot.id, updatedRoot.version));
 	}
 
 	@Test // DATAJDBC-219 Test that a delete with a version attribute works as expected.
@@ -956,6 +971,31 @@ void insertWithIdOnly() {
 		assertThat(template.save(entity).id).isNotNull();
 	}
 
+	@Test // GH-1232
+	@EnabledOnFeature(IS_HSQL)
+	void beforeSaveCallbackEffectsAreVisibleForInsert() {
+
+		beforeSaveCallback.behavior = m -> {
+
+			ManualId result = new ManualId();
+			result.id = 23L;
+			result.content = "changed by before safe";
+
+			return result;
+		};
+
+		ManualId manual = new ManualId();
+		manual.content = "original manual";
+
+		ManualId saved = template.save(manual);
+
+		assertThat(saved.content).isEqualTo("changed by before safe");
+
+		ManualId reloaded = template.findById(saved.id, ManualId.class);
+		assertThat(reloaded.content).isEqualTo("changed by before safe");
+
+	}
+
 	private <T extends Number> void saveAndUpdateAggregateWithVersion(VersionedAggregate aggregate,
 			Function<Number, T> toConcreteNumber) {
 		saveAndUpdateAggregateWithVersion(aggregate, toConcreteNumber, 0);
@@ -1373,6 +1413,11 @@ class WithIdOnly {
 		@Id Long id;
 	}
 
+	static class ManualId {
+		@Id private Long id;
+		private String content;
+	}
+
 	@Configuration
 	@Import(TestConfiguration.class)
 	static class Config {
@@ -1385,7 +1430,46 @@ Class<?> testClass() {
 		@Bean
 		JdbcAggregateOperations operations(ApplicationEventPublisher publisher, RelationalMappingContext context,
 				DataAccessStrategy dataAccessStrategy, JdbcConverter converter) {
+
 			return new JdbcAggregateTemplate(publisher, context, converter, dataAccessStrategy);
 		}
+
+		@Bean
+		ConfigurableBeforeSaveCallback beforeSaveCallback() {
+			return new ConfigurableBeforeSaveCallback();
+		}
+
+		@Bean
+		ConfigurableBeforeConvertCallback beforeConvertCallback() {
+			return new ConfigurableBeforeConvertCallback();
+		}
+
+		private static class ConfigurableBeforeSaveCallback implements BeforeSaveCallback<ManualId> {
+
+			Function<ManualId, ManualId> behavior;
+
+			@Override
+			public ManualId onBeforeSave(ManualId aggregate, MutableAggregateChange<ManualId> aggregateChange) {
+
+				if (behavior == null) {
+					return aggregate;
+				}
+				return behavior.apply(aggregate);
+			}
+		}
+
+		private static class ConfigurableBeforeConvertCallback implements BeforeConvertCallback<ManualId> {
+
+			Function<ManualId, ManualId> behavior;
+
+			@Override
+			public ManualId onBeforeConvert(ManualId aggregate) {
+
+				if (behavior == null) {
+					return aggregate;
+				}
+				return behavior.apply(aggregate);
+			}
+		}
 	}
 }

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java

@@ -110,7 +110,8 @@ public void manuallyGeneratedId() {
 
 		assertThat(saved.getId()).isNotNull();
 
-		assertThat(immutableWithManualIdEntityRepository.findAll()).hasSize(1);
+		final Iterable<ImmutableWithManualIdEntity> reloaded = immutableWithManualIdEntityRepository.findAll();
+		assertThat(reloaded).extracting(i -> i.id).containsExactly(TestConfiguration.lastId.get());
 	}
 
 	private interface PrimitiveIdEntityRepository extends CrudRepository<PrimitiveIdEntity, Long> {}
@@ -147,7 +148,7 @@ static class ImmutableWithManualIdEntity {
 			includeFilters = @ComponentScan.Filter(value = CrudRepository.class, type = FilterType.ASSIGNABLE_TYPE))
 	static class TestConfiguration {
 
-		AtomicLong lastId = new AtomicLong(0);
+		static AtomicLong lastId = new AtomicLong(0);
 
 		@Bean
 		Class<?> testClass() {

spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-hsql.sql

@@ -328,4 +328,10 @@ CREATE TABLE WITH_LOCAL_DATE_TIME
 CREATE TABLE WITH_ID_ONLY
 (
     ID BIGINT GENERATED BY DEFAULT AS IDENTITY (START WITH 1) PRIMARY KEY
-)
+);
+
+CREATE TABLE MANUAL_ID
+(
+    ID         BIGINT PRIMARY KEY,
+    CONTENT     VARCHAR(2000)
+);

spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/DbAction.java

@@ -106,8 +106,8 @@ public String toString() {
 	 */
 	class InsertRoot<T> implements WithGeneratedId<T> {
 
-		private final T entity;
-		private final IdValueSource idValueSource;
+		private T entity;
+		private IdValueSource idValueSource;
 
 		public InsertRoot(T entity, IdValueSource idValueSource) {
 
@@ -127,6 +127,21 @@ public IdValueSource getIdValueSource() {
 		public String toString() {
 			return "InsertRoot{" + "entity=" + entity + ", idValueSource=" + idValueSource + '}';
 		}
+
+		/**
+		 * Updates the entity and {@link IdValueSource} of this action.
+		 * 
+		 * @param newAggregateRoot must not be {@literal null}.
+		 * @param idValueSource must not be {@literal null}.
+		 * @deprecated This method exists only to support a temporary workaround. It will be removed completely without
+		 *             replacement.
+		 */
+		@Deprecated
+		public void updateAction(T newAggregateRoot, IdValueSource idValueSource) {
+
+			this.entity = newAggregateRoot;
+			this.idValueSource = idValueSource;
+		}
 	}
 
 	/**

spring-data-relational/src/main/java/org/springframework/data/relational/core/mapping/event/BeforeConvertCallback.java

@@ -19,7 +19,10 @@
 
 /**
  * An {@link EntityCallback} that gets invoked before the aggregate is converted into a database change. The decision if
- * the change will be an insert or update is made after this callback gets called.
+ * the change will be an insert or update is already made when this callback gets called.
+ * <p>
+ * The main use of this kind of callback is to create application side Ids for aggregates where this isn't done on the
+ * database side.
  *
  * @author Jens Schauder
  * @author Mark Paluch

spring-data-relational/src/main/java/org/springframework/data/relational/core/mapping/event/BeforeSaveCallback.java

@@ -21,6 +21,14 @@
 /**
  * An {@link EntityCallback} that gets invoked before changes are applied to the database, after the aggregate was
  * converted to a database change.
+ * <p>
+ * Changes to the aggregate are not taken into account to decide whether the change will be an insert or update. Use the
+ * {@link BeforeConvertCallback} to change the persistent the entity before being converted.
+ * <p>
+ * Currently a {@link BeforeSaveCallback} may be used to create an Id for new aggregate roots. This is for backward
+ * compatibility purposes within the 2.x development line. The preferred way to do this is to use a
+ * {@link BeforeConvertCallback} or {@link BeforeConvertEvent} and beginning with 3.0. this will be the only correct way
+ * to do this.
  *
  * @author Jens Schauder
  * @author Mark Paluch
@@ -32,9 +40,7 @@ public interface BeforeSaveCallback<T> extends EntityCallback<T> {
 	/**
 	 * Entity callback method invoked before an aggregate root is saved. Can return either the same or a modified instance
 	 * of the aggregate and can modify {@link MutableAggregateChange} contents. This method is called after converting the
-	 * {@code aggregate} to {@link MutableAggregateChange}. Changes to the aggregate are not taken into account to decide
-	 * whether the change will be an insert or update. Use the {@link BeforeConvertCallback} to change the persistent the
-	 * entity before being converted.
+	 * {@code aggregate} to {@link MutableAggregateChange}.
 	 *
 	 * @param aggregate the aggregate.
 	 * @param aggregateChange the associated {@link MutableAggregateChange}.

spring-data-relational/src/main/java/org/springframework/data/relational/core/mapping/event/BeforeSaveEvent.java

@@ -15,10 +15,20 @@
  */
 package org.springframework.data.relational.core.mapping.event;
 
+import org.springframework.context.ApplicationEvent;
 import org.springframework.data.relational.core.conversion.AggregateChange;
 
 /**
- * Gets published before an entity gets saved to the database.
+ * An {@link ApplicationEvent} that gets triggered before changes are applied to the database, after the aggregate was
+ * converted to a database change.
+ * <p>
+ * Changes to the aggregate are not taken into account to decide whether the change will be an insert or update. Use the
+ * {@link BeforeConvertCallback} to change the persistent the entity before being converted.
+ * <p>
+ * Currently a {@link BeforeSaveEvent} may be used to create an Id for new aggregate roots. This is for backward
+ * compatibility purposes within the 2.x development line. The preferred way to do this is to use a
+ * {@link BeforeConvertCallback} or {@link BeforeConvertEvent} and beginning with 3.0. this will be the only correct way
+ * to do this.
  *
  * @author Jens Schauder
  */

src/main/asciidoc/jdbc.adoc

@@ -902,6 +902,8 @@ This is the correct callback if you want to set an id programmatically.
 | {javadoc-base}/org/springframework/data/relational/core/mapping/event/BeforeSaveCallback.html[`BeforeSaveCallback`]
 | Before an aggregate root gets saved (that is, inserted or updated but after the decision about whether if it gets inserted or updated was made).
 
+In the 2.x version of Spring Data JDBC you may use it to create ids for entities. The preferred way an in the future only correct callback to use is `BeforeConvertCallback`.
+
 | {javadoc-base}org/springframework/data/relational/core/mapping/event/AfterSaveCallback.html[`AfterSaveCallback`]
 | After an aggregate root gets saved (that is, inserted or updated).