Add SSSQL list/remove authoring flow and reference docs#765
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds CLI inspection/removal for SSSQL ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as ztd-cli
participant Core as rawsql-ts.SSSQLFilterBuilder
participant FS as FileSystem
participant Diff as Diff/Formatter
User->>CLI: "ztd query sssql scaffold/remove/list <sqlFile> --flags"
CLI->>FS: read <sqlFile>
CLI->>Core: list/scaffoldBranch/remove/removeAll (parsed SQL, spec)
Core-->>CLI: branch info / rewritten SQL / errors
CLI->>Diff: format & create diff (if --preview)
Diff-->>CLI: unified diff / formatted SQL
CLI->>FS: write file (unless --preview)
CLI-->>User: text/json report (diff, changed/written, branches)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/core/tests/transformers/SSSQLFilterBuilder.test.ts (1)
148-226: Add removal coverage fornot-existsbranches.This file now scaffolds the new kind, but the removal assertions here still only exercise
exists. A regression inlist/remove/removeAllhandling fornot-existswould slip through.As per coding guidelines, "Unless the request explicitly says not to, behavior changes must add or update tests in the same change" and "When ambiguous, prefer the strongest executable test for the changed behavior."🧪 Example test to close the gap
+ it('removes not-exists branches idempotently', () => { + const builder = new SSSQLFilterBuilder(); + const scaffolded = builder.scaffoldBranch( + ` + SELECT p.product_id, p.product_name + FROM products p + `, + { + kind: 'not-exists', + parameterName: 'archived_name', + anchorColumns: ['products.product_id'], + query: ` + SELECT 1 + FROM archived_products ap + WHERE ap.product_id = $c0 + AND ap.product_name = :archived_name + ` + } + ); + + const removed = builder.remove(scaffolded, { parameterName: 'archived_name', kind: 'not-exists' }); + expect(normalizeSql(new SqlFormatter().format(removed).formattedSql)) + .toBe('select "p"."product_id", "p"."product_name" from "products" as "p"'); + + expect(normalizeSql(new SqlFormatter().format(builder.removeAll(scaffolded)).formattedSql)) + .toBe('select "p"."product_id", "p"."product_name" from "products" as "p"'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/transformers/SSSQLFilterBuilder.test.ts` around lines 148 - 226, Add coverage for the new "not-exists" branch kind in the SSSQLFilterBuilder tests: in the existing tests that scaffold an "exists" branch (using SSSQLFilterBuilder.scaffoldBranch), also scaffold a parallel branch with { kind: 'not-exists', parameterName: 'category_name', anchorColumns: [...], query: '...' } and update assertions for builder.list(...) to expect an entry with parameterName 'category_name' and kind 'not-exists'; then mirror the remove/removeAll assertions to call builder.remove(...) and builder.removeAll(...) for the not-exists branch and assert that the normalized SQL no longer contains the :category_name placeholder and that repeated remove is idempotent (same normalized SQL), and that removeAll yields the original base SELECT without the not-exists branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guide/ztd-cli-sssql-authoring.md`:
- Around line 51-54: The docs example uses the wrong flag name: replace the
incorrect --parameter flag in the example command ("ztd query sssql remove
src/sql/products/list_products.sql --parameter category_name --preview") with
the actual CLI flag --parameter-name so it matches the CLI implementation (use
--parameter-name category_name); update the sample line(s) accordingly to ensure
the example matches the defined flag.
In `@packages/core/src/transformers/SSSQLFilterBuilder.ts`:
- Around line 565-579: In scaffoldExistsBranch, add a fail-fast validation that
spec.anchorColumns is non-empty before resolving targets: check
spec.anchorColumns.length and if zero throw a clear Error (e.g., "SSSQL
EXISTS/NOT EXISTS scaffold requires at least one anchorColumn.") so you never
dereference targetQueries[0] or call targetQuery.appendWhere; update logic
around resolveTarget(root, anchorColumn), anchorTargets, and targetQueries
accordingly to assume at least one anchor column is present.
- Around line 297-331: In getScalarBranchDetails, avoid calling
normalizeScalarOperator(predicate.operator...) before verifying the binary
predicate shape and parameter/column positions; instead, first ensure predicate
is a BinaryExpression and that one side is a ParameterExpression matching
parameterName and the other is a ColumnReference (using the existing left/right
instanceof checks), then call normalizeScalarOperator and validate the operator;
if the operator is unrecognized or normalization would fail, return null so the
caller falls back to the generic "expression" handling rather than throwing;
reference getScalarBranchDetails, predicate.operator, BinaryExpression,
normalizeScalarOperator, ParameterExpression, and ColumnReference when making
the change.
In `@packages/ztd-cli/src/commands/query.ts`:
- Around line 925-948: The current logic never overwrites the input file unless
options.out is provided, but the intended behavior is that non-preview runs
should default to overwriting the original SQL file; update the flow so that
when options.out is falsy and preview is false you treat outputFile as the input
path (i.e. set outputFile = absoluteInputPath or write to absoluteInputPath),
then call writeFileSync with that resolved path and ensure the returned fields
(output_file and written) reflect this (update references to outputFile, created
diff via createTwoFilesPatch stays correct using outputFile ??
absoluteInputPath). Use the existing symbols outputFile, preview,
absoluteInputPath, writeFileSync and the returned written/output_file flags to
implement this single-path overwrite behavior.
- Around line 653-657: collectSupportedOptionalConditionBranches() returns only
parameterName but SSSQLFilterBuilder.refresh() expects keys to map to target
column names (and anchor info) so branches like --parameter q or EXISTS branches
(e.g., :category_name on p.product_id) cannot be re-anchored and throw “Could
not resolve SSSQL filter target ...”. Fix by changing the transform to pass a
map keyed by the filter's actual target/anchor metadata instead of just
parameterName: call collectSupportedOptionalConditionBranches (or extend it) to
include each branch's targetColumn and anchorContext and construct filters as
entries using that target/anchor identifier (preserving parameterName as
value/null), then call new SSSQLFilterBuilder().refresh(parsed, filters) so
refresh can resolve and re-anchor branches correctly (refer to
collectSupportedOptionalConditionBranches, SSSQLFilterBuilder.refresh,
parameterName, targetColumn, anchorContext).
---
Nitpick comments:
In `@packages/core/tests/transformers/SSSQLFilterBuilder.test.ts`:
- Around line 148-226: Add coverage for the new "not-exists" branch kind in the
SSSQLFilterBuilder tests: in the existing tests that scaffold an "exists" branch
(using SSSQLFilterBuilder.scaffoldBranch), also scaffold a parallel branch with
{ kind: 'not-exists', parameterName: 'category_name', anchorColumns: [...],
query: '...' } and update assertions for builder.list(...) to expect an entry
with parameterName 'category_name' and kind 'not-exists'; then mirror the
remove/removeAll assertions to call builder.remove(...) and
builder.removeAll(...) for the not-exists branch and assert that the normalized
SQL no longer contains the :category_name placeholder and that repeated remove
is idempotent (same normalized SQL), and that removeAll yields the original base
SELECT without the not-exists branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b7b8253-156d-4bca-af1e-4c9eee981bb4
📒 Files selected for processing (13)
.changeset/sssql-cli-reference-and-remove-all.mddocs/dogfooding/dynamic-filter-routing.mddocs/guide/dynamic-filter-routing.mddocs/guide/feature-index.mddocs/guide/sssql-optional-branch-pruning.mddocs/guide/ztd-cli-sssql-authoring.mddocs/guide/ztd-cli-sssql-reference.mdpackages/core/src/transformers/SSSQLFilterBuilder.tspackages/core/tests/transformers/PruneOptionalConditionBranches.test.tspackages/core/tests/transformers/SSSQLFilterBuilder.test.tspackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/query.tspackages/ztd-cli/tests/cliCommands.test.ts
Comment on lines
+51
to
+54
| ```bash | ||
| ztd query sssql list src/sql/products/list_products.sql | ||
| ztd query sssql remove src/sql/products/list_products.sql --parameter category_name --preview | ||
| ``` |
There was a problem hiding this comment.
Use the actual remove flag name in this example.
Line 53 uses --parameter, but packages/ztd-cli/src/commands/query.ts defines --parameter-name, so the sample command will fail as written.
📝 Suggested doc fix
ztd query sssql list src/sql/products/list_products.sql
-ztd query sssql remove src/sql/products/list_products.sql --parameter category_name --preview
+ztd query sssql remove src/sql/products/list_products.sql --parameter-name category_name --preview📝 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
| ```bash | |
| ztd query sssql list src/sql/products/list_products.sql | |
| ztd query sssql remove src/sql/products/list_products.sql --parameter category_name --preview | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guide/ztd-cli-sssql-authoring.md` around lines 51 - 54, The docs example
uses the wrong flag name: replace the incorrect --parameter flag in the example
command ("ztd query sssql remove src/sql/products/list_products.sql --parameter
category_name --preview") with the actual CLI flag --parameter-name so it
matches the CLI implementation (use --parameter-name category_name); update the
sample line(s) accordingly to ensure the example matches the defined flag.
Comment on lines
+653
to
+657
| transform: (sql) => { | ||
| const parsed = SelectQueryParser.parse(sql); | ||
| const existingBranches = collectSupportedOptionalConditionBranches(parsed); | ||
| const filters = Object.fromEntries(existingBranches.map((branch) => [branch.parameterName, null])); | ||
| return new SSSQLFilterBuilder().refresh(parsed, filters); |
There was a problem hiding this comment.
Refresh is keyed by parameter names, so structured branches stop being refreshable.
collectSupportedOptionalConditionBranches() only gives you parameterName, but SSSQLFilterBuilder.refresh() resolves each key as a target column name. That works for :status -> status, but it breaks as soon as the scaffold uses --parameter q or an EXISTS branch like :category_name anchored on p.product_id; those now fail with “Could not resolve SSSQL filter target ...” instead of re-anchoring the branch. refresh needs target/anchor metadata here, not just parameterName.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ztd-cli/src/commands/query.ts` around lines 653 - 657,
collectSupportedOptionalConditionBranches() returns only parameterName but
SSSQLFilterBuilder.refresh() expects keys to map to target column names (and
anchor info) so branches like --parameter q or EXISTS branches (e.g.,
:category_name on p.product_id) cannot be re-anchored and throw “Could not
resolve SSSQL filter target ...”. Fix by changing the transform to pass a map
keyed by the filter's actual target/anchor metadata instead of just
parameterName: call collectSupportedOptionalConditionBranches (or extend it) to
include each branch's targetColumn and anchorContext and construct filters as
entries using that target/anchor identifier (preserving parameterName as
value/null), then call new SSSQLFilterBuilder().refresh(parsed, filters) so
refresh can resolve and re-anchor branches correctly (refer to
collectSupportedOptionalConditionBranches, SSSQLFilterBuilder.refresh,
parameterName, targetColumn, anchorContext).
mk3008
deleted the
764-sssql-cli拡張-removelist追加existsnot-exists-scaffold対応安全なidempotent-authoring
branch
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.
Issue
Customer Value
Outcome
ztd query sssql listto report recognized SSSQL branches in text and JSON forms.ztd query sssql scaffoldsupport for scalar operators plusEXISTS/NOT EXISTSauthoring with preview mode, idempotency, and fail-fast validation.ztd query sssql removefor single-branch removal andztd query sssql remove --allfor bulk removal of all recognized branches.refreshfocused on re-anchoring existing recognized scalar branches without inventing new runtime predicates.Acceptance Criteria
ztd query sssql list <sqlFile>reports recognized authored branches in machine-readable and human-readable forms.ztd query sssql remove <sqlFile>removes exactly one targeted recognized branch safely, supports preview, and stays idempotent when the target is already absent.ztd query sssql remove <sqlFile> --allremoves every recognized branch explicitly and rejects mixed targeted flags.ztd query sssql scaffold <sqlFile>supports richer scalar operators and structuredEXISTS/NOT EXISTSauthoring.optionalConditionParametersand supports authored scalar,EXISTS, andNOT EXISTSbranches.Verification
pnpm exec vitest run packages/core/tests/transformers/SSSQLFilterBuilder.test.tspnpm exec vitest run packages/ztd-cli/tests/cliCommands.test.ts -t "query sssql"pnpm --filter @rawsql-ts/ztd-cli buildRepository Evidence
packages/core/src/transformers/SSSQLFilterBuilder.tspackages/core/tests/transformers/SSSQLFilterBuilder.test.tspackages/ztd-cli/src/commands/query.tspackages/ztd-cli/tests/cliCommands.test.tsdocs/guide/ztd-cli-sssql-reference.md,docs/guide/ztd-cli-sssql-authoring.md,packages/ztd-cli/README.md.changeset/sssql-cli-reference-and-remove-all.mdSupplementary Evidence
pg_dumpis not available in PATH.Open Questions
refreshimplementation still keys off recognized scalar branch targets. I did not extend it to infer and move correlatedEXISTS/NOT EXISTSbranches because that behavior is not yet safely proven.Merge Blockers
rawsql-tsand@rawsql-ts/ztd-cli.Merge Readiness
CLI Surface Migration
Upgrade note:
ztd query sssqlnow includeslist, structuredscaffold, targetedremove, andremove --all; non-preview rewrites overwrite the source SQL file by default unless--outis supplied.Deprecation/removal plan or issue: No deprecation is introduced in this PR.
Docs/help/examples updated: Added
docs/guide/ztd-cli-sssql-reference.mdand updateddocs/guide/ztd-cli-sssql-authoring.mdpluspackages/ztd-cli/README.md.Release/changeset wording: Covered by
.changeset/sssql-cli-reference-and-remove-all.md.Scaffold Contract Proof
Non-edit assertion:
query sssql listinspects recognized branches without mutating the SQL file.Fail-fast input-contract proof: Added coverage for empty
anchorColumnsand preserved fail-fast handling for unsupported scalar operator parsing.Generated-output viability proof: Focused CLI tests verify preview/apply flows, overwrite-by-default behavior, branch removal, and bulk removal.