Skip to content

fix: traverse skipped expression children in TablesNamesFinder - #2826

Open
AndreasLanz01 wants to merge 1 commit into
JSQLParser:masterfrom
AndreasLanz01:fix/tables-finder-expression-positions
Open

AndreasLanz01 wants to merge 1 commit into
JSQLParser:masterfrom
AndreasLanz01:fix/tables-finder-expression-positions

Conversation

@AndreasLanz01

Copy link
Copy Markdown
Contributor

What

TablesNamesFinder skipped some child expressions. A table read by a subquery in one of these positions was missing from getTables(..), getTablesOrOtherSources(..) and findTablesInExpression(..):

Node Child that was not visited
Function aggregate ORDER BY: string_agg(a, ',' ORDER BY ..), array_agg(a ORDER BY ..), group_concat(a ORDER BY ..); KEEP (DENSE_RANK FIRST ORDER BY ..); HAVING MAX ..; LIMIT ..; keyword arguments such as SEPARATOR ..; an attribute function: f(a).g(..)
AnalyticExpression HAVING MAX .., LIMIT .., keyword arguments
KeepExpression its ORDER BY (the visit was empty)
MySQLGroupConcat all children (the visit was empty); the grammar does not create this node, so this only affects trees built through the API and has no test
LikeExpression the ESCAPE expression
TimezoneExpression the time zone: d AT TIME ZONE (..)
JsonExpression operands after ->, ->>, #>, #>>
IsUnknownExpression the operand (the visit was empty)
FullTextSearch MATCH (..) AGAINST (..) columns and value (the visit was empty)
JsonAggregateFunction key and value of JSON_OBJECTAGG, its ORDER BY, OVER (PARTITION BY .. ORDER BY .. ROWS ..)
TableFunction XMLTABLE(.. PASSING ..) in FROM

Two private helpers keep this in one place: visitAggregateModifiers (KEEP, HAVING, keyword arguments and LIMIT of Function and AnalyticExpression) and visitWindowElement (window frame, moved out of visit(AnalyticExpression) and reused for JsonAggregateFunction).

visit(TableFunction) called visit(function, null), which binds at compile time to visit(Function). XmlTableFunction and JsonTableFunction extend Function, so their own visit methods were never reached. It now calls function.accept(this, context), which also passes the caller's context instead of null. JSON_TABLE is now traversed by visit(JsonTableFunction); the result is the same and is covered by a test.

Two outputs change beyond the missing tables:

  • MATCH (..) AGAINST (..) columns are now visited like other qualified columns: findTablesInExpression("MATCH (t.a) AGAINST ('x')") returns [t] (before: []).
  • An attribute column is a field name, not a column reference, so it is not visited: findTablesInExpression("f(a).b.c") returns no table.

Why

A caller that checks a statement against a table allow-list with TablesNamesFinder never saw t2 in any of these; each returned only t1, or nothing:

SELECT string_agg(a, ',' ORDER BY (SELECT max(x) FROM t2)) FROM t1
SELECT a FROM t1 WHERE a LIKE 'a' ESCAPE (SELECT max(x) FROM t2)
SELECT now() AT TIME ZONE (SELECT max(x) FROM t2)
SELECT j -> (SELECT max(x) FROM t2) FROM t1                        -- Dialect.POSTGRESQL
SELECT max(a) KEEP (DENSE_RANK FIRST ORDER BY (SELECT x FROM t2)) FROM t1
SELECT a FROM t1 WHERE (SELECT x FROM t2) IS UNKNOWN
SELECT * FROM XMLTABLE('/r' PASSING (SELECT x FROM t2) COLUMNS a int PATH 'a') x

Testing

  • TablesNamesFinderTraversalTest:

    • nestedTables: 15 new cases, one per skipped child (KEEP, HAVING, LIMIT, SEPARATOR, GROUP_CONCAT ... ORDER BY, attribute function, IS UNKNOWN, MATCH ... AGAINST, the JSON aggregates, XMLTABLE), and 2 controls for JSON_TABLE and a plain table function;
    • new findsTablesInPostgresExpressionChildren, parsed with Dialect.POSTGRESQL, checking getTables and getTablesOrOtherSources:
      • 8 cases for aggregate ORDER BY, ESCAPE, AT TIME ZONE and the four JSON operators;
      • 3 cases for array subscript and slice bounds;
      • 25 controls for positions that were already visited: function arguments, FILTER, WITHIN GROUP, window PARTITION BY / ORDER BY / frame, LIMIT / OFFSET / FETCH, ANY / ALL / SOME, ARRAY(..), VALUES, substring / trim / position / overlay, CASE, casts, GROUP BY / GROUPING SETS, DISTINCT ON, LATERAL;
    • expressionTables: 4 new cases through findTablesInExpression, and the f(a).b.c control;
    • visitsAggregateChildrenOnce: no table is visited twice.

    On base bdf8ac31 the 28 cases for skipped children fail; the 3 array cases (fixed by fix: traverse missing query and DML children in TablesNamesFinder #2749) and all controls pass.

  • ./gradlew check on Windows 11 with the JDK 17 toolchain: 10,411 test cases, 0 failures, 0 errors, 25 skipped. Grammar ambiguity, Spotless, Checkstyle, PMD, SpotBugs and JaCoCo verification pass.

  • Not run: mvn verify, JMH (no grammar or parser change), Linux/macOS.

Not changed

  • DECLARE @x int = (SELECT ..) and PRAGMA x = (SELECT ..): their visits are empty on purpose (PRAGMA: "no tables involved").
  • JSON_OBJECTAGG(KEY (SELECT ..) VALUE b) does not parse, so only the VALUE position is tested.
  • visitBinaryExpression(BinaryExpression) still passes a null context to its children; it is public, so I left it alone.

Origin

First observed with com.manticore-projects.jsqlformatter:jsqlparser:5.4.127 and Dialect.POSTGRESQL. Reproduced on upstream master bdf8ac31bb17b2ae76400a8ddd22a39a990dcc89. The array slice upper bound, also missed in 5.4.127, is already fixed on master by #2749; this PR only adds regression tests for it. No existing issue is linked.

AI disclosure: this change was prepared with an AI coding agent (Claude Code) and reviewed by me.

- Visit aggregate ORDER BY, KEEP, HAVING, LIMIT, keyword arguments
  and attribute functions of Function and AnalyticExpression
- Visit LIKE ESCAPE, AT TIME ZONE zones, JSON operator paths,
  IS UNKNOWN and MATCH ... AGAINST operands
- Visit GROUP_CONCAT and JSON aggregate key, value, ORDER BY and OVER
- Dispatch table functions dynamically so XMLTABLE PASSING is visited

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant