From 55ae7db22d791f883d50ea36960fe66ec825ccad Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 17 Aug 2022 12:42:40 +0000 Subject: [PATCH 01/33] feat: incorporate dml with returning clause --- .../connection/AbstractStatementParser.java | 39 ++- .../spanner/connection/ConnectionImpl.java | 60 ++++- .../connection/PostgreSQLStatementParser.java | 52 ++++ .../connection/ReadWriteTransaction.java | 6 +- .../connection/SingleUseTransaction.java | 37 ++- .../connection/SpannerStatementParser.java | 85 +++++++ .../cloud/spanner/GceTestEnvConfig.java | 6 +- .../cloud/spanner/IntegrationTestEnv.java | 3 +- .../connection/StatementParserTest.java | 150 +++++++++++ .../connection/it/ITDmlReturningTest.java | 233 ++++++++++++++++++ 10 files changed, 653 insertions(+), 18 deletions(-) create mode 100644 google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index b2b94c0213e..a701c4c5237 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -140,6 +140,7 @@ public static class ParsedStatement { private final ClientSideStatementImpl clientSideStatement; private final Statement statement; private final String sqlWithoutComments; + private final boolean returningClause; private static ParsedStatement clientSideStatement( ClientSideStatementImpl clientSideStatement, @@ -155,11 +156,13 @@ private static ParsedStatement ddl(Statement statement, String sqlWithoutComment private static ParsedStatement query( Statement statement, String sqlWithoutComments, QueryOptions defaultQueryOptions) { return new ParsedStatement( - StatementType.QUERY, statement, sqlWithoutComments, defaultQueryOptions); + StatementType.QUERY, statement, sqlWithoutComments, defaultQueryOptions, false); } - private static ParsedStatement update(Statement statement, String sqlWithoutComments) { - return new ParsedStatement(StatementType.UPDATE, statement, sqlWithoutComments); + private static ParsedStatement update( + Statement statement, String sqlWithoutComments, boolean returningClause) { + return new ParsedStatement( + StatementType.UPDATE, statement, sqlWithoutComments, returningClause); } private static ParsedStatement unknown(Statement statement, String sqlWithoutComments) { @@ -176,23 +179,34 @@ private ParsedStatement( this.clientSideStatement = clientSideStatement; this.statement = statement; this.sqlWithoutComments = sqlWithoutComments; + this.returningClause = false; + } + + private ParsedStatement( + StatementType type, + Statement statement, + String sqlWithoutComments, + boolean returningClause) { + this(type, statement, sqlWithoutComments, null, returningClause); } private ParsedStatement(StatementType type, Statement statement, String sqlWithoutComments) { - this(type, statement, sqlWithoutComments, null); + this(type, statement, sqlWithoutComments, null, false); } private ParsedStatement( StatementType type, Statement statement, String sqlWithoutComments, - QueryOptions defaultQueryOptions) { + QueryOptions defaultQueryOptions, + boolean returningClause) { Preconditions.checkNotNull(type); Preconditions.checkNotNull(statement); this.type = type; this.clientSideStatement = null; this.statement = mergeQueryOptions(statement, defaultQueryOptions); this.sqlWithoutComments = sqlWithoutComments; + this.returningClause = returningClause; } @Override @@ -219,6 +233,11 @@ public StatementType getType() { return type; } + @InternalApi + public boolean hasReturningClause() { + return this.returningClause; + } + /** * Returns true if the statement is a query that will return a {@link * com.google.cloud.spanner.ResultSet}. @@ -354,7 +373,7 @@ ParsedStatement parse(Statement statement, QueryOptions defaultQueryOptions) { } else if (isQuery(sql)) { return ParsedStatement.query(statement, sql, defaultQueryOptions); } else if (isUpdateStatement(sql)) { - return ParsedStatement.update(statement, sql); + return ParsedStatement.update(statement, sql, checkReturningClause(sql)); } else if (isDdlStatement(sql)) { return ParsedStatement.ddl(statement, sql); } @@ -521,4 +540,12 @@ static int countOccurrencesOf(char c, String string) { } return res; } + + @InternalApi + abstract boolean checkReturningClauseInternal(String sql); + + @InternalApi + public boolean checkReturningClause(String sql) { + return checkReturningClauseInternal(sql); + } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index f0c772acd9e..270631fa08c 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -853,6 +853,9 @@ public StatementResult execute(Statement statement) { case QUERY: return StatementResultImpl.of(internalExecuteQuery(parsedStatement, AnalyzeMode.NONE)); case UPDATE: + if (parsedStatement.hasReturningClause()) { + return StatementResultImpl.of(internalExecuteQuery(parsedStatement, AnalyzeMode.NONE)); + } return StatementResultImpl.of(get(internalExecuteUpdateAsync(parsedStatement))); case DDL: get(executeDdlAsync(parsedStatement)); @@ -881,6 +884,10 @@ public AsyncStatementResult executeAsync(Statement statement) { return AsyncStatementResultImpl.of( internalExecuteQueryAsync(parsedStatement, AnalyzeMode.NONE)); case UPDATE: + if (parsedStatement.hasReturningClause()) { + return AsyncStatementResultImpl.of( + internalExecuteQueryAsync(parsedStatement, AnalyzeMode.NONE)); + } return AsyncStatementResultImpl.of(internalExecuteUpdateAsync(parsedStatement)); case DDL: return AsyncStatementResultImpl.noResult(executeDdlAsync(parsedStatement)); @@ -918,7 +925,7 @@ private ResultSet parseAndExecuteQuery( Preconditions.checkNotNull(analyzeMode); ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG); ParsedStatement parsedStatement = getStatementParser().parse(query, this.queryOptions); - if (parsedStatement.isQuery()) { + if (parsedStatement.isQuery() || parsedStatement.isUpdate()) { switch (parsedStatement.getType()) { case CLIENT_SIDE: return parsedStatement @@ -928,6 +935,19 @@ private ResultSet parseAndExecuteQuery( case QUERY: return internalExecuteQuery(parsedStatement, analyzeMode, options); case UPDATE: + if (parsedStatement.hasReturningClause()) { + // Cannot execute in read-only mode or in READ_ONLY_TRANSACTION transaction mode. + if (this.isReadOnly() + || (this.isInTransaction() + && this.getTransactionMode() == TransactionMode.READ_ONLY_TRANSACTION)) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.FAILED_PRECONDITION, + "DML statement with returning clause cannot be executed in read-only mode: " + + parsedStatement.getSqlWithoutComments()); + } + return internalExecuteQuery(parsedStatement, analyzeMode, options); + } + case DDL: case UNKNOWN: default: @@ -943,7 +963,7 @@ private AsyncResultSet parseAndExecuteQueryAsync( Preconditions.checkNotNull(query); ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG); ParsedStatement parsedStatement = getStatementParser().parse(query, this.queryOptions); - if (parsedStatement.isQuery()) { + if (parsedStatement.isQuery() || parsedStatement.isUpdate()) { switch (parsedStatement.getType()) { case CLIENT_SIDE: return ResultSets.toAsyncResultSet( @@ -956,6 +976,19 @@ private AsyncResultSet parseAndExecuteQueryAsync( case QUERY: return internalExecuteQueryAsync(parsedStatement, analyzeMode, options); case UPDATE: + if (parsedStatement.hasReturningClause()) { + // Cannot execute in read-only mode or in READ_ONLY_TRANSACTION transaction mode. + if (this.isReadOnly() + || (this.isInTransaction() + && this.getTransactionMode() == TransactionMode.READ_ONLY_TRANSACTION)) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.FAILED_PRECONDITION, + "DML statement with returning clause cannot be executed in read-only mode: " + + parsedStatement.getSqlWithoutComments()); + } + return internalExecuteQueryAsync(parsedStatement, analyzeMode, options); + } + case DDL: case UNKNOWN: default: @@ -974,6 +1007,13 @@ public long executeUpdate(Statement update) { if (parsedStatement.isUpdate()) { switch (parsedStatement.getType()) { case UPDATE: + if (parsedStatement.hasReturningClause()) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.FAILED_PRECONDITION, + "DML statement with returning clause cannot be executed over executeUpdate: " + + parsedStatement.getSqlWithoutComments() + + ". Please use executeQuery instead."); + } return get(internalExecuteUpdateAsync(parsedStatement)); case CLIENT_SIDE: case QUERY: @@ -995,6 +1035,13 @@ public ApiFuture executeUpdateAsync(Statement update) { if (parsedStatement.isUpdate()) { switch (parsedStatement.getType()) { case UPDATE: + if (parsedStatement.hasReturningClause()) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.FAILED_PRECONDITION, + "DML statement with returning clause cannot be executed over executeUpdateAsync: " + + parsedStatement.getSqlWithoutComments() + + ". Please use executeQueryAsync instead."); + } return internalExecuteUpdateAsync(parsedStatement); case CLIENT_SIDE: case QUERY: @@ -1141,8 +1188,9 @@ private ResultSet internalExecuteQuery( final QueryOption... options) { Preconditions.checkArgument( statement.getType() == StatementType.QUERY - || (statement.getType() == StatementType.UPDATE && analyzeMode != AnalyzeMode.NONE), - "Statement must either be a query or a DML mode with analyzeMode!=NONE"); + || (statement.getType() == StatementType.UPDATE + && (analyzeMode != AnalyzeMode.NONE || statement.hasReturningClause())), + "Statement must either be a query or a DML mode with analyzeMode!=NONE or returning clause"); UnitOfWork transaction = getCurrentUnitOfWorkOrStartNewUnitOfWork(); return get( transaction.executeQueryAsync( @@ -1154,7 +1202,9 @@ private AsyncResultSet internalExecuteQueryAsync( final AnalyzeMode analyzeMode, final QueryOption... options) { Preconditions.checkArgument( - statement.getType() == StatementType.QUERY, "Statement must be a query"); + (statement.getType() == StatementType.QUERY) + || (statement.getType() == StatementType.UPDATE && statement.hasReturningClause()), + "Statement must be a query or DML with returning clause."); UnitOfWork transaction = getCurrentUnitOfWorkOrStartNewUnitOfWork(); return ResultSets.toAsyncResultSet( transaction.executeQueryAsync( diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java index 6eba0ce1b29..e767e425f57 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java @@ -25,6 +25,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.Set; +import java.util.regex.Pattern; import javax.annotation.Nullable; @InternalApi @@ -65,6 +66,8 @@ String removeCommentsAndTrimInternal(String sql) { Preconditions.checkNotNull(sql); boolean isInSingleLineComment = false; int multiLineCommentLevel = 0; + boolean whitespaceBeforeOrAfterMultiLineComment = false; + int multiLineCommentStartIdx = -1; StringBuilder res = new StringBuilder(sql.length()); int index = 0; while (index < sql.length()) { @@ -78,6 +81,19 @@ String removeCommentsAndTrimInternal(String sql) { } else if (multiLineCommentLevel > 0) { if (sql.length() > index + 1 && c == ASTERISK && sql.charAt(index + 1) == SLASH) { multiLineCommentLevel--; + if (multiLineCommentLevel == 0) { + if (!whitespaceBeforeOrAfterMultiLineComment && (sql.length() > index + 2)) { + whitespaceBeforeOrAfterMultiLineComment = + Character.isWhitespace(sql.charAt(index + 2)); + } + // If the multiline comment does not have any whitespace before or after it, and it is + // neither at the start nor at the end of SQL string, append an extra space. + if (!whitespaceBeforeOrAfterMultiLineComment + && (multiLineCommentStartIdx != 0) + && (index != sql.length() - 2)) { + res.append(' '); + } + } index++; } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERISK) { multiLineCommentLevel++; @@ -92,6 +108,10 @@ String removeCommentsAndTrimInternal(String sql) { continue; } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERISK) { multiLineCommentLevel++; + if (index >= 1) { + whitespaceBeforeOrAfterMultiLineComment = Character.isWhitespace(sql.charAt(index - 1)); + } + multiLineCommentStartIdx = index; index += 2; continue; } else { @@ -267,4 +287,36 @@ private void appendIfNotNull( result.append(prefix).append(tag).append(suffix); } } + + private boolean isReturning(String sql, int index) { + // RETURNING is a reserved keyword in PG, but requires a + // leading AS to be used as column label, to avoid ambiguity. + // We thus check for cases which do not have a leading AS. + // (https://www.postgresql.org/docs/current/sql-keywords-appendix.html) + return Pattern.compile("[\\s)]RETURNING[\\s(][\\s\\S]*", Pattern.CASE_INSENSITIVE) + .matcher(sql.substring(index)) + .matches() + && !((index >= 3) + && Pattern.compile("[\\s]AS[\\s]RETURNING[\\s(][\\s\\S]*", Pattern.CASE_INSENSITIVE) + .matcher(sql.substring(index - 3)) + .matches()); + } + + @InternalApi + @Override + boolean checkReturningClauseInternal(String rawSql) { + Preconditions.checkNotNull(rawSql); + int index = 0; + String sql = rawSql.replaceAll("\\s+", " "); + boolean hasReturningClause = false; + while (index < sql.length()) { + if (!hasReturningClause && isReturning(sql, index)) { + hasReturningClause = true; + index++; + } else { + index = skip(sql, index, null); + } + } + return hasReturningClause; + } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java index 93d38f17935..ad417731122 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java @@ -45,6 +45,7 @@ import com.google.cloud.spanner.TransactionContext; import com.google.cloud.spanner.TransactionManager; import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement; +import com.google.cloud.spanner.connection.AbstractStatementParser.StatementType; import com.google.cloud.spanner.connection.TransactionRetryListener.RetryResult; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -337,7 +338,10 @@ public ApiFuture executeQueryAsync( final ParsedStatement statement, final AnalyzeMode analyzeMode, final QueryOption... options) { - Preconditions.checkArgument(statement.isQuery(), "Statement is not a query"); + Preconditions.checkArgument( + (statement.getType() == StatementType.QUERY) + || (statement.getType() == StatementType.UPDATE && statement.hasReturningClause()), + "Statement must be a query or DML with returning clause"); checkValidTransaction(); ApiFuture res; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java index 9646c7c9c95..e8af3e9204e 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java @@ -179,12 +179,18 @@ public ApiFuture executeQueryAsync( final QueryOption... options) { Preconditions.checkNotNull(statement); Preconditions.checkArgument( - statement.isQuery() || (statement.isUpdate() && analyzeMode != AnalyzeMode.NONE), + statement.isQuery() + || (statement.isUpdate() + && (analyzeMode != AnalyzeMode.NONE || statement.hasReturningClause())), "The statement must be a query, or the statement must be DML and AnalyzeMode must be PLAN or PROFILE"); checkAndMarkUsed(); - if (statement.isUpdate() && analyzeMode != AnalyzeMode.NONE) { - return analyzeTransactionalUpdateAsync(statement, analyzeMode); + if (statement.isUpdate()) { + if (analyzeMode != AnalyzeMode.NONE) { + return analyzeTransactionalUpdateAsync(statement, analyzeMode); + } + // DML with returning clause. + return executeDmlReturningAsync(statement, options); } final ReadOnlyTransaction currentTransaction = @@ -217,6 +223,31 @@ public ApiFuture executeQueryAsync( return executeStatementAsync(statement, callable, SpannerGrpc.getExecuteStreamingSqlMethod()); } + private ApiFuture executeDmlReturningAsync( + final ParsedStatement update, QueryOption... options) { + Callable callable = + () -> { + try { + writeTransaction = createWriteTransaction(); + ResultSet resultSet = + writeTransaction.run( + transaction -> + DirectExecuteResultSet.ofResultSet( + transaction.executeQuery(update.getStatement(), options))); + state = UnitOfWorkState.COMMITTED; + return resultSet; + } catch (Throwable t) { + state = UnitOfWorkState.COMMIT_FAILED; + throw t; + } + }; + return executeStatementAsync( + update, + callable, + ImmutableList.of( + SpannerGrpc.getExecuteStreamingSqlMethod(), SpannerGrpc.getCommitMethod())); + } + @Override public Timestamp getReadTimestamp() { ConnectionPreconditions.checkState( diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java index f223cbb935a..aecae09ab05 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java @@ -25,6 +25,7 @@ import com.google.common.collect.Sets; import java.util.Collections; import java.util.Set; +import java.util.regex.Pattern; @InternalApi public class SpannerStatementParser extends AbstractStatementParser { @@ -69,6 +70,8 @@ String removeCommentsAndTrimInternal(String sql) { char startQuote = 0; boolean lastCharWasEscapeChar = false; boolean isTripleQuoted = false; + boolean whitespaceBeforeOrAfterMultiLineComment = false; + int multiLineCommentStartIdx = -1; StringBuilder res = new StringBuilder(sql.length()); int index = 0; while (index < sql.length()) { @@ -112,6 +115,17 @@ String removeCommentsAndTrimInternal(String sql) { } else if (isInMultiLineComment) { if (sql.length() > index + 1 && c == ASTERISK && sql.charAt(index + 1) == SLASH) { isInMultiLineComment = false; + if (!whitespaceBeforeOrAfterMultiLineComment && (sql.length() > index + 2)) { + whitespaceBeforeOrAfterMultiLineComment = + Character.isWhitespace(sql.charAt(index + 2)); + } + // If the multiline comment does not have any whitespace before or after it, and it is + // neither at the start nor at the end of SQL string, append an extra space. + if (!whitespaceBeforeOrAfterMultiLineComment + && (multiLineCommentStartIdx != 0) + && (index != sql.length() - 2)) { + res.append(' '); + } index++; } } else { @@ -121,6 +135,11 @@ String removeCommentsAndTrimInternal(String sql) { isInSingleLineComment = true; } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERISK) { isInMultiLineComment = true; + if (index >= 1) { + whitespaceBeforeOrAfterMultiLineComment = + Character.isWhitespace(sql.charAt(index - 1)); + } + multiLineCommentStartIdx = index; index++; } else { if (c == SINGLE_QUOTE || c == DOUBLE_QUOTE || c == BACKTICK_QUOTE) { @@ -250,4 +269,70 @@ ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramCh } return new ParametersInfo(paramIndex - 1, named.toString()); } + + private boolean isReturning(String sql, int index) { + return Pattern.compile("[\\s)]THEN(\\s+)RETURN[\\s(][\\s\\S]*", Pattern.CASE_INSENSITIVE) + .matcher(sql.substring(index)) + .matches(); + } + + @InternalApi + @Override + boolean checkReturningClauseInternal(String rawSql) { + Preconditions.checkNotNull(rawSql); + String sql = rawSql.replaceAll("\\s+", " "); + final char SINGLE_QUOTE = '\''; + final char DOUBLE_QUOTE = '"'; + final char BACKTICK_QUOTE = '`'; + boolean isInQuoted = false; + char startQuote = 0; + boolean lastCharWasEscapeChar = false; + boolean isTripleQuoted = false; + boolean hasReturningClause = false; + for (int index = 0; index < sql.length(); index++) { + char c = sql.charAt(index); + if (isInQuoted) { + if (c == startQuote) { + if (lastCharWasEscapeChar) { + lastCharWasEscapeChar = false; + } else if (isTripleQuoted) { + if (sql.length() > index + 2 + && sql.charAt(index + 1) == startQuote + && sql.charAt(index + 2) == startQuote) { + isInQuoted = false; + startQuote = 0; + isTripleQuoted = false; + } + } else { + isInQuoted = false; + startQuote = 0; + } + } else if (c == '\\') { + lastCharWasEscapeChar = true; + } else { + lastCharWasEscapeChar = false; + } + } else { + if (!hasReturningClause && isReturning(sql, index)) { + hasReturningClause = true; + } else { + if (c == SINGLE_QUOTE || c == DOUBLE_QUOTE || c == BACKTICK_QUOTE) { + isInQuoted = true; + startQuote = c; + // check whether it is a triple-quote + if (sql.length() > index + 2 + && sql.charAt(index + 1) == startQuote + && sql.charAt(index + 2) == startQuote) { + isTripleQuoted = true; + } + } + } + } + } + if (isInQuoted) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); + } + return hasReturningClause; + } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java index ab8e3933f9f..808b3922696 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java @@ -58,8 +58,10 @@ public class GceTestEnvConfig implements TestEnvConfig { private final SpannerOptions options; public GceTestEnvConfig() { - String projectId = System.getProperty(GCE_PROJECT_ID, ""); - String serverUrl = System.getProperty(GCE_SERVER_URL, ""); + // String projectId = System.getProperty(GCE_PROJECT_ID, ""); + String projectId = "span-cloud-testing"; + // String serverUrl = System.getProperty(GCE_SERVER_URL, ""); + String serverUrl = "https://staging-wrenchworks.sandbox.googleapis.com"; String credentialsFile = System.getProperty(GCE_CREDENTIALS_FILE, ""); double errorProbability = Double.parseDouble(System.getProperty(GCE_STREAM_BROKEN_PROBABILITY, "0.0")); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java index 9161b1da568..aff5a1c2488 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java @@ -82,7 +82,8 @@ protected void before() throws Throwable { this.config.setUp(); SpannerOptions options = config.spannerOptions(); - String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); + // String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); + String instanceProperty = "projects/span-cloud-testing/instances/rajatrb-test-instance"; InstanceId instanceId; if (!instanceProperty.isEmpty()) { instanceId = InstanceId.of(instanceProperty); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index 2e39025bf33..b135fb354cf 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -1266,6 +1266,156 @@ public void testPostgreSQLGetQueryParameters() { parser.getQueryParameters("select '$2' from foo where bar=$1 and baz=$foo")); } + @Test + public void testGoogleSQLReturningClause() { + assumeTrue(dialect == Dialect.GOOGLE_STANDARD_SQL); + + SpannerStatementParser parser = (SpannerStatementParser) this.parser; + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) then return *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) then\nreturn *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)\nthen\n\n\nreturn\n*")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)then return *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) then return(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)then return(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) then/*comment*/return *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) then return /*then return*/ *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)then/*comment*/return *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)then/*comment*/return(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)then /*comment*/return(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse( + Statement.of("insert into x (a,b) values (1,2)/*comment*/then/*comment*/return(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse( + Statement.of( + "insert into x (a,b) values (1,2)/*comment*/then/*comment*/return/*comment*/(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse( + Statement.of( + "insert into x (a,b) values (1,2)/*comment" + + "*/then" + + "/*comment" + + "*/return/*" + + "comment*/(a)")) + .hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) returning (a)")) + .hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) /*then return **/")) + .hasReturningClause()); + assertFalse( + parser.parse(Statement.of("insert into x (a,b) values (1,2)")).hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)thenreturn*")) + .hasReturningClause()); + } + + @Test + public void testPostgreSQLReturningClause() { + assumeTrue(dialect == Dialect.POSTGRESQL); + + PostgreSQLStatementParser parser = (PostgreSQLStatementParser) this.parser; + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) returning *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)returning *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) returning(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)returning(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x (a,b) values (1,2)/*comment*/returning(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse( + Statement.of("insert into x (a,b) values (1,2)/*comment*/returning/*comment*/(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse( + Statement.of( + "insert into x (a,b) values (1,2)/*comment" + "*/returning/*" + "comment*/(a)")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into t1 select 1 as returning returning *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into t1 select 1/*as /*returning*/ returning*/returning *")) + .hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("insert into x (a,b) values (1,2) then return (a)")) + .hasReturningClause()); + assertFalse( + parser.parse(Statement.of("insert into x (a,b) values (1,2)")).hasReturningClause()); + assertFalse( + parser.parse(Statement.of("insert into t1 select 1 as returning")).hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("insert into t1 select 1\nas\n\nreturning")) + .hasReturningClause()); + assertFalse( + parser.parse(Statement.of("insert into t1 select 1asreturning")).hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into t1 select 1 as/*eomment*/returning returning *")) + .hasReturningClause()); + } + private void assertUnclosedLiteral(String sql) { try { parser.convertPositionalParametersToNamedParameters('?', sql); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java new file mode 100644 index 00000000000..247917f55e0 --- /dev/null +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -0,0 +1,233 @@ +/* + * Copyright 2022 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.spanner.connection.it; + +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 com.google.cloud.spanner.AsyncResultSet; +import com.google.cloud.spanner.ErrorCode; +import com.google.cloud.spanner.ParallelIntegrationTest; +import com.google.cloud.spanner.ResultSet; +import com.google.cloud.spanner.SpannerException; +import com.google.cloud.spanner.Statement; +import com.google.cloud.spanner.connection.AsyncStatementResult; +import com.google.cloud.spanner.connection.Connection; +import com.google.cloud.spanner.connection.ITAbstractSpannerTest; +import com.google.cloud.spanner.connection.StatementResult; +import com.google.cloud.spanner.connection.StatementResult.ResultType; +import com.google.cloud.spanner.connection.TransactionMode; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Execute DML Returning statements using the generic connection API. */ +@Category(ParallelIntegrationTest.class) +@RunWith(JUnit4.class) +public class ITDmlReturningTest extends ITAbstractSpannerTest { + private final String UPDATE = + "UPDATE Singers SET LastName = \"XYZ\" WHERE FirstName = \"ABC\" THEN RETURN *"; + private static boolean initialized = false; + + @Before + public void setupTable() { + if (!initialized) { + try (Connection connection = createConnection()) { + if (!tableExists(connection, "Singers")) { + connection.execute( + Statement.of( + "CREATE TABLE Singers (" + + " SingerId INT64 NOT NULL," + + " FirstName STRING(1024)," + + " LastName STRING(1024)" + + ") PRIMARY KEY(SingerId)")); + connection.execute( + Statement.of( + "INSERT INTO SINGERS " + + "(SINGERID, FIRSTNAME, LASTNAME) VALUES " + + "(1, \"ABC\", \"XYZ\"), " + + "(2, \"ABC\", \"DEF\"), " + + "(3, \"DEF\", \"XYZ\"), " + + "(4, \"PQR\", \"ABC\"), " + + "(5, \"ABC\", \"GHI\")")); + } + } + initialized = true; + } + } + + @Test + public void testDmlReturningExecuteQuery() { + try (Connection connection = createConnection()) { + ResultSet rs = connection.executeQuery(Statement.of(UPDATE)); + assertEquals(rs.getColumnCount(), 3); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertFalse(rs.next()); + assertNotNull(rs.getStats()); + assertEquals(rs.getStats().getRowCountExact(), 3); + } + } + + @Test + public void testDmlReturningExecuteQueryAsync() { + try (Connection connection = createConnection()) { + AsyncResultSet rs = connection.executeQueryAsync(Statement.of(UPDATE)); + assertTrue(rs.next()); + assertEquals(rs.getColumnCount(), 3); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertFalse(rs.next()); + assertNotNull(rs.getStats()); + assertEquals(rs.getStats().getRowCountExact(), 3); + } + } + + @Test + public void testDmlReturningExecuteUpdate() { + try (Connection connection = createConnection()) { + connection.setAutocommit(false); + try { + connection.executeUpdate(Statement.of(UPDATE)); + fail("missing exception"); + } catch (SpannerException e) { + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); + } + } + } + + @Test + public void testDmlReturningExecuteUpdateAsync() { + try (Connection connection = createConnection()) { + connection.setAutocommit(false); + try { + connection.executeUpdateAsync(Statement.of(UPDATE)); + fail("missing exception"); + } catch (SpannerException e) { + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); + } + } + } + + @Test + public void testDmlReturningExecute() { + try (Connection connection = createConnection()) { + connection.setAutocommit(false); + StatementResult res = connection.execute(Statement.of(UPDATE)); + assertEquals(res.getResultType(), ResultType.RESULT_SET); + ResultSet rs = res.getResultSet(); + assertEquals(rs.getColumnCount(), 3); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertFalse(rs.next()); + assertNotNull(rs.getStats()); + assertEquals(rs.getStats().getRowCountExact(), 3); + } + } + + @Test + public void testDmlReturningExecuteAsync() { + try (Connection connection = createConnection()) { + connection.setAutocommit(false); + AsyncStatementResult res = connection.executeAsync(Statement.of(UPDATE)); + assertEquals(res.getResultType(), ResultType.RESULT_SET); + AsyncResultSet rs = res.getResultSetAsync(); + assertTrue(rs.next()); + assertEquals(rs.getColumnCount(), 3); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertFalse(rs.next()); + assertNotNull(rs.getStats()); + assertEquals(rs.getStats().getRowCountExact(), 3); + } + } + + @Test + public void testDmlReturningExecuteQueryReadOnlyMode() { + try (Connection connection = createConnection()) { + connection.setReadOnly(true); + try { + connection.executeQuery(Statement.of(UPDATE)); + fail("missing exception"); + } catch (SpannerException e) { + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); + } + } + } + + @Test + public void testDmlReturningExecuteQueryReadOnlyTransaction() { + try (Connection connection = createConnection()) { + connection.setReadOnly(false); + connection.setAutocommit(false); + connection.setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION); + try { + connection.executeQuery(Statement.of(UPDATE)); + fail("missing exception"); + } catch (SpannerException e) { + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); + } + } + } + + @Test + public void testDmlReturningExecuteQueryAsyncReadOnlyMode() { + try (Connection connection = createConnection()) { + connection.setReadOnly(true); + try { + connection.executeQueryAsync(Statement.of(UPDATE)); + fail("missing exception"); + } catch (SpannerException e) { + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); + } + } + } + + @Test + public void testDmlReturningExecuteQueryAsyncReadOnlyTransaction() { + try (Connection connection = createConnection()) { + connection.setReadOnly(false); + connection.setAutocommit(false); + connection.setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION); + try { + connection.executeQueryAsync(Statement.of(UPDATE)); + fail("missing exception"); + } catch (SpannerException e) { + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); + } + } + } +} From f20d02f18b001b5d7b2ffc4e89eae09ece8a77d2 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 17 Aug 2022 12:57:48 +0000 Subject: [PATCH 02/33] feat: changes --- .../cloud/spanner/connection/AbstractStatementParser.java | 1 + .../google/cloud/spanner/connection/SingleUseTransaction.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index a701c4c5237..067d1c9f87c 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -233,6 +233,7 @@ public StatementType getType() { return type; } + /** Returns whether the statement has a returning clause or not. **/ @InternalApi public boolean hasReturningClause() { return this.returningClause; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java index e8af3e9204e..b2a330a32be 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java @@ -245,7 +245,7 @@ private ApiFuture executeDmlReturningAsync( update, callable, ImmutableList.of( - SpannerGrpc.getExecuteStreamingSqlMethod(), SpannerGrpc.getCommitMethod())); + SpannerGrpc.getExecuteSqlMethod(), SpannerGrpc.getCommitMethod())); } @Override From e06d436f9ec1af6380fc0a4aa524f2c1f609312a Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 17 Aug 2022 13:19:35 +0000 Subject: [PATCH 03/33] feat: change handling of AsyncResultSet. --- .../connection/it/ITDmlReturningTest.java | 73 ++++++++++++++----- 1 file changed, 53 insertions(+), 20 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index 247917f55e0..cc3f2c7b4de 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -23,6 +23,8 @@ import static org.junit.Assert.fail; import com.google.cloud.spanner.AsyncResultSet; +import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; +import com.google.cloud.spanner.AsyncResultSet.ReadyCallback; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ParallelIntegrationTest; import com.google.cloud.spanner.ResultSet; @@ -34,6 +36,7 @@ import com.google.cloud.spanner.connection.StatementResult; import com.google.cloud.spanner.connection.StatementResult.ResultType; import com.google.cloud.spanner.connection.TransactionMode; +import java.util.concurrent.Executors; import org.junit.Before; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -96,16 +99,31 @@ public void testDmlReturningExecuteQuery() { public void testDmlReturningExecuteQueryAsync() { try (Connection connection = createConnection()) { AsyncResultSet rs = connection.executeQueryAsync(Statement.of(UPDATE)); - assertTrue(rs.next()); - assertEquals(rs.getColumnCount(), 3); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertFalse(rs.next()); - assertNotNull(rs.getStats()); - assertEquals(rs.getStats().getRowCountExact(), 3); + rs.setCallback( + Executors.newSingleThreadExecutor(), + resultSet -> { + try { + while (true) { + switch (resultSet.tryNext()) { + case OK: + assertEquals(resultSet.getColumnCount(), 3); + assertEquals(resultSet.getString(1), "ABC"); + break; + case DONE: + assertNotNull(resultSet.getStats()); + assertEquals(resultSet.getStats().getRowCountExact(), 3); + return CallbackResponse.DONE; + case NOT_READY: + return CallbackResponse.CONTINUE; + default: + throw new IllegalStateException(); + } + } + } catch (SpannerException e) { + System.out.printf("Error in callback: %s%n", e.getMessage()); + return CallbackResponse.DONE; + } + }); } } @@ -162,16 +180,31 @@ public void testDmlReturningExecuteAsync() { AsyncStatementResult res = connection.executeAsync(Statement.of(UPDATE)); assertEquals(res.getResultType(), ResultType.RESULT_SET); AsyncResultSet rs = res.getResultSetAsync(); - assertTrue(rs.next()); - assertEquals(rs.getColumnCount(), 3); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertFalse(rs.next()); - assertNotNull(rs.getStats()); - assertEquals(rs.getStats().getRowCountExact(), 3); + rs.setCallback( + Executors.newSingleThreadExecutor(), + resultSet -> { + try { + while (true) { + switch (resultSet.tryNext()) { + case OK: + assertEquals(resultSet.getColumnCount(), 3); + assertEquals(resultSet.getString(1), "ABC"); + break; + case DONE: + assertNotNull(resultSet.getStats()); + assertEquals(resultSet.getStats().getRowCountExact(), 3); + return CallbackResponse.DONE; + case NOT_READY: + return CallbackResponse.CONTINUE; + default: + throw new IllegalStateException(); + } + } + } catch (SpannerException e) { + System.out.printf("Error in callback: %s%n", e.getMessage()); + return CallbackResponse.DONE; + } + }); } } From bfbacfa2a1cdcc669e2ef89bc8111b4dc7856935 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 17 Aug 2022 13:23:48 +0000 Subject: [PATCH 04/33] fix: lint --- .../cloud/spanner/connection/AbstractStatementParser.java | 2 +- .../google/cloud/spanner/connection/SingleUseTransaction.java | 3 +-- .../google/cloud/spanner/connection/it/ITDmlReturningTest.java | 1 - 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index 067d1c9f87c..36b21926eaf 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -233,7 +233,7 @@ public StatementType getType() { return type; } - /** Returns whether the statement has a returning clause or not. **/ + /** Returns whether the statement has a returning clause or not. * */ @InternalApi public boolean hasReturningClause() { return this.returningClause; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java index b2a330a32be..cbde67393aa 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java @@ -244,8 +244,7 @@ private ApiFuture executeDmlReturningAsync( return executeStatementAsync( update, callable, - ImmutableList.of( - SpannerGrpc.getExecuteSqlMethod(), SpannerGrpc.getCommitMethod())); + ImmutableList.of(SpannerGrpc.getExecuteSqlMethod(), SpannerGrpc.getCommitMethod())); } @Override diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index cc3f2c7b4de..0cbfb667f9b 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -24,7 +24,6 @@ import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; -import com.google.cloud.spanner.AsyncResultSet.ReadyCallback; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ParallelIntegrationTest; import com.google.cloud.spanner.ResultSet; From 3807e5fe48cfb690e2f21b507d1965091caff684 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 17 Aug 2022 13:39:33 +0000 Subject: [PATCH 05/33] doc: add comments --- .../cloud/spanner/connection/AbstractStatementParser.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index 36b21926eaf..b140b2a1ec7 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -542,6 +542,14 @@ static int countOccurrencesOf(char c, String string) { return res; } + /** + * Checks if the given SQL string contains a Returning clause. This method is used only in case + * of a DML statement. + * + * @param sql The sql string without comments that has to be evaluated. + * @return A boolean indicating whether the sql string has a Returning clause or not. + * @throws SpannerException If the input sql string contains an unclosed string/byte literal. + */ @InternalApi abstract boolean checkReturningClauseInternal(String sql); From 595ee17f8f873b50d7a14b242bbaa253ab2269fe Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 17 Aug 2022 13:42:16 +0000 Subject: [PATCH 06/33] fix: lint --- .../cloud/spanner/connection/AbstractStatementParser.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index b140b2a1ec7..0f9c0f50601 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -543,8 +543,8 @@ static int countOccurrencesOf(char c, String string) { } /** - * Checks if the given SQL string contains a Returning clause. This method is used only in case - * of a DML statement. + * Checks if the given SQL string contains a Returning clause. This method is used only in case of + * a DML statement. * * @param sql The sql string without comments that has to be evaluated. * @return A boolean indicating whether the sql string has a Returning clause or not. From f1326ff82b6d0965b64da62d4ef1827c81ce58dc Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Fri, 19 Aug 2022 04:37:59 +0000 Subject: [PATCH 07/33] test: add tests for executeBatchUpdate --- .../connection/it/ITDmlReturningTest.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index 0cbfb667f9b..ac4d65b672f 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -16,6 +16,7 @@ package com.google.cloud.spanner.connection.it; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -24,6 +25,7 @@ import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; +qimport com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ParallelIntegrationTest; import com.google.cloud.spanner.ResultSet; @@ -35,6 +37,8 @@ import com.google.cloud.spanner.connection.StatementResult; import com.google.cloud.spanner.connection.StatementResult.ResultType; import com.google.cloud.spanner.connection.TransactionMode; +import com.google.common.collect.ImmutableList; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import org.junit.Before; import org.junit.Test; @@ -152,6 +156,30 @@ public void testDmlReturningExecuteUpdateAsync() { } } + @Test + public void testDmlReturningExecuteBatchUpdate() { + try (Connection connection = createConnection()) { + connection.setAutocommit(false); + final Statement UPDATE_STMT = Statement.of(UPDATE); + long[] counts = + connection.executeBatchUpdate(ImmutableList.of(UPDATE_STMT, UPDATE_STMT, UPDATE_STMT)); + assertArrayEquals(counts, new long[] {3, 3, 3}); + } + } + + @Test + public void testDmlReturningExecuteBatchUpdateAsync() { + try (Connection connection = createConnection()) { + connection.setAutocommit(false); + final Statement UPDATE_STMT = Statement.of(UPDATE); + long[] counts = + connection.executeBatchUpdateAsync(ImmutableList.of(UPDATE_STMT, UPDATE_STMT, UPDATE_STMT)).get(); + assertArrayEquals(counts, new long[] {3, 3, 3}); + } catch (ExecutionException | InterruptedException e) { + // ignore + } + } + @Test public void testDmlReturningExecute() { try (Connection connection = createConnection()) { From 3f069bdfb098059cd9de545f1c3004aac9fba217 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Fri, 19 Aug 2022 04:59:36 +0000 Subject: [PATCH 08/33] test: import fix --- .../google/cloud/spanner/connection/it/ITDmlReturningTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index ac4d65b672f..97e6f719c12 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -25,7 +25,7 @@ import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; -qimport com.google.cloud.spanner.ErrorCode; +import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ParallelIntegrationTest; import com.google.cloud.spanner.ResultSet; From ba692dc6d967407fba095136f2ab0d184e756050 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Fri, 19 Aug 2022 05:00:20 +0000 Subject: [PATCH 09/33] test: remove redundant import --- .../google/cloud/spanner/connection/it/ITDmlReturningTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index 97e6f719c12..79b8252a626 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -26,7 +26,6 @@ import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; import com.google.cloud.spanner.ErrorCode; -import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ParallelIntegrationTest; import com.google.cloud.spanner.ResultSet; import com.google.cloud.spanner.SpannerException; From ff3c0ff5658cdb2308795f5dd0de0d541996116a Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Fri, 19 Aug 2022 09:25:40 +0000 Subject: [PATCH 10/33] test: add abort tests for dml returning --- .../cloud/spanner/MockSpannerServiceImpl.java | 24 ++- .../cloud/spanner/connection/AbortedTest.java | 194 ++++++++++++------ .../connection/AbstractMockServerTest.java | 13 ++ 3 files changed, 160 insertions(+), 71 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java index 44942bb566c..6ff2b7c98d2 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java @@ -270,6 +270,14 @@ public static StatementResult update(Statement statement, long updateCount) { return new StatementResult(statement, updateCount); } + /** + * Creates a {@link StatementResult} for a DML statement with returning clause that returns a + * ResultSet. + */ + public static StatementResult updateReturning(Statement statement, ResultSet resultSet) { + return new StatementResult(statement, resultSet); + } + /** Creates a {@link StatementResult} for statement that should return an error. */ public static StatementResult exception(Statement statement, StatusRuntimeException exception) { return new StatementResult(statement, exception); @@ -1093,9 +1101,6 @@ public void executeBatchDml( .build(); break resultLoop; case RESULT_SET: - throw Status.INVALID_ARGUMENT - .withDescription("Not a DML statement: " + statement.getSql()) - .asRuntimeException(); case UPDATE_COUNT: results.add(res); break; @@ -1120,10 +1125,21 @@ public void executeBatchDml( } ExecuteBatchDmlResponse.Builder builder = ExecuteBatchDmlResponse.newBuilder(); for (StatementResult res : results) { + Long updateCount; + switch (res.getType()) { + case UPDATE_COUNT: + updateCount = res.getUpdateCount(); + break; + case RESULT_SET: + updateCount = res.getResultSet().getStats().getRowCountExact(); + break; + default: + throw new IllegalStateException("Invalid result type: " + res.getType()); + } builder.addResultSets( ResultSet.newBuilder() .setStats( - ResultSetStats.newBuilder().setRowCountExact(res.getUpdateCount()).build()) + ResultSetStats.newBuilder().setRowCountExact(updateCount).build()) .setMetadata( ResultSetMetadata.newBuilder() .setTransaction( diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java index 274f6d27304..3980c8e9c75 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java @@ -86,6 +86,46 @@ public void testCommitAborted() { } } + @Test + public void testCommitAbortedDuringUpdateWithReturning() { + // Do two iterations to ensure that each iteration gets its own transaction, and that each + // transaction is the most recent transaction of that session. + for (int i = 0; i < 2; i++) { + mockSpanner.putStatementResult( + StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_BEFORE_INSERT)); + mockSpanner.putStatementResult( + StatementResult.updateReturning(INSERT_RETURNING_STATEMENT, UPDATE_RETURNING_RESULTSET)); + AbortInterceptor interceptor = new AbortInterceptor(0); + try (ITConnection connection = + createConnection(interceptor, new CountTransactionRetryListener())) { + // verify that the there is no test record + try (ResultSet rs = + connection.executeQuery(Statement.of("SELECT COUNT(*) AS C FROM TEST WHERE ID=1"))) { + assertThat(rs.next(), is(true)); + assertThat(rs.getLong("C"), is(equalTo(0L))); + assertThat(rs.next(), is(false)); + } + // do an insert with returning + connection.executeQuery( + Statement.of("INSERT INTO TEST (ID, NAME) VALUES (1, 'test aborted') THEN RETURN *")); + // indicate that the next statement should abort + interceptor.setProbability(1.0); + interceptor.setOnlyInjectOnce(true); + // do a commit that will first abort, and then on retry will succeed + connection.commit(); + mockSpanner.putStatementResult( + StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_AFTER_INSERT)); + // verify that the insert succeeded + try (ResultSet rs = + connection.executeQuery(Statement.of("SELECT COUNT(*) AS C FROM TEST WHERE ID=1"))) { + assertThat(rs.next(), is(true)); + assertThat(rs.getLong("C"), is(equalTo(1L))); + assertThat(rs.next(), is(false)); + } + } + } + } + @Test public void testAbortedDuringRetryOfFailedQuery() { final Statement invalidStatement = Statement.of("SELECT * FROM FOO"); @@ -115,53 +155,61 @@ public void testAbortedDuringRetryOfFailedQuery() { @Test public void testAbortedDuringRetryOfFailedUpdate() { - final Statement invalidStatement = Statement.of("INSERT INTO FOO"); + final Iterable invalidStatements = ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { - connection.execute(INSERT_STATEMENT); - try { - connection.execute(invalidStatement); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + for (Statement invalidStatement : invalidStatements) { + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + connection.execute(INSERT_STATEMENT); + try { + connection.execute(invalidStatement); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + // The transaction will be executed 3 times, which means that there will be 6 + // ExecuteSqlRequests: + // 1. The initial attempt. + // 2. The first retry attempt. This will fail on the invalid statement as it is aborted. + // 3. the second retry attempt. This will succeed. + assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(6); + mockSpanner.clearRequests(); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - // The transaction will be executed 3 times, which means that there will be 6 - // ExecuteSqlRequests: - // 1. The initial attempt. - // 2. The first retry attempt. This will fail on the invalid statement as it is aborted. - // 3. the second retry attempt. This will succeed. - assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(6); } @Test public void testAbortedDuringRetryOfFailedBatchUpdate() { - final Statement invalidStatement = Statement.of("INSERT INTO FOO"); + final Iterable invalidStatements = ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { - connection.execute(INSERT_STATEMENT); - try { - connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + for (Statement invalidStatement : invalidStatements) { + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + connection.execute(INSERT_STATEMENT); + try { + connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(3); + mockSpanner.clearRequests(); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(3); } @Test @@ -201,48 +249,56 @@ public void testAbortedDuringRetryOfFailedQueryAsFirstStatement() { @Test public void testAbortedDuringRetryOfFailedUpdateAsFirstStatement() { - final Statement invalidStatement = Statement.of("INSERT INTO FOO"); + final Iterable invalidStatements = ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortRetryListener(2, invalidStatement, notFound))) { - try { - connection.execute(invalidStatement); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + for (Statement invalidStatement : invalidStatements) { + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortRetryListener(2, invalidStatement, notFound))) { + try { + connection.execute(invalidStatement); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + connection.execute(INSERT_STATEMENT); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } - connection.execute(INSERT_STATEMENT); - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(8); + mockSpanner.clearRequests(); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(8); } @Test public void testAbortedDuringRetryOfFailedBatchUpdateAsFirstStatement() { - final Statement invalidStatement = Statement.of("INSERT INTO FOO"); + final Iterable invalidStatements = ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { - try { - connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + for (Statement invalidStatement : invalidStatements) { + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + try { + connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + connection.execute(INSERT_STATEMENT); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } - connection.execute(INSERT_STATEMENT); - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(6); + mockSpanner.clearRequests(); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(6); } @Test @@ -250,14 +306,18 @@ public void testRetryUsesTags() { mockSpanner.putStatementResult( StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_BEFORE_INSERT)); mockSpanner.putStatementResult(StatementResult.update(INSERT_STATEMENT, UPDATE_COUNT)); + mockSpanner.putStatementResult( + StatementResult.updateReturning(INSERT_RETURNING_STATEMENT, UPDATE_RETURNING_RESULTSET)); try (ITConnection connection = createConnection()) { connection.setTransactionTag("transaction-tag"); connection.setStatementTag("statement-tag"); connection.executeUpdate(INSERT_STATEMENT); connection.setStatementTag("statement-tag"); - connection.executeBatchUpdate(Collections.singleton(INSERT_STATEMENT)); + connection.executeBatchUpdate(ImmutableList.of(INSERT_STATEMENT, INSERT_RETURNING_STATEMENT)); connection.setStatementTag("statement-tag"); connection.executeQuery(SELECT_COUNT_STATEMENT); + connection.setStatementTag("statement-tag"); + connection.executeQuery(INSERT_RETURNING_STATEMENT); mockSpanner.abortNextStatement(); connection.commit(); @@ -272,7 +332,7 @@ public void testRetryUsesTags() { .getTransactionTag() .equals("transaction-tag")) .count(); - assertEquals(4L, executeSqlRequestCount); + assertEquals(6L, executeSqlRequestCount); long executeBatchSqlRequestCount = mockSpanner.getRequestsOfType(ExecuteBatchDmlRequest.class).stream() diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java index 7f9163e8e46..009702de3b1 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java @@ -37,6 +37,7 @@ import com.google.protobuf.Value; import com.google.spanner.v1.ExecuteSqlRequest; import com.google.spanner.v1.ResultSetMetadata; +import com.google.spanner.v1.ResultSetStats; import com.google.spanner.v1.StructType; import com.google.spanner.v1.StructType.Field; import com.google.spanner.v1.Type; @@ -96,8 +97,18 @@ public abstract class AbstractMockServerTest { .build()) .setMetadata(SELECT_COUNT_METADATA) .build(); + public static final com.google.spanner.v1.ResultSet UPDATE_RETURNING_RESULTSET = + com.google.spanner.v1.ResultSet.newBuilder() + .setStats(ResultSetStats.newBuilder().setRowCountExact(1)) + .setMetadata( + ResultSetMetadata.getDefaultInstance() + .toBuilder() + .setRowType(StructType.getDefaultInstance())) + .build(); public static final Statement INSERT_STATEMENT = Statement.of("INSERT INTO TEST (ID, NAME) VALUES (1, 'test aborted')"); + public static final Statement INSERT_RETURNING_STATEMENT = + Statement.of("INSERT INTO TEST (ID, NAME) VALUES (1, 'test aborted') THEN RETURN *"); public static final long UPDATE_COUNT = 1L; public static final int RANDOM_RESULT_SET_ROW_COUNT = 100; @@ -149,6 +160,8 @@ public void getOperation( mockSpanner.putStatementResult( StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_BEFORE_INSERT)); mockSpanner.putStatementResult(StatementResult.update(INSERT_STATEMENT, UPDATE_COUNT)); + mockSpanner.putStatementResult( + StatementResult.updateReturning(INSERT_RETURNING_STATEMENT, UPDATE_RETURNING_RESULTSET)); mockSpanner.putStatementResult( StatementResult.query(SELECT_RANDOM_STATEMENT, RANDOM_RESULT_SET)); From 5d1c0e56bce3e4b9de011c7eaef40c0fd57a1841 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Mon, 22 Aug 2022 10:20:31 +0000 Subject: [PATCH 11/33] test: add pg dml returning tests --- .../cloud/spanner/MockSpannerServiceImpl.java | 3 +- .../cloud/spanner/connection/AbortedTest.java | 20 +-- .../connection/it/ITDmlReturningTest.java | 129 ++++++++++++------ 3 files changed, 103 insertions(+), 49 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java index 6ff2b7c98d2..340f999d8ee 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java @@ -1138,8 +1138,7 @@ public void executeBatchDml( } builder.addResultSets( ResultSet.newBuilder() - .setStats( - ResultSetStats.newBuilder().setRowCountExact(updateCount).build()) + .setStats(ResultSetStats.newBuilder().setRowCountExact(updateCount).build()) .setMetadata( ResultSetMetadata.newBuilder() .setTransaction( diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java index 3980c8e9c75..6950c409a94 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java @@ -155,8 +155,9 @@ public void testAbortedDuringRetryOfFailedQuery() { @Test public void testAbortedDuringRetryOfFailedUpdate() { - final Iterable invalidStatements = ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Iterable invalidStatements = + ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); for (Statement invalidStatement : invalidStatements) { @@ -187,8 +188,9 @@ public void testAbortedDuringRetryOfFailedUpdate() { @Test public void testAbortedDuringRetryOfFailedBatchUpdate() { - final Iterable invalidStatements = ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Iterable invalidStatements = + ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); for (Statement invalidStatement : invalidStatements) { @@ -249,8 +251,9 @@ public void testAbortedDuringRetryOfFailedQueryAsFirstStatement() { @Test public void testAbortedDuringRetryOfFailedUpdateAsFirstStatement() { - final Iterable invalidStatements = ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Iterable invalidStatements = + ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); for (Statement invalidStatement : invalidStatements) { @@ -276,8 +279,9 @@ public void testAbortedDuringRetryOfFailedUpdateAsFirstStatement() { @Test public void testAbortedDuringRetryOfFailedBatchUpdateAsFirstStatement() { - final Iterable invalidStatements = ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Iterable invalidStatements = + ImmutableList.of( + Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); for (Statement invalidStatement : invalidStatements) { diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index 79b8252a626..c31c5e2a85f 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -25,7 +25,9 @@ import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; +import com.google.cloud.spanner.Dialect; import com.google.cloud.spanner.ErrorCode; +import com.google.cloud.spanner.Mutation; import com.google.cloud.spanner.ParallelIntegrationTest; import com.google.cloud.spanner.ResultSet; import com.google.cloud.spanner.SpannerException; @@ -37,53 +39,100 @@ import com.google.cloud.spanner.connection.StatementResult.ResultType; import com.google.cloud.spanner.connection.TransactionMode; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import org.junit.Before; import org.junit.Test; import org.junit.experimental.categories.Category; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; /** Execute DML Returning statements using the generic connection API. */ @Category(ParallelIntegrationTest.class) -@RunWith(JUnit4.class) +@RunWith(Parameterized.class) public class ITDmlReturningTest extends ITAbstractSpannerTest { - private final String UPDATE = - "UPDATE Singers SET LastName = \"XYZ\" WHERE FirstName = \"ABC\" THEN RETURN *"; - private static boolean initialized = false; + private final ImmutableMap UPDATE_RETURNING_MAP = + ImmutableMap.of( + Dialect.GOOGLE_STANDARD_SQL, + Statement.of("UPDATE Singers SET LastName = 'XYZ' WHERE FirstName = 'ABC' THEN RETURN *"), + Dialect.POSTGRESQL, + Statement.of("UPDATE Singers SET LastName = 'XYZ' WHERE FirstName = 'ABC' RETURNING *")); + private final ImmutableMap DDL_MAP = + ImmutableMap.of( + Dialect.GOOGLE_STANDARD_SQL, + "CREATE TABLE Singers (" + + " SingerId INT64," + + " FirstName STRING(1024)," + + " LastName STRING(1024)" + + ") PRIMARY KEY(SingerId)", + Dialect.POSTGRESQL, + "CREATE TABLE Singers (" + + " SingerId BIGINT PRIMARY KEY," + + " FirstName character varying(1024)," + + " LastName character varying(1024))"); + private final Map IS_INITIALIZED = new HashMap<>(); + + public ITDmlReturningTest() { + IS_INITIALIZED.put(Dialect.GOOGLE_STANDARD_SQL, false); + IS_INITIALIZED.put(Dialect.POSTGRESQL, false); + } + + @Parameter public Dialect dialect; + + @Parameters(name = "dialect = {0}") + public static Object[] data() { + return Dialect.values(); + } + + private boolean checkAndSetInitialized() { + if ((dialect == Dialect.GOOGLE_STANDARD_SQL) && !IS_INITIALIZED.get(dialect)) { + IS_INITIALIZED.put(dialect, true); + return true; + } + if ((dialect == Dialect.POSTGRESQL) && !IS_INITIALIZED.get(dialect)) { + IS_INITIALIZED.put(dialect, true); + return true; + } + return false; + } @Before public void setupTable() { - if (!initialized) { - try (Connection connection = createConnection()) { - if (!tableExists(connection, "Singers")) { - connection.execute( - Statement.of( - "CREATE TABLE Singers (" - + " SingerId INT64 NOT NULL," - + " FirstName STRING(1024)," - + " LastName STRING(1024)" - + ") PRIMARY KEY(SingerId)")); - connection.execute( - Statement.of( - "INSERT INTO SINGERS " - + "(SINGERID, FIRSTNAME, LASTNAME) VALUES " - + "(1, \"ABC\", \"XYZ\"), " - + "(2, \"ABC\", \"DEF\"), " - + "(3, \"DEF\", \"XYZ\"), " - + "(4, \"PQR\", \"ABC\"), " - + "(5, \"ABC\", \"GHI\")")); - } + if (checkAndSetInitialized()) { + database = + env.getTestHelper() + .createTestDatabase(dialect, Collections.singleton(DDL_MAP.get(dialect))); + List firstNames = Arrays.asList("ABC", "ABC", "DEF", "PQR", "ABC"); + List lastNames = Arrays.asList("XYZ", "DEF", "XYZ", "ABC", "GHI"); + List mutations = new ArrayList<>(); + for (int id = 1; id <= 5; id++) { + mutations.add( + Mutation.newInsertBuilder("SINGERS") + .set("SINGERID") + .to(id) + .set("FIRSTNAME") + .to(firstNames.get(id - 1)) + .set("LASTNAME") + .to(lastNames.get(id - 1)) + .build()); } - initialized = true; + env.getTestHelper().getDatabaseClient(database).write(mutations); } } @Test public void testDmlReturningExecuteQuery() { try (Connection connection = createConnection()) { - ResultSet rs = connection.executeQuery(Statement.of(UPDATE)); + ResultSet rs = connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect)); assertEquals(rs.getColumnCount(), 3); assertTrue(rs.next()); assertEquals(rs.getString(1), "ABC"); @@ -100,7 +149,7 @@ public void testDmlReturningExecuteQuery() { @Test public void testDmlReturningExecuteQueryAsync() { try (Connection connection = createConnection()) { - AsyncResultSet rs = connection.executeQueryAsync(Statement.of(UPDATE)); + AsyncResultSet rs = connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect)); rs.setCallback( Executors.newSingleThreadExecutor(), resultSet -> { @@ -134,7 +183,7 @@ public void testDmlReturningExecuteUpdate() { try (Connection connection = createConnection()) { connection.setAutocommit(false); try { - connection.executeUpdate(Statement.of(UPDATE)); + connection.executeUpdate(UPDATE_RETURNING_MAP.get(dialect)); fail("missing exception"); } catch (SpannerException e) { assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); @@ -147,7 +196,7 @@ public void testDmlReturningExecuteUpdateAsync() { try (Connection connection = createConnection()) { connection.setAutocommit(false); try { - connection.executeUpdateAsync(Statement.of(UPDATE)); + connection.executeUpdateAsync(UPDATE_RETURNING_MAP.get(dialect)); fail("missing exception"); } catch (SpannerException e) { assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); @@ -159,7 +208,7 @@ public void testDmlReturningExecuteUpdateAsync() { public void testDmlReturningExecuteBatchUpdate() { try (Connection connection = createConnection()) { connection.setAutocommit(false); - final Statement UPDATE_STMT = Statement.of(UPDATE); + final Statement UPDATE_STMT = UPDATE_RETURNING_MAP.get(dialect); long[] counts = connection.executeBatchUpdate(ImmutableList.of(UPDATE_STMT, UPDATE_STMT, UPDATE_STMT)); assertArrayEquals(counts, new long[] {3, 3, 3}); @@ -170,9 +219,11 @@ public void testDmlReturningExecuteBatchUpdate() { public void testDmlReturningExecuteBatchUpdateAsync() { try (Connection connection = createConnection()) { connection.setAutocommit(false); - final Statement UPDATE_STMT = Statement.of(UPDATE); + final Statement UPDATE_STMT = UPDATE_RETURNING_MAP.get(dialect); long[] counts = - connection.executeBatchUpdateAsync(ImmutableList.of(UPDATE_STMT, UPDATE_STMT, UPDATE_STMT)).get(); + connection + .executeBatchUpdateAsync(ImmutableList.of(UPDATE_STMT, UPDATE_STMT, UPDATE_STMT)) + .get(); assertArrayEquals(counts, new long[] {3, 3, 3}); } catch (ExecutionException | InterruptedException e) { // ignore @@ -183,7 +234,7 @@ public void testDmlReturningExecuteBatchUpdateAsync() { public void testDmlReturningExecute() { try (Connection connection = createConnection()) { connection.setAutocommit(false); - StatementResult res = connection.execute(Statement.of(UPDATE)); + StatementResult res = connection.execute(UPDATE_RETURNING_MAP.get(dialect)); assertEquals(res.getResultType(), ResultType.RESULT_SET); ResultSet rs = res.getResultSet(); assertEquals(rs.getColumnCount(), 3); @@ -203,7 +254,7 @@ public void testDmlReturningExecute() { public void testDmlReturningExecuteAsync() { try (Connection connection = createConnection()) { connection.setAutocommit(false); - AsyncStatementResult res = connection.executeAsync(Statement.of(UPDATE)); + AsyncStatementResult res = connection.executeAsync(UPDATE_RETURNING_MAP.get(dialect)); assertEquals(res.getResultType(), ResultType.RESULT_SET); AsyncResultSet rs = res.getResultSetAsync(); rs.setCallback( @@ -239,7 +290,7 @@ public void testDmlReturningExecuteQueryReadOnlyMode() { try (Connection connection = createConnection()) { connection.setReadOnly(true); try { - connection.executeQuery(Statement.of(UPDATE)); + connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect)); fail("missing exception"); } catch (SpannerException e) { assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); @@ -254,7 +305,7 @@ public void testDmlReturningExecuteQueryReadOnlyTransaction() { connection.setAutocommit(false); connection.setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION); try { - connection.executeQuery(Statement.of(UPDATE)); + connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect)); fail("missing exception"); } catch (SpannerException e) { assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); @@ -267,7 +318,7 @@ public void testDmlReturningExecuteQueryAsyncReadOnlyMode() { try (Connection connection = createConnection()) { connection.setReadOnly(true); try { - connection.executeQueryAsync(Statement.of(UPDATE)); + connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect)); fail("missing exception"); } catch (SpannerException e) { assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); @@ -282,7 +333,7 @@ public void testDmlReturningExecuteQueryAsyncReadOnlyTransaction() { connection.setAutocommit(false); connection.setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION); try { - connection.executeQueryAsync(Statement.of(UPDATE)); + connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect)); fail("missing exception"); } catch (SpannerException e) { assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); From edaad926e402d140d97e1fd2b0f6ee87e33c90c2 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Mon, 22 Aug 2022 10:25:50 +0000 Subject: [PATCH 12/33] feat: change error statement --- .../cloud/spanner/connection/ConnectionImpl.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index 270631fa08c..f718d795c4e 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -936,7 +936,8 @@ private ResultSet parseAndExecuteQuery( return internalExecuteQuery(parsedStatement, analyzeMode, options); case UPDATE: if (parsedStatement.hasReturningClause()) { - // Cannot execute in read-only mode or in READ_ONLY_TRANSACTION transaction mode. + // Cannot execute DML statement with returning clause in read-only mode or in + // READ_ONLY_TRANSACTION transaction mode. if (this.isReadOnly() || (this.isInTransaction() && this.getTransactionMode() == TransactionMode.READ_ONLY_TRANSACTION)) { @@ -955,7 +956,8 @@ private ResultSet parseAndExecuteQuery( } throw SpannerExceptionFactory.newSpannerException( ErrorCode.INVALID_ARGUMENT, - "Statement is not a query: " + parsedStatement.getSqlWithoutComments()); + "Statement is not a query or DML with returning clause: " + + parsedStatement.getSqlWithoutComments()); } private AsyncResultSet parseAndExecuteQueryAsync( @@ -977,7 +979,8 @@ private AsyncResultSet parseAndExecuteQueryAsync( return internalExecuteQueryAsync(parsedStatement, analyzeMode, options); case UPDATE: if (parsedStatement.hasReturningClause()) { - // Cannot execute in read-only mode or in READ_ONLY_TRANSACTION transaction mode. + // Cannot execute DML statement with returning clause in read-only mode or in + // READ_ONLY_TRANSACTION transaction mode. if (this.isReadOnly() || (this.isInTransaction() && this.getTransactionMode() == TransactionMode.READ_ONLY_TRANSACTION)) { @@ -996,7 +999,8 @@ private AsyncResultSet parseAndExecuteQueryAsync( } throw SpannerExceptionFactory.newSpannerException( ErrorCode.INVALID_ARGUMENT, - "Statement is not a query: " + parsedStatement.getSqlWithoutComments()); + "Statement is not a query or DML with returning clause: " + + parsedStatement.getSqlWithoutComments()); } @Override From 24cf255c16fc854e1f53c3b7802f7ffa3e5140b5 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Mon, 22 Aug 2022 18:38:43 +0000 Subject: [PATCH 13/33] doc: add doc for dml with returning clause usage --- .../cloud/spanner/connection/Connection.java | 53 ++++++++++--------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java index 04ea893cb9c..afb3723c143 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java @@ -856,8 +856,8 @@ default RpcPriority getRPCPriority() { * state. The returned value depends on the type of statement: * *
    - *
  • Queries will return a {@link ResultSet} - *
  • DML statements will return an update count + *
  • Queries and DML statements with returning clause will return a {@link ResultSet}. + *
  • Simple DML statements will return an update count *
  • DDL statements will return a {@link ResultType#NO_RESULT} *
  • Connection and transaction statements (SET AUTOCOMMIT=TRUE|FALSE, SHOW AUTOCOMMIT, SET * TRANSACTION READ ONLY, etc) will return either a {@link ResultSet} or {@link @@ -874,9 +874,9 @@ default RpcPriority getRPCPriority() { * state asynchronously. The returned value depends on the type of statement: * *
      - *
    • Queries will return an {@link AsyncResultSet} - *
    • DML statements will return an {@link ApiFuture} with an update count that is done when - * the DML statement has been applied successfully, or that throws an {@link + *
    • Queries and DML statements with returning clause will return an {@link AsyncResultSet}. + *
    • Simple DML statements will return an {@link ApiFuture} with an update count that is done + * when the DML statement has been applied successfully, or that throws an {@link * ExecutionException} if the DML statement failed. *
    • DDL statements will return an {@link ApiFuture} containing a {@link Void} that is done * when the DDL statement has been applied successfully, or that throws an {@link @@ -894,31 +894,33 @@ default RpcPriority getRPCPriority() { AsyncStatementResult executeAsync(Statement statement); /** - * Executes the given statement as a query and returns the result as a {@link ResultSet}. This - * method blocks and waits for a response from Spanner. If the statement does not contain a valid - * query, the method will throw a {@link SpannerException}. + * Executes the given statement (a query or a DML statement with returning clause) and returns the + * result as a {@link ResultSet}. This method blocks and waits for a response from Spanner. If the + * statement does not contain a valid query or a DML statement with returning clause, the method + * will throw a {@link SpannerException}. * - * @param query The query statement to execute + * @param query The query statement or DML statement with returning clause to execute * @param options the options to configure the query - * @return a {@link ResultSet} with the results of the query + * @return a {@link ResultSet} with the results of the statement */ ResultSet executeQuery(Statement query, QueryOption... options); /** - * Executes the given statement asynchronously as a query and returns the result as an {@link - * AsyncResultSet}. This method is guaranteed to be non-blocking. If the statement does not - * contain a valid query, the method will throw a {@link SpannerException}. + * Executes the given statement (a query or a DML statement with returning clause) asynchronously + * and returns the result as an {@link AsyncResultSet}. This method is guaranteed to be + * non-blocking. If the statement does not contain a valid query or a DML statement with returning + * clause, the method will throw a {@link SpannerException}. * *

      See {@link AsyncResultSet#setCallback(java.util.concurrent.Executor, * com.google.cloud.spanner.AsyncResultSet.ReadyCallback)} for more information on how to consume - * the results of the query asynchronously. + * the results of the statement asynchronously. * *

      It is also possible to consume the returned {@link AsyncResultSet} in the same way as a * normal {@link ResultSet}, i.e. in a while-loop calling {@link AsyncResultSet#next()}. * - * @param query The query statement to execute + * @param query The query statement or DML statement with returning clause to execute * @param options the options to configure the query - * @return an {@link AsyncResultSet} with the results of the query + * @return an {@link AsyncResultSet} with the results of the statement */ AsyncResultSet executeQueryAsync(Statement query, QueryOption... options); @@ -951,8 +953,8 @@ default RpcPriority getRPCPriority() { ResultSet analyzeQuery(Statement query, QueryAnalyzeMode queryMode); /** - * Executes the given statement as a DML statement. If the statement does not contain a valid DML - * statement, the method will throw a {@link SpannerException}. + * Executes the given statement as a simple DML statement. If the statement does not contain a + * valid DML statement, the method will throw a {@link SpannerException}. * * @param update The update statement to execute * @return the number of records that were inserted/updated/deleted by this statement @@ -972,8 +974,9 @@ default ResultSetStats analyzeUpdate(Statement update, QueryAnalyzeMode analyzeM } /** - * Executes the given statement asynchronously as a DML statement. If the statement does not - * contain a valid DML statement, the method will throw a {@link SpannerException}. + * Executes the given statement asynchronously as a simple DML statement. If the statement does + * not contain a simple DML statement, the method will throw a {@link SpannerException}. A DML + * statement with returning clause will throw a {@link SpannerException}. * *

      This method is guaranteed to be non-blocking. * @@ -984,8 +987,9 @@ default ResultSetStats analyzeUpdate(Statement update, QueryAnalyzeMode analyzeM ApiFuture executeUpdateAsync(Statement update); /** - * Executes a list of DML statements in a single request. The statements will be executed in order - * and the semantics is the same as if each statement is executed by {@link + * Executes a list of DML statements (can be simple DML statements or DML statements with + * returning clause) in a single request. The statements will be executed in order and the + * semantics is the same as if each statement is executed by {@link * Connection#executeUpdate(Statement)} in a loop. This method returns an array of long integers, * each representing the number of rows modified by each statement. * @@ -1006,8 +1010,9 @@ default ResultSetStats analyzeUpdate(Statement update, QueryAnalyzeMode analyzeM long[] executeBatchUpdate(Iterable updates); /** - * Executes a list of DML statements in a single request. The statements will be executed in order - * and the semantics is the same as if each statement is executed by {@link + * Executes a list of DML statements (can be simple DML statements or DML statements with + * returning clause) in a single request. The statements will be executed in order and the + * semantics is the same as if each statement is executed by {@link * Connection#executeUpdate(Statement)} in a loop. This method returns an {@link ApiFuture} that * contains an array of long integers, each representing the number of rows modified by each * statement. From 4024412fef28aebeb3b903a1f4638d69a908c710 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta <93644539+rajatbhatta@users.noreply.github.com> Date: Tue, 23 Aug 2022 13:40:24 +0530 Subject: [PATCH 14/33] Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Knut Olav Løite --- .../com/google/cloud/spanner/connection/ConnectionImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index f718d795c4e..337958b9c24 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -1014,7 +1014,7 @@ public long executeUpdate(Statement update) { if (parsedStatement.hasReturningClause()) { throw SpannerExceptionFactory.newSpannerException( ErrorCode.FAILED_PRECONDITION, - "DML statement with returning clause cannot be executed over executeUpdate: " + "DML statement with returning clause cannot be executed using executeUpdate: " + parsedStatement.getSqlWithoutComments() + ". Please use executeQuery instead."); } From 4ae2f8db80b5116f9f39452fe250a178eeff5e17 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta <93644539+rajatbhatta@users.noreply.github.com> Date: Tue, 23 Aug 2022 13:40:40 +0530 Subject: [PATCH 15/33] Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Knut Olav Løite --- .../com/google/cloud/spanner/connection/ConnectionImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index 337958b9c24..ba7c000d0da 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -1042,7 +1042,7 @@ public ApiFuture executeUpdateAsync(Statement update) { if (parsedStatement.hasReturningClause()) { throw SpannerExceptionFactory.newSpannerException( ErrorCode.FAILED_PRECONDITION, - "DML statement with returning clause cannot be executed over executeUpdateAsync: " + "DML statement with returning clause cannot be executed using executeUpdateAsync: " + parsedStatement.getSqlWithoutComments() + ". Please use executeQueryAsync instead."); } From c601a501e89323f154c484b7af97349222ff0cc8 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 23 Aug 2022 13:35:50 +0000 Subject: [PATCH 16/33] fix: incorporate review comments --- .../spanner/connection/ConnectionImpl.java | 2 - .../connection/PostgreSQLStatementParser.java | 25 ++++--- .../connection/SpannerStatementParser.java | 21 +++--- .../connection/StatementParserTest.java | 20 +++++- .../connection/it/ITDmlReturningTest.java | 69 +++++++++---------- 5 files changed, 71 insertions(+), 66 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index ba7c000d0da..e935a607a83 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -948,7 +948,6 @@ private ResultSet parseAndExecuteQuery( } return internalExecuteQuery(parsedStatement, analyzeMode, options); } - case DDL: case UNKNOWN: default: @@ -991,7 +990,6 @@ private AsyncResultSet parseAndExecuteQueryAsync( } return internalExecuteQueryAsync(parsedStatement, analyzeMode, options); } - case DDL: case UNKNOWN: default: diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java index e767e425f57..15e857e8744 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java @@ -30,6 +30,9 @@ @InternalApi public class PostgreSQLStatementParser extends AbstractStatementParser { + private static final Pattern RETURNING_PATTERN = Pattern.compile("[ ')\"]returning[ '(\"]"); + private static final Pattern AS_RETURNING_PATTERN = Pattern.compile("[ ')\"]as returning[ '(\"]"); + PostgreSQLStatementParser() throws CompileException { super( Collections.unmodifiableSet( @@ -293,13 +296,11 @@ private boolean isReturning(String sql, int index) { // leading AS to be used as column label, to avoid ambiguity. // We thus check for cases which do not have a leading AS. // (https://www.postgresql.org/docs/current/sql-keywords-appendix.html) - return Pattern.compile("[\\s)]RETURNING[\\s(][\\s\\S]*", Pattern.CASE_INSENSITIVE) - .matcher(sql.substring(index)) - .matches() - && !((index >= 3) - && Pattern.compile("[\\s]AS[\\s]RETURNING[\\s(][\\s\\S]*", Pattern.CASE_INSENSITIVE) - .matcher(sql.substring(index - 3)) - .matches()); + return (index >= 1) + && (index + 10 <= sql.length()) + && RETURNING_PATTERN.matcher(sql.substring(index - 1, index + 10)).matches() + && !((index >= 4) + && AS_RETURNING_PATTERN.matcher(sql.substring(index - 4, index + 10)).matches()); } @InternalApi @@ -307,16 +308,14 @@ private boolean isReturning(String sql, int index) { boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); int index = 0; - String sql = rawSql.replaceAll("\\s+", " "); - boolean hasReturningClause = false; + String sql = rawSql.replaceAll("\\s+", " ").toLowerCase(); while (index < sql.length()) { - if (!hasReturningClause && isReturning(sql, index)) { - hasReturningClause = true; - index++; + if (isReturning(sql, index)) { + return true; } else { index = skip(sql, index, null); } } - return hasReturningClause; + return false; } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java index aecae09ab05..d7520714890 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java @@ -30,6 +30,8 @@ @InternalApi public class SpannerStatementParser extends AbstractStatementParser { + final Pattern THEN_RETURN_PATTERN = Pattern.compile("[ `')\"]then return[ `'(\"]"); + public SpannerStatementParser() throws CompileException { super( Collections.unmodifiableSet( @@ -271,16 +273,16 @@ ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramCh } private boolean isReturning(String sql, int index) { - return Pattern.compile("[\\s)]THEN(\\s+)RETURN[\\s(][\\s\\S]*", Pattern.CASE_INSENSITIVE) - .matcher(sql.substring(index)) - .matches(); + return (index >= 1) + && (index + 12 <= sql.length()) + && THEN_RETURN_PATTERN.matcher(sql.substring(index - 1, index + 12)).matches(); } @InternalApi @Override boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); - String sql = rawSql.replaceAll("\\s+", " "); + String sql = rawSql.replaceAll("\\s+", " ").toLowerCase(); final char SINGLE_QUOTE = '\''; final char DOUBLE_QUOTE = '"'; final char BACKTICK_QUOTE = '`'; @@ -288,7 +290,6 @@ boolean checkReturningClauseInternal(String rawSql) { char startQuote = 0; boolean lastCharWasEscapeChar = false; boolean isTripleQuoted = false; - boolean hasReturningClause = false; for (int index = 0; index < sql.length(); index++) { char c = sql.charAt(index); if (isInQuoted) { @@ -313,8 +314,8 @@ boolean checkReturningClauseInternal(String rawSql) { lastCharWasEscapeChar = false; } } else { - if (!hasReturningClause && isReturning(sql, index)) { - hasReturningClause = true; + if (isReturning(sql, index)) { + return true; } else { if (c == SINGLE_QUOTE || c == DOUBLE_QUOTE || c == BACKTICK_QUOTE) { isInQuoted = true; @@ -329,10 +330,6 @@ boolean checkReturningClauseInternal(String rawSql) { } } } - if (isInQuoted) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } - return hasReturningClause; + return false; } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index b135fb354cf..17450d531d1 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -1336,6 +1336,12 @@ public void testGoogleSQLReturningClause() { + "*/return/*" + "comment*/(a)")) .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("delete from x where y=\"z\"then return *")) + .hasReturningClause()); + assertTrue( + parser.parse(Statement.of("delete from x where 10=`z`then return *")).hasReturningClause()); assertFalse( parser .parse(Statement.of("insert into x (a,b) values (1,2) returning (a)")) @@ -1390,8 +1396,20 @@ public void testPostgreSQLReturningClause() { .hasReturningClause()); assertTrue( parser - .parse(Statement.of("insert into t1 select 1 as returning returning *")) + .parse(Statement.of("insert into x select 1 as returning returning *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x select 'returning' as returning returning *")) + .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x select 'returning'as returning returning *")) .hasReturningClause()); + assertTrue( + parser.parse(Statement.of("delete from x where y=\"z\"returning *")).hasReturningClause()); + assertTrue( + parser.parse(Statement.of("delete from x where y='z'returning *")).hasReturningClause()); assertTrue( parser .parse(Statement.of("insert into t1 select 1/*as /*returning*/ returning*/returning *")) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index c31c5e2a85f..a5fa17692c2 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -20,8 +20,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; @@ -171,7 +171,6 @@ public void testDmlReturningExecuteQueryAsync() { } } } catch (SpannerException e) { - System.out.printf("Error in callback: %s%n", e.getMessage()); return CallbackResponse.DONE; } }); @@ -182,12 +181,11 @@ public void testDmlReturningExecuteQueryAsync() { public void testDmlReturningExecuteUpdate() { try (Connection connection = createConnection()) { connection.setAutocommit(false); - try { - connection.executeUpdate(UPDATE_RETURNING_MAP.get(dialect)); - fail("missing exception"); - } catch (SpannerException e) { - assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); - } + SpannerException e = + assertThrows( + SpannerException.class, + () -> connection.executeUpdate(UPDATE_RETURNING_MAP.get(dialect))); + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); } } @@ -195,12 +193,11 @@ public void testDmlReturningExecuteUpdate() { public void testDmlReturningExecuteUpdateAsync() { try (Connection connection = createConnection()) { connection.setAutocommit(false); - try { - connection.executeUpdateAsync(UPDATE_RETURNING_MAP.get(dialect)); - fail("missing exception"); - } catch (SpannerException e) { - assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); - } + SpannerException e = + assertThrows( + SpannerException.class, + () -> connection.executeUpdateAsync(UPDATE_RETURNING_MAP.get(dialect))); + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); } } @@ -289,12 +286,11 @@ public void testDmlReturningExecuteAsync() { public void testDmlReturningExecuteQueryReadOnlyMode() { try (Connection connection = createConnection()) { connection.setReadOnly(true); - try { - connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect)); - fail("missing exception"); - } catch (SpannerException e) { - assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); - } + SpannerException e = + assertThrows( + SpannerException.class, + () -> connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect))); + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); } } @@ -304,12 +300,11 @@ public void testDmlReturningExecuteQueryReadOnlyTransaction() { connection.setReadOnly(false); connection.setAutocommit(false); connection.setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION); - try { - connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect)); - fail("missing exception"); - } catch (SpannerException e) { - assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); - } + SpannerException e = + assertThrows( + SpannerException.class, + () -> connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect))); + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); } } @@ -317,12 +312,11 @@ public void testDmlReturningExecuteQueryReadOnlyTransaction() { public void testDmlReturningExecuteQueryAsyncReadOnlyMode() { try (Connection connection = createConnection()) { connection.setReadOnly(true); - try { - connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect)); - fail("missing exception"); - } catch (SpannerException e) { - assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); - } + SpannerException e = + assertThrows( + SpannerException.class, + () -> connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect))); + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); } } @@ -332,12 +326,11 @@ public void testDmlReturningExecuteQueryAsyncReadOnlyTransaction() { connection.setReadOnly(false); connection.setAutocommit(false); connection.setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION); - try { - connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect)); - fail("missing exception"); - } catch (SpannerException e) { - assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); - } + SpannerException e = + assertThrows( + SpannerException.class, + () -> connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect))); + assertEquals(e.getErrorCode(), ErrorCode.FAILED_PRECONDITION); } } } From 14e741f627dbb0137296ebf1a09fb6e9a6595fc4 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 23 Aug 2022 13:42:02 +0000 Subject: [PATCH 17/33] test: add more test cases --- .../google/cloud/spanner/connection/StatementParserTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index 17450d531d1..cf7d79fcd89 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -1340,6 +1340,10 @@ public void testGoogleSQLReturningClause() { parser .parse(Statement.of("delete from x where y=\"z\"then return *")) .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into x select 'then return' as returning then return *")) + .hasReturningClause()); assertTrue( parser.parse(Statement.of("delete from x where 10=`z`then return *")).hasReturningClause()); assertFalse( From 64422eb8b01099c319cf259de733673f16189619 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 23 Aug 2022 14:13:24 +0000 Subject: [PATCH 18/33] test: add todo --- .../src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java | 1 + .../test/java/com/google/cloud/spanner/IntegrationTestEnv.java | 1 + 2 files changed, 2 insertions(+) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java index 808b3922696..30db3e09f73 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java @@ -58,6 +58,7 @@ public class GceTestEnvConfig implements TestEnvConfig { private final SpannerOptions options; public GceTestEnvConfig() { + // TODO: Revert back any changes to this file before merging. // String projectId = System.getProperty(GCE_PROJECT_ID, ""); String projectId = "span-cloud-testing"; // String serverUrl = System.getProperty(GCE_SERVER_URL, ""); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java index 466ba356b89..d2596b78c6b 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java @@ -91,6 +91,7 @@ protected void before() throws Throwable { this.config.setUp(); SpannerOptions options = config.spannerOptions(); + // TODO: Revert back any changes to this file before merging. // String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); String instanceProperty = "projects/span-cloud-testing/instances/rajatrb-test-instance"; InstanceId instanceId; From 82e35fe3f5726eb9f8815574c1ed9f017f15ad4b Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 23 Aug 2022 14:43:56 +0000 Subject: [PATCH 19/33] test: add separate abort tests for dml returning --- .../cloud/spanner/connection/AbortedTest.java | 301 +++++++++++++----- .../connection/AbstractMockServerTest.java | 11 +- 2 files changed, 224 insertions(+), 88 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java index 6950c409a94..89406df2cb7 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java @@ -155,63 +155,104 @@ public void testAbortedDuringRetryOfFailedQuery() { @Test public void testAbortedDuringRetryOfFailedUpdate() { - final Iterable invalidStatements = - ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Statement invalidStatement = Statement.of("INSERT INTO FOO"); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - for (Statement invalidStatement : invalidStatements) { - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { - connection.execute(INSERT_STATEMENT); - try { - connection.execute(invalidStatement); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); - } - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + connection.execute(INSERT_STATEMENT); + try { + connection.execute(invalidStatement); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - // The transaction will be executed 3 times, which means that there will be 6 - // ExecuteSqlRequests: - // 1. The initial attempt. - // 2. The first retry attempt. This will fail on the invalid statement as it is aborted. - // 3. the second retry attempt. This will succeed. - assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(6); - mockSpanner.clearRequests(); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + // The transaction will be executed 3 times, which means that there will be 6 + // ExecuteSqlRequests: + // 1. The initial attempt. + // 2. The first retry attempt. This will fail on the invalid statement as it is aborted. + // 3. the second retry attempt. This will succeed. + assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(6); + } + + @Test + public void testAbortedDuringRetryOfFailedUpdateWithReturning() { + final Statement invalidStatement = Statement.of("INSERT INTO FOO THEN RETURN *"); + StatusRuntimeException notFound = + Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + connection.execute(INSERT_STATEMENT); + try { + connection.execute(invalidStatement); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); + } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + // The transaction will be executed 3 times, which means that there will be 6 + // ExecuteSqlRequests: + // 1. The initial attempt. + // 2. The first retry attempt. This will fail on the invalid statement as it is aborted. + // 3. the second retry attempt. This will succeed. + assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(6); } @Test public void testAbortedDuringRetryOfFailedBatchUpdate() { - final Iterable invalidStatements = - ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Statement invalidStatement = Statement.of("INSERT INTO FOO"); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - for (Statement invalidStatement : invalidStatements) { - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { - connection.execute(INSERT_STATEMENT); - try { - connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); - } - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + connection.execute(INSERT_STATEMENT); + try { + connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); + } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(3); + } + + @Test + public void testAbortedDuringRetryOfFailedBatchUpdateWithReturning() { + final Statement invalidStatement = Statement.of("INSERT INTO FOO THEN RETURN *"); + StatusRuntimeException notFound = + Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + connection.execute(INSERT_STATEMENT); + try { + connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(3); - mockSpanner.clearRequests(); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(3); } @Test @@ -251,62 +292,152 @@ public void testAbortedDuringRetryOfFailedQueryAsFirstStatement() { @Test public void testAbortedDuringRetryOfFailedUpdateAsFirstStatement() { - final Iterable invalidStatements = - ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Statement invalidStatement = Statement.of("INSERT INTO FOO"); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - for (Statement invalidStatement : invalidStatements) { - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortRetryListener(2, invalidStatement, notFound))) { - try { - connection.execute(invalidStatement); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); - } - connection.execute(INSERT_STATEMENT); - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortRetryListener(2, invalidStatement, notFound))) { + try { + connection.execute(invalidStatement); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(8); - mockSpanner.clearRequests(); + connection.execute(INSERT_STATEMENT); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(8); + } + + @Test + public void testAbortedDuringRetryOfFailedUpdateWithReturningAsFirstStatement() { + final Statement invalidStatement = Statement.of("INSERT INTO FOO THEN RETURN *"); + StatusRuntimeException notFound = + Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortRetryListener(2, invalidStatement, notFound))) { + try { + connection.execute(invalidStatement); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + connection.execute(INSERT_STATEMENT); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); + } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)).isEqualTo(8); } @Test public void testAbortedDuringRetryOfFailedBatchUpdateAsFirstStatement() { - final Iterable invalidStatements = - ImmutableList.of( - Statement.of("INSERT INTO FOO"), Statement.of("INSERT INTO FOO THEN RETURN *")); + final Statement invalidStatement = Statement.of("INSERT INTO FOO"); StatusRuntimeException notFound = Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); - for (Statement invalidStatement : invalidStatements) { - mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); - try (ITConnection connection = - createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { - try { - connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); - } - connection.execute(INSERT_STATEMENT); - // Force an abort and retry. - mockSpanner.abortNextStatement(); - connection.commit(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + try { + connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); + } + connection.execute(INSERT_STATEMENT); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); + } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(6); + } + + @Test + public void testAbortedDuringRetryOfFailedBatchUpdateWithReturningAsFirstStatement() { + final Statement invalidStatement = Statement.of("INSERT INTO FOO THEN RETURN *"); + StatusRuntimeException notFound = + Status.NOT_FOUND.withDescription("Table not found").asRuntimeException(); + mockSpanner.putStatementResult(StatementResult.exception(invalidStatement, notFound)); + try (ITConnection connection = + createConnection(createAbortFirstRetryListener(invalidStatement, notFound))) { + try { + connection.executeBatchUpdate(Collections.singletonList(invalidStatement)); + fail("missing expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.NOT_FOUND); } - assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); - assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(6); - mockSpanner.clearRequests(); + connection.execute(INSERT_STATEMENT); + // Force an abort and retry. + mockSpanner.abortNextStatement(); + connection.commit(); } + assertThat(mockSpanner.countRequestsOfType(CommitRequest.class)).isEqualTo(2); + assertThat(mockSpanner.countRequestsOfType(ExecuteBatchDmlRequest.class)).isEqualTo(6); } @Test public void testRetryUsesTags() { + mockSpanner.putStatementResult( + StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_BEFORE_INSERT)); + mockSpanner.putStatementResult(StatementResult.update(INSERT_STATEMENT, UPDATE_COUNT)); + try (ITConnection connection = createConnection()) { + connection.setTransactionTag("transaction-tag"); + connection.setStatementTag("statement-tag"); + connection.executeUpdate(INSERT_STATEMENT); + connection.setStatementTag("statement-tag"); + connection.executeBatchUpdate(Collections.singleton(INSERT_STATEMENT)); + connection.setStatementTag("statement-tag"); + connection.executeQuery(SELECT_COUNT_STATEMENT); + + mockSpanner.abortNextStatement(); + connection.commit(); + } + long executeSqlRequestCount = + mockSpanner.getRequestsOfType(ExecuteSqlRequest.class).stream() + .filter( + request -> + request.getRequestOptions().getRequestTag().equals("statement-tag") + && request + .getRequestOptions() + .getTransactionTag() + .equals("transaction-tag")) + .count(); + assertEquals(4L, executeSqlRequestCount); + + long executeBatchSqlRequestCount = + mockSpanner.getRequestsOfType(ExecuteBatchDmlRequest.class).stream() + .filter( + request -> + request.getRequestOptions().getRequestTag().equals("statement-tag") + && request + .getRequestOptions() + .getTransactionTag() + .equals("transaction-tag")) + .count(); + assertEquals(2L, executeBatchSqlRequestCount); + + long commitRequestCount = + mockSpanner.getRequestsOfType(CommitRequest.class).stream() + .filter( + request -> + request.getRequestOptions().getRequestTag().equals("") + && request + .getRequestOptions() + .getTransactionTag() + .equals("transaction-tag")) + .count(); + assertEquals(2L, commitRequestCount); + } + + @Test + public void testRetryUsesTagsWithUpdateReturning() { mockSpanner.putStatementResult( StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_BEFORE_INSERT)); mockSpanner.putStatementResult(StatementResult.update(INSERT_STATEMENT, UPDATE_COUNT)); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java index 009702de3b1..7b65b44f361 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java @@ -101,9 +101,14 @@ public abstract class AbstractMockServerTest { com.google.spanner.v1.ResultSet.newBuilder() .setStats(ResultSetStats.newBuilder().setRowCountExact(1)) .setMetadata( - ResultSetMetadata.getDefaultInstance() - .toBuilder() - .setRowType(StructType.getDefaultInstance())) + ResultSetMetadata.newBuilder() + .setRowType( + StructType.newBuilder() + .addFields( + Field.newBuilder() + .setName("col") + .setType(Type.newBuilder().setCodeValue(TypeCode.INT64_VALUE)) + .build()))) .build(); public static final Statement INSERT_STATEMENT = Statement.of("INSERT INTO TEST (ID, NAME) VALUES (1, 'test aborted')"); From a7db0c1848a236c271ea95a23a05ab282c31e260 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 23 Aug 2022 15:13:34 +0000 Subject: [PATCH 20/33] fix: add try-with-resources block around ResultSet --- .../connection/it/ITDmlReturningTest.java | 142 +++++++++--------- 1 file changed, 73 insertions(+), 69 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index a5fa17692c2..3a7a599cb8a 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -132,48 +132,50 @@ public void setupTable() { @Test public void testDmlReturningExecuteQuery() { try (Connection connection = createConnection()) { - ResultSet rs = connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect)); - assertEquals(rs.getColumnCount(), 3); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertFalse(rs.next()); - assertNotNull(rs.getStats()); - assertEquals(rs.getStats().getRowCountExact(), 3); + try (ResultSet rs = connection.executeQuery(UPDATE_RETURNING_MAP.get(dialect))) { + assertEquals(rs.getColumnCount(), 3); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertFalse(rs.next()); + assertNotNull(rs.getStats()); + assertEquals(rs.getStats().getRowCountExact(), 3); + } } } @Test public void testDmlReturningExecuteQueryAsync() { try (Connection connection = createConnection()) { - AsyncResultSet rs = connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect)); - rs.setCallback( - Executors.newSingleThreadExecutor(), - resultSet -> { - try { - while (true) { - switch (resultSet.tryNext()) { - case OK: - assertEquals(resultSet.getColumnCount(), 3); - assertEquals(resultSet.getString(1), "ABC"); - break; - case DONE: - assertNotNull(resultSet.getStats()); - assertEquals(resultSet.getStats().getRowCountExact(), 3); - return CallbackResponse.DONE; - case NOT_READY: - return CallbackResponse.CONTINUE; - default: - throw new IllegalStateException(); + try (AsyncResultSet rs = connection.executeQueryAsync(UPDATE_RETURNING_MAP.get(dialect))) { + rs.setCallback( + Executors.newSingleThreadExecutor(), + resultSet -> { + try { + while (true) { + switch (resultSet.tryNext()) { + case OK: + assertEquals(resultSet.getColumnCount(), 3); + assertEquals(resultSet.getString(1), "ABC"); + break; + case DONE: + assertNotNull(resultSet.getStats()); + assertEquals(resultSet.getStats().getRowCountExact(), 3); + return CallbackResponse.DONE; + case NOT_READY: + return CallbackResponse.CONTINUE; + default: + throw new IllegalStateException(); + } } + } catch (SpannerException e) { + return CallbackResponse.DONE; } - } catch (SpannerException e) { - return CallbackResponse.DONE; - } - }); + }); + } } } @@ -233,17 +235,18 @@ public void testDmlReturningExecute() { connection.setAutocommit(false); StatementResult res = connection.execute(UPDATE_RETURNING_MAP.get(dialect)); assertEquals(res.getResultType(), ResultType.RESULT_SET); - ResultSet rs = res.getResultSet(); - assertEquals(rs.getColumnCount(), 3); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertTrue(rs.next()); - assertEquals(rs.getString(1), "ABC"); - assertFalse(rs.next()); - assertNotNull(rs.getStats()); - assertEquals(rs.getStats().getRowCountExact(), 3); + try (ResultSet rs = res.getResultSet()) { + assertEquals(rs.getColumnCount(), 3); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertTrue(rs.next()); + assertEquals(rs.getString(1), "ABC"); + assertFalse(rs.next()); + assertNotNull(rs.getStats()); + assertEquals(rs.getStats().getRowCountExact(), 3); + } } } @@ -253,32 +256,33 @@ public void testDmlReturningExecuteAsync() { connection.setAutocommit(false); AsyncStatementResult res = connection.executeAsync(UPDATE_RETURNING_MAP.get(dialect)); assertEquals(res.getResultType(), ResultType.RESULT_SET); - AsyncResultSet rs = res.getResultSetAsync(); - rs.setCallback( - Executors.newSingleThreadExecutor(), - resultSet -> { - try { - while (true) { - switch (resultSet.tryNext()) { - case OK: - assertEquals(resultSet.getColumnCount(), 3); - assertEquals(resultSet.getString(1), "ABC"); - break; - case DONE: - assertNotNull(resultSet.getStats()); - assertEquals(resultSet.getStats().getRowCountExact(), 3); - return CallbackResponse.DONE; - case NOT_READY: - return CallbackResponse.CONTINUE; - default: - throw new IllegalStateException(); + try (AsyncResultSet rs = res.getResultSetAsync()) { + rs.setCallback( + Executors.newSingleThreadExecutor(), + resultSet -> { + try { + while (true) { + switch (resultSet.tryNext()) { + case OK: + assertEquals(resultSet.getColumnCount(), 3); + assertEquals(resultSet.getString(1), "ABC"); + break; + case DONE: + assertNotNull(resultSet.getStats()); + assertEquals(resultSet.getStats().getRowCountExact(), 3); + return CallbackResponse.DONE; + case NOT_READY: + return CallbackResponse.CONTINUE; + default: + throw new IllegalStateException(); + } } + } catch (SpannerException e) { + System.out.printf("Error in callback: %s%n", e.getMessage()); + return CallbackResponse.DONE; } - } catch (SpannerException e) { - System.out.printf("Error in callback: %s%n", e.getMessage()); - return CallbackResponse.DONE; - } - }); + }); + } } } From cc0039625edfa52a3e2956f85bd9fb3e58520b0f Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 24 Aug 2022 07:07:46 +0000 Subject: [PATCH 21/33] feat: enhancement by adding a pre-check --- .../spanner/connection/PostgreSQLStatementParser.java | 7 ++++++- .../cloud/spanner/connection/SpannerStatementParser.java | 5 +++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java index 15e857e8744..0d1a6b2999b 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java @@ -307,8 +307,13 @@ private boolean isReturning(String sql, int index) { @Override boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); - int index = 0; String sql = rawSql.replaceAll("\\s+", " ").toLowerCase(); + // Do a pre-check to check if the SQL string definitely does not have a returning clause. + // If this check fails, do a more involved check to check for a returning clause. + if (!sql.contains("returning")) { + return false; + } + int index = 0; while (index < sql.length()) { if (isReturning(sql, index)) { return true; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java index d7520714890..2c78e329125 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java @@ -283,6 +283,11 @@ private boolean isReturning(String sql, int index) { boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); String sql = rawSql.replaceAll("\\s+", " ").toLowerCase(); + // Do a pre-check to check if the SQL string definitely does not have a returning clause. + // If this check fails, do a more involved check to check for a returning clause. + if (!(sql.contains("then return"))) { + return false; + } final char SINGLE_QUOTE = '\''; final char DOUBLE_QUOTE = '"'; final char BACKTICK_QUOTE = '`'; From c093b9b801acdc2ba24513dc91622670734912a1 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 24 Aug 2022 08:55:10 +0000 Subject: [PATCH 22/33] feat: changes --- .../cloud/spanner/connection/PostgreSQLStatementParser.java | 3 ++- .../cloud/spanner/connection/SpannerStatementParser.java | 5 +++-- .../com/google/cloud/spanner/connection/it/RegexTester.java | 5 +++++ 3 files changed, 10 insertions(+), 3 deletions(-) create mode 100644 google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java index 0d1a6b2999b..fe0c9cc88ba 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java @@ -307,12 +307,13 @@ private boolean isReturning(String sql, int index) { @Override boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); - String sql = rawSql.replaceAll("\\s+", " ").toLowerCase(); + String sql = rawSql.toLowerCase(); // Do a pre-check to check if the SQL string definitely does not have a returning clause. // If this check fails, do a more involved check to check for a returning clause. if (!sql.contains("returning")) { return false; } + sql = sql.replaceAll("\\s+", " "); int index = 0; while (index < sql.length()) { if (isReturning(sql, index)) { diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java index 2c78e329125..9d2489fa5d6 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java @@ -282,12 +282,13 @@ private boolean isReturning(String sql, int index) { @Override boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); - String sql = rawSql.replaceAll("\\s+", " ").toLowerCase(); + String sql = rawSql.toLowerCase(); // Do a pre-check to check if the SQL string definitely does not have a returning clause. // If this check fails, do a more involved check to check for a returning clause. - if (!(sql.contains("then return"))) { + if (!(sql.contains("then") && sql.contains("return"))) { return false; } + sql = sql.replaceAll("\\s+", " "); final char SINGLE_QUOTE = '\''; final char DOUBLE_QUOTE = '"'; final char BACKTICK_QUOTE = '`'; diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java new file mode 100644 index 00000000000..1d2a4c53816 --- /dev/null +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java @@ -0,0 +1,5 @@ +package com.google.cloud.spanner.connection.it; + +public class RegexTester { + +} From 4af9cbe43e0c8101d2838753f5c78ca78c8cfcb1 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 24 Aug 2022 11:52:56 +0000 Subject: [PATCH 23/33] test: delete unnecessary test --- .../com/google/cloud/spanner/connection/it/RegexTester.java | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java deleted file mode 100644 index 1d2a4c53816..00000000000 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/RegexTester.java +++ /dev/null @@ -1,5 +0,0 @@ -package com.google.cloud.spanner.connection.it; - -public class RegexTester { - -} From 9f59715e76511dceb43825a66c06f290978ed02e Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 24 Aug 2022 19:17:25 +0000 Subject: [PATCH 24/33] test: add few more tests to PG parser --- .../connection/StatementParserTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index cf7d79fcd89..3923c9821c5 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -1436,6 +1436,25 @@ public void testPostgreSQLReturningClause() { parser .parse(Statement.of("insert into t1 select 1 as/*eomment*/returning returning *")) .hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("UPDATE x SET y = $$ RETURNING a, b, c$$ WHERE z = 123")) + .hasReturningClause()); + assertFalse( + parser + .parse( + Statement.of("UPDATE x SET y = $foobar$ RETURNING a, b, c$foobar$ WHERE z = 123")) + .hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("UPDATE x SET y = $returning$ returning $returning$ WHERE z = 123")) + .hasReturningClause()); + assertTrue( + parser + .parse( + Statement.of( + "UPDATE x SET y = $returning$returning$returning$ WHERE z = 123 ReTuRnInG *")) + .hasReturningClause()); } private void assertUnclosedLiteral(String sql) { From 218c3b7941eba748379f3dec9179146ea53551e7 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Thu, 25 Aug 2022 17:08:39 +0000 Subject: [PATCH 25/33] feat: method doc update --- .../google/cloud/spanner/connection/AbstractStatementParser.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index 93c11a172b3..1fdd02ada5b 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -549,7 +549,6 @@ static int countOccurrencesOf(char c, String string) { * * @param sql The sql string without comments that has to be evaluated. * @return A boolean indicating whether the sql string has a Returning clause or not. - * @throws SpannerException If the input sql string contains an unclosed string/byte literal. */ @InternalApi abstract boolean checkReturningClauseInternal(String sql); From ea4152fe0fe89581d8cd0eacbc95769bedbd045d Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Fri, 26 Aug 2022 09:46:53 +0000 Subject: [PATCH 26/33] test: nit fixes --- .../cloud/spanner/connection/AbstractStatementParser.java | 2 +- .../spanner/connection/PostgreSQLStatementParser.java | 5 +++-- .../cloud/spanner/connection/SpannerStatementParser.java | 8 +++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index 1fdd02ada5b..b649adfb2ac 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -551,7 +551,7 @@ static int countOccurrencesOf(char c, String string) { * @return A boolean indicating whether the sql string has a Returning clause or not. */ @InternalApi - abstract boolean checkReturningClauseInternal(String sql); + abstract protected boolean checkReturningClauseInternal(String sql); @InternalApi public boolean checkReturningClause(String sql) { diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java index fe0c9cc88ba..3c854ab515b 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java @@ -32,6 +32,7 @@ public class PostgreSQLStatementParser extends AbstractStatementParser { private static final Pattern RETURNING_PATTERN = Pattern.compile("[ ')\"]returning[ '(\"]"); private static final Pattern AS_RETURNING_PATTERN = Pattern.compile("[ ')\"]as returning[ '(\"]"); + private static final String RETURNING_STRING = "returning"; PostgreSQLStatementParser() throws CompileException { super( @@ -305,12 +306,12 @@ private boolean isReturning(String sql, int index) { @InternalApi @Override - boolean checkReturningClauseInternal(String rawSql) { + protected boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); String sql = rawSql.toLowerCase(); // Do a pre-check to check if the SQL string definitely does not have a returning clause. // If this check fails, do a more involved check to check for a returning clause. - if (!sql.contains("returning")) { + if (!sql.contains(RETURNING_STRING)) { return false; } sql = sql.replaceAll("\\s+", " "); diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java index 9d2489fa5d6..b7247918821 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java @@ -30,7 +30,9 @@ @InternalApi public class SpannerStatementParser extends AbstractStatementParser { - final Pattern THEN_RETURN_PATTERN = Pattern.compile("[ `')\"]then return[ `'(\"]"); + private static final Pattern THEN_RETURN_PATTERN = Pattern.compile("[ `')\"]then return[ `'(\"]"); + private static final String THEN_STRING = "then"; + private static final String RETURN_STRING = "return"; public SpannerStatementParser() throws CompileException { super( @@ -280,12 +282,12 @@ private boolean isReturning(String sql, int index) { @InternalApi @Override - boolean checkReturningClauseInternal(String rawSql) { + protected boolean checkReturningClauseInternal(String rawSql) { Preconditions.checkNotNull(rawSql); String sql = rawSql.toLowerCase(); // Do a pre-check to check if the SQL string definitely does not have a returning clause. // If this check fails, do a more involved check to check for a returning clause. - if (!(sql.contains("then") && sql.contains("return"))) { + if (!(sql.contains(THEN_STRING) && sql.contains(RETURN_STRING))) { return false; } sql = sql.replaceAll("\\s+", " "); From e2a463e4f91c602cd4a237634f557bcf4793de21 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 30 Aug 2022 05:02:39 +0000 Subject: [PATCH 27/33] feat: handle another corner case --- .../connection/AbstractStatementParser.java | 6 ++- .../connection/PostgreSQLStatementParser.java | 53 ++++++++++++++++--- .../connection/SpannerStatementParser.java | 3 +- .../connection/StatementParserTest.java | 16 ++++++ 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java index b649adfb2ac..b0ae8863e6f 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java @@ -480,6 +480,10 @@ private boolean statementStartsWith(String sql, Iterable checkStatements static final char SLASH = '/'; static final char ASTERISK = '*'; static final char DOLLAR = '$'; + static final char SPACE = ' '; + static final char CLOSE_PARENTHESIS = ')'; + static final char COMMA = ','; + static final char UNDERSCORE = '_'; /** * Removes comments from and trims the given sql statement using the dialect of this parser. @@ -551,7 +555,7 @@ static int countOccurrencesOf(char c, String string) { * @return A boolean indicating whether the sql string has a Returning clause or not. */ @InternalApi - abstract protected boolean checkReturningClauseInternal(String sql); + protected abstract boolean checkReturningClauseInternal(String sql); @InternalApi public boolean checkReturningClause(String sql) { diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java index 3c854ab515b..012bfbba875 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java @@ -30,7 +30,7 @@ @InternalApi public class PostgreSQLStatementParser extends AbstractStatementParser { - private static final Pattern RETURNING_PATTERN = Pattern.compile("[ ')\"]returning[ '(\"]"); + private static final Pattern RETURNING_PATTERN = Pattern.compile("returning[ '(\"*]"); private static final Pattern AS_RETURNING_PATTERN = Pattern.compile("[ ')\"]as returning[ '(\"]"); private static final String RETURNING_STRING = "returning"; @@ -144,7 +144,7 @@ String parseDollarQuotedString(String sql, int index) { if (c == DOLLAR) { return tag.toString(); } - if (!Character.isJavaIdentifierPart(c)) { + if (!isValidIdentifierChar(c)) { break; } tag.append(c); @@ -292,16 +292,55 @@ private void appendIfNotNull( } } + private boolean isValidIdentifierFirstChar(char c) { + return Character.isLetter(c) || c == UNDERSCORE; + } + + private boolean isValidIdentifierChar(char c) { + return isValidIdentifierFirstChar(c) || Character.isDigit(c) || c == DOLLAR; + } + + private boolean checkCharPrecedingReturning(char ch) { + return (ch == SPACE) + || (ch == SINGLE_QUOTE) + || (ch == CLOSE_PARENTHESIS) + || (ch == DOUBLE_QUOTE) + || (ch == DOLLAR); + } + + private boolean checkCharPrecedingSubstrWithReturning(char ch) { + return (ch == SPACE) + || (ch == SINGLE_QUOTE) + || (ch == CLOSE_PARENTHESIS) + || (ch == DOUBLE_QUOTE) + || (ch == COMMA); + } + private boolean isReturning(String sql, int index) { // RETURNING is a reserved keyword in PG, but requires a // leading AS to be used as column label, to avoid ambiguity. // We thus check for cases which do not have a leading AS. // (https://www.postgresql.org/docs/current/sql-keywords-appendix.html) - return (index >= 1) - && (index + 10 <= sql.length()) - && RETURNING_PATTERN.matcher(sql.substring(index - 1, index + 10)).matches() - && !((index >= 4) - && AS_RETURNING_PATTERN.matcher(sql.substring(index - 4, index + 10)).matches()); + if (index >= 1) { + if (((index + 10 <= sql.length()) + && RETURNING_PATTERN.matcher(sql.substring(index, index + 10)).matches() + && !((index >= 4) + && AS_RETURNING_PATTERN.matcher(sql.substring(index - 4, index + 10)).matches()))) { + if (checkCharPrecedingReturning(sql.charAt(index - 1))) { + return true; + } + // Check for cases where returning clause is part of a substring which starts with an + // invalid first character of an identifier. + // For example, + // insert into t select 2returning *; + int ind = index - 1; + while ((ind >= 0) && !checkCharPrecedingSubstrWithReturning(sql.charAt(ind))) { + ind--; + } + return !isValidIdentifierFirstChar(sql.charAt(ind + 1)); + } + } + return false; } @InternalApi diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java index b7247918821..a9ca5fb9726 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java @@ -30,7 +30,8 @@ @InternalApi public class SpannerStatementParser extends AbstractStatementParser { - private static final Pattern THEN_RETURN_PATTERN = Pattern.compile("[ `')\"]then return[ `'(\"]"); + private static final Pattern THEN_RETURN_PATTERN = + Pattern.compile("[ `')\"]then return[ *`'(\"]"); private static final String THEN_STRING = "then"; private static final String RETURN_STRING = "return"; diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index 3923c9821c5..c56ca8b5a08 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -1360,6 +1360,10 @@ public void testGoogleSQLReturningClause() { parser .parse(Statement.of("insert into x (a,b) values (1,2)thenreturn*")) .hasReturningClause()); + assertTrue( + parser + .parse(Statement.of("insert into t(a) select \"x\"then return*")) + .hasReturningClause()); } @Test @@ -1455,6 +1459,18 @@ public void testPostgreSQLReturningClause() { Statement.of( "UPDATE x SET y = $returning$returning$returning$ WHERE z = 123 ReTuRnInG *")) .hasReturningClause()); + assertTrue( + parser.parse(Statement.of("insert into t1 select 1 returning*")).hasReturningClause()); + assertTrue( + parser.parse(Statement.of("insert into t1 select 2returning*")).hasReturningClause()); + assertTrue( + parser.parse(Statement.of("insert into t1 select 10e2returning*")).hasReturningClause()); + assertFalse( + parser + .parse(Statement.of("insert into t1 select 'test''returning *'")) + .hasReturningClause()); + assertTrue( + parser.parse(Statement.of("insert into t select 2,3returning*")).hasReturningClause()); } private void assertUnclosedLiteral(String sql) { From 60fcb4cdfb47fdc64924bff914e35d5cd454aa3d Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Tue, 30 Aug 2022 08:53:54 +0000 Subject: [PATCH 28/33] test: add another test --- .../google/cloud/spanner/connection/StatementParserTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index c56ca8b5a08..56760d900af 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -1471,6 +1471,8 @@ public void testPostgreSQLReturningClause() { .hasReturningClause()); assertTrue( parser.parse(Statement.of("insert into t select 2,3returning*")).hasReturningClause()); + assertTrue( + parser.parse(Statement.of("insert into t1 select 10.returning*")).hasReturningClause()); } private void assertUnclosedLiteral(String sql) { From 58d02789876cb9bc3f4b18247f24e1b2d17f604b Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 9 Nov 2022 05:55:37 +0000 Subject: [PATCH 29/33] clirr fixes --- google-cloud-spanner/clirr-ignored-differences.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/google-cloud-spanner/clirr-ignored-differences.xml b/google-cloud-spanner/clirr-ignored-differences.xml index 039278a6436..adda4c3c854 100644 --- a/google-cloud-spanner/clirr-ignored-differences.xml +++ b/google-cloud-spanner/clirr-ignored-differences.xml @@ -202,4 +202,9 @@ com/google/cloud/spanner/DatabaseClient java.lang.String getDatabaseRole() + + 7013 + com/google/cloud/spanner/connection/AbstractStatementParser + boolean checkReturningClauseInternal(java.lang.String) + From 81b7908f262a508835c02a4958fb8f13f8e1cdca Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 9 Nov 2022 06:10:56 +0000 Subject: [PATCH 30/33] revert env for integration tests --- .../java/com/google/cloud/spanner/GceTestEnvConfig.java | 6 ++---- .../java/com/google/cloud/spanner/IntegrationTestEnv.java | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java index 30db3e09f73..71636a673b8 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java @@ -59,10 +59,8 @@ public class GceTestEnvConfig implements TestEnvConfig { public GceTestEnvConfig() { // TODO: Revert back any changes to this file before merging. - // String projectId = System.getProperty(GCE_PROJECT_ID, ""); - String projectId = "span-cloud-testing"; - // String serverUrl = System.getProperty(GCE_SERVER_URL, ""); - String serverUrl = "https://staging-wrenchworks.sandbox.googleapis.com"; + String projectId = System.getProperty(GCE_PROJECT_ID, ""); + String serverUrl = System.getProperty(GCE_SERVER_URL, ""); String credentialsFile = System.getProperty(GCE_CREDENTIALS_FILE, ""); double errorProbability = Double.parseDouble(System.getProperty(GCE_STREAM_BROKEN_PROBABILITY, "0.0")); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java index d2596b78c6b..460b1000cee 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java @@ -91,9 +91,7 @@ protected void before() throws Throwable { this.config.setUp(); SpannerOptions options = config.spannerOptions(); - // TODO: Revert back any changes to this file before merging. - // String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); - String instanceProperty = "projects/span-cloud-testing/instances/rajatrb-test-instance"; + String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); InstanceId instanceId; if (!instanceProperty.isEmpty() && !alwaysCreateNewInstance) { instanceId = InstanceId.of(instanceProperty); From d0e173d8110857b9800c3f29b8a6b910f1b67066 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 9 Nov 2022 06:14:16 +0000 Subject: [PATCH 31/33] remove comments --- .../src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java index 71636a673b8..ab8e3933f9f 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java @@ -58,7 +58,6 @@ public class GceTestEnvConfig implements TestEnvConfig { private final SpannerOptions options; public GceTestEnvConfig() { - // TODO: Revert back any changes to this file before merging. String projectId = System.getProperty(GCE_PROJECT_ID, ""); String serverUrl = System.getProperty(GCE_SERVER_URL, ""); String credentialsFile = System.getProperty(GCE_CREDENTIALS_FILE, ""); From 9e151592b8ab50125aae858243fed8677f71fc5a Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 9 Nov 2022 08:36:19 +0000 Subject: [PATCH 32/33] skip returning tests in emulator --- .../cloud/spanner/connection/it/ITDmlReturningTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index 3a7a599cb8a..22d17394ca0 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeFalse; import com.google.cloud.spanner.AsyncResultSet; import com.google.cloud.spanner.AsyncResultSet.CallbackResponse; @@ -38,6 +39,7 @@ import com.google.cloud.spanner.connection.StatementResult; import com.google.cloud.spanner.connection.StatementResult.ResultType; import com.google.cloud.spanner.connection.TransactionMode; +import com.google.cloud.spanner.testing.EmulatorSpannerHelper; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.ArrayList; @@ -49,6 +51,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; import org.junit.runner.RunWith; @@ -107,6 +110,8 @@ private boolean checkAndSetInitialized() { @Before public void setupTable() { + assumeFalse( + "DML Returning is not supported in the emulator", EmulatorSpannerHelper.isUsingEmulator()); if (checkAndSetInitialized()) { database = env.getTestHelper() From ac3ea4fc6218a3f045b0aeb8b7f03c1b47a5d931 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Wed, 9 Nov 2022 08:41:42 +0000 Subject: [PATCH 33/33] fix: linting --- .../google/cloud/spanner/connection/it/ITDmlReturningTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java index 22d17394ca0..5e5ca800402 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/it/ITDmlReturningTest.java @@ -51,7 +51,6 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; import org.junit.runner.RunWith;