Extract query uses into sql-grep-core - #540
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR extracts SQL query usage analysis from ztd-cli into a new package Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as ztd-cli (Client)
participant Core as sql-grep-core (Core)
participant FS as Filesystem
participant Parser as rawsql-ts (Parser)
CLI->>Core: buildQueryUsageReport(params)
Core->>FS: walkSqlCatalogSpecFiles(root)
FS-->>Core: spec file list
Core->>FS: loadSqlCatalogSpecsFromFile(path)
FS-->>Core: LoadedSqlCatalogSpec[]
Core->>FS: read SQL file(s)
FS-->>Core: sqlText
Core->>Core: buildCatalogStatements(catalogId, sqlFile, sqlText)
Core->>Parser: parseStatement(statementText)
Parser-->>Core: AST
Core->>Core: analyzeTableUsage(statement, target, mode)
Core->>Core: analyzeColumnUsage(statement, target, mode)
Core->>Core: locateUsageText(candidates, clauseAnchor)
Core-->>Core: QueryUsageMatch[] / warnings
Core->>Core: aggregateImpactMatches / formatQueryUsageReport
Core-->>CLI: QueryUsageReport or formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
packages/sql-grep-core/package.json (1)
13-19: Complex inline scripts work but are hard to maintain.The
prepackandreleasescripts use inline Node.js code which is functional but harder to read and maintain. Consider extracting to small script files if these grow more complex in the future. For now, this matches patterns used elsewhere in the monorepo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sql-grep-core/package.json` around lines 13 - 19, The inline Node.js logic in the "prepack" and "release" npm scripts is hard to maintain; extract each inline command into dedicated script files (e.g., scripts/prepack.js and scripts/release.js) that reproduce the exact behavior (prepack: check for dist/index.js and run build via platform-aware npm command; release: run lint/test/build, ensure tmp dir exists, run pnpm pack and pnpm publish) and update the package.json "prepack" and "release" entries to call node scripts/prepack.js and node scripts/release.js respectively so functionality is identical but code is easier to read and maintain.packages/sql-grep-core/src/utils/sqlCatalogDiscovery.ts (2)
43-51: Consider unshift instead of push for breadth-first ordering.The stack-based traversal with
push()andpop()processes directories in reverse alphabetical order. While the final sort at line 77 produces correct output, usingunshift()instead ofpush()(or switching to a queue) would process directories in natural order and could be more intuitive for debugging.This is a minor observation—the current implementation is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sql-grep-core/src/utils/sqlCatalogDiscovery.ts` around lines 43 - 51, The directory traversal uses a LIFO stack (stack.push + stack.pop) which causes directories to be visited in reverse alphabetical order; change the traversal to breadth-first by replacing stack.push(absolute) with stack.unshift(absolute) (or switch to a queue and use push + shift) in the loop that reads entries (the block using stack, readdirSync, entries, entry.isDirectory) so directories are processed in natural order; keep the final sort behavior unchanged.
150-196: Note: Regex-based extraction does not handle braces in strings or comments.The brace-depth counting in
extractTsJsSpecBlockscould be confused by{or}characters inside string literals or comments. For typical spec files this should not be an issue, but consider adding a note in the doc comment about this limitation if spec files might contain complex string literals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sql-grep-core/src/utils/sqlCatalogDiscovery.ts` around lines 150 - 196, The function extractTsJsSpecBlocks uses a simple brace-depth scan (starting from the last '{' before an id match and using idRegex) which can be thrown off by `{` or `}` inside string literals or comments; update the function's doc comment to explicitly state this limitation (that the parser is regex/brace-based and does not handle braces inside strings or comments), reference extractTsJsSpecBlocks and the idRegex/bracedepth logic so future maintainers know the exact area of concern, and suggest as a mitigation note that a proper JS/TS parser (e.g., TypeScript AST) should be used if spec files may contain complex string literals or comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sql-grep-core/package.json`:
- Around line 41-46: The CI failed because the pnpm lockfile is out of sync with
the updated packages/sql-grep-core/package.json; run pnpm install from the
repository root to regenerate pnpm-lock.yaml, verify the updated pnpm-lock.yaml
includes the change for "rawsql-ts" in packages/sql-grep-core/package.json, and
commit the updated pnpm-lock.yaml to the branch before pushing/merging.
In `@packages/sql-grep-core/src/query/analyzeColumnUsage.ts`:
- Around line 440-447: The current wildcard branch treats any namespace.* as a
hit when scope.targetTablePresent is true, causing qualified wildcards from
other relations to be counted; change the logic in the wildcard handling so that
when namespace is provided you only treat it as matching the target table if the
namespace equals the target table's name or alias from the scope (e.g. compare
namespace to scope.targetTableName or scope.targetTableAlias); if the namespace
does not match the target table, do not add a search term (or return []) so
other tables' qualified wildcards (like orders.*) are not attributed to the
target; keep the existing behavior for an unqualified '*' when the target table
is present.
- Around line 151-170: The UPDATE/DELETE handling builds a scope but never
traverses nested FROM/USING sources or join sources, so subquery sources (e.g.
parsed.fromClause.source, join.source, parsed.usingClause) are never visited;
fix by mirroring the SELECT branch: after building scope (buildScope), call
collectSourceMatches on parsed.fromClause.source (if present) and on each
parsed.usingClause item, and ensure collectJoinMatches is invoked for each join
(and that collectJoinMatches itself traverses join.source); also ensure
collectSelectItemMatches/collectExpressionMatches are called for any source
items like select-list in subqueries so subquery columns are discovered. Use the
existing helpers collectSourceMatches, collectJoinMatches,
collectExpressionMatches, collectSelectItemMatches and the parsed.fromClause,
parsed.usingClause, parsed.fromClause.joins symbols to implement this traversal.
In `@packages/sql-grep-core/src/query/analyzeTableUsage.ts`:
- Around line 93-119: The SELECT branch currently skips
parsed.selectClause.items so scalar subqueries in projections are ignored;
iterate parsed.selectClause.items and for each item call
collectExpressionQueryOccurrences (e.g., pass item instanceof SelectItem ?
item.value : item, target, mode, context) to catch expression-level subqueries,
and if an item can embed a query/table node that needs deeper traversal also
call collectTableOccurrences on that subquery (using the same context and inCte
flags as used for withClause) so projected scalar subqueries and nested table
references are reported.
In `@packages/sql-grep-core/src/query/location.ts`:
- Around line 152-167: findClauseWindow currently chooses the first
cache.clauseMarkers entry whose keyword equals the anchorPattern, causing later
identical clauses to map to the same window; change the selection logic inside
findClauseWindow so it finds the clause marker that corresponds to the specific
clauseAnchor occurrence (not just the first match): filter cache.clauseMarkers
by marker.keyword === anchorPattern, then pick the marker whose start is the
greatest value <= clauseAnchor.position (or clauseAnchor.startToken/anchorStart
property on QueryUsageClauseAnchor) so you get the closest preceding matching
marker; if none found, fall back to the original behavior, then compute
nextClause and return the window as before.
- Around line 86-92: The clause marker extraction is missing standalone SELECT
and preserves raw whitespace so multi-word clauses like "ORDER\nBY" don't
normalize and fail lookups; update the regex used for clauseMarkers (the const
in this file that maps statementText.matchAll(...)) to include SELECT and to
normalize matched keywords by collapsing all internal whitespace to a single
space and uppercasing (e.g., convert match[0] -> match[0].replace(/\s+/g,'
').toUpperCase()) so resolveClauseAnchor() can reliably find anchors regardless
of formatting or newlines.
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 1697-1707: The tsconfig built by
buildLocalSourceTsconfigContents() sets allowImportingTsExtensions but still
inherits an emitting outDir from TSCONFIG_TEMPLATE which conflicts with
TypeScript's requirement; update buildLocalSourceTsconfigContents to set a
non-emitting config (e.g., compilerOptions.noEmit = true) or alternatively set
compilerOptions.emitDeclarationOnly = true or
compilerOptions.rewriteRelativeImportExtensions = true so the generated scaffold
won't emit JS and will pass type checking; modify the parsed.compilerOptions
object before assigning it back to parsed and returning the JSON.
---
Nitpick comments:
In `@packages/sql-grep-core/package.json`:
- Around line 13-19: The inline Node.js logic in the "prepack" and "release" npm
scripts is hard to maintain; extract each inline command into dedicated script
files (e.g., scripts/prepack.js and scripts/release.js) that reproduce the exact
behavior (prepack: check for dist/index.js and run build via platform-aware npm
command; release: run lint/test/build, ensure tmp dir exists, run pnpm pack and
pnpm publish) and update the package.json "prepack" and "release" entries to
call node scripts/prepack.js and node scripts/release.js respectively so
functionality is identical but code is easier to read and maintain.
In `@packages/sql-grep-core/src/utils/sqlCatalogDiscovery.ts`:
- Around line 43-51: The directory traversal uses a LIFO stack (stack.push +
stack.pop) which causes directories to be visited in reverse alphabetical order;
change the traversal to breadth-first by replacing stack.push(absolute) with
stack.unshift(absolute) (or switch to a queue and use push + shift) in the loop
that reads entries (the block using stack, readdirSync, entries,
entry.isDirectory) so directories are processed in natural order; keep the final
sort behavior unchanged.
- Around line 150-196: The function extractTsJsSpecBlocks uses a simple
brace-depth scan (starting from the last '{' before an id match and using
idRegex) which can be thrown off by `{` or `}` inside string literals or
comments; update the function's doc comment to explicitly state this limitation
(that the parser is regex/brace-based and does not handle braces inside strings
or comments), reference extractTsJsSpecBlocks and the idRegex/bracedepth logic
so future maintainers know the exact area of concern, and suggest as a
mitigation note that a proper JS/TS parser (e.g., TypeScript AST) should be used
if spec files may contain complex string literals or comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a000710a-2455-47de-8400-accaafc12e75
📒 Files selected for processing (28)
README.mddocs/guide/query-uses-impact-checks.mddocs/guide/query-uses-overview.mdpackages/sql-grep-core/README.mdpackages/sql-grep-core/package.jsonpackages/sql-grep-core/src/index.tspackages/sql-grep-core/src/query/analyzeColumnUsage.tspackages/sql-grep-core/src/query/analyzeTableUsage.tspackages/sql-grep-core/src/query/format.tspackages/sql-grep-core/src/query/location.tspackages/sql-grep-core/src/query/report.tspackages/sql-grep-core/src/query/targets.tspackages/sql-grep-core/src/query/types.tspackages/sql-grep-core/src/utils/queryFingerprint.tspackages/sql-grep-core/src/utils/sqlCatalogDiscovery.tspackages/sql-grep-core/src/utils/sqlCatalogStatements.tspackages/sql-grep-core/tsconfig.jsonpackages/sql-grep-core/vitest.config.tspackages/ztd-cli/README.mdpackages/ztd-cli/package.jsonpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/query/format.tspackages/ztd-cli/src/query/report.tspackages/ztd-cli/src/query/targets.tspackages/ztd-cli/templates/scripts/local-source-guard.mjspackages/ztd-cli/tsconfig.jsontsconfig.jsonvitest.config.ts
Comment on lines
+41
to
+46
| "dependencies": { | ||
| "rawsql-ts": "workspace:^" | ||
| }, | ||
| "devDependencies": { | ||
| "typescript": "^5.8.2", | ||
| "vitest": "^4.0.7" |
There was a problem hiding this comment.
Pipeline failure: lockfile needs update.
The CI pipeline failed because pnpm-lock.yaml is not synchronized with this new package.json. Run pnpm install at the repository root to regenerate the lockfile before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sql-grep-core/package.json` around lines 41 - 46, The CI failed
because the pnpm lockfile is out of sync with the updated
packages/sql-grep-core/package.json; run pnpm install from the repository root
to regenerate pnpm-lock.yaml, verify the updated pnpm-lock.yaml includes the
change for "rawsql-ts" in packages/sql-grep-core/package.json, and commit the
updated pnpm-lock.yaml to the branch before pushing/merging.
Comment on lines
+93
to
+119
| if (parsed instanceof SimpleSelectQuery) { | ||
| const matches: TableOccurrence[] = []; | ||
| if (parsed.withClause) { | ||
| for (const table of parsed.withClause.tables) { | ||
| matches.push(...collectTableOccurrences(table.query, target, mode, { inCte: true })); | ||
| } | ||
| } | ||
| if (parsed.fromClause) { | ||
| matches.push(...collectFromClauseOccurrences(parsed.fromClause, target, mode, context)); | ||
| } | ||
| if (parsed.whereClause) { | ||
| matches.push(...collectExpressionQueryOccurrences(parsed.whereClause.condition, target, mode, context)); | ||
| } | ||
| if (parsed.havingClause) { | ||
| matches.push(...collectExpressionQueryOccurrences(parsed.havingClause.condition, target, mode, context)); | ||
| } | ||
| if (parsed.groupByClause) { | ||
| for (const group of parsed.groupByClause.grouping) { | ||
| matches.push(...collectExpressionQueryOccurrences(group, target, mode, context)); | ||
| } | ||
| } | ||
| if (parsed.orderByClause) { | ||
| for (const order of parsed.orderByClause.order) { | ||
| matches.push(...collectExpressionQueryOccurrences(order instanceof OrderByItem ? order.value : order, target, mode, context)); | ||
| } | ||
| } | ||
| return matches; |
There was a problem hiding this comment.
Walk SELECT projections for nested table references.
The select branch never inspects parsed.selectClause.items, so scalar subqueries in projections are dropped. A statement like SELECT (SELECT 1 FROM public.users) currently reports no users usage, which creates false negatives in both detail and impact views.
Suggested fix
if (parsed.fromClause) {
matches.push(...collectFromClauseOccurrences(parsed.fromClause, target, mode, context));
}
+ for (const item of parsed.selectClause.items) {
+ matches.push(...collectExpressionQueryOccurrences(item.value, target, mode, context));
+ }
if (parsed.whereClause) {
matches.push(...collectExpressionQueryOccurrences(parsed.whereClause.condition, target, mode, context));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (parsed instanceof SimpleSelectQuery) { | |
| const matches: TableOccurrence[] = []; | |
| if (parsed.withClause) { | |
| for (const table of parsed.withClause.tables) { | |
| matches.push(...collectTableOccurrences(table.query, target, mode, { inCte: true })); | |
| } | |
| } | |
| if (parsed.fromClause) { | |
| matches.push(...collectFromClauseOccurrences(parsed.fromClause, target, mode, context)); | |
| } | |
| if (parsed.whereClause) { | |
| matches.push(...collectExpressionQueryOccurrences(parsed.whereClause.condition, target, mode, context)); | |
| } | |
| if (parsed.havingClause) { | |
| matches.push(...collectExpressionQueryOccurrences(parsed.havingClause.condition, target, mode, context)); | |
| } | |
| if (parsed.groupByClause) { | |
| for (const group of parsed.groupByClause.grouping) { | |
| matches.push(...collectExpressionQueryOccurrences(group, target, mode, context)); | |
| } | |
| } | |
| if (parsed.orderByClause) { | |
| for (const order of parsed.orderByClause.order) { | |
| matches.push(...collectExpressionQueryOccurrences(order instanceof OrderByItem ? order.value : order, target, mode, context)); | |
| } | |
| } | |
| return matches; | |
| if (parsed instanceof SimpleSelectQuery) { | |
| const matches: TableOccurrence[] = []; | |
| if (parsed.withClause) { | |
| for (const table of parsed.withClause.tables) { | |
| matches.push(...collectTableOccurrences(table.query, target, mode, { inCte: true })); | |
| } | |
| } | |
| if (parsed.fromClause) { | |
| matches.push(...collectFromClauseOccurrences(parsed.fromClause, target, mode, context)); | |
| } | |
| for (const item of parsed.selectClause.items) { | |
| matches.push(...collectExpressionQueryOccurrences(item.value, target, mode, context)); | |
| } | |
| if (parsed.whereClause) { | |
| matches.push(...collectExpressionQueryOccurrences(parsed.whereClause.condition, target, mode, context)); | |
| } | |
| if (parsed.havingClause) { | |
| matches.push(...collectExpressionQueryOccurrences(parsed.havingClause.condition, target, mode, context)); | |
| } | |
| if (parsed.groupByClause) { | |
| for (const group of parsed.groupByClause.grouping) { | |
| matches.push(...collectExpressionQueryOccurrences(group, target, mode, context)); | |
| } | |
| } | |
| if (parsed.orderByClause) { | |
| for (const order of parsed.orderByClause.order) { | |
| matches.push(...collectExpressionQueryOccurrences(order instanceof OrderByItem ? order.value : order, target, mode, context)); | |
| } | |
| } | |
| return matches; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sql-grep-core/src/query/analyzeTableUsage.ts` around lines 93 - 119,
The SELECT branch currently skips parsed.selectClause.items so scalar subqueries
in projections are ignored; iterate parsed.selectClause.items and for each item
call collectExpressionQueryOccurrences (e.g., pass item instanceof SelectItem ?
item.value : item, target, mode, context) to catch expression-level subqueries,
and if an item can embed a query/table node that needs deeper traversal also
call collectTableOccurrences on that subquery (using the same context and inCte
flags as used for withClause) so projected scalar subqueries and nested table
references are reported.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/sql-grep-core/src/query/location.ts (1)
198-204: Potential ReDoS with constructed regex patterns.The
escapeRegexfunction mitigates most injection concerns, but the combined pattern structure ((?:...)groups with\s*\.\s*separators and lookahead/lookbehind) could still exhibit super-linear matching time on crafted input with many repeated segments. For CLI batch processing of untrusted SQL, consider adding a length limit on candidate strings or using a non-backtracking match approach.Consider adding a safeguard
function buildCandidatePattern(candidate: string): RegExp { + // Guard against pathologically long candidates that could cause slow regex matching + if (candidate.length > 200) { + return new RegExp(`(?<![A-Za-z0-9_])${escapeRegex(candidate)}(?![A-Za-z0-9_])`, 'gi'); + } const parts = candidate.split('.'); const pattern = parts .map((part) => `(?:"${escapeRegex(part)}"|${escapeRegex(part)})`) .join('\\s*\\.\\s*'); return new RegExp(`(?<![A-Za-z0-9_])${pattern}(?![A-Za-z0-9_])`, 'gi'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sql-grep-core/src/query/location.ts` around lines 198 - 204, The buildCandidatePattern function can produce complex backtracking regexes from attacker-controlled candidate strings; to prevent ReDoS, validate and constrain input before constructing the regex: in buildCandidatePattern (and where candidates are sourced) enforce a maximum candidate length (e.g., 200 chars) and a maximum number of dot-separated parts (e.g., 10), reject or truncate candidates that exceed these limits, and only then call escapeRegex and build the pattern; alternatively, if truncation is used ensure it occurs on the original candidate string prior to splitting to avoid producing many repeated segments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/sql-grep-core/src/query/location.ts`:
- Around line 198-204: The buildCandidatePattern function can produce complex
backtracking regexes from attacker-controlled candidate strings; to prevent
ReDoS, validate and constrain input before constructing the regex: in
buildCandidatePattern (and where candidates are sourced) enforce a maximum
candidate length (e.g., 200 chars) and a maximum number of dot-separated parts
(e.g., 10), reject or truncate candidates that exceed these limits, and only
then call escapeRegex and build the pattern; alternatively, if truncation is
used ensure it occurs on the original candidate string prior to splitting to
avoid producing many repeated segments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42ea327f-bbe7-4bb6-9201-dd8fa834ebd9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/sql-grep-core/src/query/analyzeColumnUsage.tspackages/sql-grep-core/src/query/analyzeTableUsage.tspackages/sql-grep-core/src/query/location.tspackages/ztd-cli/src/commands/init.tspackages/ztd-cli/tests/queryUses.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ztd-cli/src/commands/init.ts
This was referenced Mar 26, 2026
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ztd query usesengine into a new reusable@rawsql-ts/sql-grep-corepackage@rawsql-ts/ztd-clibehavior intact by turning the existing query-uses modules into thin wrappersTesting
Notes
packages/ztd-cli/tests/ztdLint.test.tsfailures, so the commit was created with--no-verifyafter the focused checks above passedSummary by CodeRabbit
New Features
Documentation
Refactor
Tests