Skip to content

Commit f00388f

Browse files
committed
feat(analysis): diagnose untyped placeholders and suggest a cast
A placeholder that no clause types and no cast states the type of is now rejected as "Cannot infer the type of <placeholder> in <where>; state it with a cast, such as <placeholder>::text", naming the first such placeholder in textual order as the query spells it. A bare placeholder projection gets the same diagnostic. Clause rejections still come first, and the placeholder accounting check remains as an internal guard.
1 parent 94fa78d commit f00388f

4 files changed

Lines changed: 277 additions & 43 deletions

File tree

‎docs/queries.md‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,19 @@ string literal, a quoted identifier, or a comment is not a parameter.
602602
such as `:"x"`, and an `&name` placeholder.
603603
- A placeholder that neither a clause nor a cast types — for example
604604
`ORDER BY $1`, `lower(name) = :name`, or `(:x)::int`, whose placeholder is not
605-
the direct operand of its cast — is rejected rather than left unbound.
605+
the direct operand of its cast — is rejected rather than left unbound, as
606+
`Cannot infer the type of :name in lower(name) = :name; state it with a cast,
607+
such as :name::text`. The diagnostic writes the placeholder as the query
608+
spells it, `$1` or `:name`, and names where the query writes it: the smallest
609+
expression written around the placeholder, on one line, with whatever
610+
separates two tokens written as one space. A concatenated pattern is
611+
therefore named in its whole `LIKE` predicate and an `IN` list in its `IN`
612+
predicate, while a placeholder written inside no expression stands alone in
613+
its clause and is named by the keyword that opens that clause, in upper case:
614+
`SELECT`, `DISTINCT ON`, `ON`, `WHERE`, `GROUP BY`, `HAVING`, `ORDER BY`,
615+
`LIMIT`, `OFFSET`, `FETCH`, `VALUES`, `SET`, or `RETURNING`. One query reports
616+
its first untyped placeholder in textual order, after the rules of its clauses
617+
have run, so a clause's own rejection comes first.
606618

607619
### Logical order versus textual order
608620

@@ -882,7 +894,12 @@ name, and its header line.
882894
`LIMIT $1 + 1` or `OFFSET $1 + 1`, a `LIMIT a, b` row count such as
883895
`LIMIT 5, $1`, a placeholder inside a write value such as
884896
`COALESCE($2, bio)`, and a `FETCH FIRST $1 ROWS ONLY` clause, so dynamic `IN`
885-
expansion is unavailable. A cast states the type of such a placeholder in
897+
expansion is unavailable. Each is rejected as `Cannot infer the type of $1 in
898+
$1 + 1; state it with a cast, such as $1::text`, which names the placeholder
899+
as the query spells it and, after `in`, the smallest expression the query
900+
writes around it, on one line, or the upper-case keyword of the clause the
901+
placeholder stands alone in, such as `ORDER BY` for `ORDER BY $1` and
902+
`SELECT` for `SELECT $1 AS x`. A cast states the type of such a placeholder in
886903
every one of those locations, and it does not accept a placeholder that is not
887904
the direct operand of its cast, such as `(:x)::int`.
888905
- A cast type sqlcj does not map, such as `$1::interval` or a
@@ -1220,7 +1237,7 @@ Writes and `RETURNING`:
12201237
and `QueryAnalyzerTest.shouldRejectAnUnknownConflictColumn` cover the accepted
12211238
conflict target, both actions, the `EXCLUDED`, placeholder, cast, and
12221239
computed assignments, the binding order after the `VALUES` placeholders, the
1223-
returning row type, and the conflict forms, unaccounted placeholder, and
1240+
returning row type, and the conflict forms, untyped placeholder, and
12241241
unknown target, assigned, and `EXCLUDED` columns that are rejected on an
12251242
`:exec` and on a returning insert alike, and
12261243
`PostgresIntegrationTest.shouldExecuteGeneratedUpsertsAgainstPostgres`
@@ -1268,6 +1285,15 @@ Parameters:
12681285
`QueryAnalyzerTest.shouldRejectAnonymousParameter`, and
12691286
`QueryAnalyzerTest.shouldKeepPlaceholderTextThatIsNotAParameter` cover
12701287
ordering, repetition, and rejection.
1288+
- `QueryAnalyzerTest.shouldRejectAnUntypedPlaceholder`,
1289+
`QueryAnalyzerTest.shouldRejectAnUntypedPlaceholderOfAConcatenatedPattern`,
1290+
`QueryAnalyzerTest.shouldRejectTheFirstUntypedPlaceholderOfAQuery`,
1291+
`SqlParserTest.shouldReportTheEnclosingExpressionOfAPlaceholder`,
1292+
`SqlParserTest.shouldReportTheClauseOfAPlaceholderThatStandsAlone`, and
1293+
`SqlcjCompilerIntegrationTest.shouldRejectUntypedPlaceholderBeforeWriting`
1294+
cover the untyped-placeholder diagnostic, in both placeholder forms, with the
1295+
enclosing expression and the clause keyword it names, the first occurrence it
1296+
reports, and the whole located line of a compiled source.
12711297
- `SqlParserTest.shouldCompileNamedParametersByFirstOccurrence`,
12721298
`SqlParserTest.shouldCompileNamesThatDifferInCaseAsDistinctParameters`,
12731299
`SqlParserTest.shouldReportBothPlaceholderFormsOfOneSource`,

