From 4ab6017ec3e2b77b311700ed2b79877af77efea4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Wed, 13 Mar 2024 14:41:59 +0100 Subject: [PATCH 1/5] refactor: move skip methods to abstract parser Move the PostgreSQL skip methods from the PostgreSQL parser to the abstract parser. This is step 1 in refactoring the GoogleSQL and PostgreSQL parser so they can share more code. The eventual goal is to allow the GoogleSQL parser to be able to handle SQL string without having to remove the comments from the string first. --- .../connection/AbstractStatementParser.java | 166 ++++++++++++++++++ .../connection/PostgreSQLStatementParser.java | 147 ---------------- 2 files changed, 166 insertions(+), 147 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 8fc3043791..ac984a0f86 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 @@ -40,6 +40,7 @@ import java.util.concurrent.Callable; import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.Nullable; /** * Internal class for the Spanner Connection API. @@ -696,4 +697,169 @@ static int countOccurrencesOf(char c, String string) { public boolean checkReturningClause(String sql) { return checkReturningClauseInternal(sql); } + + /** + * Returns true for characters that can be used as the first character in unquoted identifiers. + */ + boolean isValidIdentifierFirstChar(char c) { + return Character.isLetter(c) || c == UNDERSCORE; + } + + /** Returns true for characters that can be used in unquoted identifiers. */ + boolean isValidIdentifierChar(char c) { + return isValidIdentifierFirstChar(c) || Character.isDigit(c) || c == DOLLAR; + } + + /** Reads a dollar-quoted string literal from position index in the given sql string. */ + String parseDollarQuotedString(String sql, int index) { + // Look ahead to the next dollar sign (if any). Everything in between is the quote tag. + StringBuilder tag = new StringBuilder(); + while (index < sql.length()) { + char c = sql.charAt(index); + if (c == DOLLAR) { + return tag.toString(); + } + if (!isValidIdentifierChar(c)) { + break; + } + tag.append(c); + index++; + } + return null; + } + + /** + * Skips the next character, literal, identifier, or comment in the given sql string from the + * given index. The skipped characters are added to result if it is not null. + */ + int skip(String sql, int currentIndex, @Nullable StringBuilder result) { + char currentChar = sql.charAt(currentIndex); + if (currentChar == SINGLE_QUOTE || currentChar == DOUBLE_QUOTE) { + appendIfNotNull(result, currentChar); + return skipQuoted(sql, currentIndex, currentChar, result); + } else if (currentChar == DOLLAR) { + String dollarTag = parseDollarQuotedString(sql, currentIndex + 1); + if (dollarTag != null) { + appendIfNotNull(result, currentChar, dollarTag, currentChar); + return skipQuoted( + sql, currentIndex + dollarTag.length() + 1, currentChar, dollarTag, result); + } + } else if (currentChar == HYPHEN + && sql.length() > (currentIndex + 1) + && sql.charAt(currentIndex + 1) == HYPHEN) { + return skipSingleLineComment(sql, currentIndex, result); + } else if (currentChar == SLASH + && sql.length() > (currentIndex + 1) + && sql.charAt(currentIndex + 1) == ASTERISK) { + return skipMultiLineComment(sql, currentIndex, result); + } + + appendIfNotNull(result, currentChar); + return currentIndex + 1; + } + + /** Skips a single-line comment from startIndex and adds it to result if result is not null. */ + static int skipSingleLineComment(String sql, int startIndex, @Nullable StringBuilder result) { + int endIndex = sql.indexOf('\n', startIndex + 2); + if (endIndex == -1) { + endIndex = sql.length(); + } else { + // Include the newline character. + endIndex++; + } + appendIfNotNull(result, sql.substring(startIndex, endIndex)); + return endIndex; + } + + /** Skips a multi-line comment from startIndex and adds it to result if result is not null. */ + static int skipMultiLineComment(String sql, int startIndex, @Nullable StringBuilder result) { + // Current position is start + '/*'.length(). + int pos = startIndex + 2; + // PostgreSQL allows comments to be nested. That is, the following is allowed: + // '/* test /* inner comment */ still a comment */' + int level = 1; + while (pos < sql.length()) { + if (sql.charAt(pos) == SLASH && sql.length() > (pos + 1) && sql.charAt(pos + 1) == ASTERISK) { + level++; + } + if (sql.charAt(pos) == ASTERISK && sql.length() > (pos + 1) && sql.charAt(pos + 1) == SLASH) { + level--; + if (level == 0) { + pos += 2; + appendIfNotNull(result, sql.substring(startIndex, pos)); + return pos; + } + } + pos++; + } + appendIfNotNull(result, sql.substring(startIndex)); + return sql.length(); + } + + /** Skips a quoted string from startIndex. */ + private int skipQuoted( + String sql, int startIndex, char startQuote, @Nullable StringBuilder result) { + return skipQuoted(sql, startIndex, startQuote, null, result); + } + + /** + * Skips a quoted string from startIndex. The quote character is assumed to be $ if dollarTag is + * not null. + */ + private int skipQuoted( + String sql, + int startIndex, + char startQuote, + String dollarTag, + @Nullable StringBuilder result) { + int currentIndex = startIndex + 1; + while (currentIndex < sql.length()) { + char currentChar = sql.charAt(currentIndex); + if (currentChar == startQuote) { + if (currentChar == DOLLAR) { + // Check if this is the end of the current dollar quoted string. + String tag = parseDollarQuotedString(sql, currentIndex + 1); + if (tag != null && tag.equals(dollarTag)) { + appendIfNotNull(result, currentChar, dollarTag, currentChar); + return currentIndex + tag.length() + 2; + } + } else if (sql.length() > currentIndex + 1 && sql.charAt(currentIndex + 1) == startQuote) { + // This is an escaped quote (e.g. 'foo''bar') + appendIfNotNull(result, currentChar); + appendIfNotNull(result, currentChar); + currentIndex += 2; + continue; + } else { + appendIfNotNull(result, currentChar); + return currentIndex + 1; + } + } + currentIndex++; + appendIfNotNull(result, currentChar); + } + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); + } + + /** Appends the given character to result if result is not null. */ + private void appendIfNotNull(@Nullable StringBuilder result, char currentChar) { + if (result != null) { + result.append(currentChar); + } + } + + /** Appends the given suffix to result if result is not null. */ + private static void appendIfNotNull(@Nullable StringBuilder result, String suffix) { + if (result != null) { + result.append(suffix); + } + } + + /** Appends the given prefix, tag, and suffix to result if result is not null. */ + private static void appendIfNotNull( + @Nullable StringBuilder result, char prefix, String tag, char suffix) { + if (result != null) { + result.append(prefix).append(tag).append(suffix); + } + } } 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 8cb2b7e464..572ea05654 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 @@ -26,7 +26,6 @@ import java.util.HashSet; import java.util.Set; import java.util.regex.Pattern; -import javax.annotation.Nullable; @InternalApi public class PostgreSQLStatementParser extends AbstractStatementParser { @@ -136,23 +135,6 @@ String removeCommentsAndTrimInternal(String sql) { return res.toString().trim(); } - String parseDollarQuotedString(String sql, int index) { - // Look ahead to the next dollar sign (if any). Everything in between is the quote tag. - StringBuilder tag = new StringBuilder(); - while (index < sql.length()) { - char c = sql.charAt(index); - if (c == DOLLAR) { - return tag.toString(); - } - if (!isValidIdentifierChar(c)) { - break; - } - tag.append(c); - index++; - } - return null; - } - /** PostgreSQL does not support statement hints. */ @Override String removeStatementHint(String sql) { @@ -220,135 +202,6 @@ public Set getQueryParameters(String sql) { return parameters; } - private int skip(String sql, int currentIndex, @Nullable StringBuilder result) { - char currentChar = sql.charAt(currentIndex); - if (currentChar == SINGLE_QUOTE || currentChar == DOUBLE_QUOTE) { - appendIfNotNull(result, currentChar); - return skipQuoted(sql, currentIndex, currentChar, result); - } else if (currentChar == DOLLAR) { - String dollarTag = parseDollarQuotedString(sql, currentIndex + 1); - if (dollarTag != null) { - appendIfNotNull(result, currentChar, dollarTag, currentChar); - return skipQuoted( - sql, currentIndex + dollarTag.length() + 1, currentChar, dollarTag, result); - } - } else if (currentChar == HYPHEN - && sql.length() > (currentIndex + 1) - && sql.charAt(currentIndex + 1) == HYPHEN) { - return skipSingleLineComment(sql, currentIndex, result); - } else if (currentChar == SLASH - && sql.length() > (currentIndex + 1) - && sql.charAt(currentIndex + 1) == ASTERISK) { - return skipMultiLineComment(sql, currentIndex, result); - } - - appendIfNotNull(result, currentChar); - return currentIndex + 1; - } - - static int skipSingleLineComment(String sql, int currentIndex, @Nullable StringBuilder result) { - int endIndex = sql.indexOf('\n', currentIndex + 2); - if (endIndex == -1) { - endIndex = sql.length(); - } else { - // Include the newline character. - endIndex++; - } - appendIfNotNull(result, sql.substring(currentIndex, endIndex)); - return endIndex; - } - - static int skipMultiLineComment(String sql, int startIndex, @Nullable StringBuilder result) { - // Current position is start + '/*'.length(). - int pos = startIndex + 2; - // PostgreSQL allows comments to be nested. That is, the following is allowed: - // '/* test /* inner comment */ still a comment */' - int level = 1; - while (pos < sql.length()) { - if (sql.charAt(pos) == SLASH && sql.length() > (pos + 1) && sql.charAt(pos + 1) == ASTERISK) { - level++; - } - if (sql.charAt(pos) == ASTERISK && sql.length() > (pos + 1) && sql.charAt(pos + 1) == SLASH) { - level--; - if (level == 0) { - pos += 2; - appendIfNotNull(result, sql.substring(startIndex, pos)); - return pos; - } - } - pos++; - } - appendIfNotNull(result, sql.substring(startIndex)); - return sql.length(); - } - - private int skipQuoted( - String sql, int startIndex, char startQuote, @Nullable StringBuilder result) { - return skipQuoted(sql, startIndex, startQuote, null, result); - } - - private int skipQuoted( - String sql, - int startIndex, - char startQuote, - String dollarTag, - @Nullable StringBuilder result) { - int currentIndex = startIndex + 1; - while (currentIndex < sql.length()) { - char currentChar = sql.charAt(currentIndex); - if (currentChar == startQuote) { - if (currentChar == DOLLAR) { - // Check if this is the end of the current dollar quoted string. - String tag = parseDollarQuotedString(sql, currentIndex + 1); - if (tag != null && tag.equals(dollarTag)) { - appendIfNotNull(result, currentChar, dollarTag, currentChar); - return currentIndex + tag.length() + 2; - } - } else if (sql.length() > currentIndex + 1 && sql.charAt(currentIndex + 1) == startQuote) { - // This is an escaped quote (e.g. 'foo''bar') - appendIfNotNull(result, currentChar); - appendIfNotNull(result, currentChar); - currentIndex += 2; - continue; - } else { - appendIfNotNull(result, currentChar); - return currentIndex + 1; - } - } - currentIndex++; - appendIfNotNull(result, currentChar); - } - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } - - private void appendIfNotNull(@Nullable StringBuilder result, char currentChar) { - if (result != null) { - result.append(currentChar); - } - } - - private static void appendIfNotNull(@Nullable StringBuilder result, String suffix) { - if (result != null) { - result.append(suffix); - } - } - - private void appendIfNotNull( - @Nullable StringBuilder result, char prefix, String tag, char suffix) { - if (result != null) { - result.append(prefix).append(tag).append(suffix); - } - } - - 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) From 2d30e032163e7f967c2af0f239c73122649cdf31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Wed, 13 Mar 2024 15:18:52 +0100 Subject: [PATCH 2/5] refactor: generalize skip methods Generalize the various skip methods so these can be used for both dialects. Each dialect implements a number of abstract methods to indicate what type of statements and constructs they support. These methods are used by the generalized skip methods to determine the start and end of literals, identifiers, and comments. --- .../connection/AbstractStatementParser.java | 120 ++++++++++++++++-- .../connection/PostgreSQLStatementParser.java | 40 ++++++ .../connection/SpannerStatementParser.java | 40 ++++++ .../SpannerStatementParserTest.java | 83 ++++++++++++ .../connection/StatementParserTest.java | 4 +- 5 files changed, 276 insertions(+), 11 deletions(-) create mode 100644 google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.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 ac984a0f86..0382757a80 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 @@ -595,6 +595,7 @@ private boolean statementStartsWith(String sql, Iterable checkStatements static final char CLOSE_PARENTHESIS = ')'; static final char COMMA = ','; static final char UNDERSCORE = '_'; + static final char BACKSLASH = '\\'; /** * Removes comments from and trims the given sql statement using the dialect of this parser. @@ -698,6 +699,62 @@ public boolean checkReturningClause(String sql) { return checkReturningClauseInternal(sql); } + /** + * Returns true if this dialect supports nested comments. + * + *
    + *
  • This method should return false for dialects that consider this to be a valid comment: + * /* A comment /* still a comment */. + *
  • This method should return true for dialects that require all comment start sequences to + * be balanced with a comment end sequence: + * /* A comment /* still a comment */ Also still a comment */. + *
+ */ + abstract boolean supportsNestedComments(); + + /** + * Returns true for dialects that support dollar-quoted string literals. + * + *

Example: $tag$This is a string$tag$. + */ + abstract boolean supportsDollarQuotedStrings(); + + /** + * Returns true for dialects that support backticks as a quoting character, either for string + * literals or identifiers. + */ + abstract boolean supportsBacktickQuote(); + + /** + * Returns true for dialects that support triple-quoted string literals and identifiers. + * + *

Example: ```This is a triple-quoted string``` + */ + abstract boolean supportsTripleQuotedStrings(); + + /** + * Returns true if the dialect supports escaping a quote character within a literal with the same + * quote as the literal is using. That is: 'foo''bar' means "foo'bar". + */ + abstract boolean supportsEscapeQuoteWithQuote(); + + /** Returns true if the dialect supports starting an escape sequence with a backslash. */ + abstract boolean supportsBackslashEscape(); + + /** + * Returns true if the dialect supports single-line comments that start with a dash. + * + *

Example: # This is a comment + */ + abstract boolean supportsHashSingleLineComments(); + + /** + * Returns true for dialects that allow line-feeds in quoted strings. Note that the return value + * of this is not used for triple-quoted strings. Triple-quoted strings are assumed to always + * support line-feeds. + */ + abstract boolean supportsLineFeedInQuotedString(); + /** * Returns true for characters that can be used as the first character in unquoted identifiers. */ @@ -733,11 +790,17 @@ String parseDollarQuotedString(String sql, int index) { * given index. The skipped characters are added to result if it is not null. */ int skip(String sql, int currentIndex, @Nullable StringBuilder result) { + if (currentIndex >= sql.length()) { + return currentIndex; + } char currentChar = sql.charAt(currentIndex); - if (currentChar == SINGLE_QUOTE || currentChar == DOUBLE_QUOTE) { + + if (currentChar == SINGLE_QUOTE + || currentChar == DOUBLE_QUOTE + || (supportsBacktickQuote() && currentChar == BACKTICK_QUOTE)) { appendIfNotNull(result, currentChar); return skipQuoted(sql, currentIndex, currentChar, result); - } else if (currentChar == DOLLAR) { + } else if (supportsDollarQuotedStrings() && currentChar == DOLLAR) { String dollarTag = parseDollarQuotedString(sql, currentIndex + 1); if (dollarTag != null) { appendIfNotNull(result, currentChar, dollarTag, currentChar); @@ -748,6 +811,8 @@ int skip(String sql, int currentIndex, @Nullable StringBuilder result) { && sql.length() > (currentIndex + 1) && sql.charAt(currentIndex + 1) == HYPHEN) { return skipSingleLineComment(sql, currentIndex, result); + } else if (currentChar == DASH && supportsHashSingleLineComments()) { + return skipSingleLineComment(sql, currentIndex, result); } else if (currentChar == SLASH && sql.length() > (currentIndex + 1) && sql.charAt(currentIndex + 1) == ASTERISK) { @@ -772,14 +837,17 @@ static int skipSingleLineComment(String sql, int startIndex, @Nullable StringBui } /** Skips a multi-line comment from startIndex and adds it to result if result is not null. */ - static int skipMultiLineComment(String sql, int startIndex, @Nullable StringBuilder result) { + int skipMultiLineComment(String sql, int startIndex, @Nullable StringBuilder result) { // Current position is start + '/*'.length(). int pos = startIndex + 2; // PostgreSQL allows comments to be nested. That is, the following is allowed: // '/* test /* inner comment */ still a comment */' int level = 1; while (pos < sql.length()) { - if (sql.charAt(pos) == SLASH && sql.length() > (pos + 1) && sql.charAt(pos + 1) == ASTERISK) { + if (supportsNestedComments() + && sql.charAt(pos) == SLASH + && sql.length() > (pos + 1) + && sql.charAt(pos + 1) == ASTERISK) { level++; } if (sql.charAt(pos) == ASTERISK && sql.length() > (pos + 1) && sql.charAt(pos + 1) == SLASH) { @@ -806,33 +874,67 @@ private int skipQuoted( * Skips a quoted string from startIndex. The quote character is assumed to be $ if dollarTag is * not null. */ - private int skipQuoted( + int skipQuoted( String sql, int startIndex, char startQuote, - String dollarTag, + @Nullable String dollarTag, @Nullable StringBuilder result) { - int currentIndex = startIndex + 1; + boolean isTripleQuoted = + supportsTripleQuotedStrings() + && sql.length() > startIndex + 2 + && sql.charAt(startIndex + 1) == startQuote + && sql.charAt(startIndex + 2) == startQuote; + int currentIndex = startIndex + (isTripleQuoted ? 3 : 1); + if (isTripleQuoted) { + appendIfNotNull(result, startQuote); + appendIfNotNull(result, startQuote); + } while (currentIndex < sql.length()) { char currentChar = sql.charAt(currentIndex); if (currentChar == startQuote) { - if (currentChar == DOLLAR) { + if (supportsDollarQuotedStrings() && currentChar == DOLLAR) { // Check if this is the end of the current dollar quoted string. String tag = parseDollarQuotedString(sql, currentIndex + 1); if (tag != null && tag.equals(dollarTag)) { appendIfNotNull(result, currentChar, dollarTag, currentChar); return currentIndex + tag.length() + 2; } - } else if (sql.length() > currentIndex + 1 && sql.charAt(currentIndex + 1) == startQuote) { + } else if (supportsEscapeQuoteWithQuote() + && sql.length() > currentIndex + 1 + && sql.charAt(currentIndex + 1) == startQuote) { // This is an escaped quote (e.g. 'foo''bar') appendIfNotNull(result, currentChar); appendIfNotNull(result, currentChar); currentIndex += 2; continue; + } else if (isTripleQuoted) { + // Check if this is the end of the triple-quoted string. + if (sql.length() > currentIndex + 2 + && sql.charAt(currentIndex + 1) == startQuote + && sql.charAt(currentIndex + 2) == startQuote) { + appendIfNotNull(result, currentChar); + appendIfNotNull(result, currentChar); + appendIfNotNull(result, currentChar); + return currentIndex + 3; + } } else { appendIfNotNull(result, currentChar); return currentIndex + 1; } + } else if (supportsBackslashEscape() + && currentChar == BACKSLASH + && sql.length() > currentIndex + 1 + && sql.charAt(currentIndex + 1) == startQuote) { + // This is an escaped quote (e.g. 'foo\'bar'). + // Note that in raw strings, the \ officially does not start an escape sequence, but the + // result is still the same, as in a raw string 'both characters are preserved'. + appendIfNotNull(result, currentChar); + appendIfNotNull(result, sql.charAt(currentIndex + 1)); + currentIndex += 2; + continue; + } else if (currentChar == '\n' && !isTripleQuoted && !supportsLineFeedInQuotedString()) { + break; } currentIndex++; appendIfNotNull(result, currentChar); 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 572ea05654..6b0c69d40a 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 @@ -48,6 +48,46 @@ protected boolean supportsExplain() { return false; } + @Override + boolean supportsNestedComments() { + return true; + } + + @Override + boolean supportsDollarQuotedStrings() { + return true; + } + + @Override + boolean supportsBacktickQuote() { + return false; + } + + @Override + boolean supportsTripleQuotedStrings() { + return false; + } + + @Override + boolean supportsEscapeQuoteWithQuote() { + return true; + } + + @Override + boolean supportsBackslashEscape() { + return false; + } + + @Override + boolean supportsHashSingleLineComments() { + return false; + } + + @Override + boolean supportsLineFeedInQuotedString() { + return true; + } + /** * Removes comments from and trims the given sql statement. PostgreSQL supports two types of * comments: 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 251c5a2e6e..1c5cdda7b0 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 @@ -50,6 +50,46 @@ protected boolean supportsExplain() { return true; } + @Override + boolean supportsNestedComments() { + return false; + } + + @Override + boolean supportsDollarQuotedStrings() { + return false; + } + + @Override + boolean supportsBacktickQuote() { + return true; + } + + @Override + boolean supportsTripleQuotedStrings() { + return true; + } + + @Override + boolean supportsEscapeQuoteWithQuote() { + return false; + } + + @Override + boolean supportsBackslashEscape() { + return true; + } + + @Override + boolean supportsHashSingleLineComments() { + return true; + } + + @Override + boolean supportsLineFeedInQuotedString() { + return false; + } + /** * Removes comments from and trims the given sql statement. Spanner supports three types of * comments: diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java new file mode 100644 index 0000000000..d4dc76d48b --- /dev/null +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java @@ -0,0 +1,83 @@ +/* + * Copyright 2024 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; + +import static org.junit.Assert.assertEquals; + +import com.google.cloud.spanner.Dialect; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class SpannerStatementParserTest { + + static String skip(String sql) { + return skip(sql, 0); + } + + static String skip(String sql, int currentIndex) { + int position = + AbstractStatementParser.getInstance(Dialect.GOOGLE_STANDARD_SQL) + .skip(sql, currentIndex, null); + return sql.substring(currentIndex, position); + } + + @Test + public void testSkip() { + assertEquals("", skip("")); + assertEquals("1", skip("1 ")); + assertEquals("1", skip("12 ")); + assertEquals("2", skip("12 ", 1)); + assertEquals("", skip("12", 2)); + + assertEquals("'foo'", skip("'foo' ", 0)); + assertEquals("'foo'", skip("'foo''bar' ", 0)); + assertEquals("'foo'", skip("'foo' 'bar' ", 0)); + assertEquals("'bar'", skip("'foo''bar' ", 5)); + assertEquals("'foo\"bar\"'", skip("'foo\"bar\"' ", 0)); + assertEquals("\"foo'bar'\"", skip("\"foo'bar'\" ", 0)); + assertEquals("`foo'bar'`", skip("`foo'bar'` ", 0)); + + assertEquals("'''foo'bar'''", skip("'''foo'bar''' ", 0)); + assertEquals("'''foo\\'bar'''", skip("'''foo\\'bar''' ", 0)); + assertEquals("'''foo\\'\\'bar'''", skip("'''foo\\'\\'bar''' ", 0)); + assertEquals("'''foo\\'\\'\\'bar'''", skip("'''foo\\'\\'\\'bar''' ", 0)); + assertEquals("\"\"\"foo'bar\"\"\"", skip("\"\"\"foo'bar\"\"\"", 0)); + assertEquals("```foo'bar```", skip("```foo'bar```", 0)); + + assertEquals("-- comment\n", skip("-- comment\nselect * from foo", 0)); + assertEquals("# comment\n", skip("# comment\nselect * from foo", 0)); + assertEquals("/* comment */", skip("/* comment */ select * from foo", 0)); + assertEquals( + "/* comment /* GoogleSQL does not support nested comments */", + skip("/* comment /* GoogleSQL does not support nested comments */ select * from foo", 0)); + // GoogleSQL does not support dollar-quoted strings. + assertEquals("$", skip("$tag$not a string$tag$ select * from foo", 0)); + + assertEquals("/* 'test' */", skip("/* 'test' */ foo")); + assertEquals("-- 'test' \n", skip("-- 'test' \n foo")); + assertEquals("'/* test */'", skip("'/* test */' foo")); + + // Raw strings do not consider '\' as something that starts an escape sequence, but any + // quote character following it is still preserved within the string, as the definition of a + // raw string says that 'both characters are preserved'. + assertEquals("'foo\\''", skip("'foo\\'' ", 0)); + assertEquals("'foo\\''", skip("r'foo\\'' ", 1)); + assertEquals("'''foo\\'\\'\\'bar'''", skip("'''foo\\'\\'\\'bar''' ", 0)); + } +} 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 d3438b2b66..c60550c3ba 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 @@ -1600,11 +1600,11 @@ public void testPostgreSQLReturningClause() { } int skipSingleLineComment(String sql, int startIndex) { - return PostgreSQLStatementParser.skipSingleLineComment(sql, startIndex, null); + return AbstractStatementParser.skipSingleLineComment(sql, startIndex, null); } int skipMultiLineComment(String sql, int startIndex) { - return PostgreSQLStatementParser.skipMultiLineComment(sql, startIndex, null); + return parser.skipMultiLineComment(sql, startIndex, null); } @Test From 51ce4704280a70ad46c83c85e55784f45fcd8b53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 14 Mar 2024 14:49:43 +0100 Subject: [PATCH 3/5] perf: keep comments when searching for params Keep all comments in the SQL string in place when converting positional parameters to named parameters. This reduces the amount of string operations that are needed for each query that is executed, and also enables actually sending comments from the client to Spanner when using positional parameters (e.g. in JDBC). --- .../connection/AbstractStatementParser.java | 46 +-- .../connection/PostgreSQLStatementParser.java | 26 +- .../connection/SpannerStatementParser.java | 67 +--- .../SpannerStatementParserTest.java | 156 ++++++++ .../connection/StatementParserTest.java | 362 ++++++++---------- 5 files changed, 359 insertions(+), 298 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 0382757a80..dfe3c39c5d 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 @@ -634,36 +634,33 @@ public static class ParametersInfo { /** * Converts all positional parameters (?) in the given sql string into named parameters. The - * parameters are named @p1, @p2, etc. This method is used when converting a JDBC statement that - * uses positional parameters to a Cloud Spanner {@link Statement} instance that requires named - * parameters. The input SQL string may not contain any comments, except for PostgreSQL-dialect - * SQL strings. + * parameters are named @p1, @p2, etc. for GoogleSQL, and $1, $2, etc. for PostgreSQL. This method + * is used when converting a JDBC statement that uses positional parameters to a Cloud Spanner + * {@link Statement} instance that requires named parameters. * - * @param sql The sql string that should be converted - * @return A {@link ParametersInfo} object containing a string with named parameters instead of - * positional parameters and the number of parameters. - * @throws SpannerException If the input sql string contains an unclosed string/byte literal. - */ - @InternalApi - abstract ParametersInfo convertPositionalParametersToNamedParametersInternal( - char paramChar, String sql); - - /** - * Converts all positional parameters (?) in the given sql string into named parameters. The - * parameters are named @p1, @p2, etc. This method is used when converting a JDBC statement that - * uses positional parameters to a Cloud Spanner {@link Statement} instance that requires named - * parameters. The input SQL string may not contain any comments. There is an exception case if - * the statement starts with a GSQL comment which forces it to be interpreted as a GoogleSql - * statement. - * - * @param sql The sql string without comments that should be converted + * @param sql The sql string that should be converted to use named parameters * @return A {@link ParametersInfo} object containing a string with named parameters instead of * positional parameters and the number of parameters. * @throws SpannerException If the input sql string contains an unclosed string/byte literal. */ @InternalApi public ParametersInfo convertPositionalParametersToNamedParameters(char paramChar, String sql) { - return convertPositionalParametersToNamedParametersInternal(paramChar, sql); + Preconditions.checkNotNull(sql); + final String namedParamPrefix = getQueryParameterPrefix(); + StringBuilder named = new StringBuilder(sql.length() + countOccurrencesOf(paramChar, sql)); + int index = 0; + int paramIndex = 1; + while (index < sql.length()) { + char c = sql.charAt(index); + if (c == paramChar) { + named.append(namedParamPrefix).append(paramIndex); + paramIndex++; + index++; + } else { + index = skip(sql, index, named); + } + } + return new ParametersInfo(paramIndex - 1, named.toString()); } /** Convenience method that is used to estimate the number of parameters in a SQL statement. */ @@ -755,6 +752,9 @@ public boolean checkReturningClause(String sql) { */ abstract boolean supportsLineFeedInQuotedString(); + /** Returns the query parameter prefix that should be used for this dialect. */ + abstract String getQueryParameterPrefix(); + /** * Returns true for characters that can be used as the first character in unquoted identifiers. */ 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 6b0c69d40a..be4aa9d7f4 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 @@ -88,6 +88,11 @@ boolean supportsLineFeedInQuotedString() { return true; } + @Override + String getQueryParameterPrefix() { + return "$"; + } + /** * Removes comments from and trims the given sql statement. PostgreSQL supports two types of * comments: @@ -181,27 +186,6 @@ String removeStatementHint(String sql) { return sql; } - @InternalApi - @Override - ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramChar, String sql) { - Preconditions.checkNotNull(sql); - final String namedParamPrefix = "$"; - StringBuilder named = new StringBuilder(sql.length() + countOccurrencesOf(paramChar, sql)); - int index = 0; - int paramIndex = 1; - while (index < sql.length()) { - char c = sql.charAt(index); - if (c == paramChar) { - named.append(namedParamPrefix).append(paramIndex); - paramIndex++; - index++; - } else { - index = skip(sql, index, named); - } - } - return new ParametersInfo(paramIndex - 1, named.toString()); - } - /** * Note: This is an internal API and breaking changes can be made without prior notice. * 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 1c5cdda7b0..892672ad0d 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 @@ -90,6 +90,11 @@ boolean supportsLineFeedInQuotedString() { return false; } + @Override + String getQueryParameterPrefix() { + return "@p"; + } + /** * Removes comments from and trims the given sql statement. Spanner supports three types of * comments: @@ -250,68 +255,6 @@ String removeStatementHint(String sql) { return sql; } - @InternalApi - @Override - ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramChar, String sql) { - boolean isInQuoted = false; - char startQuote = 0; - boolean lastCharWasEscapeChar = false; - boolean isTripleQuoted = false; - int paramIndex = 1; - StringBuilder named = new StringBuilder(sql.length() + countOccurrencesOf(paramChar, sql)); - for (int index = 0; index < sql.length(); index++) { - char c = sql.charAt(index); - if (isInQuoted) { - if ((c == '\n' || c == '\r') && !isTripleQuoted) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } else 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; - } - named.append(c); - } else { - if (c == paramChar) { - named.append("@p" + paramIndex); - paramIndex++; - } 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; - } - } - named.append(c); - } - } - } - if (isInQuoted) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } - return new ParametersInfo(paramIndex - 1, named.toString()); - } - private boolean isReturning(String sql, int index) { return (index >= 1) && (index + 12 <= sql.length()) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java index d4dc76d48b..5cec5d838d 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java @@ -16,9 +16,11 @@ package com.google.cloud.spanner.connection; +import static com.google.cloud.spanner.connection.StatementParserTest.assertUnclosedLiteral; import static org.junit.Assert.assertEquals; import com.google.cloud.spanner.Dialect; +import com.google.cloud.spanner.connection.StatementParserTest.CommentInjector; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -80,4 +82,158 @@ public void testSkip() { assertEquals("'foo\\''", skip("r'foo\\'' ", 1)); assertEquals("'''foo\\'\\'\\'bar'''", skip("'''foo\\'\\'\\'bar''' ", 0)); } + + @Test + public void testConvertPositionalParametersToNamedParameters() { + AbstractStatementParser parser = + AbstractStatementParser.getInstance(Dialect.GOOGLE_STANDARD_SQL); + + for (String comment : + new String[] { + "-- test comment\n", + "/* another test comment */", + "/* comment\nwith\nmultiple\nlines\n */", + "/* comment /* with nested */ comment */" + }) { + for (CommentInjector injector : CommentInjector.values()) { + assertEquals( + injector.inject("select * %sfrom foo where name=@p1", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("select * %sfrom foo where name=?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1%s'?test?\"?test?\"?'@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s'?test?\"?test?\"?'?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1'?it\\'?s'%s@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?'?it\\'?s'%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1'?it\\\"?s'%s@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?'?it\\\"?s'%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1\"?it\\\"?s\"%s@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?\"?it\\\"?s\"%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1%s'''?it\\''?s'''@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s'''?it\\''?s'''?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1\"\"\"?it\\\"\"?s\"\"\"%s@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?\"\"\"?it\\\"\"?s\"\"\"%s?", comment)) + .sqlWithNamedParameters); + + // GoogleSQL does not support dollar-quoted strings, so these are all ignored. + assertEquals( + injector.inject("@p1$$@p2it$@p3s$$%s@p4", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?$$?it$?s$$%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1$tag$@p2it$$@p3s$tag$%s@p4", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?$tag$?it$$?s$tag$%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1%s$$@p2it\\'?s \t ?it\\'?s'$$@p3", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s$$?it\\'?s \t ?it\\'?s'$$?", comment)) + .sqlWithNamedParameters); + + // Note: GoogleSQL does not allowa a single-quoted string literal to contain line feeds. + assertUnclosedLiteral(parser, injector.inject("?'?it\\''?s \n ?it\\''?s'%s?", comment)); + assertEquals( + "@p1'?it\\''@p2s \n @p3it\\''@p4s@p5", + parser.convertPositionalParametersToNamedParameters('?', "?'?it\\''?s \n ?it\\''?s?") + .sqlWithNamedParameters); + assertEquals( + injector.inject("@p1%s'''?it\\''?s \n ?it\\''?s'''@p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s'''?it\\''?s \n ?it\\''?s'''?", comment)) + .sqlWithNamedParameters); + + assertEquals( + injector.inject( + "select 1, @p1, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=@p2 and col2='test' and col3=@p3 and col4='?' and col5=\"?\" and col6='?''?''?'", + comment), + parser.convertPositionalParametersToNamedParameters( + '?', + injector.inject( + "select 1, ?, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=? and col2='test' and col3=? and col4='?' and col5=\"?\" and col6='?''?''?'", + comment)) + .sqlWithNamedParameters); + + assertEquals( + injector.inject( + "select * " + + "%sfrom foo " + + "where name=@p1 " + + "and col2 like @p2 " + + "and col3 > @p3", + comment), + parser.convertPositionalParametersToNamedParameters( + '?', + injector.inject( + "select * " + + "%sfrom foo " + + "where name=? " + + "and col2 like ? " + + "and col3 > ?", + comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("select * " + "from foo " + "where id between @p1%s and @p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', + injector.inject( + "select * " + "from foo " + "where id between ?%s and ?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("select * " + "from foo " + "limit @p1 %s offset @p2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', + injector.inject("select * " + "from foo " + "limit ? %s offset ?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject( + "select * " + + "from foo " + + "where col1=@p1 " + + "and col2 like @p2 " + + " %s " + + "and col3 > @p3 " + + "and col4 < @p4 " + + "and col5 != @p5 " + + "and col6 not in (@p6, @p7, @p8) " + + "and col7 in (@p9, @p10, @p11) " + + "and col8 between @p12 and @p13", + comment), + parser.convertPositionalParametersToNamedParameters( + '?', + injector.inject( + "select * " + + "from foo " + + "where col1=? " + + "and col2 like ? " + + " %s " + + "and col3 > ? " + + "and col4 < ? " + + "and col5 != ? " + + "and col6 not in (?, ?, ?) " + + "and col7 in (?, ?, ?) " + + "and col8 between ? and ?", + comment)) + .sqlWithNamedParameters); + } + } + } } 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 c60550c3ba..3739aa1106 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 @@ -17,12 +17,11 @@ package com.google.cloud.spanner.connection; import static com.google.common.truth.Truth.assertThat; -import static org.hamcrest.CoreMatchers.equalTo; -import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.junit.Assume.assumeTrue; @@ -1066,85 +1065,86 @@ private void testParseStatementWithOneParame public void testGoogleStandardSQLDialectConvertPositionalParametersToNamedParameters() { assumeTrue(dialect == Dialect.GOOGLE_STANDARD_SQL); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', "select * from foo where name=?") - .sqlWithNamedParameters) - .isEqualTo("select * from foo where name=@p1"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'?test?\"?test?\"?'?") - .sqlWithNamedParameters) - .isEqualTo("@p1'?test?\"?test?\"?'@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'?it\\'?s'?") - .sqlWithNamedParameters) - .isEqualTo("@p1'?it\\'?s'@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'?it\\\"?s'?") - .sqlWithNamedParameters) - .isEqualTo("@p1'?it\\\"?s'@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?\"?it\\\"?s\"?") - .sqlWithNamedParameters) - .isEqualTo("@p1\"?it\\\"?s\"@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\'?s'''?") - .sqlWithNamedParameters) - .isEqualTo("@p1'''?it\\'?s'''@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?\"\"\"?it\\\"?s\"\"\"?") - .sqlWithNamedParameters) - .isEqualTo("@p1\"\"\"?it\\\"?s\"\"\"@p2"); + assertEquals( + "select * from foo where name=@p1", + parser.convertPositionalParametersToNamedParameters('?', "select * from foo where name=?") + .sqlWithNamedParameters); + assertEquals( + "@p1'?test?\"?test?\"?'@p2", + parser.convertPositionalParametersToNamedParameters('?', "?'?test?\"?test?\"?'?") + .sqlWithNamedParameters); + assertEquals( + "@p1'?it\\'?s'@p2", + parser.convertPositionalParametersToNamedParameters('?', "?'?it\\'?s'?") + .sqlWithNamedParameters); + assertEquals( + "@p1'?it\\\"?s'@p2", + parser.convertPositionalParametersToNamedParameters('?', "?'?it\\\"?s'?") + .sqlWithNamedParameters); + assertEquals( + "@p1\"?it\\\"?s\"@p2", + parser.convertPositionalParametersToNamedParameters('?', "?\"?it\\\"?s\"?") + .sqlWithNamedParameters); + assertEquals( + "@p1'''?it\\'?s'''@p2", + parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\'?s'''?") + .sqlWithNamedParameters); + assertEquals( + "@p1\"\"\"?it\\\"?s\"\"\"@p2", + parser.convertPositionalParametersToNamedParameters('?', "?\"\"\"?it\\\"?s\"\"\"?") + .sqlWithNamedParameters); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?`?it\\`?s`?") - .sqlWithNamedParameters) - .isEqualTo("@p1`?it\\`?s`@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?```?it\\`?s```?") - .sqlWithNamedParameters) - .isEqualTo("@p1```?it\\`?s```@p2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\'?s \n ?it\\'?s'''?") - .sqlWithNamedParameters) - .isEqualTo("@p1'''?it\\'?s \n ?it\\'?s'''@p2"); + assertEquals( + "@p1`?it\\`?s`@p2", + parser.convertPositionalParametersToNamedParameters('?', "?`?it\\`?s`?") + .sqlWithNamedParameters); + assertEquals( + "@p1```?it\\`?s```@p2", + parser.convertPositionalParametersToNamedParameters('?', "?```?it\\`?s```?") + .sqlWithNamedParameters); + assertEquals( + "@p1'''?it\\'?s \n ?it\\'?s'''@p2", + parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\'?s \n ?it\\'?s'''?") + .sqlWithNamedParameters); - assertUnclosedLiteral("?'?it\\'?s \n ?it\\'?s'?"); - assertUnclosedLiteral("?'?it\\'?s \n ?it\\'?s?"); - assertUnclosedLiteral("?'''?it\\'?s \n ?it\\'?s'?"); + assertUnclosedLiteral(parser, "?'?it\\'?s \n ?it\\'?s'?"); + assertUnclosedLiteral(parser, "?'?it\\'?s \n ?it\\'?s?"); + assertUnclosedLiteral(parser, "?'''?it\\'?s \n ?it\\'?s'?"); - assertThat( + assertEquals( + "select 1, @p1, 'test?test', \"test?test\", foo.* from `foo` where col1=@p2 and col2='test' and col3=@p3 and col4='?' and col5=\"?\" and col6='?''?''?'", parser.convertPositionalParametersToNamedParameters( '?', "select 1, ?, 'test?test', \"test?test\", foo.* from `foo` where col1=? and col2='test' and col3=? and col4='?' and col5=\"?\" and col6='?''?''?'") - .sqlWithNamedParameters, - is( - equalTo( - "select 1, @p1, 'test?test', \"test?test\", foo.* from `foo` where col1=@p2 and col2='test' and col3=@p3 and col4='?' and col5=\"?\" and col6='?''?''?'"))); + .sqlWithNamedParameters); - assertThat( + assertEquals( + "select * " + "from foo " + "where name=@p1 " + "and col2 like @p2 " + "and col3 > @p3", parser.convertPositionalParametersToNamedParameters( '?', "select * " + "from foo " + "where name=? " + "and col2 like ? " + "and col3 > ?") - .sqlWithNamedParameters, - is( - equalTo( - "select * " - + "from foo " - + "where name=@p1 " - + "and col2 like @p2 " - + "and col3 > @p3"))); - assertThat( + .sqlWithNamedParameters); + assertEquals( + "select * " + "from foo " + "where id between @p1 and @p2", parser.convertPositionalParametersToNamedParameters( '?', "select * " + "from foo " + "where id between ? and ?") - .sqlWithNamedParameters, - is(equalTo("select * " + "from foo " + "where id between @p1 and @p2"))); - assertThat( + .sqlWithNamedParameters); + assertEquals( + "select * " + "from foo " + "limit @p1 offset @p2", parser.convertPositionalParametersToNamedParameters( '?', "select * " + "from foo " + "limit ? offset ?") - .sqlWithNamedParameters, - is(equalTo("select * " + "from foo " + "limit @p1 offset @p2"))); - assertThat( + .sqlWithNamedParameters); + assertEquals( + "select * " + + "from foo " + + "where col1=@p1 " + + "and col2 like @p2 " + + "and col3 > @p3 " + + "and col4 < @p4 " + + "and col5 != @p5 " + + "and col6 not in (@p6, @p7, @p8) " + + "and col7 in (@p9, @p10, @p11) " + + "and col8 between @p12 and @p13", parser.convertPositionalParametersToNamedParameters( '?', "select * " @@ -1157,22 +1157,10 @@ public void testGoogleStandardSQLDialectConvertPositionalParametersToNamedParame + "and col6 not in (?, ?, ?) " + "and col7 in (?, ?, ?) " + "and col8 between ? and ?") - .sqlWithNamedParameters, - is( - equalTo( - "select * " - + "from foo " - + "where col1=@p1 " - + "and col2 like @p2 " - + "and col3 > @p3 " - + "and col4 < @p4 " - + "and col5 != @p5 " - + "and col6 not in (@p6, @p7, @p8) " - + "and col7 in (@p9, @p10, @p11) " - + "and col8 between @p12 and @p13"))); + .sqlWithNamedParameters); } - private enum CommentInjector { + enum CommentInjector { NONE { @Override String inject(String sql, String comment) { @@ -1213,57 +1201,57 @@ public void testPostgreSQLDialectDialectConvertPositionalParametersToNamedParame "/* comment /* with nested */ comment */" }) { for (CommentInjector injector : CommentInjector.values()) { - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("select * %sfrom foo where name=?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("select * %sfrom foo where name=$1", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?%s'?test?\"?test?\"?'?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1%s'?test?\"?test?\"?'$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?'?it\\''?s'%s?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1'?it\\''?s'%s$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?'?it\\\"?s'%s?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1'?it\\\"?s'%s$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?\"?it\\\"\"?s\"%s?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1\"?it\\\"\"?s\"%s$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?%s'''?it\\''?s'''?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1%s'''?it\\''?s'''$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?\"\"\"?it\\\"\"?s\"\"\"%s?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1\"\"\"?it\\\"\"?s\"\"\"%s$2", comment)); - - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?$$?it$?s$$%s?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1$$?it$?s$$%s$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?$tag$?it$$?s$tag$%s?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1$tag$?it$$?s$tag$%s$2", comment)); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', injector.inject("?%s$$?it\\'?s \n ?it\\'?s$$?", comment)) - .sqlWithNamedParameters) - .isEqualTo(injector.inject("$1%s$$?it\\'?s \n ?it\\'?s$$$2", comment)); + assertEquals( + injector.inject("select * %sfrom foo where name=$1", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("select * %sfrom foo where name=?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1%s'?test?\"?test?\"?'$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s'?test?\"?test?\"?'?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1'?it\\''?s'%s$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?'?it\\''?s'%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1'?it\\\"?s'%s$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?'?it\\\"?s'%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1\"?it\\\"\"?s\"%s$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?\"?it\\\"\"?s\"%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1%s'''?it\\''?s'''$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s'''?it\\''?s'''?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1\"\"\"?it\\\"\"?s\"\"\"%s$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?\"\"\"?it\\\"\"?s\"\"\"%s?", comment)) + .sqlWithNamedParameters); + + assertEquals( + injector.inject("$1$$?it$?s$$%s$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?$$?it$?s$$%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1$tag$?it$$?s$tag$%s$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?$tag$?it$$?s$tag$%s?", comment)) + .sqlWithNamedParameters); + assertEquals( + injector.inject("$1%s$$?it\\'?s \n ?it\\'?s$$$2", comment), + parser.convertPositionalParametersToNamedParameters( + '?', injector.inject("?%s$$?it\\'?s \n ?it\\'?s$$?", comment)) + .sqlWithNamedParameters); // Note: PostgreSQL allows a single-quoted string literal to contain line feeds. assertEquals( @@ -1271,27 +1259,32 @@ public void testPostgreSQLDialectDialectConvertPositionalParametersToNamedParame parser.convertPositionalParametersToNamedParameters( '?', injector.inject("?'?it\\''?s \n ?it\\''?s'%s?", comment)) .sqlWithNamedParameters); - assertUnclosedLiteral("?'?it\\''?s \n ?it\\''?s?"); + assertUnclosedLiteral(parser, "?'?it\\''?s \n ?it\\''?s?"); assertEquals( injector.inject("$1%s'''?it\\''?s \n ?it\\''?s'$2", comment), parser.convertPositionalParametersToNamedParameters( '?', injector.inject("?%s'''?it\\''?s \n ?it\\''?s'?", comment)) .sqlWithNamedParameters); - assertThat( + assertEquals( + injector.inject( + "select 1, $1, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=$2 and col2='test' and col3=$3 and col4='?' and col5=\"?\" and col6='?''?''?'", + comment), parser.convertPositionalParametersToNamedParameters( '?', injector.inject( "select 1, ?, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=? and col2='test' and col3=? and col4='?' and col5=\"?\" and col6='?''?''?'", comment)) - .sqlWithNamedParameters, - is( - equalTo( - injector.inject( - "select 1, $1, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=$2 and col2='test' and col3=$3 and col4='?' and col5=\"?\" and col6='?''?''?'", - comment)))); + .sqlWithNamedParameters); - assertThat( + assertEquals( + injector.inject( + "select * " + + "%sfrom foo " + + "where name=$1 " + + "and col2 like $2 " + + "and col3 > $3", + comment), parser.convertPositionalParametersToNamedParameters( '?', injector.inject( @@ -1301,36 +1294,34 @@ public void testPostgreSQLDialectDialectConvertPositionalParametersToNamedParame + "and col2 like ? " + "and col3 > ?", comment)) - .sqlWithNamedParameters, - is( - equalTo( - injector.inject( - "select * " - + "%sfrom foo " - + "where name=$1 " - + "and col2 like $2 " - + "and col3 > $3", - comment)))); - assertThat( + .sqlWithNamedParameters); + assertEquals( + injector.inject("select * " + "from foo " + "where id between $1%s and $2", comment), parser.convertPositionalParametersToNamedParameters( '?', injector.inject( "select * " + "from foo " + "where id between ?%s and ?", comment)) - .sqlWithNamedParameters, - is( - equalTo( - injector.inject( - "select * " + "from foo " + "where id between $1%s and $2", comment)))); - assertThat( + .sqlWithNamedParameters); + assertEquals( + injector.inject("select * " + "from foo " + "limit $1 %s offset $2", comment), parser.convertPositionalParametersToNamedParameters( '?', injector.inject("select * " + "from foo " + "limit ? %s offset ?", comment)) - .sqlWithNamedParameters, - is( - equalTo( - injector.inject( - "select * " + "from foo " + "limit $1 %s offset $2", comment)))); - assertThat( + .sqlWithNamedParameters); + assertEquals( + injector.inject( + "select * " + + "from foo " + + "where col1=$1 " + + "and col2 like $2 " + + " %s " + + "and col3 > $3 " + + "and col4 < $4 " + + "and col5 != $5 " + + "and col6 not in ($6, $7, $8) " + + "and col7 in ($9, $10, $11) " + + "and col8 between $12 and $13", + comment), parser.convertPositionalParametersToNamedParameters( '?', injector.inject( @@ -1346,22 +1337,7 @@ public void testPostgreSQLDialectDialectConvertPositionalParametersToNamedParame + "and col7 in (?, ?, ?) " + "and col8 between ? and ?", comment)) - .sqlWithNamedParameters, - is( - equalTo( - injector.inject( - "select * " - + "from foo " - + "where col1=$1 " - + "and col2 like $2 " - + " %s " - + "and col3 > $3 " - + "and col4 < $4 " - + "and col5 != $5 " - + "and col6 not in ($6, $7, $8) " - + "and col7 in ($9, $10, $11) " - + "and col8 between $12 and $13", - comment)))); + .sqlWithNamedParameters); } } } @@ -1714,18 +1690,20 @@ public void testStatementCache_ParameterizedStatement() { assertEquals(1, stats.hitCount()); } - private void assertUnclosedLiteral(String sql) { - try { - parser.convertPositionalParametersToNamedParameters('?', sql); - fail("missing expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isSameInstanceAs(ErrorCode.INVALID_ARGUMENT); - assertThat(e.getMessage()) - .startsWith( - ErrorCode.INVALID_ARGUMENT.name() - + ": SQL statement contains an unclosed literal: " - + sql); - } + static void assertUnclosedLiteral(AbstractStatementParser parser, String sql) { + SpannerException exception = + assertThrows( + SpannerException.class, + () -> parser.convertPositionalParametersToNamedParameters('?', sql)); + assertEquals(ErrorCode.INVALID_ARGUMENT, exception.getErrorCode()); + assertTrue( + exception.getMessage(), + exception + .getMessage() + .startsWith( + ErrorCode.INVALID_ARGUMENT.name() + + ": SQL statement contains an unclosed literal: " + + sql)); } @SuppressWarnings("unchecked") From c073dd83bc66e3f0376dd48cb66793578cdfeacd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Tue, 19 Mar 2024 11:06:14 +0100 Subject: [PATCH 4/5] chore: run formatter --- .../cloud/spanner/connection/AbstractStatementParser.java | 7 ++----- .../spanner/connection/SpannerStatementParserTest.java | 3 --- 2 files changed, 2 insertions(+), 8 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 dde8e2464a..9793a50c63 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 @@ -697,11 +697,8 @@ public boolean checkReturningClause(String sql) { } /** -<<<<<<< HEAD - * Returns true if this dialect supports nested comments. -======= - * <<<<<<< HEAD Returns true if this dialect supports nested comments. ->>>>>>> main + * <<<<<<< HEAD Returns true if this dialect supports nested comments. ======= <<<<<<< HEAD + * Returns true if this dialect supports nested comments. >>>>>>> main * *

    *
  • This method should return false for dialects that consider this to be a valid comment: diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java index ebcf89f2bc..5cec5d838d 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java @@ -21,9 +21,6 @@ import com.google.cloud.spanner.Dialect; import com.google.cloud.spanner.connection.StatementParserTest.CommentInjector; -import static org.junit.Assert.assertEquals; - -import com.google.cloud.spanner.Dialect; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; From f3d5ec0a32371052e10ab762c9b60314e850432e Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Tue, 19 Mar 2024 10:06:34 +0000 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- .../cloud/spanner/connection/AbstractStatementParser.java | 7 ++----- .../spanner/connection/SpannerStatementParserTest.java | 3 --- 2 files changed, 2 insertions(+), 8 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 dde8e2464a..9793a50c63 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 @@ -697,11 +697,8 @@ public boolean checkReturningClause(String sql) { } /** -<<<<<<< HEAD - * Returns true if this dialect supports nested comments. -======= - * <<<<<<< HEAD Returns true if this dialect supports nested comments. ->>>>>>> main + * <<<<<<< HEAD Returns true if this dialect supports nested comments. ======= <<<<<<< HEAD + * Returns true if this dialect supports nested comments. >>>>>>> main * *
      *
    • This method should return false for dialects that consider this to be a valid comment: diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java index ebcf89f2bc..5cec5d838d 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java @@ -21,9 +21,6 @@ import com.google.cloud.spanner.Dialect; import com.google.cloud.spanner.connection.StatementParserTest.CommentInjector; -import static org.junit.Assert.assertEquals; - -import com.google.cloud.spanner.Dialect; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4;