From 3993f5c50bc308c202076db4a1979e9e22dcf4fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20=C3=87engel?= Date: Fri, 18 Sep 2026 21:00:26 +0300 Subject: [PATCH] feat: replace parsed SQL parameters and reject unbindable queries --- queries.sql | 2 +- .../dev/sqlcj/analysis/QueryAnalyzer.java | 216 ++++++++-- .../java/dev/sqlcj/analysis/QueryModel.java | 7 +- .../sqlcj/compiler/DefaultSourceLoader.java | 9 +- src/main/java/dev/sqlcj/compiler/Source.java | 7 + .../dev/sqlcj/compiler/SqlcjCompiler.java | 91 +++- .../dev/sqlcj/parser/DefaultQueryParser.java | 19 +- src/main/java/dev/sqlcj/parser/Query.java | 13 +- src/main/java/dev/sqlcj/sql/ParsedSql.java | 16 + .../dev/sqlcj/sql/SqlParameterCompiler.java | 126 ++++++ .../java/dev/sqlcj/sql/SqlParameters.java | 26 ++ .../java/dev/sqlcj/sql/SqlParseException.java | 4 + src/main/java/dev/sqlcj/sql/SqlParser.java | 31 +- .../dev/sqlcj/analysis/QueryAnalyzerTest.java | 398 +++++++++++++++--- .../cli/command/GenerateCommandTest.java | 47 +++ .../compiler/DefaultSourceLoaderTest.java | 37 ++ .../SqlcjCompilerIntegrationTest.java | 266 +++++++++++- .../generator/JavaCodeGeneratorTest.java | 38 ++ .../sqlcj/parser/DefaultQueryParserTest.java | 21 + .../sqlcj/sql/SqlParameterCompilerTest.java | 128 ++++++ .../java/dev/sqlcj/sql/SqlParserTest.java | 153 +++++++ 21 files changed, 1506 insertions(+), 149 deletions(-) create mode 100644 src/main/java/dev/sqlcj/sql/ParsedSql.java create mode 100644 src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java create mode 100644 src/main/java/dev/sqlcj/sql/SqlParameters.java create mode 100644 src/test/java/dev/sqlcj/sql/SqlParameterCompilerTest.java create mode 100644 src/test/java/dev/sqlcj/sql/SqlParserTest.java diff --git a/queries.sql b/queries.sql index 9a9962b..ad37a24 100644 --- a/queries.sql +++ b/queries.sql @@ -17,7 +17,7 @@ WHERE id = $1 -- name: GetUserById :one SELECT id, name FROM users -WHERE id = ?; +WHERE id = $1; -- name: FindUsers :many SELECT id, name diff --git a/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java b/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java index a974a1c..535e557 100644 --- a/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java +++ b/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java @@ -3,7 +3,11 @@ import dev.sqlcj.parser.Query; import dev.sqlcj.parser.QueryType; import dev.sqlcj.schema.Schema; +import dev.sqlcj.sql.ParsedSql; +import dev.sqlcj.type.DefaultTypeResolver; +import dev.sqlcj.type.TypeResolver; import net.sf.jsqlparser.expression.Expression; +import net.sf.jsqlparser.expression.JdbcNamedParameter; import net.sf.jsqlparser.expression.JdbcParameter; import net.sf.jsqlparser.expression.operators.conditional.AndExpression; import net.sf.jsqlparser.expression.operators.conditional.OrExpression; @@ -13,6 +17,7 @@ import net.sf.jsqlparser.expression.operators.relational.InExpression; import net.sf.jsqlparser.expression.operators.relational.ParenthesedExpressionList; import net.sf.jsqlparser.schema.Table; +import net.sf.jsqlparser.statement.ReturningClause; import net.sf.jsqlparser.statement.Statement; import net.sf.jsqlparser.statement.delete.Delete; import net.sf.jsqlparser.statement.insert.Insert; @@ -29,14 +34,22 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Comparator; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.Optional; -import java.util.regex.Pattern; import java.util.stream.Collectors; public final class QueryAnalyzer { - private static final Pattern PLACEHOLDER = Pattern.compile("\\$\\d+"); + private static final String ANONYMOUS_PARAMETER_REJECTION = """ + Anonymous '?' parameters are not supported; use an indexed placeholder such as $1"""; + + /** + * Resolves the Java type of parameter occurrence, which decides whether a + * repeated placeholder index can share one generated parameter. + */ + private final TypeResolver typeResolver = new DefaultTypeResolver(); /** * One query source and the name it exposes to column references, which is @@ -49,21 +62,25 @@ private record Source(String name, dev.sqlcj.schema.Table table) { private record ResolvedColumn(Source source, dev.sqlcj.schema.Column column) { } - public QueryModel analyze(Query query, Statement statement, Schema schema) { + public QueryModel analyze(Query query, ParsedSql parsedSql, Schema schema) { + requireIndexedPlaceholders(parsedSql); + + Statement statement = parsedSql.statement(); + if (statement instanceof Select select) { - return analyzeSelect(query, select, schema); + return analyzeSelect(query, parsedSql, select, schema); } if (statement instanceof Insert insert) { - return analyzeInsert(query, insert, schema); + return analyzeInsert(query, parsedSql, insert, schema); } if (statement instanceof Update update) { - return analyzeUpdate(query, update, schema); + return analyzeUpdate(query, parsedSql, update, schema); } if (statement instanceof Delete delete) { - return analyzeDelete(query, delete, schema); + return analyzeDelete(query, parsedSql, delete, schema); } throw new UnsupportedOperationException( @@ -71,7 +88,9 @@ public QueryModel analyze(Query query, Statement statement, Schema schema) { ); } - private QueryModel analyzeSelect(Query query, Select select, Schema schema) { + private QueryModel analyzeSelect(Query query, ParsedSql parsedSql, Select select, Schema schema) { + requireResultQueryType(query); + PlainSelect plainSelect = select.getPlainSelect(); Table table = getTable(plainSelect); @@ -83,28 +102,30 @@ private QueryModel analyzeSelect(Query query, Select select, Schema schema) { return toQueryModel( query, + parsedSql, table.getUnquotedName(), columns, bindingParameters ); } - private QueryModel analyzeInsert(Query query, Insert insert, Schema schema) { - requireExecQueryType(query); + private QueryModel analyzeInsert(Query query, ParsedSql parsedSql, Insert insert, Schema schema) { + requireExecWrite(query, insert.getReturningClause()); Table table = insert.getTable(); dev.sqlcj.schema.Table schemaTable = findTable(schema, table.getUnquotedName()); return toQueryModel( query, + parsedSql, table.getUnquotedName(), List.of(), resolveInsertParameters(insert, schemaTable) ); } - private QueryModel analyzeUpdate(Query query, Update update, Schema schema) { - requireExecQueryType(query); + private QueryModel analyzeUpdate(Query query, ParsedSql parsedSql, Update update, Schema schema) { + requireExecWrite(query, update.getReturningClause()); Table table = update.getTable(); Source source = toSource(table, schema); @@ -117,14 +138,15 @@ private QueryModel analyzeUpdate(Query query, Update update, Schema schema) { return toQueryModel( query, + parsedSql, table.getUnquotedName(), List.of(), bindingParameters ); } - private QueryModel analyzeDelete(Query query, Delete delete, Schema schema) { - requireExecQueryType(query); + private QueryModel analyzeDelete(Query query, ParsedSql parsedSql, Delete delete, Schema schema) { + requireExecWrite(query, delete.getReturningClause()); Table table = delete.getTable(); Source source = toSource(table, schema); @@ -137,18 +159,33 @@ private QueryModel analyzeDelete(Query query, Delete delete, Schema schema) { return toQueryModel( query, + parsedSql, table.getUnquotedName(), List.of(), bindingParameters ); } - private void requireExecQueryType(Query query) { + private void requireResultQueryType(Query query) { + if (query.type() != QueryType.ONE && query.type() != QueryType.MANY) { + throw new UnsupportedOperationException( + "SELECT queries must be declared as :one or :many" + ); + } + } + + private void requireExecWrite(Query query, ReturningClause returningClause) { if (query.type() != QueryType.EXEC) { throw new UnsupportedOperationException( "Write queries must be declared as :exec" ); } + + if (returningClause != null) { + throw new UnsupportedOperationException( + "RETURNING is not supported" + ); + } } /** @@ -232,37 +269,108 @@ private List resolveUpdateSetParameters(Update update, dev.sqlcj } /** - * Builds the analyzed model from parameters collected in textual order, - * keeping that order for JDBC binding and exposing the parameters in - * logical placeholder-index order. + * Builds the analyzed model from the parameter occurrences collected in + * textual order, keeping that order for JDBC binding and exposing one + * logical parameter per placeholder index. */ private QueryModel toQueryModel( Query query, + ParsedSql parsedSql, String tableName, List columns, - List bindingParameters + List occurrences ) { - List bindingParameterIndexes = bindingParameters.stream() - .map(QueryParameter::index) - .toList(); - - List parameters = bindingParameters.stream() - .sorted(Comparator.comparingInt(QueryParameter::index)) - .toList(); + List bindingParameterIndexes = requireAccountedOccurrences(parsedSql, occurrences); return new QueryModel( query.name(), query.type(), tableName, - toExecutableSql(query.sql()), + parsedSql.parameters().executableSql(), bindingParameterIndexes, columns, - parameters + toParameters(occurrences) ); } - private String toExecutableSql(String sql) { - return PLACEHOLDER.matcher(sql).replaceAll("?"); + /** + * Requires that the occurrences resolved against the schema are exactly the + * placeholder tokens the SQL parser reported, in the same textual order, so + * that every executable {@code ?} position has one typed binding source. + */ + private List requireAccountedOccurrences(ParsedSql parsedSql, List occurrences) { + List analyzed = occurrences.stream() + .map(QueryParameter::index) + .toList(); + + List placeholders = parsedSql.parameters().indexes(); + + if (!analyzed.equals(placeholders)) { + throw new UnsupportedOperationException( + "SQL placeholders %s are not the analyzed parameters %s; a placeholder is in an unsupported location" + .formatted(placeholders, analyzed) + ); + } + + return analyzed; + } + + /** + * Retains one parameter per placeholder index in logical index order. A + * repeated index keeps the name and type of its first occurrence and is + * accepted only when every occurrence resolves to the same Java type. + */ + private List toParameters(List occurrences) { + Map parametersByIndex = new LinkedHashMap<>(); + + for (QueryParameter occurrence : occurrences) { + QueryParameter parameter = parametersByIndex.putIfAbsent(occurrence.index(), occurrence); + + if (parameter != null) { + requireSameParameterType(parameter, occurrence); + } + } + + List parameters = parametersByIndex.values().stream() + .sorted(Comparator.comparingInt(QueryParameter::index)) + .toList(); + + requireContiguousIndexes(parameters); + + return parameters; + } + + private void requireSameParameterType(QueryParameter parameter, QueryParameter occurrence) { + String type = typeResolver.resolve(parameter.type()); + String occurrenceType = typeResolver.resolve(occurrence.type()); + + if (!type.equals(occurrenceType)) { + throw new UnsupportedOperationException( + "Placeholder $%d has conflicting types: %s from '%s' and %s from '%s'" + .formatted( + parameter.index(), + type, + parameter.name(), + occurrenceType, + occurrence.name() + ) + ); + } + } + + private void requireContiguousIndexes(List parameters) { + for (int index = 0; index < parameters.size(); index++) { + if (parameters.get(index).index() != index + 1) { + throw new UnsupportedOperationException( + "Placeholder indexes must start at $1 without gaps, but were %s" + .formatted( + parameters.stream() + .map(QueryParameter::index) + .toList() + ) + ); + } + } } private Table getTable(PlainSelect plainSelect) { @@ -470,6 +578,8 @@ private void resolveInExpression(InExpression in, List sources, List expressionList) { for (Expression expression : expressionList) { + requireIndexedParameter(expression); + if (expression instanceof JdbcParameter parameter) { addParameter(parameter, schemaColumn, parameters); } @@ -492,6 +602,8 @@ private void resolveInExpression( List sources, List parameters ) { + requireIndexedParameter(expression); + if (expression instanceof JdbcParameter parameter) { addParameter(parameter, schemaColumn, parameters); return; @@ -535,6 +647,8 @@ private void resolveInExpression( if (expression instanceof ParenthesedExpressionList expressionList) { for (Expression nestedExpression : expressionList) { + requireIndexedParameter(nestedExpression); + if (nestedExpression instanceof JdbcParameter parameter) { addParameter(parameter, schemaColumn, parameters); } else { @@ -554,10 +668,10 @@ private void resolveParameterComparison( List sources, List parameters ) { - if ( - left instanceof net.sf.jsqlparser.schema.Column column - && right instanceof JdbcParameter parameter - ) { + requireIndexedParameter(left); + requireIndexedParameter(right); + + if (left instanceof net.sf.jsqlparser.schema.Column column && right instanceof JdbcParameter parameter) { addParameter( parameter, resolveColumn(column, sources).column(), @@ -566,10 +680,7 @@ private void resolveParameterComparison( return; } - if ( - left instanceof JdbcParameter parameter - && right instanceof net.sf.jsqlparser.schema.Column column - ) { + if (left instanceof JdbcParameter parameter && right instanceof net.sf.jsqlparser.schema.Column column) { addParameter( parameter, resolveColumn(column, sources).column(), @@ -596,6 +707,10 @@ private void addParameter( dev.sqlcj.schema.Column column, List parameters ) { + if (!parameter.isUseFixedIndex()) { + throw new UnsupportedOperationException(ANONYMOUS_PARAMETER_REJECTION); + } + parameters.add( new QueryParameter( parameter.getIndex(), @@ -605,6 +720,31 @@ private void addParameter( ); } + /** + * Rejects an anonymous placeholder reported anywhere in the SQL source, + * including a clause this analyzer does not traverse, so that an accepted + * query never keeps an unbound placeholder in its executable SQL. + */ + private void requireIndexedPlaceholders(ParsedSql parsedSql) { + if (parsedSql.parameters().hasAnonymousParameter()) { + throw new UnsupportedOperationException(ANONYMOUS_PARAMETER_REJECTION); + } + } + + /** + * Rejects a named placeholder where an indexed placeholder is supported, so + * that an accepted query never keeps an unbound placeholder in its + * executable SQL. + */ + private void requireIndexedParameter(Expression expression) { + if (expression instanceof JdbcNamedParameter named) { + throw new UnsupportedOperationException( + "Named parameter ':%s' is not supported; use an indexed placeholder such as $1" + .formatted(named.getName()) + ); + } + } + /** * Resolves the selected columns in declared order, expanding {@code *} * across the query sources in their declared order and diff --git a/src/main/java/dev/sqlcj/analysis/QueryModel.java b/src/main/java/dev/sqlcj/analysis/QueryModel.java index 1ba9fbc..d450a31 100644 --- a/src/main/java/dev/sqlcj/analysis/QueryModel.java +++ b/src/main/java/dev/sqlcj/analysis/QueryModel.java @@ -7,11 +7,12 @@ /** * Analyzed query facts required by code generation. * - * @param executableSql JDBC-executable SQL where supported {@code $N} - * placeholders are replaced by {@code ?} + * @param executableSql JDBC-executable SQL where every parsed {@code $N} + * parameter token is replaced by {@code ?} * @param bindingParameterIndexes placeholder indexes in the textual order of the * {@code ?} positions in {@link #executableSql()} - * @param parameters query parameters in logical placeholder-index order + * @param parameters one query parameter per placeholder index, in + * logical placeholder-index order */ public record QueryModel( String name, diff --git a/src/main/java/dev/sqlcj/compiler/DefaultSourceLoader.java b/src/main/java/dev/sqlcj/compiler/DefaultSourceLoader.java index e85dfff..b084371 100644 --- a/src/main/java/dev/sqlcj/compiler/DefaultSourceLoader.java +++ b/src/main/java/dev/sqlcj/compiler/DefaultSourceLoader.java @@ -41,7 +41,14 @@ public List load(Config config) { } } - sources.add(new Source(schema, queries)); + sources.add( + new Source( + schemaPath, + schema, + queriesPath, + queries + ) + ); } return sources; diff --git a/src/main/java/dev/sqlcj/compiler/Source.java b/src/main/java/dev/sqlcj/compiler/Source.java index 5883fe0..f4d80d6 100644 --- a/src/main/java/dev/sqlcj/compiler/Source.java +++ b/src/main/java/dev/sqlcj/compiler/Source.java @@ -2,10 +2,17 @@ import dev.sqlcj.parser.Query; +import java.nio.file.Path; import java.util.List; +/** + * One loaded configuration entry and the paths it was loaded from, which + * identify a source in a compilation diagnostic. + */ public record Source( + Path schemaPath, String schema, + Path queriesPath, List queries ) { } diff --git a/src/main/java/dev/sqlcj/compiler/SqlcjCompiler.java b/src/main/java/dev/sqlcj/compiler/SqlcjCompiler.java index 6fef50b..3cf4214 100644 --- a/src/main/java/dev/sqlcj/compiler/SqlcjCompiler.java +++ b/src/main/java/dev/sqlcj/compiler/SqlcjCompiler.java @@ -11,12 +11,13 @@ import dev.sqlcj.schema.Schema; import dev.sqlcj.schema.parser.DefaultSchemaParser; import dev.sqlcj.schema.parser.SchemaParser; +import dev.sqlcj.sql.ParsedSql; import dev.sqlcj.sql.SqlParser; -import net.sf.jsqlparser.statement.Statement; import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Path; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -30,49 +31,101 @@ public final class SqlcjCompiler { private final GeneratedFileWriter generatedFileWriter = new GeneratedFileWriter(); private final SchemaParser schemaParser = new DefaultSchemaParser(); + /** + * Compiles every configured entry before any generated file is written, so + * a failure in a later source cannot leave a mixture of previously + * generated and newly generated output. + */ public void compile(Config config) { List sources = sourceLoader.load(config); CodeGenerator codeGenerator = new JavaCodeGenerator(config.java().packageName()); + List files = generate(sources, codeGenerator); + Path outputDirectory = Path.of(config.java().out()); + for (GeneratedFile file : files) { + write(file, outputDirectory); + } + } + + private List generate(List sources, CodeGenerator codeGenerator) { + List files = new ArrayList<>(); Map generatedQueries = new HashMap<>(); for (Source source : sources) { - Schema schema = schemaParser.parse(source.schema()); + Schema schema = parseSchema(source); for (Query query : source.queries()) { - compileQuery( - query, - schema, - codeGenerator, - outputDirectory, - generatedQueries - ); + GeneratedFile file = compileQuery(source, query, schema, codeGenerator); + + checkGeneratedPath(query, file, generatedQueries); + + files.add(file); } } + + return files; + } + + private Schema parseSchema(Source source) { + try { + return schemaParser.parse(source.schema()); + } catch (RuntimeException e) { + throw new CompilationException( + "Invalid schema source %s: %s".formatted(source.schemaPath(), reason(e)), + e + ); + } } - private void compileQuery( + /** + * Compiles one query, reporting a parse, analysis, or generation failure + * with the source, query, and header line it belongs to. + */ + private GeneratedFile compileQuery( + Source source, Query query, Schema schema, - CodeGenerator codeGenerator, - Path outputDirectory, - Map generatedQueries + CodeGenerator codeGenerator ) { - Statement statement = sqlParser.parse(query.sql()); - QueryModel model = queryAnalyzer.analyze(query, statement, schema); - GeneratedFile file = codeGenerator.generate(model); + try { + ParsedSql parsedSql = sqlParser.parse(query.sql()); + QueryModel model = queryAnalyzer.analyze(query, parsedSql, schema); - checkGeneratedPath(query, file, generatedQueries); + return codeGenerator.generate(model); + } catch (RuntimeException e) { + throw new CompilationException( + "Invalid query '%s' in %s at line %d: %s" + .formatted( + query.name(), + source.queriesPath(), + query.line(), + reason(e) + ), + e + ); + } + } + + /** Uses the first message line so a diagnostic stays focused. */ + private String reason(RuntimeException e) { + String message = e.getMessage(); + + if (message == null || message.isBlank()) { + return e.getClass().getSimpleName(); + } - write(file, outputDirectory); + return message.lines() + .findFirst() + .orElse(message) + .trim(); } /** * Rejects a generated path that repeats, or differs only by case from, an - * already generated path before the earlier file can be overwritten. + * already generated path before any file is written. */ private void checkGeneratedPath( Query query, diff --git a/src/main/java/dev/sqlcj/parser/DefaultQueryParser.java b/src/main/java/dev/sqlcj/parser/DefaultQueryParser.java index 535d976..1f2301a 100644 --- a/src/main/java/dev/sqlcj/parser/DefaultQueryParser.java +++ b/src/main/java/dev/sqlcj/parser/DefaultQueryParser.java @@ -15,17 +15,22 @@ public List parse(String source) { String currentName = null; QueryType currentType = null; + int currentLine = 0; + int lineNumber = 0; StringBuilder builder = new StringBuilder(); for (String line : source.lines().toList()) { + lineNumber++; + if (line.startsWith(HEADER_PREFIX)) { if (currentName != null) { - queries.add(buildQuery(builder, currentName, currentType)); + queries.add(buildQuery(builder, currentName, currentType, currentLine)); } QueryHeader header = parseHeader(line); currentName = header.name(); currentType = header.type(); + currentLine = lineNumber; builder.setLength(0); continue; @@ -39,7 +44,7 @@ public List parse(String source) { } if (currentName != null) { - queries.add(buildQuery(builder, currentName, currentType)); + queries.add(buildQuery(builder, currentName, currentType, currentLine)); } Set names = new HashSet<>(); @@ -54,7 +59,12 @@ public List parse(String source) { return queries; } - private Query buildQuery(StringBuilder builder, String currentName, QueryType currentType) { + private Query buildQuery( + StringBuilder builder, + String currentName, + QueryType currentType, + int currentLine + ) { String sql = builder.toString().trim(); if (sql.isBlank()) { @@ -66,7 +76,8 @@ private Query buildQuery(StringBuilder builder, String currentName, QueryType cu return new Query( currentName, currentType, - sql + sql, + currentLine ); } diff --git a/src/main/java/dev/sqlcj/parser/Query.java b/src/main/java/dev/sqlcj/parser/Query.java index a6d754a..b652fd4 100644 --- a/src/main/java/dev/sqlcj/parser/Query.java +++ b/src/main/java/dev/sqlcj/parser/Query.java @@ -1,8 +1,19 @@ package dev.sqlcj.parser; +/** + * One named query. + * + * @param line the one-based line of the query header in its source, or + * {@code 0} when the source line is unknown + */ public record Query( String name, QueryType type, - String sql + String sql, + int line ) { + + public Query(String name, QueryType type, String sql) { + this(name, type, sql, 0); + } } diff --git a/src/main/java/dev/sqlcj/sql/ParsedSql.java b/src/main/java/dev/sqlcj/sql/ParsedSql.java new file mode 100644 index 0000000..c50ada9 --- /dev/null +++ b/src/main/java/dev/sqlcj/sql/ParsedSql.java @@ -0,0 +1,16 @@ +package dev.sqlcj.sql; + +import net.sf.jsqlparser.statement.Statement; + +/** + * Syntax-level result of parsing one query SQL source. + * + * @param statement the parsed statement + * @param parameters the positional parameters compiled from the parsed + * {@code $N} tokens of the same source + */ +public record ParsedSql( + Statement statement, + SqlParameters parameters +) { +} diff --git a/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java b/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java new file mode 100644 index 0000000..ecdecbb --- /dev/null +++ b/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java @@ -0,0 +1,126 @@ +package dev.sqlcj.sql; + +import net.sf.jsqlparser.parser.CCJSqlParserConstants; +import net.sf.jsqlparser.parser.Node; +import net.sf.jsqlparser.parser.SimpleNode; +import net.sf.jsqlparser.parser.Token; + +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Pattern; + +/** + * Compiles the positional {@code $N} parameters of a parsed SQL source into + * JDBC {@code ?} markers. + * + *

Only tokens that the SQL parser itself reported as parameter tokens are + * replaced. Every other character is copied from the source, so placeholder + * text inside string literals, quoted identifiers, comments, and identifiers + * stays byte-for-byte unchanged. + * + *

An anonymous {@code ?} placeholder has no index to bind, so it is + * reported instead of replaced and is rejected by semantic analysis. + */ +final class SqlParameterCompiler { + + /** + * A positional parameter image. The digits are bounded because an index + * outside the {@code int} range is not a parameter this compiler can + * report; such a token stays in the SQL and is rejected later as an + * unaccounted parameter. + */ + private static final Pattern POSITIONAL_PARAMETER = Pattern.compile("\\$\\d{1,9}"); + + /** + * The image of an anonymous placeholder token. The parser reports it as a + * token of its own, so a {@code ?} inside a string literal, a quoted + * identifier, or a comment is not reported here, and the JSON operators + * {@code ?|} and {@code ?&} are reported with their own images. + */ + private static final String ANONYMOUS_PARAMETER = "?"; + + /** + * {@link Token#absoluteBegin} and {@link Token#absoluteEnd} count the first + * source character as position one, so a source offset is one less than the + * reported position. + */ + private static final int SOURCE_OFFSET = 1; + + SqlParameters compile(String sql, Node astRoot) { + if (!(astRoot instanceof SimpleNode node)) { + throw new SqlParseException("SQL parse tree is unavailable."); + } + + Token firstToken = node.jjtGetFirstToken(); + Token lastToken = node.jjtGetLastToken(); + + if (firstToken == null || lastToken == null) { + throw new SqlParseException("SQL parse tree has no tokens."); + } + + StringBuilder executableSql = new StringBuilder(); + List indexes = new ArrayList<>(); + boolean anonymous = false; + int copied = 0; + + for (Token token = firstToken; token != null; token = token.next) { + if (isAnonymousParameter(token)) { + anonymous = true; + } + + if (isPositionalParameter(token)) { + int begin = requireSpan(sql, token, copied); + + executableSql + .append(sql, copied, begin) + .append('?'); + + copied = begin + token.image.length(); + + indexes.add(Integer.parseInt(token.image.substring(1))); + } + + if (token == lastToken) { + break; + } + } + + executableSql.append(sql, copied, sql.length()); + + return new SqlParameters(executableSql.toString(), indexes, anonymous); + } + + private boolean isAnonymousParameter(Token token) { + return ANONYMOUS_PARAMETER.equals(token.image); + } + + private boolean isPositionalParameter(Token token) { + return token.kind == CCJSqlParserConstants.S_PARAMETER + && token.image != null + && POSITIONAL_PARAMETER.matcher(token.image).matches(); + } + + /** + * Returns the source offset of a parameter token after requiring that its + * reported span follows the previous replacement, stays inside the source, + * and holds exactly the token image. + */ + private int requireSpan(String sql, Token token, int copied) { + int begin = token.absoluteBegin - SOURCE_OFFSET; + int end = token.absoluteEnd - SOURCE_OFFSET; + + boolean valid = begin >= copied + && end <= sql.length() + && end - begin == token.image.length() + && sql.startsWith(token.image, begin); + + if (!valid) { + throw new SqlParseException( + "Parameter '%s' reported an unusable source position: [%d, %d)" + .formatted(token.image, begin, end) + ); + } + + return begin; + } +} diff --git a/src/main/java/dev/sqlcj/sql/SqlParameters.java b/src/main/java/dev/sqlcj/sql/SqlParameters.java new file mode 100644 index 0000000..671f5c2 --- /dev/null +++ b/src/main/java/dev/sqlcj/sql/SqlParameters.java @@ -0,0 +1,26 @@ +package dev.sqlcj.sql; + +import java.util.List; + +/** + * Compiled positional parameters of one SQL source. + * + * @param executableSql the source SQL in which every parsed {@code $N} + * parameter token is replaced by a JDBC {@code ?} and + * every other character is preserved + * @param indexes the parameter indexes in the textual order of the + * {@code ?} positions in {@link #executableSql()} + * @param hasAnonymousParameter + * whether the source contains an anonymous {@code ?} + * placeholder token, which has no index to bind + */ +public record SqlParameters( + String executableSql, + List indexes, + boolean hasAnonymousParameter +) { + + public SqlParameters { + indexes = List.copyOf(indexes); + } +} diff --git a/src/main/java/dev/sqlcj/sql/SqlParseException.java b/src/main/java/dev/sqlcj/sql/SqlParseException.java index add079e..2fbf1c6 100644 --- a/src/main/java/dev/sqlcj/sql/SqlParseException.java +++ b/src/main/java/dev/sqlcj/sql/SqlParseException.java @@ -2,6 +2,10 @@ public class SqlParseException extends RuntimeException { + public SqlParseException(String message) { + super(message); + } + public SqlParseException(Throwable cause) { super(cause); } diff --git a/src/main/java/dev/sqlcj/sql/SqlParser.java b/src/main/java/dev/sqlcj/sql/SqlParser.java index 609be72..0cd6648 100644 --- a/src/main/java/dev/sqlcj/sql/SqlParser.java +++ b/src/main/java/dev/sqlcj/sql/SqlParser.java @@ -1,16 +1,43 @@ package dev.sqlcj.sql; import net.sf.jsqlparser.JSQLParserException; +import net.sf.jsqlparser.parser.CCJSqlParser; import net.sf.jsqlparser.parser.CCJSqlParserUtil; import net.sf.jsqlparser.statement.Statement; +import java.util.concurrent.atomic.AtomicReference; + public final class SqlParser { - public Statement parse(String sql) { + private final SqlParameterCompiler parameterCompiler = new SqlParameterCompiler(); + + /** + * Parses one SQL source into its statement and its compiled positional + * parameters. + * + *

The parser used for the returned statement is captured while parsing. + * {@link CCJSqlParserUtil} hands a newly created parser to the consumer + * again when it retries a source with complex parsing, so the last captured + * parser owns the syntax tree of the returned statement. + */ + public ParsedSql parse(String sql) { + AtomicReference parser = new AtomicReference<>(); + + Statement statement; + try { - return CCJSqlParserUtil.parse(sql); + statement = CCJSqlParserUtil.parse(sql, parser::set); } catch (JSQLParserException e) { throw new SqlParseException(e); } + + if (statement == null || parser.get() == null) { + throw new SqlParseException("SQL source contains no statement."); + } + + return new ParsedSql( + statement, + parameterCompiler.compile(sql, parser.get().getASTRoot()) + ); } } diff --git a/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java b/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java index 23dac1d..03b7785 100644 --- a/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java +++ b/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java @@ -6,8 +6,8 @@ import dev.sqlcj.schema.ColumnType; import dev.sqlcj.schema.Schema; import dev.sqlcj.schema.Table; +import dev.sqlcj.sql.ParsedSql; import dev.sqlcj.sql.SqlParser; -import net.sf.jsqlparser.statement.Statement; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -79,8 +79,8 @@ void shouldAnalyzeSelectWithoutParameters() { """ ); - Statement statement = parser.parse(query.sql()); - QueryModel model = analyzer.analyze(query, statement, schema); + ParsedSql parsedSql = parser.parse(query.sql()); + QueryModel model = analyzer.analyze(query, parsedSql, schema); assertEquals("ListUsers", model.name()); assertEquals(QueryType.MANY, model.type()); @@ -100,8 +100,8 @@ void shouldAnalyzeSelectWithSingleParameter() { """ ); - Statement statement = parser.parse(query.sql()); - QueryModel model = analyzer.analyze(query, statement, schema); + ParsedSql parsedSql = parser.parse(query.sql()); + QueryModel model = analyzer.analyze(query, parsedSql, schema); assertEquals("users", model.table()); assertEquals( @@ -125,8 +125,8 @@ void shouldAnalyzeSelectWithMultipleParameters() { """ ); - Statement statement = parser.parse(query.sql()); - QueryModel model = analyzer.analyze(query, statement, schema); + ParsedSql parsedSql = parser.parse(query.sql()); + QueryModel model = analyzer.analyze(query, parsedSql, schema); assertEquals("users", model.table()); assertEquals( @@ -308,11 +308,11 @@ INSERT INTO users (id) """ ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); UnsupportedOperationException exception = assertThrows( UnsupportedOperationException.class, - () -> analyzer.analyze(query, statement, schema) + () -> analyzer.analyze(query, parsedSql, schema) ); assertEquals( @@ -329,11 +329,11 @@ void shouldAnalyzeSelectWithExplicitColumns() { "SELECT id, name FROM users WHERE id = $1" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -358,11 +358,11 @@ void shouldResolveAllColumns() { "SELECT * FROM users" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -386,11 +386,11 @@ void shouldResolveParameterTypeFromColumn() { "SELECT * FROM users WHERE id = $1" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -415,11 +415,11 @@ void shouldResolveMultipleParameters() { """ ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -440,11 +440,11 @@ void shouldNotCreateParametersForLiteralExpressions() { "SELECT * FROM users WHERE 1 = 1" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -459,11 +459,11 @@ void shouldThrowWhenTableDoesNotExist() { "SELECT * FROM orders" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); assertThrows( IllegalArgumentException.class, - () -> analyzer.analyze(query, statement, schema) + () -> analyzer.analyze(query, parsedSql, schema) ); } @@ -475,11 +475,11 @@ void shouldThrowWhenColumnDoesNotExist() { "SELECT username FROM users" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); assertThrows( IllegalArgumentException.class, - () -> analyzer.analyze(query, statement, schema) + () -> analyzer.analyze(query, parsedSql, schema) ); } @@ -491,11 +491,11 @@ void shouldResolveQueryParameterFromReferencedColumn() { "SELECT * FROM users WHERE id = $1" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -516,11 +516,11 @@ void shouldResolveMultipleQueryParameters() { "SELECT * FROM users WHERE id = $1 AND active = $2" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -545,11 +545,11 @@ void shouldResolveMultipleParametersForSameColumn() { "SELECT * FROM users WHERE id = $1 AND id = $2" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -574,11 +574,11 @@ void shouldResolveQueryParametersInIndexOrder() { "SELECT * FROM users WHERE active = $2 AND id = $1" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -603,11 +603,11 @@ void shouldResolveQueryParametersInsideOrExpression() { "SELECT * FROM users WHERE id = $1 OR active = $2" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -637,11 +637,11 @@ void shouldResolveQueryParametersInsideNestedAndOrExpressions() { """ ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -676,11 +676,11 @@ void shouldResolveQueryParametersForComparisonOperators() { """ ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -722,11 +722,11 @@ void shouldResolveQueryParameterForComparisonOperator( .formatted(operator) ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -747,11 +747,11 @@ void shouldResolveQueryParameterWhenParameterIsOnLeftSide() { "SELECT * FROM users WHERE $1 = id" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -772,11 +772,11 @@ void shouldResolveQueryParametersInsideInExpression() { "SELECT * FROM users WHERE id IN ($1, $2, $3)" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -806,11 +806,11 @@ void shouldResolveQueryParametersInsideInExpressionInIndexOrder() { "SELECT * FROM users WHERE id IN ($3, $1, $2)" ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -845,11 +845,11 @@ WHERE id IN ($1, $2) """ ); - Statement statement = parser.parse(query.sql()); + ParsedSql parsedSql = parser.parse(query.sql()); QueryModel model = analyzer.analyze( query, - statement, + parsedSql, schema ); @@ -1074,11 +1074,11 @@ void shouldRejectTableNameHiddenByAlias() { """; Query query = new Query("GetUser", QueryType.ONE, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) ); assertTrue(exception.getMessage().contains("users")); @@ -1255,11 +1255,11 @@ void shouldRejectAmbiguousUnqualifiedColumn() { """; Query query = new Query("ListIds", QueryType.MANY, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) ); assertTrue(exception.getMessage().contains("id")); @@ -1274,11 +1274,11 @@ void shouldRejectUnknownColumnQualifier() { """; Query query = new Query("ListIds", QueryType.MANY, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) ); assertTrue(exception.getMessage().contains("o")); @@ -1293,11 +1293,11 @@ void shouldRejectDuplicateExposedSourceName() { """; Query query = new Query("ListIds", QueryType.MANY, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) ); assertTrue(exception.getMessage().contains("U")); @@ -1322,11 +1322,11 @@ void shouldRejectExcludedJoinReportedAsInnerJoin(String fromClause) { """.formatted(fromClause); Query query = new Query("ListIds", QueryType.MANY, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); assertThrows( UnsupportedOperationException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) ); } @@ -1339,11 +1339,11 @@ void shouldRejectJoinWithoutSingleQualifiedEquality() { """; Query query = new Query("ListIds", QueryType.MANY, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); assertThrows( UnsupportedOperationException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) ); } @@ -1356,11 +1356,287 @@ void shouldRejectJoinConditionWithoutEarlierSource() { """; Query query = new Query("ListIds", QueryType.MANY, sql); - Statement statement = parser.parse(sql); + ParsedSql parsedSql = parser.parse(sql); assertThrows( UnsupportedOperationException.class, - () -> analyzer.analyze(query, statement, joinSchema) + () -> analyzer.analyze(query, parsedSql, joinSchema) + ); + } + + @Test + void shouldRetainOneParameterForRepeatedIndex() { + String sql = """ + SELECT * + FROM users + WHERE name = $2 + AND (id = $1 OR id = $1) + """; + + QueryModel model = analyzer.analyze( + new Query("FindUsers", QueryType.MANY, sql), + parser.parse(sql), + schema + ); + + assertEquals( + List.of( + new QueryParameter(1, "id", ColumnType.BIGINT), + new QueryParameter(2, "name", ColumnType.VARCHAR) + ), + model.parameters() + ); + + assertEquals(List.of(2, 1, 1), model.bindingParameterIndexes()); + + assertEquals( + """ + SELECT * + FROM users + WHERE name = ? + AND (id = ? OR id = ?) + """, + model.executableSql() + ); + } + + @Test + void shouldRetainFirstOccurrenceOfRepeatedIndexWithSameJavaType() { + Schema textSchema = new Schema( + List.of( + new Table( + "users", + List.of( + new Column("name", ColumnType.VARCHAR, true), + new Column("note", ColumnType.TEXT, true) + ), + List.of() + ) + ) + ); + + String sql = "SELECT name FROM users WHERE name = $1 AND note = $1"; + + QueryModel model = analyzer.analyze( + new Query("FindUsers", QueryType.MANY, sql), + parser.parse(sql), + textSchema + ); + + assertEquals( + List.of(new QueryParameter(1, "name", ColumnType.VARCHAR)), + model.parameters() + ); + + assertEquals(List.of(1, 1), model.bindingParameterIndexes()); + } + + @Test + void shouldRejectRepeatedIndexWithConflictingType() { + String sql = "UPDATE users SET name = $1 WHERE id = $1"; + + Query query = new Query("UpdateUser", QueryType.EXEC, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "Placeholder $1 has conflicting types: String from 'name' and Long from 'id'", + exception.getMessage() + ); + } + + @Test + void shouldRejectGappedParameterIndexes() { + String sql = "SELECT * FROM users WHERE id = $1 AND name = $3"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "Placeholder indexes must start at $1 without gaps, but were [1, 3]", + exception.getMessage() + ); + } + + @Test + void shouldRejectZeroParameterIndex() { + String sql = "SELECT * FROM users WHERE id = $0"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "Placeholder indexes must start at $1 without gaps, but were [0]", + exception.getMessage() + ); + } + + @Test + void shouldRejectPlaceholderInUnsupportedLocation() { + String sql = "SELECT * FROM users WHERE id = $1 LIMIT $2"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "SQL placeholders [1, 2] are not the analyzed parameters [1]; " + + "a placeholder is in an unsupported location", + exception.getMessage() + ); + } + + @Test + void shouldRejectAnonymousParameter() { + String sql = "SELECT * FROM users WHERE id = ?"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "Anonymous '?' parameters are not supported; use an indexed placeholder such as $1", + exception.getMessage() + ); + } + + @Test + void shouldRejectAnonymousParameterInUnsupportedLocation() { + String sql = "SELECT id, name FROM users WHERE NOT (id = ?) AND name = $1"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "Anonymous '?' parameters are not supported; use an indexed placeholder such as $1", + exception.getMessage() + ); + } + + @Test + void shouldRejectNamedParameter() { + String sql = "SELECT * FROM users WHERE id = :userId"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "Named parameter ':userId' is not supported; use an indexed placeholder such as $1", + exception.getMessage() + ); + } + + @Test + void shouldRejectNamedParameterInInList() { + String sql = "SELECT * FROM users WHERE id IN ($1, :other)"; + + Query query = new Query("FindUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + } + + @Test + void shouldRejectSelectWithoutResultQueryType() { + String sql = "SELECT id FROM users WHERE id = $1"; + + Query query = new Query("GetUser", QueryType.EXEC, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals( + "SELECT queries must be declared as :one or :many", + exception.getMessage() + ); + } + + @ParameterizedTest + @ValueSource( + strings = { + "INSERT INTO users (id) VALUES ($1) RETURNING id", + "UPDATE users SET name = $2 WHERE id = $1 RETURNING id", + "DELETE FROM users WHERE id = $1 RETURNING id" + } + ) + void shouldRejectReturningWrite(String sql) { + Query query = new Query("WriteUser", QueryType.EXEC, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, schema) + ); + + assertEquals("RETURNING is not supported", exception.getMessage()); + } + + @Test + void shouldKeepPlaceholderTextThatIsNotAParameter() { + String sql = """ + SELECT id, name + FROM users -- keep $9 + WHERE name = '$1 literal' + AND id = $1 + """; + + QueryModel model = analyzer.analyze( + new Query("FindUsers", QueryType.MANY, sql), + parser.parse(sql), + schema + ); + + assertEquals( + """ + SELECT id, name + FROM users -- keep $9 + WHERE name = '$1 literal' + AND id = ? + """, + model.executableSql() + ); + + assertEquals( + List.of(new QueryParameter(1, "id", ColumnType.BIGINT)), + model.parameters() ); } } diff --git a/src/test/java/dev/sqlcj/cli/command/GenerateCommandTest.java b/src/test/java/dev/sqlcj/cli/command/GenerateCommandTest.java index e106af8..5e02775 100644 --- a/src/test/java/dev/sqlcj/cli/command/GenerateCommandTest.java +++ b/src/test/java/dev/sqlcj/cli/command/GenerateCommandTest.java @@ -119,6 +119,53 @@ void shouldFailWithConciseDiagnosticForUnreadableSource() throws Exception { assertFalse(Files.exists(workingDirectory.resolve("generated"))); } + @Test + void shouldFailWithConciseDiagnosticForInvalidQuery() throws Exception { + Files.writeString(workingDirectory.resolve("schema.sql"), SCHEMA); + Files.writeString( + workingDirectory.resolve("queries.sql"), + """ + -- name: ListUsers :many + SELECT id + FROM users; + + -- name: GetUser :one + SELECT id, name + FROM users + WHERE id = $1 + AND name = $3; + """ + ); + Files.writeString( + workingDirectory.resolve("sqlcj.yaml"), + """ + version: "1" + sql: + - schema: schema.sql + queries: queries.sql + java: + package: dev.example.generated + out: generated + """ + ); + + Result result = runGenerate(); + + assertEquals(1, result.exitCode(), result.error()); + + assertTrue( + result.error().contains( + "sqlcj: Invalid query 'GetUser' in %s at line 5: " + .formatted(workingDirectory.resolve("queries.sql")) + + "Placeholder indexes must start at $1 without gaps, but were [1, 3]" + ), + result.error() + ); + + assertFalse(result.error().contains("\tat ")); + assertFalse(Files.exists(workingDirectory.resolve("generated"))); + } + private Result runGenerate() throws IOException, InterruptedException { Path java = Path.of(System.getProperty("java.home"), "bin", "java"); diff --git a/src/test/java/dev/sqlcj/compiler/DefaultSourceLoaderTest.java b/src/test/java/dev/sqlcj/compiler/DefaultSourceLoaderTest.java index 590ec92..bc2d09c 100644 --- a/src/test/java/dev/sqlcj/compiler/DefaultSourceLoaderTest.java +++ b/src/test/java/dev/sqlcj/compiler/DefaultSourceLoaderTest.java @@ -216,6 +216,43 @@ void shouldReportInvalidQuerySourceWithItsPath() throws IOException { ); } + @Test + void shouldCarrySourcePathsAndQueryLines() throws IOException { + Path schema = write("schema.sql", """ + CREATE TABLE users + ( + id BIGINT NOT NULL + ); + """); + + Path queries = write("queries.sql", """ + -- name: GetUser :one + SELECT id + FROM users + WHERE id = $1; + + -- name: ListUsers :many + SELECT id + FROM users; + """); + + List sources = sourceLoader.load( + config( + new SqlConfig( + schema.toString(), + queries.toString() + ) + ) + ); + + Source source = sources.getFirst(); + + assertEquals(schema, source.schemaPath()); + assertEquals(queries, source.queriesPath()); + assertEquals(1, source.queries().getFirst().line()); + assertEquals(6, source.queries().get(1).line()); + } + private List names(Source source) { return source.queries().stream().map(Query::name).toList(); } diff --git a/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java b/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java index a3423ce..d612a6f 100644 --- a/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java +++ b/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java @@ -606,6 +606,109 @@ void shouldExecuteGeneratedQueryWithOutOfOrderPlaceholders() throws Exception { } } + @Test + void shouldExecuteGeneratedQueryWithRepeatedPlaceholder() throws Exception { + Path classesDirectory = generateAndCompile( + """ + -- name: FindUser :one + SELECT id, name, active + FROM users + WHERE name = $2 + AND (id = $1 OR id = $1); + """, + "FindUser" + ); + + String source = Files.readString(tempDir.resolve("generated/generated/FindUser.java")); + + assertTrue( + source.contains( + "public FindUserResult findUser(Long id, String name)" + ) + ); + + assertTrue(source.contains("List.of(name, id, id)")); + assertTrue(source.contains("AND (id = ? OR id = ?)")); + + QueryExecutor executor = new JdbcQueryExecutor(usersDataSource()); + + try (URLClassLoader classLoader = classLoader(classesDirectory)) { + Class generatedClass = Class.forName( + "generated.FindUser", + true, + classLoader + ); + + Object generatedQuery = generatedClass + .getConstructor(QueryExecutor.class) + .newInstance(executor); + + Method method = generatedClass.getMethod( + "findUser", + Long.class, + String.class + ); + + Object result = method.invoke(generatedQuery, 1L, "Alice"); + + assertNotNull(result); + + assertEquals(1L, getRecordComponent(result, "id")); + assertEquals("Alice", getRecordComponent(result, "name")); + + assertNull(method.invoke(generatedQuery, 2L, "Alice")); + } + } + + @Test + void shouldExecuteGeneratedQueryWithProtectedPlaceholderText() throws Exception { + Path classesDirectory = generateAndCompile( + """ + -- name: FindUser :one + -- Keeps $9 in a comment. + SELECT id, name + FROM users + WHERE name <> '$1 literal' /* keeps $8 */ + AND id = $1; + """, + "FindUser" + ); + + String source = Files.readString(tempDir.resolve("generated/generated/FindUser.java")); + + assertTrue(source.contains("-- Keeps $9 in a comment.")); + assertTrue(source.contains("WHERE name <> '$1 literal' /* keeps $8 */")); + assertTrue(source.contains("AND id = ?")); + + assertTrue( + source.contains( + "public FindUserResult findUser(Long id)" + ) + ); + + QueryExecutor executor = new JdbcQueryExecutor(usersDataSource()); + + try (URLClassLoader classLoader = classLoader(classesDirectory)) { + Class generatedClass = Class.forName( + "generated.FindUser", + true, + classLoader + ); + + Object generatedQuery = generatedClass + .getConstructor(QueryExecutor.class) + .newInstance(executor); + + Object result = generatedClass + .getMethod("findUser", Long.class) + .invoke(generatedQuery, 1L); + + assertNotNull(result); + + assertEquals("Alice", getRecordComponent(result, "name")); + } + } + @Test void shouldExecuteGeneratedQueryWithoutParameters() throws Exception { Path classesDirectory = generateAndCompile( @@ -1151,7 +1254,147 @@ void shouldRejectDuplicateQueryNameAcrossEntriesBeforeWriting() throws IOExcepti } @Test - void shouldRejectNormalizedGeneratedPathCollisionBeforeOverwriting() throws IOException { + void shouldReportLateFailureAndLeavePreviousOutputIntact() throws IOException { + Path usersSchema = tempDir.resolve("users-schema.sql"); + Path usersQueries = tempDir.resolve("users-queries.sql"); + Path ordersSchema = tempDir.resolve("orders-schema.sql"); + Path ordersQueries = tempDir.resolve("orders-queries.sql"); + Path generatedDirectory = tempDir.resolve("generated"); + + Files.writeString( + usersSchema, + """ + CREATE TABLE users + ( + id BIGINT NOT NULL, + name VARCHAR(255) + ); + """ + ); + + Files.writeString( + usersQueries, + """ + -- name: GetUser :one + SELECT id + FROM users + WHERE id = $1; + """ + ); + + new SqlcjCompiler().compile( + new Config( + List.of(new SqlConfig(usersSchema.toString(), usersQueries.toString())), + new JavaConfig(generatedDirectory.toString(), "dev.example.generated") + ) + ); + + Path generatedFile = generatedDirectory + .resolve("dev/example/generated") + .resolve("GetUser.java"); + + String previous = Files.readString(generatedFile); + + assertTrue(previous.contains("resultSet.getObject(1, Long.class)")); + + Files.writeString( + usersQueries, + """ + -- name: GetUser :one + SELECT name + FROM users + WHERE id = $1; + """ + ); + + Files.writeString( + ordersSchema, + """ + CREATE TABLE orders + ( + id BIGINT NOT NULL + ); + """ + ); + + Files.writeString( + ordersQueries, + """ + -- name: ListOrders :many + SELECT id + FROM orders; + + -- name: GetOrder :one + SELECT id + FROM orders + WHERE id = $2; + """ + ); + + Config config = new Config( + List.of( + new SqlConfig(usersSchema.toString(), usersQueries.toString()), + new SqlConfig(ordersSchema.toString(), ordersQueries.toString()) + ), + new JavaConfig(generatedDirectory.toString(), "dev.example.generated") + ); + + SqlcjCompiler compiler = new SqlcjCompiler(); + + CompilationException exception = assertThrows( + CompilationException.class, + () -> compiler.compile(config) + ); + + assertEquals( + "Invalid query 'GetOrder' in %s at line 5: " + .formatted(ordersQueries) + + "Placeholder indexes must start at $1 without gaps, but were [2]", + exception.getMessage() + ); + + assertEquals(previous, Files.readString(generatedFile)); + + assertFalse( + Files.exists( + generatedDirectory + .resolve("dev/example/generated") + .resolve("ListOrders.java") + ) + ); + } + + @Test + void shouldRejectAnonymousPlaceholderBeforeWriting() { + Path generatedDirectory = tempDir.resolve("generated"); + + CompilationException exception = assertThrows( + CompilationException.class, + () -> compileUsersQueries( + """ + -- name: ListUsers :many + SELECT id + FROM users + WHERE id = $1 + LIMIT ?; + """, + generatedDirectory + ) + ); + + assertEquals( + "Invalid query 'ListUsers' in %s at line 1: " + .formatted(tempDir.resolve("queries.sql")) + + "Anonymous '?' parameters are not supported; " + + "use an indexed placeholder such as $1", + exception.getMessage() + ); + + assertFalse(Files.exists(generatedDirectory)); + } + + @Test + void shouldRejectNormalizedGeneratedPathCollisionBeforeWriting() throws IOException { Path generatedDirectory = tempDir.resolve("generated"); CompilationException exception = assertThrows( @@ -1178,16 +1421,11 @@ void shouldRejectNormalizedGeneratedPathCollisionBeforeOverwriting() throws IOEx exception.getMessage() ); - String generated = Files.readString( - generatedDirectory.resolve("generated").resolve("Get_User.java") - ); - - assertTrue(generated.contains("resultSet.getObject(1, Long.class)")); - assertFalse(generated.contains("resultSet.getObject(1, String.class)")); + assertFalse(Files.exists(generatedDirectory)); } @Test - void shouldRejectGeneratedPathsThatDifferOnlyByCaseBeforeOverwriting() { + void shouldRejectGeneratedPathsThatDifferOnlyByCaseBeforeWriting() { Path generatedDirectory = tempDir.resolve("generated"); CompilationException exception = assertThrows( @@ -1216,17 +1454,7 @@ void shouldRejectGeneratedPathsThatDifferOnlyByCaseBeforeOverwriting() { exception.getMessage() ); - assertTrue( - Files.exists( - generatedDirectory.resolve("generated").resolve("GetUser.java") - ) - ); - - assertFalse( - Files.exists( - generatedDirectory.resolve("generated").resolve("getuser.java") - ) - ); + assertFalse(Files.exists(generatedDirectory)); } @Test diff --git a/src/test/java/dev/sqlcj/generator/JavaCodeGeneratorTest.java b/src/test/java/dev/sqlcj/generator/JavaCodeGeneratorTest.java index 7d083e6..dafb141 100644 --- a/src/test/java/dev/sqlcj/generator/JavaCodeGeneratorTest.java +++ b/src/test/java/dev/sqlcj/generator/JavaCodeGeneratorTest.java @@ -1253,6 +1253,44 @@ void shouldGenerateLogicalParametersWithTextualBindingOrder() throws IOException assertCompiles(file); } + @Test + void shouldRepeatArgumentForRepeatedPlaceholderIndex() throws IOException { + QueryModel query = new QueryModel( + "FindUser", + QueryType.ONE, + "users", + """ + SELECT id, name + FROM users + WHERE name = ? + AND (id = ? OR id = ?) + """, + List.of(2, 1, 1), + List.of( + new QueryColumn("id", ColumnType.BIGINT, false), + new QueryColumn("name", ColumnType.VARCHAR, true) + ), + List.of( + new QueryParameter(1, "id", ColumnType.BIGINT), + new QueryParameter(2, "name", ColumnType.VARCHAR) + ) + ); + + GeneratedFile file = codeGenerator.generate(query); + + String source = file.content(); + + assertTrue( + source.contains( + "public FindUserResult findUser(Long id, String name)" + ) + ); + + assertTrue(source.contains("List.of(name, id, id)")); + + assertCompiles(file); + } + @Test void shouldGenerateEmptyArgumentListForQueryWithoutParameters() throws IOException { QueryModel query = new QueryModel( diff --git a/src/test/java/dev/sqlcj/parser/DefaultQueryParserTest.java b/src/test/java/dev/sqlcj/parser/DefaultQueryParserTest.java index 53e95c0..a2473d9 100644 --- a/src/test/java/dev/sqlcj/parser/DefaultQueryParserTest.java +++ b/src/test/java/dev/sqlcj/parser/DefaultQueryParserTest.java @@ -188,4 +188,25 @@ void preservesQueryOrder() { assertEquals("Second", queries.get(1).name()); assertEquals("Third", queries.get(2).name()); } + + @Test + void parsesHeaderLineOfEachQuery() { + String source = """ + -- A leading comment. + + -- name: GetUser :one + SELECT * + FROM users + WHERE id = $1; + + -- name: ListUsers :many + SELECT * + FROM users; + """; + + List queries = parser.parse(source); + + assertEquals(3, queries.getFirst().line()); + assertEquals(8, queries.get(1).line()); + } } diff --git a/src/test/java/dev/sqlcj/sql/SqlParameterCompilerTest.java b/src/test/java/dev/sqlcj/sql/SqlParameterCompilerTest.java new file mode 100644 index 0000000..b396d5c --- /dev/null +++ b/src/test/java/dev/sqlcj/sql/SqlParameterCompilerTest.java @@ -0,0 +1,128 @@ +package dev.sqlcj.sql; + +import net.sf.jsqlparser.parser.CCJSqlParserConstants; +import net.sf.jsqlparser.parser.SimpleNode; +import net.sf.jsqlparser.parser.Token; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Exercises the token-span handling of {@link SqlParameterCompiler} with + * fabricated tokens, which is the only way to reach the guards that protect the + * compiler from a span the parser cannot describe. + */ +class SqlParameterCompilerTest { + + private final SqlParameterCompiler compiler = new SqlParameterCompiler(); + + @Test + void shouldReplaceReportedParameterSpans() { + String sql = "SELECT * FROM users WHERE id = $1"; + + SqlParameters parameters = compile( + sql, + token(CCJSqlParserConstants.S_PARAMETER, "$1", 31) + ); + + assertEquals("SELECT * FROM users WHERE id = ?", parameters.executableSql()); + assertEquals(List.of(1), parameters.indexes()); + } + + @Test + void shouldIgnoreTokenThatIsNotAParameter() { + String sql = "SELECT a$1b FROM users"; + + SqlParameters parameters = compile( + sql, + token(CCJSqlParserConstants.S_IDENTIFIER, "a$1b", 7) + ); + + assertEquals(sql, parameters.executableSql()); + assertTrue(parameters.indexes().isEmpty()); + } + + @Test + void shouldIgnoreParameterTokenWithoutIndexDigits() { + String sql = "SELECT * FROM users WHERE id = $"; + + SqlParameters parameters = compile( + sql, + token(CCJSqlParserConstants.S_PARAMETER, "$", 31) + ); + + assertEquals(sql, parameters.executableSql()); + assertTrue(parameters.indexes().isEmpty()); + } + + @Test + void shouldRejectSpanThatDoesNotHoldTheTokenImage() { + String sql = "SELECT * FROM users WHERE id = $1"; + + Token parameter = token(CCJSqlParserConstants.S_PARAMETER, "$1", 30); + + SqlParseException exception = assertThrows( + SqlParseException.class, + () -> compile(sql, parameter) + ); + + assertTrue( + exception.getMessage() + .startsWith("Parameter '$1' reported an unusable source position"), + exception.getMessage() + ); + } + + @Test + void shouldRejectSpanOutsideTheSource() { + String sql = "SELECT * FROM users WHERE id = $1"; + + Token parameter = token(CCJSqlParserConstants.S_PARAMETER, "$1", sql.length()); + + assertThrows( + SqlParseException.class, + () -> compile(sql, parameter) + ); + } + + @Test + void shouldRejectSpansThatAreNotInTextualOrder() { + String sql = "SELECT * FROM users WHERE id = $1 AND id = $2"; + + Token second = token(CCJSqlParserConstants.S_PARAMETER, "$2", 43); + Token first = token(CCJSqlParserConstants.S_PARAMETER, "$1", 31); + + assertThrows( + SqlParseException.class, + () -> compile(sql, second, first) + ); + } + + private SqlParameters compile(String sql, Token... tokens) { + for (int index = 0; index + 1 < tokens.length; index++) { + tokens[index].next = tokens[index + 1]; + } + + SimpleNode node = new SimpleNode(0); + node.jjtSetFirstToken(tokens[0]); + node.jjtSetLastToken(tokens[tokens.length - 1]); + + return compiler.compile(sql, node); + } + + /** + * Builds a token whose reported position follows the parser convention of + * counting the first source character as position one. + */ + private Token token(int kind, String image, int offset) { + Token token = new Token(kind, image); + token.absoluteBegin = offset + 1; + token.absoluteEnd = token.absoluteBegin + image.length(); + + return token; + } +} diff --git a/src/test/java/dev/sqlcj/sql/SqlParserTest.java b/src/test/java/dev/sqlcj/sql/SqlParserTest.java new file mode 100644 index 0000000..3a30de4 --- /dev/null +++ b/src/test/java/dev/sqlcj/sql/SqlParserTest.java @@ -0,0 +1,153 @@ +package dev.sqlcj.sql; + +import net.sf.jsqlparser.statement.select.Select; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SqlParserTest { + + private final SqlParser parser = new SqlParser(); + + @Test + void shouldReplaceOnlyParsedParameterTokens() { + String sql = """ + SELECT id, '$1 literal' AS "c$1" + FROM a$1b -- $9 line comment + WHERE id = $1 /* $8 block comment */ + AND name = $2 + """; + + ParsedSql parsedSql = parser.parse(sql); + + assertInstanceOf(Select.class, parsedSql.statement()); + + assertEquals( + """ + SELECT id, '$1 literal' AS "c$1" + FROM a$1b -- $9 line comment + WHERE id = ? /* $8 block comment */ + AND name = ? + """, + parsedSql.parameters().executableSql() + ); + + assertEquals(List.of(1, 2), parsedSql.parameters().indexes()); + } + + @Test + void shouldPreserveSourceWithoutParameters() { + String sql = """ + SELECT id + FROM users; + """; + + ParsedSql parsedSql = parser.parse(sql); + + assertEquals(sql, parsedSql.parameters().executableSql()); + assertTrue(parsedSql.parameters().indexes().isEmpty()); + } + + @Test + void shouldPreserveCarriageReturns() { + String sql = "UPDATE users\r\nSET name = $2\r\nWHERE id = $1\r\n"; + + ParsedSql parsedSql = parser.parse(sql); + + assertEquals( + "UPDATE users\r\nSET name = ?\r\nWHERE id = ?\r\n", + parsedSql.parameters().executableSql() + ); + + assertEquals(List.of(2, 1), parsedSql.parameters().indexes()); + } + + @Test + void shouldReportRepeatedAndOutOfOrderIndexesInTextualOrder() { + ParsedSql parsedSql = parser.parse( + "SELECT * FROM users WHERE id = $2 AND id = $1 AND id = $2" + ); + + assertEquals( + "SELECT * FROM users WHERE id = ? AND id = ? AND id = ?", + parsedSql.parameters().executableSql() + ); + + assertEquals(List.of(2, 1, 2), parsedSql.parameters().indexes()); + } + + @Test + void shouldCompileParametersOfSupportedWrites() { + ParsedSql insert = parser.parse("INSERT INTO users (id, name) VALUES ($1, $2)"); + + assertEquals( + "INSERT INTO users (id, name) VALUES (?, ?)", + insert.parameters().executableSql() + ); + + ParsedSql delete = parser.parse("DELETE FROM users WHERE id = $1"); + + assertEquals( + "DELETE FROM users WHERE id = ?", + delete.parameters().executableSql() + ); + } + + @Test + void shouldReportAnonymousParameterOutsideAnalyzedExpressions() { + ParsedSql parsedSql = parser.parse("SELECT id FROM users WHERE id = $1 LIMIT ?"); + + assertTrue(parsedSql.parameters().hasAnonymousParameter()); + } + + @Test + void shouldNotReportAnonymousParameterForProtectedText() { + String sql = """ + SELECT id, '? literal' AS "c?" + FROM users -- ? line comment + WHERE id = $1 /* ? block comment */ + """; + + ParsedSql parsedSql = parser.parse(sql); + + assertFalse(parsedSql.parameters().hasAnonymousParameter()); + + assertEquals( + """ + SELECT id, '? literal' AS "c?" + FROM users -- ? line comment + WHERE id = ? /* ? block comment */ + """, + parsedSql.parameters().executableSql() + ); + } + + @Test + void shouldRejectDollarQuotedText() { + assertThrows( + SqlParseException.class, + () -> parser.parse("SELECT $$ $1 dollar quoted $$ FROM users WHERE id = $1") + ); + } + + /** A block comment ends at its first delimiter and does not nest. */ + @Test + void shouldPreserveParameterTextInsideBlockCommentWithNestedDelimiter() { + ParsedSql parsedSql = parser.parse( + "SELECT id FROM users /* outer /* $8 inner */ WHERE id = $1" + ); + + assertEquals( + "SELECT id FROM users /* outer /* $8 inner */ WHERE id = ?", + parsedSql.parameters().executableSql() + ); + + assertEquals(List.of(1), parsedSql.parameters().indexes()); + } +}