DATAJDBC-335 - Address review comments.

Author: schauder

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/AbstractImportValidator.java

@@ -18,6 +18,8 @@
 import java.util.HashSet;
 import java.util.Set;
 
+import org.springframework.lang.Nullable;
+
 /**
  * Validator for statements to import columns.
  *
@@ -42,13 +44,7 @@ public void enter(Visitable segment) {
 		}
 
 		if (segment instanceof Where) {
-
-			segment.visit(item -> {
-
-				if (item instanceof Table) {
-					requiredByWhere.add((Table) item);
-				}
-			});
+			segment.visit(new SubselectFilteringWhereVisitor());
 		}
 
 		if (segment instanceof Join || segment instanceof OrderByField || segment instanceof From
@@ -63,4 +59,45 @@ public void enter(Visitable segment) {
 	 */
 	@Override
 	public void leave(Visitable segment) {}
+
+	/**
+	 * {@link Visitor} that skips sub-{@link Select} and collects columns within a {@link Where} clause.
+	 */
+	class SubselectFilteringWhereVisitor implements Visitor {
+
+		private @Nullable Select selectFilter;
+
+		/* 
+		 * (non-Javadoc)
+		 * @see org.springframework.data.relational.core.sql.Visitor#enter(org.springframework.data.relational.core.sql.Visitable)
+		 */
+		@Override
+		public void enter(Visitable segment) {
+
+			if (selectFilter != null) {
+				return;
+			}
+
+			if (segment instanceof Select) {
+				this.selectFilter = (Select) segment;
+				return;
+			}
+
+			if (segment instanceof Table) {
+				requiredByWhere.add((Table) segment);
+			}
+		}
+
+		/* 
+		 * (non-Javadoc)
+		 * @see org.springframework.data.relational.core.sql.Visitor#leave(org.springframework.data.relational.core.sql.Visitable)
+		 */
+		@Override
+		public void leave(Visitable segment) {
+
+			if (this.selectFilter == segment) {
+				this.selectFilter = null;
+			}
+		}
+	}
 }

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/AssignValue.java

@@ -65,14 +65,14 @@ public Expression getValue() {
 		return value;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see java.lang.Object#toString()
 	 */
 	@Override
 	public String toString() {
 
-		StringBuilder builder = new StringBuilder(32);
+		StringBuilder builder = new StringBuilder();
 		return builder.append(this.column).append(" = ").append(this.value).toString();
 	}
 }

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/DefaultUpdate.java

