Allow NamedParameterJdbcTemplate to optionally include parameters in comments of SQL

[benelog]

NamedParameterJdbcTemplate skips parameters in comments by the logic in NamedParameterUtils.parseSqlStatement(String sql)

...
            int skipToPosition = i;
            while (i < statement.length) {
                skipToPosition = skipCommentsAndQuotes(statement, i);
                if (i == skipToPosition) {
                    break;
                }
                else {
                    i = skipToPosition;
                }
            }

...

I use NamedParameterJdbcTemplate to run SQLs on our own DBMS engine in the company. I have special cases to use parameters in comment, for example, "/@ queryCkey(:cafeId) / SELECT ... ",

If it is allowed in NamedParameterJdbcTemplate, it will be very helpful for our system. ( For your information, MyBatis allows parameters in comments of SQL)

I'd like you to consider adding an optional flag to support this.

var template = new NamedParameterJdbcTemplate(dataSource);
template.setAllowParametersInComments(true);

[pashazadeh]

Setting this kind of parameters is not supported by PreparedStatements, because they change the execution plan of the query. Cause major databases support which support prepared-statements just parse the query once, so they don't accept placeholders for what changes the execution plan.

[benelog]

@pashazadeh I understand your concerns. It is reasonable default to disable parameters in comments.

However, if it is customizable, it will be very helpful for developers who use a special DBMS like us. If the JDBC driver allows it, I think it's unnatural to prevent it in the Spring framework. So, MyBatis also seems to allow this.

Since Spring framework is traditionally highly customizable, I expect that.

[junoyoon]

Or.. I think it can be solved by making skipCommentsAndQuotes protected and non-static so that it can be extended in the sub-class of NamedParemterJdbcTemplate .

[snicoll]

The problem is that we delegate everything to NamedParameterUtils that is static at the moment. I think we'd probably need to make it non static and offer some way to customize it.

With the interesting feedback of @pashazadeh, I am wondering though if it would be a good idea to support it.

@benelog can't you use simple ? for those advanced use cases, rather than named parameters?

[benelog]

@snicoll Unfortunately, all queries in our system require parameters within SQL hints. We use our company's own DBMS system, and this DBMS requires that the shard key be included in the hint. The cost of giving up the NamedParameterJdbcTemplate is high in this case.

We are currently working around this issue by creating a class that inherited the NamedParameterJdbcTemplate. It is also possible to maintain this workaround. I raised this issue because there is an internal opinion that it would be beneficial if NamedParameterJdbcTemplate could directly support what is already allowed in MyBatis.

If we are the only ones who want this feature, it would be okay for you to consider implementing it when other users request it too.

[snicoll]

We are currently working around this issue by creating a class that inherited the NamedParameterJdbcTemplate.

Can you share your workaround?

[benelog]

@snicoll I'm sharing it with you for reference to the code below.

I think it's just workaround, not a natural way.

import java.util.List;
import java.util.regex.Pattern;

import javax.sql.DataSource;

import org.apache.commons.lang3.ObjectUtils;
import org.springframework.core.convert.converter.Converter;
import org.springframework.jdbc.core.PreparedStatementCreatorFactory;
import org.springframework.jdbc.core.SqlParameter;
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
import org.springframework.jdbc.core.namedparam.NamedParameterUtils;
import org.springframework.jdbc.core.namedparam.ParsedSql;
import org.springframework.jdbc.core.namedparam.SqlParameterSource;
import org.springframework.lang.Nullable;
import org.springframework.util.ConcurrentLruCache;

import com.navercorp.nbaset.spring.jdbc.comment.NbaseNodeCommentGenerator;

