From 9903101c8eb56aa840d9d6e2fcc6e8dde08a8d7a Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Sat, 25 May 2019 09:50:32 +0300 Subject: [PATCH 1/3] DATAJDBC-378 - Prepare branch --- pom.xml | 2 +- spring-data-jdbc-distribution/pom.xml | 2 +- spring-data-jdbc/pom.xml | 4 ++-- spring-data-relational/pom.xml | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index a529f1001e..6a905e8617 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-relational-parent - 1.1.0.BUILD-SNAPSHOT + 1.1.0.DATAJDBC-378-SNAPSHOT pom Spring Data Relational Parent diff --git a/spring-data-jdbc-distribution/pom.xml b/spring-data-jdbc-distribution/pom.xml index f6d4373844..83e8a30c0e 100644 --- a/spring-data-jdbc-distribution/pom.xml +++ b/spring-data-jdbc-distribution/pom.xml @@ -14,7 +14,7 @@ org.springframework.data spring-data-relational-parent - 1.1.0.BUILD-SNAPSHOT + 1.1.0.DATAJDBC-378-SNAPSHOT ../pom.xml diff --git a/spring-data-jdbc/pom.xml b/spring-data-jdbc/pom.xml index d33566bc88..2a48c1b136 100644 --- a/spring-data-jdbc/pom.xml +++ b/spring-data-jdbc/pom.xml @@ -5,7 +5,7 @@ 4.0.0 spring-data-jdbc - 1.1.0.BUILD-SNAPSHOT + 1.1.0.DATAJDBC-378-SNAPSHOT Spring Data JDBC Spring Data module for JDBC repositories. @@ -14,7 +14,7 @@ org.springframework.data spring-data-relational-parent - 1.1.0.BUILD-SNAPSHOT + 1.1.0.DATAJDBC-378-SNAPSHOT diff --git a/spring-data-relational/pom.xml b/spring-data-relational/pom.xml index fa7f2f4873..0294e78b07 100644 --- a/spring-data-relational/pom.xml +++ b/spring-data-relational/pom.xml @@ -5,7 +5,7 @@ 4.0.0 spring-data-relational - 1.1.0.BUILD-SNAPSHOT + 1.1.0.DATAJDBC-378-SNAPSHOT Spring Data Relational Spring Data Relational support @@ -13,7 +13,7 @@ org.springframework.data spring-data-relational-parent - 1.1.0.BUILD-SNAPSHOT + 1.1.0.DATAJDBC-378-SNAPSHOT From caedb1fd4c4cb01374d39bfc25d09125139ee13f Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Sat, 25 May 2019 10:25:07 +0300 Subject: [PATCH 2/3] DATAJDBC-378 - Proper handling of null and empty collections in JdbcAggregateTemplate. --- .../data/jdbc/core/JdbcAggregateTemplate.java | 24 +++++++++++++++++++ .../convert/DefaultDataAccessStrategy.java | 5 ++++ ...JdbcAggregateTemplateIntegrationTests.java | 19 ++++++++++++++- 3 files changed, 47 insertions(+), 1 deletion(-) 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 c4b9b3bf32..cdba2ed382 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 @@ -152,6 +152,9 @@ public T update(T instance) { */ @Override public long count(Class domainType) { + + Assert.notNull(domainType, "Domain type must not be null"); + return accessStrategy.count(domainType); } @@ -162,6 +165,9 @@ public long count(Class domainType) { @Override public T findById(Object id, Class domainType) { + Assert.notNull(id, "Id must not be null"); + Assert.notNull(domainType, "Domain type must not be null"); + T entity = accessStrategy.findById(id, domainType); if (entity != null) { publishAfterLoad(id, entity); @@ -175,6 +181,10 @@ public T findById(Object id, Class domainType) { */ @Override public boolean existsById(Object id, Class domainType) { + + Assert.notNull(id, "Id must not be null"); + Assert.notNull(domainType, "Domain type must not be null"); + return accessStrategy.existsById(id, domainType); } @@ -185,6 +195,8 @@ public boolean existsById(Object id, Class domainType) { @Override public Iterable findAll(Class domainType) { + Assert.notNull(domainType, "Domain type must not be null"); + Iterable all = accessStrategy.findAll(domainType); publishAfterLoad(all); return all; @@ -197,6 +209,9 @@ public Iterable findAll(Class domainType) { @Override public Iterable findAllById(Iterable ids, Class domainType) { + Assert.notNull(ids, "Ids must not be null"); + Assert.notNull(domainType, "Domain type must not be null"); + Iterable allById = accessStrategy.findAllById(ids, domainType); publishAfterLoad(allById); return allById; @@ -209,6 +224,9 @@ public Iterable findAllById(Iterable ids, Class domainType) { @Override public void delete(S aggregateRoot, Class domainType) { + Assert.notNull(aggregateRoot, "Aggregate root must not be null"); + Assert.notNull(domainType, "Domain type must not be null"); + IdentifierAccessor identifierAccessor = context.getRequiredPersistentEntity(domainType) .getIdentifierAccessor(aggregateRoot); @@ -221,6 +239,10 @@ public void delete(S aggregateRoot, Class domainType) { */ @Override public void deleteById(Object id, Class domainType) { + + Assert.notNull(id, "Id must not be null"); + Assert.notNull(domainType, "Domain type must not be null"); + deleteTree(id, null, domainType); } @@ -231,6 +253,8 @@ public void deleteById(Object id, Class domainType) { @Override public void deleteAll(Class domainType) { + Assert.notNull(domainType, "Domain type must not be null"); + AggregateChange change = createDeletingChange(domainType); change.executeWith(interpreter, context, converter); } 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 6c7f2bdeb4..ceb4fcfaaa 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 @@ -18,6 +18,7 @@ import java.sql.JDBCType; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -241,6 +242,10 @@ public Iterable findAll(Class domainType) { @SuppressWarnings("unchecked") public Iterable findAllById(Iterable ids, Class domainType) { + if (!ids.iterator().hasNext()) { + return Collections.emptyList(); + } + RelationalPersistentProperty idProperty = getRequiredPersistentEntity(domainType).getRequiredIdProperty(); MapSqlParameterSource parameterSource = new MapSqlParameterSource(); diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java index 8c35785fee..332794d7d2 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java @@ -35,7 +35,6 @@ import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; - import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.annotation.Bean; @@ -597,6 +596,24 @@ public void shouldDeleteChainOfMapsWithoutIds() { }); } + @Test // DATAJDBC-378 + public void findAllByIdMustNotAcceptNullArgumentForType() { + + assertThatThrownBy(() -> template.findAllById(singleton(23L), null)).isInstanceOf(IllegalArgumentException.class); + } + + @Test // DATAJDBC-378 + public void findAllByIdMustNotAcceptNullArgumentForIds() { + + assertThatThrownBy(() -> template.findAllById(null, LegoSet.class)).isInstanceOf(IllegalArgumentException.class); + } + + @Test // DATAJDBC-378 + public void findAllByIdWithEmpthListMustReturnEmptyResult() { + + assertThat(template.findAllById(emptyList(), LegoSet.class)).isEmpty(); + } + private static NoIdMapChain4 createNoIdMapTree() { NoIdMapChain4 chain4 = new NoIdMapChain4(); From c70c6dbd13f70cbcc97402bf4c644c2c45ef9641 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 13 Jun 2019 09:46:29 +0200 Subject: [PATCH 3/3] DATAJDBC-378 - Polishing Move non integration tests to separate class. --- .../data/jdbc/core/JdbcAggregateTemplate.java | 27 +++---- ...JdbcAggregateTemplateIntegrationTests.java | 18 ----- .../core/JdbcAggregateTemplateUnitTests.java | 78 +++++++++++++++++++ 3 files changed, 92 insertions(+), 31 deletions(-) create mode 100644 spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateUnitTests.java 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 cdba2ed382..e409c06ba6 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 @@ -46,6 +46,7 @@ * @author Jens Schauder * @author Mark Paluch * @author Thomas Lang + * @author Christoph Strobl */ public class JdbcAggregateTemplate implements JdbcAggregateOperations { @@ -165,8 +166,8 @@ public long count(Class domainType) { @Override public T findById(Object id, Class domainType) { - Assert.notNull(id, "Id must not be null"); - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(id, "Id must not be null!"); + Assert.notNull(domainType, "Domain type must not be null!"); T entity = accessStrategy.findById(id, domainType); if (entity != null) { @@ -182,8 +183,8 @@ public T findById(Object id, Class domainType) { @Override public boolean existsById(Object id, Class domainType) { - Assert.notNull(id, "Id must not be null"); - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(id, "Id must not be null!"); + Assert.notNull(domainType, "Domain type must not be null!"); return accessStrategy.existsById(id, domainType); } @@ -195,7 +196,7 @@ public boolean existsById(Object id, Class domainType) { @Override public Iterable findAll(Class domainType) { - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(domainType, "Domain type must not be null!"); Iterable all = accessStrategy.findAll(domainType); publishAfterLoad(all); @@ -209,8 +210,8 @@ public Iterable findAll(Class domainType) { @Override public Iterable findAllById(Iterable ids, Class domainType) { - Assert.notNull(ids, "Ids must not be null"); - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(ids, "Ids must not be null!"); + Assert.notNull(domainType, "Domain type must not be null!"); Iterable allById = accessStrategy.findAllById(ids, domainType); publishAfterLoad(allById); @@ -224,8 +225,8 @@ public Iterable findAllById(Iterable ids, Class domainType) { @Override public void delete(S aggregateRoot, Class domainType) { - Assert.notNull(aggregateRoot, "Aggregate root must not be null"); - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(aggregateRoot, "Aggregate root must not be null!"); + Assert.notNull(domainType, "Domain type must not be null!"); IdentifierAccessor identifierAccessor = context.getRequiredPersistentEntity(domainType) .getIdentifierAccessor(aggregateRoot); @@ -240,8 +241,8 @@ public void delete(S aggregateRoot, Class domainType) { @Override public void deleteById(Object id, Class domainType) { - Assert.notNull(id, "Id must not be null"); - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(id, "Id must not be null!"); + Assert.notNull(domainType, "Domain type must not be null!"); deleteTree(id, null, domainType); } @@ -253,7 +254,7 @@ public void deleteById(Object id, Class domainType) { @Override public void deleteAll(Class domainType) { - Assert.notNull(domainType, "Domain type must not be null"); + Assert.notNull(domainType, "Domain type must not be null!"); AggregateChange change = createDeletingChange(domainType); change.executeWith(interpreter, context, converter); @@ -274,7 +275,7 @@ private T store(T instance, IdentifierAccessor identifierAccessor, Aggregate Object identifier = persistentEntity.getIdentifierAccessor(change.getEntity()).getIdentifier(); - Assert.notNull(identifier, "After saving the identifier must not be null"); + Assert.notNull(identifier, "After saving the identifier must not be null!"); publisher.publishEvent(new AfterSaveEvent( // Identifier.of(identifier), // diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java index 332794d7d2..15093f3fa3 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java @@ -596,24 +596,6 @@ public void shouldDeleteChainOfMapsWithoutIds() { }); } - @Test // DATAJDBC-378 - public void findAllByIdMustNotAcceptNullArgumentForType() { - - assertThatThrownBy(() -> template.findAllById(singleton(23L), null)).isInstanceOf(IllegalArgumentException.class); - } - - @Test // DATAJDBC-378 - public void findAllByIdMustNotAcceptNullArgumentForIds() { - - assertThatThrownBy(() -> template.findAllById(null, LegoSet.class)).isInstanceOf(IllegalArgumentException.class); - } - - @Test // DATAJDBC-378 - public void findAllByIdWithEmpthListMustReturnEmptyResult() { - - assertThat(template.findAllById(emptyList(), LegoSet.class)).isEmpty(); - } - private static NoIdMapChain4 createNoIdMapTree() { NoIdMapChain4 chain4 = new NoIdMapChain4(); 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 new file mode 100644 index 0000000000..4d0b32c1c1 --- /dev/null +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateUnitTests.java @@ -0,0 +1,78 @@ +/* + * Copyright 2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jdbc.core; + +import static java.util.Collections.*; +import static org.assertj.core.api.Assertions.*; + +import javax.sql.DataSource; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.data.jdbc.core.JdbcAggregateTemplateIntegrationTests.LegoSet; +import org.springframework.data.jdbc.core.convert.BasicJdbcConverter; +import org.springframework.data.jdbc.core.convert.DataAccessStrategy; +import org.springframework.data.jdbc.core.convert.DefaultDataAccessStrategy; +import org.springframework.data.jdbc.core.convert.JdbcConverter; +import org.springframework.data.jdbc.core.convert.RelationResolver; +import org.springframework.data.jdbc.core.convert.SqlGeneratorSource; +import org.springframework.data.relational.core.mapping.NamingStrategy; +import org.springframework.data.relational.core.mapping.RelationalMappingContext; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; + +/** + * @author Christoph Strobl + */ +@RunWith(MockitoJUnitRunner.class) +public class JdbcAggregateTemplateUnitTests { + + JdbcAggregateOperations template; + + @Mock ApplicationEventPublisher eventPublisher; + @Mock RelationResolver relationResolver; + @Mock DataSource dataSource; + + @Before + public void setUp() { + + RelationalMappingContext mappingContext = new RelationalMappingContext(NamingStrategy.INSTANCE); + JdbcConverter converter = new BasicJdbcConverter(mappingContext, relationResolver); + NamedParameterJdbcOperations namedParameterJdbcOperations = new NamedParameterJdbcTemplate(dataSource); + DataAccessStrategy dataAccessStrategy = new DefaultDataAccessStrategy(new SqlGeneratorSource(mappingContext), + mappingContext, converter, namedParameterJdbcOperations); + template = new JdbcAggregateTemplate(eventPublisher, mappingContext, converter, dataAccessStrategy); + } + + @Test // DATAJDBC-378 + public void findAllByIdMustNotAcceptNullArgumentForType() { + assertThatThrownBy(() -> template.findAllById(singleton(23L), null)).isInstanceOf(IllegalArgumentException.class); + } + + @Test // DATAJDBC-378 + public void findAllByIdMustNotAcceptNullArgumentForIds() { + assertThatThrownBy(() -> template.findAllById(null, LegoSet.class)).isInstanceOf(IllegalArgumentException.class); + } + + @Test // DATAJDBC-378 + public void findAllByIdWithEmpthListMustReturnEmptyResult() { + assertThat(template.findAllById(emptyList(), LegoSet.class)).isEmpty(); + } +}