fix(data-compare): avoid unsafe key inference
This commit is contained in:
1 parent
665e5920c9
commit
fab5c75fd2
4 files changed
+109
-6
No files matched your search
@@ -14,6 +14,7 @@ import { databaseOptionsForConnection, fetchNamespaceOptionsForConnection } from
|
|||||||
import { isSchemaAware } from "@/lib/database/databaseCapabilities";
|
import { isSchemaAware } from "@/lib/database/databaseCapabilities";
|
||||||
import { copyToClipboard } from "@/lib/common/clipboard";
|
import { copyToClipboard } from "@/lib/common/clipboard";
|
||||||
import type { DataCompareCellValue, DataCompareModifiedRow, DataCompareResult, DataCompareRow, DataCompareSyncPlan, DataCompareSyncPlanTableOptions } from "@/lib/dataGrid/dataCompare";
|
import type { DataCompareCellValue, DataCompareModifiedRow, DataCompareResult, DataCompareRow, DataCompareSyncPlan, DataCompareSyncPlanTableOptions } from "@/lib/dataGrid/dataCompare";
|
||||||
|
import { inferCompareKeyColumns } from "@/lib/dataGrid/dataCompare";
|
||||||
import type { ColumnInfo, DatabaseType } from "@/types/database";
|
import type { ColumnInfo, DatabaseType } from "@/types/database";
|
||||||
import * as api from "@/lib/backend/api";
|
import * as api from "@/lib/backend/api";
|
||||||
import { executeWithProductionSqlGuard } from "@/lib/database/productionExecutionGuard";
|
import { executeWithProductionSqlGuard } from "@/lib/database/productionExecutionGuard";
|
||||||
@@ -394,9 +395,7 @@ async function loadColumnsWithCache(cache: Map<string, CompareColumn[]>, connect
|
|||||||
async function inferKeyColumnsForTable(table: string, sourceColumnCache?: Map<string, CompareColumn[]>): Promise<string[]> {
|
async function inferKeyColumnsForTable(table: string, sourceColumnCache?: Map<string, CompareColumn[]>): Promise<string[]> {
|
||||||
if (!sourceConnectionId.value || !sourceDatabase.value || !sourceSchema.value || !table) return [];
|
if (!sourceConnectionId.value || !sourceDatabase.value || !sourceSchema.value || !table) return [];
|
||||||
const columns = sourceColumnCache ? await loadColumnsWithCache(sourceColumnCache, sourceConnectionId.value, sourceDatabase.value, sourceSchema.value, table) : (((await api.getColumns(sourceConnectionId.value, sourceDatabase.value, sourceSchema.value, table)) as CompareColumn[]) ?? []);
|
const columns = sourceColumnCache ? await loadColumnsWithCache(sourceColumnCache, sourceConnectionId.value, sourceDatabase.value, sourceSchema.value, table) : (((await api.getColumns(sourceConnectionId.value, sourceDatabase.value, sourceSchema.value, table)) as CompareColumn[]) ?? []);
|
||||||
const primaryKeys = columns.filter((column) => column.is_primary_key).map((column) => column.name);
|
return inferCompareKeyColumns(columns);
|
||||||
if (primaryKeys.length > 0) return primaryKeys;
|
|
||||||
return columns.slice(0, 1).map((column) => column.name);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
async function inferKeyColumns() {
|
async function inferKeyColumns() {
|
||||||
|
|||||||
@@ -0,0 +1,36 @@
|
|||||||
|
import { describe, expect, it } from "vitest";
|
||||||
|
import { inferCompareKeyColumns } from "../dataCompare";
|
||||||
|
|
||||||
|
describe("inferCompareKeyColumns", () => {
|
||||||
|
it("returns all primary-key columns when a primary key exists", () => {
|
||||||
|
expect(
|
||||||
|
inferCompareKeyColumns([
|
||||||
|
{ name: "id", is_primary_key: true },
|
||||||
|
{ name: "name", is_primary_key: false },
|
||||||
|
]),
|
||||||
|
).toEqual(["id"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns a composite primary key as-is", () => {
|
||||||
|
expect(
|
||||||
|
inferCompareKeyColumns([
|
||||||
|
{ name: "tenant_id", is_primary_key: true },
|
||||||
|
{ name: "user_id", is_primary_key: true },
|
||||||
|
{ name: "name", is_primary_key: false },
|
||||||
|
]),
|
||||||
|
).toEqual(["tenant_id", "user_id"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns an empty array instead of falling back to the first column when there is no primary key", () => {
|
||||||
|
expect(
|
||||||
|
inferCompareKeyColumns([
|
||||||
|
{ name: "category", is_primary_key: false },
|
||||||
|
{ name: "name", is_primary_key: false },
|
||||||
|
]),
|
||||||
|
).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns an empty array for an empty column list", () => {
|
||||||
|
expect(inferCompareKeyColumns([])).toEqual([]);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -118,3 +118,24 @@ export interface DataCompareSyncPlan {
|
|||||||
syncStatements: string[];
|
syncStatements: string[];
|
||||||
syncSql: string;
|
syncSql: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Safely infers the columns to use as comparison keys for a table.
|
||||||
|
*
|
||||||
|
* Only primary-key columns are trusted: a primary key is unique by
|
||||||
|
* definition, so comparing on it can never produce duplicate-key failures.
|
||||||
|
* Composite primary keys are returned as-is.
|
||||||
|
*
|
||||||
|
* When no primary key exists an empty array is returned so the caller can ask
|
||||||
|
* the user to pick matching columns explicitly. Falling back to the first
|
||||||
|
* column would silently select an arbitrary, possibly non-unique field
|
||||||
|
* (category, status, date, ...) and make the whole table comparison fail with
|
||||||
|
* an unhelpful duplicate-key error.
|
||||||
|
*/
|
||||||
|
export function inferCompareKeyColumns(columns: Pick<ColumnInfo, "name" | "is_primary_key">[]): string[] {
|
||||||
|
const primaryKeys: string[] = [];
|
||||||
|
for (const column of columns) {
|
||||||
|
if (column.is_primary_key) primaryKeys.push(column.name);
|
||||||
|
}
|
||||||
|
return primaryKeys;
|
||||||
|
}
|
||||||
@@ -735,7 +735,7 @@ fn collect_compare_rows(
|
|||||||
for row in rows {
|
for row in rows {
|
||||||
let key = key_for_row(&row, key_columns, column_indexes);
|
let key = key_for_row(&row, key_columns, column_indexes);
|
||||||
if items.contains_key(&key) {
|
if items.contains_key(&key) {
|
||||||
return Err(format!("Duplicate {label} key: {key}"));
|
return Err(duplicate_key_error(label, key_columns, &key));
|
||||||
}
|
}
|
||||||
order.push(key.clone());
|
order.push(key.clone());
|
||||||
items.insert(key, normalize_row_len(row, columns.len()));
|
items.insert(key, normalize_row_len(row, columns.len()));
|
||||||
@@ -744,6 +744,22 @@ fn collect_compare_rows(
|
|||||||
Ok((items, order))
|
Ok((items, order))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Formats a duplicate-key error so the user can tell which side (source or
|
||||||
|
/// target) failed, which columns make up the key and which key value repeated.
|
||||||
|
///
|
||||||
|
/// Single-column keys render as `Duplicate source key for column(s) [id]: "1"`;
|
||||||
|
/// composite keys render as `Duplicate target key for column(s) [a, b]: ["1", "2"]`.
|
||||||
|
fn duplicate_key_error(label: &str, key_columns: &[String], key: &str) -> String {
|
||||||
|
let columns = key_columns.join(", ");
|
||||||
|
let key_display = if key_columns.len() > 1 {
|
||||||
|
let values = key.split('\u{001f}').collect::<Vec<_>>();
|
||||||
|
format!("[{}]", values.join(", "))
|
||||||
|
} else {
|
||||||
|
key.to_string()
|
||||||
|
};
|
||||||
|
format!("Duplicate {label} key for column(s) [{columns}]: {key_display}")
|
||||||
|
}
|
||||||
|
|
||||||
#[derive(Debug, Clone, Copy)]
|
#[derive(Debug, Clone, Copy)]
|
||||||
struct GenerateDataSyncSqlOptions<'a> {
|
struct GenerateDataSyncSqlOptions<'a> {
|
||||||
table_name: &'a str,
|
table_name: &'a str,
|
||||||
@@ -2283,7 +2299,7 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn rejects_duplicate_row_keys() {
|
fn rejects_duplicate_row_keys_with_key_column_context() {
|
||||||
let err = compare_data_rows(CompareDataRowsOptions {
|
let err = compare_data_rows(CompareDataRowsOptions {
|
||||||
columns: vec!["id".to_string(), "name".to_string()],
|
columns: vec!["id".to_string(), "name".to_string()],
|
||||||
key_columns: vec!["id".to_string()],
|
key_columns: vec!["id".to_string()],
|
||||||
@@ -2292,7 +2308,38 @@ mod tests {
|
|||||||
})
|
})
|
||||||
.expect_err("duplicate source keys should fail");
|
.expect_err("duplicate source keys should fail");
|
||||||
|
|
||||||
assert!(err.contains("Duplicate source key"));
|
assert!(err.contains("Duplicate source key for column(s) [id]: 1"), "{err}");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rejects_duplicate_target_keys_with_key_column_context() {
|
||||||
|
let err = compare_data_rows(CompareDataRowsOptions {
|
||||||
|
columns: vec!["id".to_string(), "name".to_string()],
|
||||||
|
key_columns: vec!["id".to_string()],
|
||||||
|
source_rows: vec![vec![json!(1), json!("Ada")]],
|
||||||
|
target_rows: vec![vec![json!(1), json!("Ada")], vec![json!(1), json!("Ada Clone")]],
|
||||||
|
})
|
||||||
|
.expect_err("duplicate target keys should fail");
|
||||||
|
|
||||||
|
assert!(err.contains("Duplicate target key for column(s) [id]: 1"), "{err}");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rejects_duplicate_composite_keys_with_both_columns_named() {
|
||||||
|
let err = compare_data_rows(CompareDataRowsOptions {
|
||||||
|
columns: vec!["tenant_id".to_string(), "user_id".to_string(), "name".to_string()],
|
||||||
|
key_columns: vec!["tenant_id".to_string(), "user_id".to_string()],
|
||||||
|
source_rows: vec![
|
||||||
|
vec![json!("A"), json!(1001), json!("Ada")],
|
||||||
|
vec![json!("A"), json!(1001), json!("Ada Clone")],
|
||||||
|
],
|
||||||
|
target_rows: vec![vec![json!("A"), json!(1001), json!("Ada")]],
|
||||||
|
})
|
||||||
|
.expect_err("duplicate composite source keys should fail");
|
||||||
|
|
||||||
|
assert!(err.contains("Duplicate source key"), "{err}");
|
||||||
|
assert!(err.contains("[tenant_id, user_id]"), "{err}");
|
||||||
|
assert!(err.contains("[\"A\", 1001]"), "{err}");
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in new issue
Block a user