Skip to content

Commit 7095f94

Browse files
olavloiteolavloite
authored
fix: untyped null parameters would cause NPE (#1680)
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
1 parent 5dc3e19 commit 7095f94

File tree

7 files changed

+86
-19
lines changed

7 files changed

+86
-19
lines changed

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -583,8 +583,10 @@ ExecuteSqlRequest.Builder getExecuteSqlRequestBuilder(
583583
if (!stmtParameters.isEmpty()) {
584584
com.google.protobuf.Struct.Builder paramsBuilder = builder.getParamsBuilder();
585585
for (Map.Entry<String, Value> param : stmtParameters.entrySet()) {
586-
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
587-
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
586+
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
587+
if (param.getValue() != null) {
588+
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
589+
}
588590
}
589591
}
590592
if (withTransactionSelector) {
@@ -612,10 +614,12 @@ ExecuteBatchDmlRequest.Builder getExecuteBatchDmlRequestBuilder(
612614
com.google.protobuf.Struct.Builder paramsBuilder =
613615
builder.getStatementsBuilder(idx).getParamsBuilder();
614616
for (Map.Entry<String, Value> param : stmtParameters.entrySet()) {
615-
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
616-
builder
617-
.getStatementsBuilder(idx)
618-
.putParamTypes(param.getKey(), param.getValue().getType().toProto());
617+
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
618+
if (param.getValue() != null) {
619+
builder
620+
.getStatementsBuilder(idx)
621+
.putParamTypes(param.getKey(), param.getValue().getType().toProto());
622+
}
619623
}
620624
}
621625
idx++;

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,10 @@ public List<Partition> partitionQuery(
169169
if (!stmtParameters.isEmpty()) {
170170
Struct.Builder paramsBuilder = builder.getParamsBuilder();
171171
for (Map.Entry<String, Value> param : stmtParameters.entrySet()) {
172-
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
173-
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
172+
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
173+
if (param.getValue() != null) {
174+
builder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
175+
}
174176
}
175177
}
176178
TransactionSelector selector = getTransactionSelector();

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,8 +217,10 @@ private void setParameters(
217217
if (!statementParameters.isEmpty()) {
218218
com.google.protobuf.Struct.Builder paramsBuilder = requestBuilder.getParamsBuilder();
219219
for (Map.Entry<String, Value> param : statementParameters.entrySet()) {
220-
paramsBuilder.putFields(param.getKey(), param.getValue().toProto());
221-
requestBuilder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
220+
paramsBuilder.putFields(param.getKey(), Value.toProto(param.getValue()));
221+
if (param.getValue() != null) {
222+
requestBuilder.putParamTypes(param.getKey(), param.getValue().getType().toProto());
223+
}
222224
}
223225
}
224226
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/Statement.java

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@
2121

2222
import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode;
2323
import com.google.common.base.Preconditions;
24-
import com.google.common.collect.ImmutableMap;
2524
import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions;
2625
import java.io.Serializable;
26+
import java.util.Collections;
2727
import java.util.HashMap;
2828
import java.util.Map;
2929
import java.util.Objects;
@@ -56,11 +56,11 @@
5656
public final class Statement implements Serializable {
5757
private static final long serialVersionUID = -1967958247625065259L;
5858

59-
private final ImmutableMap<String, Value> parameters;
59+
private final Map<String, Value> parameters;
6060
private final String sql;
6161
private final QueryOptions queryOptions;
6262

63-
private Statement(String sql, ImmutableMap<String, Value> parameters, QueryOptions queryOptions) {
63+
private Statement(String sql, Map<String, Value> parameters, QueryOptions queryOptions) {
6464
this.sql = sql;
6565
this.parameters = parameters;
6666
this.queryOptions = queryOptions;
@@ -112,14 +112,17 @@ public ValueBinder<Builder> bind(String parameter) {
112112
public Statement build() {
113113
checkState(
114114
currentBinding == null, "Binding for parameter '%s' is incomplete.", currentBinding);
115-
return new Statement(sqlBuffer.toString(), ImmutableMap.copyOf(parameters), queryOptions);
115+
return new Statement(
116+
sqlBuffer.toString(),
117+
Collections.unmodifiableMap(new HashMap<>(parameters)),
118+
queryOptions);
116119
}
117120

118121
private class Binder extends ValueBinder<Builder> {
119122
@Override
120123
Builder handle(Value value) {
121124
Preconditions.checkArgument(
122-
!value.isCommitTimestamp(),
125+
value == null || !value.isCommitTimestamp(),
123126
"Mutation.COMMIT_TIMESTAMP cannot be bound as a query parameter");
124127
checkState(currentBinding != null, "No binding in progress");
125128
parameters.put(currentBinding, value);
@@ -218,7 +221,11 @@ StringBuilder toString(StringBuilder b) {
218221
b.append(", ");
219222
}
220223
b.append(parameter.getKey()).append(": ");
221-
parameter.getValue().toString(b);
224+
if (parameter.getValue() == null) {
225+
b.append("NULL");
226+
} else {
227+
parameter.getValue().toString(b);
228+
}
222229
}
223230
b.append("}");
224231
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ public abstract class Value implements Serializable {
7373
*/
7474
public static final Timestamp COMMIT_TIMESTAMP = Timestamp.ofTimeMicroseconds(0L);
7575

76+
static final com.google.protobuf.Value NULL_PROTO =
77+
com.google.protobuf.Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build();
78+
7679
/** Constant to specify a PG Numeric NaN value. */
7780
public static final String NAN = "NaN";
7881

@@ -622,6 +625,10 @@ public String toString() {
622625

623626
// END OF PUBLIC API.
624627

628+
static com.google.protobuf.Value toProto(Value value) {
629+
return value == null ? NULL_PROTO : value.toProto();
630+
}
631+
625632
abstract void toString(StringBuilder b);
626633

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

738745
/** Template class for {@code Value} implementations. */
739746
private abstract static class AbstractValue extends Value {
740-
static final com.google.protobuf.Value NULL_PROTO =
741-
com.google.protobuf.Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build();
742-
743747
private final boolean isNull;
744748
private final Type type;
745749

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/StatementTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ public void serialization() {
6161
.append("bytes_field = @bytes_field ")
6262
.bind("bytes_field")
6363
.to(ByteArray.fromBase64("abcd"))
64+
.bind("untyped_null_field")
65+
.to((Value) null)
6466
.build();
6567
reserializeAndAssert(stmt);
6668
}
@@ -165,6 +167,9 @@ public void equalsAndHashCode() {
165167
tester.addEqualityGroup(Statement.newBuilder("SELECT @x, @y").bind("y").to(2).build());
166168
tester.addEqualityGroup(
167169
Statement.newBuilder("SELECT @x, @y").bind("x").to(1).bind("y").to(2).build());
170+
tester.addEqualityGroup(
171+
Statement.newBuilder("SELECT @x, @y").bind("x").to((Value) null).build(),
172+
Statement.newBuilder("SELECT @x, @y").bind("x").to((Value) null).build());
168173
tester.testEquals();
169174
}
170175
}

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITDMLTest.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,12 @@
1818

1919
import static com.google.cloud.spanner.testing.EmulatorSpannerHelper.isUsingEmulator;
2020
import static com.google.common.truth.Truth.assertThat;
21+
import static org.junit.Assert.assertEquals;
22+
import static org.junit.Assert.assertFalse;
23+
import static org.junit.Assert.assertNotNull;
24+
import static org.junit.Assert.assertTrue;
2125
import static org.junit.Assert.fail;
26+
import static org.junit.Assume.assumeFalse;
2227

2328
import com.google.cloud.spanner.AbortedException;
2429
import com.google.cloud.spanner.Database;
@@ -38,6 +43,7 @@
3843
import com.google.cloud.spanner.TimestampBound;
3944
import com.google.cloud.spanner.TransactionRunner;
4045
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
46+
import com.google.cloud.spanner.Value;
4147
import com.google.cloud.spanner.connection.ConnectionOptions;
4248
import java.util.ArrayList;
4349
import java.util.Arrays;
@@ -380,4 +386,41 @@ public void standardDMLWithExecuteSQL() {
380386
// checks for multi-stmts within a txn, therefore also verifying seqNo.
381387
executeQuery(DML_COUNT * 2, updateDml(), deleteDml());
382388
}
389+
390+
@Test
391+
public void testUntypedNullValues() {
392+
assumeFalse(
393+
"Spanner PostgreSQL does not yet support untyped null values",
394+
dialect.dialect == Dialect.POSTGRESQL);
395+
396+
DatabaseClient client = getClient(dialect.dialect);
397+
String sql;
398+
if (dialect.dialect == Dialect.POSTGRESQL) {
399+
sql = "INSERT INTO T (K, V) VALUES ($1, $2)";
400+
} else {
401+
sql = "INSERT INTO T (K, V) VALUES (@p1, @p2)";
402+
}
403+
Long updateCount =
404+
client
405+
.readWriteTransaction()
406+
.run(
407+
transaction ->
408+
transaction.executeUpdate(
409+
Statement.newBuilder(sql)
410+
.bind("p1")
411+
.to("k1")
412+
.bind("p2")
413+
.to((Value) null)
414+
.build()));
415+
416+
assertNotNull(updateCount);
417+
assertEquals(1L, updateCount.longValue());
418+
419+
// Read the row back and verify that the value is null.
420+
try (ResultSet resultSet = client.singleUse().executeQuery(Statement.of("SELECT V FROM T"))) {
421+
assertTrue(resultSet.next());
422+
assertTrue(resultSet.isNull(0));
423+
assertFalse(resultSet.next());
424+
}
425+
}
383426
}

0 commit comments

Comments
 (0)