Skip to content

Commit

Permalink
fix: untyped null parameters would cause NPE (#1680)
Browse files Browse the repository at this point in the history
Adding an untyped null value as a parameter to a statement was not
possible, as:
1. The parameter collection would allow a null value to be added, but
   when the statement was built, it would throw a NullPointerException
   because it used an ImmutableMap internally, which does not support
   null values.
2. The translation from a hand-written Statement instance to a proto
   Statement instance would fail, as it did not take into account that
   the parameter could be null.

Fixes #1679
  • Loading branch information
olavloite authored Feb 15, 2022
1 parent 5dc3e19 commit 7095f94
Show file tree
Hide file tree
Showing 7 changed files with 86 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,10 @@ ExecuteSqlRequest.Builder getExecuteSqlRequestBuilder(
if (!stmtParameters.isEmpty()) {
com.google.protobuf.Struct.Builder paramsBuilder = builder.getParamsBuilder();
for (Map.Entry<String, Value> param : stmtParameters.entrySet()) {
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
if (param.getValue() != null) {
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
}
}
}
if (withTransactionSelector) {
Expand Down Expand Up @@ -612,10 +614,12 @@ ExecuteBatchDmlRequest.Builder getExecuteBatchDmlRequestBuilder(
com.google.protobuf.Struct.Builder paramsBuilder =
builder.getStatementsBuilder(idx).getParamsBuilder();
for (Map.Entry<String, Value> param : stmtParameters.entrySet()) {
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
builder
.getStatementsBuilder(idx)
.putParamTypes(param.getKey(), param.getValue().getType().toProto());
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
if (param.getValue() != null) {
builder
.getStatementsBuilder(idx)
.putParamTypes(param.getKey(), param.getValue().getType().toProto());
}
}
}
idx++;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,10 @@ public List<Partition> partitionQuery(
if (!stmtParameters.isEmpty()) {
Struct.Builder paramsBuilder = builder.getParamsBuilder();
for (Map.Entry<String, Value> param : stmtParameters.entrySet()) {
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
if (param.getValue() != null) {
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
}
}
}
TransactionSelector selector = getTransactionSelector();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,10 @@ private void setParameters(
if (!statementParameters.isEmpty()) {
com.google.protobuf.Struct.Builder paramsBuilder = requestBuilder.getParamsBuilder();
for (Map.Entry<String, Value> param : statementParameters.entrySet()) {
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
requestBuilder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
if (param.getValue() != null) {
requestBuilder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@

import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions;
import java.io.Serializable;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -56,11 +56,11 @@
public final class Statement implements Serializable {
private static final long serialVersionUID = -1967958247625065259L;

private final ImmutableMap<String, Value> parameters;
private final Map<String, Value> parameters;
private final String sql;
private final QueryOptions queryOptions;

private Statement(String sql, ImmutableMap<String, Value> parameters, QueryOptions queryOptions) {
private Statement(String sql, Map<String, Value> parameters, QueryOptions queryOptions) {
this.sql = sql;
this.parameters = parameters;
this.queryOptions = queryOptions;
Expand Down Expand Up @@ -112,14 +112,17 @@ public ValueBinder<Builder> bind(String parameter) {
public Statement build() {
checkState(
currentBinding == null, "Binding for parameter '%s' is incomplete.", currentBinding);
return new Statement(sqlBuffer.toString(), ImmutableMap.copyOf(parameters), queryOptions);
return new Statement(
sqlBuffer.toString(),
Collections.unmodifiableMap(new HashMap<>(parameters)),
queryOptions);
}

private class Binder extends ValueBinder<Builder> {
@Override
Builder handle(Value value) {
Preconditions.checkArgument(
!value.isCommitTimestamp(),
value == null || !value.isCommitTimestamp(),
"Mutation.COMMIT_TIMESTAMP cannot be bound as a query parameter");
checkState(currentBinding != null, "No binding in progress");
parameters.put(currentBinding, value);
Expand Down Expand Up @@ -218,7 +221,11 @@ StringBuilder toString(StringBuilder b) {
b.append(", ");
}
b.append(parameter.getKey()).append(": ");
parameter.getValue().toString(b);
if (parameter.getValue() == null) {
b.append("NULL");
} else {
parameter.getValue().toString(b);
}
}
b.append("}");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ public abstract class Value implements Serializable {
*/
public static final Timestamp COMMIT_TIMESTAMP = Timestamp.ofTimeMicroseconds(0L);

static final com.google.protobuf.Value NULL_PROTO =
com.google.protobuf.Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build();

/** Constant to specify a PG Numeric NaN value. */
public static final String NAN = "NaN";

Expand Down Expand Up @@ -622,6 +625,10 @@ public String toString() {

// END OF PUBLIC API.

static com.google.protobuf.Value toProto(Value value) {
return value == null ? NULL_PROTO : value.toProto();
}

abstract void toString(StringBuilder b);

abstract com.google.protobuf.Value toProto();
Expand Down Expand Up @@ -737,9 +744,6 @@ Value newValue(boolean isNull, BitSet nulls, boolean[] values) {

/** Template class for {@code Value} implementations. */
private abstract static class AbstractValue extends Value {
static final com.google.protobuf.Value NULL_PROTO =
com.google.protobuf.Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build();

private final boolean isNull;
private final Type type;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ public void serialization() {
.append("bytes_field = @bytes_field ")
.bind("bytes_field")
.to(ByteArray.fromBase64("abcd"))
.bind("untyped_null_field")
.to((Value) null)
.build();
reserializeAndAssert(stmt);
}
Expand Down Expand Up @@ -165,6 +167,9 @@ public void equalsAndHashCode() {
tester.addEqualityGroup(Statement.newBuilder("SELECT @x, @y").bind("y").to(2).build());
tester.addEqualityGroup(
Statement.newBuilder("SELECT @x, @y").bind("x").to(1).bind("y").to(2).build());
tester.addEqualityGroup(
Statement.newBuilder("SELECT @x, @y").bind("x").to((Value) null).build(),
Statement.newBuilder("SELECT @x, @y").bind("x").to((Value) null).build());
tester.testEquals();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@

import static com.google.cloud.spanner.testing.EmulatorSpannerHelper.isUsingEmulator;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeFalse;

import com.google.cloud.spanner.AbortedException;
import com.google.cloud.spanner.Database;
Expand All @@ -38,6 +43,7 @@
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.TransactionRunner;
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
import com.google.cloud.spanner.Value;
import com.google.cloud.spanner.connection.ConnectionOptions;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -380,4 +386,41 @@ public void standardDMLWithExecuteSQL() {
// checks for multi-stmts within a txn, therefore also verifying seqNo.
executeQuery(DML_COUNT * 2, updateDml(), deleteDml());
}

@Test
public void testUntypedNullValues() {
assumeFalse(
"Spanner PostgreSQL does not yet support untyped null values",
dialect.dialect == Dialect.POSTGRESQL);

DatabaseClient client = getClient(dialect.dialect);
String sql;
if (dialect.dialect == Dialect.POSTGRESQL) {
sql = "INSERT INTO T (K, V) VALUES ($1, $2)";
} else {
sql = "INSERT INTO T (K, V) VALUES (@p1, @p2)";
}
Long updateCount =
client
.readWriteTransaction()
.run(
transaction ->
transaction.executeUpdate(
Statement.newBuilder(sql)
.bind("p1")
.to("k1")
.bind("p2")
.to((Value) null)
.build()));

assertNotNull(updateCount);
assertEquals(1L, updateCount.longValue());

// Read the row back and verify that the value is null.
try (ResultSet resultSet = client.singleUse().executeQuery(Statement.of("SELECT V FROM T"))) {
assertTrue(resultSet.next());
assertTrue(resultSet.isNull(0));
assertFalse(resultSet.next());
}
}
}

0 comments on commit 7095f94

Please sign in to comment.