DATAJDBC-186 - Removed requirement for id properties.

It is now ok for an entity to be “new” after saving.
We now only check that an entity is not new when the id is provided by the database.
If in such a case the entity is still “new” after saving it basically means obtaining the id from JDBC and setting it in the entity failed.

Removed need for entities in maps to have an id.

Together these changes simplify the requirements for Map values, which now can be value objects.

src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java

@@ -89,25 +89,25 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 		MapSqlParameterSource parameterSource = getPropertyMap(instance, persistentEntity);
 
 		Object idValue = getIdValueOrNull(instance, persistentEntity);
-		JdbcPersistentProperty idProperty = persistentEntity.getRequiredIdProperty();
-		parameterSource.addValue(idProperty.getColumnName(), convert(idValue, idProperty.getColumnType()),
-				JdbcUtil.sqlTypeFor(idProperty.getColumnType()));
+		JdbcPersistentProperty idProperty = persistentEntity.getIdProperty();
 
-		additionalParameters.forEach(parameterSource::addValue);
+		if (idValue != null) {
+			additionalParameters.put(idProperty.getColumnName(), convert(idValue, idProperty.getColumnType()));
+		}
 
-		boolean idValueDoesNotComeFromEntity = //
-				idValue == null //
-						|| additionalParameters.containsKey(idProperty.getColumnName());
+		additionalParameters.forEach(parameterSource::addValue);
 
 		operations.update( //
-				sql(domainType).getInsert(idValueDoesNotComeFromEntity, additionalParameters.keySet()), //
+				sql(domainType).getInsert(additionalParameters.keySet()), //
 				parameterSource, //
 				holder //
 		);
 
 		setIdFromJdbc(instance, holder, persistentEntity);
 
