reparenting a committed split drops its KVP slots on SQL backends (`xaccSplitSetParent`)

[array_hourly_0u at icloud.com]

Hi all,

While building a Python tool on the bindings I hit what looks like an
engine + SQL-backend
bug, and I'd like to sanity-check my analysis before opening a Bugzilla & PR.

Reparenting splits rather than deep copying into new split instances
allows for more natural expressions for operations through the C &
Python Bindings. The bindings interface already exposes this
`SetParent` mechanism, but some properties of the split are dropped
for SQL backends -- XML backends are fine.

**Symptom.** If I move an already-committed split to a different
transaction with
`xaccSplitSetParent`, then destroy the old transaction and save, the
moved split *keeps*
its native fields (account, value, reconcile) but **loses its KVP
slots** — in my case the
OFX `online_id`, but it's any slot on the split (e.g. the cap-gains
`gains-source`/
`gains-split` links). This happens on the **SQL/DBI backends**
(reproduced on SQLite; the
code path is shared, so MySQL/PostgreSQL too). The **XML backend is
unaffected**. Depending
on the exact sequence the SQL save can also fail outright with
`UNIQUE constraint failed: transactions.guid` rather than silently
dropping the slot.

**Minimal reproduction (C engine API, SQL-backed book):**

```c
/* Two committed transactions T1, T2 in a SQL-backed book (sqlite3://,
mysql://, ...).
   Split D is in T2, on a credit-card account, tagged with an online_id slot
   (exactly as the OFX importer leaves it): */
gnc_import_set_split_online_id(D, "F_cc");      /* set + committed earlier */

/* move the committed split D from T2 to T1, then destroy the vacated
old txn: */
xaccSplitSetParent(D, T1);
xaccTransBeginEdit(T2);
xaccTransDestroy(T2);
xaccTransCommitEdit(T2);
qof_session_save(session, NULL);

/* reload the book and re-fetch the surviving split on the credit-card account:
     xaccSplitGetParent(split) == T1, account/value/reconcile all intact, BUT
     gnc_import_get_split_online_id(split) == NULL          -- the slot is gone.
   (The XML backend preserves it; on SQL the save can instead fail with
     "UNIQUE constraint failed: transactions.guid".)  */
```

**Root cause (from reading the source).** Two things conspire:

1. *Engine, `xaccSplitSetParent` (Split.cpp).* The split is marked dirty
   (`qof_instance_set_dirty`) **after**
`xaccTransCommitEdit(old_trans)`. So during the old
   transaction's commit, `trans_cleanup_commit` (Transaction.cpp) sees
the split as *clean*
   and skips the branch that would remove a moved-out split — even
though `split->parent`
   already points at the new transaction. The split is left dangling
in `old_trans`'s
   in-memory split list.

2. *SQL backend, `delete_splits` / `delete_split_slots_cb`
(gnc-transaction-sql.cpp).* When
   the old transaction is destroyed, the backend deletes split *rows*
keyed on `tx_guid`
   (the moved row is safe — it now carries the new `tx_guid`), but it
walks the old
   transaction's (stale) split list and deletes each split's *slots*
by the split's **own
   GUID** (`gnc_sql_slots_delete`, gnc-slots-sql.cpp), with no check
that the split still
   belongs to that transaction. So the moved split's slots are deleted
out from under it.

That explains the native-vs-KVP asymmetry: native fields live in the
`splits` row (keyed by
`tx_guid`, rewritten under the new transaction); KVP lives in the
`slots` table keyed by the
split's GUID, which the old transaction's destroy deletes. XML is
unaffected because it
re-serializes the whole in-memory book (which is correct) rather than
doing incremental,
GUID-keyed deletes.

**Why it seems to have gone unnoticed.** Reparenting a *committed*
split between two
existing transactions appears exposed only through the API bindings — every
`xaccSplitSetParent` caller I found in the GUI, importers, and scrub
code creates a fresh
split first (prior parent NULL), and the generic import matcher copies
the `online_id` onto
the existing split and destroys the imported transaction rather than
moving a split. So the
trigger is the intersection of (a script that moves a live split) and
(a SQL backend).

**Proposed fix.** The minimal, root fix is in the engine: in
`xaccSplitSetParent`, move the
`qof_instance_set_dirty(QOF_INSTANCE(s))` to *before*
`xaccTransCommitEdit(old_trans)` (right
after `s->parent = t`). Then the moved split is dirty during the old
transaction's cleanup
commit and is removed from its split list as that code already intends
— so the old
transaction's destroy never touches it. This is backend-agnostic and
fixes loss of *any* KVP
slot on a reparented split, not just `online_id`. As defense-in-depth,
`delete_split_slots_cb`
could also guard on `xaccSplitGetParent(sp) == <transaction being
destroyed>` before deleting
a split's slots. (Separately, `xaccSplitSetParent` is currently
undocumented in Split.h — happy
to add a doc comment covering the move semantics and the
single-edit-block constraint.)



Does the dirty-ordering fix look like the right place to address this?

Thanks for all your work on GnuCash.

Best,
Noah