Add option to parse named parameters in SQL comments

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<String, ParsedSql> 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}.
+	 * <p>Turning this flag on is useful for database engines that interpret hints
+	 * or routing keys declared in comments, for example
+	 * <code>"&#47;*@ queryKey(:userId) *&#47; SELECT ..."</code>.
+	 * <p><b>NOTE: Even with this flag turned on, string literals and quoted
+	 * identifiers are still skipped.</b> 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.
+	 * <p>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<String, ParsedSql> createParsedSqlCache(int cacheLimit) {
+		boolean allowInComments = this.allowParametersInComments;
+		return new ConcurrentLruCache<>(cacheLimit,
+				sql -> NamedParameterUtils.parseSqlStatement(sql, allowInComments));
+	}
+
 
 	@Override
 	public <T extends @Nullable Object> T execute(String sql, SqlParameterSource paramSource, PreparedStatementCallback<T> action)

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.
+	 * <p>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.
+	 * <p>By default, named parameters within line comments (<code>--</code>) and block
+	 * comments (<code>&#47;* ... *&#47;</code>) 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
+	 * <code>"&#47;*@ queryKey(:userId) *&#47; SELECT ..."</code>.
+	 * <p><b>NOTE: Even with the flag set to {@code true}, string literals and quoted
+	 * identifiers are still skipped.</b> As a consequence, an apostrophe within a
+	 * comment (as in <code>"&#47;* don't touch *&#47;"</code>) 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<String> 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<String> 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++) {

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<Object>) 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<Object>) 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 {

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";