diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplate.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplate.java index db76fa820dea..8d1523ad47af 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplate.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplate.java @@ -86,6 +86,9 @@ public class NamedParameterJdbcTemplate implements NamedParameterJdbcOperations /** The JdbcTemplate we are wrapping. */ private final JdbcOperations classicJdbcTemplate; + /** Whether named parameters within SQL comments are to be parsed as well. */ + private boolean allowParametersInComments = false; + /** Cache of original SQL String to ParsedSql representation. */ private volatile ConcurrentLruCache parsedSqlCache; @@ -107,7 +110,7 @@ public NamedParameterJdbcTemplate(DataSource dataSource) { public NamedParameterJdbcTemplate(JdbcOperations classicJdbcTemplate) { Assert.notNull(classicJdbcTemplate, "JdbcTemplate must not be null"); this.classicJdbcTemplate = classicJdbcTemplate; - this.parsedSqlCache = new ConcurrentLruCache<>(DEFAULT_CACHE_LIMIT, NamedParameterUtils::parseSqlStatement); + this.parsedSqlCache = createParsedSqlCache(DEFAULT_CACHE_LIMIT); } /** @@ -119,6 +122,7 @@ public NamedParameterJdbcTemplate(JdbcOperations classicJdbcTemplate) { public NamedParameterJdbcTemplate(NamedParameterJdbcTemplate original, JdbcTemplate classicJdbcTemplate) { Assert.notNull(classicJdbcTemplate, "JdbcTemplate must not be null"); this.classicJdbcTemplate = classicJdbcTemplate; + this.allowParametersInComments = original.allowParametersInComments; this.parsedSqlCache = original.parsedSqlCache; } @@ -149,7 +153,7 @@ public JdbcTemplate getJdbcTemplate() { * Default is 256. 0 indicates no caching, always parsing each statement. */ public void setCacheLimit(int cacheLimit) { - this.parsedSqlCache = new ConcurrentLruCache<>(cacheLimit, NamedParameterUtils::parseSqlStatement); + this.parsedSqlCache = createParsedSqlCache(cacheLimit); } /** @@ -159,6 +163,41 @@ public int getCacheLimit() { return this.parsedSqlCache.capacity(); } + /** + * Specify whether named parameters within SQL comments are to be parsed and + * substituted as well, rather than being ignored. Default is {@code false}. + *

Turning this flag on is useful for database engines that interpret hints + * or routing keys declared in comments, for example + * "/*@ queryKey(:userId) */ SELECT ...". + *

NOTE: Even with this flag turned on, string literals and quoted + * identifiers are still skipped. Also, the target driver or engine needs to + * count a placeholder within a comment as an actual bind parameter; otherwise, + * the number of bind values is not going to match. See + * {@link NamedParameterUtils#parseSqlStatement(String, boolean)} for details. + *

Note that changing this setting resets this template's SQL cache, + * retaining the configured {@link #setCacheLimit cache limit}. + * @since 7.1 + * @see NamedParameterUtils#parseSqlStatement(String, boolean) + */ + public void setAllowParametersInComments(boolean allowParametersInComments) { + this.allowParametersInComments = allowParametersInComments; + this.parsedSqlCache = createParsedSqlCache(getCacheLimit()); + } + + /** + * Return whether named parameters within SQL comments are to be parsed as well. + * @since 7.1 + */ + public boolean isAllowParametersInComments() { + return this.allowParametersInComments; + } + + private ConcurrentLruCache createParsedSqlCache(int cacheLimit) { + boolean allowInComments = this.allowParametersInComments; + return new ConcurrentLruCache<>(cacheLimit, + sql -> NamedParameterUtils.parseSqlStatement(sql, allowInComments)); + } + @Override public T execute(String sql, SqlParameterSource paramSource, PreparedStatementCallback action) diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java index 3c1902a90619..46e454269e77 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java @@ -51,6 +51,12 @@ public abstract class NamedParameterUtils { */ private static final String[] STOP_SKIP = {"'", "\"", "\n", "*/", "`"}; + /** + * Flags indicating which of the {@link #START_SKIP} entries start a comment + * rather than a quoted identifier or string literal. + */ + private static final boolean[] COMMENT_SKIP = {false, false, true, true, false}; + /** * Set of characters that qualify as parameter separators, * indicating that a parameter name in an SQL String has ended. @@ -77,10 +83,41 @@ public abstract class NamedParameterUtils { /** * Parse the SQL statement and locate any placeholders or named parameters. * Named parameters are substituted for a JDBC placeholder. + *

Named parameters within comments are ignored; see + * {@link #parseSqlStatement(String, boolean)} for an option to include them. * @param sql the SQL statement * @return the parsed statement, represented as {@link ParsedSql} instance */ public static ParsedSql parseSqlStatement(String sql) { + return parseSqlStatement(sql, false); + } + + /** + * Parse the SQL statement and locate any placeholders or named parameters, + * optionally including named parameters within comments. + * Named parameters are substituted for a JDBC placeholder. + *

By default, named parameters within line comments (--) and block + * comments (/* ... */) are ignored, just like named parameters + * within string literals and quoted identifiers. Specifying {@code true} for the + * {@code allowParametersInComments} flag makes named parameters within comments + * subject to regular parsing and substitution, which is useful for database engines + * that interpret hints or routing keys in comments, for example + * "/*@ queryKey(:userId) */ SELECT ...". + *

