From f7274a2efa0800b2e823310ace358c9f23fd63e6 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Mon, 12 Mar 2018 09:54:32 +0100 Subject: [PATCH] DATAJDBC-186 - Removed requirement for id properties. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../jdbc/core/DefaultDataAccessStrategy.java | 20 ++++++++++--------- .../data/jdbc/core/EntityRowMapper.java | 6 ++++-- .../data/jdbc/core/SqlGenerator.java | 9 +++++---- .../DefaultDataAccessStrategyUnitTests.java | 16 +++++++++------ 4 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java b/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java index 3297d9d4..c16b7eeb 100644 --- a/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java +++ b/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(); + + if (idValue != null) { + additionalParameters.put(idProperty.getColumnName(), convert(idValue, idProperty.getColumnType())); + } additionalParameters.forEach(parameterSource::addValue); - boolean idValueDoesNotComeFromEntity = // - idValue == null // - || additionalParameters.containsKey(idProperty.getColumnName()); - 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 Iterable 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()); diff --git a/src/main/java/org/springframework/data/jdbc/core/EntityRowMapper.java b/src/main/java/org/springframework/data/jdbc/core/EntityRowMapper.java index 78b80852..89dae3e2 100644 --- a/src/main/java/org/springframework/data/jdbc/core/EntityRowMapper.java +++ b/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 implements RowMapper { this.context = context; this.accessStrategy = accessStrategy; - idProperty = entity.getRequiredIdProperty(); + idProperty = entity.getIdProperty(); } /* @@ -77,7 +79,7 @@ public class EntityRowMapper implements RowMapper { 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) { diff --git a/src/main/java/org/springframework/data/jdbc/core/SqlGenerator.java b/src/main/java/org/springframework/data/jdbc/core/SqlGenerator.java index 369fc9c1..282269bd 100644 --- a/src/main/java/org/springframework/data/jdbc/core/SqlGenerator.java +++ b/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 additionalColumns) { - return createInsertSql(excludeId, additionalColumns); + String getInsert(Set 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 additionalColumns) { + private String createInsertSql(Set additionalColumns) { String insertTemplate = "insert into %s (%s) values (%s)"; - List columnNamesForInsert = new ArrayList<>(excludeId ? nonIdColumnNames : columnNames); + LinkedHashSet columnNamesForInsert = new LinkedHashSet<>(nonIdColumnNames); columnNamesForInsert.addAll(additionalColumns); String tableColumns = String.join(", ", columnNamesForInsert); diff --git a/src/test/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategyUnitTests.java b/src/test/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategyUnitTests.java index 6ffbe140..6c3ae9f2 100644 --- a/src/test/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategyUnitTests.java +++ b/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 additionalParameters = new HashMap<>(); - ArgumentCaptor captor = ArgumentCaptor.forClass(SqlParameterSource.class); + ArgumentCaptor 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 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