#138 - Allow multiple usages of the same named parameter.

We now allow multiple usages of the same named parameter if the underlying database supports identifiable placeholders.

SELECT * FROM person where name = :id or lastname = :id

gets translated to

SELECT * FROM person where name = $1 or lastname = $1

src/main/java/org/springframework/data/r2dbc/core/NamedParameterExpander.java

@@ -29,6 +29,9 @@ import org.springframework.data.r2dbc.dialect.BindMarkersFactory;
  * This class expands SQL from named parameters to native style placeholders at execution time. It also allows for
  * expanding a {@link java.util.List} of values to the appropriate number of placeholders.
  * <p>
+ * References to the same parameter name are substituted with the same bind marker placeholder if a
+ * {@link BindMarkersFactory} uses {@link BindMarkersFactory#identifiablePlaceholders() identifiable} placeholders.
+ * <p>
  * <b>NOTE: An instance of this class is thread-safe once configured.</b>
  *
  * @author Mark Paluch

src/main/java/org/springframework/data/r2dbc/core/NamedParameterUtils.java

@@ -31,6 +31,7 @@ import org.springframework.data.r2dbc.dialect.BindMarkers;
 import org.springframework.data.r2dbc.dialect.BindMarkersFactory;
 import org.springframework.data.r2dbc.dialect.BindTarget;
 import org.springframework.util.Assert;
+import org.springframework.util.CollectionUtils;
 
 /**
  * Helper methods for named parameter parsing.
@@ -38,6 +39,9 @@ import org.springframework.util.Assert;
  * Only intended for internal use within Spring's Data's R2DBC framework. Partially extracted from Spring's JDBC named
  * parameter support.
  * <p>
+ * References to the same parameter name are substituted with the same bind marker placeholder if a
+ * {@link BindMarkersFactory} uses {@link BindMarkersFactory#identifiablePlaceholders() identifiable} placeholders.
+ * <p>
  * This is a subset of Spring Frameworks's {@code org.springframework.r2dbc.namedparam.NamedParameterUtils}.
  *
  * @author Thomas Risberg
@@ -260,7 +264,7 @@ abstract class NamedParameterUtils {
 	public static PreparedOperation<String> substituteNamedParameters(ParsedSql parsedSql,
 			BindMarkersFactory bindMarkersFactory, BindParameterSource paramSource) {
 
-		BindMarkerHolder markerHolder = new BindMarkerHolder(bindMarkersFactory.create());
+		NamedParameters markerHolder = new NamedParameters(bindMarkersFactory);
 
 		String originalSql = parsedSql.getOriginalSql();
 		List<String> paramNames = parsedSql.getParameterNames();
@@ -276,9 +280,11 @@ abstract class NamedParameterUtils {
 			int startIndex = indexes[0];
 			int endIndex = indexes[1];
 			actualSql.append(originalSql, lastIndex, startIndex);
+			NamedParameters.NamedParameter marker = markerHolder.getOrCreate(paramName);
 			if (paramSource.hasValue(paramName)) {
 				Object value = paramSource.getValue(paramName);
 				if (value instanceof Collection) {
+
 					Iterator<?> entryIter = ((Collection<?>) value).iterator();
 					int k = 0;
 					while (entryIter.hasNext()) {
@@ -294,19 +300,19 @@ abstract class NamedParameterUtils {
 								if (m > 0) {
 									actualSql.append(", ");
 								}
-								actualSql.append(markerHolder.addMarker(paramName));
+								actualSql.append(marker.addPlaceholder());
 							}
 							actualSql.append(')');
 						} else {
-							actualSql.append(markerHolder.addMarker(paramName));
+							actualSql.append(marker.addPlaceholder());
 						}
 
 					}
 				} else {
-					actualSql.append(markerHolder.addMarker(paramName));
+					actualSql.append(marker.getPlaceholder());
 				}
 			} else {
-				actualSql.append(markerHolder.addMarker(paramName));
+				actualSql.append(marker.getPlaceholder());
 			}
 			lastIndex = endIndex;
 		}
@@ -387,22 +393,79 @@ abstract class NamedParameterUtils {
 	}
 
 	/**
-	 * Holder for bind marker progress.
+	 * Holder for bind markers progress.
 	 */
-	private static class BindMarkerHolder {
+	static class NamedParameters {
 
 		private final BindMarkers bindMarkers;
-		private final Map<String, List<BindMarker>> markers = new TreeMap<>();
+		private final boolean identifiable;
+		private final Map<String, List<NamedParameter>> references = new TreeMap<>();
 
-		BindMarkerHolder(BindMarkers bindMarkers) {
-			this.bindMarkers = bindMarkers;
+		NamedParameters(BindMarkersFactory factory) {
+			this.bindMarkers = factory.create();
+			this.identifiable = factory.identifiablePlaceholders();
 		}
 
-		String addMarker(String name) {
+		/**
+		 * Get the {@link NamedParameter} identified by {@code namedParameter}.
+		 *
+		 * @param namedParameter
+		 * @return
+		 */
+		NamedParameter getOrCreate(String namedParameter) {
 
-			BindMarker bindMarker = this.bindMarkers.next(name);
-			this.markers.computeIfAbsent(name, ignore -> new ArrayList<>()).add(bindMarker);
-			return bindMarker.getPlaceholder();
+			List<NamedParameter> reference = this.references.computeIfAbsent(namedParameter, ignore -> new ArrayList<>());
+
+			if (reference.isEmpty()) {
+				NamedParameter param = new NamedParameter(namedParameter);
+				reference.add(param);
+				return param;
+			}
+
+			if (this.identifiable) {
+				return reference.get(0);
+			}
+
+			NamedParameter param = new NamedParameter(namedParameter);
+			reference.add(param);
+			return param;
+		}
+
+		List<NamedParameter> getMarker(String name) {
+			return this.references.get(name);
+		}
+
+		class NamedParameter {
+
+			private final String namedParameter;
+			private final List<BindMarker> placeholders = new ArrayList<>();
+
+			NamedParameter(String namedParameter) {
+				this.namedParameter = namedParameter;
+			}
+
+			/**
+			 * Create a placeholder to translate a single value into a bindable parameter.
+			 * <p>
+			 * Can be called multiple times to create placeholders for array/collections.
+			 *
+			 * @return
+			 */
+			String addPlaceholder() {
+
+				BindMarker bindMarker = NamedParameters.this.bindMarkers.next(this.namedParameter);
+				this.placeholders.add(bindMarker);
+				return bindMarker.getPlaceholder();
+			}
+
+			String getPlaceholder() {
+
+				if (this.placeholders.isEmpty()) {
+					return addPlaceholder();
+				}
+
+				return this.placeholders.get(0).getPlaceholder();
+			}
 		}
 	}
 
@@ -414,13 +477,13 @@ abstract class NamedParameterUtils {
 
 		private final String expandedSql;
 
-		private final Map<String, List<BindMarker>> markers;
+		private final NamedParameters parameters;
 
 		private final BindParameterSource parameterSource;
 
-		ExpandedQuery(String expandedSql, BindMarkerHolder bindMarkerHolder, BindParameterSource parameterSource) {
+		ExpandedQuery(String expandedSql, NamedParameters parameters, BindParameterSource parameterSource) {
 			this.expandedSql = expandedSql;
-			this.markers = bindMarkerHolder.markers;
+			this.parameters = parameters;
 			this.parameterSource = parameterSource;
 		}
 
@@ -435,13 +498,7 @@ abstract class NamedParameterUtils {
 				return;
 			}
 
-			if (bindMarkers.size() == 1) {
-				bindMarkers.get(0).bind(target, value);
-			} else {
-
-				Assert.isInstanceOf(Collection.class, value,
-						() -> String.format("Value [%s] must be an Collection with a size of [%d]", value, bindMarkers.size()));
-
+			if (value instanceof Collection) {
 				Collection<Object> collection = (Collection<Object>) value;
 
 				Iterator<Object> iterator = collection.iterator();
@@ -460,6 +517,10 @@ abstract class NamedParameterUtils {
 						bind(target, markers, valueToBind);
 					}
 				}
+			} else {
+				for (BindMarker bindMarker : bindMarkers) {
+					bindMarker.bind(target, value);
+				}
 			}
 		}
 
@@ -483,16 +544,26 @@ abstract class NamedParameterUtils {
 				return;
 			}
 
-			if (bindMarkers.size() == 1) {
-				bindMarkers.get(0).bindNull(target, valueType);
-				return;
+			for (BindMarker bindMarker : bindMarkers) {
+				bindMarker.bindNull(target, valueType);
 			}
-
-			throw new UnsupportedOperationException("bindNull(…) can bind only singular values");
 		}
 
-		private List<BindMarker> getBindMarkers(String identifier) {
-			return this.markers.get(identifier);
+		List<BindMarker> getBindMarkers(String identifier) {
+
+			List<NamedParameters.NamedParameter> parameters = this.parameters.getMarker(identifier);
+
+			if (parameters == null) {
+				return null;
+			}
+
+			List<BindMarker> markers = new ArrayList<>();
+
+			for (NamedParameters.NamedParameter parameter : parameters) {
+				markers.addAll(parameter.placeholders);
+			}
+
+			return markers;
 		}
 
 		@Override

src/main/java/org/springframework/data/r2dbc/dialect/BindMarkersFactory.java

@@ -26,6 +26,17 @@ public interface BindMarkersFactory {
 	 */
 	BindMarkers create();
 
+	/**
+	 * Return whether the {@link BindMarkersFactory} uses identifiable placeholders.
+	 *
+	 * @return whether the {@link BindMarkersFactory} uses identifiable placeholders. {@literal false} if multiple
+	 *         placeholders cannot be distinguished by just the {@link BindMarker#getPlaceholder() placeholder}
+	 *         identifier.
+	 */
+	default boolean identifiablePlaceholders() {
+		return true;
+	}
+
 	/**
 	 * Create index-based {@link BindMarkers} using indexes to bind parameters. Allow customization of the bind marker
 	 * placeholder {@code prefix} to represent the bind marker as placeholder within the query.
@@ -40,6 +51,7 @@ public interface BindMarkersFactory {
 	static BindMarkersFactory indexed(String prefix, int beginWith) {
 
 		Assert.notNull(prefix, "Prefix must not be null!");
+
 		return () -> new IndexedBindMarkers(prefix, beginWith);
 	}
 
@@ -56,7 +68,19 @@ public interface BindMarkersFactory {
 	static BindMarkersFactory anonymous(String placeholder) {
 
 		Assert.hasText(placeholder, "Placeholder must not be empty!");
-		return () -> new AnonymousBindMarkers(placeholder);
+
+		return new BindMarkersFactory() {
+
+			@Override
+			public BindMarkers create() {
+				return new AnonymousBindMarkers(placeholder);
+			}
+
+			@Override
+			public boolean identifiablePlaceholders() {
+				return false;
+			}
+		};
 	}
 
 	/**

src/test/java/org/springframework/data/r2dbc/core/AbstractDatabaseClientIntegrationTests.java

@@ -152,6 +152,18 @@ public abstract class AbstractDatabaseClientIntegrationTests extends R2dbcIntegr
 				}).verifyComplete();
 	}
 
+	@Test // gh-2
+	public void executeSelectNamedParameters() {
+
+		DatabaseClient databaseClient = DatabaseClient.create(connectionFactory);
+
+		databaseClient.execute("SELECT id, name, manual FROM legoset WHERE name = :name or manual = :name") //
+				.bind("name", "unknown").as(LegoSet.class) //
+				.fetch().all() //
+				.as(StepVerifier::create) //
+				.verifyComplete();
+	}
+
 	@Test // gh-2
 	public void insert() {
 

src/test/java/org/springframework/data/r2dbc/core/NamedParameterUtilsUnitTests.java

@@ -19,7 +19,10 @@ import static org.assertj.core.api.Assertions.*;
 import static org.mockito.Mockito.*;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
 
 import org.junit.Test;
 
@@ -27,6 +30,7 @@ import org.springframework.data.r2dbc.dialect.BindMarkersFactory;
 import org.springframework.data.r2dbc.dialect.BindTarget;
 import org.springframework.data.r2dbc.dialect.PostgresDialect;
 import org.springframework.data.r2dbc.dialect.SqlServerDialect;
+import org.springframework.data.r2dbc.mapping.SettableValue;
 
 /**
  * Unit tests for {@link NamedParameterUtils}.
@@ -111,7 +115,7 @@ public class NamedParameterUtilsUnitTests {
 		String sql1 = "/*+ HINT */ xxx /* comment ? */ :a yyyy :b :c :a zzzzz -- :xx XX\n";
 
 		ParsedSql psql1 = NamedParameterUtils.parseSqlStatement(sql1);
-		assertThat(expand(psql1)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $4 zzzzz -- :xx XX\n");
+		assertThat(expand(psql1)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $1 zzzzz -- :xx XX\n");
 
 		MapBindParameterSource paramMap = new MapBindParameterSource(new HashMap<>());
 		paramMap.addValue("a", "a");
@@ -120,7 +124,7 @@ public class NamedParameterUtilsUnitTests {
 
 		String sql2 = "/*+ HINT */ xxx /* comment ? */ :a yyyy :b :c :a zzzzz -- :xx XX";
 		ParsedSql psql2 = NamedParameterUtils.parseSqlStatement(sql2);
-		assertThat(expand(psql2)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $4 zzzzz -- :xx XX");
+		assertThat(expand(psql2)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $1 zzzzz -- :xx XX");
 	}
 
 	@Test // gh-23
@@ -283,6 +287,129 @@ public class NamedParameterUtilsUnitTests {
 		assertThat(psql2.getParameterNames()).containsExactly("xxx");
 	}
 
+	@Test // gh-138
+	public void shouldAllowParsingMultipleUseOfParameter() {
+
+		String sql = "SELECT * FROM person where name = :id or lastname = :id";
+
+		ParsedSql parsed = NamedParameterUtils.parseSqlStatement(sql);
+
+		assertThat(parsed.getTotalParameterCount()).isEqualTo(2);
+		assertThat(parsed.getNamedParameterCount()).isEqualTo(1);
+		assertThat(parsed.getParameterNames()).containsExactly("id", "id");
+	}
+
+	@Test // gh-138
+	public void multipleEqualParameterReferencesBindsValueOnce() {
+
+		String sql = "SELECT * FROM person where name = :id or lastname = :id";
+
+		BindMarkersFactory factory = BindMarkersFactory.indexed("$", 0);
+
+		PreparedOperation<String> operation = NamedParameterUtils.substituteNamedParameters(sql, factory,
+				new MapBindParameterSource(Collections.singletonMap("id", SettableValue.from("foo"))));
+
+		assertThat(operation.toQuery()).isEqualTo("SELECT * FROM person where name = $0 or lastname = $0");
+
+		operation.bindTo(new BindTarget() {
+			@Override
+			public void bind(Object identifier, Object value) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bind(int index, Object value) {
+				assertThat(index).isEqualTo(0);
+				assertThat(value).isEqualTo("foo");
+			}
+
+			@Override
+			public void bindNull(Object identifier, Class<?> type) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bindNull(int index, Class<?> type) {
+				throw new UnsupportedOperationException();
+			}
+		});
+	}
+
+	@Test // gh-138
+	public void multipleEqualParameterReferencesForAnonymousMarkersBindsValueMultipleTimes() {
+
+		String sql = "SELECT * FROM person where name = :id or lastname = :id";
+
+		BindMarkersFactory factory = BindMarkersFactory.anonymous("?");
+
+		PreparedOperation<String> operation = NamedParameterUtils.substituteNamedParameters(sql, factory,
+				new MapBindParameterSource(Collections.singletonMap("id", SettableValue.from("foo"))));
+
+		assertThat(operation.toQuery()).isEqualTo("SELECT * FROM person where name = ? or lastname = ?");
+
+		Map<Integer, Object> bindValues = new LinkedHashMap<>();
+
+		operation.bindTo(new BindTarget() {
+			@Override
+			public void bind(Object identifier, Object value) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bind(int index, Object value) {
+				bindValues.put(index, value);
+			}
+
+			@Override
+			public void bindNull(Object identifier, Class<?> type) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bindNull(int index, Class<?> type) {
+				throw new UnsupportedOperationException();
+			}
+		});
+
+		assertThat(bindValues).hasSize(2).containsEntry(0, "foo").containsEntry(1, "foo");
+	}
+
+	@Test // gh-138
+	public void multipleEqualParameterReferencesBindsNullOnce() {
+
+		String sql = "SELECT * FROM person where name = :id or lastname = :id";
+
+		BindMarkersFactory factory = BindMarkersFactory.indexed("$", 0);
+
+		PreparedOperation<String> operation = NamedParameterUtils.substituteNamedParameters(sql, factory,
+				new MapBindParameterSource(Collections.singletonMap("id", SettableValue.empty(String.class))));
+
+		assertThat(operation.toQuery()).isEqualTo("SELECT * FROM person where name = $0 or lastname = $0");
+
+		operation.bindTo(new BindTarget() {
+			@Override
+			public void bind(Object identifier, Object value) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bind(int index, Object value) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bindNull(Object identifier, Class<?> type) {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void bindNull(int index, Class<?> type) {
+				assertThat(index).isEqualTo(0);
+				assertThat(type).isEqualTo(String.class);
+			}
+		});
+	}
+
 	private String expand(ParsedSql sql) {
 		return NamedParameterUtils.substituteNamedParameters(sql, BIND_MARKERS, new MapBindParameterSource()).toQuery();
 	}

src/test/java/org/springframework/data/r2dbc/core/PostgresDatabaseClientIntegrationTests.java

@@ -55,6 +55,11 @@ public class PostgresDatabaseClientIntegrationTests extends AbstractDatabaseClie
 	@Override
 	public void insert() {}
 
+	@Ignore("Postgres considers multiple references to the same parameter as multiple bindings (where name = $1 OR lastname = $1)")
+	@Test
+	@Override
+	public void executeSelectNamedParameters() {}
+
 	@Ignore("Adding RETURNING * lets Postgres report 0 affected rows.")
 	@Test
 	@Override