-		if (entityInformation.isNew(instance)) {
+		// if there is an id property and it was null before the save
+		// The database should have created an id and provided it.
+		if (idProperty != null && idValue == null && entityInformation.isNew(instance)) {
 			throw new IllegalStateException(String.format(ENTITY_NEW_AFTER_INSERT, persistentEntity));
 		}
 
@@ -198,6 +198,8 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	@Override
 	public <T> Iterable<T> findAllByProperty(Object rootId, JdbcPersistentProperty property) {
 
+		Assert.notNull(rootId, "rootId must not be null.");
+
 		Class<?> actualType = property.getActualType();
 		String findAllByProperty = sql(actualType).getFindAllByProperty(property.getReverseColumnName(),
 				property.getKeyColumn(), property.isOrdered());

src/main/java/org/springframework/data/jdbc/core/EntityRowMapper.java

@@ -35,6 +35,8 @@ import org.springframework.data.mapping.PreferredConstructor.Parameter;
 import org.springframework.data.mapping.model.ConvertingPropertyAccessor;
 import org.springframework.data.mapping.model.ParameterValueProvider;
 import org.springframework.jdbc.core.RowMapper;
+import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 
 /**
  * Maps a ResultSet to an entity of type {@code T}, including entities referenced.
@@ -62,7 +64,7 @@ public class EntityRowMapper<T> implements RowMapper<T> {
 		this.context = context;
 		this.accessStrategy = accessStrategy;
 
-		idProperty = entity.getRequiredIdProperty();
+		idProperty = entity.getIdProperty();
 	}
 
 	/*
@@ -77,7 +79,7 @@ public class EntityRowMapper<T> implements RowMapper<T> {
 		ConvertingPropertyAccessor propertyAccessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(result),
 				conversions);
 
-		Object id = readFrom(resultSet, idProperty, "");
+		Object id = idProperty == null ? null : readFrom(resultSet, idProperty, "");
 
 		for (JdbcPersistentProperty property : entity) {
 

src/main/java/org/springframework/data/jdbc/core/SqlGenerator.java

@@ -27,6 +27,7 @@ import org.springframework.util.Assert;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -121,8 +122,8 @@ class SqlGenerator {
 		return findOneSql.get();
 	}
 
-	String getInsert(boolean excludeId, Set<String> additionalColumns) {
-		return createInsertSql(excludeId, additionalColumns);
+	String getInsert(Set<String> additionalColumns) {
+		return createInsertSql(additionalColumns);
 	}
 
 	String getUpdate() {
@@ -237,11 +238,11 @@ class SqlGenerator {
 		return String.format("select count(*) from %s", entity.getTableName());
 	}
 
-	private String createInsertSql(boolean excludeId, Set<String> additionalColumns) {
+	private String createInsertSql(Set<String> additionalColumns) {
 
 		String insertTemplate = "insert into %s (%s) values (%s)";
 
-		List<String> columnNamesForInsert = new ArrayList<>(excludeId ? nonIdColumnNames : columnNames);
+		LinkedHashSet<String> columnNamesForInsert = new LinkedHashSet<>(nonIdColumnNames);
 		columnNamesForInsert.addAll(additionalColumns);
 
 		String tableColumns = String.join(", ", columnNamesForInsert);

src/test/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategyUnitTests.java

@@ -42,7 +42,7 @@ public class DefaultDataAccessStrategyUnitTests {
 	NamedParameterJdbcOperations jdbcOperations = mock(NamedParameterJdbcOperations.class);
 	JdbcMappingContext context = new JdbcMappingContext(new DefaultNamingStrategy(), jdbcOperations, __ -> {});
 	HashMap<String, Object> additionalParameters = new HashMap<>();
-	ArgumentCaptor<SqlParameterSource> captor = ArgumentCaptor.forClass(SqlParameterSource.class);
+	ArgumentCaptor<SqlParameterSource> paramSourceCaptor = ArgumentCaptor.forClass(SqlParameterSource.class);
 
 	DefaultDataAccessStrategy accessStrategy = new DefaultDataAccessStrategy( //
 			new SqlGeneratorSource(context), //
@@ -57,21 +57,25 @@ public class DefaultDataAccessStrategyUnitTests {
 
 		accessStrategy.insert(new DummyEntity(ORIGINAL_ID), DummyEntity.class, additionalParameters);
 
-		verify(jdbcOperations).update(eq("insert into DummyEntity (id) values (:id)"), captor.capture(),
+		verify(jdbcOperations).update(eq("insert into DummyEntity (id) values (:id)"), paramSourceCaptor.capture(),
 				any(KeyHolder.class));
-		assertThat(captor.getValue().getValue("id")).isEqualTo(ID_FROM_ADDITIONAL_VALUES);
 	}
 
 	@Test // DATAJDBC-146
 	public void additionalParametersGetAddedToStatement() {
 
+		ArgumentCaptor<String> sqlCaptor = ArgumentCaptor.forClass(String.class);
+
 		additionalParameters.put("reference", ID_FROM_ADDITIONAL_VALUES);
 
 		accessStrategy.insert(new DummyEntity(ORIGINAL_ID), DummyEntity.class, additionalParameters);
 
-		verify(jdbcOperations).update(eq("insert into DummyEntity (id, reference) values (:id, :reference)"),
-				captor.capture(), any(KeyHolder.class));
-		assertThat(captor.getValue().getValue("id")).isEqualTo(ORIGINAL_ID);
+		verify(jdbcOperations).update(sqlCaptor.capture(), paramSourceCaptor.capture(), any(KeyHolder.class));
+
+		assertThat(sqlCaptor.getValue()) //
+				.containsSequence("insert into DummyEntity (", "id", ") values (", ":id", ")") //
+				.containsSequence("insert into DummyEntity (", "reference", ") values (", ":reference", ")");
+		assertThat(paramSourceCaptor.getValue().getValue("id")).isEqualTo(ORIGINAL_ID);
 	}
 
 	@RequiredArgsConstructor