// NamedParameterJdbcTemplate parameters in comment issue
// https://github.com/spring-projects/spring-framework/issues/22255
public class NbaseNamedParameterJdbcTemplate extends NamedParameterJdbcTemplate
    implements NbaseNamedParameterJdbcOperations {
    private static final Pattern CKEY_COMMENT_PATTERN = Pattern.compile("(/\\*@.*?\\*/)",
        Pattern.MULTILINE | Pattern.DOTALL);

    @Nullable
    private final NbaseNodeCommentGenerator commentGenerator;
    private final Converter<String, String> sqlConverter;
    private volatile ConcurrentLruCache<String, ParsedSql> parsedSqlCache =
        new ConcurrentLruCache<>(DEFAULT_CACHE_LIMIT, this::createParsedSqlInstantWithComment);

    public NbaseNamedParameterJdbcTemplate(DataSource dataSource) {
        this(dataSource, null, null);
    }

    public NbaseNamedParameterJdbcTemplate(DataSource dataSource,
        @Nullable NbaseNodeCommentGenerator commentGenerator) {
        this(dataSource, commentGenerator, null);
    }

    public NbaseNamedParameterJdbcTemplate(
        DataSource dataSource,
        @Nullable NbaseNodeCommentGenerator commentGenerator,
        Converter<String, String> sqlConverter) {

        super(dataSource);
        this.commentGenerator = commentGenerator;
        this.sqlConverter = ObjectUtils.defaultIfNull(sqlConverter, sql -> sql);
    }

    @Override
    public int getCacheLimit() {
        return this.parsedSqlCache.capacity();
    }

    @Override
    public void setCacheLimit(int cacheLimit) {
        this.parsedSqlCache = new ConcurrentLruCache<>(cacheLimit, this::createParsedSqlInstantWithComment);
    }

    @Override
    protected ParsedSql getParsedSql(String sql) {
        return this.parsedSqlCache.get(sql);
    }

    @Override
    protected PreparedStatementCreatorFactory getPreparedStatementCreatorFactory(
        ParsedSql parsedSql, SqlParameterSource paramSource) {

        String sqlToUse = NamedParameterUtils.substituteNamedParameters(parsedSql, paramSource);
        sqlToUse = this.sqlConverter.convert(sqlToUse);
        List<SqlParameter> declaredParameters = NamedParameterUtils.buildSqlParameterList(parsedSql, paramSource);
        return new PreparedStatementCreatorFactory(sqlToUse, declaredParameters);
    }

    private ParsedSql createParsedSqlInstantWithComment(String sql) {
        if (this.commentGenerator != null && !CKEY_COMMENT_PATTERN.matcher(sql).find()) {
            sql = this.commentGenerator.queryComment() + sql;
        }

        return NbaseNamedParameterUtils.createParsedSqlInstantWithComment(sql);
    }
}

import static java.util.Comparator.comparingInt;
import static java.util.Objects.requireNonNull;
import static org.springframework.util.ReflectionUtils.accessibleConstructor;
import static org.springframework.util.ReflectionUtils.findField;
import static org.springframework.util.ReflectionUtils.getField;
import static org.springframework.util.ReflectionUtils.setField;

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.apache.commons.lang3.StringUtils;
import org.springframework.jdbc.core.namedparam.NamedParameterUtils;
import org.springframework.jdbc.core.namedparam.ParsedSql;

public class NbaseNamedParameterUtils {
    private static final Field PARSED_SQL_PARAMETER_INDEXES = findAndMakeAccessible("parameterIndexes");
    private static final Field PARSED_SQL_PARAMETER_NAMES = findAndMakeAccessible("parameterNames");
    private static final Field PARSED_SQL_NAMED_PARAMETER_COUNT = findAndMakeAccessible("namedParameterCount");
    private static final Field PARSED_SQL_UNNAMED_PARAMETER_COUNT = findAndMakeAccessible("unnamedParameterCount");
    private static final Field PARSED_SQL_TOTAL_PARAMETER_COUNT = findAndMakeAccessible("totalParameterCount");

    public static ParsedSql createParsedSqlInstantWithComment(String sql) {
        ParsedSql parsedSql = NamedParameterUtils.parseSqlStatement(sql);
        List<String> commentParameterNames = getFieldValue(parsedSql, PARSED_SQL_PARAMETER_NAMES);
        List<int[]> commentParameterIndexes = getFieldValue(parsedSql, PARSED_SQL_PARAMETER_INDEXES);
        int totalCount = getFieldValue(parsedSql, PARSED_SQL_TOTAL_PARAMETER_COUNT);
        int namedCount = getFieldValue(parsedSql, PARSED_SQL_NAMED_PARAMETER_COUNT);
        int unnamedCount = getFieldValue(parsedSql, PARSED_SQL_UNNAMED_PARAMETER_COUNT);

        for (MatchedComment each : MatchedComment.findComments(sql)) {
            ParsedSql parsedComment = NamedParameterUtils.parseSqlStatement(each.getUncommented());
            commentParameterNames.addAll(getFieldValue(parsedComment, PARSED_SQL_PARAMETER_NAMES));
            commentParameterIndexes.addAll(getFieldValue(parsedComment, PARSED_SQL_PARAMETER_INDEXES));
            int commentTotalParamCount = getFieldValue(parsedComment, PARSED_SQL_TOTAL_PARAMETER_COUNT);
            int commentNamedParamCount = getFieldValue(parsedComment, PARSED_SQL_NAMED_PARAMETER_COUNT);
            int commentUnnamedParamCount = getFieldValue(parsedComment, PARSED_SQL_UNNAMED_PARAMETER_COUNT);
            totalCount += commentTotalParamCount;
            namedCount += commentNamedParamCount;
            unnamedCount += commentUnnamedParamCount;
        }
        return createParsedSqlInstance(sql, commentParameterNames, commentParameterIndexes, totalCount, namedCount, unnamedCount);
    }

