diff --git a/docs/queries.md b/docs/queries.md index 5f49de8..477d187 100644 --- a/docs/queries.md +++ b/docs/queries.md @@ -602,7 +602,19 @@ string literal, a quoted identifier, or a comment is not a parameter. such as `:"x"`, and an `&name` placeholder. - A placeholder that neither a clause nor a cast types — for example `ORDER BY $1`, `lower(name) = :name`, or `(:x)::int`, whose placeholder is not - the direct operand of its cast — is rejected rather than left unbound. + the direct operand of its cast — is rejected rather than left unbound, as + `Cannot infer the type of :name in lower(name) = :name; state it with a cast, + such as :name::text`. The diagnostic writes the placeholder as the query + spells it, `$1` or `:name`, and names where the query writes it: the smallest + expression written around the placeholder, on one line, with whatever + separates two tokens written as one space. A concatenated pattern is + therefore named in its whole `LIKE` predicate and an `IN` list in its `IN` + predicate, while a placeholder written inside no expression stands alone in + its clause and is named by the keyword that opens that clause, in upper case: + `SELECT`, `DISTINCT ON`, `ON`, `WHERE`, `GROUP BY`, `HAVING`, `ORDER BY`, + `LIMIT`, `OFFSET`, `FETCH`, `VALUES`, `SET`, or `RETURNING`. One query reports + its first untyped placeholder in textual order, after the rules of its clauses + have run, so a clause's own rejection comes first. ### Logical order versus textual order @@ -882,7 +894,12 @@ name, and its header line. `LIMIT $1 + 1` or `OFFSET $1 + 1`, a `LIMIT a, b` row count such as `LIMIT 5, $1`, a placeholder inside a write value such as `COALESCE($2, bio)`, and a `FETCH FIRST $1 ROWS ONLY` clause, so dynamic `IN` - expansion is unavailable. A cast states the type of such a placeholder in + expansion is unavailable. Each is rejected as `Cannot infer the type of $1 in + $1 + 1; state it with a cast, such as $1::text`, which names the placeholder + as the query spells it and, after `in`, the smallest expression the query + writes around it, on one line, or the upper-case keyword of the clause the + placeholder stands alone in, such as `ORDER BY` for `ORDER BY $1` and + `SELECT` for `SELECT $1 AS x`. A cast states the type of such a placeholder in every one of those locations, and it does not accept a placeholder that is not the direct operand of its cast, such as `(:x)::int`. - A cast type sqlcj does not map, such as `$1::interval` or a @@ -1220,7 +1237,7 @@ Writes and `RETURNING`: and `QueryAnalyzerTest.shouldRejectAnUnknownConflictColumn` cover the accepted conflict target, both actions, the `EXCLUDED`, placeholder, cast, and computed assignments, the binding order after the `VALUES` placeholders, the - returning row type, and the conflict forms, unaccounted placeholder, and + returning row type, and the conflict forms, untyped placeholder, and unknown target, assigned, and `EXCLUDED` columns that are rejected on an `:exec` and on a returning insert alike, and `PostgresIntegrationTest.shouldExecuteGeneratedUpsertsAgainstPostgres` @@ -1268,6 +1285,15 @@ Parameters: `QueryAnalyzerTest.shouldRejectAnonymousParameter`, and `QueryAnalyzerTest.shouldKeepPlaceholderTextThatIsNotAParameter` cover ordering, repetition, and rejection. +- `QueryAnalyzerTest.shouldRejectAnUntypedPlaceholder`, + `QueryAnalyzerTest.shouldRejectAnUntypedPlaceholderOfAConcatenatedPattern`, + `QueryAnalyzerTest.shouldRejectTheFirstUntypedPlaceholderOfAQuery`, + `SqlParserTest.shouldReportTheEnclosingExpressionOfAPlaceholder`, + `SqlParserTest.shouldReportTheClauseOfAPlaceholderThatStandsAlone`, and + `SqlcjCompilerIntegrationTest.shouldRejectUntypedPlaceholderBeforeWriting` + cover the untyped-placeholder diagnostic, in both placeholder forms, with the + enclosing expression and the clause keyword it names, the first occurrence it + reports, and the whole located line of a compiled source. - `SqlParserTest.shouldCompileNamedParametersByFirstOccurrence`, `SqlParserTest.shouldCompileNamesThatDifferInCaseAsDistinctParameters`, `SqlParserTest.shouldReportBothPlaceholderFormsOfOneSource`, diff --git a/sqlcj-cli/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java b/sqlcj-cli/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java index 2b50da3..3629108 100644 --- a/sqlcj-cli/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java +++ b/sqlcj-cli/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java @@ -81,6 +81,15 @@ public final class QueryAnalyzer { private static final String MIXED_PLACEHOLDER_REJECTION = """ Indexed '$N' and named ':name' placeholders must not be mixed in one query"""; + /** + * The rejection of a placeholder that no clause of the query types and no + * cast states the type of, which names the placeholder as the query spells + * it and where the query writes it. The suggested cast is always + * {@code ::text}, because sqlcj infers no type for such a placeholder. + */ + private static final String UNTYPED_PLACEHOLDER_REJECTION = """ + Cannot infer the type of %s in %s; state it with a cast, such as %s::text"""; + /** The character that opens a supported named placeholder. */ private static final String NAME_PREFIX = ":"; @@ -337,7 +346,7 @@ private QueryModel analyzeSelect(Query query, ParsedSql parsedSql, Select select List sources = resolveSources(plainSelect, table, schema); - List columns = resolveColumns(plainSelect, sources, schema); + List columns = resolveColumns(plainSelect, parsedSql, sources, schema); Placeholders placeholders = toPlaceholders(parsedSql, schema); @@ -864,8 +873,9 @@ private QueryModel toQueryModel( *

