From 3a89ec04441fc5ad9f004d3fdd5db624f8bbffcc Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 9 May 2019 15:25:37 +0200 Subject: [PATCH] DATAJDBC-370 - Fix read/write of nulled embedded objects. We now allow read and write of Objects annotated with Embedded that are actually null. When writing all contained fields will be nulled. Reading back the entity considers an embedded object to be null itself if all contained properties are null within the backing result. Relates to DATAJDBC-364 Original Pull Request: #151 --- .../jdbc/core/convert/BasicJdbcConverter.java | 34 ++++- .../convert/DefaultDataAccessStrategy.java | 33 +++- .../convert/EntityRowMapperUnitTests.java | 141 +++++++++++++++++- ...dbcRepositoryEmbeddedIntegrationTests.java | 10 +- 4 files changed, 205 insertions(+), 13 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java index 003d10fd..4ae014c3 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java @@ -52,6 +52,7 @@ import org.springframework.util.Assert; * * @author Mark Paluch * @author Jens Schauder + * @author Christoph Strobl * @see MappingContext * @see SimpleTypeHolder * @see CustomConversions @@ -345,15 +346,40 @@ public class BasicJdbcConverter extends BasicRelationalConverter implements Jdbc Object value = getObjectFromResultSet(path.extendBy(property).getColumnAlias()); return readValue(value, property.getTypeInformation()); - } - @SuppressWarnings("unchecked") + @Nullable private Object readEmbeddedEntityFrom(@Nullable Object idValue, RelationalPersistentProperty property) { - ReadingContext newContext = extendBy(property); + ReadingContext ctx = extendBy(property); + return hasInstanceValues(property, ctx) ? ctx.createInstanceInternal(idValue) : null; + } - return newContext.createInstanceInternal(idValue); + private boolean hasInstanceValues(RelationalPersistentProperty property, ReadingContext ctx) { + + RelationalPersistentEntity persistentEntity = getMappingContext() + .getPersistentEntity(property.getTypeInformation()); + + PersistentPropertyPathExtension extension = ctx.path; + + for (RelationalPersistentProperty embeddedProperty : persistentEntity) { + + if (embeddedProperty.isQualified() || embeddedProperty.isReference()) { + return true; + } + + try { + if (ctx.getObjectFromResultSet(extension.extendBy(embeddedProperty).getColumnName()) != null) { + return true; + } + } catch (MappingException e) { + if (ctx.getObjectFromResultSet(extension.extendBy(embeddedProperty).getReverseColumnNameAlias()) != null) { + return true; + } + } + } + + return false; } @Nullable diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/DefaultDataAccessStrategy.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/DefaultDataAccessStrategy.java index 685f5f80..4b66d647 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/DefaultDataAccessStrategy.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/DefaultDataAccessStrategy.java @@ -54,6 +54,7 @@ import org.springframework.util.Assert; * @author Mark Paluch * @author Thomas Lang * @author Bastian Wilhelm + * @author Christoph Strobl * @since 1.1 */ public class DefaultDataAccessStrategy implements DataAccessStrategy { @@ -313,7 +314,8 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy { MapSqlParameterSource parameters = new MapSqlParameterSource(); - PersistentPropertyAccessor propertyAccessor = persistentEntity.getPropertyAccessor(instance); + PersistentPropertyAccessor propertyAccessor = instance != null ? persistentEntity.getPropertyAccessor(instance) + : NoValuePropertyAccessor.instance(); persistentEntity.doWithProperties((PropertyHandler) property -> { @@ -480,4 +482,33 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy { return it -> false; } } + + /** + * A {@link PersistentPropertyAccessor} implementation always returning null + * + * @param + */ + static class NoValuePropertyAccessor implements PersistentPropertyAccessor { + + private static final NoValuePropertyAccessor INSTANCE = new NoValuePropertyAccessor(); + + static NoValuePropertyAccessor instance() { + return INSTANCE; + } + + @Override + public void setProperty(PersistentProperty property, Object value) { + throw new UnsupportedOperationException("Cannot set value on 'null' target object."); + } + + @Override + public Object getProperty(PersistentProperty property) { + return null; + } + + @Override + public T getBean() { + return null; + } + } } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/EntityRowMapperUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/EntityRowMapperUnitTests.java index 0e6447a4..2004a335 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/EntityRowMapperUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/EntityRowMapperUnitTests.java @@ -18,11 +18,15 @@ package org.springframework.data.jdbc.core.convert; import static java.util.Arrays.*; import static java.util.Collections.*; import static org.assertj.core.api.Assertions.*; -import static org.mockito.ArgumentMatchers.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; import lombok.AllArgsConstructor; +import lombok.EqualsAndHashCode; +import lombok.NoArgsConstructor; import lombok.RequiredArgsConstructor; +import lombok.SneakyThrows; import lombok.Value; import lombok.experimental.Wither; @@ -301,6 +305,101 @@ public class EntityRowMapperUnitTests { .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, new ImmutableValue("ru'Ha'")); } + @Test // DATAJDBC-370 + @SneakyThrows + public void simplePrimitiveImmutableEmbeddedGetsProperlyExtracted() { + + ResultSet rs = mockResultSet(asList("id", "value"), // + ID_FOR_ENTITY_NOT_REFERENCING_MAP, 24); + rs.next(); + + WithPrimitiveImmutableValue extracted = createRowMapper(WithPrimitiveImmutableValue.class).mapRow(rs, 1); + + assertThat(extracted) // + .isNotNull() // + .extracting(e -> e.id, e -> e.embeddedImmutablePrimitiveValue) // + .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, new ImmutablePrimitiveValue(24)); + } + + @Test // DATAJDBC-370 + public void simpleImmutableEmbeddedShouldBeNullIfAllOfTheEmbeddableAreNull() throws SQLException { + + ResultSet rs = mockResultSet(asList("id", "value"), // + ID_FOR_ENTITY_NOT_REFERENCING_MAP, null); + rs.next(); + + WithImmutableValue extracted = createRowMapper(WithImmutableValue.class).mapRow(rs, 1); + + assertThat(extracted) // + .isNotNull() // + .extracting(e -> e.id, e -> e.embeddedImmutableValue) // + .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, null); + } + + @Test // DATAJDBC-370 + @SneakyThrows + public void embeddedShouldBeNullWhenFieldsAreNull() { + + ResultSet rs = mockResultSet(asList("id", "name", "prefix_id", "prefix_name"), // + ID_FOR_ENTITY_NOT_REFERENCING_MAP, "alpha", null, null); + rs.next(); + + EmbeddedEntity extracted = createRowMapper(EmbeddedEntity.class).mapRow(rs, 1); + + assertThat(extracted) // + .isNotNull() // + .extracting(e -> e.id, e -> e.name, e -> e.children) // + .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, "alpha", null); + } + + @Test // DATAJDBC-370 + @SneakyThrows + public void embeddedShouldNotBeNullWhenAtLeastOneFieldIsNotNull() { + + ResultSet rs = mockResultSet(asList("id", "name", "prefix_id", "prefix_name"), // + ID_FOR_ENTITY_NOT_REFERENCING_MAP, "alpha", 24, null); + rs.next(); + + EmbeddedEntity extracted = createRowMapper(EmbeddedEntity.class).mapRow(rs, 1); + + assertThat(extracted) // + .isNotNull() // + .extracting(e -> e.id, e -> e.name, e -> e.children) // + .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, "alpha", new Trivial(24L, null)); + } + + @Test // DATAJDBC-370 + @SneakyThrows + public void primitiveEmbeddedShouldBeNullWhenNoValuePresent() { + + ResultSet rs = mockResultSet(asList("id", "value"), // + ID_FOR_ENTITY_NOT_REFERENCING_MAP, null); + rs.next(); + + WithPrimitiveImmutableValue extracted = createRowMapper(WithPrimitiveImmutableValue.class).mapRow(rs, 1); + + assertThat(extracted) // + .isNotNull() // + .extracting(e -> e.id, e -> e.embeddedImmutablePrimitiveValue) // + .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, null); + } + + @Test // DATAJDBC-370 + @SneakyThrows + public void deepNestedEmbeddable() { + + ResultSet rs = mockResultSet(asList("id", "level0", "level1_value", "level1_level2_value"), // + ID_FOR_ENTITY_NOT_REFERENCING_MAP, "0", "1", "2"); + rs.next(); + + WithDeepNestedEmbeddable extracted = createRowMapper(WithDeepNestedEmbeddable.class).mapRow(rs, 1); + + assertThat(extracted) // + .isNotNull() // + .extracting(e -> e.id, e -> extracted.level0, e -> e.level1.value, e -> e.level1.level2.value) // + .containsExactly(ID_FOR_ENTITY_NOT_REFERENCING_MAP, "0", "1", "2"); + } + // Model classes to be used in tests @Wither @@ -311,6 +410,9 @@ public class EntityRowMapperUnitTests { private final String name; } + @EqualsAndHashCode + @NoArgsConstructor + @AllArgsConstructor static class Trivial { @Id Long id; @@ -432,11 +534,35 @@ public class EntityRowMapperUnitTests { @Embedded ImmutableValue embeddedImmutableValue; } + static class WithPrimitiveImmutableValue { + + @Id Long id; + @Embedded ImmutablePrimitiveValue embeddedImmutablePrimitiveValue; + } + @Value static class ImmutableValue { Object value; } + @Value + static class ImmutablePrimitiveValue { + int value; + } + + static class WithDeepNestedEmbeddable { + + @Id Long id; + String level0; + @Embedded("level1_") EmbeddedWithEmbedded level1; + } + + static class EmbeddedWithEmbedded { + + Object value; + @Embedded("level2_") ImmutableValue level2; + } + // Infrastructure for assertions and constructing mocks private FixtureBuilder buildFixture() { @@ -454,22 +580,23 @@ public class EntityRowMapperUnitTests { DataAccessStrategy accessStrategy = mock(DataAccessStrategy.class); // the ID of the entity is used to determine what kind of ResultSet is needed for subsequent selects. - doReturn(new HashSet<>(asList(new Trivial(), new Trivial()))).when(accessStrategy) + doReturn(new HashSet<>(asList(new Trivial(1L, "one"), new Trivial(2L, "two")))).when(accessStrategy) .findAllByProperty(eq(ID_FOR_ENTITY_NOT_REFERENCING_MAP), any(RelationalPersistentProperty.class)); doReturn(new HashSet<>(asList( // - new SimpleEntry<>("one", new Trivial()), // - new SimpleEntry<>("two", new Trivial()) // + new SimpleEntry<>("one", new Trivial(1L, "one")), // + new SimpleEntry<>("two", new Trivial(2L, "two")) // ))).when(accessStrategy).findAllByProperty(eq(ID_FOR_ENTITY_REFERENCING_MAP), any(RelationalPersistentProperty.class)); doReturn(new HashSet<>(asList( // - new SimpleEntry<>(1, new Trivial()), // - new SimpleEntry<>(2, new Trivial()) // + new SimpleEntry<>(1, new Trivial(1L, "one")), // + new SimpleEntry<>(2, new Trivial(2L, "tow")) // ))).when(accessStrategy).findAllByProperty(eq(ID_FOR_ENTITY_REFERENCING_LIST), any(RelationalPersistentProperty.class)); - JdbcConverter converter = new BasicJdbcConverter(context, new JdbcCustomConversions()); + JdbcConverter converter = new BasicJdbcConverter(context, new JdbcCustomConversions(), + JdbcTypeFactory.unsupported()); return new EntityRowMapper<>( // (RelationalPersistentEntity) context.getRequiredPersistentEntity(type), // diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryEmbeddedIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryEmbeddedIntegrationTests.java index 5d0ac760..924c7289 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryEmbeddedIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryEmbeddedIntegrationTests.java @@ -44,6 +44,7 @@ import org.springframework.transaction.annotation.Transactional; * Very simple use cases for creation and usage of JdbcRepositories with test {@link Embedded} annotation in Entities. * * @author Bastian Wilhelm + * @author Christoph Strobl */ @ContextConfiguration @Transactional @@ -209,6 +210,14 @@ public class JdbcRepositoryEmbeddedIntegrationTests { assertThat(repository.findAll()).isEmpty(); } + @Test // DATAJDBC-370 + public void saveWithNullValueEmbeddable() { + + DummyEntity entity = repository.save(new DummyEntity()); + + assertThat(JdbcTestUtils.countRowsInTableWhere((JdbcTemplate) template.getJdbcOperations(), "dummy_entity", + "id = " + entity.getId())).isEqualTo(1); + } private static DummyEntity createDummyEntity() { DummyEntity entity = new DummyEntity(); @@ -222,7 +231,6 @@ public class JdbcRepositoryEmbeddedIntegrationTests { entity.setPrefixedEmbeddable(prefixedCascadedEmbeddable); - final CascadedEmbeddable cascadedEmbeddable = new CascadedEmbeddable(); cascadedEmbeddable.setTest("c2");