DATAJDBC-101 - Polishing.

Refactored SQL generation.
Adapted some assertions to the "Spring Data JDBC style".
Minor formatting.

Original pull request: #188.

Author: schauder

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateOperations.java

@@ -140,6 +140,7 @@ public interface JdbcAggregateOperations {
 	 * @param <T> the type of the aggregate roots. Must not be {@code null}.
 	 * @param sort the sorting information. Must not be {@code null}.
 	 * @return Guaranteed to be not {@code null}.
+	 * @since 2.0
 	 */
 	<T> Iterable<T> findAll(Class<T> domainType, Sort sort);
 
@@ -150,6 +151,7 @@ public interface JdbcAggregateOperations {
 	 * @param <T> the type of the aggregate roots. Must not be {@code null}.
 	 * @param pageable the pagination information. Must not be {@code null}.
 	 * @return Guaranteed to be not {@code null}.
+	 * @since 2.0
 	 */
 	<T> Page<T> findAll(Class<T> domainType, Pageable pageable);
 }

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/DataAccessStrategy.java

@@ -226,6 +226,7 @@ default Iterable<Object> findAllByPath(Identifier identifier,
 	 * @param <T> the type of entities to load.
 	 * @param sort the sorting information. Must not be {@code null}.
 	 * @return Guaranteed to be not {@code null}.
+	 * @since 2.0
 	 */
 	<T> Iterable<T> findAll(Class<T> domainType, Sort sort);
 
@@ -236,6 +237,7 @@ default Iterable<Object> findAllByPath(Identifier identifier,
 	 * @param <T> the type of entities to load.
 	 * @param pageable the pagination information. Must not be {@code null}.
 	 * @return Guaranteed to be not {@code null}.
+	 * @since 2.0
 	 */
 	<T> Iterable<T> findAll(Class<T> domainType, Pageable pageable);
 }

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java

@@ -17,16 +17,7 @@
 
 import lombok.Value;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.HashSet;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
+import java.util.*;
 import java.util.function.Function;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
@@ -420,24 +411,29 @@ private SelectBuilder.SelectWhere selectBuilder(Collection<String> keyColumns) {
 	}
 
 	private SelectBuilder.SelectOrdered selectBuilder(Collection<String> keyColumns, Sort sort, Pageable pageable) {
-		SelectBuilder.SelectWhere baseSelect = this.selectBuilder(keyColumns);
 
-		if (baseSelect instanceof SelectBuilder.SelectFromAndJoin) {
-			if (pageable.isPaged()) {
-				return ((SelectBuilder.SelectFromAndJoin) baseSelect).limitOffset(pageable.getPageSize(), pageable.getOffset())
-						.orderBy(extractOrderByFields(sort));
-			}
-			return ((SelectBuilder.SelectFromAndJoin) baseSelect).orderBy(extractOrderByFields(sort));
+		SelectBuilder.SelectOrdered sortable = this.selectBuilder(keyColumns);
+		sortable = applyPagination(pageable, sortable);
+		return sortable.orderBy(extractOrderByFields(sort));
 
-		} else if (baseSelect instanceof SelectBuilder.SelectFromAndJoinCondition) {
-			if (pageable.isPaged()) {
-				return ((SelectBuilder.SelectFromAndJoinCondition) baseSelect)
-						.limitOffset(pageable.getPageSize(), pageable.getOffset()).orderBy(extractOrderByFields(sort));
-			}
-			return baseSelect.orderBy(extractOrderByFields(sort));
-		} else {
-			throw new RuntimeException("Unexpected type found!");
+	}
+
+	private SelectBuilder.SelectOrdered applyPagination(Pageable pageable, SelectBuilder.SelectOrdered select) {
+
+		if (!pageable.isPaged()) {
+			return select;
 		}
+
+		Assert.isTrue(select instanceof SelectBuilder.SelectLimitOffset,
+				() -> String.format("Can't apply limit clause to statement of type %s", select.getClass()));
+
+		SelectBuilder.SelectLimitOffset limitable = (SelectBuilder.SelectLimitOffset) select;
+		SelectBuilder.SelectLimitOffset limitResult = limitable.limitOffset(pageable.getPageSize(), pageable.getOffset());
+
+		Assert.state(limitResult instanceof SelectBuilder.SelectOrdered,
+				String.format("The result of applying the limit-clause must be of type SelectOrdered in order to apply the order-by-clause but is of type %s.", select.getClass()));
+
+		return (SelectBuilder.SelectOrdered) limitResult;
 	}
 
 	/**

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/mybatis/MyBatisDataAccessStrategy.java

@@ -347,6 +347,7 @@ public <T> boolean existsById(Object id, Class<T> domainType) {
 	 */
 	@Override
 	public <T> Iterable<T> findAll(Class<T> domainType, Sort sort) {
+
 		Map<String, Object> additionalContext = new HashMap<>();
 		additionalContext.put("sort", sort);
 		return sqlSession().selectList(namespace(domainType) + ".findAllSorted",
@@ -359,6 +360,7 @@ public <T> Iterable<T> findAll(Class<T> domainType, Sort sort) {
 	 */
 	@Override
 	public <T> Iterable<T> findAll(Class<T> domainType, Pageable pageable) {
+
 		Map<String, Object> additionalContext = new HashMap<>();
 		additionalContext.put("pageable", pageable);
 		return sqlSession().selectList(namespace(domainType) + ".findAllPaged",

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/support/SimpleJdbcRepository.java

@@ -40,7 +40,7 @@
  */
 @RequiredArgsConstructor
 @Transactional(readOnly = true)
-public class SimpleJdbcRepository<T, ID> implements CrudRepository<T, ID>, PagingAndSortingRepository<T, ID> {
+public class SimpleJdbcRepository<T, ID> implements PagingAndSortingRepository<T, ID> {
 
 	private final @NonNull JdbcAggregateOperations entityOperations;
 	private final @NonNull PersistentEntity<T, ?> entity;

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java

@@ -39,7 +39,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;
@@ -225,41 +224,51 @@ public void saveAndLoadManyEntitiesWithReferencedEntity() {
 
 		Iterable<LegoSet> reloadedLegoSets = template.findAll(LegoSet.class);
 
-		assertThat(reloadedLegoSets).hasSize(1).extracting("id", "manual.id", "manual.content")
-				.contains(tuple(legoSet.getId(), legoSet.getManual().getId(), legoSet.getManual().getContent()));
+		assertThat(reloadedLegoSets) //
+				.extracting("id", "manual.id", "manual.content") //
+				.containsExactly(tuple(legoSet.getId(), legoSet.getManual().getId(), legoSet.getManual().getContent()));
 	}
 
 	@Test // DATAJDBC-101
 	public void saveAndLoadManyEntitiesWithReferencedEntitySorted() {
+
 		template.save(createLegoSet("Lava"));
 		template.save(createLegoSet("Star"));
 		template.save(createLegoSet("Frozen"));
 
 		Iterable<LegoSet> reloadedLegoSets = template.findAll(LegoSet.class, Sort.by("name"));
 
-		assertThat(reloadedLegoSets).hasSize(3).extracting("name").isEqualTo(Arrays.asList("Frozen", "Lava", "Star"));
+		assertThat(reloadedLegoSets) //
+				.extracting("name") //
+				.containsExactly("Frozen", "Lava", "Star");
 	}
 
 	@Test // DATAJDBC-101
 	public void saveAndLoadManyEntitiesWithReferencedEntityPaged() {
+
 		template.save(createLegoSet("Lava"));
 		template.save(createLegoSet("Star"));
 		template.save(createLegoSet("Frozen"));
 
 		Iterable<LegoSet> reloadedLegoSets = template.findAll(LegoSet.class, PageRequest.of(1, 1));
 
-		assertThat(reloadedLegoSets).hasSize(1).extracting("name").isEqualTo(singletonList("Star"));
+		assertThat(reloadedLegoSets) //
+				.extracting("name") //
+				.containsExactly("Star");
 	}
 
 	@Test // DATAJDBC-101
 	public void saveAndLoadManyEntitiesWithReferencedEntitySortedAndPaged() {
+
 		template.save(createLegoSet("Lava"));
 		template.save(createLegoSet("Star"));
 		template.save(createLegoSet("Frozen"));
 
 		Iterable<LegoSet> reloadedLegoSets = template.findAll(LegoSet.class, PageRequest.of(1, 2, Sort.by("name")));
 
-		assertThat(reloadedLegoSets).hasSize(1).extracting("name").isEqualTo(singletonList("Star"));
+		assertThat(reloadedLegoSets) //
+				.extracting("name") //
+				.containsExactly("Star");
 	}
 
 	@Test // DATAJDBC-112
@@ -749,12 +758,10 @@ public void saveAndUpdateAggregateWithImmutableVersion() {
 		AggregateWithImmutableVersion savedAgain = template.save(reloadedAggregate);
 		AggregateWithImmutableVersion reloadedAgain = template.findById(id, aggregate.getClass());
 
-		assertThat(savedAgain.version)
-				.describedAs("The object returned by save should have an increased version")
+		assertThat(savedAgain.version).describedAs("The object returned by save should have an increased version")
 				.isEqualTo(2L);
 
-		assertThat(reloadedAgain.getVersion())
-				.describedAs("version field should increment by one with each save")
+		assertThat(reloadedAgain.getVersion()).describedAs("version field should increment by one with each save")
 				.isEqualTo(2L);
 
 		assertThatThrownBy(() -> template.save(new AggregateWithImmutableVersion(id, 1L)))