From 51d20f2c0687655eda2f61304ab8f6c346e884db Mon Sep 17 00:00:00 2001
From: Jens Schauder <jens.schauder@broadcom.com>
Date: Tue, 13 Aug 2024 10:56:18 +0200
Subject: [PATCH] Dedicated exception for aggregate roots without id property.

We now distinguish between an id not set during insert and a supposed aggregate root without id property.

Closes #1502
Original pull request #1855
---
 .../data/jdbc/core/JdbcAggregateTemplate.java |  8 ++++
 .../core/JdbcAggregateTemplateUnitTests.java  | 39 +++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
index efa04a8d8b2..ad690dc2738 100644
--- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
+++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
@@ -165,6 +165,8 @@ public <T> T save(T instance) {
 
 		Assert.notNull(instance, "Aggregate instance must not be null");
 
+		verifyIdProperty(instance);
+
 		return performSave(new EntityAndChangeCreator<>(instance, changeCreatorSelectorForSave(instance)));
 	}
 
@@ -179,6 +181,7 @@ public <T> Iterable<T> saveAll(Iterable<T> instances) {
 
 		List<EntityAndChangeCreator<T>> entityAndChangeCreators = new ArrayList<>();
 		for (T instance : instances) {
+			verifyIdProperty(instance);
 			entityAndChangeCreators.add(new EntityAndChangeCreator<>(instance, changeCreatorSelectorForSave(instance)));
 		}
 		return performSaveAll(entityAndChangeCreators);
@@ -425,6 +428,11 @@ public <T> void deleteAll(Iterable<? extends T> instances) {
 		}
 	}
 
+	private <T> void verifyIdProperty(T instance) {
+		// accessing the id property just to raise an exception in the case it does not exist.
+		context.getRequiredPersistentEntity(instance.getClass()).getRequiredIdProperty();
+	}
+
 	private <T> void doDeleteAll(Iterable<? extends T> instances, Class<T> domainType) {
 
 		BatchingAggregateChange<T, DeleteAggregateChange<T>> batchingAggregateChange = BatchingAggregateChange
diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateUnitTests.java
index 39090350e9d..97ccb746cdf 100644
--- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateUnitTests.java
+++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateUnitTests.java
@@ -32,10 +32,12 @@
 import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Sort;
 import org.springframework.data.jdbc.core.convert.DataAccessStrategy;
+import org.springframework.data.jdbc.core.convert.Identifier;
 import org.springframework.data.jdbc.core.convert.JdbcConverter;
 import org.springframework.data.jdbc.core.convert.MappingJdbcConverter;
 import org.springframework.data.jdbc.core.convert.RelationResolver;
 import org.springframework.data.mapping.callback.EntityCallbacks;
+import org.springframework.data.relational.core.conversion.IdValueSource;
 import org.springframework.data.relational.core.conversion.MutableAggregateChange;
 import org.springframework.data.relational.core.mapping.Column;
 import org.springframework.data.relational.core.mapping.RelationalMappingContext;
@@ -46,6 +48,8 @@
 import org.springframework.data.relational.core.mapping.event.BeforeDeleteCallback;
 import org.springframework.data.relational.core.mapping.event.BeforeSaveCallback;
 
+import java.util.List;
+
 /**
  * Unit tests for {@link JdbcAggregateTemplate}.
  *
@@ -333,6 +337,38 @@ void deleteAllByIdWithEmptyListDoesNothing() {
 		template.deleteAllById(emptyList(), SampleEntity.class);
 	}
 
+	@Test // GH-1502
+	void saveThrowsExceptionWhenIdIsNotSet() {
+
+		SampleEntity alfred = new SampleEntity(null, "Alfred");
+		when(callbacks.callback(any(), any(), any(Object[].class))).thenReturn(alfred);
+
+		when(dataAccessStrategy.insert(eq(alfred), any(Class.class), any(Identifier.class), any(IdValueSource.class)))
+				.thenReturn(null);
+
+		assertThatIllegalArgumentException().isThrownBy(() -> template.save(alfred))
+				.withMessage("After saving the identifier must not be null");
+	}
+
+	@Test // GH-1502
+	void saveThrowsExceptionWhenIdDoesNotExist() {
+
+		NoIdEntity alfred = new NoIdEntity("Alfred");
+
+		assertThatIllegalStateException().isThrownBy(() -> template.save(alfred))
+				.withMessage("Required identifier property not found for class %s".formatted(NoIdEntity.class.getName()));
+	}
+
+	@Test // GH-1502
+	void saveThrowsExceptionWhenIdDoesNotExistOnSaveAll() {
+
+		NoIdEntity alfred = new NoIdEntity("Alfred");
+		NoIdEntity berta = new NoIdEntity("Berta");
+
+		assertThatIllegalStateException().isThrownBy(() -> template.saveAll(	List.of(alfred, berta)))
+				.withMessage("Required identifier property not found for class %s".formatted(NoIdEntity.class.getName()));
+	}
+
 	private static class SampleEntity {
 
 		@Column("id1")
@@ -451,4 +487,7 @@ public long getVersion() {
 			return this.version;
 		}
 	}
+
+	record NoIdEntity(String name) {
+	}
 }