    private static ParsedSql createParsedSqlInstance(
        String originalSql,
        List<String> commentParameterNames,
        List<int[]> commentParameterIndexes,
        int totalCount,
        int namedCount,
        int unnamedCount) {

        try {
            ParsedSql parsedSql = accessibleConstructor(ParsedSql.class, String.class).newInstance(originalSql);

            List<CommentPair> sorted = sortCommentParameter(requireNonNull(commentParameterNames), commentParameterIndexes);
            List<String> newCommentParameterNames = new ArrayList<>();
            List<int[]> newCommentParameterIndexes = new ArrayList<>();

            for (CommentPair commentPair : sorted) {
                newCommentParameterNames.add(commentPair.getName());
                newCommentParameterIndexes.add(commentPair.getIndex());
            }

            setField(PARSED_SQL_PARAMETER_NAMES, parsedSql, newCommentParameterNames);
            setField(PARSED_SQL_PARAMETER_INDEXES, parsedSql, newCommentParameterIndexes);
            setField(PARSED_SQL_TOTAL_PARAMETER_COUNT, parsedSql, totalCount);
            setField(PARSED_SQL_NAMED_PARAMETER_COUNT, parsedSql, namedCount);
            setField(PARSED_SQL_UNNAMED_PARAMETER_COUNT, parsedSql, unnamedCount);
            return parsedSql;
        } catch (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) {
            throw new NbaseParseSqlException("Parse sql reflection exception. sql: " + originalSql, e);
        }
    }

    private static List<CommentPair> sortCommentParameter(
        List<String> commentParameterNames, List<int[]> commentParameterIndexes) {

        List<CommentPair> commentParameters = new ArrayList<>();
        for (int i = 0, size = commentParameterNames.size(); i < size; i++) {
            int[] index = commentParameterIndexes.get(i);
            commentParameters.add(CommentPair.of(commentParameterNames.get(i), index));
        }
        commentParameters.sort(comparingInt(l -> l.getIndex()[0]));
        return commentParameters;
    }

    private static Field findAndMakeAccessible(String fieldName) {
        Field field = requireNonNull(findField(ParsedSql.class, fieldName));
        field.setAccessible(true);
        return field;
    }

    @SuppressWarnings("unchecked")
    private static <T> T getFieldValue(ParsedSql parsedSql, Field field) {
        return (T)requireNonNull(getField(field, parsedSql));
    }

    static class CommentPair {
        private String name;
        private int[] index;

        static CommentPair of(String name, int[] index) {
            CommentPair pair = new CommentPair();
            pair.name = name;
            pair.index = index;
            return pair;
        }

        public String getName() {
            return this.name;
        }

        public int[] getIndex() {
            return this.index;
        }
    }

    static class MatchedComment {
        private static final Pattern COMMENT_PATTERN = Pattern.compile("(/\\*@.*?\\*/)", Pattern.MULTILINE | Pattern.DOTALL);

        String original;
        String text;
        int start;
        int end;

        static List<MatchedComment> findComments(String sql) {
            List<MatchedComment> matches = new ArrayList<>();
            Matcher matcher = MatchedComment.COMMENT_PATTERN.matcher(sql);
            while (matcher.find()) {
                MatchedComment match = new MatchedComment();
                match.original = sql;
                match.text = matcher.group();
                match.start = matcher.start();
                match.end = matcher.end();
                matches.add(match);
            }
            return matches;
        }

        String getUncommented() {
            return StringUtils.repeat(" ", start) + "   " + text.substring(3, text.length() - 2) + "  ";
        }
    }
}