From 5b985b670d90f99dcad577c8877f885212bc94a6 Mon Sep 17 00:00:00 2001 From: zipg Date: Wed, 15 Jul 2026 20:11:04 +0800 Subject: [PATCH] fix(query): match primary keys by dialect-aware casing --- apps/desktop/src/lib/sql/sqlAnalysis.ts | 62 +++- apps/desktop/src/stores/queryStore.ts | 70 ++-- crates/dbx-core/src/data_grid_sql.rs | 300 +++++++++++++++++- crates/dbx-core/src/data_grid_tdengine_sql.rs | 6 +- packages/app-tests/queryStore.test.ts | 84 +++++ packages/app-tests/sqlAnalysis.test.ts | 31 +- 6 files changed, 502 insertions(+), 51 deletions(-) diff --git a/apps/desktop/src/lib/sql/sqlAnalysis.ts b/apps/desktop/src/lib/sql/sqlAnalysis.ts index f7d19e227..8ac164e4e 100644 --- a/apps/desktop/src/lib/sql/sqlAnalysis.ts +++ b/apps/desktop/src/lib/sql/sqlAnalysis.ts @@ -48,6 +48,44 @@ export interface EditableQuerySource { alias?: string; } +const POSTGRES_FOLDED_IDENTIFIER_TYPES = new Set(["postgres", "redshift", "gaussdb", "highgo", "vastbase", "kwdb", "opengauss", "questdb"]); +const ORACLE_FOLDED_IDENTIFIER_TYPES = new Set(["oracle", "dameng", "oceanbase-oracle"]); + +/** + * Resolve a SQL source identifier to the canonical name returned by table + * metadata. Quoted identifiers are always exact. PostgreSQL-compatible + * unquoted identifiers fold to lower case, while Oracle-compatible identifiers + * fold to upper case. Other dialects may use a case-insensitive match only when + * it identifies exactly one physical column. + */ +export function resolveMetadataColumnName(databaseType: string, sourceName: string, sourceNameQuoted: boolean | undefined, metadataColumns: readonly string[]): string | undefined { + if (sourceNameQuoted) return metadataColumns.find((column) => column === sourceName); + + // Result-set labels are already database-resolved names rather than SQL + // source tokens. Preserve exact PostgreSQL/Oracle casing instead of folding + // a physical quoted column such as `"ID"` to `id`. + if (sourceNameQuoted === undefined) { + const exact = metadataColumns.find((column) => column === sourceName); + if (exact || POSTGRES_FOLDED_IDENTIFIER_TYPES.has(databaseType) || ORACLE_FOLDED_IDENTIFIER_TYPES.has(databaseType)) return exact; + const caseOnlyMatches = metadataColumns.filter((column) => column.toLowerCase() === sourceName.toLowerCase()); + return caseOnlyMatches.length === 1 ? caseOnlyMatches[0] : undefined; + } + + if (POSTGRES_FOLDED_IDENTIFIER_TYPES.has(databaseType)) { + const folded = sourceName.toLowerCase(); + return metadataColumns.find((column) => column === folded); + } + if (ORACLE_FOLDED_IDENTIFIER_TYPES.has(databaseType)) { + const folded = sourceName.toUpperCase(); + return metadataColumns.find((column) => column === folded); + } + + const exact = metadataColumns.find((column) => column === sourceName); + if (exact) return exact; + const caseOnlyMatches = metadataColumns.filter((column) => column.toLowerCase() === sourceName.toLowerCase()); + return caseOnlyMatches.length === 1 ? caseOnlyMatches[0] : undefined; +} + export type QueryEditabilityReason = "not-select" | "cte" | "set-operation" | "aggregation" | "external-source" | "complex-source" | "computed-columns" | "no-table" | "no-primary-key" | "primary-key-not-returned" | "aliased-columns" | "metadata-unavailable"; export type QueryEditability = { editable: true; analysis: EditableQueryInfo } | { editable: false; reason: QueryEditabilityReason }; @@ -451,7 +489,9 @@ function escapeRegExp(value: string): string { /** * Check if all primary key columns are present in the result set columns. - * Comparison is case-insensitive. + * Source names must already be resolved to canonical metadata names. Exact + * comparison prevents `id` from being mistaken for a distinct quoted `"ID"` + * column in PostgreSQL. */ export function allPrimaryKeysPresent(primaryKeys: string[], resultColumns: string[], analysis?: EditableQueryInfo, sourceKey?: string): boolean { if (analysis && !analysis.selectStar) { @@ -459,26 +499,24 @@ export function allPrimaryKeysPresent(primaryKeys: string[], resultColumns: stri analysis.columns.flatMap((column) => { if (!column.sourceName) return []; if (sourceKey && column.sourceKey !== sourceKey) return []; - return [column.sourceName.toLowerCase()]; + return [column.sourceName]; }), ); - return primaryKeys.every((pk) => sourceColumns.has(pk.toLowerCase())); + return primaryKeys.every((pk) => sourceColumns.has(pk)); } - const colSet = new Set(resultColumns.map((c) => c.toLowerCase())); - return primaryKeys.every((pk) => colSet.has(pk.toLowerCase())); + const colSet = new Set(resultColumns); + return primaryKeys.every((pk) => colSet.has(pk)); } function matchColumnsForResult(analysis: EditableQueryInfo, resultColumns: string[]): EditableQueryColumn[] | undefined { const matches: EditableQueryColumn[] = []; let searchFrom = 0; for (const resultColumn of resultColumns) { - const normalized = resultColumn.toLowerCase(); - let matchIndex = -1; - for (let index = searchFrom; index < analysis.columns.length; index++) { - if (analysis.columns[index]!.resultName.toLowerCase() === normalized) { - matchIndex = index; - break; - } + let matchIndex = analysis.columns.findIndex((column, index) => index >= searchFrom && column.resultName === resultColumn); + if (matchIndex < 0) { + const normalized = resultColumn.toLowerCase(); + const caseOnlyMatches = analysis.columns.flatMap((column, index) => (index >= searchFrom && column.resultName.toLowerCase() === normalized ? [index] : [])); + if (caseOnlyMatches.length === 1) matchIndex = caseOnlyMatches[0]!; } if (matchIndex < 0) return undefined; matches.push(analysis.columns[matchIndex]!); diff --git a/apps/desktop/src/stores/queryStore.ts b/apps/desktop/src/stores/queryStore.ts index 1f9ef4263..70e68844f 100644 --- a/apps/desktop/src/stores/queryStore.ts +++ b/apps/desktop/src/stores/queryStore.ts @@ -6,7 +6,7 @@ import type { ConnectionConfig, DatabaseType, ObjectBrowserViewport, QueryResult import { orderPinnedFirst } from "@/lib/app/pinnedItems"; import { canCancelQueryExecution } from "@/lib/sql/queryExecutionState"; import { buildExplainSql, parseExplainResult, parseDamengExplainText, parseOracleExplainText, type BuildExplainSqlResult } from "@/lib/diagram/explainPlan"; -import { allEditableColumnsWriteable, allPrimaryKeysPresent, analyzeEditableQueryEditability, sourceColumnsForResult, type EditableQueryInfo, type EditableQuerySource } from "@/lib/sql/sqlAnalysis"; +import { allEditableColumnsWriteable, allPrimaryKeysPresent, analyzeEditableQueryEditability, resolveMetadataColumnName, sourceColumnsForResult, type EditableQueryInfo, type EditableQuerySource } from "@/lib/sql/sqlAnalysis"; import { buildQueryWithHiddenPrimaryKeys, hiddenResultColumnIndexes, type HiddenPrimaryKeyProjection } from "@/lib/sql/editableQueryHiddenKeys"; import { ACTIVE_TAB_STORAGE_KEY, OPEN_TABS_STORAGE_KEY, restoreOpenTabsPayload, restoreOpenTabsState, serializeOpenTabs } from "@/lib/app/openTabsPersistence"; import { @@ -250,24 +250,52 @@ function cloneAnalysisForSource(analysis: EditableQueryInfo, source: EditableQue }; } -function sourceMatchesColumn(columnName: string, tableColumns: readonly { name: string }[]): boolean { - const normalizedColumn = columnName.toLowerCase(); - return tableColumns.some((column) => column.name.toLowerCase() === normalizedColumn); +function resolveSourceColumnName(dbType: string, columnName: string, quoted: boolean | undefined, tableColumns: readonly { name: string }[]): string | undefined { + return resolveMetadataColumnName( + dbType, + columnName, + quoted, + tableColumns.map((column) => column.name), + ); } -function bindUnqualifiedColumnsForSource(analysis: EditableQueryInfo, source: EditableQuerySource, tableColumns: readonly { name: string }[], allSourceColumns: Array<{ source: EditableQuerySource; columns: readonly { name: string }[] }> = [{ source, columns: tableColumns }]): EditableQueryInfo { +function bindColumnsForSource( + dbType: string, + analysis: EditableQueryInfo, + source: EditableQuerySource, + tableColumns: readonly { name: string }[], + allSourceColumns: Array<{ source: EditableQuerySource; columns: readonly { name: string }[] }> = [{ source, columns: tableColumns }], +): EditableQueryInfo { return { ...analysis, columns: analysis.columns.map((column) => { - if (!column.sourceName || column.sourceKey || column.sourceQualifier) return column; - if (!sourceMatchesColumn(column.sourceName, tableColumns)) return column; - const matchingSources = allSourceColumns.filter((entry) => sourceMatchesColumn(column.sourceName!, entry.columns)); + if (!column.sourceName) return column; + if (column.sourceKey) { + if (column.sourceKey !== source.key) return column; + const canonicalName = resolveSourceColumnName(dbType, column.sourceName, column.sourceNameQuoted, tableColumns); + return { ...column, sourceName: canonicalName }; + } + if (column.sourceQualifier) return column; + const matchingSources = allSourceColumns.flatMap((entry) => { + const canonicalName = resolveSourceColumnName(dbType, column.sourceName!, column.sourceNameQuoted, entry.columns); + return canonicalName ? [{ source: entry.source, canonicalName }] : []; + }); if (matchingSources.length !== 1 || matchingSources[0]?.source.key !== source.key) return column; - return { ...column, sourceKey: source.key }; + return { ...column, sourceName: matchingSources[0].canonicalName, sourceKey: source.key }; }), }; } +function primaryKeysPresentForSource(dbType: string, primaryKeys: string[], resultColumns: string[], analysis: EditableQueryInfo, sourceKey: string, tableColumns: readonly { name: string }[]): boolean { + if (!analysis.selectStar) return allPrimaryKeysPresent(primaryKeys, resultColumns, analysis, sourceKey); + const metadataNames = tableColumns.map((column) => column.name); + const canonicalResultColumns = resultColumns.flatMap((column) => { + const canonicalName = resolveMetadataColumnName(dbType, column, undefined, metadataNames); + return canonicalName ? [canonicalName] : []; + }); + return allPrimaryKeysPresent(primaryKeys, canonicalResultColumns); +} + function expandStarProjectionColumnsForSource(analysis: EditableQueryInfo, source: EditableQuerySource, tableColumns: readonly { name: string }[]): EditableQueryInfo { if (analysis.selectStar || !analysis.columns.some((column) => column.star)) return analysis; return { @@ -2252,8 +2280,8 @@ export const useQueryStore = defineStore("query", () => { function missingPrimaryKeysForSource(primaryKeys: string[], analysis: EditableQueryInfo, sourceKey: string): string[] { if (analysis.selectStar) return []; - const selectedColumns = new Set(analysis.columns.flatMap((column) => (column.sourceName && column.sourceKey === sourceKey ? [column.sourceName.toLowerCase()] : []))); - return primaryKeys.filter((primaryKey) => !selectedColumns.has(primaryKey.toLowerCase())); + const selectedColumns = new Set(analysis.columns.flatMap((column) => (column.sourceName && column.sourceKey === sourceKey ? [column.sourceName] : []))); + return primaryKeys.filter((primaryKey) => !selectedColumns.has(primaryKey)); } async function oracleRowIdIsSafeForQuery(tab: QueryTab, loaded: LoadedEditableSource): Promise { @@ -2277,7 +2305,7 @@ export const useQueryStore = defineStore("query", () => { const loaded = await loadEditableQuerySource(tab, analysis, sources[0]!, conn, databaseType, traceId, elapsed); if (loaded.tableMeta.tableType?.toUpperCase().includes("VIEW")) return unchanged; - const metadataAnalysis = expandStarProjectionColumnsForSource(bindUnqualifiedColumnsForSource(loaded.analysis, loaded.source, loaded.tableMeta.columns), loaded.source, loaded.tableMeta.columns); + const metadataAnalysis = expandStarProjectionColumnsForSource(bindColumnsForSource(databaseType, loaded.analysis, loaded.source, loaded.tableMeta.columns), loaded.source, loaded.tableMeta.columns); const declaredPrimaryKeys = loaded.tableMeta.columns.filter((column) => column.is_primary_key).map((column) => column.name); // Oracle base tables without declared keys use the same ROWID identity as // table-data tabs. Confirm the object is a base table because selecting @@ -2287,10 +2315,8 @@ export const useQueryStore = defineStore("query", () => { const missingPrimaryKeys = declaredPrimaryKeys.length === 0 ? primaryKeys : missingPrimaryKeysForSource(primaryKeys, metadataAnalysis, loaded.source.key); if (missingPrimaryKeys.length === 0) return unchanged; - const primaryKeySet = new Set(primaryKeys.map((primaryKey) => primaryKey.toLowerCase())); - const hasWritableProjection = metadataAnalysis.selectStar - ? loaded.tableMeta.columns.some((column) => !primaryKeySet.has(column.name.toLowerCase())) - : metadataAnalysis.columns.some((column) => column.sourceName && column.sourceKey === loaded.source.key && !primaryKeySet.has(column.sourceName.toLowerCase())); + const primaryKeySet = new Set(primaryKeys); + const hasWritableProjection = metadataAnalysis.selectStar ? loaded.tableMeta.columns.some((column) => !primaryKeySet.has(column.name)) : metadataAnalysis.columns.some((column) => column.sourceName && column.sourceKey === loaded.source.key && !primaryKeySet.has(column.sourceName)); if (!hasWritableProjection) return unchanged; const rewritten = buildQueryWithHiddenPrimaryKeys({ @@ -2369,13 +2395,13 @@ export const useQueryStore = defineStore("query", () => { // source table has a complete row identifier and at least one writable column. const candidates = loadedSources .map((loaded) => { - const metadataAnalysis = expandStarProjectionColumnsForSource(bindUnqualifiedColumnsForSource(loaded.analysis, loaded.source, loaded.tableMeta.columns, allSourceColumns), loaded.source, loaded.tableMeta.columns); + const metadataAnalysis = expandStarProjectionColumnsForSource(bindColumnsForSource(dbType, loaded.analysis, loaded.source, loaded.tableMeta.columns, allSourceColumns), loaded.source, loaded.tableMeta.columns); const primaryKeys = loaded.tableMeta.primaryKeys; const sourceColumns = sourceColumnsForResult(metadataAnalysis, tab.result!.columns, loaded.source.key); - const primaryKeysPresent = allPrimaryKeysPresent(primaryKeys, tab.result!.columns, metadataAnalysis, loaded.source.key); + const primaryKeysPresent = primaryKeysPresentForSource(dbType, primaryKeys, tab.result!.columns, metadataAnalysis, loaded.source.key, loaded.tableMeta.columns); const keylessAllowed = sources.length === 1 && canUseKeylessRowPredicate(dbType as DatabaseType, primaryKeys); - const primaryKeySet = new Set(primaryKeys.map((key) => key.toLowerCase())); - const editableSourceColumnCount = (sourceColumns ?? []).filter((column) => column && !primaryKeySet.has(column.toLowerCase())).length; + const primaryKeySet = new Set(primaryKeys); + const editableSourceColumnCount = (sourceColumns ?? []).filter((column) => column && !primaryKeySet.has(column)).length; return { ...loaded, analysis: metadataAnalysis, @@ -2389,7 +2415,7 @@ export const useQueryStore = defineStore("query", () => { if (loadedSources.length === 1) { const loaded = loadedSources[0]!; - const metadataAnalysis = expandStarProjectionColumnsForSource(bindUnqualifiedColumnsForSource(loaded.analysis, loaded.source, loaded.tableMeta.columns, allSourceColumns), loaded.source, loaded.tableMeta.columns); + const metadataAnalysis = expandStarProjectionColumnsForSource(bindColumnsForSource(dbType, loaded.analysis, loaded.source, loaded.tableMeta.columns, allSourceColumns), loaded.source, loaded.tableMeta.columns); const syntheticRowIdProjection = hiddenPrimaryKeys.find((projection) => projection.sourceName.toUpperCase() === DBX_ROWID_COLUMN); const primaryKeys = loaded.tableMeta.primaryKeys.length === 0 && syntheticRowIdProjection ? [DBX_ROWID_COLUMN] : loaded.tableMeta.primaryKeys; const sourceColumns = sourceColumnsForResult(metadataAnalysis, tab.result.columns, loaded.source.key); @@ -2406,7 +2432,7 @@ export const useQueryStore = defineStore("query", () => { }; } - const primaryKeysPresent = syntheticRowIdProjection ? sourceColumns?.some((column) => column?.toUpperCase() === DBX_ROWID_COLUMN) === true : allPrimaryKeysPresent(primaryKeys, tab.result.columns, metadataAnalysis, loaded.source.key); + const primaryKeysPresent = syntheticRowIdProjection ? sourceColumns?.some((column) => column?.toUpperCase() === DBX_ROWID_COLUMN) === true : primaryKeysPresentForSource(dbType, primaryKeys, tab.result.columns, metadataAnalysis, loaded.source.key, loaded.tableMeta.columns); if (!primaryKeysPresent) { return { queryAnalysis: undefined, diff --git a/crates/dbx-core/src/data_grid_sql.rs b/crates/dbx-core/src/data_grid_sql.rs index 3800e7ed5..9608c9832 100644 --- a/crates/dbx-core/src/data_grid_sql.rs +++ b/crates/dbx-core/src/data_grid_sql.rs @@ -288,8 +288,10 @@ pub fn build_data_grid_copy_update_statements(options: DataGridCopyUpdateStateme let save_columns = effective_copy_columns(options.source_columns.as_deref(), &options.columns); let column_info = options.table_meta.columns.as_deref().unwrap_or(&[]); - let primary_key_indexes: Vec> = - primary_keys.iter().map(|primary_key| find_column_index(&save_columns, primary_key)).collect(); + let primary_key_indexes: Vec> = primary_keys + .iter() + .map(|primary_key| find_column_index(options.database_type, &save_columns, primary_key)) + .collect(); if primary_key_indexes.iter().any(Option::is_none) { return Vec::new(); } @@ -828,6 +830,9 @@ fn validate_data_grid_save(options: &DataGridSaveStatementOptions) -> Option Option Option { + let primary_keys = &options.table_meta.primary_keys; + if primary_keys.is_empty() || (options.dirty_rows.is_empty() && options.deleted_rows.is_empty()) { + return None; + } + + let save_columns = effective_columns(options); + let primary_key_indexes: Vec> = primary_keys + .iter() + .map(|primary_key| find_column_index(options.database_type, &save_columns, primary_key)) + .collect(); + let missing_primary_keys = primary_keys + .iter() + .zip(&primary_key_indexes) + .filter_map(|(primary_key, index)| index.is_none().then_some(primary_key.as_str())) + .collect::>(); + if !missing_primary_keys.is_empty() { + return Some(format!( + "Cannot safely update or delete rows because the query result does not include every primary key column (missing: {}). Refresh or rerun the query before saving.", + missing_primary_keys.join(", ") + )); + } + + let primary_key_indexes = primary_key_indexes.into_iter().flatten().collect::>(); + for row_index in + options.dirty_rows.iter().map(|(row_index, _)| *row_index).chain(options.deleted_rows.iter().copied()) + { + let Some(row) = options.rows.get(row_index) else { + continue; + }; + if let Some((primary_key, _)) = + primary_keys.iter().zip(&primary_key_indexes).find(|(_, index)| row.get(**index).is_none_or(Value::is_null)) + { + return Some(format!( + "Cannot safely update or delete rows because primary key column \"{primary_key}\" has no value in the query result. Refresh or rerun the query before saving." + )); + } + } + + None +} + fn validate_oracle_keyless_lob_predicate(options: &DataGridSaveStatementOptions) -> Option { if !uses_oracle_row_id(options.database_type) || !options.table_meta.primary_keys.is_empty() @@ -946,8 +993,10 @@ fn validate_inserted_primary_keys(options: &DataGridSaveStatementOptions) -> Opt } let save_columns = effective_columns(options); - let primary_key_indexes: Vec> = - primary_keys.iter().map(|primary_key| find_column_index(&save_columns, primary_key)).collect(); + let primary_key_indexes: Vec> = primary_keys + .iter() + .map(|primary_key| find_column_index(options.database_type, &save_columns, primary_key)) + .collect(); if primary_key_indexes.iter().any(Option::is_none) { return None; } @@ -1864,12 +1913,7 @@ fn build_primary_key_where( .iter() .map(|primary_key| { let value = row - .get( - columns - .iter() - .position(|column| column.as_deref() == Some(primary_key.as_str())) - .unwrap_or(usize::MAX), - ) + .get(find_column_index(database_type, columns, primary_key).unwrap_or(usize::MAX)) .unwrap_or(&Value::Null); build_column_predicate(database_type, primary_key, value, column_info_for(column_info, primary_key), false) }) @@ -2128,11 +2172,22 @@ fn is_null_write_to_not_null_column( value.is_null() && not_null_columns.iter().any(|not_null| not_null == &normalize_column_name(column)) } -fn find_column_index(columns: &[Option], target: &str) -> Option { +fn find_column_index(database_type: Option, columns: &[Option], target: &str) -> Option { + if let Some(index) = columns.iter().position(|column| column.as_deref() == Some(target)) { + return Some(index); + } + // PostgreSQL can have distinct `id` and quoted `"ID"` columns. Only + // dialects whose result metadata is known to drift in case may fall back, + // and even then a case-only match must be unique. + if !matches!(database_type, Some(DatabaseType::Kingbase | DatabaseType::Tdengine)) { + return None; + } let normalized_target = normalize_column_name(target); - columns - .iter() - .position(|column| column.as_deref().map(normalize_column_name).unwrap_or_default() == normalized_target) + let mut matches = columns.iter().enumerate().filter_map(|(index, column)| { + (column.as_deref().map(normalize_column_name).unwrap_or_default() == normalized_target).then_some(index) + }); + let first = matches.next()?; + matches.next().is_none().then_some(first) } fn primary_key_value_key(primary_key_indexes: &[usize], row: &[Value]) -> Option { @@ -3408,6 +3463,223 @@ mod tests { assert_eq!(result.statements, vec!["UPDATE [dbo].[users] SET [UserId] = 144847503924137986 WHERE [Id] = 1;"]); } + #[test] + fn prepares_kingbase_update_when_source_primary_key_case_differs() { + let result = prepare_data_grid_save(DataGridSaveStatementOptions { + database_type: Some(DatabaseType::Kingbase), + table_meta: DataGridTableMeta { + catalog: None, + database: None, + schema: Some("ltcins_qd_db".to_string()), + table_name: "KG07".to_string(), + primary_keys: vec!["CKG023".to_string()], + columns: Some(vec![ + column("CKG023", "varchar", false, None), + column("CKG096", "character", true, None), + ]), + }, + columns: vec!["ckg023".to_string(), "CKG096".to_string()], + source_columns: Some(vec![Some("ckg023".to_string()), Some("CKG096".to_string())]), + rows: vec![vec![json!("2026071511071859"), json!("03")]], + dirty_rows: vec![(0, vec![(1, json!("02"))])], + deleted_rows: vec![], + new_rows: vec![], + }); + + assert_eq!(result.validation_error, None); + assert_eq!( + result.statements, + vec![r#"UPDATE "ltcins_qd_db"."KG07" SET "CKG096" = '02' WHERE "CKG023" = '2026071511071859';"#] + ); + assert_eq!( + result.rollback_statements, + vec![ + r#"UPDATE "ltcins_qd_db"."KG07" SET "CKG096" = '03' WHERE "CKG023" = '2026071511071859' AND "CKG096" = '02';"# + ] + ); + } + + #[test] + fn rejects_existing_row_save_when_primary_key_is_missing() { + let result = prepare_data_grid_save(DataGridSaveStatementOptions { + database_type: Some(DatabaseType::Kingbase), + table_meta: DataGridTableMeta { + catalog: None, + database: None, + schema: Some("ltcins_qd_db".to_string()), + table_name: "KG07".to_string(), + primary_keys: vec!["CKG023".to_string()], + columns: Some(vec![ + column("CKG023", "varchar", false, None), + column("CKG096", "character", true, None), + ]), + }, + columns: vec!["CKG096".to_string()], + source_columns: Some(vec![Some("CKG096".to_string())]), + rows: vec![vec![json!("03")]], + dirty_rows: vec![(0, vec![(0, json!("02"))])], + deleted_rows: vec![], + new_rows: vec![], + }); + + assert_eq!( + result.validation_error.as_deref(), + Some( + "Cannot safely update or delete rows because the query result does not include every primary key column (missing: CKG023). Refresh or rerun the query before saving." + ) + ); + assert!(result.statements.is_empty()); + assert!(result.rollback_statements.is_empty()); + } + + #[test] + fn rejects_existing_row_save_when_primary_key_value_is_null() { + let result = prepare_data_grid_save(DataGridSaveStatementOptions { + database_type: Some(DatabaseType::Kingbase), + table_meta: DataGridTableMeta { + catalog: None, + database: None, + schema: Some("ltcins_qd_db".to_string()), + table_name: "KG07".to_string(), + primary_keys: vec!["CKG023".to_string()], + columns: Some(vec![ + column("CKG023", "varchar", false, None), + column("CKG096", "character", true, None), + ]), + }, + columns: vec!["CKG023".to_string(), "CKG096".to_string()], + source_columns: None, + rows: vec![vec![Value::Null, json!("03")]], + dirty_rows: vec![(0, vec![(1, json!("02"))])], + deleted_rows: vec![], + new_rows: vec![], + }); + + assert_eq!( + result.validation_error.as_deref(), + Some( + "Cannot safely update or delete rows because primary key column \"CKG023\" has no value in the query result. Refresh or rerun the query before saving." + ) + ); + assert!(result.statements.is_empty()); + assert!(result.rollback_statements.is_empty()); + } + + #[test] + fn kingbase_column_index_prefers_exact_case_and_rejects_ambiguous_fallback() { + let columns = vec![Some("ckg023".to_string()), Some("CKG023".to_string())]; + + assert_eq!(find_column_index(Some(DatabaseType::Kingbase), &columns, "CKG023"), Some(1)); + assert_eq!(find_column_index(Some(DatabaseType::Kingbase), &columns[..1], "CKG023"), Some(0)); + assert_eq!( + find_column_index( + Some(DatabaseType::Kingbase), + &[Some("ckg023".to_string()), Some("Ckg023".to_string())], + "CKG023" + ), + None + ); + } + + #[test] + fn rejects_postgres_save_when_only_case_different_column_is_returned() { + let result = prepare_data_grid_save(DataGridSaveStatementOptions { + database_type: Some(DatabaseType::Postgres), + table_meta: DataGridTableMeta { + catalog: None, + database: None, + schema: Some("public".to_string()), + table_name: "case_keys".to_string(), + primary_keys: vec!["ID".to_string()], + columns: Some(vec![ + column("id", "integer", false, None), + column("ID", "integer", false, None), + column("name", "text", true, None), + ]), + }, + columns: vec!["id".to_string(), "name".to_string()], + source_columns: Some(vec![Some("id".to_string()), Some("name".to_string())]), + rows: vec![vec![json!(1), json!("Ada")]], + dirty_rows: vec![(0, vec![(1, json!("Grace"))])], + deleted_rows: vec![0], + new_rows: vec![], + }); + + assert_eq!( + result.validation_error.as_deref(), + Some( + "Cannot safely update or delete rows because the query result does not include every primary key column (missing: ID). Refresh or rerun the query before saving." + ) + ); + assert!(result.statements.is_empty()); + assert!(result.rollback_statements.is_empty()); + } + + #[test] + fn rejects_kingbase_save_when_case_only_primary_key_match_is_ambiguous() { + let result = prepare_data_grid_save(DataGridSaveStatementOptions { + database_type: Some(DatabaseType::Kingbase), + table_meta: DataGridTableMeta { + catalog: None, + database: None, + schema: Some("public".to_string()), + table_name: "case_keys".to_string(), + primary_keys: vec!["CKG023".to_string()], + columns: Some(vec![column("CKG023", "varchar", false, None), column("name", "varchar", true, None)]), + }, + columns: vec!["ckg023".to_string(), "Ckg023".to_string(), "name".to_string()], + source_columns: Some(vec![ + Some("ckg023".to_string()), + Some("Ckg023".to_string()), + Some("name".to_string()), + ]), + rows: vec![vec![json!("first"), json!("second"), json!("Ada")]], + dirty_rows: vec![(0, vec![(2, json!("Grace"))])], + deleted_rows: vec![], + new_rows: vec![], + }); + + assert!(result.validation_error.as_deref().is_some_and(|error| error.contains("missing: CKG023"))); + assert!(result.statements.is_empty()); + assert!(result.rollback_statements.is_empty()); + } + + #[test] + fn postgres_save_uses_exact_quoted_primary_key_for_update_delete_and_rollback() { + let result = prepare_data_grid_save(DataGridSaveStatementOptions { + database_type: Some(DatabaseType::Postgres), + table_meta: DataGridTableMeta { + catalog: None, + database: None, + schema: Some("public".to_string()), + table_name: "case_keys".to_string(), + primary_keys: vec!["ID".to_string()], + columns: Some(vec![ + column("id", "integer", false, None), + column("ID", "integer", false, None), + column("name", "text", true, None), + ]), + }, + columns: vec!["id".to_string(), "ID".to_string(), "name".to_string()], + source_columns: Some(vec![Some("id".to_string()), Some("ID".to_string()), Some("name".to_string())]), + rows: vec![vec![json!(1), json!(101), json!("Ada")], vec![json!(2), json!(202), json!("Grace")]], + dirty_rows: vec![(0, vec![(2, json!("Ada Lovelace"))])], + deleted_rows: vec![1], + new_rows: vec![], + }); + + assert_eq!(result.validation_error, None); + assert_eq!( + result.statements, + vec![ + r#"UPDATE "public"."case_keys" SET "name" = 'Ada Lovelace' WHERE "ID" = 101;"#, + r#"DELETE FROM "public"."case_keys" WHERE "ID" = 202;"#, + ] + ); + assert!(result.rollback_statements.iter().all(|statement| !statement.contains(r#"WHERE "ID" = 1 AND"#))); + assert!(result.rollback_statements.iter().any(|statement| statement.contains(r#"WHERE "ID" = 101"#))); + } + #[test] fn prepares_oracle_timestamp_insert_from_iso_grid_value() { let result = prepare_data_grid_save(DataGridSaveStatementOptions { diff --git a/crates/dbx-core/src/data_grid_tdengine_sql.rs b/crates/dbx-core/src/data_grid_tdengine_sql.rs index 966273e19..9dfb34bab 100644 --- a/crates/dbx-core/src/data_grid_tdengine_sql.rs +++ b/crates/dbx-core/src/data_grid_tdengine_sql.rs @@ -134,7 +134,9 @@ pub(super) fn validate_tdengine_existing_rows(options: &DataGridSaveStatementOpt column.as_deref().is_some_and(|column| column.eq_ignore_ascii_case(DBX_TDENGINE_TBNAME_COLUMN)) })) || primary_keys.is_empty() - || primary_keys.iter().any(|primary_key| find_column_index(&save_columns, primary_key).is_none()) + || primary_keys + .iter() + .any(|primary_key| find_column_index(options.database_type, &save_columns, primary_key).is_none()) { return Some(tdengine_row_identity_error()); } @@ -157,7 +159,7 @@ pub(super) fn validate_tdengine_existing_rows(options: &DataGridSaveStatementOpt }; if (requires_tbname && tdengine_tbname_value(&save_columns, row).is_none_or(|tbname| tbname.trim().is_empty())) || primary_keys.iter().any(|primary_key| { - find_column_index(&save_columns, primary_key) + find_column_index(options.database_type, &save_columns, primary_key) .and_then(|index| row.get(index)) .is_none_or(Value::is_null) }) diff --git a/packages/app-tests/queryStore.test.ts b/packages/app-tests/queryStore.test.ts index f934a8b1b..0cccec0ad 100644 --- a/packages/app-tests/queryStore.test.ts +++ b/packages/app-tests/queryStore.test.ts @@ -1491,6 +1491,90 @@ test("normalizes unquoted Oracle query identifiers before loading editable metad } }); +test("keeps PostgreSQL quoted primary keys distinct from case-only result columns", async () => { + const restoreStorage = installMemoryStorage(); + setActivePinia(createPinia()); + const connectionStore = useConnectionStore(); + const store = useQueryStore(); + const originalFetch = globalThis.fetch; + let executedSql = ""; + + connectionStore.addEphemeralConnection(conn("postgres-case-keys")); + + globalThis.fetch = withConnectionHealthMock(async (input, init) => { + const url = String(input); + if (url.startsWith("/api/schema/columns?")) { + return new Response( + JSON.stringify([ + { name: "id", data_type: "integer", is_nullable: false, column_default: null, is_primary_key: false, extra: null, comment: null }, + { name: "ID", data_type: "integer", is_nullable: false, column_default: null, is_primary_key: true, extra: null, comment: null }, + { name: "name", data_type: "text", is_nullable: true, column_default: null, is_primary_key: false, extra: null, comment: null }, + ]), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + } + if (url === "/api/query/prepare-pagination-plan") { + const body = JSON.parse(String(init?.body ?? "{}")); + return new Response(JSON.stringify({ sqlToExecute: body.options.sql, useAgentResultSession: false }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + if (url === "/api/query/execute-multi") { + const body = JSON.parse(String(init?.body ?? "{}")); + executedSql = body.sql; + return new Response( + JSON.stringify([ + { + columns: ["id", "name", "__DBX_PK_0"], + rows: [[1, "lower id row", 101]], + affected_rows: 0, + execution_time_ms: 1, + }, + ]), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + } + if (url === "/api/query/analyze-editability") { + const body = JSON.parse(String(init?.body ?? "{}")); + assert.match(body.sql, /"ID" AS "__DBX_PK_0"/); + return new Response( + JSON.stringify({ + editable: true, + analysis: { + schema: "public", + schemaQuoted: false, + tableName: "case_keys", + tableNameQuoted: false, + selectStar: false, + columns: [ + { sourceName: "id", sourceNameQuoted: false, resultName: "id", expression: "id" }, + { sourceName: "name", sourceNameQuoted: false, resultName: "name", expression: "name" }, + { sourceName: "ID", sourceNameQuoted: true, resultName: "__DBX_PK_0", expression: '"ID"' }, + ], + }, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + } + return new Response("unexpected request", { status: 500 }); + }); + + try { + const tabId = store.createTab("postgres-case-keys", "appdb", "Query 1", "query", "public"); + await store.executeTabSql(tabId, "select id, name from case_keys"); + + const tab = store.tabs.find((item) => item.id === tabId); + await waitFor(() => tab?.tableMeta?.tableName === "case_keys"); + assert.match(executedSql, /"ID" AS "__DBX_PK_0"/); + assert.deepEqual(tab?.querySourceColumns, ["id", "name", "ID"]); + assert.equal(tab?.queryEditabilityReason, undefined); + } finally { + globalThis.fetch = originalFetch; + restoreStorage(); + } +}); + test("binds DISTINCT qualified-star edits to the single safe joined source", async () => { const restoreStorage = installMemoryStorage(); setActivePinia(createPinia()); diff --git a/packages/app-tests/sqlAnalysis.test.ts b/packages/app-tests/sqlAnalysis.test.ts index 0bcd68aea..c8f88191a 100644 --- a/packages/app-tests/sqlAnalysis.test.ts +++ b/packages/app-tests/sqlAnalysis.test.ts @@ -1,6 +1,6 @@ import { strict as assert } from "node:assert"; import { test } from "vitest"; -import { allEditableColumnsWriteable, allPrimaryKeysPresent, analyzeEditableQuery, analyzeEditableQueryEditability, isBinaryType, queryEditabilityMessageKey, sourceColumnsForResult } from "../../apps/desktop/src/lib/sql/sqlAnalysis.ts"; +import { allEditableColumnsWriteable, allPrimaryKeysPresent, analyzeEditableQuery, analyzeEditableQueryEditability, isBinaryType, queryEditabilityMessageKey, resolveMetadataColumnName, sourceColumnsForResult } from "../../apps/desktop/src/lib/sql/sqlAnalysis.ts"; test("recognizes a simple single-table SELECT as editable", () => { const result = analyzeEditableQueryEditability("select id, name from public.users where active = true order by id"); @@ -145,6 +145,35 @@ test("accepts aliased primary key source columns for row identity", () => { assert.equal(allPrimaryKeysPresent(["id"], ["id", "name"], analyzeEditableQuery("select id, name from users")!), true); }); +test("resolves metadata columns with dialect and quote aware identifier rules", () => { + const postgresColumns = ["id", "ID", "name"]; + assert.equal(resolveMetadataColumnName("postgres", "ID", false, postgresColumns), "id"); + assert.equal(resolveMetadataColumnName("postgres", "ID", true, postgresColumns), "ID"); + assert.equal(resolveMetadataColumnName("postgres", "ID", undefined, postgresColumns), "ID"); + assert.equal(resolveMetadataColumnName("postgres", "Id", true, postgresColumns), undefined); + + assert.equal(resolveMetadataColumnName("kingbase", "ckg023", false, ["CKG023", "CKG096"]), "CKG023"); + assert.equal(resolveMetadataColumnName("kingbase", "ckg023", false, ["CKG023", "Ckg023"]), undefined); + assert.equal(resolveMetadataColumnName("kingbase", "ckg023", true, ["CKG023"]), undefined); +}); + +test("requires canonical primary key names instead of case-only matches", () => { + const lowerId = analyzeEditableQuery("select id, name from case_keys"); + const quotedId = analyzeEditableQuery('select "ID", name from case_keys'); + + assert.ok(lowerId); + assert.ok(quotedId); + assert.equal(allPrimaryKeysPresent(["ID"], ["id", "name"], lowerId), false); + assert.equal(allPrimaryKeysPresent(["ID"], ["ID", "name"], quotedId), true); +}); + +test("rejects ambiguous case-only result column mapping", () => { + const analysis = analyzeEditableQuery('select id as id, "ID" as "ID" from case_keys'); + + assert.ok(analysis); + assert.equal(sourceColumnsForResult(analysis, ["Id"]), undefined); +}); + test("maps ClickHouse simple query results when identifier columns are returned", () => { const analysis = analyzeEditableQuery("SELECT id, name, score + 1 AS next_score FROM default.people");