Skip to content

Commit 1b57bfa

Browse files
manticore-projectsmanticore-projects
committed
fix(adapter): connect StatementVisitorAdapter sub-visitors and traverse DML CTEs
Signed-off-by: Andreas Reichel <andreas@manticore-projects.com>
1 parent 5c59b7b commit 1b57bfa

4 files changed

Lines changed: 248 additions & 8 deletions

File tree

‎gradle.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Specifies the JVM arguments used for the daemon process.
22
# The setting is particularly useful for tweaking memory settings.
3-
org.gradle.jvmargs=-Xmx8G -Xss8m -Dfile.encoding=UTF-8 -XX:+HeapDumpOnOutOfMemoryError
3+
org.gradle.jvmargs=-Xmx8G -Xss16m -Dfile.encoding=UTF-8 -XX:+HeapDumpOnOutOfMemoryError
44

55
org.gradle.caching=true
66

‎src/main/java/net/sf/jsqlparser/statement/StatementVisitorAdapter.java‎

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,14 +72,46 @@ public class StatementVisitorAdapter<T> implements StatementVisitor<T> {
7272
private final SelectVisitor<T> selectVisitor;
7373
private final MergeOperationVisitor<T> mergeOperationVisitor;
7474

75+
/**
76+
* Builds a fully connected visitor graph.
77+
*
78+
* <p>
79+
* The sub-visitors are mutually recursive, so none of them can be handed its collaborators at
80+
* construction time; the cycle has to be closed afterwards through the setters. The previous
81+
* version skipped that step and the graph was not actually connected:
82+
*
83+
* <ul>
84+
* <li>{@code new ExpressionVisitorAdapter<>()} leaves its {@code selectVisitor} null, and
85+
* {@code ExpressionVisitorAdapter#visit(Select)} is a no-op when it is null - so every
86+
* sub-select in an expression ({@code WHERE id IN (SELECT ..)}, scalar subqueries, EXISTS) was
87+
* silently dropped;</li>
88+
* <li>{@code new FromItemVisitorAdapter<>()} builds its own throwaway SelectVisitorAdapter and
89+
* ExpressionVisitorAdapter, so subqueries in FROM were traversed by a detached chain that no
90+
* subclass of this adapter could observe.</li>
91+
* </ul>
92+
*
93+
* <p>
94+
* Both meant a subclass overriding a visit method never saw the nested nodes. The three setter
95+
* calls below fix that; {@code setStatementVisitor} additionally lets the select visitor hand a
96+
* data-modifying CTE body back here.
97+
*/
7598
public StatementVisitorAdapter() {
76-
this.expressionVisitor = new ExpressionVisitorAdapter<>();
77-
this.pivotVisitor = new PivotVisitorAdapter<>(this.expressionVisitor);
78-
this.selectItemVisitor = new SelectItemVisitorAdapter<>(this.expressionVisitor);
79-
this.fromItemVisitor = new FromItemVisitorAdapter<>();
99+
ExpressionVisitorAdapter<T> expressions = new ExpressionVisitorAdapter<>();
100+
FromItemVisitorAdapter<T> fromItems = new FromItemVisitorAdapter<>();
80101

81-
this.selectVisitor = new SelectVisitorAdapter<>(this.expressionVisitor, this.pivotVisitor,
82-
this.selectItemVisitor, this.fromItemVisitor);
102+
this.expressionVisitor = expressions;
103+
this.pivotVisitor = new PivotVisitorAdapter<>(expressions);
104+
this.selectItemVisitor = new SelectItemVisitorAdapter<>(expressions);
105+
this.fromItemVisitor = fromItems;
106+
107+
SelectVisitorAdapter<T> selects = new SelectVisitorAdapter<>(expressions, this.pivotVisitor,
108+
this.selectItemVisitor, fromItems);
109+
110+
expressions.setSelectVisitor(selects);
111+
fromItems.setSelectVisitor((SelectVisitor<T>) selects).setExpressionVisitor(expressions);
112+
selects.setStatementVisitor(this);
113+
114+
this.selectVisitor = selects;
83115
this.mergeOperationVisitor = new MergeOperationVisitorAdapter<>();
84116
}
85117

@@ -93,17 +125,36 @@ public StatementVisitorAdapter(ExpressionVisitor<T> expressionVisitor,
93125
this.fromItemVisitor = fromItemVisitor;
94126
this.selectVisitor = selectVisitor;
95127
this.mergeOperationVisitor = mergeOperationVisitor;
128+
adoptAsStatementVisitor(selectVisitor);
96129
}
97130

98131
public StatementVisitorAdapter(SelectVisitorAdapter<T> selectVisitor) {
99132
this.selectVisitor = selectVisitor;
133+
adoptAsStatementVisitor(selectVisitor);
100134
this.expressionVisitor = selectVisitor.getExpressionVisitor();
101135
this.pivotVisitor = selectVisitor.getPivotVisitor();
102136
this.selectItemVisitor = selectVisitor.getSelectItemVisitor();
103137
this.fromItemVisitor = selectVisitor.getFromItemVisitor();
104138
this.mergeOperationVisitor = new MergeOperationVisitorAdapter<>();
105139
}
106140

141+
/**
142+
* Lets the select visitor hand a data-modifying CTE body back to this statement visitor.
143+
* Without it, {@code SelectVisitorAdapter#visit(WithItem)} has no way to traverse
144+
* {@code WITH c AS (DELETE FROM t RETURNING *) ..} and silently skips it.
145+
*
146+
* <p>
147+
* An explicitly configured statement visitor is never overwritten.
148+
*/
149+
private void adoptAsStatementVisitor(SelectVisitor<T> visitor) {
150+
if (visitor instanceof SelectVisitorAdapter) {
151+
SelectVisitorAdapter<T> adapter = (SelectVisitorAdapter<T>) visitor;
152+
if (adapter.getStatementVisitor() == null) {
153+
adapter.setStatementVisitor(this);
154+
}
155+
}
156+
}
157+
107158
@Override
108159
public <S> T visit(Comment comment, S context) {
109160

@@ -137,6 +188,10 @@ public <S> T visit(Delete delete, S context) {
137188

138189
expressionVisitor.visitLimit(delete.getLimit(), context);
139190

191+
// was missing, unlike visit(Insert) and visit(Update):
192+
// DELETE FROM t RETURNING f(x) left f(x) untraversed
193+
visitReturningClause(delete.getReturningClause(), context);
194+
140195
return null;
141196
}
142197

‎src/main/java/net/sf/jsqlparser/statement/select/SelectVisitorAdapter.java‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
import net.sf.jsqlparser.expression.ExpressionVisitorAdapter;
1414
import net.sf.jsqlparser.expression.Function;
1515
import net.sf.jsqlparser.statement.OutputClause;
16+
import net.sf.jsqlparser.statement.ParenthesedStatement;
17+
import net.sf.jsqlparser.statement.Statement;
18+
import net.sf.jsqlparser.statement.StatementVisitor;
1619
import net.sf.jsqlparser.statement.piped.FromQuery;
1720

1821
@SuppressWarnings({"PMD.UncommentedEmptyMethodBody"})
@@ -22,6 +25,14 @@ public class SelectVisitorAdapter<T> implements SelectVisitor<T> {
2225
private final SelectItemVisitor<T> selectItemVisitor;
2326
private final FromItemVisitor<T> fromItemVisitor;
2427

28+
/**
29+
* Optional, and only needed for data-modifying CTEs - see {@link #visit(WithItem, Object)}. A
30+
* plain SelectVisitor cannot accept an INSERT/UPDATE/DELETE body; without this the CTE is
31+
* skipped rather than traversed. {@link net.sf.jsqlparser.statement.StatementVisitorAdapter}
32+
* sets it automatically.
33+
*/
34+
private StatementVisitor<T> statementVisitor;
35+
2536
public SelectVisitorAdapter() {
2637
this.expressionVisitor = new ExpressionVisitorAdapter<>(this);
2738
this.pivotVisitor = new PivotVisitorAdapter<>(this.expressionVisitor);
@@ -91,6 +102,15 @@ public FromItemVisitor<T> getFromItemVisitor() {
91102
return fromItemVisitor;
92103
}
93104

105+
public StatementVisitor<T> getStatementVisitor() {
106+
return statementVisitor;
107+
}
108+
109+
public SelectVisitorAdapter<T> setStatementVisitor(StatementVisitor<T> statementVisitor) {
110+
this.statementVisitor = statementVisitor;
111+
return this;
112+
}
113+
94114
@Override
95115
public <S> T visit(ParenthesedSelect select, S context) {
96116
visitWithItems(select.withItemsList, context);
@@ -257,9 +277,33 @@ public <S> T visit(SetOperationList setOpList, S context) {
257277
return null;
258278
}
259279

280+
/**
281+
* A CTE body is a {@link ParenthesedStatement}, which is not necessarily a SELECT: PostgreSQL
282+
* and others allow {@code WITH c AS (DELETE FROM t RETURNING *) SELECT * FROM c}.
283+
*
284+
* <p>
285+
* The previous implementation called {@code withItem.getSelect()}, an unchecked cast to
286+
* {@link ParenthesedSelect}, and threw {@link ClassCastException} on every data-modifying CTE
287+
* reached through the select path - which {@code ExpressionVisitorAdapter#visit(Select)} does
288+
* for any sub-select. {@code WithItem#accept(StatementVisitor)} has always dispatched correctly
289+
* through {@code getParenthesedStatement()}; only this path did not.
290+
*/
260291
@Override
261292
public <S> T visit(WithItem<?> withItem, S context) {
262-
return withItem.getSelect().accept(this, context);
293+
ParenthesedStatement body = withItem.getParenthesedStatement();
294+
295+
// ParenthesedSelect is a Select and stays on the select path
296+
if (body instanceof Select) {
297+
return ((Select) body).accept(this, context);
298+
}
299+
300+
// ParenthesedInsert / ParenthesedUpdate / ParenthesedDelete need a StatementVisitor
301+
if (statementVisitor != null && body instanceof Statement) {
302+
return ((Statement) body).accept(statementVisitor, context);
303+
}
304+
305+
// no statement visitor available: skip the body rather than fail
306+
return null;
263307
}
264308

265309
@Override

‎src/test/java/net/sf/jsqlparser/statement/AdapterRegressionTest.java‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
/*-
2+
* #%L
3+
* JSQLParser library
4+
* %%
5+
* Copyright (C) 2004 - 2026 JSQLParser
6+
* %%
7+
* Dual licensed under GNU LGPL 2.1 or Apache License 2.0
8+
* #L%
9+
*/
10+
package net.sf.jsqlparser.statement;
11+
12+
import net.sf.jsqlparser.JSQLParserException;
13+
import net.sf.jsqlparser.expression.ExpressionVisitorAdapter;
14+
import net.sf.jsqlparser.expression.Function;
15+
import net.sf.jsqlparser.parser.CCJSqlParserUtil;
16+
import net.sf.jsqlparser.statement.select.Select;
17+
import net.sf.jsqlparser.statement.select.SelectVisitorAdapter;
18+
import org.junit.jupiter.api.DisplayName;
19+
import org.junit.jupiter.api.Test;
20+
21+
import java.util.ArrayList;
22+
import java.util.List;
23+
24+
import static org.assertj.core.api.Assertions.assertThat;
25+
import static org.assertj.core.api.Assertions.assertThatCode;
26+
27+
/**
28+
* Regressions for two adapter traversal defects.
29+
*/
30+
class AdapterRegressionTest {
31+
32+
/**
33+
* {@code SelectVisitorAdapter#visit(WithItem)} used to call {@code withItem.getSelect()}, an
34+
* unchecked cast to {@code ParenthesedSelect}. Any data-modifying CTE reached through the
35+
* select path threw {@link ClassCastException} - and the select path is taken for every
36+
* sub-select, because {@code ExpressionVisitorAdapter#visit(Select)} loops the with-items into
37+
* the select visitor.
38+
*/
39+
@Test
40+
@DisplayName("a data-modifying CTE inside a sub-select no longer throws ClassCastException")
41+
void dataModifyingCteOnTheSelectPath() throws JSQLParserException {
42+
Statement statement = CCJSqlParserUtil.parse("SELECT * FROM u WHERE id IN ("
43+
+ "WITH c AS (DELETE FROM t RETURNING id) SELECT id FROM c)");
44+
45+
assertThatCode(() -> statement.accept(new StatementVisitorAdapter<>(), null))
46+
.doesNotThrowAnyException();
47+
}
48+
49+
/**
50+
* The CTE body must actually be traversed, not merely survived.
51+
*
52+
* <p>
53+
* This is the test that catches <em>both</em> defects, and it is the one that exposed the
54+
* larger of the two. Reaching {@code visit(Delete)} here requires:
55+
*
56+
* <ol>
57+
* <li>the expression chain to be connected at all. {@code StatementVisitorAdapter()} used to
58+
* build its {@code ExpressionVisitorAdapter} with the no-arg constructor, which leaves
59+
* {@code selectVisitor} null - and {@code ExpressionVisitorAdapter#visit(Select)} returns
60+
* immediately when it is. Every sub-select inside an expression (IN, EXISTS, scalar) was
61+
* silently dropped, so no subclass could observe anything below one. Its
62+
* {@code FromItemVisitorAdapter} had the mirror problem: the no-arg constructor builds a
63+
* throwaway select/expression chain of its own.</li>
64+
* <li>{@code SelectVisitorAdapter#visit(WithItem)} to dispatch the CTE body through
65+
* {@code getParenthesedStatement()} rather than casting it to ParenthesedSelect, and to have a
66+
* statement visitor to hand a DELETE body to.</li>
67+
* </ol>
68+
*
69+
* <p>
70+
* Defect (1) is also why (2) never surfaced as a ClassCastException in practice: the traversal
71+
* never got that far. Fixing the wiring is what makes the cast reachable, so the two fixes have
72+
* to land together.
73+
*
74+
* <p>
75+
* Note this can only be asserted through its effects - {@code StatementVisitorAdapter} exposes
76+
* no getters for its sub-visitors, so the wiring cannot be inspected directly. Adding them
77+
* would be a reasonable follow-up.
78+
*/
79+
@Test
80+
@DisplayName("the CTE body is traversed, not skipped")
81+
void dataModifyingCteBodyIsVisited() throws JSQLParserException {
82+
List<String> visited = new ArrayList<>();
83+
84+
StatementVisitorAdapter<Void> visitor = new StatementVisitorAdapter<Void>() {
85+
@Override
86+
public <S> Void visit(net.sf.jsqlparser.statement.delete.Delete delete, S context) {
87+
visited.add("delete");
88+
return super.visit(delete, context);
89+
}
90+
};
91+
92+
CCJSqlParserUtil
93+
.parse("SELECT * FROM u WHERE id IN ("
94+
+ "WITH c AS (DELETE FROM t RETURNING id) SELECT id FROM c)")
95+
.accept(visitor, null);
96+
97+
assertThat(visited).containsExactly("delete");
98+
}
99+
100+
/**
101+
* A bare SelectVisitorAdapter has no statement visitor, so it cannot accept a DELETE body. It
102+
* must skip it rather than fail - the old cast made this case fatal.
103+
*/
104+
@Test
105+
@DisplayName("a standalone SelectVisitorAdapter skips a DML CTE instead of failing")
106+
void standaloneSelectVisitorSkipsDmlCte() throws JSQLParserException {
107+
Select select = (Select) CCJSqlParserUtil
108+
.parse("WITH c AS (DELETE FROM t RETURNING id) SELECT id FROM c");
109+
110+
SelectVisitorAdapter<Void> selects = new SelectVisitorAdapter<>();
111+
assertThat(selects.getStatementVisitor()).isNull();
112+
assertThatCode(() -> select.accept(selects, null)).doesNotThrowAnyException();
113+
}
114+
115+
/**
116+
* {@code StatementVisitorAdapter#visit(Delete)} never called {@code visitReturningClause},
117+
* unlike its Insert and Update counterparts, so expressions in the RETURNING list were never
118+
* handed to the expression visitor.
119+
*/
120+
@Test
121+
@DisplayName("DELETE .. RETURNING expressions are traversed")
122+
void deleteReturningExpressionsAreVisited() throws JSQLParserException {
123+
List<String> functions = new ArrayList<>();
124+
125+
ExpressionVisitorAdapter<Void> expressions = new ExpressionVisitorAdapter<Void>() {
126+
@Override
127+
public <S> Void visit(Function function, S context) {
128+
functions.add(function.getName().toLowerCase());
129+
return super.visit(function, context);
130+
}
131+
};
132+
133+
StatementVisitorAdapter<Void> visitor =
134+
new StatementVisitorAdapter<>(new SelectVisitorAdapter<>(expressions));
135+
136+
CCJSqlParserUtil.parse("DELETE FROM t WHERE a = 1 RETURNING upper(name)")
137+
.accept(visitor, null);
138+
139+
assertThat(functions).contains("upper");
140+
}
141+
}

0 commit comments

Comments
 (0)