From 74adda44ff2a0fc6a3bf6995b71bd4fb71a43df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 7 Mar 2022 13:24:41 +0100 Subject: [PATCH 1/2] fix: PostgreSQL supports newline in quoted literals and identifiers PostgreSQL supports newline characters in string literals and quoted identifiers. Trying to execute a statement with a string literal or quoted identifier that contained a newline character would cause an 'Unclosed string literal' error. Fixes #1730 --- .../connection/PostgreSQLStatementParser.java | 220 +++++++----------- .../connection/StatementParserTest.java | 38 ++- 2 files changed, 123 insertions(+), 135 deletions(-) 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 0b29bec5333..d2a581fd4eb 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 @@ -56,95 +56,44 @@ protected boolean supportsExplain() { @Override String removeCommentsAndTrimInternal(String sql) { Preconditions.checkNotNull(sql); - String currentTag = null; - boolean isInQuoted = false; boolean isInSingleLineComment = false; int multiLineCommentLevel = 0; - char startQuote = 0; - boolean lastCharWasEscapeChar = false; StringBuilder res = new StringBuilder(sql.length()); int index = 0; while (index < sql.length()) { char c = sql.charAt(index); - if (isInQuoted) { - if ((c == '\n' || c == '\r') && startQuote != DOLLAR) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } else if (c == startQuote) { - if (c == DOLLAR) { - // Check if this is the end of the current dollar quoted string. - String tag = parseDollarQuotedString(sql, index + 1); - if (tag != null && tag.equals(currentTag)) { - index += tag.length() + 1; - res.append(c); - res.append(tag); - isInQuoted = false; - startQuote = 0; - } - } else if (lastCharWasEscapeChar) { - lastCharWasEscapeChar = false; - } else if (sql.length() > index + 1 && sql.charAt(index + 1) == startQuote) { - // This is an escaped quote (e.g. 'foo''bar') - res.append(c); - index++; - } else { - isInQuoted = false; - startQuote = 0; - } - } else if (c == '\\') { - lastCharWasEscapeChar = true; - } else { - lastCharWasEscapeChar = false; + if (isInSingleLineComment) { + if (c == '\n') { + isInSingleLineComment = false; + // Include the line feed in the result. + res.append(c); + } + } else if (multiLineCommentLevel > 0) { + if (sql.length() > index + 1 && c == ASTERIKS && sql.charAt(index + 1) == SLASH) { + multiLineCommentLevel--; + index++; + } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { + multiLineCommentLevel++; + index++; } - res.append(c); } else { - // We are not in a quoted string. - if (isInSingleLineComment) { - if (c == '\n') { - isInSingleLineComment = false; - // Include the line feed in the result. - res.append(c); - } - } else if (multiLineCommentLevel > 0) { - if (sql.length() > index + 1 && c == ASTERIKS && sql.charAt(index + 1) == SLASH) { - multiLineCommentLevel--; - index++; - } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { - multiLineCommentLevel++; - index++; - } + // Check for -- which indicates the start of a single-line comment. + if (sql.length() > index + 1 && c == HYPHEN && sql.charAt(index + 1) == HYPHEN) { + // This is a single line comment. + isInSingleLineComment = true; + index += 2; + continue; + } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { + multiLineCommentLevel++; + index += 2; + continue; } else { - // Check for -- which indicates the start of a single-line comment. - if (sql.length() > index + 1 && c == HYPHEN && sql.charAt(index + 1) == HYPHEN) { - // This is a single line comment. - isInSingleLineComment = true; - } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { - multiLineCommentLevel++; - index++; - } else { - if (c == SINGLE_QUOTE || c == DOUBLE_QUOTE) { - isInQuoted = true; - startQuote = c; - } else if (c == DOLLAR) { - currentTag = parseDollarQuotedString(sql, index + 1); - if (currentTag != null) { - isInQuoted = true; - startQuote = DOLLAR; - index += currentTag.length() + 1; - res.append(c); - res.append(currentTag); - } - } - res.append(c); - } + index = skip(sql, index, res); + continue; } } index++; } - if (isInQuoted) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } if (multiLineCommentLevel > 0) { throw SpannerExceptionFactory.newSpannerException( ErrorCode.INVALID_ARGUMENT, @@ -184,73 +133,78 @@ String removeStatementHint(String sql) { ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramChar, String sql) { Preconditions.checkNotNull(sql); final String namedParamPrefix = "$"; - String currentTag = null; - boolean isInQuoted = false; - char startQuote = 0; - boolean lastCharWasEscapeChar = false; 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 (isInQuoted) { - if ((c == '\n' || c == '\r') && startQuote != DOLLAR) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); - } else if (c == startQuote) { - if (c == DOLLAR) { - // Check if this is the end of the current dollar quoted string. - String tag = parseDollarQuotedString(sql, index + 1); - if (tag != null && tag.equals(currentTag)) { - index += tag.length() + 1; - named.append(c); - named.append(tag); - isInQuoted = false; - startQuote = 0; - } - } else if (lastCharWasEscapeChar) { - lastCharWasEscapeChar = false; - } else if (sql.length() > index + 1 && sql.charAt(index + 1) == startQuote) { - // This is an escaped quote (e.g. 'foo''bar') - named.append(c); - index++; - } else { - isInQuoted = false; - startQuote = 0; + if (c == paramChar) { + named.append(namedParamPrefix).append(paramIndex); + paramIndex++; + index++; + } else { + index = skip(sql, index, named); + } + } + return new ParametersInfo(paramIndex - 1, named.toString()); + } + + private int skip(String sql, int currentIndex, StringBuilder result) { + char currentChar = sql.charAt(currentIndex); + if (currentChar == SINGLE_QUOTE || currentChar == DOUBLE_QUOTE) { + result.append(currentChar); + return skipQuoted(sql, currentIndex, currentChar, result); + } else if (currentChar == DOLLAR) { + String dollarTag = parseDollarQuotedString(sql, currentIndex + 1); + if (dollarTag != null) { + result.append(currentChar).append(dollarTag).append(currentChar); + return skipQuoted( + sql, currentIndex + dollarTag.length() + 1, currentChar, dollarTag, result); + } + } + + result.append(currentChar); + return currentIndex + 1; + } + + private int skipQuoted(String sql, int startIndex, char startQuote, StringBuilder result) { + return skipQuoted(sql, startIndex, startQuote, null, result); + } + + private int skipQuoted( + String sql, int startIndex, char startQuote, String dollarTag, StringBuilder result) { + boolean lastCharWasEscapeChar = false; + 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)) { + result.append(currentChar).append(tag).append(currentChar); + return currentIndex + tag.length() + 2; } - } else if (c == '\\') { - lastCharWasEscapeChar = true; - } else { + } else if (lastCharWasEscapeChar) { lastCharWasEscapeChar = false; - } - named.append(c); - } else { - if (c == paramChar) { - named.append(namedParamPrefix + paramIndex); - paramIndex++; + } else if (sql.length() > currentIndex + 1 && sql.charAt(currentIndex + 1) == startQuote) { + // This is an escaped quote (e.g. 'foo''bar') + result.append(currentChar).append(currentChar); + currentIndex += 2; + continue; } else { - if (c == SINGLE_QUOTE || c == DOUBLE_QUOTE) { - isInQuoted = true; - startQuote = c; - } else if (c == DOLLAR) { - currentTag = parseDollarQuotedString(sql, index + 1); - if (currentTag != null) { - isInQuoted = true; - startQuote = DOLLAR; - index += currentTag.length() + 1; - named.append(c); - named.append(currentTag); - } - } - named.append(c); + result.append(currentChar); + return currentIndex + 1; } + } else if (currentChar == '\\') { + lastCharWasEscapeChar = true; + } else { + lastCharWasEscapeChar = false; } - index++; - } - if (isInQuoted) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); + currentIndex++; + result.append(currentChar); } - return new ParametersInfo(paramIndex - 1, named.toString()); + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql); } } 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 d2176b53a2e..ed6c9c9e453 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 @@ -127,6 +127,33 @@ public void testRemoveComments() { parser.removeCommentsAndTrim( "/* This\nis\na\nmulti\nline\ncomment */\nSELECT * FROM FOO")) .isEqualTo("SELECT * FROM FOO"); + + assertEquals( + "SELECT \"FOO\" FROM \"BAR\" WHERE name='test'", + parser.removeCommentsAndTrim( + "-- Single line comment\nSELECT \"FOO\" FROM \"BAR\" WHERE name='test'")); + assertEquals( + "SELECT \"FOO\" FROM \"BAR\" WHERE name='test' and id=1", + parser.removeCommentsAndTrim( + "/* Multi\n" + + "line\n" + + "comment\n" + + "*/SELECT \"FOO\" FROM \"BAR\" WHERE name='test' and id=1")); + + if (dialect == Dialect.POSTGRESQL) { + // PostgreSQL allows string literals and quoted identifiers to contain newline characters. + assertEquals( + "SELECT \"FOO\nBAR\" FROM \"BAR\" WHERE name='test\ntest'", + parser.removeCommentsAndTrim( + "-- Single line comment\nSELECT \"FOO\nBAR\" FROM \"BAR\" WHERE name='test\ntest'")); + assertEquals( + "SELECT \"FOO\nBAR\" FROM \"BAR\" WHERE name='test\ntest' and id=1", + parser.removeCommentsAndTrim( + "/* Multi\n" + + "line\n" + + "comment\n" + + "*/SELECT \"FOO\nBAR\" FROM \"BAR\" WHERE name='test\ntest' and id=1")); + } } @Test @@ -1221,9 +1248,16 @@ public void testPostgreSQLDialectDialectConvertPositionalParametersToNamedParame .sqlWithNamedParameters) .isEqualTo("$1$$?it\\'?s \n ?it\\'?s$$$2"); - assertUnclosedLiteral("?'?it\\'?s \n ?it\\'?s'?"); + // Note: PostgreSQL allows a single-quoted string literal to contain line feeds. + assertEquals( + "$1'?it\\'?s \n ?it\\'?s'$2", + parser.convertPositionalParametersToNamedParameters('?', "?'?it\\'?s \n ?it\\'?s'?") + .sqlWithNamedParameters); assertUnclosedLiteral("?'?it\\'?s \n ?it\\'?s?"); - assertUnclosedLiteral("?'''?it\\'?s \n ?it\\'?s'?"); + assertEquals( + "$1'''?it\\'?s \n ?it\\'?s'$2", + parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\'?s \n ?it\\'?s'?") + .sqlWithNamedParameters); assertThat( parser.convertPositionalParametersToNamedParameters( From c5690a493c662edaed2411391ac39a69cecca6f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Tue, 8 Mar 2022 07:07:08 +0100 Subject: [PATCH 2/2] fix: typo --- .../cloud/spanner/connection/AbstractStatementParser.java | 2 +- .../cloud/spanner/connection/PostgreSQLStatementParser.java | 6 +++--- .../cloud/spanner/connection/SpannerStatementParser.java | 4 ++-- 3 files changed, 6 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 82fb12acac3..ba68a5b513c 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 @@ -460,7 +460,7 @@ private boolean statementStartsWith(String sql, Iterable checkStatements static final char HYPHEN = '-'; static final char DASH = '#'; static final char SLASH = '/'; - static final char ASTERIKS = '*'; + static final char ASTERISK = '*'; static final char DOLLAR = '$'; /** 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 d2a581fd4eb..837ee77c247 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 @@ -69,10 +69,10 @@ String removeCommentsAndTrimInternal(String sql) { res.append(c); } } else if (multiLineCommentLevel > 0) { - if (sql.length() > index + 1 && c == ASTERIKS && sql.charAt(index + 1) == SLASH) { + if (sql.length() > index + 1 && c == ASTERISK && sql.charAt(index + 1) == SLASH) { multiLineCommentLevel--; index++; - } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { + } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERISK) { multiLineCommentLevel++; index++; } @@ -83,7 +83,7 @@ String removeCommentsAndTrimInternal(String sql) { isInSingleLineComment = true; index += 2; continue; - } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { + } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERISK) { multiLineCommentLevel++; index += 2; continue; 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 a9c7624f273..156ee2e7f6f 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 @@ -106,7 +106,7 @@ String removeCommentsAndTrimInternal(String sql) { res.append(c); } } else if (isInMultiLineComment) { - if (sql.length() > index + 1 && c == ASTERIKS && sql.charAt(index + 1) == SLASH) { + if (sql.length() > index + 1 && c == ASTERISK && sql.charAt(index + 1) == SLASH) { isInMultiLineComment = false; index++; } @@ -115,7 +115,7 @@ String removeCommentsAndTrimInternal(String sql) { || (sql.length() > index + 1 && c == HYPHEN && sql.charAt(index + 1) == HYPHEN)) { // This is a single line comment. isInSingleLineComment = true; - } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERIKS) { + } else if (sql.length() > index + 1 && c == SLASH && sql.charAt(index + 1) == ASTERISK) { isInMultiLineComment = true; index++; } else {