feat(sql-editor): add type-aware INSERT value hints#1933
Conversation
实现说明与截图补充区这次改动只针对带明确字段列表的 INSERT: INSERT INTO demo (id, enabled, name, birthday, created_at) VALUES (输入 每个 value 后面的
后端分层:
|
Chat2DB-Pro
left a comment
There was a problem hiding this comment.
Thanks for the feature — the overall direction is good: injected-text labels that never enter executable SQL, solid async race guards (modelVersionId + requestId + epoch), undo grouping so one Ctrl+Z reverts the whole auto-fill, and the conservative explicit-column-list requirement are all the right calls. I verified the branch locally; requesting changes for the items below, ranked by severity.
1. Two existing contract tests fail on this branch (verified locally)
MysqlSqlCompletionScenarioCoverageTest has two contract cases asserting that the INSERT/REPLACE VALUES expression slot returns no completion candidates (expected: <EMPTY> but was: <SUCCESS>):
insertValuesExpressionSlotDoesNotUseInsertTargetColumnsreplaceValuesExpressionSlotDoesNotUseReplaceTargetColumns
On main all 176 tests in that class pass; on this branch these two fail (full mysql module run: Tests run: 607, Failures: 2). The PR's verification list only ran MysqlSqlCompletionProviderTest (163 tests) and missed this suite in the same module, so the "0 failures" claim doesn't hold. If the behavior change is intentional, please update the contract tests and call it out in the description; otherwise adjust MysqlSqlCompletionValueCandidateProvider.
2. Multi-statement documents: the regex binds to the FIRST insert, and auto-fill silently stops working
StandardSqlInsertEditorHintProvider / PostgreSqlEditorHintProvider run matcher.find() over everything before the cursor, so the first INSERT INTO ... VALUES ( in the document wins — but the frontend sends the whole editor content. With any completed INSERT above the cursor (the most common workflow: writing several inserts in one console), the hint is computed against the wrong table and group(3) spans multiple statements. Knock-on effect: the value ranges are non-empty, so the frontend isEmptyRange guard rejects the auto-fill — auto-fill silently never fires whenever any complete INSERT precedes the current one. Suggest anchoring to the last unterminated INSERT before the cursor (or slicing at statement boundaries first). Related: after VALUES (1, 2); is closed, (.*)$ still matches and produces a hint — the provider should return empty once the VALUES parenthesis is balanced.
3. Hint stickiness: auto-fill effectively works once per document
findValuesRow (sqlInsertValueDefaults.ts, fallback branch) returns the ranges of the first VALUES row in the document whenever the exact-value match fails, and scanValuesRow never returns null. So once an auto-fill happened, rematerializeInsertValueHints stays non-empty as long as any values( text exists anywhere, and handleValueChange early-returns before the 'content'-sourced refreshBackendParameterHints — the only path that performs auto-fill. Consequences: typing VALUES ( in a second INSERT never auto-fills again, and after the user edits a value the labels can jump to the first (possibly unrelated) VALUES row in the document. Suggest clearing the hints when the exact match fails instead of falling back to the first row.
4. Default values are executable SQL — a few dialect mappings look wrong
- Informix does not accept
CURRENT_TIMESTAMP(it usesCURRENT/CURRENT YEAR TO SECOND), so the auto-filled row fails on execution. - TDengine conventionally uses
NOW. ZERO_BOOLEAN_DIALECTScontains both"INFORMIX"and"INFOMIX"— the duplicate spelling suggests the actual dbType enum values weren't verified. Please cross-check againstDataSourceTypeEnum, or narrow the supported list to dialects whose defaults were actually validated.
5. Smaller items
ISqlSyntaxPlugin.getSqlEditorHintsdefaults to the standard provider for every dialect (including Redis/Mongo), relying on the regex not matching — this contradicts the description ("frontend enables for listed dialects"). Suggest defaulting to empty and opting in explicitly.PostgreSqlEditorHintProviderduplicates ~300 lines of the standard provider with only quoting/2-part-name differences; consider parameterizing instead.- Backslash escapes aren't handled in the literal scanners: MariaDB/TiDB default to backslash escaping, so
'it\'s'desyncs the quote state machine on both backend and frontend (labels misplace; cosmetic but visible). insideLiteralOrCommenttracksdoubleQuotedbut only returnssingleQuoted.
Items 1 and 2 are blocking; 3 is strongly recommended; 4 needs at least the Informix/TDengine check or a narrower dialect list. With those addressed this is a nice feature.
|
已根据 review 完成修复,提交: 主要调整:
实际验证:
|
|
补充修复 PostgreSQL 多条 INSERT 场景:此前每次自动填充都会用最新一条 INSERT 的 label 集合覆盖已有集合,因此同一编辑器中仅最后一条保留提示。现在按 VALUES 行累积并重新定位各组 label,后续 INSERT 不再清除前面语句的提示;编辑既有值时也会保留全部提示组。新增连续 3 条 INSERT 回归覆盖。验证通过:test:sql-in-clipboard、ESLint、Stylelint、build:web:community。提交:12e1dfbc。 |
|
已完成本轮多 INSERT 回归修复并更新 PR:提交 12e1dfb 修复后续 INSERT 覆盖前序 label;提交 3fd9061 修复美化 SQL 后多组 label 同时落到第一条 VALUES 的问题。新逻辑会保留每个 INSERT 的独立提示组,并基于格式化前后的 VALUES 序号一一重新定位。自动回归覆盖连续 3 条 INSERT、修改前序 value、三条 SQL 美化为多行后的 label 位置;test:sql-in-clipboard、ESLint、Stylelint、build:web:community 和 git diff --check 均通过。PostgreSQL 页面人工回归已确认多条提示、value 编辑和 SQL 美化场景正常。
|
openai0229
left a comment
There was a problem hiding this comment.
The blocking issue is the validation-to-scope mismatch. This PR changes executable INSERT defaults across roughly 30 dialects and 55 files, while the current evidence covers MySQL automated tests and PostgreSQL manual validation only. A wrong default here changes SQL that users may execute; it is not only presentation metadata.
Please either narrow the first PR to the dialects that are actually validated (preferably MySQL and PostgreSQL in focused changes), or add executable SQL coverage for every enabled dialect/default family. Please also link the product/behavior contract that defines which defaults are safe to auto-fill, then update the branch with main and run the full required CI suite.
Keep INSERT value hints scoped to MySQL and PostgreSQL, stabilize injected labels during editing, and format PostgreSQL INSERT values vertically.
|
Updated as requested and narrowed the scope to the database dialects we have actually validated. Scope
Editor UX improvements
Verification
|
# Conflicts: # chat2db-community-client/package.json
Preserve quoted identifier semantics, handle dollar-quoted values, and keep PostgreSQL formatting changes scoped to INSERT scripts. Replace metadata-type regex matching with deterministic scans so CodeQL cannot hit polynomial backtracking.
|
已完成合并前的最终收敛,并针对现有 review / CodeQL 再做了一轮完整检查。 最终 head: 本轮补强
验证证据
PR body 已同步为当前实际范围,并补上了明确的行为契约与对应的可执行测试链接,方便 reviewer 直接核对。 |
|
Thanks for the extensive revisions and verification already completed on this feature. During the final maintainer review, we found one remaining reproducible edge case: when a real INSERT was preceded by text such as We pushed maintainer commit Verification:
The new regression tests cover both commented and dollar-quoted fake VALUES rows, fail-closed auto-fill inside comments/strings, and PostgreSQL comment delimiters. We will wait for the refreshed full CI and Java CodeQL before the final merge review. |
openai0229
left a comment
There was a problem hiding this comment.
Reviewed the latest fixes at c43ecd9. INSERT value-hint rematerialization now ignores strings and line/block comments, PostgreSQL comment delimiters are handled, the focused frontend and PostgreSQL tests pass, and the current head passes the required checks plus Java CodeQL. The previously identified blockers are resolved.
Addressed by subsequent commits: 7b39538 resolved the initial review feedback, 12e1dfb fixed multi-INSERT behavior, 67265a7 narrowed the feature to validated dialects, 0c50ac5 hardened PostgreSQL handling, and c43ecd9 fixed comment/string positioning with regression coverage. The latest head passes focused tests, required checks, and Java CodeQL.



Summary
INSERT ... VALUES (statements.field:typelabels with Monaco injected text; labels are presentation-only and never enter executable SQL.Supported behavior contract
This PR deliberately uses a narrow, fail-closed contract:
Supported databases: MySQL and PostgreSQL only.
Supported statement:
INSERTonly;UPDATEand mixed/non-INSERT formatting retain existing behavior.Required shape: an explicit column list, for example:
INSERT INTO demo VALUES (is not inferred because the SQL text does not establish a safe column mapping.Every SQL column must resolve against database metadata. If it does not, no row is auto-filled.
Known scalar types receive dialect-specific executable defaults; unknown or unsafe-to-guess types use
NULL.PostgreSQL quoted identifiers keep exact case, while unquoted identifiers use PostgreSQL lowercase folding.
PostgreSQL single quotes, double quotes, comments, and dollar-quoted strings are excluded from statement/value delimiter handling.
The executable defaults and labels are separate response fields; injected labels never modify, copy with, format into, or execute as SQL.
The executable contract is covered directly by:
MysqlSqlCompletionProviderTestPostgreSqlInsertEditorHintProviderTestDbSqlServiceImplTestsqlInsertValueDefaults.test.tsReview feedback addressed
byteaexpression to'\x'::byteaafter execution against PostgreSQL 16.Verification
Local verification on the latest
main:MysqlSqlCompletionScenarioCoverageTest: 176/176).yarn run lint: passed.yarn run build:web:community --app_version=0.0.0: passed.clean package: passed with-Dmaven.test.skip=true; this is a packaging check, not a test run.bytea.git diff --check: passed.Screenshots
UI screenshots and the prior manual regression notes are available in the PR comments.