Make @interleaved eager again and again (spring-attic#2165)

spring-cloud-gcp-data-spanner/src/main/java/org/springframework/cloud/gcp/data/spanner/repository/query/Query.java

@@ -23,6 +23,7 @@
 import java.lang.annotation.Target;
 
 import org.springframework.data.annotation.QueryAnnotation;
+import org.springframework.data.domain.Pageable;
 
 /**
  * Annotation used in user-defined repositories to provide SQL for custom Query Methods.
@@ -40,6 +41,9 @@
 	/**
 	 * Takes a Cloud Spanner SQL string to define the actual query to be executed. This one will
 	 * take precedence over the method name then.
+	 * <p/>Please, pay attention that sorting conditions should be passed by a {@link Pageable} parameter
+	 * instead of the query part. Otherwise, with the current implementation, the sorting could be broken
+	 * by a logic that fetches eager-interleaved fields when they are in the query result.
 	 *
 	 * @return the SQL Cloud Spanner query string.
 	 */

spring-cloud-gcp-data-spanner/src/main/java/org/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryExecutor.java

@@ -26,8 +26,6 @@
 import java.util.StringJoiner;
 import java.util.function.BiFunction;
 import java.util.function.Function;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 import com.google.cloud.spanner.Key;
@@ -64,8 +62,6 @@
  */
 public final class SpannerStatementQueryExecutor {
 
-	private static Pattern SELECT_ALL_PATTERN = Pattern.compile("^select(\\s+?all|\\s+?distinct)*(?<star>\\s+?\\*)", Pattern.CASE_INSENSITIVE);
-
 	private SpannerStatementQueryExecutor() {
 	}
 
@@ -148,16 +144,20 @@ public static <T> String applySortingPagingQueryOptions(Class<T> entityClass,
 			SpannerMappingContext mappingContext, boolean fetchInterleaved) {
 		SpannerPersistentEntity<?> persistentEntity = mappingContext
 				.getPersistentEntity(entityClass);
-		final String sqlWithInterleaved = fetchInterleaved(sql, persistentEntity, mappingContext, fetchInterleaved);
+
 		// Cloud Spanner does not preserve the order of derived tables so we must not wrap the
 		// derived table
 		// in SELECT * FROM () if there is no overriding pageable param.
 		if ((options.getSort() == null || options.getSort().isUnsorted()) && options.getLimit() == null
-				&& options.getOffset() == null) {
-			return sqlWithInterleaved;
+				&& options.getOffset() == null && !fetchInterleaved) {
+			return sql;
 		}
+		final String subquery = fetchInterleaved ? getChildrenSubquery(persistentEntity, mappingContext) : "";
+		final String alias = subquery.isEmpty() ? "" : " " + persistentEntity.tableName();
 		StringBuilder sb = applySort(options.getSort(),
-				new StringBuilder("SELECT * FROM (").append(sqlWithInterleaved).append(")"), (o) -> {
+				new StringBuilder("SELECT *").append(subquery)
+						.append(" FROM (").append(sql).append(")").append(alias),
+				(o) -> {
 					SpannerPersistentProperty property = persistentEntity
 							.getPersistentProperty(o.getProperty());
 					return (property != null) ? property.getColumnName() : o.getProperty();
@@ -171,27 +171,6 @@ public static <T> String applySortingPagingQueryOptions(Class<T> entityClass,
 		return sb.toString();
 	}
 
-	private static String fetchInterleaved(String sql,
-			SpannerPersistentEntity<?> persistentEntity, SpannerMappingContext mappingContext, boolean fetchInterleaved) {
-		if (!fetchInterleaved) {
-			return sql;
-		}
-
-		final String subquery = getChildrenSubquery(persistentEntity, mappingContext);
-		if (subquery.isEmpty()) {
-			return sql;
-		}
-
-		final Pair<String, String> parts = splitSelectAll(sql);
-		return parts.getSecond().isEmpty() ? sql : parts.getFirst() + subquery + parts.getSecond();
-	}
-
-	private static Pair<String, String> splitSelectAll(String sql) {
-		Matcher matcher = SELECT_ALL_PATTERN.matcher(sql);
-		return !matcher.find() ? Pair.of(sql, "")
-				: Pair.of(sql.substring(0, matcher.end("star")), sql.substring(matcher.end("star")));
-	}
-
 	/**
 	 * Gets a {@link Statement} that returns the rows associated with a parent entity. This function is
 	 * intended to be used with parent-child interleaved tables, so that the retrieval of all

spring-cloud-gcp-data-spanner/src/test/java/org/springframework/cloud/gcp/data/spanner/repository/it/SpannerRepositoryIntegrationTests.java

@@ -190,7 +190,7 @@ public void queryMethodsTest() {
 				.compareTo(tradesReceivedPage2.get(1).getId())).isNegative();
 
 		List<Trade> buyTradesRetrieved = this.tradeRepository
-				.annotatedTradesByAction("BUY");
+				.annotatedTradesByAction("BUY", PageRequest.of(0, 100, Sort.by(Order.desc("id"))));
 		assertThat(buyTradesRetrieved).containsExactlyInAnyOrderElementsOf(trader1BuyTrades);
 		assertThat(buyTradesRetrieved.get(0).getId()).isGreaterThan(buyTradesRetrieved.get(1).getId());
 		assertThat(buyTradesRetrieved.get(1).getId()).isGreaterThan(buyTradesRetrieved.get(2).getId());

spring-cloud-gcp-data-spanner/src/test/java/org/springframework/cloud/gcp/data/spanner/repository/query/SqlSpannerQueryTests.java

@@ -132,9 +132,9 @@ public void noPageableParamNotWrappedQueryTest() throws NoSuchMethodException {
 		String sql = "SELECT DISTINCT * FROM "
 				+ ":org.springframework.cloud.gcp.data.spanner.repository.query.SqlSpannerQueryTests$Trade:";
 
-		String entityResolvedSql = "SELECT DISTINCT *"
+		String entityResolvedSql = "SELECT *"
 				+ ", ARRAY (SELECT AS STRUCT id, childId, value FROM children WHERE children.id = trades.id) as children "
-				+ "FROM trades";
+				+ "FROM (SELECT DISTINCT * FROM trades) trades";
 
 		Parameters parameters = mock(Parameters.class);
 
@@ -176,9 +176,9 @@ public void noPageableParamWrappedQueryTest() throws NoSuchMethodException {
 		String sql = "SELECT DISTINCT * FROM "
 				+ ":org.springframework.cloud.gcp.data.spanner.repository.query.SqlSpannerQueryTests$Trade:";
 
-		String entityResolvedSql = "SELECT DISTINCT *, " +
-				"ARRAY (SELECT AS STRUCT id, childId, value FROM children WHERE children.id = trades.id) as children" +
-				" FROM trades";
+		String entityResolvedSql = "SELECT *, " +
+				"ARRAY (SELECT AS STRUCT id, childId, value FROM children WHERE children.id = trades.id) as children " +
+				"FROM (SELECT DISTINCT * FROM trades) trades";
 
 		Parameters parameters = mock(Parameters.class);
 
@@ -227,17 +227,16 @@ public void compoundNameConventionTest() throws NoSuchMethodException {
 				+ "( trader_id=@tag2 AND price<@tag3 ) OR ( price>=@tag4 AND id<>NULL AND "
 				+ "trader_id=NULL AND trader_id LIKE %@tag5 AND price=TRUE AND price=FALSE AND "
 				+ "struct_val = @tag8 AND struct_val = @tag9 "
-				+ "price>@tag6 AND price<=@tag7 and price in unnest(@tag10))ORDER BY id DESC LIMIT 3;";
+				+ "price>@tag6 AND price<=@tag7 and price in unnest(@tag10)) ORDER BY id DESC LIMIT 3;";
 
-		String entityResolvedSql = "SELECT * FROM (SELECT DISTINCT *"
-				+ ", ARRAY (SELECT AS STRUCT id, childId, value FROM children WHERE children.id = trades.id) as children "
-				+ "FROM trades@{index=fakeindex} "
-				+ "WHERE price=@SpELtag1 AND price<>@SpELtag1 OR price<>@SpELtag2 AND "
+		String entityResolvedSql = "SELECT *, ARRAY (SELECT AS STRUCT id, childId, value FROM children WHERE children.id = trades.id) as children FROM " +
+				"(SELECT DISTINCT * FROM trades@{index=fakeindex}"
+				+ " WHERE price=@SpELtag1 AND price<>@SpELtag1 OR price<>@SpELtag2 AND "
 				+ "( action=@tag0 AND ticker=@tag1 ) OR "
 				+ "( trader_id=@tag2 AND price<@tag3 ) OR ( price>=@tag4 AND id<>NULL AND "
 				+ "trader_id=NULL AND trader_id LIKE %@tag5 AND price=TRUE AND price=FALSE AND "
 				+ "struct_val = @tag8 AND struct_val = @tag9 "
-				+ "price>@tag6 AND price<=@tag7 and price in unnest(@tag10))ORDER BY id DESC LIMIT 3) "
+				+ "price>@tag6 AND price<=@tag7 and price in unnest(@tag10)) ORDER BY id DESC LIMIT 3) trades "
 				+ "ORDER BY COLA ASC , COLB DESC LIMIT 10 OFFSET 30";
 
 		Object[] params = new Object[] { "BUY", this.pageable, "abcd", "abc123", 8.88,

spring-cloud-gcp-data-spanner/src/test/java/org/springframework/cloud/gcp/data/spanner/test/domain/TradeRepository.java

@@ -75,9 +75,11 @@ public interface TradeRepository extends SpannerRepository<Trade, Key> {
 			+ "where action = @action")
 	List<String> getFirstStringList(@Param("action") String action);
 
+	//The sort should be passed as a Pageable param - Spanner did not preserve the order
+	//of a wrapped query that we will have at fetching of eager-interleaved fields of the Trade entity
 	@Query("SELECT * FROM :org.springframework.cloud.gcp.data.spanner.test.domain.Trade:"
-			+ " WHERE action=@action AND action=#{#action} ORDER BY id desc")
-	List<Trade> annotatedTradesByAction(@Param("action") String action);
+			+ " WHERE action=@action AND action=#{#action}")
+	List<Trade> annotatedTradesByAction(@Param("action") String action, Pageable pageable);
 
 	List<TradeProjection> findByActionIgnoreCase(String action);