‎sqlcj-cli/src/main/java/dev/sqlcj/analysis/QueryAnalyzer.java‎

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,15 @@ public final class QueryAnalyzer {
8181
private static final String MIXED_PLACEHOLDER_REJECTION = """
8282
Indexed '$N' and named ':name' placeholders must not be mixed in one query""";
8383

84+
/**
85+
* The rejection of a placeholder that no clause of the query types and no
86+
* cast states the type of, which names the placeholder as the query spells
87+
* it and where the query writes it. The suggested cast is always
88+
* {@code ::text}, because sqlcj infers no type for such a placeholder.
89+
*/
90+
private static final String UNTYPED_PLACEHOLDER_REJECTION = """
91+
Cannot infer the type of %s in %s; state it with a cast, such as %s::text""";
92+
8493
/** The character that opens a supported named placeholder. */
8594
private static final String NAME_PREFIX = ":";
8695

@@ -337,7 +346,7 @@ private QueryModel analyzeSelect(Query query, ParsedSql parsedSql, Select select
337346

338347
List<Source> sources = resolveSources(plainSelect, table, schema);
339348

340-
List<QueryColumn> columns = resolveColumns(plainSelect, sources, schema);
349+
List<QueryColumn> columns = resolveColumns(plainSelect, parsedSql, sources, schema);
341350

342351
Placeholders placeholders = toPlaceholders(parsedSql, schema);
343352

@@ -864,8 +873,9 @@ private QueryModel toQueryModel(
864873
* <p>An occurrence a clause of the query typed is that clause's parameter.
865874
* Every other occurrence is typed by the cast it is the direct operand of,
866875
* wherever in the statement it stands, and is named after its own
867-
* placeholder. A placeholder neither a clause nor a cast types contributes
868-
* no parameter, so the accounting reports it as unaccounted.
876+
* placeholder. A placeholder neither a clause nor a cast types has no type
877+
* to generate, so the first such occurrence in textual order is rejected
878+
* naming where the query writes it.
869879
*/
870880
private List<QueryParameter> resolveOccurrences(ParsedSql parsedSql, Placeholders placeholders) {
871881
List<QueryParameter> occurrences = new ArrayList<>();
@@ -881,18 +891,61 @@ private List<QueryParameter> resolveOccurrences(ParsedSql parsedSql, Placeholder
881891

882892
CastPlaceholder cast = placeholders.ofCast(occurrence.cast());
883893

884-
if (cast != null) {
885-
occurrences.add(
886-
toCastParameter(cast, indexedParameterName(cast.placeholder()))
887-
);
894+
if (cast == null) {
895+
throw untypedPlaceholder(occurrence);
888896
}
897+
898+
occurrences.add(
899+
toCastParameter(cast, indexedParameterName(cast.placeholder()))
900+
);
889901
}
890902

891903
placeholders.requireReportedOccurrences();
892904

893905
return occurrences;
894906
}
895907

908+
/**
909+
* The rejection of one placeholder occurrence no clause of the query typed
910+
* and no cast states the type of, which the SQL parser reported together
911+
* with where the query writes it.
912+
*/
913+
private UnsupportedOperationException untypedPlaceholder(PlaceholderOccurrence occurrence) {
914+
String spelled = spelledPlaceholder(occurrence.placeholder());
915+
916+
return new UnsupportedOperationException(
917+
UNTYPED_PLACEHOLDER_REJECTION.formatted(spelled, occurrence.enclosing(), spelled)
918+
);
919+
}
920+
921+
/**
922+
* One placeholder expression as the query spells it, which is its own name
923+
* for a named placeholder and its index for an indexed one. Every other
924+
* placeholder form is already rejected before a placeholder is spelled.
925+
*/
926+
private String spelledPlaceholder(Expression placeholder) {
927+
if (placeholder instanceof JdbcNamedParameter named) {
928+
return NAME_PREFIX + named.getName();
929+
}
930+
931+
return "$" + ((JdbcParameter) placeholder).getIndex();
932+
}
933+
934+
/**
935+
* The occurrence the SQL parser reported for one placeholder expression of
936+
* the parsed source, which every placeholder of that source has.
937+
*/
938+
private PlaceholderOccurrence occurrenceOf(ParsedSql parsedSql, Expression placeholder) {
939+
return parsedSql.placeholders().stream()
940+
.filter(occurrence -> occurrence.placeholder() == placeholder)
941+
.findFirst()
942+
.orElseThrow(
943+
() -> new IllegalStateException(
944+
"A placeholder is not a reported placeholder occurrence."
945+
)
946+
);
947+
}
948+
896949
/**
897950
* Requires that the occurrences resolved against the schema are exactly the
898951
* placeholder tokens the SQL parser reported, in the same textual order, so
@@ -2048,9 +2101,14 @@ private Placeholders toPlaceholders(ParsedSql parsedSql, Schema schema) {
20482101
*
20492102
* <p>An aliased scalar count and an aliased cast each resolve to one column
20502103
* in their own position, so either may stand beside any other item.
2104+
*
2105+
* <p>A bare placeholder projection is a placeholder a cast makes a
2106+
* supported result column, so it is rejected as an untyped placeholder
2107+
* rather than as an unsupported result expression.
20512108
*/
20522109
private List<QueryColumn> resolveColumns(
20532110
PlainSelect plainSelect,
2111+
ParsedSql parsedSql,
20542112
List<Source> sources,
20552113
Schema schema
20562114
) {
@@ -2109,6 +2167,10 @@ private List<QueryColumn> resolveColumns(
21092167
continue;
21102168
}
21112169

2170+
if (expression instanceof JdbcParameter || expression instanceof JdbcNamedParameter) {
2171+
throw untypedPlaceholder(occurrenceOf(parsedSql, expression));
2172+
}
2173+
21122174
throw new UnsupportedOperationException(
21132175
"Unsupported SELECT expression: "
21142176
+ expression.getClass().getSimpleName()

0 commit comments

Comments
 (0)