From bfa777bc957a4abb2cbe3842a512e5cd652864eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Tue, 29 Aug 2023 15:45:00 +0200 Subject: [PATCH] chore: allow comments in PG strings (#2604) Allow comments in PostgreSQL query strings for extracting parameter definitions. The comments need to be preserved, as they could contain query hints. --- .../connection/AbstractStatementParser.java | 7 +- .../connection/PostgreSQLStatementParser.java | 57 ++- .../connection/StatementParserTest.java | 339 ++++++++++++------ 3 files changed, 286 insertions(+), 117 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 11549fbfc59..6c35fe5c96d 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 @@ -539,11 +539,10 @@ 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. There is an exception case if - * the statement starts with a GSQL comment which forces it to be interpreted as a GoogleSql - * statement. + * parameters. The input SQL string may not contain any comments, except for PostgreSQL-dialect + * SQL strings. * - * @param sql The sql string without comments that should be converted + * @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. 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 012bfbba875..8cb2b7e464a 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 @@ -184,9 +184,8 @@ ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramCh * Note: This is an internal API and breaking changes can be made without prior notice. * *

Returns the PostgreSQL-style query parameters ($1, $2, ...) in the given SQL string. The - * SQL-string is assumed to not contain any comments. Use {@link #removeCommentsAndTrim(String)} - * to remove all comments before calling this method. Occurrences of query-parameter like strings - * inside quoted identifiers or string literals are ignored. + * SQL-string is allowed to contain comments. Occurrences of query-parameter like strings inside + * quoted identifiers or string literals are ignored. * *

The following example will return a set containing ("$1", "$2"). * select col1, col2, "col$4" @@ -195,7 +194,7 @@ ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramCh * and not col3=$1 and col4='$3' * * - * @param sql the SQL-string to check for parameters. Must not contain comments. + * @param sql the SQL-string to check for parameters. * @return A set containing all the parameters in the SQL-string. */ @InternalApi @@ -233,12 +232,56 @@ private int skip(String sql, int currentIndex, @Nullable StringBuilder result) { 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); @@ -285,6 +328,12 @@ private void appendIfNotNull(@Nullable StringBuilder result, char 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) { 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 8246ca10760..1c20bc94ec6 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 @@ -1143,121 +1143,198 @@ public void testGoogleStandardSQLDialectConvertPositionalParametersToNamedParame + "and col8 between @p12 and @p13"))); } + private enum CommentInjector { + NONE { + @Override + String inject(String sql, String comment) { + return String.format(sql, ""); + } + }, + BEFORE { + @Override + String inject(String sql, String comment) { + return comment + String.format(sql, ""); + } + }, + IN_THE_MIDDLE { + @Override + String inject(String sql, String comment) { + return String.format(sql, comment); + } + }, + AFTER { + @Override + String inject(String sql, String comment) { + return String.format(sql, "") + comment; + } + }; + + abstract String inject(String sql, String comment); + } + @Test public void testPostgreSQLDialectDialectConvertPositionalParametersToNamedParameters() { assumeTrue(dialect == Dialect.POSTGRESQL); - assertThat( + 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()) { + 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)); + + // Note: PostgreSQL allows a single-quoted string literal to contain line feeds. + assertEquals( + injector.inject("$1'?it\\''?s \n ?it\\''?s'%s$2", comment), parser.convertPositionalParametersToNamedParameters( - '?', "select * from foo where name=?") - .sqlWithNamedParameters) - .isEqualTo("select * from foo where name=$1"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'?test?\"?test?\"?'?") - .sqlWithNamedParameters) - .isEqualTo("$1'?test?\"?test?\"?'$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'?it\\''?s'?") - .sqlWithNamedParameters) - .isEqualTo("$1'?it\\''?s'$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'?it\\\"?s'?") - .sqlWithNamedParameters) - .isEqualTo("$1'?it\\\"?s'$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?\"?it\\\"\"?s\"?") - .sqlWithNamedParameters) - .isEqualTo("$1\"?it\\\"\"?s\"$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\''?s'''?") - .sqlWithNamedParameters) - .isEqualTo("$1'''?it\\''?s'''$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?\"\"\"?it\\\"\"?s\"\"\"?") - .sqlWithNamedParameters) - .isEqualTo("$1\"\"\"?it\\\"\"?s\"\"\"$2"); - - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?$$?it$?s$$?") - .sqlWithNamedParameters) - .isEqualTo("$1$$?it$?s$$$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?$tag$?it$$?s$tag$?") - .sqlWithNamedParameters) - .isEqualTo("$1$tag$?it$$?s$tag$$2"); - assertThat( - parser.convertPositionalParametersToNamedParameters('?', "?$$?it\\'?s \n ?it\\'?s$$?") - .sqlWithNamedParameters) - .isEqualTo("$1$$?it\\'?s \n ?it\\'?s$$$2"); - - // 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?"); - assertEquals( - "$1'''?it\\''?s \n ?it\\''?s'$2", - parser.convertPositionalParametersToNamedParameters('?', "?'''?it\\''?s \n ?it\\''?s'?") - .sqlWithNamedParameters); - - assertThat( - 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, $1, 'test?test', \"test?test\", foo.* from `foo` where col1=$2 and col2='test' and col3=$3 and col4='?' and col5=\"?\" and col6='?''?''?'"))); + '?', injector.inject("?'?it\\''?s \n ?it\\''?s'%s?", comment)) + .sqlWithNamedParameters); + assertUnclosedLiteral("?'?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( - parser.convertPositionalParametersToNamedParameters( - '?', - "select * " + "from foo " + "where name=? " + "and col2 like ? " + "and col3 > ?") - .sqlWithNamedParameters, - is( - equalTo( - "select * " - + "from foo " - + "where name=$1 " - + "and col2 like $2 " - + "and col3 > $3"))); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', "select * " + "from foo " + "where id between ? and ?") - .sqlWithNamedParameters, - is(equalTo("select * " + "from foo " + "where id between $1 and $2"))); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', "select * " + "from foo " + "limit ? offset ?") - .sqlWithNamedParameters, - is(equalTo("select * " + "from foo " + "limit $1 offset $2"))); - assertThat( - parser.convertPositionalParametersToNamedParameters( - '?', - "select * " - + "from foo " - + "where col1=? " - + "and col2 like ? " - + "and col3 > ? " - + "and col4 < ? " - + "and col5 != ? " - + "and col6 not in (?, ?, ?) " - + "and col7 in (?, ?, ?) " - + "and col8 between ? and ?") - .sqlWithNamedParameters, - is( - equalTo( - "select * " - + "from foo " - + "where col1=$1 " - + "and col2 like $2 " - + "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"))); + assertThat( + 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)))); + + assertThat( + parser.convertPositionalParametersToNamedParameters( + '?', + injector.inject( + "select * " + + "%sfrom foo " + + "where name=? " + + "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( + 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( + 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( + 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, + 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)))); + } + } } @Test @@ -1277,6 +1354,11 @@ public void testPostgreSQLGetQueryParameters() { assertEquals( ImmutableSet.of("$1"), parser.getQueryParameters("select '$2' from foo where bar=$1 and baz=$foo")); + assertEquals( + ImmutableSet.of("$1"), + parser.getQueryParameters( + "/* @lock_scanned_ranges = exclusive */ select -- random comment\n '$2' " + + "from foo /* comment /* with nested comment */ outside of nested comment */ where bar=$1 and baz=$foo")); } @Test @@ -1488,6 +1570,45 @@ public void testPostgreSQLReturningClause() { parser.parse(Statement.of("insert into t1 select 10.returning*")).hasReturningClause()); } + int skipSingleLineComment(String sql, int startIndex) { + return PostgreSQLStatementParser.skipSingleLineComment(sql, startIndex, null); + } + + int skipMultiLineComment(String sql, int startIndex) { + return PostgreSQLStatementParser.skipMultiLineComment(sql, startIndex, null); + } + + @Test + public void testSkipSingleLineComment() { + assumeTrue(dialect == Dialect.POSTGRESQL); + + assertEquals(7, skipSingleLineComment("-- foo\n", 0)); + assertEquals(7, skipSingleLineComment("-- foo\nbar", 0)); + assertEquals(6, skipSingleLineComment("-- foo", 0)); + assertEquals(11, skipSingleLineComment("bar -- foo\n", 4)); + assertEquals(11, skipSingleLineComment("bar -- foo\nbar", 4)); + assertEquals(10, skipSingleLineComment("bar -- foo", 4)); + } + + @Test + public void testSkipMultiLineComment() { + assumeTrue(dialect == Dialect.POSTGRESQL); + + assertEquals(9, skipMultiLineComment("/* foo */", 0)); + assertEquals(9, skipMultiLineComment("/* foo */ bar", 0)); + assertEquals(6, skipMultiLineComment("/* foo", 0)); + assertEquals(8, skipMultiLineComment("/* foo *", 0)); + assertEquals(9, skipMultiLineComment("/* foo **", 0)); + assertEquals(10, skipMultiLineComment("/* foo **/ ", 0)); + assertEquals(13, skipMultiLineComment("bar /* foo */", 4)); + assertEquals(13, skipMultiLineComment("bar /* foo */bar", 4)); + assertEquals(10, skipMultiLineComment("bar /* foo", 4)); + + assertEquals( + "/* foo /* inner comment */ not in inner comment */".length(), + skipMultiLineComment("/* foo /* inner comment */ not in inner comment */ bar", 0)); + } + private void assertUnclosedLiteral(String sql) { try { parser.convertPositionalParametersToNamedParameters('?', sql);