@@ -40,7 +40,7 @@ class DefaultUpdate implements Update {
 		this.where = where != null ? new Where(where) : null;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.Visitable#visit(org.springframework.data.relational.core.sql.Visitor)
 	 */
@@ -61,22 +61,22 @@ public void visit(Visitor visitor) {
 		visitor.leave(this);
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see java.lang.Object#toString()
 	 */
 	@Override
 	public String toString() {
 
-		StringBuilder builder = new StringBuilder(32);
+		StringBuilder builder = new StringBuilder();
 		builder.append("UPDATE ").append(table);
 
 		if (!assignments.isEmpty()) {
 			builder.append(" SET ").append(StringUtils.collectionToDelimitedString(this.assignments, ", "));
 		}
 
 		if (this.where != null) {
-			builder.append(" WHERE ").append(this.where);
+			builder.append(" ").append(this.where);
 		}
 
 		return builder.toString();

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/DefaultUpdateBuilder.java

@@ -16,10 +16,11 @@
 package org.springframework.data.relational.core.sql;
 
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.List;
 
 import org.springframework.data.relational.core.sql.UpdateBuilder.UpdateAssign;
-import org.springframework.data.relational.core.sql.UpdateBuilder.UpdateAssignAnd;
 import org.springframework.data.relational.core.sql.UpdateBuilder.UpdateWhere;
 import org.springframework.data.relational.core.sql.UpdateBuilder.UpdateWhereAndOr;
 import org.springframework.lang.Nullable;
@@ -31,13 +32,13 @@
  * @author Mark Paluch
  * @since 1.1
  */
-class DefaultUpdateBuilder implements UpdateBuilder, UpdateWhere, UpdateWhereAndOr, UpdateAssign, UpdateAssignAnd {
+class DefaultUpdateBuilder implements UpdateBuilder, UpdateWhere, UpdateWhereAndOr, UpdateAssign {
 
 	private Table table;
 	private List<Assignment> assignments = new ArrayList<>();
 	private @Nullable Condition where;
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.UpdateBuilder#table(org.springframework.data.relational.core.sql.Table)
 	 */
@@ -51,7 +52,7 @@ public UpdateAssign table(Table table) {
 		return this;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateAssign#set(org.springframework.data.relational.core.sql.Assignment)
 	 */
@@ -65,16 +66,33 @@ public DefaultUpdateBuilder set(Assignment assignment) {
 		return this;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
-	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateAssignAnd#and(org.springframework.data.relational.core.sql.Assignment)
+	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateAssign#set(org.springframework.data.relational.core.sql.Assignment...)
 	 */
 	@Override
-	public DefaultUpdateBuilder and(Assignment assignment) {
-		return set(assignment);
+	public UpdateWhere set(Assignment... assignments) {
+
+		Assert.notNull(assignments, "Assignment must not be null!");
+
+		return set(Arrays.asList(assignments));
+	}
+
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateAssign#set(java.util.Collection)
+	 */
+	@Override
+	public UpdateWhere set(Collection<? extends Assignment> assignments) {
+
+		Assert.notNull(assignments, "Assignment must not be null!");
+
+		this.assignments.addAll(assignments);
+
+		return this;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateWhere#where(org.springframework.data.relational.core.sql.Condition)
 	 */
@@ -88,7 +106,7 @@ public UpdateWhereAndOr where(Condition condition) {
 		return this;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateWhereAndOr#and(org.springframework.data.relational.core.sql.Condition)
 	 */
@@ -102,7 +120,7 @@ public UpdateWhereAndOr and(Condition condition) {
 		return this;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.UpdateWhereAndOr#or(org.springframework.data.relational.core.sql.Condition)
 	 */
@@ -116,7 +134,7 @@ public UpdateWhereAndOr or(Condition condition) {
 		return this;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.relational.core.sql.UpdateBuilder.BuildUpdate#build()
 	 */

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/DeleteValidator.java

@@ -25,8 +25,14 @@
  */
 class DeleteValidator extends AbstractImportValidator {
 
-	public static void validate(Delete select) {
-		new DeleteValidator().doValidate(select);
+	/**
+	 * Validates a {@link Delete} statement.
+	 *
+	 * @param delete the {@link Delete} statement.
+	 * @throws IllegalStateException if the statement is not valid.
+	 */
+	public static void validate(Delete delete) {
+		new DeleteValidator().doValidate(delete);
 	}
 
 	private void doValidate(Delete select) {

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/SelectValidator.java

@@ -17,6 +17,7 @@
 
 import java.util.HashSet;
 import java.util.Set;
+import java.util.Stack;
 
 /**
  * Validator for {@link Select} statements.
@@ -29,12 +30,20 @@
  */
 class SelectValidator extends AbstractImportValidator {
 
+	private final Stack<Select> selects = new Stack<>();
+
 	private int selectFieldCount;
 	private Set<Table> requiredBySelect = new HashSet<>();
 	private Set<Table> requiredByOrderBy = new HashSet<>();
 
 	private Set<Table> join = new HashSet<>();
 
+	/**
+	 * Validates a {@link Select} statement.
+	 *
+	 * @param select the {@link Select} statement.
+	 * @throws IllegalStateException if the statement is not valid.
+	 */
 	public static void validate(Select select) {
 		new SelectValidator().doValidate(select);
 	}
@@ -76,6 +85,14 @@ private void doValidate(Select select) {
 	@Override
 	public void enter(Visitable segment) {
 
+		if (segment instanceof Select) {
+			selects.push((Select) segment);
+		}
+
+		if (selects.size() > 1) {
+			return;
+		}
+
 		super.enter(segment);
 
 		if (segment instanceof AsteriskFromTable && parent instanceof Select) {
@@ -107,15 +124,23 @@ public void enter(Visitable segment) {
 		if (segment instanceof Table && parent instanceof Join) {
 			join.add((Table) segment);
 		}
+	}
 
-		if (segment instanceof Where) {
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.relational.core.sql.AbstractImportValidator#leave(org.springframework.data.relational.core.sql.Visitable)
+	 */
+	@Override
+	public void leave(Visitable segment) {
 
-			segment.visit(item -> {
+		if (segment instanceof Select) {
+			selects.remove(segment);
+		}
 
-				if (item instanceof Table) {
-					requiredByWhere.add((Table) item);
-				}
-			});
+		if (selects.size() > 1) {
+			return;
 		}
+
+		super.leave(segment);
 	}
 }

spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/UpdateBuilder.java

@@ -15,6 +15,8 @@
  */
 package org.springframework.data.relational.core.sql;
 
+import java.util.Collection;
+
 /**
  * Entry point to construct an {@link Update} statement.
  *
@@ -44,22 +46,25 @@ interface UpdateAssign {
 		 * @return {@code this} builder.
 		 * @see Assignment
 		 */
-		UpdateAssignAnd set(Assignment assignment);
-	}
+		UpdateWhere set(Assignment assignment);
 
-	/**
-	 * Interface exposing {@code SET} methods.
-	 */
-	interface UpdateAssignAnd extends UpdateWhere {
+		/**
+		 * Apply one or more {@link Assignment SET assignments}.
+		 *
+		 * @param assignments the {@link Assignment column assignments}.
+		 * @return {@code this} builder.
+		 * @see Assignment
+		 */
+		UpdateWhere set(Assignment... assignments);
 
 		/**
-		 * Apply a {@link Assignment SET assignment}.
+		 * Apply one or more {@link Assignment SET assignments}.
 		 *
-		 * @param assignment a single {@link Assignment column assignment}.
+		 * @param assignments the {@link Assignment column assignments}.
 		 * @return {@code this} builder.
 		 * @see Assignment
 		 */
-		UpdateAssignAnd and(Assignment assignment);
+		UpdateWhere set(Collection<? extends Assignment> assignments);
 	}
 
 	/**

spring-data-relational/src/test/java/org/springframework/data/relational/core/sql/DeleteValidatorUnitTests.java

@@ -20,7 +20,7 @@
 import org.junit.Test;
 
 /**
- * Unit tests for {@link SelectValidator}.
+ * Unit tests for {@link DeleteValidator}.
  *
  * @author Mark Paluch
  */
@@ -40,4 +40,18 @@ public void shouldReportMissingTableForDeleteViaWhere() {
 		}).isInstanceOf(IllegalStateException.class)
 				.hasMessageContaining("Required table [table] by a WHERE predicate not imported by FROM [bar]");
 	}
+
+	@Test // DATAJDBC-335
+	public void shouldIgnoreImportsFromSubselectsInWhereClause() {
+
+		Table foo = SQL.table("foo");
+		Column bar = foo.column("bar");
+
+		Table floo = SQL.table("floo");
+		Column bah = floo.column("bah");
+
+		Select subselect = Select.builder().select(bah).from(floo).build();
+
+		assertThat(Delete.builder().from(foo).where(Conditions.in(bar, subselect)).build()).isNotNull();
+	}
 }