NOTE: Even with the flag set to {@code true}, string literals and quoted + * identifiers are still skipped. As a consequence, an apostrophe within a + * comment (as in "/* don't touch */") is going to be interpreted + * as the start of a string literal, potentially skipping over subsequent named + * parameters. Also, since a named parameter within a comment gets substituted with a + * JDBC placeholder like any other, the target driver or engine needs to count such a + * placeholder as an actual bind parameter; otherwise, the number of bind values is + * not going to match. + * @param sql the SQL statement + * @param allowParametersInComments whether to parse named parameters + * within SQL comments as well, rather than ignoring them + * @return the parsed statement, represented as {@link ParsedSql} instance + * @since 7.1 + */ + public static ParsedSql parseSqlStatement(String sql, boolean allowParametersInComments) { Assert.notNull(sql, "SQL must not be null"); Set namedParameters = new HashSet<>(); @@ -97,7 +134,7 @@ public static ParsedSql parseSqlStatement(String sql) { while (i < statement.length) { int skipToPosition = i; while (i < statement.length) { - skipToPosition = skipCommentsAndQuotes(statement, i); + skipToPosition = skipCommentsAndQuotes(statement, i, !allowParametersInComments); if (i == skipToPosition) { break; } @@ -223,10 +260,15 @@ private static int addNewNamedParameter(Set namedParameters, int namedPa * Skip over comments and quoted names present in an SQL statement. * @param statement character array containing SQL statement * @param position current position of statement + * @param skipComments whether to skip over comments as well, + * rather than just over quoted names and string literals * @return next position to process after any comments or quotes are skipped */ - private static int skipCommentsAndQuotes(char[] statement, int position) { + private static int skipCommentsAndQuotes(char[] statement, int position, boolean skipComments) { for (int i = 0; i < START_SKIP.length; i++) { + if (!skipComments && COMMENT_SKIP[i]) { + continue; + } if (statement[position] == START_SKIP[i].charAt(0)) { boolean match = true; for (int j = 1; j < START_SKIP[i].length(); j++) { diff --git a/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplateTests.java b/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplateTests.java index e2f6387965ec..1c8c3f6be914 100644 --- a/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplateTests.java +++ b/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplateTests.java @@ -80,6 +80,13 @@ class NamedParameterJdbcTemplateTests { private static final String UPDATE_ARRAY_PARAMETERS_PARSED = "update customer set type = array[?, ?, ?] where id = ?"; + private static final String SELECT_NAMED_PARAMETERS_IN_COMMENT = + "/*@ queryKey(:id) */ select id, forename from custmr where id = :id and country = :country"; + private static final String SELECT_NAMED_PARAMETERS_IN_COMMENT_PARSED = + "/*@ queryKey(?) */ select id, forename from custmr where id = ? and country = ?"; + private static final String SELECT_NAMED_PARAMETERS_IN_COMMENT_PARSED_WITHOUT_COMMENT = + "/*@ queryKey(:id) */ select id, forename from custmr where id = ? and country = ?"; + private static final String[] COLUMN_NAMES = new String[] {"id", "forename"}; @@ -147,6 +154,63 @@ void execute() throws SQLException { verify(connection).close(); } + @Test // gh-22255 + void executeWithParametersInCommentsIgnoredByDefault() throws SQLException { + given(preparedStatement.executeUpdate()).willReturn(1); + + assertThat(namedParameterTemplate.isAllowParametersInComments()).isFalse(); + + params.put("id", 1); + params.put("country", "UK"); + namedParameterTemplate.execute(SELECT_NAMED_PARAMETERS_IN_COMMENT, params, + (PreparedStatementCallback) ps -> { + ps.executeUpdate(); + return "result"; + }); + + verify(connection).prepareStatement(SELECT_NAMED_PARAMETERS_IN_COMMENT_PARSED_WITHOUT_COMMENT); + verify(preparedStatement).setObject(1, 1); + verify(preparedStatement).setString(2, "UK"); + verify(preparedStatement).close(); + verify(connection).close(); + } + + @Test // gh-22255 + void executeWithParametersInCommentsAllowed() throws SQLException { + given(preparedStatement.executeUpdate()).willReturn(1); + + namedParameterTemplate.setAllowParametersInComments(true); + assertThat(namedParameterTemplate.isAllowParametersInComments()).isTrue(); + + params.put("id", 1); + params.put("country", "UK"); + namedParameterTemplate.execute(SELECT_NAMED_PARAMETERS_IN_COMMENT, params, + (PreparedStatementCallback) ps -> { + ps.executeUpdate(); + return "result"; + }); + + verify(connection).prepareStatement(SELECT_NAMED_PARAMETERS_IN_COMMENT_PARSED); + verify(preparedStatement).setObject(1, 1); + verify(preparedStatement).setObject(2, 1); + verify(preparedStatement).setString(3, "UK"); + verify(preparedStatement).close(); + verify(connection).close(); + } + + @Test // gh-22255 + void setAllowParametersInCommentsResetsParsedSqlCacheRetainingCacheLimit() { + namedParameterTemplate.setCacheLimit(10); + assertThat(namedParameterTemplate.getParsedSql(SELECT_NAMED_PARAMETERS_IN_COMMENT) + .getTotalParameterCount()).isEqualTo(2); + + namedParameterTemplate.setAllowParametersInComments(true); + + assertThat(namedParameterTemplate.getCacheLimit()).isEqualTo(10); + assertThat(namedParameterTemplate.getParsedSql(SELECT_NAMED_PARAMETERS_IN_COMMENT) + .getTotalParameterCount()).isEqualTo(3); + } + @Disabled("SPR-16340") @Test void executeArray() throws SQLException { diff --git a/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java b/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java index 8a2a942367a6..8c54422638d7 100644 --- a/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java +++ b/spring-jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java @@ -178,6 +178,65 @@ void parseSqlContainingComments() { assertThat(NamedParameterUtils.substituteNamedParameters(parsedSql4, new MapSqlParameterSource(parameters))).isEqualTo("/*+ HINT */ xxx /* comment :a ? */ ? yyyy ? ? ? zzzzz /* :xx XX*"); } + @Test // gh-22255 + void parseSqlContainingParametersInBlockCommentWhenAllowed() { + String sql = "/*@ queryKey(:cafeId) */ select * from cafe where cafe_id = :cafeId"; + ParsedSql parsedSql = NamedParameterUtils.parseSqlStatement(sql, true); + assertThat(parsedSql.getParameterNames()).containsExactly("cafeId", "cafeId"); + assertThat(parsedSql.getTotalParameterCount()).isEqualTo(2); + assertThat(parsedSql.getNamedParameterCount()).isEqualTo(1); + + MapSqlParameterSource paramSource = new MapSqlParameterSource("cafeId", 42); + assertThat(NamedParameterUtils.substituteNamedParameters(parsedSql, paramSource)).isEqualTo( + "/*@ queryKey(?) */ select * from cafe where cafe_id = ?"); + assertThat(NamedParameterUtils.buildValueArray(parsedSql, paramSource, null)).containsExactly(42, 42); + } + + @Test // gh-22255 + void parseSqlContainingParametersInLineCommentWhenAllowed() { + String sql = "select * from cafe where cafe_id = :cafeId -- filtered by :status\n"; + ParsedSql parsedSql = NamedParameterUtils.parseSqlStatement(sql, true); + assertThat(parsedSql.getParameterNames()).containsExactly("cafeId", "status"); + assertThat(NamedParameterUtils.substituteNamedParameters(parsedSql, null)).isEqualTo( + "select * from cafe where cafe_id = ? -- filtered by ?\n"); + } + + @Test // gh-22255 + void parseSqlContainingCommentsWhenParametersInCommentsAllowed() { + String sql1 = "/*+ HINT */ xxx /* comment */ :a yyyy :b :c :a zzzzz -- :xx XX\n"; + ParsedSql parsedSql1 = NamedParameterUtils.parseSqlStatement(sql1, true); + assertThat(NamedParameterUtils.substituteNamedParameters(parsedSql1, null)).isEqualTo( + "/*+ HINT */ xxx /* comment */ ? yyyy ? ? ? zzzzz -- ? XX\n"); + assertThat(parsedSql1.getParameterNames()).containsExactly("a", "b", "c", "a", "xx"); + + // Non-terminated comment must not trip up the parser + String sql2 = "/*+ HINT */ xxx /* comment */ :a yyyy :b :c :a zzzzz /* :xx XX*"; + ParsedSql parsedSql2 = NamedParameterUtils.parseSqlStatement(sql2, true); + assertThat(NamedParameterUtils.substituteNamedParameters(parsedSql2, null)).isEqualTo( + "/*+ HINT */ xxx /* comment */ ? yyyy ? ? ? zzzzz /* ? XX*"); + } + + @Test // gh-22255 + void parseSqlContainingCommentsIgnoresParametersInCommentsByDefault() { + String sql = "/*@ queryKey(:cafeId) */ select * from cafe where cafe_id = :cafeId"; + ParsedSql parsedSql = NamedParameterUtils.parseSqlStatement(sql); + assertThat(parsedSql.getParameterNames()).containsExactly("cafeId"); + assertThat(substituteNamedParameters(parsedSql)).isEqualTo( + "/*@ queryKey(:cafeId) */ select * from cafe where cafe_id = ?"); + } + + @ParameterizedTest // gh-22255 + @ValueSource(strings = { + "SELECT ':foo'':doo', :xxx FROM DUAL", + "SELECT \":foo\"\":doo\", :xxx FROM DUAL", + "SELECT `:foo``:doo`, :xxx FROM DUAL" + }) + void parseSqlStatementWithParametersInsideQuotesWhenParametersInCommentsAllowed(String sql) { + ParsedSql parsedSql = NamedParameterUtils.parseSqlStatement(sql, true); + assertThat(parsedSql.getTotalParameterCount()).isEqualTo(1); + assertThat(parsedSql.getParameterNames()).containsExactly("xxx"); + } + @Test // SPR-4612 void parseSqlStatementWithPostgresCasting() { String expectedSql = "select 'first name' from artists where id = ? and birth_date=?::timestamp";