JAVA-1569: Allow null to be used in positional and named values in st…

…atements

changelog/README.md

@@ -4,6 +4,7 @@
 
 ### 4.0.0-alpha4 (in progress)
 
+- [bug] JAVA-1569: Allow null to be used in positional and named values in statements
 - [new feature] JAVA-1592: Expose request's total Frame size through API
 - [new feature] JAVA-1829: Add metrics for bytes-sent and bytes-received 
 - [improvement] JAVA-1755: Normalize usage of DEBUG/TRACE log levels

core/src/main/java/com/datastax/oss/driver/api/core/cql/SimpleStatement.java

@@ -20,13 +20,12 @@
 import com.datastax.oss.driver.api.core.DefaultProtocolVersion;
 import com.datastax.oss.driver.api.core.context.DriverContext;
 import com.datastax.oss.driver.api.core.session.Request;
-import com.datastax.oss.driver.internal.core.CqlIdentifiers;
 import com.datastax.oss.driver.internal.core.cql.DefaultSimpleStatement;
 import com.datastax.oss.driver.internal.core.time.ServerSideTimestampGenerator;
 import com.datastax.oss.driver.internal.core.util.Sizes;
 import com.datastax.oss.protocol.internal.PrimitiveSizes;
-import java.util.Arrays;
-import java.util.Collections;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableList;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap;
 import java.util.List;
 import java.util.Map;
 
