#15 - Incorporate review feedback.

Improve Javadoc. Rename parameters for more expressiveness. Replace ASCII filtering with filter Function to move actual filtering into dialects.

Original pull request: #19.

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

@@ -24,13 +24,13 @@ public interface BindMarkers {
 	BindMarker next();
 
 	/**
-	 * Creates a new {@link BindMarker} that accepts a {@code nameHint}. Implementations are allowed to consider/ignore
+	 * Creates a new {@link BindMarker} that accepts a {@code hint}. Implementations are allowed to consider/ignore/filter
 	 * the name hint to create more expressive bind markers.
 	 *
-	 * @param nameHint an optional name hint.
+	 * @param hint an optional name hint that can be used as part of the bind marker.
 	 * @return a new {@link BindMarker}.
 	 */
-	default BindMarker next(String nameHint) {
+	default BindMarker next(String hint) {
 		return next();
 	}
 }

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

@@ -1,9 +1,16 @@
 package org.springframework.data.r2dbc.dialect;
 
+import java.util.function.Function;
+
 import org.springframework.util.Assert;
 
 /**
  * This class creates new {@link BindMarkers} instances to bind parameter for a specific {@link io.r2dbc.spi.Statement}.
+ * <p/>
+ * Bind markers can be typically represented as placeholder and identifier. Placeholders are used within the query to
+ * execute so the underlying database system can substitute the placeholder with the actual value. Identifiers are used
+ * in R2DBC drivers to bind a value to a bind marker. Identifiers are typically a part of an entire bind marker when
+ * using indexed or named bind markers.
  *
  * @author Mark Paluch
  * @see BindMarkers
@@ -20,11 +27,15 @@ public interface BindMarkersFactory {
 	BindMarkers create();
 
 	/**
-	 * Create index-based {@link BindMarkers}.
+	 * 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.
 	 *
-	 * @param prefix bind parameter prefix.
+	 * @param prefix bind parameter prefix that is included in {@link BindMarker#getPlaceholder()} but not the actual
+	 *          identifier.
 	 * @param beginWith the first index to use.
 	 * @return a {@link BindMarkersFactory} using {@code prefix} and {@code beginWith}.
+	 * @see io.r2dbc.spi.Statement#bindNull(int, Class)
+	 * @see io.r2dbc.spi.Statement#bind(int, Object)
 	 */
 	static BindMarkersFactory indexed(String prefix, int beginWith) {
 
@@ -33,21 +44,49 @@ public interface BindMarkersFactory {
 	}
 
 	/**
-	 * Create named {@link BindMarkers}. Named bind markers can support {@link BindMarkers#next(String) name hints}.
-	 * Typically, named markers use name hints. If no namehint is given, named bind markers use a counter to generate
-	 * unique bind markers.
+	 * Create named {@link BindMarkers} using identifiers to bind parameters. Named bind markers can support
+	 * {@link BindMarkers#next(String) name hints}. If no {@link BindMarkers#next(String) hint} is given, named bind
+	 * markers can use a counter or a random value source to generate unique bind markers.
+	 * <p/>
+	 * Allow customization of the bind marker placeholder {@code prefix} and {@code namePrefix} to represent the bind
+	 * marker as placeholder within the query.
+	 *
+	 * @param prefix bind parameter prefix that is included in {@link BindMarker#getPlaceholder()} but not the actual
+	 *          identifier.
+	 * @param namePrefix prefix for bind marker name that is included in {@link BindMarker#getPlaceholder()} and the
+	 *          actual identifier.
+	 * @param maxLength maximal length of parameter names when using name hints.
+	 * @return a {@link BindMarkersFactory} using {@code prefix} and {@code beginWith}.
+	 * @see io.r2dbc.spi.Statement#bindNull(Object, Class)
+	 * @see io.r2dbc.spi.Statement#bind(Object, Object)
+	 */
+	static BindMarkersFactory named(String prefix, String namePrefix, int maxLength) {
+		return named(prefix, namePrefix, maxLength, Function.identity());
+	}
+
+	/**
+	 * Create named {@link BindMarkers} using identifiers to bind parameters. Named bind markers can support
+	 * {@link BindMarkers#next(String) name hints}. If no {@link BindMarkers#next(String) hint} is given, named bind
+	 * markers can use a counter or a random value source to generate unique bind markers.
 	 *
-	 * @param prefix bind parameter prefix.
-	 * @param indexPrefix prefix for bind markers that were created by incrementing a counter to generate a unique bind
-	 *          marker.
-	 * @param nameLimit maximal length of parameter names when using name hints.
+	 * @param prefix bind parameter prefix that is included in {@link BindMarker#getPlaceholder()} but not the actual
+	 *          identifier.
+	 * @param namePrefix prefix for bind marker name that is included in {@link BindMarker#getPlaceholder()} and the
+	 *          actual identifier.
+	 * @param maxLength maximal length of parameter names when using name hints.
+	 * @param hintFilterFunction filter {@link Function} to consider database-specific limitations in bind marker/variable
+	 *          names such as ASCII chars only.
 	 * @return a {@link BindMarkersFactory} using {@code prefix} and {@code beginWith}.
+	 * @see io.r2dbc.spi.Statement#bindNull(Object, Class)
+	 * @see io.r2dbc.spi.Statement#bind(Object, Object)
 	 */
-	static BindMarkersFactory named(String prefix, String indexPrefix, int nameLimit) {
+	static BindMarkersFactory named(String prefix, String namePrefix, int maxLength,
+			Function<String, String> hintFilterFunction) {
 
 		Assert.notNull(prefix, "Prefix must not be null!");
-		Assert.notNull(indexPrefix, "Index prefix must not be null!");
+		Assert.notNull(namePrefix, "Index prefix must not be null!");
+		Assert.notNull(hintFilterFunction, "Hint filter function must not be null!");
 
-		return () -> new NamedBindMarkers(prefix, indexPrefix, nameLimit);
+		return () -> new NamedBindMarkers(prefix, namePrefix, maxLength, hintFilterFunction);
 	}
 }

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

@@ -3,6 +3,7 @@ package org.springframework.data.r2dbc.dialect;
 import io.r2dbc.spi.Statement;
 
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
+import java.util.function.Function;
 
 import org.springframework.util.Assert;
 
@@ -21,17 +22,24 @@ class NamedBindMarkers implements BindMarkers {
 
 	private final String prefix;
 
-	private final String indexPrefix;
+	private final String namePrefix;
 
 	private final int nameLimit;
 
-	NamedBindMarkers(String prefix, String indexPrefix, int nameLimit) {
+	private final Function<String, String> hintFilterFunction;
+
+	NamedBindMarkers(String prefix, String namePrefix, int nameLimit, Function<String, String> hintFilterFunction) {
 
 		this.prefix = prefix;
-		this.indexPrefix = indexPrefix;
+		this.namePrefix = namePrefix;
 		this.nameLimit = nameLimit;
+		this.hintFilterFunction = hintFilterFunction;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.r2dbc.dialect.BindMarkers#next()
+	 */
 	@Override
 	public BindMarker next() {
 
@@ -40,18 +48,16 @@ class NamedBindMarkers implements BindMarkers {
 		return new NamedBindMarker(prefix + name, name);
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.r2dbc.dialect.BindMarkers#next(java.lang.String)
+	 */
 	@Override
-	public BindMarker next(String nameHint) {
-
-		Assert.notNull(nameHint, "Name hint must not be null");
+	public BindMarker next(String hint) {
 
-		String name = nextName();
-
-		String filteredNameHint = filter(nameHint);
+		Assert.notNull(hint, "Name hint must not be null");
 
-		if (!filteredNameHint.isEmpty()) {
-			name += "_" + filteredNameHint;
-		}
+		String name = nextName() + hintFilterFunction.apply(hint);
 
 		if (name.length() > nameLimit) {
 			name = name.substring(0, nameLimit);
@@ -63,25 +69,7 @@ class NamedBindMarkers implements BindMarkers {
 	private String nextName() {
 
 		int index = COUNTER_INCREMENTER.getAndIncrement(this);
-		return indexPrefix + index;
-	}
-
-	private static String filter(CharSequence input) {
-
-		StringBuilder builder = new StringBuilder();
-
-		for (int i = 0; i < input.length(); i++) {
-
-			char ch = input.charAt(i);
-
-			// ascii letter or digit
-			if (Character.isLetterOrDigit(ch) && ch < 127) {
-				builder.append(ch);
-			}
-
-		}
-
-		return builder.toString();
+		return namePrefix + index;
 	}
 
 	/**

src/test/java/org/springframework/data/r2dbc/dialect/IndexedBindMarkersUnitTests.java

@@ -29,13 +29,18 @@ public class IndexedBindMarkersUnitTests {
 	@Test // gh-15
 	public void nextShouldIncrementBindMarker() {
 
-		BindMarkers bindMarkers = BindMarkersFactory.indexed("$", 0).create();
+		String[] prefixes = { "$", "?" };
+
+		for (String prefix : prefixes) {
+
+			BindMarkers bindMarkers = BindMarkersFactory.indexed(prefix, 0).create();
 
-		BindMarker marker1 = bindMarkers.next();
-		BindMarker marker2 = bindMarkers.next();
+			BindMarker marker1 = bindMarkers.next();
+			BindMarker marker2 = bindMarkers.next();
 
-		assertThat(marker1.getPlaceholder()).isEqualTo("$0");
-		assertThat(marker2.getPlaceholder()).isEqualTo("$1");
+			assertThat(marker1.getPlaceholder()).isEqualTo(prefix + "0");
+			assertThat(marker2.getPlaceholder()).isEqualTo(prefix + "1");
+		}
 	}
 
 	@Test // gh-15

src/test/java/org/springframework/data/r2dbc/dialect/NamedBindMarkersUnitTests.java

@@ -29,24 +29,46 @@ public class NamedBindMarkersUnitTests {
 	@Test // gh-15
 	public void nextShouldIncrementBindMarker() {
 
-		BindMarkers bindMarkers = BindMarkersFactory.named("@", "p", 32).create();
+		String[] prefixes = { "$", "?" };
 
-		BindMarker marker1 = bindMarkers.next();
-		BindMarker marker2 = bindMarkers.next();
+		for (String prefix : prefixes) {
 
-		assertThat(marker1.getPlaceholder()).isEqualTo("@p0");
-		assertThat(marker2.getPlaceholder()).isEqualTo("@p1");
+			BindMarkers bindMarkers = BindMarkersFactory.named(prefix, "p", 32).create();
+
+			BindMarker marker1 = bindMarkers.next();
+			BindMarker marker2 = bindMarkers.next();
+
+			assertThat(marker1.getPlaceholder()).isEqualTo(prefix + "p0");
+			assertThat(marker2.getPlaceholder()).isEqualTo(prefix + "p1");
+		}
 	}
 
 	@Test // gh-15
 	public void nextShouldConsiderNameHint() {
 
-		BindMarkers bindMarkers = BindMarkersFactory.named("@", "p", 32).create();
+		BindMarkers bindMarkers = BindMarkersFactory.named("@", "x", 32).create();
+
+		BindMarker marker1 = bindMarkers.next("foo1bar");
+		BindMarker marker2 = bindMarkers.next();
 
-		BindMarker marker1 = bindMarkers.next("foo.bar?");
+		assertThat(marker1.getPlaceholder()).isEqualTo("@x0foo1bar");
+		assertThat(marker2.getPlaceholder()).isEqualTo("@x1");
+	}
+
+	@Test // gh-15
+	public void nextShouldConsiderFilteredNameHint() {
+
+		BindMarkers bindMarkers = BindMarkersFactory.named("@", "p", 32, s -> {
+
+			return s.chars().filter(Character::isAlphabetic)
+					.collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append).toString();
+
+		}).create();
+
+		BindMarker marker1 = bindMarkers.next("foo1.bar?");
 		BindMarker marker2 = bindMarkers.next();
 
-		assertThat(marker1.getPlaceholder()).isEqualTo("@p0_foobar");
+		assertThat(marker1.getPlaceholder()).isEqualTo("@p0foobar");
 		assertThat(marker2.getPlaceholder()).isEqualTo("@p1");
 	}
 
@@ -57,7 +79,7 @@ public class NamedBindMarkersUnitTests {
 
 		BindMarker marker1 = bindMarkers.next("123456789");
 
-		assertThat(marker1.getPlaceholder()).isEqualTo("@p0_1234567");
+		assertThat(marker1.getPlaceholder()).isEqualTo("@p012345678");
 	}
 
 	@Test // gh-15
@@ -82,7 +104,6 @@ public class NamedBindMarkersUnitTests {
 		BindMarkers bindMarkers = BindMarkersFactory.named("@", "p", 32).create();
 
 		bindMarkers.next(); // ignore
-
 		bindMarkers.next().bindNull(statement, Integer.class);
 
 		verify(statement).bindNull("p1", Integer.class);