fix(editor): update/delete use full composite PK in WHERE clause#324
fix(editor): update/delete use full composite PK in WHERE clause#324thomaswasle wants to merge 5 commits into
Conversation
thomaswasle
force-pushed
the
bug/fix-pk-on-update-table-problem
branch
from
ef5ac7f to
8401c24
Compare
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Resolved Issues (click to expand)CRITICAL
WARNING
Files Reviewed (28 files)
Previous Review Summaries (4 snapshots, latest commit 27720e4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 27720e4)Status: No Issues Found | Recommendation: Merge Resolved Issues (click to expand)CRITICAL
WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit 464ab32)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Resolved Issues (click to expand)CRITICAL
Files Reviewed (27 files)
Fix these issues in Kilo Cloud Previous review (commit 1f7a266)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (21 files)
Fix these issues in Kilo Cloud Previous review (commit 8401c24)Status: No Issues Found | Recommendation: Merge Files Reviewed (20 files)
Reviewed by kimi-k2.6-20260420 · Input: 122.5K · Output: 14.1K · Cached: 513.8K |
thomaswasle
force-pushed
the
bug/fix-pk-on-update-table-problem
branch
from
8401c24 to
1f7a266
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (!pkColumns || pkColumns.length === 0) return []; | ||
| return pkColumns | ||
| .map((col) => columns.indexOf(col)) | ||
| .filter((idx) => idx >= 0); |
There was a problem hiding this comment.
CRITICAL: Incomplete composite PK when query result omits PK columns
pkIndexMaps filters out missing columns via .filter((idx) => idx >= 0), which can make the array shorter than pkColumns. When a table has a composite PK (e.g. ['id', 'tenant_id']) but the query only returns a subset of those columns, buildPkMap produces an incomplete map like { id: 1, tenant_id: undefined }. This incomplete map is passed to onPendingChange, stored in pendingChanges, and eventually sent to the backend for UPDATE/DELETE. The backend generates a WHERE clause with only the available columns, which can match multiple rows and cause data corruption.
Previously with single-column PKs, a missing PK column resulted in pkIndexMap === null, disabling all row editing. The composite PK implementation should preserve the same safety: if any PK column is missing from the query result columns, pkIndexMaps should be [] so that editing and deletion of existing rows is disabled.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
thomaswasle
force-pushed
the
bug/fix-pk-on-update-table-problem
branch
from
1f7a266 to
464ab32
Compare
| const entries = Object.entries(pkMap).sort(([a], [b]) => | ||
| a.localeCompare(b), | ||
| ); | ||
| if (entries.length === 1) return String(entries[0][1]); |
There was a problem hiding this comment.
WARNING: serializePkKey collision for single-column object-type PK values
For single-column PKs this returns String(entries[0][1]). If the PK value is a JSON object (e.g. from a JSON/JSONB column), String({}) produces "[object Object]", so all rows with JSON PK values would share the same pending-changes key. The same collision risk exists for other values that stringify identically (e.g. 0 and "0").
Composite PKs already use JSON.stringify which handles objects correctly. Consider using JSON.stringify unconditionally so single- and multi-column PKs behave the same way.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
thomaswasle
force-pushed
the
bug/fix-pk-on-update-table-problem
branch
from
464ab32 to
27720e4
Compare
There was a problem hiding this comment.
Thanks for taking this on,
it's a genuinely nasty bug and I'm glad someone finally chased it down. Sending only the first PK column meant composite-key tables were quietly updating or deleting every row that shared that partial value, which is about as bad as it gets for a data tool. The pk_map approach is the right one, and I appreciate that you carried it all the way through: all four Tauri commands, the trait, the three drivers, the RPC plugin path, and the whole frontend. I went looking for leftover pkCol/pkVal references and couldn't find any, so the migration is complete.
Two things in particular made me happy. Bailing out of edit mode entirely when a PK column is missing from the result set — instead of building a partial WHERE — is exactly the kind of defensive choice I'd want here. And slipping escape_identifier onto the identifiers that used to be interpolated raw is a nice bonus on top of the actual fix.
A few things I'd like sorted before we merge:
-
Tests. Right now everything in the test diff is just the
pkColumn→pkColumnsrename — the new helpers have no coverage.serializePkKeyandbuildPkMapon the TS side, andbuild_mysql_pk_where/build_pk_map_predicate/sqlite_push_pk_whereon the Rust side. I'd especially wantserializePkKeytested for key ordering and the single vs. composite cases. We ask for tests alongside new utilities, so this is the main blocker for me. -
The doc comment on
serializePkKeysays single-column PKs fall back to plainString(value)for backward compat, but the code alwaysJSON.stringifys — soid=5ends up as{"id":5}, not"5". It's harmless since these keys never leave memory, but the comment will trip up whoever reads it next, so I'd just fix the comment. -
build_mysql_pk_wherebuilds the predicate string and then all three callers throw it away (let (_, pairs)) and rebuild it by hand withpush_bind. Thejoin(" AND ")is dead work — I'd have it return just the sorted pairs. Postgres and SQLite are fine; it's only the MySQL helper that's out of step. -
Minor, take it or leave it:
RowEditorSidebarrebuildspkMapwithObject.fromEntrieson every render, soBlobInput's fetch effect re-fires each time now (it used to lean on stable primitives). AuseMemowould settle it.
The composite WHERE logic itself reads correctly to me, so really it's the tests and that comment standing between this and a merge. Nice work overall.
Cover serializePkKey (key ordering, single vs composite) and buildPkMap on the TS side, and build_mysql_pk_where, sqlite_push_pk_where, and build_pk_map_predicate on the Rust side — including empty-map rejection, alphabetical sort invariant, identifier escaping, and placeholder indexing.
The old comment claimed single-column PKs fall back to plain String(value); the function always uses JSON.stringify.
All three callers discarded the WHERE string and rebuilt it via
push/push_bind anyway, making the join(" AND ") dead work.
…tches Object.fromEntries on every render produced a new reference each time, causing BlobInput's fetch effect to re-fire. useMemo keeps the reference stable until pkColumns or rowData actually changes.
|
I tried to address your requested changes. But I am not shure why the kilo code review is failing.Can you have a look and help @debba ? |
|
Hi @thomaswasle, it looks that kilo failed because of API error, so nothing related to the code . |
2 participants
Summary
(profile_id, phone_type, key)) previously affected all rows sharing the partial key,
because only the first PK column was sent to the backend
frontend; the row identity is now a pkMap: Record<string, unknown> object containing all
PK column values
fetch_blob_as_data_url) and all three drivers (MySQL, SQLite, PostgreSQL) now receive
pk_map: HashMap<String, Value> and build a compound WHERE col1 = ? AND col2 = ? AND col3
= ? clause
Test plan