An occurrence a clause of the query typed is that clause's parameter. * Every other occurrence is typed by the cast it is the direct operand of, * wherever in the statement it stands, and is named after its own - * placeholder. A placeholder neither a clause nor a cast types contributes - * no parameter, so the accounting reports it as unaccounted. + * placeholder. A placeholder neither a clause nor a cast types has no type + * to generate, so the first such occurrence in textual order is rejected + * naming where the query writes it. */ private List resolveOccurrences(ParsedSql parsedSql, Placeholders placeholders) { List occurrences = new ArrayList<>(); @@ -881,11 +891,13 @@ private List resolveOccurrences(ParsedSql parsedSql, Placeholder CastPlaceholder cast = placeholders.ofCast(occurrence.cast()); - if (cast != null) { - occurrences.add( - toCastParameter(cast, indexedParameterName(cast.placeholder())) - ); + if (cast == null) { + throw untypedPlaceholder(occurrence); } + + occurrences.add( + toCastParameter(cast, indexedParameterName(cast.placeholder())) + ); } placeholders.requireReportedOccurrences(); @@ -893,6 +905,47 @@ private List resolveOccurrences(ParsedSql parsedSql, Placeholder return occurrences; } + /** + * The rejection of one placeholder occurrence no clause of the query typed + * and no cast states the type of, which the SQL parser reported together + * with where the query writes it. + */ + private UnsupportedOperationException untypedPlaceholder(PlaceholderOccurrence occurrence) { + String spelled = spelledPlaceholder(occurrence.placeholder()); + + return new UnsupportedOperationException( + UNTYPED_PLACEHOLDER_REJECTION.formatted(spelled, occurrence.enclosing(), spelled) + ); + } + + /** + * One placeholder expression as the query spells it, which is its own name + * for a named placeholder and its index for an indexed one. Every other + * placeholder form is already rejected before a placeholder is spelled. + */ + private String spelledPlaceholder(Expression placeholder) { + if (placeholder instanceof JdbcNamedParameter named) { + return NAME_PREFIX + named.getName(); + } + + return "$" + ((JdbcParameter) placeholder).getIndex(); + } + + /** + * The occurrence the SQL parser reported for one placeholder expression of + * the parsed source, which every placeholder of that source has. + */ + private PlaceholderOccurrence occurrenceOf(ParsedSql parsedSql, Expression placeholder) { + return parsedSql.placeholders().stream() + .filter(occurrence -> occurrence.placeholder() == placeholder) + .findFirst() + .orElseThrow( + () -> new IllegalStateException( + "A placeholder is not a reported placeholder occurrence." + ) + ); + } + /** * Requires that the occurrences resolved against the schema are exactly the * placeholder tokens the SQL parser reported, in the same textual order, so @@ -2048,9 +2101,14 @@ private Placeholders toPlaceholders(ParsedSql parsedSql, Schema schema) { * *

