diff --git a/pom.xml b/pom.xml index 7580741..dbcd3c7 100644 --- a/pom.xml +++ b/pom.xml @@ -269,13 +269,13 @@ org.apache.derby derby - 10.17.1.0 + 10.15.2.0 test org.apache.derby derbytools - 10.16.1.1 + 10.15.2.0 test diff --git a/src/main/java/com/github/susom/database/Database.java b/src/main/java/com/github/susom/database/Database.java index 2d0f8c6..355f32e 100644 --- a/src/main/java/com/github/susom/database/Database.java +++ b/src/main/java/com/github/susom/database/Database.java @@ -35,8 +35,11 @@ public interface Database extends Supplier { * Note this call does not actually execute the SQL. * * @param sql the SQL to execute, optionally containing indexed ("?") or - * named (":foo") parameters. To include the characters '?' or ':' - * in the SQL you must escape them with two ("??" or "::"). You + * named (":foo") parameters. By default, "smart" parsing is used: + * '?' and ':' characters inside string literals, quoted identifiers, + * and comments are treated as regular SQL text and do not need to be + * escaped. When {@link Options#useSmartSqlParameterParsing()} returns {@code false} (legacy mode), literal '?' or ':' + * characters must be escaped by doubling them ("??" or "::"). You * MUST be careful not to pass untrusted strings in as SQL, since * this will be executed in the database. * @return an interface for further manipulating the statement; never null @@ -54,8 +57,11 @@ public interface Database extends Supplier { * Note this call does not actually execute the SQL. * * @param sql the SQL to execute, optionally containing indexed ("?") or - * named (":foo") parameters. To include the characters '?' or ':' - * in the SQL you must escape them with two ("??" or "::"). You + * named (":foo") parameters. By default, "smart" parsing is used: + * '?' and ':' characters inside string literals, quoted identifiers, + * and comments are treated as regular SQL text and do not need to be + * escaped. When {@link Options#useSmartSqlParameterParsing()} returns {@code false} (legacy mode), literal '?' or ':' + * characters must be escaped by doubling them ("??" or "::"). You * MUST be careful not to pass untrusted strings in as SQL, since * this will be executed in the database. * @return an interface for further manipulating the statement; never null @@ -73,8 +79,11 @@ public interface Database extends Supplier { * Note this call does not actually execute the SQL. * * @param sql the SQL to execute, optionally containing indexed ("?") or - * named (":foo") parameters. To include the characters '?' or ':' - * in the SQL you must escape them with two ("??" or "::"). You + * named (":foo") parameters. By default, "smart" parsing is used: + * '?' and ':' characters inside string literals, quoted identifiers, + * and comments are treated as regular SQL text and do not need to be + * escaped. When {@link Options#useSmartSqlParameterParsing()} returns {@code false} (legacy mode), literal '?' or ':' + * characters must be escaped by doubling them ("??" or "::"). You * MUST be careful not to pass untrusted strings in as SQL, since * this will be executed in the database. * @return an interface for further manipulating the statement; never null @@ -92,8 +101,11 @@ public interface Database extends Supplier { * Note this call does not actually execute the SQL. * * @param sql the SQL to execute, optionally containing indexed ("?") or - * named (":foo") parameters. To include the characters '?' or ':' - * in the SQL you must escape them with two ("??" or "::"). You + * named (":foo") parameters. By default, "smart" parsing is used: + * '?' and ':' characters inside string literals, quoted identifiers, + * and comments are treated as regular SQL text and do not need to be + * escaped. When {@link Options#useSmartSqlParameterParsing()} returns {@code false} (legacy mode), literal '?' or ':' + * characters must be escaped by doubling them ("??" or "::"). You * MUST be careful not to pass untrusted strings in as SQL, since * this will be executed in the database. * @return an interface for further manipulating the statement; never null @@ -111,8 +123,11 @@ public interface Database extends Supplier { * Note this call does not actually execute the SQL. * * @param sql the SQL to execute, optionally containing indexed ("?") or - * named (":foo") parameters. To include the characters '?' or ':' - * in the SQL you must escape them with two ("??" or "::"). You + * named (":foo") parameters. By default, "smart" parsing is used: + * '?' and ':' characters inside string literals, quoted identifiers, + * and comments are treated as regular SQL text and do not need to be + * escaped. When {@link Options#useSmartSqlParameterParsing()} returns {@code false} (legacy mode), literal '?' or ':' + * characters must be escaped by doubling them ("??" or "::"). You * MUST be careful not to pass untrusted strings in as SQL, since * this will be executed in the database. * @return an interface for further manipulating the statement; never null diff --git a/src/main/java/com/github/susom/database/MixedParameterSql.java b/src/main/java/com/github/susom/database/MixedParameterSql.java index 307ea75..a057ecb 100644 --- a/src/main/java/com/github/susom/database/MixedParameterSql.java +++ b/src/main/java/com/github/susom/database/MixedParameterSql.java @@ -25,10 +25,20 @@ /** * Convenience class to allow use of (:mylabel) for SQL parameters in addition to - * positional (?) parameters. This doesn't do any smart parsing of the SQL, it is just - * looking for ':' and '?' characters. If the SQL needs to include an actual ':' or '?' - * character, use two of them ('::' or '??'), and they will be replaced with a - * single ':' or '?'. + * positional (?) parameters. + * + *

By default this uses "smart" parsing, which is aware of ordinary SQL syntax. A + * ':' or '?' character that appears inside a single-quoted string literal ('...'), + * a double-quoted identifier ("..."), a line comment (-- ...) or a block comment + * (/* ... */) is treated as regular SQL text and does not need to be escaped. + * PostgreSQL-style casts (::type) are recognized and left untouched. Only a '?' or + * ':name' occurring in ordinary SQL is treated as a bind variable.

+ * + *

The legacy behavior can be requested by passing {@code useSmartParsing=false} + * to the constructor. In that mode no smart parsing is done, and the SQL is simply + * scanned for ':' and '?' characters. If the SQL needs to include an actual ':' or + * '?' character in that mode, use two of them ('::' or '??'), and they will be + * replaced with a single ':' or '?'.

* * @author garricko */ @@ -36,7 +46,22 @@ public class MixedParameterSql { private final String sqlToExecute; private final Object[] args; + /** + * Parse the SQL using the default "smart" parsing. Equivalent to calling + * {@link #MixedParameterSql(String, List, Map, boolean)} with {@code true}. + */ public MixedParameterSql(String sql, List positionalArgs, Map nameToArg) { + this(sql, positionalArgs, nameToArg, true); + } + + /** + * @param useSmartParsing if true, ':' and '?' inside string literals, quoted + * identifiers and comments are ignored (and do not need to + * be escaped); if false, the legacy escape-by-doubling + * behavior is used + */ + public MixedParameterSql(String sql, List positionalArgs, Map nameToArg, + boolean useSmartParsing) { if (positionalArgs == null) { positionalArgs = new ArrayList<>(); } @@ -48,6 +73,140 @@ public MixedParameterSql(String sql, List positionalArgs, Map argNamesList = new ArrayList<>(); List rewrittenArgs = new ArrayList<>(); List argsList = new ArrayList<>(); + int currentPositionalArg = useSmartParsing + ? parseSmart(sql, positionalArgs, nameToArg, newSql, argsList, argNamesList, rewrittenArgs) + : parseLegacy(sql, positionalArgs, nameToArg, newSql, argsList, argNamesList, rewrittenArgs); + + this.sqlToExecute = newSql.toString(); + args = argsList.toArray(new Object[argsList.size()]); + + // Sanity check number of arguments to provide a better error message + if (currentPositionalArg != positionalArgs.size()) { + throw new DatabaseException("Wrong number of positional parameters were provided (expected: " + + currentPositionalArg + ", actual: " + positionalArgs.size() + ")"); + } + if (nameToArg.size() > args.length - Math.max(0, positionalArgs.size() - 1) + rewrittenArgs.size()) { + Set unusedNames = new HashSet<>(nameToArg.keySet()); + unusedNames.removeAll(argNamesList); + unusedNames.removeAll(rewrittenArgs); + if (!unusedNames.isEmpty()) { + throw new DatabaseException("These named parameters do not exist in the query: " + unusedNames); + } + } + } + + /** + * Context-aware parsing that skips over string literals, quoted identifiers and + * comments so ':' and '?' characters within them are left untouched. + * + * @return the number of positional parameters consumed + */ + private int parseSmart(String sql, List positionalArgs, Map nameToArg, + StringBuilder newSql, List argsList, List argNamesList, + List rewrittenArgs) { + int currentPositionalArg = 0; + int length = sql.length(); + int i = 0; + while (i < length) { + char c = sql.charAt(i); + switch (c) { + case '$': + // PostgreSQL dollar-quoting: $$...$$ or $tag$...$tag$ + if (i + 1 < length && (sql.charAt(i + 1) == '$' || Character.isLetter(sql.charAt(i + 1)) || sql.charAt(i + 1) == '_')) { + int tagEnd = i + 1; + while (tagEnd < length && sql.charAt(tagEnd) != '$') { + tagEnd++; + } + if (tagEnd < length) { + String tag = sql.substring(i, tagEnd + 1); // e.g. "$$" or "$tag$" + int bodyStart = tagEnd + 1; + int closeIndex = sql.indexOf(tag, bodyStart); + if (closeIndex >= 0) { + newSql.append(sql, i, closeIndex + tag.length()); + i = closeIndex + tag.length(); + } else { + // Unterminated dollar-quoted string - copy to end + newSql.append(sql, i, length); + i = length; + } + } else { + newSql.append(c); + i++; + } + } else { + newSql.append(c); + i++; + } + break; + case '[': + // SQL Server bracketed identifier: [...] (] is escaped as ]]) + i = appendBracketedIdentifier(sql, i, newSql); + break; + case '\'': + // Single-quoted string literal (with '' as an embedded quote) + i = appendQuoted(sql, i, '\'', newSql); + break; + case '"': + // Double-quoted identifier (with "" as an embedded quote) + i = appendQuoted(sql, i, '"', newSql); + break; + case '-': + if (i + 1 < length && sql.charAt(i + 1) == '-') { + i = appendLineComment(sql, i, newSql); + } else { + newSql.append(c); + i++; + } + break; + case '/': + if (i + 1 < length && sql.charAt(i + 1) == '*') { + i = appendBlockComment(sql, i, newSql); + } else { + newSql.append(c); + i++; + } + break; + case '?': + currentPositionalArg = appendPositionalParam(newSql, currentPositionalArg, positionalArgs, argsList); + i++; + break; + case ':': + if (i + 1 < length && sql.charAt(i + 1) == ':') { + // PostgreSQL cast operator (::) - leave it untouched + newSql.append("::"); + i += 2; + } else if (i + 1 < length && Character.isJavaIdentifierPart(sql.charAt(i + 1))) { + // Named parameter (":foo") + int endOfNameIndex = i + 1; + while (endOfNameIndex < length && Character.isJavaIdentifierPart(sql.charAt(endOfNameIndex))) { + endOfNameIndex++; + } + appendNamedParam(newSql, sql.substring(i + 1, endOfNameIndex), nameToArg, argsList, argNamesList, + rewrittenArgs); + i = endOfNameIndex; + } else { + // A lone ':' that is not a parameter (e.g. an operator) - leave it as-is + newSql.append(c); + i++; + } + break; + default: + newSql.append(c); + i++; + } + } + return currentPositionalArg; + } + + /** + * Legacy parsing that treats every ':' and '?' as a parameter marker, and relies + * on doubling ('::' or '??') to escape a literal ':' or '?'. + * + * @return the number of positional parameters consumed + */ + private int parseLegacy(String sql, List positionalArgs, Map nameToArg, + StringBuilder newSql, List argsList, List argNamesList, + List rewrittenArgs) { int searchIndex = 0; int currentPositionalArg = 0; while (searchIndex < sql.length()) { @@ -78,21 +237,8 @@ public MixedParameterSql(String sql, List positionalArgs, Map positionalArgs, Map= positionalArgs.size()) { - throw new DatabaseException("Not enough positional parameters (" + positionalArgs.size() + ") were provided"); - } - if (positionalArgs.get(currentPositionalArg) instanceof RewriteArg) { - newSql.append(((RewriteArg) positionalArgs.get(currentPositionalArg)).sql); - } else { - newSql.append('?'); - argsList.add(positionalArgs.get(currentPositionalArg)); - } - currentPositionalArg++; + currentPositionalArg = appendPositionalParam(newSql, currentPositionalArg, positionalArgs, argsList); searchIndex = nextQmIndex + 1; } } + return currentPositionalArg; + } - this.sqlToExecute = newSql.toString(); - args = argsList.toArray(new Object[argsList.size()]); + /** + * Emit a named parameter as a '?' placeholder (or its rewritten SQL) and record + * its value/name for binding. + */ + private void appendNamedParam(StringBuilder newSql, String paramName, Map nameToArg, + List argsList, List argNamesList, List rewrittenArgs) { + boolean secretParam = paramName.startsWith("secret"); + Object arg = nameToArg.get(paramName); + if (arg instanceof RewriteArg) { + newSql.append(((RewriteArg) arg).sql); + rewrittenArgs.add(paramName); + } else { + newSql.append('?'); + if (nameToArg.containsKey(paramName)) { + argsList.add(secretParam ? new SecretArg(arg): arg); + } else { + throw new DatabaseException("The SQL requires parameter ':" + paramName + "' but no value was provided"); + } + argNamesList.add(paramName); + } + } - // Sanity check number of arguments to provide a better error message - if (currentPositionalArg != positionalArgs.size()) { - throw new DatabaseException("Wrong number of positional parameters were provided (expected: " - + currentPositionalArg + ", actual: " + positionalArgs.size() + ")"); + /** + * Emit a positional parameter as a '?' placeholder (or its rewritten SQL) and + * record its value for binding. + * + * @return the index of the next positional parameter to consume + */ + private int appendPositionalParam(StringBuilder newSql, int currentPositionalArg, + List positionalArgs, List argsList) { + if (currentPositionalArg >= positionalArgs.size()) { + throw new DatabaseException("Not enough positional parameters (" + positionalArgs.size() + ") were provided"); } - if (nameToArg.size() > args.length - Math.max(0, positionalArgs.size() - 1) + rewrittenArgs.size()) { - Set unusedNames = new HashSet<>(nameToArg.keySet()); - unusedNames.removeAll(argNamesList); - unusedNames.removeAll(rewrittenArgs); - if (!unusedNames.isEmpty()) { - throw new DatabaseException("These named parameters do not exist in the query: " + unusedNames); + if (positionalArgs.get(currentPositionalArg) instanceof RewriteArg) { + newSql.append(((RewriteArg) positionalArgs.get(currentPositionalArg)).sql); + } else { + newSql.append('?'); + argsList.add(positionalArgs.get(currentPositionalArg)); + } + return currentPositionalArg + 1; + } + + /** + * Copy a SQL Server bracketed identifier ({@code [...]}), including the surrounding + * brackets, verbatim into the output. A {@code ]]} inside the identifier is treated + * as an escaped {@code ]}, not a terminator. + * + * @param start index of the opening '[' + * @return the index immediately after the closing ']' (or the end of the SQL if + * the identifier is not terminated) + */ + private static int appendBracketedIdentifier(String sql, int start, StringBuilder newSql) { + int length = sql.length(); + newSql.append('['); + int i = start + 1; + while (i < length) { + char c = sql.charAt(i); + if (c == ']') { + if (i + 1 < length && sql.charAt(i + 1) == ']') { + // Escaped ]] - part of the identifier + newSql.append("]]"); + i += 2; + continue; + } + // Closing bracket + newSql.append(']'); + return i + 1; + } + newSql.append(c); + i++; + } + // Unterminated - everything remaining has already been copied + return i; + } + + /** + * Copy a quoted region (string literal or quoted identifier), including the + * surrounding quote characters, verbatim into the output. A doubled quote + * ({@code ''} or {@code ""}) is treated as an embedded quote, not a terminator. + * + * @param start index of the opening quote + * @return the index immediately after the closing quote (or the end of the SQL if + * the quote is not terminated) + */ + private static int appendQuoted(String sql, int start, char quote, StringBuilder newSql) { + int length = sql.length(); + newSql.append(quote); + int i = start + 1; + while (i < length) { + char c = sql.charAt(i); + if (c == quote) { + if (i + 1 < length && sql.charAt(i + 1) == quote) { + // Embedded (doubled) quote - part of the quoted text + newSql.append(quote).append(quote); + i += 2; + continue; + } + // Closing quote + newSql.append(quote); + return i + 1; + } + newSql.append(c); + i++; + } + // Unterminated quote - everything remaining has already been copied + return i; + } + + /** + * Copy a line comment ({@code -- ...}) verbatim into the output, up to but not + * including the terminating newline. + * + * @param start index of the first '-' + * @return the index of the newline that ends the comment (or the end of the SQL) + */ + private static int appendLineComment(String sql, int start, StringBuilder newSql) { + int length = sql.length(); + int i = start; + while (i < length && sql.charAt(i) != '\n') { + newSql.append(sql.charAt(i)); + i++; + } + return i; + } + + /** + * Copy a block comment ({@code /}{@code * ... *}{@code /}) verbatim into the + * output. Nested block comments (as supported by PostgreSQL) are handled. + * + * @param start index of the opening '/' + * @return the index immediately after the closing "*/" (or the end of the SQL + * if the comment is not terminated) + */ + private static int appendBlockComment(String sql, int start, StringBuilder newSql) { + int length = sql.length(); + newSql.append("/*"); + int i = start + 2; + int depth = 1; + while (i < length && depth > 0) { + if (i + 1 < length && sql.charAt(i) == '/' && sql.charAt(i + 1) == '*') { + newSql.append("/*"); + i += 2; + depth++; + } else if (i + 1 < length && sql.charAt(i) == '*' && sql.charAt(i + 1) == '/') { + newSql.append("*/"); + i += 2; + depth--; + } else { + newSql.append(sql.charAt(i)); + i++; } } + return i; } public String getSqlToExecute() { diff --git a/src/main/java/com/github/susom/database/Options.java b/src/main/java/com/github/susom/database/Options.java index 3c8eb49..95ef70c 100644 --- a/src/main/java/com/github/susom/database/Options.java +++ b/src/main/java/com/github/susom/database/Options.java @@ -145,4 +145,29 @@ public interface Options { * both {@code argString()} and {@code argClobString()} methods. */ int maxStringLengthParam(); + + /** + * Control how bind variables ({@code ?} and {@code :name}) are located within the + * SQL text before it is handed to the JDBC driver. + * + *

When this returns true (the default), "smart" parsing is used. The parser is + * aware of ordinary SQL syntax, so {@code ?} and {@code :} characters that appear + * inside single-quoted string literals ({@code '...'}), double-quoted identifiers + * ({@code "..."}), line comments ({@code -- ...}) and block comments + * ({@code /* ... *}{@code /}) are treated as regular SQL text and do not + * need to be escaped. PostgreSQL-style casts ({@code ::type}) are also recognized + * and left untouched. Only a {@code ?} or {@code :name} occurring in ordinary SQL + * is treated as a bind variable.

+ * + *

When this returns false, the legacy behavior is used: every {@code ?} and + * {@code :} in the SQL is treated as a parameter marker regardless of where it + * appears, and a literal {@code ?} or {@code :} must be escaped by doubling it + * ({@code ??} or {@code ::}).

+ * + * @return true to use context-aware ("smart") parsing, false to use the legacy + * escape-by-doubling behavior + */ + default boolean useSmartSqlParameterParsing() { + return true; + } } diff --git a/src/main/java/com/github/susom/database/OptionsDefault.java b/src/main/java/com/github/susom/database/OptionsDefault.java index 4920e77..6c21cb7 100644 --- a/src/main/java/com/github/susom/database/OptionsDefault.java +++ b/src/main/java/com/github/susom/database/OptionsDefault.java @@ -98,4 +98,9 @@ public Calendar calendarForTimestamps() { public int maxStringLengthParam() { return 4000; } + + @Override + public boolean useSmartSqlParameterParsing() { + return true; + } } diff --git a/src/main/java/com/github/susom/database/OptionsOverride.java b/src/main/java/com/github/susom/database/OptionsOverride.java index 99ae1b4..afdb59d 100644 --- a/src/main/java/com/github/susom/database/OptionsOverride.java +++ b/src/main/java/com/github/susom/database/OptionsOverride.java @@ -122,4 +122,9 @@ public Calendar calendarForTimestamps() { public int maxStringLengthParam() { return parent.maxStringLengthParam(); } + + @Override + public boolean useSmartSqlParameterParsing() { + return parent.useSmartSqlParameterParsing(); + } } diff --git a/src/main/java/com/github/susom/database/SqlInsertImpl.java b/src/main/java/com/github/susom/database/SqlInsertImpl.java index 323bb02..3bbd8df 100644 --- a/src/main/java/com/github/susom/database/SqlInsertImpl.java +++ b/src/main/java/com/github/susom/database/SqlInsertImpl.java @@ -465,7 +465,8 @@ private int[] updateBatch() { Exception logEx = null; try { for (Batch batch : batched) { - MixedParameterSql mpSql = new MixedParameterSql(sql, batch.parameterList, batch.parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, batch.parameterList, batch.parameterMap, + options.useSmartSqlParameterParsing()); if (firstRowParameters == null) { executeSql = mpSql.getSqlToExecute(); firstRowParameters = mpSql.getArgs(); @@ -539,7 +540,8 @@ private int updateInternal(int expectedNumAffectedRows) { String errorCode = null; Exception logEx = null; try { - MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap, + options.useSmartSqlParameterParsing()); executeSql = mpSql.getSqlToExecute(); parameters = mpSql.getArgs(); @@ -602,7 +604,8 @@ private Long updateInternal(int expectedNumAffectedRows, @Nonnull String pkToRet String errorCode = null; Exception logEx = null; try { - MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap, + options.useSmartSqlParameterParsing()); executeSql = mpSql.getSqlToExecute(); parameters = mpSql.getArgs(); @@ -673,7 +676,8 @@ private T updateInternal(int expectedNumAffectedRows, @Nonnull String pkToRe String errorCode = null; Exception logEx = null; try { - MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap, + options.useSmartSqlParameterParsing()); executeSql = mpSql.getSqlToExecute(); parameters = mpSql.getArgs(); @@ -745,7 +749,8 @@ private Long updateInternalWithGeneratedKeys(int expectedNumAffectedRows, @Nonnu String errorCode = null; Exception logEx = null; try { - MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap, + options.useSmartSqlParameterParsing()); executeSql = mpSql.getSqlToExecute(); parameters = mpSql.getArgs(); diff --git a/src/main/java/com/github/susom/database/SqlSelectImpl.java b/src/main/java/com/github/susom/database/SqlSelectImpl.java index b5d1411..4058359 100644 --- a/src/main/java/com/github/susom/database/SqlSelectImpl.java +++ b/src/main/java/com/github/susom/database/SqlSelectImpl.java @@ -711,7 +711,8 @@ private T queryWithTimeout(RowsHandler handler) { String errorCode = null; Exception logEx = null; try { - MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap, + options.useSmartSqlParameterParsing()); executeSql = mpSql.getSqlToExecute(); parameters = mpSql.getArgs(); diff --git a/src/main/java/com/github/susom/database/SqlUpdateImpl.java b/src/main/java/com/github/susom/database/SqlUpdateImpl.java index 5da6d62..0bed2dd 100644 --- a/src/main/java/com/github/susom/database/SqlUpdateImpl.java +++ b/src/main/java/com/github/susom/database/SqlUpdateImpl.java @@ -284,7 +284,8 @@ private int updateInternal(int expectedNumAffectedRows) { String errorCode = null; Exception logEx = null; try { - MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap); + MixedParameterSql mpSql = new MixedParameterSql(sql, parameterList, parameterMap, + options.useSmartSqlParameterParsing()); executeSql = mpSql.getSqlToExecute(); parameters = mpSql.getArgs(); diff --git a/src/test/java/com/github/susom/database/test/CommonTest.java b/src/test/java/com/github/susom/database/test/CommonTest.java index 82727cb..b7cbf5c 100644 --- a/src/test/java/com/github/susom/database/test/CommonTest.java +++ b/src/test/java/com/github/susom/database/test/CommonTest.java @@ -1871,7 +1871,7 @@ public void stringDateFunctions() { .addColumn("d").asDate().schema().execute(db); db.toInsert("insert into dbtest (d) values (" - + db.flavor().dateAsSqlFunction(date, db.options().calendarForTimestamps()).replace(":", "::") + ")") + + db.flavor().dateAsSqlFunction(date, db.options().calendarForTimestamps()) + ")") .insert(1); assertEquals("1970-01-02 18:17:36.789000-0400", new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS000Z").format( @@ -1884,7 +1884,7 @@ public void stringDateFunctions() { db.toDelete("delete from dbtest where d=?").argDate(date).update(1); db.toInsert("insert into dbtest (d) values (" - + db.flavor().dateAsSqlFunction(date, db.options().calendarForTimestamps()).replace(":", "::") + ")") + + db.flavor().dateAsSqlFunction(date, db.options().calendarForTimestamps()) + ")") .insert(1); assertEquals("1970-01-03 02:17:36.789000+0400", new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS000Z").format( @@ -1892,7 +1892,7 @@ public void stringDateFunctions() { // Verify the function maps correctly for equals operations as well db.toDelete("delete from dbtest where d=" + db.flavor().dateAsSqlFunction(date, - db.options().calendarForTimestamps()).replace(":", "::")).update(1); + db.options().calendarForTimestamps())).update(1); } finally { TimeZone.setDefault(defaultTZ); } diff --git a/src/test/java/com/github/susom/database/test/DatabaseTest.java b/src/test/java/com/github/susom/database/test/DatabaseTest.java index a22417f..d83fcb3 100644 --- a/src/test/java/com/github/susom/database/test/DatabaseTest.java +++ b/src/test/java/com/github/susom/database/test/DatabaseTest.java @@ -45,6 +45,7 @@ import com.github.susom.database.DatabaseProvider; import com.github.susom.database.DebugSql; import com.github.susom.database.Flavor; +import com.github.susom.database.Options; import com.github.susom.database.OptionsDefault; import com.github.susom.database.OptionsOverride; import com.github.susom.database.RowStub; @@ -84,6 +85,12 @@ public boolean isLogParameters() { return true; } }; + private OptionsOverride optionsLegacyParsing = new OptionsOverride(options) { + @Override + public boolean useSmartSqlParameterParsing() { + return false; + } + }; private LogCaptureAppender capturedLog; @Before @@ -336,11 +343,308 @@ public void sqlArgLongNamed() throws Exception { control.replay(); - assertNull(new DatabaseImpl(c, options).toSelect("select '::a' from b where c=:c").argLong("c", 1L).queryLongOrNull()); + // With smart parsing, a ':' inside a string literal is left alone and does not need escaping + assertNull(new DatabaseImpl(c, options).toSelect("select ':a' from b where c=:c").argLong("c", 1L).queryLongOrNull()); + + control.verify(); + } + + /** + * Assert that {@code inputSql} (which uses no bind parameters) is rewritten to + * {@code expectedSql} when parsed with the given options. + */ + private void assertParsedSqlNoArgs(Options opts, String inputSql, String expectedSql) throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + expect(c.prepareStatement(expectedSql)).andReturn(ps); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, opts).toSelect(inputSql).queryLongOrNull()); control.verify(); } + @Test + public void smartParsingIgnoresCharsInStringLiterals() throws Exception { + // A '?' or ':' inside a string literal is not a bind variable and needs no escaping + assertParsedSqlNoArgs(options, "select 'a?b:c' from dual", "select 'a?b:c' from dual"); + // A doubled quote inside the literal does not prematurely end it + assertParsedSqlNoArgs(options, "select 'it''s a ? and :x' from dual", "select 'it''s a ? and :x' from dual"); + // An unterminated literal is copied through verbatim (no parameters found) + assertParsedSqlNoArgs(options, "select 'a?b:c from dual", "select 'a?b:c from dual"); + } + + @Test + public void smartParsingPreservesPostgresCast() throws Exception { + // The PostgreSQL cast operator (::type) is left untouched, not collapsed or treated as a parameter + assertParsedSqlNoArgs(options, "select a::text from b", "select a::text from b"); + } + + @Test + public void smartParsingLeavesLoneColonAlone() throws Exception { + // A ':' that is not followed by an identifier character is not a named parameter + assertParsedSqlNoArgs(options, "select a : b from dual", "select a : b from dual"); + // ...including a ':' at the very end of the SQL (boundary condition) + assertParsedSqlNoArgs(options, "select a from dual :", "select a from dual :"); + } + + @Test + public void smartParsingDoesNotTreatOperatorsAsComments() throws Exception { + // A single '-' (subtraction) or '/' (division) is not the start of a comment + assertParsedSqlNoArgs(options, "select a-b, c/d from e", "select a-b, c/d from e"); + } + + @Test + public void smartParsingIgnoresCharsInQuotedIdentifier() throws Exception { + // A '?' or ':' inside a double-quoted identifier is not a bind variable + assertParsedSqlNoArgs(options, "select x as \"a:b?c\" from b", "select x as \"a:b?c\" from b"); + // A doubled double-quote inside the identifier does not prematurely end it + assertParsedSqlNoArgs(options, "select x as \"a\"\"b?:c\" from b", "select x as \"a\"\"b?:c\" from b"); + } + + @Test + public void smartParsingIgnoresCharsInComments() throws Exception { + // A '?' or ':' inside a line comment or block comment is not a bind variable + assertParsedSqlNoArgs(options, "select a -- comment with ? and :x\nfrom b", + "select a -- comment with ? and :x\nfrom b"); + // A line comment that runs to the end of the SQL (no trailing newline) + assertParsedSqlNoArgs(options, "select a from b -- trailing ? and :x", + "select a from b -- trailing ? and :x"); + assertParsedSqlNoArgs(options, "select a /* ? and :x */ from b", "select a /* ? and :x */ from b"); + // Nested block comments (as supported by PostgreSQL) are handled + assertParsedSqlNoArgs(options, "select a /* outer ? /* inner :x */ still ? */ from b", + "select a /* outer ? /* inner :x */ still ? */ from b"); + // An unterminated block comment is copied through verbatim + assertParsedSqlNoArgs(options, "select a /* ? and :x from b", "select a /* ? and :x from b"); + } + + @Test + public void smartParsingFindsParameterAfterComment() throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + // Parsing resumes after a comment, so a real parameter that follows it is still found + expect(c.prepareStatement("select a -- pick one: ? or :x\nfrom b where c=?")).andReturn(ps); + ps.setObject(eq(1), eq(Long.valueOf(1))); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, options) + .toSelect("select a -- pick one: ? or :x\nfrom b where c=:id") + .argLong("id", 1L).queryLongOrNull()); + + control.verify(); + } + + @Test + public void smartParsingMixesLiteralsAndRealParameters() throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + // The '?' inside the literal and the '::text' cast are preserved; only the real + // positional (?) and named (:x) parameters become bind placeholders + expect(c.prepareStatement("select 'a?b' as r, c::text from b where d=? and e=?")).andReturn(ps); + ps.setObject(eq(1), eq(Long.valueOf(1))); + ps.setObject(eq(2), eq(Long.valueOf(2))); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, options) + .toSelect("select 'a?b' as r, c::text from b where d=? and e=:x") + .argLong(1L).argLong("x", 2L).queryLongOrNull()); + + control.verify(); + } + + @Test + public void smartParsingIgnoresCharsInDollarQuotedStrings() throws Exception { + // Plain $$ dollar quoting: '?' and ':' inside are not bind variables + assertParsedSqlNoArgs(options, "select $$?:missing$$ from dual", "select $$?:missing$$ from dual"); + // Tagged dollar quoting: $tag$...$tag$ + assertParsedSqlNoArgs(options, "select $body$? and :x$body$ from dual", "select $body$? and :x$body$ from dual"); + // Content after the dollar-quoted string is still parsed normally + assertParsedSqlNoArgs(options, "select $$?$$ from dual", "select $$?$$ from dual"); + // Unterminated dollar-quoted string is copied through verbatim + assertParsedSqlNoArgs(options, "select $$?:missing from dual", "select $$?:missing from dual"); + } + + @Test + public void smartParsingFindsParameterAfterDollarQuotedString() throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + // A real parameter after a dollar-quoted string is still found + expect(c.prepareStatement("select $$?$$ from b where c=?")).andReturn(ps); + ps.setObject(eq(1), eq(Long.valueOf(42))); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, options) + .toSelect("select $$?$$ from b where c=:id") + .argLong("id", 42L).queryLongOrNull()); + + control.verify(); + } + + @Test + public void smartParsingIgnoresCharsInBracketedIdentifiers() throws Exception { + // SQL Server bracketed identifier: '?' and ':' inside are not bind variables + assertParsedSqlNoArgs(options, "select [?:missing] from t", "select [?:missing] from t"); + // Escaped ]] inside a bracketed identifier does not prematurely close it + assertParsedSqlNoArgs(options, "select [a]]b?:c] from t", "select [a]]b?:c] from t"); + // Unterminated bracketed identifier is copied through verbatim + assertParsedSqlNoArgs(options, "select [?:missing from t", "select [?:missing from t"); + } + + @Test + public void smartParsingFindsParameterAfterBracketedIdentifier() throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + // A real parameter following a bracketed identifier column reference is still bound + expect(c.prepareStatement("select [col?] from t where id=?")).andReturn(ps); + ps.setObject(eq(1), eq(Long.valueOf(7))); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, options) + .toSelect("select [col?] from t where id=:id") + .argLong("id", 7L).queryLongOrNull()); + + control.verify(); + } + + @Test + public void legacyParsingCollapsesEscapedCharacters() throws Exception { + // In legacy mode, '??' and '::' are treated as escapes and collapse to a single character + assertParsedSqlNoArgs(optionsLegacyParsing, "select 'a??b::c' from dual", "select 'a?b:c' from dual"); + } + + @Test + public void legacyParsingTreatsCastAsEscape() throws Exception { + // In legacy mode, the PostgreSQL cast '::' is (incorrectly) collapsed to a single ':' + assertParsedSqlNoArgs(optionsLegacyParsing, "select a::text from b", "select a:text from b"); + } + + @Test + public void legacyParsingTreatsCharsInLiteralsAsParameters() throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + // In legacy mode, a '?' inside a string literal IS treated as a positional parameter + expect(c.prepareStatement("select 'a?b' from dual")).andReturn(ps); + ps.setObject(eq(1), eq(Long.valueOf(1))); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, optionsLegacyParsing) + .toSelect("select 'a?b' from dual").argLong(1L).queryLongOrNull()); + + control.verify(); + } + + @Test + public void legacyParsingMixesPositionalAndNamedParameters() throws Exception { + IMocksControl control = createStrictControl(); + + Connection c = control.createMock(Connection.class); + PreparedStatement ps = control.createMock(PreparedStatement.class); + ResultSet rs = control.createMock(ResultSet.class); + + // In legacy mode, real positional (?) and named (:x) parameters still resolve correctly + expect(c.prepareStatement("select a from b where c=? and d=? and e=1")).andReturn(ps); + ps.setObject(eq(1), eq(Long.valueOf(1))); + ps.setObject(eq(2), eq(Long.valueOf(2))); + expect(ps.executeQuery()).andReturn(rs); + expect(rs.next()).andReturn(false); + rs.close(); + ps.close(); + + control.replay(); + + assertNull(new DatabaseImpl(c, optionsLegacyParsing) + .toSelect("select a from b where c=? and d=:x and e=1") + .argLong(1L).argLong("x", 2L).queryLongOrNull()); + + control.verify(); + } + + @Test + public void parsingMissingNamedParameterThrows() { + DatabaseException ex = assertThrows(DatabaseException.class, () -> + new DatabaseImpl(createNiceMock(Connection.class), options) + .toSelect("select a from b where c=:missing").queryLongOrNull()); + assertThat(ex.getCause().getMessage(), containsString("requires parameter ':missing'")); + } + + @Test + public void parsingTooFewPositionalParametersThrows() { + DatabaseException ex = assertThrows(DatabaseException.class, () -> + new DatabaseImpl(createNiceMock(Connection.class), options) + .toSelect("select a from b where c=? and d=?").argLong(1L).queryLongOrNull()); + assertThat(ex.getCause().getMessage(), containsString("Not enough positional parameters")); + } + + @Test + public void parsingTooManyPositionalParametersThrows() { + DatabaseException ex = assertThrows(DatabaseException.class, () -> + new DatabaseImpl(createNiceMock(Connection.class), options) + .toSelect("select a from b where c=?").argLong(1L).argLong(2L).queryLongOrNull()); + assertThat(ex.getCause().getMessage(), containsString("Wrong number of positional parameters")); + } + + @Test + public void parsingUnusedNamedParameterThrows() { + DatabaseException ex = assertThrows(DatabaseException.class, () -> + new DatabaseImpl(createNiceMock(Connection.class), options) + .toSelect("select a from b").argLong("unused", 2L).queryLongOrNull()); + assertThat(ex.getCause().getMessage(), containsString("do not exist in the query")); + } + @Test @Retry public void logFormatNoDebugSql() throws Exception { System.out.println(new DatabaseImpl(createNiceMock(DatabaseMock.class), options) @@ -982,28 +1286,28 @@ public void escapedParametersInLoggingShouldNotCauseWrongArgsMessage() { control.replay(); - // Test with escaped question marks (??) + // With smart parsing (the default) a '?' inside a string literal is not a parameter new DatabaseImpl(mock, optionsFullLog) - .toSelect("select 'test??value' as result, a from b where c=?") + .toSelect("select 'test?value' as result, a from b where c=?") .argString("hi") .queryFirstOrNull(r -> r.getStringOrNull("result")); - // Test with escaped colons (::) + // ...and neither is a ':' inside a string literal new DatabaseImpl(mock, optionsFullLog) - .toSelect("select 'test::value' as result, a from b where c=?") + .toSelect("select 'test:value' as result, a from b where c=?") .argString("hi") .queryFirstOrNull(r -> r.getStringOrNull("result")); - // Test with both types of escaped parameters + // Both kinds of character together inside a literal, alongside real bind variables new DatabaseImpl(mock, optionsFullLog) - .toSelect("select 'test??value::end' as result, a from b where c=? and d=:param") + .toSelect("select 'test?value:end' as result, a from b where c=? and d=:param") .argString("hi") .argString("param", "test") .queryFirstOrNull(r -> r.getStringOrNull("result")); - // Test with both types of escaped parameters + // Multiple literals in the same statement, none of them treated as parameters new DatabaseImpl(mock, optionsFullLog) - .toSelect("select 'a??b::c' as result, a from b where c=? and d=:param union select 'd??e::f'") + .toSelect("select 'a?b:c' as result, a from b where c=? and d=:param union select 'd?e:f'") .argString("hi") .argString("param", "test") .queryFirstOrNull(r -> r.getStringOrNull("result"));