#64 - Polishing.

Incorporated feedback from review. Polished documentation and Javadoc. Minor code improvements restructuring for better readability. Removed unused methods types. Some polishing for compiler warnings.

Original pull request: #106.

Author: odrotbohm

src/main/asciidoc/reference/r2dbc-core.adoc

@@ -236,6 +236,6 @@ As of writing the following drivers are available:
 * https://github.com/r2dbc/r2dbc-mssql[Microsoft SQL Server] (`io.r2dbc:r2dbc-mssql`)
 * https://github.com/jasync-sql/jasync-sql[Microsoft SQL Server] (`com.github.jasync-sql:jasync-r2dbc-mysql`)
 
-Spring Data R2DBC reacts to database specifics by inspecting `ConnectionFactoryMetadata` and selects the appropriate database dialect.
-You can configure an own `Dialect` if the used driver is not yet known to Spring Data R2DBC.
+Spring Data R2DBC reacts to database specifics by inspecting `ConnectionFactoryMetadata` exposed by the `ConnectionFactory` and selects the appropriate database dialect accordingly.
+You can configure an own https://docs.spring.io/spring-data/r2dbc/docs/{version}/api/org/springframework/data/r2dbc/dialect/Dialect.html[`Dialect`] if the used driver is not yet known to Spring Data R2DBC.
 

src/main/asciidoc/reference/r2dbc-databaseclient.adoc

@@ -24,10 +24,10 @@ DatabaseClient.create(ConnectionFactory connectionFactory)
 
 The above method creates a `DatabaseClient` with default settings.
 
-You can also use `DatabaseClient.builder()` with further options to customize the client:
+You can also obtain a `Builder` instance via `DatabaseClient.builder()` with further options to customize the client by calling the following methods:
 
-* `exceptionTranslator`: Supply a specific `R2dbcExceptionTranslator` to customize how R2DBC exceptions are translated into Spring's portable Data Access Exception hierarchy.  See "`<<r2dbc.exception>>`" for more information.
-* `dataAccessStrategy`: Strategy how SQL queries are generated and how objects are mapped.
+* `….exceptionTranslator(…)`: Supply a specific `R2dbcExceptionTranslator` to customize how R2DBC exceptions are translated into Spring's portable Data Access Exception hierarchy.  See "`<<r2dbc.exception>>`" for more information.
+* `….dataAccessStrategy(…)`: Strategy how SQL queries are generated and how objects are mapped.
 
 Once built, a `DatabaseClient` instance is immutable. However, you can clone it and build a modified copy without affecting the original instance, as the following example shows:
 
@@ -50,6 +50,19 @@ When you use Spring Data R2DBC, you can create a `ConnectionFactory` using your
 `ConnectionFactory` implementations can either return the same connection, different connections or provide connection pooling.
 `DatabaseClient` uses `ConnectionFactory` to create and release connections per operation without affinity to a particular connection across multiple operations.
 
+Assuming you'd be using H2 as a database, a typical programmatic setup looks something like this:
+
+[source, java]
+----
+H2ConnectionConfiguration config = … <1>
+ConnectionFactory factory = new H2ConnectionFactory(config); <2>
+
+DatabaseClient client = DatabaseClient.create(factory); <3>
+----
+<1> Prepare the database specific configuration (host, port, credentials etc.)
+<2> Create a connection factory using that configuration.
+<3> Create a `DatabaseClient` to use that connection factory.
+
 [[r2dbc.exception]]
 = Exception Translation
 
@@ -66,7 +79,7 @@ It considers R2DBC's categorized exception hierarchy to translate these into Spr
 `SqlErrorCodeR2dbcExceptionTranslator` uses specific vendor codes using Spring JDBC's `SQLErrorCodes`.
 It is more precise than the SQLState implementation.
 The error code translations are based on codes held in a JavaBean type class called `SQLErrorCodes`.
-This class is created and populated by an `SQLErrorCodesFactory`, which (as the name suggests) is a factory for creating SQLErrorCodes based on the contents of a configuration file named `sql-error-codes.xml` from Spring's Data Access module.
+Instances of this class are created and populated by an `SQLErrorCodesFactory`, which (as the name suggests) is a factory for creating SQLErrorCodes based on the contents of a configuration file named `sql-error-codes.xml` from Spring's Data Access module.
 This file is populated with vendor codes and based on the `ConnectionFactoryName` taken from `ConnectionFactoryMetadata`.
 The codes for the actual database you are using are used.
 

src/main/java/org/springframework/data/r2dbc/BadSqlGrammarException.java