@@ -60,15 +59,15 @@ public interface SimpleStatement extends BatchableStatement<SimpleStatement> {
   static SimpleStatement newInstance(String cqlQuery) {
     return new DefaultSimpleStatement(
         cqlQuery,
-        Collections.emptyList(),
-        Collections.emptyMap(),
+        NullAllowingImmutableList.of(),
+        NullAllowingImmutableMap.of(),
         null,
         null,
         null,
         null,
         null,
         null,
-        Collections.emptyMap(),
+        NullAllowingImmutableMap.of(),
         null,
         false,
         Long.MIN_VALUE,
@@ -82,15 +81,15 @@ static SimpleStatement newInstance(String cqlQuery) {
   static SimpleStatement newInstance(String cqlQuery, Object... positionalValues) {
     return new DefaultSimpleStatement(
         cqlQuery,
-        Arrays.asList(positionalValues),
-        Collections.emptyMap(),
+        NullAllowingImmutableList.of(positionalValues),
+        NullAllowingImmutableMap.of(),
         null,
         null,
         null,
         null,
         null,
         null,
-        Collections.emptyMap(),
+        NullAllowingImmutableMap.of(),
         null,
         false,
         Long.MIN_VALUE,
@@ -104,15 +103,15 @@ static SimpleStatement newInstance(String cqlQuery, Object... positionalValues)
   static SimpleStatement newInstance(String cqlQuery, Map<String, Object> namedValues) {
     return new DefaultSimpleStatement(
         cqlQuery,
-        Collections.emptyList(),
-        CqlIdentifiers.wrapKeys(namedValues),
+        NullAllowingImmutableList.of(),
+        DefaultSimpleStatement.wrapKeys(namedValues),
         null,
         null,
         null,
         null,
         null,
         null,
-        Collections.emptyMap(),
+        NullAllowingImmutableMap.of(),
         null,
         false,
         Long.MIN_VALUE,
@@ -215,7 +214,7 @@ default SimpleStatement setKeyspace(String newKeyspaceName) {
    * converted on the fly with {@link CqlIdentifier#fromCql(String)}.
    */
   default SimpleStatement setNamedValues(Map<String, Object> newNamedValues) {
-    return setNamedValuesWithIds(CqlIdentifiers.wrapKeys(newNamedValues));
+    return setNamedValuesWithIds(DefaultSimpleStatement.wrapKeys(newNamedValues));
   }
 
   @Override

core/src/main/java/com/datastax/oss/driver/api/core/cql/SimpleStatementBuilder.java

@@ -17,10 +17,9 @@
 
 import com.datastax.oss.driver.api.core.CqlIdentifier;
 import com.datastax.oss.driver.internal.core.cql.DefaultSimpleStatement;
-import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList;
-import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableList;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import net.jcip.annotations.NotThreadSafe;
@@ -31,8 +30,8 @@ public class SimpleStatementBuilder
 
   private String query;
   private CqlIdentifier keyspace;
-  private ImmutableList.Builder<Object> positionalValuesBuilder;
-  private ImmutableMap.Builder<CqlIdentifier, Object> namedValuesBuilder;
+  private NullAllowingImmutableList.Builder<Object> positionalValuesBuilder;
+  private NullAllowingImmutableMap.Builder<CqlIdentifier, Object> namedValuesBuilder;
 
   public SimpleStatementBuilder(String query) {
     this.query = query;
@@ -47,11 +46,14 @@ public SimpleStatementBuilder(SimpleStatement template) {
 
     this.query = template.getQuery();
     if (!template.getPositionalValues().isEmpty()) {
-      this.positionalValuesBuilder = ImmutableList.builder().addAll(template.getPositionalValues());
+      this.positionalValuesBuilder =
+          NullAllowingImmutableList.builder(template.getPositionalValues().size())
+              .addAll(template.getPositionalValues());
     }
     if (!template.getNamedValues().isEmpty()) {
       this.namedValuesBuilder =
-          ImmutableMap.<CqlIdentifier, Object>builder().putAll(template.getNamedValues());
+          NullAllowingImmutableMap.<CqlIdentifier, Object>builder(template.getNamedValues().size())
+              .putAll(template.getNamedValues());
     }
   }
 
@@ -82,7 +84,7 @@ public SimpleStatementBuilder addPositionalValue(Object value) {
           "Can't have both positional and named values in a statement.");
     }
     if (positionalValuesBuilder == null) {
-      positionalValuesBuilder = ImmutableList.builder();
+      positionalValuesBuilder = NullAllowingImmutableList.builder();
     }
     positionalValuesBuilder.add(value);
     return this;
@@ -95,7 +97,7 @@ public SimpleStatementBuilder addPositionalValues(Iterable<Object> values) {
           "Can't have both positional and named values in a statement.");
     }
     if (positionalValuesBuilder == null) {
-      positionalValuesBuilder = ImmutableList.builder();
+      positionalValuesBuilder = NullAllowingImmutableList.builder();
     }
     positionalValuesBuilder.addAll(values);
     return this;
@@ -108,7 +110,7 @@ public SimpleStatementBuilder addPositionalValues(Object... values) {
 
   /** @see SimpleStatement#setPositionalValues(List) */
   public SimpleStatementBuilder clearPositionalValues() {
-    positionalValuesBuilder = null;
+    positionalValuesBuilder = NullAllowingImmutableList.builder();
     return this;
   }
 
@@ -119,7 +121,7 @@ public SimpleStatementBuilder addNamedValue(CqlIdentifier name, Object value) {
           "Can't have both positional and named values in a statement.");
     }
     if (namedValuesBuilder == null) {
-      namedValuesBuilder = ImmutableMap.builder();
+      namedValuesBuilder = NullAllowingImmutableMap.builder();
     }
     namedValuesBuilder.put(name, value);
     return this;
@@ -135,7 +137,7 @@ public SimpleStatementBuilder addNamedValue(String name, Object value) {
 
   /** @see SimpleStatement#setNamedValuesWithIds(Map) */
   public SimpleStatementBuilder clearNamedValues() {
-    namedValuesBuilder = null;
+    namedValuesBuilder = NullAllowingImmutableMap.builder();
     return this;
   }
 
@@ -144,9 +146,9 @@ public SimpleStatement build() {
     return new DefaultSimpleStatement(
         query,
         (positionalValuesBuilder == null)
-            ? Collections.emptyList()
+            ? NullAllowingImmutableList.of()
             : positionalValuesBuilder.build(),
-        (namedValuesBuilder == null) ? Collections.emptyMap() : namedValuesBuilder.build(),
+        (namedValuesBuilder == null) ? NullAllowingImmutableMap.of() : namedValuesBuilder.build(),
         configProfileName,
         configProfile,
         keyspace,

core/src/main/java/com/datastax/oss/driver/api/core/cql/Statement.java

@@ -118,8 +118,13 @@ default T setRoutingKey(ByteBuffer... newRoutingKeyComponents) {
   /**
    * Sets the custom payload to use for execution.
    *
-   * <p>All the driver's built-in implementations are immutable, and return a new instance from this
-   * method. However custom implementations may choose to be mutable and return the same instance.
+   * <p>All the driver's built-in statement implementations are immutable, and return a new instance
+   * from this method. However custom implementations may choose to be mutable and return the same
+   * instance.
+   *
+   * <p>Note that it's your responsibility to provide a thread-safe map. This can be achieved with a
+   * concurrent or immutable implementation, or by making it effectively immutable (meaning that
+   * it's never modified after being set on the statement).
    */
   T setCustomPayload(Map<String, ByteBuffer> newCustomPayload);
 

core/src/main/java/com/datastax/oss/driver/api/core/cql/StatementBuilder.java

@@ -18,7 +18,7 @@
 import com.datastax.oss.driver.api.core.CqlIdentifier;
 import com.datastax.oss.driver.api.core.config.DriverConfigProfile;
 import com.datastax.oss.driver.api.core.metadata.token.Token;
-import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap;
 import java.nio.ByteBuffer;
 import java.util.Collections;
 import java.util.Map;
@@ -42,7 +42,7 @@ public abstract class StatementBuilder<T extends StatementBuilder<T, S>, S exten
   protected CqlIdentifier routingKeyspace;
   protected ByteBuffer routingKey;
   protected Token routingToken;
-  private ImmutableMap.Builder<String, ByteBuffer> customPayloadBuilder;
+  private NullAllowingImmutableMap.Builder<String, ByteBuffer> customPayloadBuilder;
   protected Boolean idempotent;
   protected boolean tracing;
   protected long timestamp = Long.MIN_VALUE;
@@ -56,7 +56,7 @@ protected StatementBuilder(S template) {
     this.configProfileName = template.getConfigProfileName();
     this.configProfile = template.getConfigProfile();
     this.customPayloadBuilder =
-        ImmutableMap.<String, ByteBuffer>builder().putAll(template.getCustomPayload());
+        NullAllowingImmutableMap.<String, ByteBuffer>builder().putAll(template.getCustomPayload());
     this.idempotent = template.isIdempotent();
     this.tracing = template.isTracing();
     this.timestamp = template.getTimestamp();
@@ -105,7 +105,7 @@ public T withRoutingToken(Token routingToken) {
   /** @see Statement#setCustomPayload(Map) */
   public T addCustomPayload(String key, ByteBuffer value) {
     if (customPayloadBuilder == null) {
-      customPayloadBuilder = ImmutableMap.builder();
+      customPayloadBuilder = NullAllowingImmutableMap.builder();
     }
     customPayloadBuilder.put(key, value);
     return self;

core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java

@@ -79,6 +79,8 @@
 import com.datastax.oss.protocol.internal.response.result.Rows;
 import com.datastax.oss.protocol.internal.response.result.RowsMetadata;
 import com.datastax.oss.protocol.internal.util.Bytes;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableList;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -214,11 +216,16 @@ public static List<ByteBuffer> encode(
     if (values.isEmpty()) {
       return Collections.emptyList();
     } else {
-      List<ByteBuffer> encodedValues = new ArrayList<>(values.size());
+      NullAllowingImmutableList.Builder<ByteBuffer> encodedValues =
+          NullAllowingImmutableList.builder(values.size());
       for (Object value : values) {
-        encodedValues.add(encode(value, codecRegistry, protocolVersion));
+        if (value == null) {
+          encodedValues.add(null);
+        } else {
+          encodedValues.add(encode(value, codecRegistry, protocolVersion));
+        }
       }
-      return encodedValues;
+      return encodedValues.build();
     }
   }
 
@@ -229,10 +236,16 @@ public static Map<String, ByteBuffer> encode(
     if (values.isEmpty()) {
       return Collections.emptyMap();
     } else {
-      ImmutableMap.Builder<String, ByteBuffer> encodedValues = ImmutableMap.builder();
+      NullAllowingImmutableMap.Builder<String, ByteBuffer> encodedValues =
+          NullAllowingImmutableMap.builder(values.size());
       for (Map.Entry<CqlIdentifier, Object> entry : values.entrySet()) {
-        encodedValues.put(
-            entry.getKey().asInternal(), encode(entry.getValue(), codecRegistry, protocolVersion));
+        if (entry.getValue() == null) {
+          encodedValues.put(entry.getKey().asInternal(), null);
+        } else {
+          encodedValues.put(
+              entry.getKey().asInternal(),
+              encode(entry.getValue(), codecRegistry, protocolVersion));
+        }
       }
       return encodedValues.build();
     }

core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultSimpleStatement.java

@@ -19,8 +19,8 @@
 import com.datastax.oss.driver.api.core.config.DriverConfigProfile;
 import com.datastax.oss.driver.api.core.cql.SimpleStatement;
 import com.datastax.oss.driver.api.core.metadata.token.Token;
-import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList;
-import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableList;
+import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap;
 import java.nio.ByteBuffer;
 import java.util.List;
 import java.util.Map;
@@ -65,8 +65,8 @@ public DefaultSimpleStatement(
       throw new IllegalArgumentException("Can't have both positional and named values");
     }
     this.query = query;
-    this.positionalValues = ImmutableList.copyOf(positionalValues);
-    this.namedValues = ImmutableMap.copyOf(namedValues);
+    this.positionalValues = NullAllowingImmutableList.copyOf(positionalValues);
+    this.namedValues = NullAllowingImmutableMap.copyOf(namedValues);
     this.configProfileName = configProfileName;
     this.configProfile = configProfile;
     this.keyspace = keyspace;
@@ -415,4 +415,13 @@ public SimpleStatement setPagingState(ByteBuffer newPagingState) {
         timestamp,
         newPagingState);
   }
+
+  public static Map<CqlIdentifier, Object> wrapKeys(Map<String, Object> namedValues) {
+    NullAllowingImmutableMap.Builder<CqlIdentifier, Object> builder =
+        NullAllowingImmutableMap.builder();
+    for (Map.Entry<String, Object> entry : namedValues.entrySet()) {
+      builder.put(CqlIdentifier.fromCql(entry.getKey()), entry.getValue());
+    }
+    return builder.build();
+  }
 }

integration-tests/src/test/java/com/datastax/oss/driver/api/core/cql/SimpleStatementIT.java

@@ -188,6 +188,32 @@ public void should_use_positional_values() {
     assertThat(row.getInt("v")).isEqualTo(4);
   }
 
+  @Test
+  public void should_allow_nulls_in_positional_values() {
+    // given a statement with positional values
+    SimpleStatement insert =
+        SimpleStatement.builder("INSERT into test2 (k, v) values (?, ?)")
+            .addPositionalValue(name.getMethodName())
+            .addPositionalValue(null)
+            .build();
+
+    // when executing that statement
+    cluster.session().execute(insert);
+
+    // then we should be able to retrieve the data as inserted.
+    SimpleStatement select =
+        SimpleStatement.builder("select k,v from test2 where k=?")
+            .addPositionalValue(name.getMethodName())
+            .build();
+
+    ResultSet result = cluster.session().execute(select);
+    assertThat(result.getAvailableWithoutFetching()).isEqualTo(1);
+
+    Row row = result.iterator().next();
+    assertThat(row.getString("k")).isEqualTo(name.getMethodName());
+    assertThat(row.getObject("v")).isNull();
+  }
+
   @Test(expected = InvalidQueryException.class)
   public void should_fail_when_too_many_positional_values_provided() {
     // given a statement with more bound values than anticipated (3 given vs. 2 expected)
@@ -242,6 +268,32 @@ public void should_use_named_values() {
     assertThat(row.getInt("v")).isEqualTo(7);
   }
 
+  @Test
+  public void should_allow_nulls_in_named_values() {
+    // given a statement with named values
+    SimpleStatement insert =
+        SimpleStatement.builder("INSERT into test2 (k, v) values (:k, :v)")
+            .addNamedValue("k", name.getMethodName())
+            .addNamedValue("v", null)
+            .build();
+
+    // when executing that statement
+    cluster.session().execute(insert);
+
+    // then we should be able to retrieve the data as inserted.
+    SimpleStatement select =
+        SimpleStatement.builder("select k,v from test2 where k=:k")
+            .addNamedValue("k", name.getMethodName())
+            .build();
+
+    ResultSet result = cluster.session().execute(select);
+    assertThat(result.getAvailableWithoutFetching()).isEqualTo(1);
+
+    Row row = result.iterator().next();
+    assertThat(row.getString("k")).isEqualTo(name.getMethodName());
+    assertThat(row.getObject("v")).isNull();
+  }
+
   @Test(expected = InvalidQueryException.class)
   public void should_fail_when_named_value_missing() {
     // given a statement with a missing named value (:k)