Skip to content

Commit c34d51e

Browse files
olavloiteolavloite
authored
chore: refactor client side statements to accept the entire parsed statement (#2556)
* chore: refactor client side statements to accept the entire parsed statement Refactor the internal interface of client-side statements so these receive the entire parsed statement, including any query parameters in the statement. This allows us to create client-side statements that actually use the query parameters that have been specified by the user. * chore: simplify test
1 parent 423e1a4 commit c34d51e

12 files changed

+86
-88
lines changed

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatement.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package com.google.cloud.spanner.connection;
1818

1919
import com.google.cloud.spanner.ResultSet;
20+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
2021
import com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType;
2122
import java.util.List;
2223

@@ -63,5 +64,5 @@ interface ClientSideStatement {
6364
* needed for the execution of the {@link ClientSideStatement}.
6465
* @return the result of the execution of the statement.
6566
*/
66-
StatementResult execute(ConnectionStatementExecutor executor, String statement);
67+
StatementResult execute(ConnectionStatementExecutor executor, ParsedStatement statement);
6768
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementExecutor.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package com.google.cloud.spanner.connection;
1818

19+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
20+
1921
/**
2022
* A {@link ClientSideStatementExecutor} is used to compile {@link ClientSideStatement}s from the
2123
* json source file, and to execute these against a {@link Connection} (through a {@link
@@ -29,13 +31,13 @@ interface ClientSideStatementExecutor {
2931
*
3032
* @param connectionExecutor The {@link ConnectionStatementExecutor} to use to execute the
3133
* statement on a {@link Connection}.
32-
* @param sql The sql statement that is executed. This can be used to parse any additional
34+
* @param statement The statement that is executed. This can be used to parse any additional
3335
* arguments that might be needed for the execution of the {@link ClientSideStatementImpl}.
3436
* @return the result of the execution.
3537
* @throws Exception If an error occurs while executing the statement, for example if an invalid
3638
* argument has been specified in the sql statement, or if the statement is invalid for the
3739
* current state of the {@link Connection}.
3840
*/
39-
StatementResult execute(ConnectionStatementExecutor connectionExecutor, String sql)
41+
StatementResult execute(ConnectionStatementExecutor connectionExecutor, ParsedStatement statement)
4042
throws Exception;
4143
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementExplainExecutor.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.cloud.spanner.ErrorCode;
2020
import com.google.cloud.spanner.SpannerExceptionFactory;
21+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
2122
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
2223
import com.google.cloud.spanner.connection.ClientSideStatementValueConverters.ExplainCommandConverter;
2324
import com.google.common.collect.ImmutableSet;
@@ -47,9 +48,10 @@ class ClientSideStatementExplainExecutor implements ClientSideStatementExecutor
4748
}
4849

4950
@Override
50-
public StatementResult execute(ConnectionStatementExecutor connection, String sql)
51+
public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
5152
throws Exception {
52-
return (StatementResult) method.invoke(connection, getParameterValue(sql));
53+
return (StatementResult)
54+
method.invoke(connection, getParameterValue(statement.getSqlWithoutComments()));
5355
}
5456

5557
String getParameterValue(String sql) {

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementImpl.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package com.google.cloud.spanner.connection;
1818

1919
import com.google.cloud.spanner.SpannerException;
20+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
2021
import com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType;
2122
import com.google.cloud.spanner.connection.StatementResult.ResultType;
2223
import com.google.common.base.Preconditions;
@@ -160,7 +161,8 @@ ClientSideStatementImpl compile() throws CompileException {
160161
}
161162

162163
@Override
163-
public StatementResult execute(ConnectionStatementExecutor connection, String statement) {
164+
public StatementResult execute(
165+
ConnectionStatementExecutor connection, ParsedStatement statement) {
164166
Preconditions.checkState(executor != null, "This statement has not been compiled");
165167
try {
166168
return executor.execute(connection, statement);
@@ -170,9 +172,9 @@ public StatementResult execute(ConnectionStatementExecutor connection, String st
170172
if (e.getCause() instanceof SpannerException) {
171173
throw (SpannerException) e.getCause();
172174
}
173-
throw new ExecuteException(e.getCause(), this, statement);
175+
throw new ExecuteException(e.getCause(), this, statement.getStatement().getSql());
174176
} catch (Exception e) {
175-
throw new ExecuteException(e, this, statement);
177+
throw new ExecuteException(e, this, statement.getStatement().getSql());
176178
}
177179
}
178180

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementNoParamExecutor.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.google.cloud.spanner.connection;
1818

19+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
1920
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
2021
import java.lang.reflect.Method;
2122

@@ -42,7 +43,7 @@ class ClientSideStatementNoParamExecutor implements ClientSideStatementExecutor
4243
}
4344

4445
@Override
45-
public StatementResult execute(ConnectionStatementExecutor connection, String statement)
46+
public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
4647
throws Exception {
4748
return (StatementResult) method.invoke(connection);
4849
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementPgBeginExecutor.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.cloud.spanner.ErrorCode;
2020
import com.google.cloud.spanner.SpannerExceptionFactory;
21+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
2122
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
2223
import com.google.cloud.spanner.connection.ClientSideStatementValueConverters.PgTransactionModeConverter;
2324
import java.lang.reflect.Method;
@@ -42,9 +43,10 @@ class ClientSideStatementPgBeginExecutor implements ClientSideStatementExecutor
4243
}
4344

4445
@Override
45-
public StatementResult execute(ConnectionStatementExecutor connection, String sql)
46+
public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
4647
throws Exception {
47-
return (StatementResult) method.invoke(connection, getParameterValue(sql));
48+
return (StatementResult)
49+
method.invoke(connection, getParameterValue(statement.getSqlWithoutComments()));
4850
}
4951

5052
PgTransactionMode getParameterValue(String sql) {

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementSetExecutor.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.cloud.spanner.ErrorCode;
2020
import com.google.cloud.spanner.SpannerExceptionFactory;
21+
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
2122
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
2223
import com.google.common.base.Preconditions;
2324
import java.lang.reflect.Constructor;
@@ -72,9 +73,10 @@ class ClientSideStatementSetExecutor<T> implements ClientSideStatementExecutor {
7273
}
7374

7475
@Override
75-
public StatementResult execute(ConnectionStatementExecutor connection, String sql)
76+
public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
7677
throws Exception {
77-
return (StatementResult) method.invoke(connection, getParameterValue(sql));
78+
return (StatementResult)
79+
method.invoke(connection, getParameterValue(statement.getSqlWithoutComments()));
7880
}
7981

8082
T getParameterValue(String sql) {

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -925,7 +925,7 @@ public StatementResult execute(Statement statement) {
925925
case CLIENT_SIDE:
926926
return parsedStatement
927927
.getClientSideStatement()
928-
.execute(connectionStatementExecutor, parsedStatement.getSqlWithoutComments());
928+
.execute(connectionStatementExecutor, parsedStatement);
929929
case QUERY:
930930
return StatementResultImpl.of(
931931
internalExecuteQuery(CallType.SYNC, parsedStatement, AnalyzeMode.NONE));
@@ -957,7 +957,7 @@ public AsyncStatementResult executeAsync(Statement statement) {
957957
return AsyncStatementResultImpl.of(
958958
parsedStatement
959959
.getClientSideStatement()
960-
.execute(connectionStatementExecutor, parsedStatement.getSqlWithoutComments()),
960+
.execute(connectionStatementExecutor, parsedStatement),
961961
spanner.getAsyncExecutorProvider());
962962
case QUERY:
963963
return AsyncStatementResultImpl.of(
@@ -1010,7 +1010,7 @@ private ResultSet parseAndExecuteQuery(
10101010
case CLIENT_SIDE:
10111011
return parsedStatement
10121012
.getClientSideStatement()
1013-
.execute(connectionStatementExecutor, parsedStatement.getSqlWithoutComments())
1013+
.execute(connectionStatementExecutor, parsedStatement)
10141014
.getResultSet();
10151015
case QUERY:
10161016
return internalExecuteQuery(callType, parsedStatement, analyzeMode, options);
@@ -1050,7 +1050,7 @@ private AsyncResultSet parseAndExecuteQueryAsync(
10501050
return ResultSets.toAsyncResultSet(
10511051
parsedStatement
10521052
.getClientSideStatement()
1053-
.execute(connectionStatementExecutor, parsedStatement.getSqlWithoutComments())
1053+
.execute(connectionStatementExecutor, parsedStatement)
10541054
.getResultSet(),
10551055
spanner.getAsyncExecutorProvider(),
10561056
options);

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/BeginPgTransactionTest.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public void testBeginWithNoOption() {
6565
"start work isolation level serializable")) {
6666
ParsedStatement statement = parser.parse(Statement.of(sql));
6767
assertEquals(sql, StatementType.CLIENT_SIDE, statement.getType());
68-
statement.getClientSideStatement().execute(executor, sql);
68+
statement.getClientSideStatement().execute(executor, statement);
6969

7070
verify(connection, times(index)).beginTransaction();
7171
verify(connection, never()).setTransactionMode(any());
@@ -89,7 +89,7 @@ public void testBeginReadOnly() {
8989
"start work read only")) {
9090
ParsedStatement statement = parser.parse(Statement.of(sql));
9191
assertEquals(sql, StatementType.CLIENT_SIDE, statement.getType());
92-
statement.getClientSideStatement().execute(executor, sql);
92+
statement.getClientSideStatement().execute(executor, statement);
9393

9494
verify(connection, times(index)).beginTransaction();
9595
verify(connection, times(index)).setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION);
@@ -114,7 +114,7 @@ public void testBeginReadWrite() {
114114
"start work read write")) {
115115
ParsedStatement statement = parser.parse(Statement.of(sql));
116116
assertEquals(sql, StatementType.CLIENT_SIDE, statement.getType());
117-
statement.getClientSideStatement().execute(executor, sql);
117+
statement.getClientSideStatement().execute(executor, statement);
118118

119119
verify(connection, times(index)).beginTransaction();
120120
verify(connection, times(index)).setTransactionMode(TransactionMode.READ_WRITE_TRANSACTION);
@@ -140,7 +140,7 @@ public void testBeginReadOnlyWithIsolationLevel() {
140140
"begin read write , \nisolation level default\n\t,read only")) {
141141
ParsedStatement statement = parser.parse(Statement.of(sql));
142142
assertEquals(sql, StatementType.CLIENT_SIDE, statement.getType());
143-
statement.getClientSideStatement().execute(executor, sql);
143+
statement.getClientSideStatement().execute(executor, statement);
144144

145145
verify(connection, times(index)).beginTransaction();
146146
verify(connection, times(index)).setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION);
@@ -173,7 +173,7 @@ public void testBeginWithNotDeferrable() {
173173
"begin not deferrable read write , \nisolation level default\n\t,read only")) {
174174
ParsedStatement statement = parser.parse(Statement.of(sql));
175175
assertEquals(sql, StatementType.CLIENT_SIDE, statement.getType());
176-
statement.getClientSideStatement().execute(executor, sql);
176+
statement.getClientSideStatement().execute(executor, statement);
177177

178178
verify(connection, times(index)).beginTransaction();
179179
verify(connection, times(index)).setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION);

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionStatementWithNoParametersTest.java‎

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,18 @@ public void setup() {
5151
parser = AbstractStatementParser.getInstance(dialect);
5252
}
5353

54+
ParsedStatement parse(String sql) {
55+
return parser.parse(Statement.of(sql));
56+
}
57+
5458
@Test
5559
public void testExecuteGetAutocommit() {
5660
ParsedStatement statement = parser.parse(Statement.of("show variable autocommit"));
5761
ConnectionImpl connection = mock(ConnectionImpl.class);
5862
ConnectionStatementExecutorImpl executor = mock(ConnectionStatementExecutorImpl.class);
5963
when(executor.getConnection()).thenReturn(connection);
6064
when(executor.statementShowAutocommit()).thenCallRealMethod();
61-
statement.getClientSideStatement().execute(executor, "show variable autocommit");
65+
statement.getClientSideStatement().execute(executor, statement);
6266
verify(connection, times(1)).isAutocommit();
6367
}
6468

@@ -70,9 +74,7 @@ public void testExecuteGetReadOnly() {
7074
ConnectionImpl connection = mock(ConnectionImpl.class);
7175
when(connection.getDialect()).thenReturn(dialect);
7276
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
73-
statement
74-
.getClientSideStatement()
75-
.execute(executor, String.format("show variable %sreadonly", getNamespace(dialect)));
77+
statement.getClientSideStatement().execute(executor, statement);
7678
verify(connection, times(1)).isReadOnly();
7779
}
7880

@@ -86,10 +88,7 @@ public void testExecuteGetAutocommitDmlMode() {
8688
when(connection.getDialect()).thenReturn(dialect);
8789
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
8890
when(connection.getAutocommitDmlMode()).thenReturn(AutocommitDmlMode.TRANSACTIONAL);
89-
statement
90-
.getClientSideStatement()
91-
.execute(
92-
executor, String.format("show variable %sautocommit_dml_mode", getNamespace(dialect)));
91+
statement.getClientSideStatement().execute(executor, statement);
9392
verify(connection, times(1)).getAutocommitDmlMode();
9493
}
9594

@@ -102,7 +101,7 @@ public void testExecuteGetStatementTimeout() {
102101
when(executor.statementShowStatementTimeout()).thenCallRealMethod();
103102
when(connection.hasStatementTimeout()).thenReturn(true);
104103
when(connection.getStatementTimeout(TimeUnit.NANOSECONDS)).thenReturn(1L);
105-
statement.getClientSideStatement().execute(executor, "show variable statement_timeout");
104+
statement.getClientSideStatement().execute(executor, statement);
106105
verify(connection, times(2)).getStatementTimeout(TimeUnit.NANOSECONDS);
107106
}
108107

@@ -115,9 +114,7 @@ public void testExecuteGetReadTimestamp() {
115114
when(connection.getDialect()).thenReturn(dialect);
116115
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
117116
when(connection.getReadTimestampOrNull()).thenReturn(Timestamp.now());
118-
statement
119-
.getClientSideStatement()
120-
.execute(executor, String.format("show variable %sread_timestamp", getNamespace(dialect)));
117+
statement.getClientSideStatement().execute(executor, statement);
121118
verify(connection, times(1)).getReadTimestampOrNull();
122119
}
123120

@@ -130,10 +127,7 @@ public void testExecuteGetCommitTimestamp() {
130127
when(connection.getDialect()).thenReturn(dialect);
131128
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
132129
when(connection.getCommitTimestampOrNull()).thenReturn(Timestamp.now());
133-
statement
134-
.getClientSideStatement()
135-
.execute(
136-
executor, String.format("show variable %scommit_timestamp", getNamespace(dialect)));
130+
statement.getClientSideStatement().execute(executor, statement);
137131
verify(connection, times(1)).getCommitTimestampOrNull();
138132
}
139133

@@ -147,10 +141,7 @@ public void testExecuteGetReadOnlyStaleness() {
147141
when(connection.getDialect()).thenReturn(dialect);
148142
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
149143
when(connection.getReadOnlyStaleness()).thenReturn(TimestampBound.strong());
150-
statement
151-
.getClientSideStatement()
152-
.execute(
153-
executor, String.format("show variable %sread_only_staleness", getNamespace(dialect)));
144+
statement.getClientSideStatement().execute(executor, statement);
154145
verify(connection, times(1)).getReadOnlyStaleness();
155146
}
156147

@@ -164,10 +155,7 @@ public void testExecuteGetOptimizerVersion() {
164155
when(connection.getDialect()).thenReturn(dialect);
165156
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
166157
when(connection.getOptimizerVersion()).thenReturn("1");
167-
statement
168-
.getClientSideStatement()
169-
.execute(
170-
executor, String.format("show variable %soptimizer_version", getNamespace(dialect)));
158+
statement.getClientSideStatement().execute(executor, statement);
171159
verify(connection, times(1)).getOptimizerVersion();
172160
}
173161

@@ -182,11 +170,7 @@ public void testExecuteGetOptimizerStatisticsPackage() {
182170
when(connection.getDialect()).thenReturn(dialect);
183171
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
184172
when(connection.getOptimizerStatisticsPackage()).thenReturn("custom-package");
185-
statement
186-
.getClientSideStatement()
187-
.execute(
188-
executor,
189-
String.format("show variable %soptimizer_statistics_package", getNamespace(dialect)));
173+
statement.getClientSideStatement().execute(executor, statement);
190174
verify(connection, times(1)).getOptimizerStatisticsPackage();
191175
}
192176

@@ -196,7 +180,7 @@ public void testExecuteBegin() {
196180
for (String statement : subject.getClientSideStatement().getExampleStatements()) {
197181
ConnectionImpl connection = mock(ConnectionImpl.class);
198182
ConnectionStatementExecutorImpl executor = new ConnectionStatementExecutorImpl(connection);
199-
subject.getClientSideStatement().execute(executor, statement);
183+
subject.getClientSideStatement().execute(executor, parse(statement));
200184
verify(connection, times(1)).beginTransaction();
201185
}
202186
}
@@ -209,7 +193,7 @@ public void testExecuteCommit() {
209193
ConnectionStatementExecutorImpl executor = mock(ConnectionStatementExecutorImpl.class);
210194
when(executor.getConnection()).thenReturn(connection);
211195
when(executor.statementCommit()).thenCallRealMethod();
212-
subject.getClientSideStatement().execute(executor, statement);
196+
subject.getClientSideStatement().execute(executor, parse(statement));
213197
verify(connection, times(1)).commit();
214198
}
215199
}
@@ -222,7 +206,7 @@ public void testExecuteRollback() {
222206
ConnectionStatementExecutorImpl executor = mock(ConnectionStatementExecutorImpl.class);
223207
when(executor.getConnection()).thenReturn(connection);
224208
when(executor.statementRollback()).thenCallRealMethod();
225-
subject.getClientSideStatement().execute(executor, statement);
209+
subject.getClientSideStatement().execute(executor, parse(statement));
226210
verify(connection, times(1)).rollback();
227211
}
228212
}

0 commit comments

Comments
 (0)