@@ -31,6 +31,8 @@
  */
 public class BadSqlGrammarException extends InvalidDataAccessResourceUsageException {
 
+	private static final long serialVersionUID = 3814579246913482054L;
+
 	private final String sql;
 
 	/**

src/main/java/org/springframework/data/r2dbc/UncategorizedR2dbcException.java

@@ -27,6 +27,8 @@
  */
 public class UncategorizedR2dbcException extends UncategorizedDataAccessException {
 
+	private static final long serialVersionUID = 361587356435210266L;
+
 	/**
 	 * SQL that led to the problem
 	 */

src/main/java/org/springframework/data/r2dbc/function/DefaultDatabaseClient.java

@@ -45,7 +45,6 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.reactivestreams.Publisher;
-
 import org.springframework.dao.DataAccessException;
 import org.springframework.dao.InvalidDataAccessApiUsageException;
 import org.springframework.data.domain.Pageable;
@@ -232,7 +231,7 @@ protected Connection createConnectionProxy(Connection con) {
 	protected DataAccessException translateException(String task, @Nullable String sql, R2dbcException ex) {
 
 		DataAccessException dae = this.exceptionTranslator.translate(task, sql, ex);
-		return (dae != null ? dae : new UncategorizedR2dbcException(task, sql, ex));
+		return dae != null ? dae : new UncategorizedR2dbcException(task, sql, ex);
 	}
 
 	/**
@@ -267,25 +266,6 @@ protected DefaultGenericExecuteSpec createGenericExecuteSpec(Supplier<String> sq
 		return new DefaultGenericExecuteSpec(sqlSupplier);
 	}
 
-	private static void doBind(Statement statement, Map<String, SettableValue> byName,
-			Map<Integer, SettableValue> byIndex) {
-
-		bindByIndex(statement, byIndex);
-		bindByName(statement, byName);
-	}
-
-	private static void bindByName(Statement statement, Map<String, SettableValue> byName) {
-
-		byName.forEach((name, o) -> {
-
-			if (o.getValue() != null) {
-				statement.bind(name, o.getValue());
-			} else {
-				statement.bindNull(name, o.getType());
-			}
-		});
-	}
-
 	private static void bindByIndex(Statement statement, Map<Integer, SettableValue> byIndex) {
 
 		byIndex.forEach((i, o) -> {
@@ -591,7 +571,7 @@ public DefaultTypedExecuteSpec<T> bindNull(int index, Class<?> type) {
 
 		@Override
 		public DefaultTypedExecuteSpec<T> bind(String name, Object value) {
-			return (DefaultTypedExecuteSpec) super.bind(name, value);
+			return (DefaultTypedExecuteSpec<T>) super.bind(name, value);
 		}
 
 		@Override
@@ -789,14 +769,11 @@ private class DefaultTypedSelectSpec<T> extends DefaultSelectSpecSupport impleme
 			this.mappingFunction = dataAccessStrategy.getRowMapper(typeToRead);
 		}
 
-		DefaultTypedSelectSpec(String table, List<String> projectedFields, Criteria criteria, Sort sort, Pageable page,
-				BiFunction<Row, RowMetadata, T> mappingFunction) {
-			this(table, projectedFields, criteria, sort, page, null, mappingFunction);
-		}
-
 		DefaultTypedSelectSpec(String table, List<String> projectedFields, Criteria criteria, Sort sort, Pageable page,
 				Class<T> typeToRead, BiFunction<Row, RowMetadata, T> mappingFunction) {
+
 			super(table, projectedFields, criteria, sort, page);
+
 			this.typeToRead = typeToRead;
 			this.mappingFunction = mappingFunction;
 		}
@@ -900,13 +877,14 @@ class DefaultGenericInsertSpec<T> implements GenericInsertSpec<T> {
 		private final BiFunction<Row, RowMetadata, T> mappingFunction;
 
 		@Override
-		public GenericInsertSpec value(String field, Object value) {
+		public GenericInsertSpec<T> value(String field, Object value) {
 
 			Assert.notNull(field, "Field must not be null!");
 			Assert.notNull(value,
 					() -> String.format("Value for field %s must not be null. Use nullValue(…) instead.", field));
 
 			Map<String, SettableValue> byName = new LinkedHashMap<>(this.byName);
+
 			if (value instanceof SettableValue) {
 				byName.put(field, (SettableValue) value);
 			} else {
@@ -917,7 +895,7 @@ public GenericInsertSpec value(String field, Object value) {
 		}
 
 		@Override
-		public GenericInsertSpec nullValue(String field) {
+		public GenericInsertSpec<T> nullValue(String field) {
 
 			Assert.notNull(field, "Field must not be null!");
 
@@ -991,6 +969,7 @@ public TypedInsertSpec<T> table(String tableName) {
 		}
 
 		@Override
+		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public InsertSpec using(T objectToInsert) {
 
 			Assert.notNull(objectToInsert, "Object to insert must not be null!");
@@ -1000,6 +979,7 @@ public InsertSpec using(T objectToInsert) {
 		}
 
 		@Override
+		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public InsertSpec using(Publisher<T> objectToInsert) {
 
 			Assert.notNull(objectToInsert, "Publisher to insert must not be null!");
@@ -1417,7 +1397,7 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
 
 			if (method.getName().equals("equals")) {
 				// Only consider equal when proxies are identical.
-				return (proxy == args[0]);
+				return proxy == args[0];
 			} else if (method.getName().equals("hashCode")) {
 				// Use hashCode of PersistenceManager proxy.
 				return System.identityHashCode(proxy);
@@ -1454,6 +1434,8 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
 	@RequiredArgsConstructor
 	static class ConnectionCloseHolder extends AtomicBoolean {
 
+		private static final long serialVersionUID = -8994138383301201380L;
+
 		final Connection connection;
 		final Function<Connection, Publisher<Void>> closeFunction;
 

src/main/java/org/springframework/data/r2dbc/function/DefaultStatementFactory.java

@@ -55,6 +55,7 @@
  * Converter for R2DBC.
  *
  * @author Mark Paluch
+ * @author Oliver Drotbohm
  */
 public class MappingR2dbcConverter extends BasicRelationalConverter implements R2dbcConverter {
 
@@ -176,29 +177,29 @@ private Object getPotentiallyConvertedSimpleRead(@Nullable Object value, @Nullab
 		return getConversionService().convert(value, target);
 	}
 
+	@SuppressWarnings("unchecked")
 	private <S> S readEntityFrom(Row row, PersistentProperty<?> property) {
 
 		String prefix = property.getName() + "_";
 
-		RelationalPersistentEntity<S> entity = (RelationalPersistentEntity<S>) getMappingContext()
-				.getRequiredPersistentEntity(property.getActualType());
+		RelationalPersistentEntity<?> entity = getMappingContext().getRequiredPersistentEntity(property.getActualType());
 
 		if (readFrom(row, entity.getRequiredIdProperty(), prefix) == null) {
 			return null;
 		}
 
-		S instance = createInstance(row, prefix, entity);
+		Object instance = createInstance(row, prefix, entity);
 
-		PersistentPropertyAccessor<S> accessor = entity.getPropertyAccessor(instance);
-		ConvertingPropertyAccessor<S> propertyAccessor = new ConvertingPropertyAccessor<>(accessor, getConversionService());
+		PersistentPropertyAccessor<?> accessor = entity.getPropertyAccessor(instance);
+		ConvertingPropertyAccessor<?> propertyAccessor = new ConvertingPropertyAccessor<>(accessor, getConversionService());
 
 		for (RelationalPersistentProperty p : entity) {
 			if (!entity.isConstructorArgument(property)) {
 				propertyAccessor.setProperty(p, readFrom(row, p, prefix));
 			}
 		}
 
-		return instance;
+		return (S) instance;
 	}
 
 	private <S> S createInstance(Row row, String prefix, RelationalPersistentEntity<S> entity) {
@@ -346,17 +347,16 @@ public <T> BiFunction<Row, RowMetadata, T> populateIdIfNecessary(T object) {
 
 		return (row, metadata) -> {
 
-			PersistentPropertyAccessor propertyAccessor = entity.getPropertyAccessor(object);
+			PersistentPropertyAccessor<?> propertyAccessor = entity.getPropertyAccessor(object);
 			RelationalPersistentProperty idProperty = entity.getRequiredIdProperty();
 
-			if (propertyAccessor.getProperty(idProperty) == null) {
-
-				if (potentiallySetId(row, metadata, propertyAccessor, idProperty)) {
-					return (T) propertyAccessor.getBean();
-				}
+			if (propertyAccessor.getProperty(idProperty) != null) {
+				return object;
 			}
 
-			return object;
+			return potentiallySetId(row, metadata, propertyAccessor, idProperty) //
+					? (T) propertyAccessor.getBean() //
+					: object;
 		};
 	}
 
@@ -376,19 +376,19 @@ private boolean potentiallySetId(Row row, RowMetadata metadata, PersistentProper
 			generatedIdValue = row.get(key);
 		}
 
-		if (generatedIdValue != null) {
-
-			ConversionService conversionService = getConversionService();
-			propertyAccessor.setProperty(idProperty, conversionService.convert(generatedIdValue, idProperty.getType()));
-			return true;
+		if (generatedIdValue == null) {
+			return false;
 		}
 
-		return false;
+		ConversionService conversionService = getConversionService();
+		propertyAccessor.setProperty(idProperty, conversionService.convert(generatedIdValue, idProperty.getType()));
+
+		return true;
 	}
 
 	@SuppressWarnings("unchecked")
 	private <R> RelationalPersistentEntity<R> getRequiredPersistentEntity(Class<R> type) {
-		return (RelationalPersistentEntity) getMappingContext().getRequiredPersistentEntity(type);
+		return (RelationalPersistentEntity<R>) getMappingContext().getRequiredPersistentEntity(type);
 	}
 
 	private static Map<String, ColumnMetadata> createMetadataMap(RowMetadata metadata) {