An aliased scalar count and an aliased cast each resolve to one column * in their own position, so either may stand beside any other item. + * + *

A bare placeholder projection is a placeholder a cast makes a + * supported result column, so it is rejected as an untyped placeholder + * rather than as an unsupported result expression. */ private List resolveColumns( PlainSelect plainSelect, + ParsedSql parsedSql, List sources, Schema schema ) { @@ -2109,6 +2167,10 @@ private List resolveColumns( continue; } + if (expression instanceof JdbcParameter || expression instanceof JdbcNamedParameter) { + throw untypedPlaceholder(occurrenceOf(parsedSql, expression)); + } + throw new UnsupportedOperationException( "Unsupported SELECT expression: " + expression.getClass().getSimpleName() diff --git a/sqlcj-cli/src/main/java/dev/sqlcj/sql/PlaceholderOccurrence.java b/sqlcj-cli/src/main/java/dev/sqlcj/sql/PlaceholderOccurrence.java index bcfc47c..7bc9229 100644 --- a/sqlcj-cli/src/main/java/dev/sqlcj/sql/PlaceholderOccurrence.java +++ b/sqlcj-cli/src/main/java/dev/sqlcj/sql/PlaceholderOccurrence.java @@ -16,9 +16,14 @@ * the innermost cast of a chain such as * {@code :x::text::int}, and {@code null} when the * placeholder is not cast directly + * @param enclosing where the placeholder stands, written as the query writes + * it: the smallest enclosing expression the parse tree + * reports, on one line, or the keyword of the clause the + * placeholder stands alone in, in upper case */ public record PlaceholderOccurrence( Expression placeholder, - CastExpression cast + CastExpression cast, + String enclosing ) { } diff --git a/sqlcj-cli/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java b/sqlcj-cli/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java index a9ae3f7..5223007 100644 --- a/sqlcj-cli/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java +++ b/sqlcj-cli/src/main/java/dev/sqlcj/sql/SqlParameterCompiler.java @@ -4,12 +4,15 @@ import net.sf.jsqlparser.expression.Expression; import net.sf.jsqlparser.expression.JdbcNamedParameter; import net.sf.jsqlparser.expression.JdbcParameter; +import net.sf.jsqlparser.expression.operators.relational.ExpressionList; import net.sf.jsqlparser.parser.CCJSqlParserConstants; import net.sf.jsqlparser.parser.Node; import net.sf.jsqlparser.parser.Token; +import net.sf.jsqlparser.statement.select.Select; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.regex.Pattern; /** @@ -29,8 +32,9 @@ * anonymous {@code ?} placeholder, which has no parameter to bind. * *

The same walk reports the placeholder occurrences of the source in - * textual order, each with the cast that casts it, which is the syntax-level - * order in which semantic analysis binds them. + * textual order, each with the cast that casts it and with where it stands as + * the source writes it, which is the syntax-level order in which semantic + * analysis binds them. */ final class SqlParameterCompiler { @@ -64,6 +68,28 @@ final class SqlParameterCompiler { */ private static final String ANONYMOUS_PARAMETER = "?"; + /** + * The clause keywords whose own image names the clause a placeholder stands + * alone in, compared ignoring case and written in upper case. + */ + private static final List CLAUSE_KEYWORDS = List.of( + "WHERE", + "HAVING", + "LIMIT", + "OFFSET", + "FETCH", + "SELECT", + "VALUES", + "SET", + "RETURNING" + ); + + /** The image that opens a parenthesized group. */ + private static final String OPEN_PARENTHESIS = "("; + + /** The image that closes a parenthesized group. */ + private static final String CLOSE_PARENTHESIS = ")"; + /** * {@link Token#absoluteBegin} and {@link Token#absoluteEnd} count the first * source character as position one, so a source offset is one less than the @@ -187,11 +213,11 @@ private void collectPlaceholders( Expression operand = innermost.getLeftExpression(); collectUncompiledPlaceholder(operand, names, uncompiled); - collectPlaceholder(operand, innermost, placeholders); + collectPlaceholder(operand, innermost, node, placeholders); } if (value instanceof Expression expression) { - collectPlaceholder(expression, null, placeholders); + collectPlaceholder(expression, null, node, placeholders); } for (int child = 0; child < node.jjtGetNumChildren(); child++) { @@ -214,12 +240,14 @@ private CastExpression innermostCast(CastExpression cast) { * walk reaches it. * *

One expression can hang under two parse-tree nodes, so a placeholder - * is recorded once, identified by the expression itself, and the cast that - * casts it is kept whichever node reports it first. + * is recorded once, identified by the expression itself, with where it + * stands under the node that reports it first, and the cast that casts it + * is kept whichever node reports it first. */ private void collectPlaceholder( Object value, CastExpression cast, + Node node, List placeholders ) { if (!(value instanceof JdbcParameter) && !(value instanceof JdbcNamedParameter)) { @@ -233,14 +261,175 @@ private void collectPlaceholder( if (collected.placeholder() == placeholder) { if (cast != null && collected.cast() == null) { - placeholders.set(index, new PlaceholderOccurrence(placeholder, cast)); + placeholders.set( + index, + new PlaceholderOccurrence(placeholder, cast, collected.enclosing()) + ); } return; } } - placeholders.add(new PlaceholderOccurrence(placeholder, cast)); + placeholders.add( + new PlaceholderOccurrence(placeholder, cast, enclosing(node, placeholder)) + ); + } + + /** + * Where one placeholder stands, written as the source writes it: the + * smallest expression the parse tree encloses it in, or, when the parse + * tree encloses it in none, the clause it stands alone in. + */ + private String enclosing(Node node, Expression placeholder) { + String expression = enclosingExpression(node, placeholder); + + return expression == null + ? clauseKeyword(node) + : expression; + } + + /** + * The smallest expression the parse tree reports around one placeholder, + * written on one line, and {@code null} when the placeholder stands alone + * in its clause. + * + *

A node of no value, a node of the placeholder itself, and an + * expression list, such as the {@code (:a, :b)} of an {@code IN} predicate, + * name nothing of their own, so the search steps through them to the + * expression around them. A query, a subquery included, and a node that is + * not an expression at all, such as a select item or a join, open the + * clause the placeholder stands in. + */ + private String enclosingExpression(Node node, Expression placeholder) { + for (Node ancestor = node; ancestor != null; ancestor = ancestor.jjtGetParent()) { + Object value = ancestor.jjtGetValue(); + + if (value == null || value == placeholder || value instanceof ExpressionList) { + continue; + } + + if (value instanceof Select || !(value instanceof Expression)) { + return null; + } + + return writtenText(ancestor.jjtGetFirstToken(), ancestor.jjtGetLastToken()); + } + + return null; + } + + /** + * One span of tokens as the source writes it, on one line: each token + * carries its own source image, and whatever separates two tokens, which is + * whitespace, a comment, or both, is written as one space. + */ + private String writtenText(Token first, Token last) { + StringBuilder text = new StringBuilder(); + int previousEnd = -1; + + for (Token token = first; token != null; token = token.next) { + if (previousEnd >= 0 && token.absoluteBegin > previousEnd) { + text.append(' '); + } + + text.append(token.image); + + if (token == last) { + break; + } + + previousEnd = token.absoluteEnd; + } + + return text.toString(); + } + + /** + * The keyword of the clause one placeholder stands alone in, which is the + * nearest clause keyword written before it at its own parenthesis depth or + * at a depth enclosing it, in upper case. + * + *

The scan therefore forgets a keyword of a parenthesized group that + * closed before the placeholder and keeps one written outside a group the + * placeholder stands inside. A source whose clause no keyword of the subset + * opens is named by the keyword that opens the statement. + */ + private String clauseKeyword(Node node) { + Token placeholder = node.jjtGetFirstToken(); + Token first = statementRoot(node).jjtGetFirstToken(); + + List enclosingKeywords = new ArrayList<>(); + String keyword = first.image.toUpperCase(Locale.ROOT); + Token previous = null; + + for (Token token = first; token != null && token != placeholder; token = token.next) { + if (OPEN_PARENTHESIS.equals(token.image)) { + enclosingKeywords.add(keyword); + } else if (CLOSE_PARENTHESIS.equals(token.image)) { + if (!enclosingKeywords.isEmpty()) { + keyword = enclosingKeywords.remove(enclosingKeywords.size() - 1); + } + } else { + String clause = clauseKeyword(token, previous); + + if (clause != null) { + keyword = clause; + } + } + + previous = token; + } + + return keyword; + } + + /** + * The clause one token opens, and {@code null} when the token opens none. + * {@code ON} opens a join condition on its own and a distinct list after + * {@code DISTINCT}, and {@code BY} opens the clause {@code ORDER} or + * {@code GROUP} names. + */ + private String clauseKeyword(Token token, Token previous) { + String image = token.image.toUpperCase(Locale.ROOT); + + if (CLAUSE_KEYWORDS.contains(image)) { + return image; + } + + return switch (image) { + case "ON" -> isImage(previous, "DISTINCT") + ? "DISTINCT ON" + : "ON"; + case "BY" -> byKeyword(previous); + default -> null; + }; + } + + /** The clause a {@code BY} keyword opens, which the token before it names. */ + private String byKeyword(Token previous) { + if (isImage(previous, "ORDER")) { + return "ORDER BY"; + } + + return isImage(previous, "GROUP") + ? "GROUP BY" + : null; + } + + private boolean isImage(Token token, String image) { + return token != null && image.equalsIgnoreCase(token.image); + } + + /** The parse-tree root of the statement one node belongs to. */ + private Node statementRoot(Node node) { + Node root = node; + + while (root.jjtGetParent() != null) { + root = root.jjtGetParent(); + } + + return root; } /** diff --git a/sqlcj-cli/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java b/sqlcj-cli/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java index 9ccb2a2..cf9b12d 100644 --- a/sqlcj-cli/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java +++ b/sqlcj-cli/src/test/java/dev/sqlcj/analysis/QueryAnalyzerTest.java @@ -171,6 +171,25 @@ class QueryAnalyzerTest { ) ); + /** + * A schema whose {@code users} table carries the text and {@code JSONB} + * columns the untyped-placeholder diagnostics read around a placeholder. + */ + private static final Schema untypedSchema = new Schema( + List.of( + new Table( + "users", + List.of( + new Column("id", ColumnType.BIGINT, false), + new Column("name", ColumnType.VARCHAR, true), + new Column("email", ColumnType.TEXT, true), + new Column("metadata", ColumnType.JSONB, true) + ), + List.of() + ) + ) + ); + private static final Schema joinSchema = new Schema( List.of( new Table( @@ -3032,8 +3051,8 @@ void shouldRejectPlaceholderInUnsupportedLocation() { ); assertEquals( - "SQL placeholders [1, 2] are not the analyzed parameters [1]; " - + "a placeholder is in an unsupported location", + "Cannot infer the type of $2 in $2 + 1;" + + " state it with a cast, such as $2::text", exception.getMessage() ); } @@ -3319,8 +3338,8 @@ void shouldRejectUnsupportedNamedPlaceholderForm(String sql, String placeholder) /** * A named placeholder in a location this analyzer does not visit is - * reported by the placeholder accounting, which writes the placeholder as - * the query does. + * rejected as an untyped placeholder, which writes the placeholder and the + * expression it stands in as the query does. */ @Test void shouldRejectNamedPlaceholderInUnanalyzedLocation() { @@ -3335,8 +3354,8 @@ void shouldRejectNamedPlaceholderInUnanalyzedLocation() { ); assertEquals( - "SQL placeholders [:name] are not the analyzed parameters []; " - + "a placeholder is in an unsupported location", + "Cannot infer the type of :name in lower(name) = :name;" + + " state it with a cast, such as :name::text", exception.getMessage() ); } @@ -3981,20 +4000,27 @@ void shouldRejectConflictingCastsOfDifferentClauses() { /** * Only a placeholder a cast states the type of is typed outside the clauses * that name a parameter, so an uncast placeholder, and a placeholder a cast - * does not cast directly, stay rejected by the placeholder accounting. + * does not cast directly, are rejected as untyped, each named with the + * expression or clause it stands in. */ @ParameterizedTest @CsvSource( delimiter = '|', quoteCharacter = '"', value = { - "SELECT id FROM users ORDER BY $1|[1]", - "SELECT id FROM users ORDER BY CASE WHEN :sort = 'name' THEN name END|[:sort]", - "SELECT id FROM users ORDER BY (:sort)::int|[:sort]", - "SELECT id FROM users WHERE EXISTS (SELECT 1 FROM orders WHERE orders.id = :id)|[:id]" + "SELECT id FROM users ORDER BY $1|$1|ORDER BY", + "SELECT id FROM users ORDER BY CASE WHEN :sort = 'name' THEN name END" + + "|:sort|:sort = 'name'", + "SELECT id FROM users ORDER BY (:sort)::int|:sort|(:sort)::int", + "SELECT id FROM users WHERE EXISTS (SELECT 1 FROM orders WHERE orders.id = :id)" + + "|:id|orders.id = :id" } ) - void shouldRejectUncastPlaceholderOutsideTheAnalyzedClauses(String sql, String compiled) { + void shouldRejectUncastPlaceholderOutsideTheAnalyzedClauses( + String sql, + String placeholder, + String enclosing + ) { Query query = new Query("ListUsers", QueryType.MANY, sql); ParsedSql parsedSql = parser.parse(sql); @@ -4004,9 +4030,8 @@ void shouldRejectUncastPlaceholderOutsideTheAnalyzedClauses(String sql, String c ); assertEquals( - "SQL placeholders %s are not the analyzed parameters []; " - .formatted(compiled) - + "a placeholder is in an unsupported location", + "Cannot infer the type of %s in %s; state it with a cast, such as %s::text" + .formatted(placeholder, enclosing, placeholder), exception.getMessage() ); } @@ -4033,6 +4058,91 @@ void shouldRejectUnsupportedCastTypeOutsideTheAnalyzedClauses() { ); } + /** + * A placeholder no clause types and no cast states the type of is rejected + * naming it as the query spells it, naming where the query writes it, and + * stating the cast that types it, in either placeholder form. Where it + * stands is the smallest expression written around it, and the keyword of + * its clause where the query writes no expression around it. + */ + @ParameterizedTest + @CsvSource( + delimiter = '|', + quoteCharacter = '"', + value = { + "SELECT id FROM users WHERE email = lower(:email)|:email|lower(:email)", + "SELECT id FROM users WHERE email = lower($1)|$1|lower($1)", + "SELECT id FROM users WHERE metadata->>'k' = :v|:v|metadata->>'k' = :v", + "SELECT id FROM users ORDER BY :sort|:sort|ORDER BY", + "SELECT id FROM users ORDER BY name, $1|$1|ORDER BY", + "SELECT id FROM users LIMIT :n + 1|:n|:n + 1", + "SELECT id FROM users WHERE EXISTS (SELECT 1 FROM tasks t WHERE t.x = :x)" + + "|:x|t.x = :x", + "SELECT $1 AS x FROM users|$1|SELECT" + } + ) + void shouldRejectAnUntypedPlaceholder(String sql, String placeholder, String enclosing) { + Query query = new Query("ListUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, untypedSchema) + ); + + assertEquals( + "Cannot infer the type of %s in %s; state it with a cast, such as %s::text" + .formatted(placeholder, enclosing, placeholder), + exception.getMessage() + ); + } + + /** + * A placeholder of a concatenated pattern stands in the whole {@code LIKE} + * predicate, which is the smallest expression the query writes around it. + */ + @Test + void shouldRejectAnUntypedPlaceholderOfAConcatenatedPattern() { + String sql = "SELECT id FROM users WHERE name LIKE '%' || :q || '%'"; + + Query query = new Query("ListUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, untypedSchema) + ); + + assertEquals( + "Cannot infer the type of :q in name LIKE '%' || :q || '%';" + + " state it with a cast, such as :q::text", + exception.getMessage() + ); + } + + /** + * One query reports one untyped placeholder, which is the first in textual + * order, so a repaired query reports the next one. + */ + @Test + void shouldRejectTheFirstUntypedPlaceholderOfAQuery() { + String sql = "SELECT id FROM users WHERE lower(name) = :a AND lower(email) = :b"; + + Query query = new Query("ListUsers", QueryType.MANY, sql); + ParsedSql parsedSql = parser.parse(sql); + + UnsupportedOperationException exception = assertThrows( + UnsupportedOperationException.class, + () -> analyzer.analyze(query, parsedSql, untypedSchema) + ); + + assertEquals( + "Cannot infer the type of :a in lower(name) = :a;" + + " state it with a cast, such as :a::text", + exception.getMessage() + ); + } + @Test void shouldAnalyzeSelectDeclaredAsOptional() { String sql = "SELECT id, name FROM users WHERE id = $1"; @@ -5117,7 +5227,8 @@ void shouldRejectAnUnknownNonBindingTargetColumn(String sql) { /** * A write value that contains a placeholder without being one leaves that - * placeholder unanalyzed, which the placeholder accounting rejects. + * placeholder untyped, which is rejected naming the value expression it + * stands in. */ @Test void shouldRejectAPlaceholderInsideAnInsertValue() { @@ -5132,8 +5243,8 @@ void shouldRejectAPlaceholderInsideAnInsertValue() { ); assertEquals( - "SQL placeholders [1, 2] are not the analyzed parameters [1];" - + " a placeholder is in an unsupported location", + "Cannot infer the type of $2 in $2 || 'x';" + + " state it with a cast, such as $2::text", exception.getMessage() ); } @@ -5151,8 +5262,8 @@ void shouldRejectAPlaceholderInsideAnUpdateAssignment() { ); assertEquals( - "SQL placeholders [:n, :id] are not the analyzed parameters [:id];" - + " a placeholder is in an unsupported location", + "Cannot infer the type of :n in :n || name;" + + " state it with a cast, such as :n::text", exception.getMessage() ); } @@ -5697,7 +5808,7 @@ private T conflictRejection( /** * A placeholder inside a {@code DO UPDATE} value is analyzed only through - * its own cast, so an uncast one stays unaccounted. + * its own cast, so an uncast one is rejected as untyped. */ @Test void shouldRejectAnUncastPlaceholderInsideAConflictUpdateValue() { @@ -5715,8 +5826,8 @@ INSERT INTO users (id, name) ); assertEquals( - "SQL placeholders [1, 2, 3] are not the analyzed parameters [1, 2];" - + " a placeholder is in an unsupported location", + "Cannot infer the type of $3 in version + $3;" + + " state it with a cast, such as $3::text", exception.getMessage() ); } @@ -5960,22 +6071,23 @@ void shouldRejectAListParameterOfAColumnWithoutAnArrayBinding( /** * Only the exact list-predicate shape binds a list, so another operator, * another operand order, another quantifier, a modifier, and an argument - * that is not one placeholder leave their placeholders unaccounted. + * that is not one placeholder leave their placeholders untyped, each + * rejected naming the expression it stands in. */ @ParameterizedTest @CsvSource( delimiter = '|', quoteCharacter = '"', value = { - "SELECT id FROM users WHERE id <> ANY($1)|[1]", - "SELECT id FROM users WHERE id = SOME($1)|[1]", - "SELECT id FROM users WHERE ANY($1) = id|[1]", - "SELECT id FROM users WHERE id = ANY(ARRAY[$1, $2])|[1, 2]", - "SELECT id FROM users WHERE id = ANY(DISTINCT $1)|[1]", - "SELECT id FROM users WHERE id = ANY($1 ORDER BY id)|[1]" + "SELECT id FROM users WHERE id <> ANY($1)|ANY($1)", + "SELECT id FROM users WHERE id = SOME($1)|SOME($1)", + "SELECT id FROM users WHERE ANY($1) = id|ANY($1)", + "SELECT id FROM users WHERE id = ANY(ARRAY[$1, $2])|ARRAY[$1, $2]", + "SELECT id FROM users WHERE id = ANY(DISTINCT $1)|ANY(DISTINCT $1)", + "SELECT id FROM users WHERE id = ANY($1 ORDER BY id)|ANY($1 ORDER BY id)" } ) - void shouldRejectAPlaceholderOutsideTheListPredicateShape(String sql, String compiled) { + void shouldRejectAPlaceholderOutsideTheListPredicateShape(String sql, String enclosing) { Query query = new Query("ListUsers", QueryType.MANY, sql); ParsedSql parsedSql = parser.parse(sql); @@ -5985,8 +6097,8 @@ void shouldRejectAPlaceholderOutsideTheListPredicateShape(String sql, String com ); assertEquals( - "SQL placeholders %s are not the analyzed parameters []; ".formatted(compiled) - + "a placeholder is in an unsupported location", + "Cannot infer the type of $1 in %s; state it with a cast, such as $1::text" + .formatted(enclosing), exception.getMessage() ); } diff --git a/sqlcj-cli/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java b/sqlcj-cli/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java index 300faf0..907beb4 100644 --- a/sqlcj-cli/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java +++ b/sqlcj-cli/src/test/java/dev/sqlcj/compiler/SqlcjCompilerIntegrationTest.java @@ -2313,6 +2313,40 @@ void shouldRejectAnonymousPlaceholderBeforeWriting() { assertFalse(Files.exists(generatedDirectory)); } + /** + * An untyped placeholder is reported as one located line that names the + * placeholder, the expression the query writes it in, and the cast that + * types it, even when that expression spans lines in the source. + */ + @Test + void shouldRejectUntypedPlaceholderBeforeWriting() { + Path generatedDirectory = tempDir.resolve("generated"); + + CompilationException exception = assertThrows( + CompilationException.class, + () -> compileUsersQueries( + """ + -- name: ListUsers :many + SELECT id + FROM users + WHERE lower(name) = + :name; + """, + generatedDirectory + ) + ); + + assertEquals( + "Invalid query 'ListUsers' in %s at line 1: " + .formatted(tempDir.resolve("queries.sql")) + + "Cannot infer the type of :name in lower(name) = :name; " + + "state it with a cast, such as :name::text", + exception.getMessage() + ); + + assertFalse(Files.exists(generatedDirectory)); + } + @Test void shouldRejectRepeatedRepositoryMethodBeforeWriting() throws IOException { Path generatedDirectory = tempDir.resolve("generated"); diff --git a/sqlcj-cli/src/test/java/dev/sqlcj/sql/SqlParserTest.java b/sqlcj-cli/src/test/java/dev/sqlcj/sql/SqlParserTest.java index 2ffebce..ed7cc7f 100644 --- a/sqlcj-cli/src/test/java/dev/sqlcj/sql/SqlParserTest.java +++ b/sqlcj-cli/src/test/java/dev/sqlcj/sql/SqlParserTest.java @@ -368,6 +368,84 @@ void shouldReportTheCastThatCastsAPlaceholderDirectly() { assertEquals(List.of(1, 2, 1), parsedSql.parameters().indexes()); } + /** + * Where a placeholder stands is the smallest expression the parse tree + * reports around it, written as the source writes it: an {@code IN} list is + * not an expression of its own, so its placeholders stand in the whole + * {@code IN} predicate, and an expression written across lines is written + * on one line, with whatever separates two tokens written as one space. + */ + @Test + void shouldReportTheEnclosingExpressionOfAPlaceholder() { + assertEquals( + List.of("lower(name) = :name"), + enclosings("SELECT id FROM users WHERE lower(name) = :name") + ); + + assertEquals( + List.of("id IN (:a, :b)", "id IN (:a, :b)"), + enclosings("SELECT id FROM users WHERE id IN (:a, :b)") + ); + + assertEquals( + List.of("name LIKE '%' || :q || '%'"), + enclosings(""" + SELECT id + FROM users + WHERE name LIKE '%' || -- the prefix + :q || '%'""") + ); + } + + /** + * A placeholder the parse tree reports no expression around stands alone in + * its clause, which the keyword that opens that clause names in upper case. + */ + @Test + void shouldReportTheClauseOfAPlaceholderThatStandsAlone() { + assertEquals( + List.of("ORDER BY"), + enclosings("SELECT id FROM users ORDER BY name, :sort") + ); + + assertEquals( + List.of("GROUP BY"), + enclosings("SELECT id FROM users GROUP BY :grouping") + ); + + assertEquals( + List.of("HAVING"), + enclosings("SELECT id FROM users GROUP BY name HAVING $1") + ); + + assertEquals( + List.of("FETCH"), + enclosings("SELECT id FROM users FETCH FIRST :rows ROWS ONLY") + ); + + assertEquals( + List.of("ON"), + enclosings("SELECT u.id FROM users u JOIN profiles p ON :matches") + ); + + assertEquals( + List.of("DISTINCT ON"), + enclosings("SELECT DISTINCT ON (:sort) id FROM users") + ); + + assertEquals( + List.of("WHERE"), + enclosings("UPDATE users SET name = 'x' WHERE :matches") + ); + } + + /** Where each placeholder occurrence of a source stands. */ + private List enclosings(String sql) { + return parser.parse(sql).placeholders().stream() + .map(PlaceholderOccurrence::enclosing) + .toList(); + } + private List placeholderNames(ParsedSql parsedSql) { return parsedSql.placeholders().stream() .map(occurrence -> ((JdbcNamedParameter) occurrence.placeholder()).getName())